[PATCH v13 0/3] fix vt-d hard lockup when hotplug ATS capable device

[PATCH v13 0/3] fix vt-d hard lockup when hotplug ATS capable device

[Ethan Zhao]

This patchset is used to fix vt-d hard lockup reported when surprise
unplug ATS capable endpoint device connects to system via PCIe switch
as following topology.

     +-[0000:15]-+-00.0  Intel Corporation Ice Lake Memory Map/VT-d
     |           +-00.1  Intel Corporation Ice Lake Mesh 2 PCIe
     |           +-00.2  Intel Corporation Ice Lake RAS
     |           +-00.4  Intel Corporation Device 0b23
     |           \-01.0-[16-1b]----00.0-[17-1b]--+-00.0-[18]----00.0
                                           NVIDIA Corporation Device 2324
     |                                           +-01.0-[19]----00.0
                          Mellanox Technologies MT2910 Family [ConnectX-7]

User brought endpoint device 19:00.0's link down by flapping it's hotplug
capable slot 17:01.0 link control register, as sequence DLLSC response,
pciehp_ist() will unload device driver and power it off, durning device
driver is unloading an iommu device-TLB invalidation (Intel VT-d spec, or
'ATS Invalidation' in PCIe spec) request issued to that link down device,
thus a long time completion/timeout waiting in interrupt context causes
continuous hard lockup warnning and system hang.

Other detail, see every patch commit log.

patch [1&2] were tested by yehaorong@bytedance.com on stable v6.7-rc4.
patch [1-3] passed compiling on stable v6.8-rc4 (Baolu's rbtree branch).

change log:
v13:
- rebased on Baolu's rbtree patchset.
- removed refactor patches [3/5][4/5] in v12.
- amend commit description of patch[3/3].
v12:
- use base-commit tag to format patch.
- fix building issue on v6.8-rc2 repported by lkp@intel.com.
- https://lkml.org/lkml/2024/1/28/535
v11:
- update per latest comment and suggestion from Baolu and YiLiu.
- split refactoring patch into two patches, [3/5] for simplify parameters
  and [4/5] for pdev parameter passing.
- re-order patches.
- fold target device presence check into qi_check_fault().
- combine patch[2][5] in v10 into one patch[5].
- some commit description correctness.
- add fixes tag to patch[2/5].
- rebased on 6.8rc1
- https://lkml.org/lkml/2024/1/25/1314
v10:
- refactor qi_submit_sync() and its callers to get pci_dev instance, as
  Kevin pointed out add target_flush_dev to iommu is not right.
v9:
- unify all spelling of ATS Invalidation adhere to PCIe spec per Bjorn's
  suggestion.
v8:
- add a patch to break the loop for timeout device-TLB invalidation, as
  Bjorn said there is possibility device just no response but not gone.
v7:
- reorder patches and revise commit log per Bjorn's guide.
- other code and commit log revise per Lukas' suggestion.
- rebased to stable v6.7-rc6.
v6:
- add two patches to break out device-TLB invalidation if device is gone.
v5:
- add a patch try to fix the rare case (surprise remove a device in
  safe removal process). not work because surprise removal handling can't
  re-enter when another safe removal is in process.
v4:
- move the PCI device state checking after ATS per Baolu's suggestion.
v3:
- fix commit description typo.
v2:
- revise commit[1] description part according to Lukas' suggestion.
- revise commit[2] description to clarify the issue's impact.
v1:
- https://lore.kernel.org/lkml/20231213034637.2603013-1-haifeng.zhao@
linux.intel.com/T/


Thanks,
Ethan
 


Ethan Zhao (3):
  PCI: make pci_dev_is_disconnected() helper public for other drivers
  iommu/vt-d: don't issue ATS Invalidation request when device is
    disconnected
  iommu/vt-d: improve ITE fault handling if target device isn't valid

 drivers/iommu/intel/dmar.c  | 25 +++++++++++++++++++++++++
 drivers/iommu/intel/pasid.c |  3 +++
 drivers/pci/pci.h           |  5 -----
 include/linux/pci.h         |  5 +++++
 4 files changed, 33 insertions(+), 5 deletions(-)


base-commit: e60bf5aa1a74c0652cd12d0cdc02f5c2b5fe5c74
-- 
2.31.1

[PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Ethan Zhao]

Make pci_dev_is_disconnected() public so that it can be called from
Intel VT-d driver to quickly fix/workaround the surprise removal
unplug hang issue for those ATS capable devices on PCIe switch downstream
hotplug capable ports.

Beside pci_device_is_present() function, this one has no config space
space access, so is light enough to optimize the normal pure surprise
removal and safe removal flow.

Tested-by: Haorong Ye <yehaorong@bytedance.com>
Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
---
 drivers/pci/pci.h   | 5 -----
 include/linux/pci.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e9750b1b19ba..bfc56f7bee1c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -368,11 +368,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	return 0;
 }
 
-static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
-{
-	return dev->error_state == pci_channel_io_perm_failure;
-}
-
 /* pci_dev priv_flags */
 #define PCI_DEV_ADDED 0
 #define PCI_DPC_RECOVERED 1
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7ab0d13672da..213109d3c601 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2517,6 +2517,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
 	return NULL;
 }
 
+static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
+{
+	return dev->error_state == pci_channel_io_perm_failure;
+}
+
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
 bool pci_acs_path_enabled(struct pci_dev *start,
-- 
2.31.1

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Baolu Lu]

On 2024/2/22 17:02, Ethan Zhao wrote:
> Make pci_dev_is_disconnected() public so that it can be called from
> Intel VT-d driver to quickly fix/workaround the surprise removal
> unplug hang issue for those ATS capable devices on PCIe switch downstream
> hotplug capable ports.
> 
> Beside pci_device_is_present() function, this one has no config space
> space access, so is light enough to optimize the normal pure surprise
> removal and safe removal flow.
> 
> Tested-by: Haorong Ye<yehaorong@bytedance.com>
> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> ---
>   drivers/pci/pci.h   | 5 -----
>   include/linux/pci.h | 5 +++++
>   2 files changed, 5 insertions(+), 5 deletions(-)

Hi PCI subsystem maintainers,

The iommu drivers (including, but not limited to, the Intel VT-d driver)
require a helper to check the physical presence of a PCI device in two
scenarios:

- During the iommu_release_device() path: This ensures the device is
   physically present before sending device TLB invalidation to device.

- During the device driver lifecycle when a device TLB invalidation
   timeout event is generated by the IOMMU hardware: This helps handle
   situations where the device might have been hot-removed.

While there may be some adjustments needed in patch 3/3, I'd like to
confirm with you whether it's feasible to expose this helper for general
use within the iommu subsystem.

If you agree with this change, I can route this patch to Linus through
the iommu tree with an "acked-by" or "reviewed-by" tag from you.

Best regards,
baolu

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Dan Carpenter]

I'm not a PCI maintainer, but first two patches seem good to me.  For
the third patch, my complaints were really minor.  Let's just add a
Fixes tag for sure, the rest is okay.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Ethan Zhao]

On 2/23/2024 2:47 PM, Dan Carpenter wrote:
> I'm not a PCI maintainer, but first two patches seem good to me.  For
> the third patch, my complaints were really minor.  Let's just add a
> Fixes tag for sure, the rest is okay.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

Thanks Dan.

>
> regards,
> dan carpenter
>

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Bjorn Helgaas]

On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
> On 2024/2/22 17:02, Ethan Zhao wrote:
> > Make pci_dev_is_disconnected() public so that it can be called from
> > Intel VT-d driver to quickly fix/workaround the surprise removal
> > unplug hang issue for those ATS capable devices on PCIe switch downstream
> > hotplug capable ports.
> > 
> > Beside pci_device_is_present() function, this one has no config space
> > space access, so is light enough to optimize the normal pure surprise
> > removal and safe removal flow.
> > 
> > Tested-by: Haorong Ye<yehaorong@bytedance.com>
> > Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> > ---
> >   drivers/pci/pci.h   | 5 -----
> >   include/linux/pci.h | 5 +++++
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Hi PCI subsystem maintainers,
> 
> The iommu drivers (including, but not limited to, the Intel VT-d driver)
> require a helper to check the physical presence of a PCI device in two
> scenarios:
> 
> - During the iommu_release_device() path: This ensures the device is
>   physically present before sending device TLB invalidation to device.

This wording is fundamentally wrong.  Testing
pci_dev_is_disconnected() can never ensure the device will still be
present by the time a TLB invalidation is sent.

The device may be removed after the pci_dev_is_disconnected() test and
before a TLB invalidate is sent.

This is why I hesitate to expose pci_dev_is_disconnected() (and
pci_device_is_present(), which we already export) outside
drivers/pci/.  They both lead to terrible mistakes like relying on the
false assumption that the result will remain valid after the functions
return, without any recognition that we MUST be able to deal with the
cases where that assumption is broken.

This series claims to avoid "continuous hard lockup warnings and
system hangs".  It may reduce the likelihood, but I don't think it can
completely avoid them.  

I don't see any acknowledgement of that in the commit logs of this
series.  E.g., it doesn't say "we can recover from ATS Invalidate
Completion Timeouts, but the timeouts are on the order of minutes, so
we want to avoid them when possible."

And given the "system hangs" language, I am not convinced that we
actually *can* recover from those timeouts.

Using pci_dev_is_disconnected() may make those timeouts less frequent
and give the illusion that the problem is "solved", but it just means
the problem is still there and harder to reproduce.

> - During the device driver lifecycle when a device TLB invalidation
>   timeout event is generated by the IOMMU hardware: This helps handle
>   situations where the device might have been hot-removed.
> 
> While there may be some adjustments needed in patch 3/3, I'd like to
> confirm with you whether it's feasible to expose this helper for general
> use within the iommu subsystem.
> 
> If you agree with this change, I can route this patch to Linus through
> the iommu tree with an "acked-by" or "reviewed-by" tag from you.
> 
> Best regards,
> baolu

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Ethan Zhao]

On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
> On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
>> On 2024/2/22 17:02, Ethan Zhao wrote:
>>> Make pci_dev_is_disconnected() public so that it can be called from
>>> Intel VT-d driver to quickly fix/workaround the surprise removal
>>> unplug hang issue for those ATS capable devices on PCIe switch downstream
>>> hotplug capable ports.
>>>
>>> Beside pci_device_is_present() function, this one has no config space
>>> space access, so is light enough to optimize the normal pure surprise
>>> removal and safe removal flow.
>>>
>>> Tested-by: Haorong Ye<yehaorong@bytedance.com>
>>> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
>>> ---
>>>    drivers/pci/pci.h   | 5 -----
>>>    include/linux/pci.h | 5 +++++
>>>    2 files changed, 5 insertions(+), 5 deletions(-)
>> Hi PCI subsystem maintainers,
>>
>> The iommu drivers (including, but not limited to, the Intel VT-d driver)
>> require a helper to check the physical presence of a PCI device in two
>> scenarios:
>>
>> - During the iommu_release_device() path: This ensures the device is
>>    physically present before sending device TLB invalidation to device.
> This wording is fundamentally wrong.  Testing
> pci_dev_is_disconnected() can never ensure the device will still be
> present by the time a TLB invalidation is sent.

The logic of testing pci_dev_is_disconnected() in patch [2/3] works
in the opposite:

1. if pci_dev_is_disconnected() return true, means the device is in
    the process of surprise removal handling, adapter already been
    removed from the slot.

2. for removed device, no need to send ATS invalidation request to it.
    removed device lost power, its devTLB wouldn't be valid anymore.

3. if pci_dev_is_disconnected() return false, the device is *likely*
    to be removed at any momoment after this function called.
    such case will be treated in the iommu ITE fault handling, not to
    retry the timeout request if device isn't present (patch [3/3]).

>
> The device may be removed after the pci_dev_is_disconnected() test and
> before a TLB invalidate is sent.

even in the process while TLB is invalidating.

>
> This is why I hesitate to expose pci_dev_is_disconnected() (and
> pci_device_is_present(), which we already export) outside
> drivers/pci/.  They both lead to terrible mistakes like relying on the
> false assumption that the result will remain valid after the functions
> return, without any recognition that we MUST be able to deal with the
> cases where that assumption is broken.

Yup, your concern is worthy ,but isn't happening within this patchset.

>
> This series claims to avoid "continuous hard lockup warnings and
> system hangs".  It may reduce the likelihood, but I don't think it can
> completely avoid them.

It doesn't try to close the race window, actually we are doing post-fault
handling in patch [3/3].

>
> I don't see any acknowledgement of that in the commit logs of this
> series.  E.g., it doesn't say "we can recover from ATS Invalidate
> Completion Timeouts, but the timeouts are on the order of minutes, so
> we want to avoid them when possible."
> And given the "system hangs" language, I am not convinced that we
> actually *can* recover from those timeouts.

It is testing in customer's environment. separatly, patch[3/3] vs

patch [2/3].

>
> Using pci_dev_is_disconnected() may make those timeouts less frequent

will test patch[3/3] alone, if couldn't recover from the 100% ITE fault
case, we will look for other method.

Thanks,
Ethan

> and give the illusion that the problem is "solved", but it just means
> the problem is still there and harder to reproduce.
>
>> - During the device driver lifecycle when a device TLB invalidation
>>    timeout event is generated by the IOMMU hardware: This helps handle
>>    situations where the device might have been hot-removed.
>>
>> While there may be some adjustments needed in patch 3/3, I'd like to
>> confirm with you whether it's feasible to expose this helper for general
>> use within the iommu subsystem.
>>
>> If you agree with this change, I can route this patch to Linus through
>> the iommu tree with an "acked-by" or "reviewed-by" tag from you.
>>
>> Best regards,
>> baolu

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Bjorn Helgaas]

On Thu, Feb 29, 2024 at 09:58:43AM +0800, Ethan Zhao wrote:
> On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
> > On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
> > > On 2024/2/22 17:02, Ethan Zhao wrote:
> > > > Make pci_dev_is_disconnected() public so that it can be called from
> > > > Intel VT-d driver to quickly fix/workaround the surprise removal
> > > > unplug hang issue for those ATS capable devices on PCIe switch downstream
> > > > hotplug capable ports.
> > > > 
> > > > Beside pci_device_is_present() function, this one has no config space
> > > > space access, so is light enough to optimize the normal pure surprise
> > > > removal and safe removal flow.
> > > > 
> > > > Tested-by: Haorong Ye<yehaorong@bytedance.com>
> > > > Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
> > > > ---
> > > >    drivers/pci/pci.h   | 5 -----
> > > >    include/linux/pci.h | 5 +++++
> > > >    2 files changed, 5 insertions(+), 5 deletions(-)
> > > Hi PCI subsystem maintainers,
> > > 
> > > The iommu drivers (including, but not limited to, the Intel VT-d driver)
> > > require a helper to check the physical presence of a PCI device in two
> > > scenarios:
> > > 
> > > - During the iommu_release_device() path: This ensures the device is
> > >    physically present before sending device TLB invalidation to device.
> > This wording is fundamentally wrong.  Testing
> > pci_dev_is_disconnected() can never ensure the device will still be
> > present by the time a TLB invalidation is sent.
> 
> The logic of testing pci_dev_is_disconnected() in patch [2/3] works
> in the opposite:
> 
> 1. if pci_dev_is_disconnected() return true, means the device is in
>    the process of surprise removal handling, adapter already been
>    removed from the slot.
> 
> 2. for removed device, no need to send ATS invalidation request to it.
>    removed device lost power, its devTLB wouldn't be valid anymore.
> 
> 3. if pci_dev_is_disconnected() return false, the device is *likely*
>    to be removed at any momoment after this function called.
>    such case will be treated in the iommu ITE fault handling, not to
>    retry the timeout request if device isn't present (patch [3/3]).
> 
> > The device may be removed after the pci_dev_is_disconnected() test and
> > before a TLB invalidate is sent.
> 
> even in the process while TLB is invalidating.
> 
> > This is why I hesitate to expose pci_dev_is_disconnected() (and
> > pci_device_is_present(), which we already export) outside
> > drivers/pci/.  They both lead to terrible mistakes like relying on the
> > false assumption that the result will remain valid after the functions
> > return, without any recognition that we MUST be able to deal with the
> > cases where that assumption is broken.
> 
> Yup, your concern is worthy ,but isn't happening within this patchset.

OK, I acked the patch.

I guess my complaint is really with pci_device_is_present() because
that's even harder to use correctly.

pci_device_is_present():
  slow (may do config access to device)
  true  => device *was* present in the recent past, may not be now
  false => device is not accessible

pci_dev_is_disconnected():
  fast (doesn't touch device)
  true  => device is not accessible
  false => basically means nothing

I guess they're both hard ;)

Bjorn

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Ethan Zhao]

On 3/1/2024 6:33 AM, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:58:43AM +0800, Ethan Zhao wrote:
>> On 2/27/2024 7:05 AM, Bjorn Helgaas wrote:
>>> On Thu, Feb 22, 2024 at 08:54:54PM +0800, Baolu Lu wrote:
>>>> On 2024/2/22 17:02, Ethan Zhao wrote:
>>>>> Make pci_dev_is_disconnected() public so that it can be called from
>>>>> Intel VT-d driver to quickly fix/workaround the surprise removal
>>>>> unplug hang issue for those ATS capable devices on PCIe switch downstream
>>>>> hotplug capable ports.
>>>>>
>>>>> Beside pci_device_is_present() function, this one has no config space
>>>>> space access, so is light enough to optimize the normal pure surprise
>>>>> removal and safe removal flow.
>>>>>
>>>>> Tested-by: Haorong Ye<yehaorong@bytedance.com>
>>>>> Signed-off-by: Ethan Zhao<haifeng.zhao@linux.intel.com>
>>>>> ---
>>>>>     drivers/pci/pci.h   | 5 -----
>>>>>     include/linux/pci.h | 5 +++++
>>>>>     2 files changed, 5 insertions(+), 5 deletions(-)
>>>> Hi PCI subsystem maintainers,
>>>>
>>>> The iommu drivers (including, but not limited to, the Intel VT-d driver)
>>>> require a helper to check the physical presence of a PCI device in two
>>>> scenarios:
>>>>
>>>> - During the iommu_release_device() path: This ensures the device is
>>>>     physically present before sending device TLB invalidation to device.
>>> This wording is fundamentally wrong.  Testing
>>> pci_dev_is_disconnected() can never ensure the device will still be
>>> present by the time a TLB invalidation is sent.
>> The logic of testing pci_dev_is_disconnected() in patch [2/3] works
>> in the opposite:
>>
>> 1. if pci_dev_is_disconnected() return true, means the device is in
>>     the process of surprise removal handling, adapter already been
>>     removed from the slot.
>>
>> 2. for removed device, no need to send ATS invalidation request to it.
>>     removed device lost power, its devTLB wouldn't be valid anymore.
>>
>> 3. if pci_dev_is_disconnected() return false, the device is *likely*
>>     to be removed at any momoment after this function called.
>>     such case will be treated in the iommu ITE fault handling, not to
>>     retry the timeout request if device isn't present (patch [3/3]).
>>
>>> The device may be removed after the pci_dev_is_disconnected() test and
>>> before a TLB invalidate is sent.
>> even in the process while TLB is invalidating.
>>
>>> This is why I hesitate to expose pci_dev_is_disconnected() (and
>>> pci_device_is_present(), which we already export) outside
>>> drivers/pci/.  They both lead to terrible mistakes like relying on the
>>> false assumption that the result will remain valid after the functions
>>> return, without any recognition that we MUST be able to deal with the
>>> cases where that assumption is broken.
>> Yup, your concern is worthy ,but isn't happening within this patchset.
> OK, I acked the patch.

Great !

>
> I guess my complaint is really with pci_device_is_present() because
> that's even harder to use correctly.
>
> pci_device_is_present():
>    slow (may do config access to device)
>    true  => device *was* present in the recent past, may not be now
>    false => device is not accessible

so the 'false' result is reliable for post-calling code, then give up
more attempt of the same request. the usage in patch[3/3]

>
> pci_dev_is_disconnected():
>    fast (doesn't touch device)
>    true  => device is not accessible

also we are relying on the 'true' result returned, used in  patch[2/3].

>    false => basically means nothing
>
> I guess they're both hard ;)

seems I didn't mess them up. :)

Thanks,
Ethan

>
> Bjorn

Re: [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers

[Bjorn Helgaas]

On Thu, Feb 22, 2024 at 04:02:49AM -0500, Ethan Zhao wrote:
> Make pci_dev_is_disconnected() public so that it can be called from
> Intel VT-d driver to quickly fix/workaround the surprise removal
> unplug hang issue for those ATS capable devices on PCIe switch downstream
> hotplug capable ports.
> 
> Beside pci_device_is_present() function, this one has no config space
> space access, so is light enough to optimize the normal pure surprise
> removal and safe removal flow.
> 
> Tested-by: Haorong Ye <yehaorong@bytedance.com>
> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/pci.h   | 5 -----
>  include/linux/pci.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e9750b1b19ba..bfc56f7bee1c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -368,11 +368,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  	return 0;
>  }
>  
> -static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> -{
> -	return dev->error_state == pci_channel_io_perm_failure;
> -}
> -
>  /* pci_dev priv_flags */
>  #define PCI_DEV_ADDED 0
>  #define PCI_DPC_RECOVERED 1
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7ab0d13672da..213109d3c601 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2517,6 +2517,11 @@ static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
>  	return NULL;
>  }
>  
> +static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> +{
> +	return dev->error_state == pci_channel_io_perm_failure;
> +}
> +
>  void pci_request_acs(void);
>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>  bool pci_acs_path_enabled(struct pci_dev *start,
> -- 
> 2.31.1
> 

[PATCH v13 2/3] iommu/vt-d: don't issue ATS Invalidation request when device is disconnected

[Ethan Zhao]

For those endpoint devices connect to system via hotplug capable ports,
users could request a hot reset to the device by flapping device's link
through setting the slot's link control register, as pciehp_ist() DLLSC
interrupt sequence response, pciehp will unload the device driver and
then power it off. thus cause an IOMMU device-TLB invalidation (Intel
VT-d spec, or ATS Invalidation in PCIe spec r6.1) request for non-existence
target device to be sent and deadly loop to retry that request after ITE
fault triggered in interrupt context.

That would cause following continuous hard lockup warning and system hang

[ 4211.433662] pcieport 0000:17:01.0: pciehp: Slot(108): Link Down
[ 4211.433664] pcieport 0000:17:01.0: pciehp: Slot(108): Card not present
[ 4223.822591] NMI watchdog: Watchdog detected hard LOCKUP on cpu 144
[ 4223.822622] CPU: 144 PID: 1422 Comm: irq/57-pciehp Kdump: loaded Tainted: G S
         OE    kernel version xxxx
[ 4223.822623] Hardware name: vendorname xxxx 666-106,
BIOS 01.01.02.03.01 05/15/2023
[ 4223.822623] RIP: 0010:qi_submit_sync+0x2c0/0x490
[ 4223.822624] Code: 48 be 00 00 00 00 00 08 00 00 49 85 74 24 20 0f 95 c1 48 8b
 57 10 83 c1 04 83 3c 1a 03 0f 84 a2 01 00 00 49 8b 04 24 8b 70 34 <40> f6 c6 1
0 74 17 49 8b 04 24 8b 80 80 00 00 00 89 c2 d3 fa 41 39
[ 4223.822624] RSP: 0018:ffffc4f074f0bbb8 EFLAGS: 00000093
[ 4223.822625] RAX: ffffc4f040059000 RBX: 0000000000000014 RCX: 0000000000000005
[ 4223.822625] RDX: ffff9f3841315800 RSI: 0000000000000000 RDI: ffff9f38401a8340
[ 4223.822625] RBP: ffff9f38401a8340 R08: ffffc4f074f0bc00 R09: 0000000000000000
[ 4223.822626] R10: 0000000000000010 R11: 0000000000000018 R12: ffff9f384005e200
[ 4223.822626] R13: 0000000000000004 R14: 0000000000000046 R15: 0000000000000004
[ 4223.822626] FS:  0000000000000000(0000) GS:ffffa237ae400000(0000)
knlGS:0000000000000000
[ 4223.822627] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4223.822627] CR2: 00007ffe86515d80 CR3: 000002fd3000a001 CR4: 0000000000770ee0
[ 4223.822627] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 4223.822628] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[ 4223.822628] PKRU: 55555554
[ 4223.822628] Call Trace:
[ 4223.822628]  qi_flush_dev_iotlb+0xb1/0xd0
[ 4223.822628]  __dmar_remove_one_dev_info+0x224/0x250
[ 4223.822629]  dmar_remove_one_dev_info+0x3e/0x50
[ 4223.822629]  intel_iommu_release_device+0x1f/0x30
[ 4223.822629]  iommu_release_device+0x33/0x60
[ 4223.822629]  iommu_bus_notifier+0x7f/0x90
[ 4223.822630]  blocking_notifier_call_chain+0x60/0x90
[ 4223.822630]  device_del+0x2e5/0x420
[ 4223.822630]  pci_remove_bus_device+0x70/0x110
[ 4223.822630]  pciehp_unconfigure_device+0x7c/0x130
[ 4223.822631]  pciehp_disable_slot+0x6b/0x100
[ 4223.822631]  pciehp_handle_presence_or_link_change+0xd8/0x320
[ 4223.822631]  pciehp_ist+0x176/0x180
[ 4223.822631]  ? irq_finalize_oneshot.part.50+0x110/0x110
[ 4223.822632]  irq_thread_fn+0x19/0x50
[ 4223.822632]  irq_thread+0x104/0x190
[ 4223.822632]  ? irq_forced_thread_fn+0x90/0x90
[ 4223.822632]  ? irq_thread_check_affinity+0xe0/0xe0
[ 4223.822633]  kthread+0x114/0x130
[ 4223.822633]  ? __kthread_cancel_work+0x40/0x40
[ 4223.822633]  ret_from_fork+0x1f/0x30
[ 4223.822633] Kernel panic - not syncing: Hard LOCKUP
[ 4223.822634] CPU: 144 PID: 1422 Comm: irq/57-pciehp Kdump: loaded Tainted: G S
         OE     kernel version xxxx
[ 4223.822634] Hardware name: vendorname xxxx 666-106,
BIOS 01.01.02.03.01 05/15/2023
[ 4223.822634] Call Trace:
[ 4223.822634]  <NMI>
[ 4223.822635]  dump_stack+0x6d/0x88
[ 4223.822635]  panic+0x101/0x2d0
[ 4223.822635]  ? ret_from_fork+0x11/0x30
[ 4223.822635]  nmi_panic.cold.14+0xc/0xc
[ 4223.822636]  watchdog_overflow_callback.cold.8+0x6d/0x81
[ 4223.822636]  __perf_event_overflow+0x4f/0xf0
[ 4223.822636]  handle_pmi_common+0x1ef/0x290
[ 4223.822636]  ? __set_pte_vaddr+0x28/0x40
[ 4223.822637]  ? flush_tlb_one_kernel+0xa/0x20
[ 4223.822637]  ? __native_set_fixmap+0x24/0x30
[ 4223.822637]  ? ghes_copy_tofrom_phys+0x70/0x100
[ 4223.822637]  ? __ghes_peek_estatus.isra.16+0x49/0xa0
[ 4223.822637]  intel_pmu_handle_irq+0xba/0x2b0
[ 4223.822638]  perf_event_nmi_handler+0x24/0x40
[ 4223.822638]  nmi_handle+0x4d/0xf0
[ 4223.822638]  default_do_nmi+0x49/0x100
[ 4223.822638]  exc_nmi+0x134/0x180
[ 4223.822639]  end_repeat_nmi+0x16/0x67
[ 4223.822639] RIP: 0010:qi_submit_sync+0x2c0/0x490
[ 4223.822639] Code: 48 be 00 00 00 00 00 08 00 00 49 85 74 24 20 0f 95 c1 48 8b
 57 10 83 c1 04 83 3c 1a 03 0f 84 a2 01 00 00 49 8b 04 24 8b 70 34 <40> f6 c6 10
 74 17 49 8b 04 24 8b 80 80 00 00 00 89 c2 d3 fa 41 39
[ 4223.822640] RSP: 0018:ffffc4f074f0bbb8 EFLAGS: 00000093
[ 4223.822640] RAX: ffffc4f040059000 RBX: 0000000000000014 RCX: 0000000000000005
[ 4223.822640] RDX: ffff9f3841315800 RSI: 0000000000000000 RDI: ffff9f38401a8340
[ 4223.822641] RBP: ffff9f38401a8340 R08: ffffc4f074f0bc00 R09: 0000000000000000
[ 4223.822641] R10: 0000000000000010 R11: 0000000000000018 R12: ffff9f384005e200
[ 4223.822641] R13: 0000000000000004 R14: 0000000000000046 R15: 0000000000000004
[ 4223.822641]  ? qi_submit_sync+0x2c0/0x490
[ 4223.822642]  ? qi_submit_sync+0x2c0/0x490
[ 4223.822642]  </NMI>
[ 4223.822642]  qi_flush_dev_iotlb+0xb1/0xd0
[ 4223.822642]  __dmar_remove_one_dev_info+0x224/0x250
[ 4223.822643]  dmar_remove_one_dev_info+0x3e/0x50
[ 4223.822643]  intel_iommu_release_device+0x1f/0x30
[ 4223.822643]  iommu_release_device+0x33/0x60
[ 4223.822643]  iommu_bus_notifier+0x7f/0x90
[ 4223.822644]  blocking_notifier_call_chain+0x60/0x90
[ 4223.822644]  device_del+0x2e5/0x420
[ 4223.822644]  pci_remove_bus_device+0x70/0x110
[ 4223.822644]  pciehp_unconfigure_device+0x7c/0x130
[ 4223.822644]  pciehp_disable_slot+0x6b/0x100
[ 4223.822645]  pciehp_handle_presence_or_link_change+0xd8/0x320
[ 4223.822645]  pciehp_ist+0x176/0x180
[ 4223.822645]  ? irq_finalize_oneshot.part.50+0x110/0x110
[ 4223.822645]  irq_thread_fn+0x19/0x50
[ 4223.822646]  irq_thread+0x104/0x190
[ 4223.822646]  ? irq_forced_thread_fn+0x90/0x90
[ 4223.822646]  ? irq_thread_check_affinity+0xe0/0xe0
[ 4223.822646]  kthread+0x114/0x130
[ 4223.822647]  ? __kthread_cancel_work+0x40/0x40
[ 4223.822647]  ret_from_fork+0x1f/0x30
[ 4223.822647] Kernel Offset: 0x6400000 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffffbfffffff)

Such issue could be triggered by all kinds of regular surprise removal
hotplug operation. like:

1. pull EP(endpoint device) out directly.
2. turn off EP's power.
3. bring the link down.
etc.

this patch aims to work for regular safe removal and surprise removal
unplug. these hot unplug handling process could be optimized for fix the
ATS Invalidation hang issue by calling pci_dev_is_disconnected() in
function devtlb_invalidation_with_pasid() to check target device state to
avoid sending meaningless ATS Invalidation request to iommu when device is
gone. (see IMPLEMENTATION NOTE in PCIe spec r6.1 section 10.3.1)

For safe removal, device wouldn't be removed until the whole software
handling process is done, it wouldn't trigger the hard lock up issue
caused by too long ATS Invalidation timeout wait. In safe removal path,
device state isn't set to pci_channel_io_perm_failure in
pciehp_unconfigure_device() by checking 'presence' parameter, calling
pci_dev_is_disconnected() in devtlb_invalidation_with_pasid() will return
false there, wouldn't break the function.

For surprise removal, device state is set to pci_channel_io_perm_failure in
pciehp_unconfigure_device(), means device is already gone (disconnected)
call pci_dev_is_disconnected() in devtlb_invalidation_with_pasid() will
return true to break the function not to send ATS Invalidation request to
the disconnected device blindly, thus avoid to trigger further ITE fault,
and ITE fault will block all invalidation request to be handled.
furthermore retry the timeout request could trigger hard lockup.

safe removal (present) & surprise removal (not present)

pciehp_ist()
   pciehp_handle_presence_or_link_change()
     pciehp_disable_slot()
       remove_board()
         pciehp_unconfigure_device(presence) {
           if (!presence)
                pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
           }

this patch works for regular safe removal and surprise removal of ATS
capable endpoint on PCIe switch downstream ports.

Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table interface")
Tested-by: Haorong Ye <yehaorong@bytedance.com>
Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
---
 drivers/iommu/intel/pasid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 108158e2b907..746c7abe2237 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -214,6 +214,9 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 	if (!info || !info->ats_enabled)
 		return;
 
+	if (pci_dev_is_disconnected(to_pci_dev(dev)))
+		return;
+
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
 	pfsid = info->pfsid;
-- 
2.31.1

[PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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_rbtre (probed, not released) and present.

Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
---
 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.
+		 */
+		if (ite_sid) {
+			dev = device_rbtree_find(iommu, ite_sid);
+			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)))
+				return -ETIMEDOUT;
+		}
 		if (qi->desc_status[wait_index] == QI_ABORT)
 			return -EAGAIN;
 	}
-- 
2.31.1

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Dan Carpenter]

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 <haifeng.zhao@linux.intel.com>

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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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 <haifeng.zhao@linux.intel.com>
> 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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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
>

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Dan Carpenter]

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
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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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

Sure.

> 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 patchset is based on Baolu's rbtree patchset,
https://lkml.org/lkml/2024/2/15/147
I mentioned that in the cover-letter (and the base commit tag).

>
> This is very frustrating thing.  But let's say a buggy BIOS could mess

Buggy BIOS dmar table etc doesn't corrupt the rbtree, but might feed rbtree
with inconsistent info about iommu and pci device.

e.g. the device exists, present, but in fact, the device doesn't belong to the
iommu the BIOS table claimed, if devTLB invalidation sent to such target, we
will get ITE too, but here it isn't be handled. we will deal with that case in
other patches.

The key used to index rbtree is device_domain_info->bus, device_domain_info->
devfn, see intel_iommu_probe_device(), these values not always be the same
as pdev, such as the device is vmd/VF etc, here we only handle the case we intend
to handle ---the hot removal of physical device.

Thanks,
Ethan

> 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
> 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
>

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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
>
>

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Dan Carpenter]

On Tue, Feb 27, 2024 at 04:00:33AM +0800, Ethan Zhao wrote:
> > +                       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)))

Yep, that's what I was asking.

> 
> 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.
> ... ...

Fair enough...  Thanks.

regards,
dan carpenter

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Bjorn Helgaas]

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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Bjorn Helgaas]

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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[Ethan Zhao]

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

Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid

[kernel test robot]

Hi Ethan,

kernel test robot noticed the following build errors:

[auto build test ERROR on e60bf5aa1a74c0652cd12d0cdc02f5c2b5fe5c74]

url:    https://github.com/intel-lab-lkp/linux/commits/Ethan-Zhao/PCI-make-pci_dev_is_disconnected-helper-public-for-other-drivers/20240222-170625
base:   e60bf5aa1a74c0652cd12d0cdc02f5c2b5fe5c74
patch link:    https://lore.kernel.org/r/20240222090251.2849702-4-haifeng.zhao%40linux.intel.com
patch subject: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid
config: x86_64-randconfig-074-20240226 (https://download.01.org/0day-ci/archive/20240226/202402261910.IZ3PGmH3-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240226/202402261910.IZ3PGmH3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402261910.IZ3PGmH3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: device_rbtree_find
   >>> referenced by dmar.c:1346 (drivers/iommu/intel/dmar.c:1346)
   >>>               drivers/iommu/intel/dmar.o:(qi_submit_sync) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki