[PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Lu Baolu]

Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
attached to the device and disabled when the device transitions to block
translation mode. This approach is inappropriate as PCI PASID is a device
feature independent of the type of the attached domain.

Enable PCI PASID during the IOMMU device probe and disables it during the
release path.

Suggested-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9ff8b83c19a3..5a8080c71b04 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1322,15 +1322,6 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)
 		return;
 
 	pdev = to_pci_dev(info->dev);
-
-	/* The PCIe spec, in its wisdom, declares that the behaviour of
-	   the device if you enable PASID support after ATS support is
-	   undefined. So always enable PASID support on devices which
-	   have it, even if we can't yet know if we're ever going to
-	   use it. */
-	if (info->pasid_supported && !pci_enable_pasid(pdev, info->pasid_supported & ~1))
-		info->pasid_enabled = 1;
-
 	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
 	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
@@ -1352,11 +1343,6 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
 		info->ats_enabled = 0;
 		domain_update_iotlb(info->domain);
 	}
-
-	if (info->pasid_enabled) {
-		pci_disable_pasid(pdev);
-		info->pasid_enabled = 0;
-	}
 }
 
 static void intel_flush_iotlb_all(struct iommu_domain *domain)
@@ -4110,6 +4096,16 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 		}
 	}
 
+	/*
+	 * The PCIe spec, in its wisdom, declares that the behaviour of the
+	 * device is undefined if you enable PASID support after ATS support.
+	 * So always enable PASID support on devices which have it, even if
+	 * we can't yet know if we're ever going to use it.
+	 */
+	if (info->pasid_supported &&
+	    !pci_enable_pasid(pdev, info->pasid_supported & ~1))
+		info->pasid_enabled = 1;
+
 	intel_iommu_debugfs_create_dev(info);
 
 	return &iommu->iommu;
@@ -4128,6 +4124,9 @@ static void intel_iommu_release_device(struct device *dev)
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 
+	if (info->pasid_enabled)
+		pci_disable_pasid(to_pci_dev(dev));
+
 	mutex_lock(&iommu->iopf_lock);
 	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
 		device_rbtree_remove(info);
-- 
2.34.1

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Vasant Hegde]

Hi Lu,


On 8/16/2024 4:19 PM, Lu Baolu wrote:
> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
> attached to the device and disabled when the device transitions to block
> translation mode. This approach is inappropriate as PCI PASID is a device
> feature independent of the type of the attached domain.

Reading through other thread, I thought we want to enable both PASID and PRI in
device probe path. Did I miss something?

-Vasant

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/16 20:16, Vasant Hegde wrote:
> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>> attached to the device and disabled when the device transitions to block
>> translation mode. This approach is inappropriate as PCI PASID is a device
>> feature independent of the type of the attached domain.
> Reading through other thread, I thought we want to enable both PASID and PRI in
> device probe path. Did I miss something?

PRI is different. PRI should be enabled when the first iopf-capable
domain is attached to device or its PASID, and disabled when the last
such domain is detached.

Thanks,
baolu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Jason Gunthorpe]

On Fri, Aug 16, 2024 at 09:09:04PM +0800, Baolu Lu wrote:
> On 2024/8/16 20:16, Vasant Hegde wrote:
> > On 8/16/2024 4:19 PM, Lu Baolu wrote:
> > > Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
> > > attached to the device and disabled when the device transitions to block
> > > translation mode. This approach is inappropriate as PCI PASID is a device
> > > feature independent of the type of the attached domain.
> > Reading through other thread, I thought we want to enable both PASID and PRI in
> > device probe path. Did I miss something?
> 
> PRI is different. PRI should be enabled when the first iopf-capable
> domain is attached to device or its PASID, and disabled when the last
> such domain is detached.

+1

Jason

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Vasant Hegde]

Hi,


On 8/16/2024 6:39 PM, Baolu Lu wrote:
> On 2024/8/16 20:16, Vasant Hegde wrote:
>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>> attached to the device and disabled when the device transitions to block
>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>> feature independent of the type of the attached domain.
>> Reading through other thread, I thought we want to enable both PASID and PRI in
>> device probe path. Did I miss something?
> 
> PRI is different. PRI should be enabled when the first iopf-capable
> domain is attached to device or its PASID, and disabled when the last
> such domain is detached.

Right. That's what AMD driver also does (We enable it when we attach IOPF
capable domain). But looking into pci_enable_pri() :


202         /*
203          * VFs must not implement the PRI Capability.  If their PF
204          * implements PRI, it is shared by the VFs, so if the PF PRI is
205          * enabled, it is also enabled for the VF.
206          */
207         if (pdev->is_virtfn) {
208                 if (pci_physfn(pdev)->pri_enabled)
209                         return 0;
210                 return -EINVAL;
211         }
212


If we try to enable PRI for VF without first enabling it in PF it will fail right?

Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
AMD v2 page table -OR- nested domain) it will fail right?



-Vasant

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/19 14:34, Vasant Hegde wrote:
> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>> attached to the device and disabled when the device transitions to block
>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>> feature independent of the type of the attached domain.
>>> Reading through other thread, I thought we want to enable both PASID and PRI in
>>> device probe path. Did I miss something?
>> PRI is different. PRI should be enabled when the first iopf-capable
>> domain is attached to device or its PASID, and disabled when the last
>> such domain is detached.
> Right. That's what AMD driver also does (We enable it when we attach IOPF
> capable domain). But looking into pci_enable_pri() :
> 
> 
> 202         /*
> 203          * VFs must not implement the PRI Capability.  If their PF
> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
> 205          * enabled, it is also enabled for the VF.
> 206          */
> 207         if (pdev->is_virtfn) {
> 208                 if (pci_physfn(pdev)->pri_enabled)
> 209                         return 0;
> 210                 return -EINVAL;
> 211         }
> 212
> 
> 
> If we try to enable PRI for VF without first enabling it in PF it will fail right?
> 
> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
> AMD v2 page table -OR- nested domain) it will fail right?

Yeah! So, the iommu driver should basically control the PRI switch on
the PF whenever someone wants to use it on a VF.

We could simplify things by turning on PRI for the whole PF when the
first iopf-capable domain is attached to a VF. Then, we'd only turn it
off once all VFs have such domains detached.

Thanks,
baolu
Yeah, so the iommu driver should flip the PRI switch on the PF whenever
someone wants to turn it on for its VF.

We could change things up so that when the first iopf-capable domain is 
attached to any VF or PF, we turn on PRI for the whole device. Then, we
turn it off when every VF and PF have such domains detached.

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/19 15:09, Baolu Lu wrote:
> Yeah, so the iommu driver should flip the PRI switch on the PF whenever
> someone wants to turn it on for its VF.
> 
> We could change things up so that when the first iopf-capable domain is 
> attached to any VF or PF, we turn on PRI for the whole device. Then, we
> turn it off when every VF and PF have such domains detached.

Please ignore this. I forgot to remove it when writing the words. Sorry
about it.

Thanks,
baolu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Jason Gunthorpe]

On Mon, Aug 19, 2024 at 03:09:00PM +0800, Baolu Lu wrote:
> On 2024/8/19 14:34, Vasant Hegde wrote:
> > On 8/16/2024 6:39 PM, Baolu Lu wrote:
> > > On 2024/8/16 20:16, Vasant Hegde wrote:
> > > > On 8/16/2024 4:19 PM, Lu Baolu wrote:
> > > > > Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
> > > > > attached to the device and disabled when the device transitions to block
> > > > > translation mode. This approach is inappropriate as PCI PASID is a device
> > > > > feature independent of the type of the attached domain.
> > > > Reading through other thread, I thought we want to enable both PASID and PRI in
> > > > device probe path. Did I miss something?
> > > PRI is different. PRI should be enabled when the first iopf-capable
> > > domain is attached to device or its PASID, and disabled when the last
> > > such domain is detached.
> > Right. That's what AMD driver also does (We enable it when we attach IOPF
> > capable domain). But looking into pci_enable_pri() :
> > 
> > 
> > 202         /*
> > 203          * VFs must not implement the PRI Capability.  If their PF
> > 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
> > 205          * enabled, it is also enabled for the VF.
> > 206          */
> > 207         if (pdev->is_virtfn) {
> > 208                 if (pci_physfn(pdev)->pri_enabled)
> > 209                         return 0;
> > 210                 return -EINVAL;
> > 211         }
> > 212
> > 
> > 
> > If we try to enable PRI for VF without first enabling it in PF it will fail right?
> > 
> > Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
> > domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
> > AMD v2 page table -OR- nested domain) it will fail right?
> 
> Yeah! So, the iommu driver should basically control the PRI switch on
> the PF whenever someone wants to use it on a VF.

PRI enable sounds like PASID enable to me.

The ATS control is per VF/PF, and PRI does nothing unless ATS returns
a non-present indication.

Like PASID, it seems the purpose of PRI caps is to negotiate if the
CPU can process PRI TLPs globally.

So, I'd guess that just like PASID we should turn it on at PF probe
time if the IOMMU can globall handle PRI.

Enabling ATS will cause PRI TLPs to be sent.

Probably more of this code should be lifted out of the iommu drivers..

Jason

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/19 20:34, Jason Gunthorpe wrote:
> On Mon, Aug 19, 2024 at 03:09:00PM +0800, Baolu Lu wrote:
>> On 2024/8/19 14:34, Vasant Hegde wrote:
>>> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>>>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>>>> attached to the device and disabled when the device transitions to block
>>>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>>>> feature independent of the type of the attached domain.
>>>>> Reading through other thread, I thought we want to enable both PASID and PRI in
>>>>> device probe path. Did I miss something?
>>>> PRI is different. PRI should be enabled when the first iopf-capable
>>>> domain is attached to device or its PASID, and disabled when the last
>>>> such domain is detached.
>>> Right. That's what AMD driver also does (We enable it when we attach IOPF
>>> capable domain). But looking into pci_enable_pri() :
>>>
>>>
>>> 202         /*
>>> 203          * VFs must not implement the PRI Capability.  If their PF
>>> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
>>> 205          * enabled, it is also enabled for the VF.
>>> 206          */
>>> 207         if (pdev->is_virtfn) {
>>> 208                 if (pci_physfn(pdev)->pri_enabled)
>>> 209                         return 0;
>>> 210                 return -EINVAL;
>>> 211         }
>>> 212
>>>
>>>
>>> If we try to enable PRI for VF without first enabling it in PF it will fail right?
>>>
>>> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
>>> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
>>> AMD v2 page table -OR- nested domain) it will fail right?
>> Yeah! So, the iommu driver should basically control the PRI switch on
>> the PF whenever someone wants to use it on a VF.
> PRI enable sounds like PASID enable to me.
> 
> The ATS control is per VF/PF, and PRI does nothing unless ATS returns
> a non-present indication.
> 
> Like PASID, it seems the purpose of PRI caps is to negotiate if the
> CPU can process PRI TLPs globally.
> 
> So, I'd guess that just like PASID we should turn it on at PF probe
> time if the IOMMU can globall handle PRI.
> 
> Enabling ATS will cause PRI TLPs to be sent.
> 
> Probably more of this code should be lifted out of the iommu drivers..

Some architectures, including VT-d non-scalable mode, doesn't support
ATS translation and translated requests when it is working in the
IDENTITY domain mode. In that case, probably PCI ATS still need to be
disabled when such domain is attached and re-enabled when the domain is
detached.

Thanks,
baolu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Vasant Hegde]

Hi All,



On 8/20/2024 9:40 AM, Baolu Lu wrote:
> On 2024/8/19 20:34, Jason Gunthorpe wrote:
>> On Mon, Aug 19, 2024 at 03:09:00PM +0800, Baolu Lu wrote:
>>> On 2024/8/19 14:34, Vasant Hegde wrote:
>>>> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>>>>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>>>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>>>>> attached to the device and disabled when the device transitions to block
>>>>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>>>>> feature independent of the type of the attached domain.
>>>>>> Reading through other thread, I thought we want to enable both PASID and
>>>>>> PRI in
>>>>>> device probe path. Did I miss something?
>>>>> PRI is different. PRI should be enabled when the first iopf-capable
>>>>> domain is attached to device or its PASID, and disabled when the last
>>>>> such domain is detached.
>>>> Right. That's what AMD driver also does (We enable it when we attach IOPF
>>>> capable domain). But looking into pci_enable_pri() :
>>>>
>>>>
>>>> 202         /*
>>>> 203          * VFs must not implement the PRI Capability.  If their PF
>>>> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
>>>> 205          * enabled, it is also enabled for the VF.
>>>> 206          */
>>>> 207         if (pdev->is_virtfn) {
>>>> 208                 if (pci_physfn(pdev)->pri_enabled)
>>>> 209                         return 0;
>>>> 210                 return -EINVAL;
>>>> 211         }
>>>> 212
>>>>
>>>>
>>>> If we try to enable PRI for VF without first enabling it in PF it will fail
>>>> right?
>>>>
>>>> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
>>>> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
>>>> AMD v2 page table -OR- nested domain) it will fail right?
>>> Yeah! So, the iommu driver should basically control the PRI switch on
>>> the PF whenever someone wants to use it on a VF.
>> PRI enable sounds like PASID enable to me.
>>
>> The ATS control is per VF/PF, and PRI does nothing unless ATS returns
>> a non-present indication.
>>
>> Like PASID, it seems the purpose of PRI caps is to negotiate if the
>> CPU can process PRI TLPs globally.
>>
>> So, I'd guess that just like PASID we should turn it on at PF probe
>> time if the IOMMU can globall handle PRI.
>>
>> Enabling ATS will cause PRI TLPs to be sent.
>>
>> Probably more of this code should be lifted out of the iommu drivers..
> 
> Some architectures, including VT-d non-scalable mode, doesn't support
> ATS translation and translated requests when it is working in the
> IDENTITY domain mode. In that case, probably PCI ATS still need to be
> disabled when such domain is attached and re-enabled when the domain is
> detached.

Does it make sense to move both PASID/PRI enablement to probe() path? something
like below :

[I am assuming ops->dev_enable_feat() interface is going away]

  - Enable device side PASID/PRI during ops->probe_device()
  - In device attach path (ops->attach_dev()), depending on IOMMU, device and
domain capability configure the features like PASID, IOPF and ATS. That means
ATS enablement is still done at attach device path.


-Vasant

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Yi Liu]

On 2024/8/20 16:30, Vasant Hegde wrote:
> Hi All,
> 
> 
> 
> On 8/20/2024 9:40 AM, Baolu Lu wrote:
>> On 2024/8/19 20:34, Jason Gunthorpe wrote:
>>> On Mon, Aug 19, 2024 at 03:09:00PM +0800, Baolu Lu wrote:
>>>> On 2024/8/19 14:34, Vasant Hegde wrote:
>>>>> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>>>>>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>>>>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>>>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>>>>>> attached to the device and disabled when the device transitions to block
>>>>>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>>>>>> feature independent of the type of the attached domain.
>>>>>>> Reading through other thread, I thought we want to enable both PASID and
>>>>>>> PRI in
>>>>>>> device probe path. Did I miss something?
>>>>>> PRI is different. PRI should be enabled when the first iopf-capable
>>>>>> domain is attached to device or its PASID, and disabled when the last
>>>>>> such domain is detached.
>>>>> Right. That's what AMD driver also does (We enable it when we attach IOPF
>>>>> capable domain). But looking into pci_enable_pri() :
>>>>>
>>>>>
>>>>> 202         /*
>>>>> 203          * VFs must not implement the PRI Capability.  If their PF
>>>>> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
>>>>> 205          * enabled, it is also enabled for the VF.
>>>>> 206          */
>>>>> 207         if (pdev->is_virtfn) {
>>>>> 208                 if (pci_physfn(pdev)->pri_enabled)
>>>>> 209                         return 0;
>>>>> 210                 return -EINVAL;
>>>>> 211         }
>>>>> 212
>>>>>
>>>>>
>>>>> If we try to enable PRI for VF without first enabling it in PF it will fail
>>>>> right?
>>>>>
>>>>> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
>>>>> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
>>>>> AMD v2 page table -OR- nested domain) it will fail right?
>>>> Yeah! So, the iommu driver should basically control the PRI switch on
>>>> the PF whenever someone wants to use it on a VF.
>>> PRI enable sounds like PASID enable to me.
>>>
>>> The ATS control is per VF/PF, and PRI does nothing unless ATS returns
>>> a non-present indication.
>>>
>>> Like PASID, it seems the purpose of PRI caps is to negotiate if the
>>> CPU can process PRI TLPs globally.
>>>
>>> So, I'd guess that just like PASID we should turn it on at PF probe
>>> time if the IOMMU can globall handle PRI.
>>>
>>> Enabling ATS will cause PRI TLPs to be sent.
>>>
>>> Probably more of this code should be lifted out of the iommu drivers..
>>
>> Some architectures, including VT-d non-scalable mode, doesn't support
>> ATS translation and translated requests when it is working in the
>> IDENTITY domain mode. In that case, probably PCI ATS still need to be
>> disabled when such domain is attached and re-enabled when the domain is
>> detached.
> 
> Does it make sense to move both PASID/PRI enablement to probe() path? something
> like below :
> 
> [I am assuming ops->dev_enable_feat() interface is going away]
> 
>    - Enable device side PASID/PRI during ops->probe_device()
>    - In device attach path (ops->attach_dev()), depending on IOMMU, device and
> domain capability configure the features like PASID, IOPF and ATS. That means
> ATS enablement is still done at attach device path.

Are we sure that it is ok to enable PRI before enabling ATS?

-- 
Regards,
Yi Liu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Jason Gunthorpe]

On Tue, Aug 20, 2024 at 04:55:26PM +0800, Yi Liu wrote:

> Are we sure that it is ok to enable PRI before enabling ATS?

I did not see language in the spec prohibiting this order.

Jason 

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Jason Gunthorpe]

On Tue, Aug 20, 2024 at 02:00:08PM +0530, Vasant Hegde wrote:

> > Some architectures, including VT-d non-scalable mode, doesn't support
> > ATS translation and translated requests when it is working in the
> > IDENTITY domain mode. 

ARM has a similar issue.

ATS enablement should be done when the domain is attached in those
cases.

Arguably you don't want to turn ATS on anyhow for pure IDENTITY with
no PASID because it is just pointless.

> In that case, probably PCI ATS still need to be
> > disabled when such domain is attached and re-enabled when the domain is
> > detached.
> 
> Does it make sense to move both PASID/PRI enablement to probe() path? something
> like below :

It makes sense.

I don't see any ordering restriction in the PCI specification.

Notice that PASID does have a specific called out restriction:

	/*
	 * Note that PASID must be enabled before, and disabled after ATS:
	 * PCI Express Base 4.0r1.0 - 10.5.1.3 ATS Control Register
	 *
	 *   Behavior is undefined if this bit is Set and the value of the PASID
	 *   Enable, Execute Requested Enable, or Privileged Mode Requested bits
	 *   are changed.
	 */

> [I am assuming ops->dev_enable_feat() interface is going away]

Is the plan
 
>   - Enable device side PASID/PRI during ops->probe_device()

Yes

>   - In device attach path (ops->attach_dev()), depending on IOMMU, device and
> domain capability configure the features like PASID, IOPF and ATS. That means
> ATS enablement is still done at attach device path.

From a PCI perspective only ATS can be changed at this point..

The SW construct of IOPF can be changed during domain attachment.

Everything that is PF-only must be setup during probe_device only
otherwise SRIOV VFs will be broken insome cases.

See
https://lore.kernel.org/all/0-v1-0fb4d2ab6770+7e706-ats_vf_jgg@nvidia.com/
for this concept applied to ATS.

This means probe_device() has to do:

 - ATS properties 
 - PRI
 - PASID properties

At a minimum.

It would be nice if the iommu core code did this setup in one place
immediately after calling probe_device() but before attaching a
domain.

There is no particularly good reason to have this coded in all the
iommu drivers.

Jason

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Vasant Hegde]

Jason,


On 8/20/2024 7:21 PM, Jason Gunthorpe wrote:
> On Tue, Aug 20, 2024 at 02:00:08PM +0530, Vasant Hegde wrote:
> 
>>> Some architectures, including VT-d non-scalable mode, doesn't support
>>> ATS translation and translated requests when it is working in the
>>> IDENTITY domain mode. 
> 
> ARM has a similar issue.
> 
> ATS enablement should be done when the domain is attached in those
> cases.
> 
> Arguably you don't want to turn ATS on anyhow for pure IDENTITY with
> no PASID because it is just pointless.
> 
>> In that case, probably PCI ATS still need to be
>>> disabled when such domain is attached and re-enabled when the domain is
>>> detached.
>>
>> Does it make sense to move both PASID/PRI enablement to probe() path? something
>> like below :
> 
> It makes sense.
> 
> I don't see any ordering restriction in the PCI specification.
> 
> Notice that PASID does have a specific called out restriction:
> 
> 	/*
> 	 * Note that PASID must be enabled before, and disabled after ATS:
> 	 * PCI Express Base 4.0r1.0 - 10.5.1.3 ATS Control Register
> 	 *
> 	 *   Behavior is undefined if this bit is Set and the value of the PASID
> 	 *   Enable, Execute Requested Enable, or Privileged Mode Requested bits
> 	 *   are changed.
> 	 */
> 
>> [I am assuming ops->dev_enable_feat() interface is going away]
> 
> Is the plan
>  
>>   - Enable device side PASID/PRI during ops->probe_device()
> 
> Yes
> 
>>   - In device attach path (ops->attach_dev()), depending on IOMMU, device and
>> domain capability configure the features like PASID, IOPF and ATS. That means
>> ATS enablement is still done at attach device path.
> 
> From a PCI perspective only ATS can be changed at this point..
> 
> The SW construct of IOPF can be changed during domain attachment.
> 
> Everything that is PF-only must be setup during probe_device only
> otherwise SRIOV VFs will be broken insome cases.

Makes sense. I will modify AMD driver to enable PRI/PASID in probe() path.


> 
> See
> https://lore.kernel.org/all/0-v1-0fb4d2ab6770+7e706-ats_vf_jgg@nvidia.com/
> for this concept applied to ATS.
> 
> This means probe_device() has to do:
> 
>  - ATS properties 
>  - PRI
>  - PASID properties
> 
> At a minimum.
> 
> It would be nice if the iommu core code did this setup in one place
> immediately after calling probe_device() but before attaching a
> domain.
> 
> There is no particularly good reason to have this coded in all the
> iommu drivers.

Yeah. that makes sense. We may have to adjust few things in driver (at least AMD
driver). But its doable.

-Vasant

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Yi Liu]

On 2024/8/19 14:34, Vasant Hegde wrote:
> Hi,
> 
> 
> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>> attached to the device and disabled when the device transitions to block
>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>> feature independent of the type of the attached domain.
>>> Reading through other thread, I thought we want to enable both PASID and PRI in
>>> device probe path. Did I miss something?
>>
>> PRI is different. PRI should be enabled when the first iopf-capable
>> domain is attached to device or its PASID, and disabled when the last
>> such domain is detached.
> 
> Right. That's what AMD driver also does (We enable it when we attach IOPF
> capable domain). But looking into pci_enable_pri() :
> 
> 
> 202         /*
> 203          * VFs must not implement the PRI Capability.  If their PF
> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
> 205          * enabled, it is also enabled for the VF.
> 206          */
> 207         if (pdev->is_virtfn) {
> 208                 if (pci_physfn(pdev)->pri_enabled)
> 209                         return 0;
> 210                 return -EINVAL;
> 211         }
> 212
> 
> 
> If we try to enable PRI for VF without first enabling it in PF it will fail right?
> 
> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
> AMD v2 page table -OR- nested domain) it will fail right?

It looks like iommufd does not support it for VF. Baolu's iopf series 
disallowed it since v7. And the code in the below acts it.

+	/*
+	 * Once we turn on PCI/PRI support for VF, the response failure code
+	 * should not be forwarded to the hardware due to PRI being a shared
+	 * resource between PF and VFs. There is no coordination for this
+	 * shared capability. This waits for a vPRI reset to recover.
+	 */
+	if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
+		return -EINVAL;


v6: 
https://lore.kernel.org/linux-iommu/20240527040517.38561-1-baolu.lu@linux.intel.com/
  - Refine the attach handle code by shifting the handle allocation to
    the caller. The caller will then provide the allocated handle to the
    domain attachment interfaces.
  - Add reference counter in iommufd_fault_iopf_enable/disable() helpers.
  - Fix the return values of fault FD's read/write fops.
  - Add IOMMU_CAP_USER_IOASID_TABLE capability and check it before roll
    back getting attach_handle to RID.
  - Move the iopf respond queue from iommufd device to iommufd fault.
  - Disallow PRI enablement on SR-IOV VF devices.

-- 
Regards,
Yi Liu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Yi Liu]

On 2024/8/16 18:49, Lu Baolu wrote:
> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
> attached to the device and disabled when the device transitions to block
> translation mode. This approach is inappropriate as PCI PASID is a device
> feature independent of the type of the attached domain.
> 
> Enable PCI PASID during the IOMMU device probe and disables it during the
> release path.
> 
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9ff8b83c19a3..5a8080c71b04 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1322,15 +1322,6 @@ static void iommu_enable_pci_caps(struct device_domain_info *info)
>   		return;
>   
>   	pdev = to_pci_dev(info->dev);
> -
> -	/* The PCIe spec, in its wisdom, declares that the behaviour of
> -	   the device if you enable PASID support after ATS support is
> -	   undefined. So always enable PASID support on devices which
> -	   have it, even if we can't yet know if we're ever going to
> -	   use it. */
> -	if (info->pasid_supported && !pci_enable_pasid(pdev, info->pasid_supported & ~1))
> -		info->pasid_enabled = 1;
> -
>   	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>   		info->ats_enabled = 1;
> @@ -1352,11 +1343,6 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
>   		info->ats_enabled = 0;
>   		domain_update_iotlb(info->domain);
>   	}
> -
> -	if (info->pasid_enabled) {
> -		pci_disable_pasid(pdev);
> -		info->pasid_enabled = 0;
> -	}
>   }
>   
>   static void intel_flush_iotlb_all(struct iommu_domain *domain)
> @@ -4110,6 +4096,16 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
>   		}
>   	}
>   
> +	/*
> +	 * The PCIe spec, in its wisdom, declares that the behaviour of the
> +	 * device is undefined if you enable PASID support after ATS support.
> +	 * So always enable PASID support on devices which have it, even if
> +	 * we can't yet know if we're ever going to use it.
> +	 */
> +	if (info->pasid_supported &&
> +	    !pci_enable_pasid(pdev, info->pasid_supported & ~1))
> +		info->pasid_enabled = 1;
> +
>   	intel_iommu_debugfs_create_dev(info);
>   
>   	return &iommu->iommu;
> @@ -4128,6 +4124,9 @@ static void intel_iommu_release_device(struct device *dev)
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct intel_iommu *iommu = info->iommu;
>   
> +	if (info->pasid_enabled)
> +		pci_disable_pasid(to_pci_dev(dev));
> +

would it make sense to move this behind the
intel_iommu_debugfs_remove_dev(info)? This seems to mirror the order of the
intel_iommu_probe_device(). Or you may set info->pasid_enabled to 0 in case
of any code uses it before info is freed if keeping this order. Otherwise,
lgtm. thanks for the quick action. :)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>   	mutex_lock(&iommu->iopf_lock);
>   	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
>   		device_rbtree_remove(info);

-- 
Regards,
Yi Liu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/19 11:14, Yi Liu wrote:
> On 2024/8/16 18:49, Lu Baolu wrote:
>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>> attached to the device and disabled when the device transitions to block
>> translation mode. This approach is inappropriate as PCI PASID is a device
>> feature independent of the type of the attached domain.
>>
>> Enable PCI PASID during the IOMMU device probe and disables it during the
>> release path.
>>
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 9ff8b83c19a3..5a8080c71b04 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1322,15 +1322,6 @@ static void iommu_enable_pci_caps(struct 
>> device_domain_info *info)
>>           return;
>>       pdev = to_pci_dev(info->dev);
>> -
>> -    /* The PCIe spec, in its wisdom, declares that the behaviour of
>> -       the device if you enable PASID support after ATS support is
>> -       undefined. So always enable PASID support on devices which
>> -       have it, even if we can't yet know if we're ever going to
>> -       use it. */
>> -    if (info->pasid_supported && !pci_enable_pasid(pdev, 
>> info->pasid_supported & ~1))
>> -        info->pasid_enabled = 1;
>> -
>>       if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>           !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>           info->ats_enabled = 1;
>> @@ -1352,11 +1343,6 @@ static void iommu_disable_pci_caps(struct 
>> device_domain_info *info)
>>           info->ats_enabled = 0;
>>           domain_update_iotlb(info->domain);
>>       }
>> -
>> -    if (info->pasid_enabled) {
>> -        pci_disable_pasid(pdev);
>> -        info->pasid_enabled = 0;
>> -    }
>>   }
>>   static void intel_flush_iotlb_all(struct iommu_domain *domain)
>> @@ -4110,6 +4096,16 @@ static struct iommu_device 
>> *intel_iommu_probe_device(struct device *dev)
>>           }
>>       }
>> +    /*
>> +     * The PCIe spec, in its wisdom, declares that the behaviour of the
>> +     * device is undefined if you enable PASID support after ATS 
>> support.
>> +     * So always enable PASID support on devices which have it, even if
>> +     * we can't yet know if we're ever going to use it.
>> +     */
>> +    if (info->pasid_supported &&
>> +        !pci_enable_pasid(pdev, info->pasid_supported & ~1))
>> +        info->pasid_enabled = 1;
>> +
>>       intel_iommu_debugfs_create_dev(info);
>>       return &iommu->iommu;
>> @@ -4128,6 +4124,9 @@ static void intel_iommu_release_device(struct 
>> device *dev)
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct intel_iommu *iommu = info->iommu;
>> +    if (info->pasid_enabled)
>> +        pci_disable_pasid(to_pci_dev(dev));
>> +
> 
> would it make sense to move this behind the
> intel_iommu_debugfs_remove_dev(info)? This seems to mirror the order of the
> intel_iommu_probe_device(). Or you may set info->pasid_enabled to 0 in case
> of any code uses it before info is freed if keeping this order. Otherwise,
> lgtm. thanks for the quick action. 🙂

The info->pasid_enabled change should not impact the behavior of
intel_iommu_debugfs_remove_dev(), and I didn't find any issue during my
test.

Anyway, to make it more consistent with previous behavior, maybe I could
move the part where we turn on/off pasid to the end of the probe and the
start of the release.

Additional change likes below?

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5a8080c71b04..76b317f1d1de 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4096,6 +4096,8 @@ static struct iommu_device 
*intel_iommu_probe_device(struct device *dev)
                 }
         }

+       intel_iommu_debugfs_create_dev(info);
+
         /*
          * The PCIe spec, in its wisdom, declares that the behaviour of the
          * device is undefined if you enable PASID support after ATS 
support.
@@ -4106,8 +4108,6 @@ static struct iommu_device 
*intel_iommu_probe_device(struct device *dev)
             !pci_enable_pasid(pdev, info->pasid_supported & ~1))
                 info->pasid_enabled = 1;

-       intel_iommu_debugfs_create_dev(info);
-
         return &iommu->iommu;
  free_table:
         intel_pasid_free_table(dev);

Thanks,
baolu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Yi Liu]

On 2024/8/19 11:34, Baolu Lu wrote:
> On 2024/8/19 11:14, Yi Liu wrote:
>> On 2024/8/16 18:49, Lu Baolu wrote:
>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>> attached to the device and disabled when the device transitions to block
>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>> feature independent of the type of the attached domain.
>>>
>>> Enable PCI PASID during the IOMMU device probe and disables it during the
>>> release path.
>>>
>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   drivers/iommu/intel/iommu.c | 27 +++++++++++++--------------
>>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 9ff8b83c19a3..5a8080c71b04 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -1322,15 +1322,6 @@ static void iommu_enable_pci_caps(struct 
>>> device_domain_info *info)
>>>           return;
>>>       pdev = to_pci_dev(info->dev);
>>> -
>>> -    /* The PCIe spec, in its wisdom, declares that the behaviour of
>>> -       the device if you enable PASID support after ATS support is
>>> -       undefined. So always enable PASID support on devices which
>>> -       have it, even if we can't yet know if we're ever going to
>>> -       use it. */
>>> -    if (info->pasid_supported && !pci_enable_pasid(pdev, 
>>> info->pasid_supported & ~1))
>>> -        info->pasid_enabled = 1;
>>> -
>>>       if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>>           !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>>           info->ats_enabled = 1;
>>> @@ -1352,11 +1343,6 @@ static void iommu_disable_pci_caps(struct 
>>> device_domain_info *info)
>>>           info->ats_enabled = 0;
>>>           domain_update_iotlb(info->domain);
>>>       }
>>> -
>>> -    if (info->pasid_enabled) {
>>> -        pci_disable_pasid(pdev);
>>> -        info->pasid_enabled = 0;
>>> -    }
>>>   }
>>>   static void intel_flush_iotlb_all(struct iommu_domain *domain)
>>> @@ -4110,6 +4096,16 @@ static struct iommu_device 
>>> *intel_iommu_probe_device(struct device *dev)
>>>           }
>>>       }
>>> +    /*
>>> +     * The PCIe spec, in its wisdom, declares that the behaviour of the
>>> +     * device is undefined if you enable PASID support after ATS support.
>>> +     * So always enable PASID support on devices which have it, even if
>>> +     * we can't yet know if we're ever going to use it.
>>> +     */
>>> +    if (info->pasid_supported &&
>>> +        !pci_enable_pasid(pdev, info->pasid_supported & ~1))
>>> +        info->pasid_enabled = 1;
>>> +
>>>       intel_iommu_debugfs_create_dev(info);
>>>       return &iommu->iommu;
>>> @@ -4128,6 +4124,9 @@ static void intel_iommu_release_device(struct 
>>> device *dev)
>>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>       struct intel_iommu *iommu = info->iommu;
>>> +    if (info->pasid_enabled)
>>> +        pci_disable_pasid(to_pci_dev(dev));
>>> +
>>
>> would it make sense to move this behind the
>> intel_iommu_debugfs_remove_dev(info)? This seems to mirror the order of the
>> intel_iommu_probe_device(). Or you may set info->pasid_enabled to 0 in case
>> of any code uses it before info is freed if keeping this order. Otherwise,
>> lgtm. thanks for the quick action. 🙂
> 
> The info->pasid_enabled change should not impact the behavior of
> intel_iommu_debugfs_remove_dev(), and I didn't find any issue during my
> test.
> 
> Anyway, to make it more consistent with previous behavior, maybe I could
> move the part where we turn on/off pasid to the end of the probe and the
> start of the release.

yeah, this looks ok. And you may consider to clear info->pasid_enabled
when it's disabled. I guess it does not affect device_rbtree_remove(),
intel_pasid_teardown_sm_context(), intel_pasid_free_table() nor the
intel_iommu_debugfs_remove_dev(), but good to clear it as it to
reflect the status.:)

> Additional change likes below?
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 5a8080c71b04..76b317f1d1de 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4096,6 +4096,8 @@ static struct iommu_device 
> *intel_iommu_probe_device(struct device *dev)
>                  }
>          }
> 
> +       intel_iommu_debugfs_create_dev(info);
> +
>          /*
>           * The PCIe spec, in its wisdom, declares that the behaviour of the
>           * device is undefined if you enable PASID support after ATS support.
> @@ -4106,8 +4108,6 @@ static struct iommu_device 
> *intel_iommu_probe_device(struct device *dev)
>              !pci_enable_pasid(pdev, info->pasid_supported & ~1))
>                  info->pasid_enabled = 1;
> 
> -       intel_iommu_debugfs_create_dev(info);
> -
>          return &iommu->iommu;
>   free_table:
>          intel_pasid_free_table(dev);
> 
> Thanks,
> baolu

-- 
Regards,
Yi Liu

Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Baolu Lu]

On 2024/8/19 12:51, Yi Liu wrote:
> On 2024/8/19 11:34, Baolu Lu wrote:
>> On 2024/8/19 11:14, Yi Liu wrote:
>>> On 2024/8/16 18:49, Lu Baolu wrote:
>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu 
>>>> domain is
>>>> attached to the device and disabled when the device transitions to 
>>>> block
>>>> translation mode. This approach is inappropriate as PCI PASID is a 
>>>> device
>>>> feature independent of the type of the attached domain.
>>>>
>>>> Enable PCI PASID during the IOMMU device probe and disables it 
>>>> during the
>>>> release path.
>>>>
>>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>   drivers/iommu/intel/iommu.c | 27 +++++++++++++--------------
>>>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index 9ff8b83c19a3..5a8080c71b04 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -1322,15 +1322,6 @@ static void iommu_enable_pci_caps(struct 
>>>> device_domain_info *info)
>>>>           return;
>>>>       pdev = to_pci_dev(info->dev);
>>>> -
>>>> -    /* The PCIe spec, in its wisdom, declares that the behaviour of
>>>> -       the device if you enable PASID support after ATS support is
>>>> -       undefined. So always enable PASID support on devices which
>>>> -       have it, even if we can't yet know if we're ever going to
>>>> -       use it. */
>>>> -    if (info->pasid_supported && !pci_enable_pasid(pdev, 
>>>> info->pasid_supported & ~1))
>>>> -        info->pasid_enabled = 1;
>>>> -
>>>>       if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>>>           !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>>>           info->ats_enabled = 1;
>>>> @@ -1352,11 +1343,6 @@ static void iommu_disable_pci_caps(struct 
>>>> device_domain_info *info)
>>>>           info->ats_enabled = 0;
>>>>           domain_update_iotlb(info->domain);
>>>>       }
>>>> -
>>>> -    if (info->pasid_enabled) {
>>>> -        pci_disable_pasid(pdev);
>>>> -        info->pasid_enabled = 0;
>>>> -    }
>>>>   }
>>>>   static void intel_flush_iotlb_all(struct iommu_domain *domain)
>>>> @@ -4110,6 +4096,16 @@ static struct iommu_device 
>>>> *intel_iommu_probe_device(struct device *dev)
>>>>           }
>>>>       }
>>>> +    /*
>>>> +     * The PCIe spec, in its wisdom, declares that the behaviour of 
>>>> the
>>>> +     * device is undefined if you enable PASID support after ATS 
>>>> support.
>>>> +     * So always enable PASID support on devices which have it, 
>>>> even if
>>>> +     * we can't yet know if we're ever going to use it.
>>>> +     */
>>>> +    if (info->pasid_supported &&
>>>> +        !pci_enable_pasid(pdev, info->pasid_supported & ~1))
>>>> +        info->pasid_enabled = 1;
>>>> +
>>>>       intel_iommu_debugfs_create_dev(info);
>>>>       return &iommu->iommu;
>>>> @@ -4128,6 +4124,9 @@ static void intel_iommu_release_device(struct 
>>>> device *dev)
>>>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>>       struct intel_iommu *iommu = info->iommu;
>>>> +    if (info->pasid_enabled)
>>>> +        pci_disable_pasid(to_pci_dev(dev));
>>>> +
>>>
>>> would it make sense to move this behind the
>>> intel_iommu_debugfs_remove_dev(info)? This seems to mirror the order 
>>> of the
>>> intel_iommu_probe_device(). Or you may set info->pasid_enabled to 0 
>>> in case
>>> of any code uses it before info is freed if keeping this order. 
>>> Otherwise,
>>> lgtm. thanks for the quick action. 🙂
>>
>> The info->pasid_enabled change should not impact the behavior of
>> intel_iommu_debugfs_remove_dev(), and I didn't find any issue during my
>> test.
>>
>> Anyway, to make it more consistent with previous behavior, maybe I could
>> move the part where we turn on/off pasid to the end of the probe and the
>> start of the release.
> 
> yeah, this looks ok. And you may consider to clear info->pasid_enabled
> when it's disabled. I guess it does not affect device_rbtree_remove(),
> intel_pasid_teardown_sm_context(), intel_pasid_free_table() nor the
> intel_iommu_debugfs_remove_dev(), but good to clear it as it to
> reflect the status.:)

Done.

Thanks,
baolu