[PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Lu Baolu]

When a domain is attached to a device, the required cache tags are
assigned to the domain so that the related caches could be flushed
whenever it is needed. The device TLB cache tag is created selectively 
by checking the ats_enabled field of the device's iommu data. This
creates an ordered dependency between attach and ATS enabling paths.

The device TLB cache tag will not be created if device's ATS is enabled
after the domain attachment. This causes some devices, for example
intel_vpu, to malfunction.

Create device TLB cache tags for a domain as long as the ats_supported
field of the attached device is true. In the cache invalidation paths,
the ats_enable field is checked and the device TLB invalidation requests
are issued only when the ATS is really enabled on the device.

Fixes: 3b1d9e2b2d68 ("iommu/vt-d: Add cache tag assignment interface")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/cache.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
index e8418cdd8331..ec6770f556b9 100644
--- a/drivers/iommu/intel/cache.c
+++ b/drivers/iommu/intel/cache.c
@@ -112,7 +112,7 @@ static int __cache_tag_assign_domain(struct dmar_domain *domain, u16 did,
 	int ret;
 
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_IOTLB);
-	if (ret || !info->ats_enabled)
+	if (ret || !info->ats_supported)
 		return ret;
 
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_DEVTLB);
@@ -129,7 +129,7 @@ static void __cache_tag_unassign_domain(struct dmar_domain *domain, u16 did,
 
 	cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_IOTLB);
 
-	if (info->ats_enabled)
+	if (info->ats_supported)
 		cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_DEVTLB);
 }
 
@@ -140,7 +140,7 @@ static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
 	int ret;
 
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_NESTING_IOTLB);
-	if (ret || !info->ats_enabled)
+	if (ret || !info->ats_supported)
 		return ret;
 
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_NESTING_DEVTLB);
@@ -157,7 +157,7 @@ static void __cache_tag_unassign_parent_domain(struct dmar_domain *domain, u16 d
 
 	cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_NESTING_IOTLB);
 
-	if (info->ats_enabled)
+	if (info->ats_supported)
 		cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_NESTING_DEVTLB);
 }
 
@@ -309,6 +309,9 @@ void cache_tag_flush_range(struct dmar_domain *domain, unsigned long start,
 			info = dev_iommu_priv_get(tag->dev);
 			sid = PCI_DEVID(info->bus, info->devfn);
 
+			if (!info->ats_enabled)
+				break;
+
 			if (tag->pasid == IOMMU_NO_PASID)
 				qi_flush_dev_iotlb(iommu, sid, info->pfsid,
 						   info->ats_qdep, addr, mask);
@@ -356,6 +359,9 @@ void cache_tag_flush_all(struct dmar_domain *domain)
 			info = dev_iommu_priv_get(tag->dev);
 			sid = PCI_DEVID(info->bus, info->devfn);
 
+			if (!info->ats_enabled)
+				break;
+
 			qi_flush_dev_iotlb(iommu, sid, info->pfsid, info->ats_qdep,
 					   0, MAX_AGAW_PFN_WIDTH);
 			quirk_extra_dev_tlb_flush(info, 0, MAX_AGAW_PFN_WIDTH,
-- 
2.34.1

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Jason Gunthorpe]

On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
> When a domain is attached to a device, the required cache tags are
> assigned to the domain so that the related caches could be flushed
> whenever it is needed. The device TLB cache tag is created selectively 
> by checking the ats_enabled field of the device's iommu data. This
> creates an ordered dependency between attach and ATS enabling paths.
> 
> The device TLB cache tag will not be created if device's ATS is enabled
> after the domain attachment. This causes some devices, for example
> intel_vpu, to malfunction.

What? How is this even possible?

ATS is controlled exclusively by the iommu driver, how can it be
changed without the driver knowing??

Jason

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Baolu Lu]

On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
>> When a domain is attached to a device, the required cache tags are
>> assigned to the domain so that the related caches could be flushed
>> whenever it is needed. The device TLB cache tag is created selectively
>> by checking the ats_enabled field of the device's iommu data. This
>> creates an ordered dependency between attach and ATS enabling paths.
>>
>> The device TLB cache tag will not be created if device's ATS is enabled
>> after the domain attachment. This causes some devices, for example
>> intel_vpu, to malfunction.
> What? How is this even possible?
> 
> ATS is controlled exclusively by the iommu driver, how can it be
> changed without the driver knowing??

Yes. ATS is currently controlled exclusively by the iommu driver. The
intel iommu driver enables PCI/ATS on the probe path after the default
domain is attached. That means when the default domain is attached to
the device, the ats_supported is set, but ats_enabled is cleared. So the
cache tag for the device TLB won't be created.

A possible solution is to move ATS enabling to a place before the
default domain attachment. However, this is not future-proof,
considering that we will eventually hand over the ATS control to the
device drivers. Therefore, this fix creates the device TLB cache tags as
long as ats_supported is true and relies on ats_enabled to decide
whether device TLB needs to be invalidated.

Best regards,
baolu

RE: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, June 20, 2024 8:50 AM
> 
> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
> > On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
> >> When a domain is attached to a device, the required cache tags are
> >> assigned to the domain so that the related caches could be flushed
> >> whenever it is needed. The device TLB cache tag is created selectively
> >> by checking the ats_enabled field of the device's iommu data. This
> >> creates an ordered dependency between attach and ATS enabling paths.
> >>
> >> The device TLB cache tag will not be created if device's ATS is enabled
> >> after the domain attachment. This causes some devices, for example
> >> intel_vpu, to malfunction.
> > What? How is this even possible?
> >
> > ATS is controlled exclusively by the iommu driver, how can it be
> > changed without the driver knowing??
> 
> Yes. ATS is currently controlled exclusively by the iommu driver. The
> intel iommu driver enables PCI/ATS on the probe path after the default
> domain is attached. That means when the default domain is attached to
> the device, the ats_supported is set, but ats_enabled is cleared. So the
> cache tag for the device TLB won't be created.

I don't quite get why this is specific to the probe path and the default
domain.

dmar_domain_attach_device()
{
	cache_tag_assign_domain();
	//setup pasid entry for pt/1st/2nd
	iommu_enable_pci_caps();
}

seems that for all domain attaches above is coded in a wrong order
as ats is enabled after the cache tag is assigned. why is it considered
to affect only some devices e.g. intel_vpu?

> 
> A possible solution is to move ATS enabling to a place before the
> default domain attachment. However, this is not future-proof,
> considering that we will eventually hand over the ATS control to the
> device drivers. Therefore, this fix creates the device TLB cache tags as
> long as ats_supported is true and relies on ats_enabled to decide
> whether device TLB needs to be invalidated.
> 

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Baolu Lu]

On 6/20/24 11:04 AM, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Thursday, June 20, 2024 8:50 AM
>>
>> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
>>> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
>>>> When a domain is attached to a device, the required cache tags are
>>>> assigned to the domain so that the related caches could be flushed
>>>> whenever it is needed. The device TLB cache tag is created selectively
>>>> by checking the ats_enabled field of the device's iommu data. This
>>>> creates an ordered dependency between attach and ATS enabling paths.
>>>>
>>>> The device TLB cache tag will not be created if device's ATS is enabled
>>>> after the domain attachment. This causes some devices, for example
>>>> intel_vpu, to malfunction.
>>> What? How is this even possible?
>>>
>>> ATS is controlled exclusively by the iommu driver, how can it be
>>> changed without the driver knowing??
>> Yes. ATS is currently controlled exclusively by the iommu driver. The
>> intel iommu driver enables PCI/ATS on the probe path after the default
>> domain is attached. That means when the default domain is attached to
>> the device, the ats_supported is set, but ats_enabled is cleared. So the
>> cache tag for the device TLB won't be created.
> I don't quite get why this is specific to the probe path and the default
> domain.

The issue is with the domain attaching device path, not specific to the
probe or default domain.

> 
> dmar_domain_attach_device()
> {
> 	cache_tag_assign_domain();
> 	//setup pasid entry for pt/1st/2nd
> 	iommu_enable_pci_caps();
> }
> 
> seems that for all domain attaches above is coded in a wrong order
> as ats is enabled after the cache tag is assigned.

Yes, exactly. But simply changing the order isn't future-proof,
considering ATS control will eventually be moved out of iommu drivers.

> why is it considered
> to affect only some devices e.g. intel_vpu?

This bug was discovered during testing of the intel_vpu device and
affects devices other than the intel_vpu. The commit message is a bit
confusing. :-)

Best regards,
baolu

RE: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, June 20, 2024 11:14 AM
> 
> On 6/20/24 11:04 AM, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@linux.intel.com>
> >> Sent: Thursday, June 20, 2024 8:50 AM
> >>
> >> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
> >>> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
> >>>> When a domain is attached to a device, the required cache tags are
> >>>> assigned to the domain so that the related caches could be flushed
> >>>> whenever it is needed. The device TLB cache tag is created selectively
> >>>> by checking the ats_enabled field of the device's iommu data. This
> >>>> creates an ordered dependency between attach and ATS enabling paths.
> >>>>
> >>>> The device TLB cache tag will not be created if device's ATS is enabled
> >>>> after the domain attachment. This causes some devices, for example
> >>>> intel_vpu, to malfunction.
> >>> What? How is this even possible?
> >>>
> >>> ATS is controlled exclusively by the iommu driver, how can it be
> >>> changed without the driver knowing??
> >> Yes. ATS is currently controlled exclusively by the iommu driver. The
> >> intel iommu driver enables PCI/ATS on the probe path after the default
> >> domain is attached. That means when the default domain is attached to
> >> the device, the ats_supported is set, but ats_enabled is cleared. So the
> >> cache tag for the device TLB won't be created.
> > I don't quite get why this is specific to the probe path and the default
> > domain.
> 
> The issue is with the domain attaching device path, not specific to the
> probe or default domain.
> 
> >
> > dmar_domain_attach_device()
> > {
> > 	cache_tag_assign_domain();
> > 	//setup pasid entry for pt/1st/2nd
> > 	iommu_enable_pci_caps();
> > }
> >
> > seems that for all domain attaches above is coded in a wrong order
> > as ats is enabled after the cache tag is assigned.
> 
> Yes, exactly. But simply changing the order isn't future-proof,
> considering ATS control will eventually be moved out of iommu drivers.

I'm not sure the way this patch goes is future-proof either. Ideally the devtlb
cache tag should always be assigned together with enabling the ats capability
then in that case we won't have a case assigning a tag upon info->ats_supported
at attach and then check info->ats_enabled run-time for flush.

and order-wise it makes slightly more sense to assign cache tag at end of 
the attach function after all other configurations have been completed.

with that I prefer to a simple fix by reverting the order instead of adding
unnecessary run-time overhead for unclear future. 😊

> 
> > why is it considered
> > to affect only some devices e.g. intel_vpu?
> 
> This bug was discovered during testing of the intel_vpu device and
> affects devices other than the intel_vpu. The commit message is a bit
> confusing. :-)
> 
> Best regards,
> baolu

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Vasant Hegde]

Hi Baolu,

On 6/20/2024 8:43 AM, Baolu Lu wrote:
> On 6/20/24 11:04 AM, Tian, Kevin wrote:
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Thursday, June 20, 2024 8:50 AM
>>>
>>> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
>>>> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
>>>>> When a domain is attached to a device, the required cache tags are
>>>>> assigned to the domain so that the related caches could be flushed
>>>>> whenever it is needed. The device TLB cache tag is created selectively
>>>>> by checking the ats_enabled field of the device's iommu data. This
>>>>> creates an ordered dependency between attach and ATS enabling paths.
>>>>>
>>>>> The device TLB cache tag will not be created if device's ATS is enabled
>>>>> after the domain attachment. This causes some devices, for example
>>>>> intel_vpu, to malfunction.
>>>> What? How is this even possible?
>>>>
>>>> ATS is controlled exclusively by the iommu driver, how can it be
>>>> changed without the driver knowing??
>>> Yes. ATS is currently controlled exclusively by the iommu driver. The
>>> intel iommu driver enables PCI/ATS on the probe path after the default
>>> domain is attached. That means when the default domain is attached to
>>> the device, the ats_supported is set, but ats_enabled is cleared. So the
>>> cache tag for the device TLB won't be created.
>> I don't quite get why this is specific to the probe path and the default
>> domain.
> 
> The issue is with the domain attaching device path, not specific to the
> probe or default domain.
> 
>>
>> dmar_domain_attach_device()
>> {
>>     cache_tag_assign_domain();
>>     //setup pasid entry for pt/1st/2nd
>>     iommu_enable_pci_caps();
>> }
>>
>> seems that for all domain attaches above is coded in a wrong order
>> as ats is enabled after the cache tag is assigned.
> 
> Yes, exactly. But simply changing the order isn't future-proof,
> considering ATS control will eventually be moved out of iommu drivers.

[Unrelated to this patch]

You mean ATS setup will be moved to individual device driver? Is there any
reason for that?


-Vasant

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Baolu Lu]

On 2024/6/20 11:57, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Thursday, June 20, 2024 11:14 AM
>>
>> On 6/20/24 11:04 AM, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Thursday, June 20, 2024 8:50 AM
>>>>
>>>> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
>>>>> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
>>>>>> When a domain is attached to a device, the required cache tags are
>>>>>> assigned to the domain so that the related caches could be flushed
>>>>>> whenever it is needed. The device TLB cache tag is created selectively
>>>>>> by checking the ats_enabled field of the device's iommu data. This
>>>>>> creates an ordered dependency between attach and ATS enabling paths.
>>>>>>
>>>>>> The device TLB cache tag will not be created if device's ATS is enabled
>>>>>> after the domain attachment. This causes some devices, for example
>>>>>> intel_vpu, to malfunction.
>>>>> What? How is this even possible?
>>>>>
>>>>> ATS is controlled exclusively by the iommu driver, how can it be
>>>>> changed without the driver knowing??
>>>> Yes. ATS is currently controlled exclusively by the iommu driver. The
>>>> intel iommu driver enables PCI/ATS on the probe path after the default
>>>> domain is attached. That means when the default domain is attached to
>>>> the device, the ats_supported is set, but ats_enabled is cleared. So the
>>>> cache tag for the device TLB won't be created.
>>> I don't quite get why this is specific to the probe path and the default
>>> domain.
>> The issue is with the domain attaching device path, not specific to the
>> probe or default domain.
>>
>>> dmar_domain_attach_device()
>>> {
>>> 	cache_tag_assign_domain();
>>> 	//setup pasid entry for pt/1st/2nd
>>> 	iommu_enable_pci_caps();
>>> }
>>>
>>> seems that for all domain attaches above is coded in a wrong order
>>> as ats is enabled after the cache tag is assigned.
>> Yes, exactly. But simply changing the order isn't future-proof,
>> considering ATS control will eventually be moved out of iommu drivers.
> I'm not sure the way this patch goes is future-proof either. Ideally the devtlb
> cache tag should always be assigned together with enabling the ats capability
> then in that case we won't have a case assigning a tag upon info->ats_supported
> at attach and then check info->ats_enabled run-time for flush.

Yes, creating the required cache tag when enabling the ATS feature makes
more sense.

> 
> and order-wise it makes slightly more sense to assign cache tag at end of
> the attach function after all other configurations have been completed.
> 
> with that I prefer to a simple fix by reverting the order instead of adding
> unnecessary run-time overhead for unclear future. 😊

Okay, I will follow this way to fix it.

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Baolu Lu]

On 2024/6/20 13:54, Vasant Hegde wrote:
> On 6/20/2024 8:43 AM, Baolu Lu wrote:
>> On 6/20/24 11:04 AM, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Thursday, June 20, 2024 8:50 AM
>>>>
>>>> On 6/20/24 12:46 AM, Jason Gunthorpe wrote:
>>>>> On Wed, Jun 19, 2024 at 09:53:45AM +0800, Lu Baolu wrote:
>>>>>> When a domain is attached to a device, the required cache tags are
>>>>>> assigned to the domain so that the related caches could be flushed
>>>>>> whenever it is needed. The device TLB cache tag is created selectively
>>>>>> by checking the ats_enabled field of the device's iommu data. This
>>>>>> creates an ordered dependency between attach and ATS enabling paths.
>>>>>>
>>>>>> The device TLB cache tag will not be created if device's ATS is enabled
>>>>>> after the domain attachment. This causes some devices, for example
>>>>>> intel_vpu, to malfunction.
>>>>> What? How is this even possible?
>>>>>
>>>>> ATS is controlled exclusively by the iommu driver, how can it be
>>>>> changed without the driver knowing??
>>>> Yes. ATS is currently controlled exclusively by the iommu driver. The
>>>> intel iommu driver enables PCI/ATS on the probe path after the default
>>>> domain is attached. That means when the default domain is attached to
>>>> the device, the ats_supported is set, but ats_enabled is cleared. So the
>>>> cache tag for the device TLB won't be created.
>>> I don't quite get why this is specific to the probe path and the default
>>> domain.
>> The issue is with the domain attaching device path, not specific to the
>> probe or default domain.
>>
>>> dmar_domain_attach_device()
>>> {
>>>      cache_tag_assign_domain();
>>>      //setup pasid entry for pt/1st/2nd
>>>      iommu_enable_pci_caps();
>>> }
>>>
>>> seems that for all domain attaches above is coded in a wrong order
>>> as ats is enabled after the cache tag is assigned.
>> Yes, exactly. But simply changing the order isn't future-proof,
>> considering ATS control will eventually be moved out of iommu drivers.
> [Unrelated to this patch]
> 
> You mean ATS setup will be moved to individual device driver? Is there any
> reason for that?

Not exactly to individual device drivers, but it should be out of the
iommu drivers.

https://lore.kernel.org/linux-iommu/BL1PR12MB51441FC4303BD0442EDB7A9CF7FFA@BL1PR12MB5144.namprd12.prod.outlook.com/

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Vasant Hegde]

Hi,


On 6/20/2024 11:57 AM, Baolu Lu wrote:
> On 2024/6/20 13:54, Vasant Hegde wrote:
>> On 6/20/2024 8:43 AM, Baolu Lu wrote:
>>> On 6/20/24 11:04 AM, Tian, Kevin wrote:
>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>>> Sent: Thursday, June 20, 2024 8:50 AM
>>>>>


.../...

>>>>
>>>> seems that for all domain attaches above is coded in a wrong order
>>>> as ats is enabled after the cache tag is assigned.
>>> Yes, exactly. But simply changing the order isn't future-proof,
>>> considering ATS control will eventually be moved out of iommu drivers.
>> [Unrelated to this patch]
>>
>> You mean ATS setup will be moved to individual device driver? Is there any
>> reason for that?
> 
> Not exactly to individual device drivers, but it should be out of the
> iommu drivers.
> 
> https://lore.kernel.org/linux-iommu/BL1PR12MB51441FC4303BD0442EDB7A9CF7FFA@BL1PR12MB5144.namprd12.prod.outlook.com/

Got it. Thanks.

I remember of this discussion. May be we can provide API from IOMMU driver so
that individual driver can enable/disable ATS (like iommu_dev_enable_feature()).

-Vasant

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Jason Gunthorpe]

On Thu, Jun 20, 2024 at 04:19:46PM +0530, Vasant Hegde wrote:
> >>>> seems that for all domain attaches above is coded in a wrong order
> >>>> as ats is enabled after the cache tag is assigned.
> >>> Yes, exactly. But simply changing the order isn't future-proof,
> >>> considering ATS control will eventually be moved out of iommu drivers.
> >> [Unrelated to this patch]
> >>
> >> You mean ATS setup will be moved to individual device driver? Is there any
> >> reason for that?
> > 
> > Not exactly to individual device drivers, but it should be out of the
> > iommu drivers.
> > 
> > https://lore.kernel.org/linux-iommu/BL1PR12MB51441FC4303BD0442EDB7A9CF7FFA@BL1PR12MB5144.namprd12.prod.outlook.com/
> 
> Got it. Thanks.
> 
> I remember of this discussion. May be we can provide API from IOMMU driver so
> that individual driver can enable/disable ATS (like iommu_dev_enable_feature()).

But I have a feeling if we do that it should be done by re-attaching
the domain.

For instance if you look at how I structued SMMUv3, the ATSness is an
effective property of the domain type and ATS switches on and off
dynamically already.

Having an additional input to domain attach "inhibit ats", as a flag
would be all the support the driver would need to provide for the core
code to manage this with some kind of global policy.

I would suggest to steer VTD in that direction too and make the ATS
enable be done on domain attach, and put the first ATS enable in
attach, not in probe. The logic in smmuv3 would apply just as well to
VTD, though you'd need the hitless update logic too :)

Jason

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Baolu Lu]

On 6/20/24 10:08 PM, Jason Gunthorpe wrote:
> On Thu, Jun 20, 2024 at 04:19:46PM +0530, Vasant Hegde wrote:
>>>>>> seems that for all domain attaches above is coded in a wrong order
>>>>>> as ats is enabled after the cache tag is assigned.
>>>>> Yes, exactly. But simply changing the order isn't future-proof,
>>>>> considering ATS control will eventually be moved out of iommu drivers.
>>>> [Unrelated to this patch]
>>>>
>>>> You mean ATS setup will be moved to individual device driver? Is there any
>>>> reason for that?
>>> Not exactly to individual device drivers, but it should be out of the
>>> iommu drivers.
>>>
>>> https://lore.kernel.org/linux-iommu/BL1PR12MB51441FC4303BD0442EDB7A9CF7FFA@BL1PR12MB5144.namprd12.prod.outlook.com/
>> Got it. Thanks.
>>
>> I remember of this discussion. May be we can provide API from IOMMU driver so
>> that individual driver can enable/disable ATS (like iommu_dev_enable_feature()).
> But I have a feeling if we do that it should be done by re-attaching
> the domain.
> 
> For instance if you look at how I structued SMMUv3, the ATSness is an
> effective property of the domain type and ATS switches on and off
> dynamically already.
> 
> Having an additional input to domain attach "inhibit ats", as a flag
> would be all the support the driver would need to provide for the core
> code to manage this with some kind of global policy.
> 
> I would suggest to steer VTD in that direction too and make the ATS
> enable be done on domain attach, and put the first ATS enable in
> attach, not in probe. The logic in smmuv3 would apply just as well to
> VTD, though you'd need the hitless update logic too 🙂

Enabling/Disabling ATS on domain attach seems like a feasible approach.
The ATS requirement information (required/disallowed/neutral) could be
included as an opt-in option in the domain attach path. This likely
applies to PASID attachments and VF/PF devices. The iommu driver
maintains the per-device ATS refcount and enables it for the first
request and disables it after the last one. The attachment fails
accordingly if the hardware capability doesn't match the domain attach
requirement.

Perhaps we could further include PRI as a domain attach option,
indicating that the domain requires IOPF functionality. This would allow
us to simplify the SVA and IOMMUFD by hiding device and IOMMU details
within the IOMMU driver.

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Jason Gunthorpe]

On Fri, Jun 21, 2024 at 09:44:27AM +0800, Baolu Lu wrote:

> Enabling/Disabling ATS on domain attach seems like a feasible approach.
> The ATS requirement information (required/disallowed/neutral) could be
> included as an opt-in option in the domain attach path. This likely
> applies to PASID attachments and VF/PF devices. The iommu driver
> maintains the per-device ATS refcount and enables it for the first
> request and disables it after the last one. The attachment fails
> accordingly if the hardware capability doesn't match the domain attach
> requirement.

Yes. if we do this the core code would have to take some
responsibility to manage ATS and PASID needs together. It is a bit
tricky potentially, but probably better than having drivers repeat the
same logic.

But it probably does require the drivers actually implement hitless
replace. I'd imagin a core driven flow is something like:

 - Install an identity domain without ATS
 - Decide we want a SVA pasid
 - Replace the identiy domain with the same identity domain and ATS
 - Install the SVA pasid

(assuming we want some policy like only enable ATS when needed)

> Perhaps we could further include PRI as a domain attach option,
> indicating that the domain requires IOPF functionality. This would allow
> us to simplify the SVA and IOMMUFD by hiding device and IOMMU details
> within the IOMMU driver.

This is my desire, if the domain has a fault handler then the driver
should just make PRI work on attach or fail attach.

None of the weird feature stuff is needed. We are getting closer, I
think the SVA enable stuff is almost all NO-OP'd.

Jason

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Vasant Hegde]

Jason,


On 6/20/2024 7:38 PM, Jason Gunthorpe wrote:
> On Thu, Jun 20, 2024 at 04:19:46PM +0530, Vasant Hegde wrote:
>>>>>> seems that for all domain attaches above is coded in a wrong order
>>>>>> as ats is enabled after the cache tag is assigned.
>>>>> Yes, exactly. But simply changing the order isn't future-proof,
>>>>> considering ATS control will eventually be moved out of iommu drivers.
>>>> [Unrelated to this patch]
>>>>
>>>> You mean ATS setup will be moved to individual device driver? Is there any
>>>> reason for that?
>>>
>>> Not exactly to individual device drivers, but it should be out of the
>>> iommu drivers.
>>>
>>> https://lore.kernel.org/linux-iommu/BL1PR12MB51441FC4303BD0442EDB7A9CF7FFA@BL1PR12MB5144.namprd12.prod.outlook.com/
>>
>> Got it. Thanks.
>>
>> I remember of this discussion. May be we can provide API from IOMMU driver so
>> that individual driver can enable/disable ATS (like iommu_dev_enable_feature()).
> 
> But I have a feeling if we do that it should be done by re-attaching
> the domain.

Right. By default IOMMU will enable the ATS.. But if device driver decides to
disable it (say due to HW bug), then we have to re-attach domain (in AMD case we
have to update the DTE and flush the TLB).


> 
> For instance if you look at how I structued SMMUv3, the ATSness is an
> effective property of the domain type and ATS switches on and off
> dynamically already.

In AMD case its per device property. We check both IOMMU capability and device
property before enabling ATS.

> 
> Having an additional input to domain attach "inhibit ats", as a flag
> would be all the support the driver would need to provide for the core
> code to manage this with some kind of global policy.

You mean for attach_dev() ?

.. and what about PRI and PASID? Don't device driver needs similar interface ?


-Vasant

Re: [PATCH 1/1] iommu/vt-d: Fix missed device TLB cache tag

[Jason Gunthorpe]

On Mon, Jun 24, 2024 at 07:56:03PM +0530, Vasant Hegde wrote:

> >> I remember of this discussion. May be we can provide API from IOMMU driver so
> >> that individual driver can enable/disable ATS (like iommu_dev_enable_feature()).
> > 
> > But I have a feeling if we do that it should be done by re-attaching
> > the domain.
> 
> Right. By default IOMMU will enable the ATS.. But if device driver decides to
> disable it (say due to HW bug), then we have to re-attach domain (in AMD case we
> have to update the DTE and flush the TLB).

SMMUv3 is the same, this is why I like doing it via the existing
re-attach domain interface since it already composes nicely with the
mechanisms the driver should have to update the DTE/etc.

> > Having an additional input to domain attach "inhibit ats", as a flag
> > would be all the support the driver would need to provide for the core
> > code to manage this with some kind of global policy.
> 
> You mean for attach_dev() ?

Yes
 
> .. and what about PRI and PASID? Don't device driver needs similar interface ?

PRI is enabled by attaching a PRI capable domain

PASID should be assumed to be required if the device supports PASID.

Inhibiting PASID support would have to be done during domain
allocation. Soon the dev will always be available there and we could
do a policy like that via the dev structs? Not sure how much value
there is..

Jason