Date: Tue, 27 Feb 2024 04:00:33 +0800
From: Ethan Zhao <haifeng.zhao@...ux.intel.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: baolu.lu@...ux.intel.com, bhelgaas@...gle.com, robin.murphy@....com,
 jgg@...pe.ca, kevin.tian@...el.com, dwmw2@...radead.org, will@...nel.org,
 lukas@...ner.de, yi.l.liu@...el.com, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target
 device isn't valid


On 2/23/2024 4:19 PM, Dan Carpenter wrote:
> On Fri, Feb 23, 2024 at 03:32:52PM +0800, Ethan Zhao wrote:
>> On 2/23/2024 2:08 PM, Dan Carpenter wrote:
>>> On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
>>>>>> @@ -1326,6 +1336,21 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>>>>>>     			head = (head - 2 + QI_LENGTH) % QI_LENGTH;
>>>>>>     		} while (head != tail);
>>>>>> +		/*
>>>>>> +		 * If got ITE, we need to check if the sid of ITE is one of the
>>>>>> +		 * current valid ATS invalidation target devices, if no, or the
>>>>>> +		 * target device isn't presnet, don't try this request anymore.
>>>>>> +		 * 0 value of ite_sid means old VT-d device, no ite_sid value.
>>>>>> +		 */
>>>>> This comment is kind of confusing.
>>>> Really confusing ? this is typo there, resnet-> "present"
>>>>
>>> Reading this comment again, the part about zero ite_sid values is
>>> actually useful, but what does "old" mean in "old VT-d device".  How old
>>> is it?  One year old?
>> I recite the description from Intel VT-d spec here
>>
>> "A value of 0 in this field indicates that this is an older version of DMA
>> remapping hardware which does not provide additional details about
>> the Invalidation Time-out Error"
>>
> This is good.  Put that in the comment.  Otherwise it's not clear.  I
> assumed "old" meant released or something.
>
>
>> At least, the Intel VT-d spec 4.0 released 2022 June says the same thing.
>> as to how old, I didn't find docs older than that, really out of my radar.
>>
>>>>> /*
>>>>>     * If we have an ITE, then we need to check whether the sid of the ITE
>>>>>     * is in the rbtree (meaning it is probed and not released), and that
>>>>>     * the PCI device is present.
>>>>>     */
>>>>>
>>>>> My comment is slightly shorter but I think it has the necessary
>>>>> information.
>>>>>
>>>>>> +		if (ite_sid) {
>>>>>> +			dev = device_rbtree_find(iommu, ite_sid);
>>>>>> +			if (!dev || !dev_is_pci(dev))
>>>>>> +				return -ETIMEDOUT;
>>>>> -ETIMEDOUT is weird.  The callers don't care which error code we return.
>>>>> Change this to -ENODEV or something
>>>> -ETIMEDOUT means prior ATS invalidation request hit timeout fault, and the
>>>> caller really cares about the returned value.
>>>>
>>> I don't really care about the return value and if you say it should be
>>> -ETIMEDOUT, then you're the expert.  However, I don't see anything in
>>> linux-next which cares about the return values except -EAGAIN.
>>> This function is only called from qi_submit_sync() which checks for
>>> -EAGAIN.  Then I did a git grep.
>>>
>>> $ git grep qi_submit_sync
>>> drivers/iommu/intel/dmar.c:int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/dmar.c:     qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/iommu.h:int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
>>> drivers/iommu/intel/iommu.h: * Options used in qi_submit_sync:
>>> drivers/iommu/intel/irq_remapping.c:    return qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/pasid.c:    qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/svm.c:      qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
>>> drivers/iommu/intel/svm.c:      qi_submit_sync(iommu, &desc, 1, 0);
>>> drivers/iommu/intel/svm.c:              qi_submit_sync(iommu, &desc, 1, 0);
>>>
>>> Only qi_flush_iec() in irq_remapping.c cares about the return.  Then I
>>> traced those callers back and nothing cares about -ETIMEOUT.
>>>
>>> Are you refering to patches that haven't ben merged yet?
>> Yes, patches under working, not the code running on your boxes.
>>
>> -ETIMEOUT & -ENODEV, they both describe the error that is happenning, someone
>> prefers -ETIMEOUT, they would like to know the request was timeout, and someone
>> perfers -ENODEV, they know the target device is gone, ever existed.
> Okay.  I obviously can't comment on patches that I haven't seen but,
> sure, it sounds reasonable.
>
>>>>>> +			pdev = to_pci_dev(dev);
>>>>>> +			if (!pci_device_is_present(pdev) &&
>>>>>> +				ite_sid == pci_dev_id(pci_physfn(pdev)))
>>>>> The && confused me, but then I realized that probably "ite_sid ==
>>>>> pci_dev_id(pci_physfn(pdev))" is always true.  Can we delete that part?
>>>> Here is the fault handling, just double confirm nothing else goes wrong --
>>>> beyond the assumption.
>>>>
>>> Basically for that to ever be != it would need some kind of memory
>>> corruption?  I feel like in that situation, the more conservative thing
>>> is to give up.  If the PCI device is not present then just give up.
>> memory corruption, buggy BIOS tables, faked request ...something out
>> of imagination, after confirmed the device is what it claimed to be, if
>> not present, then give up to retry the request.
> This is not correct.  We looked up the device based on the ite_sid so
> we know what the device id is, unless we experience catastrophic memory
> corruption.
>
> +                       dev = device_rbtree_find(iommu, ite_sid);
>                                                          ^^^^^^^
> We looked it up here.
>
> +                       if (!dev || !dev_is_pci(dev))
> +                               return -ETIMEDOUT;
> +                       pdev = to_pci_dev(dev);
> +                       if (!pci_device_is_present(pdev) &&
> +                               ite_sid == pci_dev_id(pci_physfn(pdev)))
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Unless the device_rbtree_find() is returning garbage then these things
> must be true.
>
> +                               return -ETIMEDOUT;
>
> I tried to double check how we were storing devices into the rbtree,
> but then I discovered that the device_rbtree_find() doesn't exist in
> linux-next and this patch breaks the build.
>
> This is very frustrating thing.  But let's say a buggy BIOS could mess
> up the rbtree.  In that situation, we would still want to change the &&
> to an ||.  If the divice is not present and^W or the rbtree is corrupted

Maybe you meant
+                       if (!pci_device_is_present(pdev) ||
+                               ite_sid != pci_dev_id(pci_physfn(pdev)))

Unfortunately, the ite_sid we got from the "Invalidation Queue Error Record Register" is the *PCI Requester-id* of faulty device, that could be different
BDF as the sid in the ATS invalidation request for devices:

1. behind the PCIe to PCI bridges.
2. behindConventional PCI Bridges  
3.PCI Express* Devices Using Phantom Functions  
4.Intel® Scalable I/O Virtualization Capable Devices  (e.g. ADI)
5. devices with ARI function.
6. behind root port without ACS enabled.
.. ...


Thanks,
Ethan

> then return an error.  But don't do this.  If the memory is corrupted we
> are already screwed and there is no way the system can really recover
> in any reasonable way.
>
> regards,
> dan carpenter
>
>

Powered by blists - more mailing lists