I'm sorry, I'm coming into this late and this is the first time I have
reviewed this patch. I see that we are at v13, and I hate to come in
with picky comments when a patch has already gone through 13
revisions...
On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
> Because surprise removal could happen anytime, e.g. user could request safe
> removal to EP(endpoint device) via sysfs and brings its link down to do
> surprise removal cocurrently. such aggressive cases would cause ATS
> invalidation request issued to non-existence target device, then deadly
> loop to retry that request after ITE fault triggered in interrupt context.
> this patch aims to optimize the ITE handling by checking the target device
> presence state to avoid retrying the timeout request blindly, thus avoid
> hard lockup or system hang.
>
> Devices are valid ATS invalidation request target only when they reside
"valid invalidation" is awkward wording. Can we instead say:
Devices should only be invalidated when they are in the
iommu->device_rbtree (probed, not released) and present.
> in the iommu->device_rbtre (probed, not released) and present.
^
Missing e in _rbtree.
>
> Signed-off-by: Ethan Zhao <[email protected]>
This patch should have a Fixes tags and be backported to stable kernels.
I think it goes back all the way...
Fixes: 704126ad81b8 ("VT-d: handle Invalidation Queue Error to avoid system hang")
> ---
> drivers/iommu/intel/dmar.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index d14797aabb7a..d01d68205557 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> {
> u32 fault;
> int head, tail;
> + u64 iqe_err, ite_sid;
> + struct device *dev = NULL;
> + struct pci_dev *pdev = NULL;
> struct q_inval *qi = iommu->qi;
> int shift = qi_shift(iommu);
>
> @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> tail = readl(iommu->reg + DMAR_IQT_REG);
> tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>
> + /*
> + * SID field is valid only when the ITE field is Set in FSTS_REG
> + * see Intel VT-d spec r4.1, section 11.4.9.9
> + */
> + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
> + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
> +
> writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
> pr_info("Invalidation Time-out Error (ITE) cleared\n");
>
> @@ -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.
/*
* 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
> + 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?
pdev = to_pci_dev(dev);
if (!pci_device_is_present(pdev))
return -ENODEV;
> + return -ETIMEDOUT;
-ENODEV.
> + }
> if (qi->desc_status[wait_index] == QI_ABORT)
> return -EAGAIN;
> }
Sorry, again for nit picking a v13 patch. I'm not a domain expert but
this patchset seems reasonable to me.
regards,
dan carpenter
On 2/22/2024 7:24 PM, Dan Carpenter wrote:
> I'm sorry, I'm coming into this late and this is the first time I have
> reviewed this patch. I see that we are at v13, and I hate to come in
> with picky comments when a patch has already gone through 13
> revisions...
Never mind that. some are totally new.
> On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
>> Because surprise removal could happen anytime, e.g. user could request safe
>> removal to EP(endpoint device) via sysfs and brings its link down to do
>> surprise removal cocurrently. such aggressive cases would cause ATS
>> invalidation request issued to non-existence target device, then deadly
>> loop to retry that request after ITE fault triggered in interrupt context.
>> this patch aims to optimize the ITE handling by checking the target device
>> presence state to avoid retrying the timeout request blindly, thus avoid
>> hard lockup or system hang.
>>
>> Devices are valid ATS invalidation request target only when they reside
> "valid invalidation" is awkward wording. Can we instead say:
If you read them together, sounds like tongue twister. but here "ATS
invalidation request target" is one term in PCIe spec.
>
> Devices should only be invalidated when they are in the
> iommu->device_rbtree (probed, not released) and present.
>
>> in the iommu->device_rbtre (probed, not released) and present.
> ^
> Missing e in _rbtree.
Yup.
>> Signed-off-by: Ethan Zhao <[email protected]>
> This patch should have a Fixes tags and be backported to stable kernels.
> I think it goes back all the way...
>
> Fixes: 704126ad81b8 ("VT-d: handle Invalidation Queue Error to avoid system hang")
Sounds reasonable.
>
>> ---
>> drivers/iommu/intel/dmar.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
>> index d14797aabb7a..d01d68205557 100644
>> --- a/drivers/iommu/intel/dmar.c
>> +++ b/drivers/iommu/intel/dmar.c
>> @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>> {
>> u32 fault;
>> int head, tail;
>> + u64 iqe_err, ite_sid;
>> + struct device *dev = NULL;
>> + struct pci_dev *pdev = NULL;
>> struct q_inval *qi = iommu->qi;
>> int shift = qi_shift(iommu);
>>
>> @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>> tail = readl(iommu->reg + DMAR_IQT_REG);
>> tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>>
>> + /*
>> + * SID field is valid only when the ITE field is Set in FSTS_REG
>> + * see Intel VT-d spec r4.1, section 11.4.9.9
>> + */
>> + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
>> + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
>> +
>> writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
>> pr_info("Invalidation Time-out Error (ITE) cleared\n");
>>
>> @@ -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"
>
> /*
> * 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.
>
>> + 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.
>
> pdev = to_pci_dev(dev);
> if (!pci_device_is_present(pdev))
> return -ENODEV;
>
>
>> + return -ETIMEDOUT;
> -ENODEV.
The ATS invalidation request could be sent from userland in later code,
the userland code will care about the returned value, -ENODEV is one aspect
of the fact (target device not present), while -ETIMEDOUT is another
(timeout happened). we couldn't return them both.
>
>> + }
>> if (qi->desc_status[wait_index] == QI_ABORT)
>> return -EAGAIN;
>> }
> Sorry, again for nit picking a v13 patch. I'm not a domain expert but
> this patchset seems reasonable to me.
Though this is the v13, it is based on new rbtree code, you are welcome.
Thanks,
Ethan
> regards,
> dan carpenter
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?
> >
> > /*
> > * 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?
> >
> > > + 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.
regards,
dan carpenter
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"
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.
>>>> + 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.
Thanks,
Ethan
>
> regards,
> dan carpenter
>
On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
> On 2/22/2024 7:24 PM, Dan Carpenter wrote:
> > On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
> > > Because surprise removal could happen anytime, e.g. user could request safe
> > > removal to EP(endpoint device) via sysfs and brings its link down to do
> > > surprise removal cocurrently. such aggressive cases would cause ATS
> > > invalidation request issued to non-existence target device, then deadly
> > > loop to retry that request after ITE fault triggered in interrupt context.
> > > this patch aims to optimize the ITE handling by checking the target device
> > > presence state to avoid retrying the timeout request blindly, thus avoid
> > > hard lockup or system hang.
> > >
> > > Devices are valid ATS invalidation request target only when they reside
> > > in the iommu->device_rbtree (probed, not released) and present.
> >
> > "valid invalidation" is awkward wording. Can we instead say:
>
> If you read them together, sounds like tongue twister. but here "ATS
> invalidation request target" is one term in PCIe spec.
"ATS invalidation request target" does not appear in the PCIe spec. I
think you're trying to avoid sending ATS Invalidate Requests when you
know they will not be completed.
It is impossible to reliably determine whether a device will be
present and able to complete an Invalidate Request. No matter what
you check to determine that a device is present *now*, it may be
removed before an Invalidate Request reaches it.
If an Invalidate Request to a non-existent device causes a "deadly
loop" (I'm not sure what that means) or a hard lockup or a system
hang, something is wrong with the hardware. There should be a
mechanism to recover from a timeout in that situation.
You can avoid sending Invalidate Requests to devices that have been
removed, and that will reduce the number of timeout cases. But if you
rely on a check like pci_device_is_present() or
pci_dev_is_disconnected(), there is *always* an unavoidable race
between a device removal and the Invalidate Request.
> > > @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> > > {
> > > u32 fault;
> > > int head, tail;
> > > + u64 iqe_err, ite_sid;
> > > + struct device *dev = NULL;
> > > + struct pci_dev *pdev = NULL;
> > > struct q_inval *qi = iommu->qi;
> > > int shift = qi_shift(iommu);
> > > @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> > > tail = readl(iommu->reg + DMAR_IQT_REG);
> > > tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
> > > + /*
> > > + * SID field is valid only when the ITE field is Set in FSTS_REG
> > > + * see Intel VT-d spec r4.1, section 11.4.9.9
> > > + */
> > > + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
> > > + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
> > > +
> > > writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
> > > pr_info("Invalidation Time-out Error (ITE) cleared\n");
> > > @@ -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"
>
> >
> > /*
> > * 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.
>
> >
> > > + 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.
>
> >
> > pdev = to_pci_dev(dev);
> > if (!pci_device_is_present(pdev))
> > return -ENODEV;
> >
> >
> > > + return -ETIMEDOUT;
> > -ENODEV.
>
> The ATS invalidation request could be sent from userland in later code,
> the userland code will care about the returned value, -ENODEV is one aspect
> of the fact (target device not present), while -ETIMEDOUT is another
> (timeout happened). we couldn't return them both.
>
> >
> > > + }
> > > if (qi->desc_status[wait_index] == QI_ABORT)
> > > return -EAGAIN;
> > > }
> > Sorry, again for nit picking a v13 patch. I'm not a domain expert but
> > this patchset seems reasonable to me.
>
> Though this is the v13, it is based on new rbtree code, you are welcome.
>
> Thanks,
> Ethan
>
> > regards,
> > dan carpenter
On 2/27/2024 6:52 AM, Bjorn Helgaas wrote:
> On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
>> On 2/22/2024 7:24 PM, Dan Carpenter wrote:
>>> On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
>>>> Because surprise removal could happen anytime, e.g. user could request safe
>>>> removal to EP(endpoint device) via sysfs and brings its link down to do
>>>> surprise removal cocurrently. such aggressive cases would cause ATS
>>>> invalidation request issued to non-existence target device, then deadly
>>>> loop to retry that request after ITE fault triggered in interrupt context.
>>>> this patch aims to optimize the ITE handling by checking the target device
>>>> presence state to avoid retrying the timeout request blindly, thus avoid
>>>> hard lockup or system hang.
>>>>
>>>> Devices are valid ATS invalidation request target only when they reside
>>>> in the iommu->device_rbtree (probed, not released) and present.
>>> "valid invalidation" is awkward wording. Can we instead say:
>> If you read them together, sounds like tongue twister. but here "ATS
>> invalidation request target" is one term in PCIe spec.
> "ATS invalidation request target" does not appear in the PCIe spec. I
> think you're trying to avoid sending ATS Invalidate Requests when you
> know they will not be completed.
I meant "ATS Invalidation Request" here is one term in PCIe spec, 'valid'
is used to describe the word 'target'.
This patch isn't intended to work as the same logic as patch [2/3], this
aims to break the blindly dead loop not to retry the timeout request after
ITE fault happened.
>
> It is impossible to reliably determine whether a device will be
> present and able to complete an Invalidate Request. No matter what
> you check to determine that a device is present *now*, it may be
> removed before an Invalidate Request reaches it.
Here we check to see if the ITE fault was caused by device is not present.
The opposite logic, not predict the future, but find the cause of the fault
already happened, if pci_device_is_present() tells us the device isn't
there, it is reliable I think.
>
> If an Invalidate Request to a non-existent device causes a "deadly
> loop" (I'm not sure what that means) or a hard lockup or a system
There is a dead loop here to blindly retry to timeout request if
ITE happened, we want to break that loop if the target device was
gone.
> hang, something is wrong with the hardware. There should be a
> mechanism to recover from a timeout in that situation.
>
> You can avoid sending Invalidate Requests to devices that have been
That logic works for simple safe /surprise removal as described in
patch[2/3], no race there that case at all.
> removed, and that will reduce the number of timeout cases. But if you
> rely on a check like pci_device_is_present() or
> pci_dev_is_disconnected(), there is *always* an unavoidable race
We are not relying on pci_device_is_present() here in this patch to close
the race window between aggressive surprise removal and ATS invalidation
Request, we are doing post-fault handling here.
Thanks,
Ethan
> between a device removal and the Invalidate Request.
>
>>>> @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>>>> {
>>>> u32 fault;
>>>> int head, tail;
>>>> + u64 iqe_err, ite_sid;
>>>> + struct device *dev = NULL;
>>>> + struct pci_dev *pdev = NULL;
>>>> struct q_inval *qi = iommu->qi;
>>>> int shift = qi_shift(iommu);
>>>> @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>>>> tail = readl(iommu->reg + DMAR_IQT_REG);
>>>> tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>>>> + /*
>>>> + * SID field is valid only when the ITE field is Set in FSTS_REG
>>>> + * see Intel VT-d spec r4.1, section 11.4.9.9
>>>> + */
>>>> + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
>>>> + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
>>>> +
>>>> writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
>>>> pr_info("Invalidation Time-out Error (ITE) cleared\n");
>>>> @@ -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"
>>
>>> /*
>>> * 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.
>>
>>>> + 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.
>>
>>> pdev = to_pci_dev(dev);
>>> if (!pci_device_is_present(pdev))
>>> return -ENODEV;
>>>
>>>
>>>> + return -ETIMEDOUT;
>>> -ENODEV.
>> The ATS invalidation request could be sent from userland in later code,
>> the userland code will care about the returned value, -ENODEV is one aspect
>> of the fact (target device not present), while -ETIMEDOUT is another
>> (timeout happened). we couldn't return them both.
>>
>>>> + }
>>>> if (qi->desc_status[wait_index] == QI_ABORT)
>>>> return -EAGAIN;
>>>> }
>>> Sorry, again for nit picking a v13 patch. I'm not a domain expert but
>>> this patchset seems reasonable to me.
>> Though this is the v13, it is based on new rbtree code, you are welcome.
>>
>> Thanks,
>> Ethan
>>
>>> regards,
>>> dan carpenter
On Tue, Feb 27, 2024 at 10:30:36AM +0800, Ethan Zhao wrote:
> On 2/27/2024 6:52 AM, Bjorn Helgaas wrote:
> > On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
> > > On 2/22/2024 7:24 PM, Dan Carpenter wrote:
> > > > On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
> > > > > Because surprise removal could happen anytime, e.g. user could request safe
> > > > > removal to EP(endpoint device) via sysfs and brings its link down to do
> > > > > surprise removal cocurrently. such aggressive cases would cause ATS
> > > > > invalidation request issued to non-existence target device, then deadly
> > > > > loop to retry that request after ITE fault triggered in interrupt context.
> > > > > this patch aims to optimize the ITE handling by checking the target device
> > > > > presence state to avoid retrying the timeout request blindly, thus avoid
> > > > > hard lockup or system hang.
> > > > >
> > > > > Devices are valid ATS invalidation request target only when they reside
> > > > > in the iommu->device_rbtree (probed, not released) and present.
> > > > "valid invalidation" is awkward wording. Can we instead say:
> > > If you read them together, sounds like tongue twister. but here "ATS
> > > invalidation request target" is one term in PCIe spec.
> > "ATS invalidation request target" does not appear in the PCIe spec. I
> > think you're trying to avoid sending ATS Invalidate Requests when you
> > know they will not be completed.
>
> I meant "ATS Invalidation Request" here is one term in PCIe spec, 'valid'
> is used to describe the word 'target'.
>
> This patch isn't intended to work as the same logic as patch [2/3], this
> aims to break the blindly dead loop not to retry the timeout request after
> ITE fault happened.
>
> >
> > It is impossible to reliably determine whether a device will be
> > present and able to complete an Invalidate Request. No matter what
> > you check to determine that a device is present *now*, it may be
> > removed before an Invalidate Request reaches it.
>
> Here we check to see if the ITE fault was caused by device is not present.
> The opposite logic, not predict the future, but find the cause of the fault
> already happened, if pci_device_is_present() tells us the device isn't
> there, it is reliable I think.
>
> >
> > If an Invalidate Request to a non-existent device causes a "deadly
> > loop" (I'm not sure what that means) or a hard lockup or a system
>
> There is a dead loop here to blindly retry to timeout request if
> ITE happened, we want to break that loop if the target device was
> gone.
>
> > hang, something is wrong with the hardware. There should be a
> > mechanism to recover from a timeout in that situation.
> >
> > You can avoid sending Invalidate Requests to devices that have been
>
> That logic works for simple safe /surprise removal as described in
> patch[2/3], no race there that case at all.
>
> > removed, and that will reduce the number of timeout cases. But if you
> > rely on a check like pci_device_is_present() or
> > pci_dev_is_disconnected(), there is *always* an unavoidable race
>
> We are not relying on pci_device_is_present() here in this patch to close
> the race window between aggressive surprise removal and ATS invalidation
> Request, we are doing post-fault handling here.
OK, sorry, I guess I missed that this fixes the code that handles the
Completion Timeouts.
> > between a device removal and the Invalidate Request.
> >
> > > > > @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> > > > > {
> > > > > u32 fault;
> > > > > int head, tail;
> > > > > + u64 iqe_err, ite_sid;
> > > > > + struct device *dev = NULL;
> > > > > + struct pci_dev *pdev = NULL;
> > > > > struct q_inval *qi = iommu->qi;
> > > > > int shift = qi_shift(iommu);
> > > > > @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> > > > > tail = readl(iommu->reg + DMAR_IQT_REG);
> > > > > tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
> > > > > + /*
> > > > > + * SID field is valid only when the ITE field is Set in FSTS_REG
> > > > > + * see Intel VT-d spec r4.1, section 11.4.9.9
> > > > > + */
> > > > > + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
> > > > > + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
> > > > > +
> > > > > writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
> > > > > pr_info("Invalidation Time-out Error (ITE) cleared\n");
> > > > > @@ -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"
> > >
> > > > /*
> > > > * 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.
> > >
> > > > > + 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.
> > >
> > > > pdev = to_pci_dev(dev);
> > > > if (!pci_device_is_present(pdev))
> > > > return -ENODEV;
> > > >
> > > >
> > > > > + return -ETIMEDOUT;
> > > > -ENODEV.
> > > The ATS invalidation request could be sent from userland in later code,
> > > the userland code will care about the returned value, -ENODEV is one aspect
> > > of the fact (target device not present), while -ETIMEDOUT is another
> > > (timeout happened). we couldn't return them both.
> > >
> > > > > + }
> > > > > if (qi->desc_status[wait_index] == QI_ABORT)
> > > > > return -EAGAIN;
> > > > > }
> > > > Sorry, again for nit picking a v13 patch. I'm not a domain expert but
> > > > this patchset seems reasonable to me.
> > > Though this is the v13, it is based on new rbtree code, you are welcome.
> > >
> > > Thanks,
> > > Ethan
> > >
> > > > regards,
> > > > dan carpenter
On 2/27/2024 6:52 AM, Bjorn Helgaas wrote:
> On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
>> On 2/22/2024 7:24 PM, Dan Carpenter wrote:
>>> On Thu, Feb 22, 2024 at 04:02:51AM -0500, Ethan Zhao wrote:
>>>> Because surprise removal could happen anytime, e.g. user could request safe
>>>> removal to EP(endpoint device) via sysfs and brings its link down to do
>>>> surprise removal cocurrently. such aggressive cases would cause ATS
>>>> invalidation request issued to non-existence target device, then deadly
>>>> loop to retry that request after ITE fault triggered in interrupt context.
>>>> this patch aims to optimize the ITE handling by checking the target device
>>>> presence state to avoid retrying the timeout request blindly, thus avoid
>>>> hard lockup or system hang.
>>>>
>>>> Devices are valid ATS invalidation request target only when they reside
>>>> in the iommu->device_rbtree (probed, not released) and present.
>>> "valid invalidation" is awkward wording. Can we instead say:
>> If you read them together, sounds like tongue twister. but here "ATS
>> invalidation request target" is one term in PCIe spec.
> "ATS invalidation request target" does not appear in the PCIe spec. I
> think you're trying to avoid sending ATS Invalidate Requests when you
> know they will not be completed.
>
> It is impossible to reliably determine whether a device will be
> present and able to complete an Invalidate Request. No matter what
> you check to determine that a device is present *now*, it may be
> removed before an Invalidate Request reaches it.
>
> If an Invalidate Request to a non-existent device causes a "deadly
> loop" (I'm not sure what that means) or a hard lockup or a system
> hang, something is wrong with the hardware. There should be a
The hardware might be innocent, here in the qi_submit_sync() &
qi_check_fault() will retry the timeout request forever if the
target device is gone or the target device always takes too much
time to reponse. there is dead loop here.
This patch aims to break the dead loop for case device is not
present anymore.
But for those devices takes too much time to complete. I am
working on other patches, not in this patchset.
Thanks,
Ethan
> mechanism to recover from a timeout in that situation.
>
> You can avoid sending Invalidate Requests to devices that have been
> removed, and that will reduce the number of timeout cases. But if you
> rely on a check like pci_device_is_present() or
> pci_dev_is_disconnected(), there is *always* an unavoidable race
> between a device removal and the Invalidate Request.
>
>>>> @@ -1273,6 +1273,9 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>>>> {
>>>> u32 fault;
>>>> int head, tail;
>>>> + u64 iqe_err, ite_sid;
>>>> + struct device *dev = NULL;
>>>> + struct pci_dev *pdev = NULL;
>>>> struct q_inval *qi = iommu->qi;
>>>> int shift = qi_shift(iommu);
>>>> @@ -1317,6 +1320,13 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>>>> tail = readl(iommu->reg + DMAR_IQT_REG);
>>>> tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>>>> + /*
>>>> + * SID field is valid only when the ITE field is Set in FSTS_REG
>>>> + * see Intel VT-d spec r4.1, section 11.4.9.9
>>>> + */
>>>> + iqe_err = dmar_readq(iommu->reg + DMAR_IQER_REG);
>>>> + ite_sid = DMAR_IQER_REG_ITESID(iqe_err);
>>>> +
>>>> writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
>>>> pr_info("Invalidation Time-out Error (ITE) cleared\n");
>>>> @@ -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"
>>
>>> /*
>>> * 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.
>>
>>>> + 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.
>>
>>> pdev = to_pci_dev(dev);
>>> if (!pci_device_is_present(pdev))
>>> return -ENODEV;
>>>
>>>
>>>> + return -ETIMEDOUT;
>>> -ENODEV.
>> The ATS invalidation request could be sent from userland in later code,
>> the userland code will care about the returned value, -ENODEV is one aspect
>> of the fact (target device not present), while -ETIMEDOUT is another
>> (timeout happened). we couldn't return them both.
>>
>>>> + }
>>>> if (qi->desc_status[wait_index] == QI_ABORT)
>>>> return -EAGAIN;
>>>> }
>>> Sorry, again for nit picking a v13 patch. I'm not a domain expert but
>>> this patchset seems reasonable to me.
>> Though this is the v13, it is based on new rbtree code, you are welcome.
>>
>> Thanks,
>> Ethan
>>
>>> regards,
>>> dan carpenter