[PATCH 1/2] iommu/vt-d: Convert dmar_ats_supported() to return bool

[PATCH 1/2] iommu/vt-d: Convert dmar_ats_supported() to return bool

[Lu Baolu]

dmar_ats_supported() returns an integer that is used as a boolean. Since
it all it needs is to return true or false, change the return type from
int to bool to make it a bit more readable and obvious.

Cleanup this helper accordingly with no functional change intended.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 38cda454fc64..07e394dfccc1 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3043,15 +3043,15 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
 	return satcu;
 }
 
-static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
+static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 {
-	int i, ret = 1;
-	struct pci_bus *bus;
 	struct pci_dev *bridge = NULL;
-	struct device *tmp;
-	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
 	struct dmar_satc_unit *satcu;
+	struct acpi_dmar_atsr *atsr;
+	struct pci_bus *bus;
+	struct device *tmp;
+	int i;
 
 	dev = pci_physfn(dev);
 	satcu = dmar_find_matched_satc_unit(dev);
@@ -3069,11 +3069,11 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 		bridge = bus->self;
 		/* If it's an integrated device, allow ATS */
 		if (!bridge)
-			return 1;
+			return true;
 		/* Connected via non-PCIe: no ATS */
 		if (!pci_is_pcie(bridge) ||
 		    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
-			return 0;
+			return false;
 		/* If we found the root port, look it up in the ATSR */
 		if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT)
 			break;
@@ -3085,18 +3085,21 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 		if (atsr->segment != pci_domain_nr(dev->bus))
 			continue;
 
-		for_each_dev_scope(atsru->devices, atsru->devices_cnt, i, tmp)
-			if (tmp == &bridge->dev)
-				goto out;
+		for_each_dev_scope(atsru->devices, atsru->devices_cnt, i, tmp) {
+			if (tmp == &bridge->dev) {
+				rcu_read_unlock();
+				return true;
+			}
+		}
 
-		if (atsru->include_all)
-			goto out;
+		if (atsru->include_all) {
+			rcu_read_unlock();
+			return true;
+		}
 	}
-	ret = 0;
-out:
 	rcu_read_unlock();
 
-	return ret;
+	return false;
 }
 
 int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
-- 
2.34.1

[PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Lu Baolu]

If a device is listed in the SATC table with ATC_REQUIRED flag set, it
indicates that the device has a functional requirement to enable its ATC
(via the ATS capability) for device operation. However, when IOMMU is
running in the legacy mode, ATS could be automatically supported by the
hardware so that the OS has no need to support the ATS functionality.

This is a backward compatibility feature which enables older OSs. Since
Linux VT-d implementation has already supported ATS features for a long
time, there is no need to rely on this compatibility hardware. Remove it
to make the driver future-proof.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 07e394dfccc1..b63347c8bf5d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 	dev = pci_physfn(dev);
 	satcu = dmar_find_matched_satc_unit(dev);
 	if (satcu)
-		/*
-		 * This device supports ATS as it is in SATC table.
-		 * When IOMMU is in legacy mode, enabling ATS is done
-		 * automatically by HW for the device that requires
-		 * ATS, hence OS should not enable this device ATS
-		 * to avoid duplicated TLB invalidation.
-		 */
-		return !(satcu->atc_required && !sm_supported(iommu));
+		return true;
 
 	for (bus = dev->bus; bus; bus = bus->parent) {
 		bridge = bus->self;
-- 
2.34.1

Re: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Yi Liu]

Hi Baolu,

On 2024/6/24 13:25, Lu Baolu wrote:
> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
> indicates that the device has a functional requirement to enable its ATC
> (via the ATS capability) for device operation. However, when IOMMU is
> running in the legacy mode, ATS could be automatically supported by the
> hardware so that the OS has no need to support the ATS functionality.

It seems like if VT-d hw is operating in legacy mode, ATS is enabled
automatically? This is really suprising. Maybe I got it wrongly. Could you
elaborate a bit?

> This is a backward compatibility feature which enables older OSs. Since
> Linux VT-d implementation has already supported ATS features for a long
> time, there is no need to rely on this compatibility hardware. Remove it
> to make the driver future-proof.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 07e394dfccc1..b63347c8bf5d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   	dev = pci_physfn(dev);
>   	satcu = dmar_find_matched_satc_unit(dev);
>   	if (satcu)
> -		/*
> -		 * This device supports ATS as it is in SATC table.
> -		 * When IOMMU is in legacy mode, enabling ATS is done
> -		 * automatically by HW for the device that requires
> -		 * ATS, hence OS should not enable this device ATS
> -		 * to avoid duplicated TLB invalidation.
> -		 */
> -		return !(satcu->atc_required && !sm_supported(iommu));
> +		return true;
>   
>   	for (bus = dev->bus; bus; bus = bus->parent) {
>   		bridge = bus->self;

-- 
Regards,
Yi Liu

Re: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Baolu Lu]

On 2024/6/24 16:06, Yi Liu wrote:
> On 2024/6/24 13:25, Lu Baolu wrote:
>> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
>> indicates that the device has a functional requirement to enable its ATC
>> (via the ATS capability) for device operation. However, when IOMMU is
>> running in the legacy mode, ATS could be automatically supported by the
>> hardware so that the OS has no need to support the ATS functionality.
> 
> It seems like if VT-d hw is operating in legacy mode, ATS is enabled
> automatically? 

Yes.

> This is really suprising. Maybe I got it wrongly. Could you
> elaborate a bit?

It's for backward compatible purpose, but I have no hardware details.

Best regards,
baolu

Re: [PATCH 1/2] iommu/vt-d: Convert dmar_ats_supported() to return bool

[Yi Liu]

On 2024/6/24 13:25, Lu Baolu wrote:
> dmar_ats_supported() returns an integer that is used as a boolean. Since
> it all it needs is to return true or false, change the return type from

nit: a redundant "it" is used here.

> int to bool to make it a bit more readable and obvious.
> 
> Cleanup this helper accordingly with no functional change intended.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 38cda454fc64..07e394dfccc1 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3043,15 +3043,15 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
>   	return satcu;
>   }
>   
> -static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
> +static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   {
> -	int i, ret = 1;
> -	struct pci_bus *bus;
>   	struct pci_dev *bridge = NULL;
> -	struct device *tmp;
> -	struct acpi_dmar_atsr *atsr;
>   	struct dmar_atsr_unit *atsru;
>   	struct dmar_satc_unit *satcu;
> +	struct acpi_dmar_atsr *atsr;
> +	struct pci_bus *bus;
> +	struct device *tmp;
> +	int i;
>   
>   	dev = pci_physfn(dev);
>   	satcu = dmar_find_matched_satc_unit(dev);
> @@ -3069,11 +3069,11 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   		bridge = bus->self;
>   		/* If it's an integrated device, allow ATS */
>   		if (!bridge)
> -			return 1;
> +			return true;
>   		/* Connected via non-PCIe: no ATS */
>   		if (!pci_is_pcie(bridge) ||
>   		    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
> -			return 0;
> +			return false;
>   		/* If we found the root port, look it up in the ATSR */
>   		if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT)
>   			break;
> @@ -3085,18 +3085,21 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   		if (atsr->segment != pci_domain_nr(dev->bus))
>   			continue;
>   
> -		for_each_dev_scope(atsru->devices, atsru->devices_cnt, i, tmp)
> -			if (tmp == &bridge->dev)
> -				goto out;
> +		for_each_dev_scope(atsru->devices, atsru->devices_cnt, i, tmp) {
> +			if (tmp == &bridge->dev) {
> +				rcu_read_unlock();
> +				return true;
> +			}
> +		}
>   
> -		if (atsru->include_all)
> -			goto out;
> +		if (atsru->include_all) {
> +			rcu_read_unlock();
> +			return true;

It appears to me that two gotos is better than opening code. :) Anyhow.

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


> +		}
>   	}
> -	ret = 0;
> -out:
>   	rcu_read_unlock();
>   
> -	return ret;
> +	return false;
>   }
>   
>   int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)

-- 
Regards,
Yi Liu

RE: [PATCH 1/2] iommu/vt-d: Convert dmar_ats_supported() to return bool

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, June 24, 2024 1:25 PM
> 
> dmar_ats_supported() returns an integer that is used as a boolean. Since
> it all it needs is to return true or false, change the return type from
> int to bool to make it a bit more readable and obvious.
> 
> Cleanup this helper accordingly with no functional change intended.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, June 24, 2024 1:25 PM
> 
> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
> indicates that the device has a functional requirement to enable its ATC
> (via the ATS capability) for device operation. However, when IOMMU is
> running in the legacy mode, ATS could be automatically supported by the
> hardware so that the OS has no need to support the ATS functionality.

hmm I don't think "has no need to support" matches...

> 
> This is a backward compatibility feature which enables older OSs. Since
> Linux VT-d implementation has already supported ATS features for a long
> time, there is no need to rely on this compatibility hardware. Remove it
> to make the driver future-proof.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 07e394dfccc1..b63347c8bf5d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev
> *dev, struct intel_iommu *iommu)
>  	dev = pci_physfn(dev);
>  	satcu = dmar_find_matched_satc_unit(dev);
>  	if (satcu)
> -		/*
> -		 * This device supports ATS as it is in SATC table.
> -		 * When IOMMU is in legacy mode, enabling ATS is done
> -		 * automatically by HW for the device that requires
> -		 * ATS, hence OS should not enable this device ATS
> -		 * to avoid duplicated TLB invalidation.
> -		 */

...what above comment tries to convey.

If this comment is valid, it's not about whether the OS itself supports
ATS. instead it's a requirement for the OS to not manage ATS when
it's already managed by HW.

Unless there is a way to disable hw management with this change...

> -		return !(satcu->atc_required && !sm_supported(iommu));
> +		return true;
> 
>  	for (bus = dev->bus; bus; bus = bus->parent) {
>  		bridge = bus->self;
> --
> 2.34.1

Re: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Baolu Lu]

On 2024/6/25 10:32, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Monday, June 24, 2024 1:25 PM
>>
>> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
>> indicates that the device has a functional requirement to enable its ATC
>> (via the ATS capability) for device operation. However, when IOMMU is
>> running in the legacy mode, ATS could be automatically supported by the
>> hardware so that the OS has no need to support the ATS functionality.
> hmm I don't think "has no need to support" matches...
> 
>> This is a backward compatibility feature which enables older OSs. Since
>> Linux VT-d implementation has already supported ATS features for a long
>> time, there is no need to rely on this compatibility hardware. Remove it
>> to make the driver future-proof.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 07e394dfccc1..b63347c8bf5d 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev
>> *dev, struct intel_iommu *iommu)
>>   	dev = pci_physfn(dev);
>>   	satcu = dmar_find_matched_satc_unit(dev);
>>   	if (satcu)
>> -		/*
>> -		 * This device supports ATS as it is in SATC table.
>> -		 * When IOMMU is in legacy mode, enabling ATS is done
>> -		 * automatically by HW for the device that requires
>> -		 * ATS, hence OS should not enable this device ATS
>> -		 * to avoid duplicated TLB invalidation.
>> -		 */
> ...what above comment tries to convey.
> 
> If this comment is valid, it's not about whether the OS itself supports
> ATS. instead it's a requirement for the OS to not manage ATS when
> it's already managed by HW.
> 
> Unless there is a way to disable hw management with this change...

This comment is not correct. The hardware automatic ATS is for older OS
compatible purposes, where the ATS is not aware of by the OS yet, but
ATS is functionally required for some SOC-integrated accelerators.

The HAS specification for those platforms states that OSs supporting ATS
(so-called enlightened OSs) don't require automatic ATS anymore.

 From the iommu driver's point of view, automatic ATS is not part of the
VT-d spec and also not enumerable, hence it should be transparent.

Best regards,
baolu

RE: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, June 25, 2024 4:40 PM
> 
> On 2024/6/25 10:32, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@linux.intel.com>
> >> Sent: Monday, June 24, 2024 1:25 PM
> >>
> >> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
> >> indicates that the device has a functional requirement to enable its ATC
> >> (via the ATS capability) for device operation. However, when IOMMU is
> >> running in the legacy mode, ATS could be automatically supported by the
> >> hardware so that the OS has no need to support the ATS functionality.
> > hmm I don't think "has no need to support" matches...
> >
> >> This is a backward compatibility feature which enables older OSs. Since
> >> Linux VT-d implementation has already supported ATS features for a long
> >> time, there is no need to rely on this compatibility hardware. Remove it
> >> to make the driver future-proof.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/intel/iommu.c | 9 +--------
> >>   1 file changed, 1 insertion(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >> index 07e394dfccc1..b63347c8bf5d 100644
> >> --- a/drivers/iommu/intel/iommu.c
> >> +++ b/drivers/iommu/intel/iommu.c
> >> @@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev
> >> *dev, struct intel_iommu *iommu)
> >>   	dev = pci_physfn(dev);
> >>   	satcu = dmar_find_matched_satc_unit(dev);
> >>   	if (satcu)
> >> -		/*
> >> -		 * This device supports ATS as it is in SATC table.
> >> -		 * When IOMMU is in legacy mode, enabling ATS is done
> >> -		 * automatically by HW for the device that requires
> >> -		 * ATS, hence OS should not enable this device ATS
> >> -		 * to avoid duplicated TLB invalidation.
> >> -		 */
> > ...what above comment tries to convey.
> >
> > If this comment is valid, it's not about whether the OS itself supports
> > ATS. instead it's a requirement for the OS to not manage ATS when
> > it's already managed by HW.
> >
> > Unless there is a way to disable hw management with this change...
> 
> This comment is not correct. The hardware automatic ATS is for older OS
> compatible purposes, where the ATS is not aware of by the OS yet, but
> ATS is functionally required for some SOC-integrated accelerators.
> 
> The HAS specification for those platforms states that OSs supporting ATS
> (so-called enlightened OSs) don't require automatic ATS anymore.
> 
>  From the iommu driver's point of view, automatic ATS is not part of the
> VT-d spec and also not enumerable, hence it should be transparent.
> 

I'm curious how automatic ATS can be disabled otherwise the old
comment still makes sense as you will have both HW and SW
managing ATS and then duplicated invalidations.

Is there a BIOS option to disable automatic ATS? Then the user will
need to know which kernel version supports ATS to decide.

Or is it automatically enabled/disabled based on whether the
IOMMU is in legacy or scalable mode? If yes then we may still want
to disable SW-managed ATS when the IOMMU is in legacy mode to
avoid duplicated invalidations.

btw ATS support was introduced long long time ago:

commit 93a23a7271dfb811b3adb72779054c3a24433112
Author: Yu Zhao <yu.zhao@intel.com>
Date:   Mon May 18 13:51:37 2009 +0800

    VT-d: support the device IOTLB

    Enable the device IOTLB (i.e. ATS) for both the bare metal and KVM
    environments.

    Signed-off-by: Yu Zhao <yu.zhao@intel.com>
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

while this hw-managed ATS was supported much later:

commit 97f2f2c5317f55ae3440733a090a96a251da222b
Author: Yian Chen <yian.chen@intel.com>
Date:   Tue Mar 1 10:01:59 2022 +0800

    iommu/vt-d: Enable ATS for the devices in SATC table

    Starting from Intel VT-d v3.2, Intel platform BIOS can provide additional
    SATC table structure. SATC table includes a list of SoC integrated devices
    that support ATC (Address translation cache).

    Enabling ATC (via ATS capability) can be a functional requirement for SATC
    device operation or optional to enhance device performance/functionality.
    This is determined by the bit of ATC_REQUIRED in SATC table. When IOMMU is
    working in scalable mode, software chooses to always enable ATS for every
    device in SATC table because Intel SoC devices in SATC table are trusted to
    use ATS.

    On the other hand, if IOMMU is in legacy mode, ATS of SATC capable devices
    can work transparently to software and be automatically enabled by IOMMU
    hardware. As the result, there is no need for software to enable ATS on
    these devices.

    This also removes dmar_find_matched_atsr_unit() helper as it becomes dead
    code now.

    Signed-off-by: Yian Chen <yian.chen@intel.com>
    Link: https://lore.kernel.org/r/20220222185416.1722611-1-yian.chen@intel.com
    Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
    Link: https://lore.kernel.org/r/20220301020159.633356-13-baolu.lu@linux.inte
    Signed-off-by: Joerg Roedel <jroedel@suse.de>

That history doesn't appear to support your argument that it's only
for old OS which is not aware of ATS...

Re: [PATCH 2/2] iommu/vt-d: Remove hardware automatic ATS dependency

[Baolu Lu]

On 6/26/24 9:53 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, June 25, 2024 4:40 PM
>>
>> On 2024/6/25 10:32, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Monday, June 24, 2024 1:25 PM
>>>>
>>>> If a device is listed in the SATC table with ATC_REQUIRED flag set, it
>>>> indicates that the device has a functional requirement to enable its ATC
>>>> (via the ATS capability) for device operation. However, when IOMMU is
>>>> running in the legacy mode, ATS could be automatically supported by the
>>>> hardware so that the OS has no need to support the ATS functionality.
>>> hmm I don't think "has no need to support" matches...
>>>
>>>> This is a backward compatibility feature which enables older OSs. Since
>>>> Linux VT-d implementation has already supported ATS features for a long
>>>> time, there is no need to rely on this compatibility hardware. Remove it
>>>> to make the driver future-proof.
>>>>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/intel/iommu.c | 9 +--------
>>>>    1 file changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index 07e394dfccc1..b63347c8bf5d 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -3056,14 +3056,7 @@ static bool dmar_ats_supported(struct pci_dev
>>>> *dev, struct intel_iommu *iommu)
>>>>    	dev = pci_physfn(dev);
>>>>    	satcu = dmar_find_matched_satc_unit(dev);
>>>>    	if (satcu)
>>>> -		/*
>>>> -		 * This device supports ATS as it is in SATC table.
>>>> -		 * When IOMMU is in legacy mode, enabling ATS is done
>>>> -		 * automatically by HW for the device that requires
>>>> -		 * ATS, hence OS should not enable this device ATS
>>>> -		 * to avoid duplicated TLB invalidation.
>>>> -		 */
>>> ...what above comment tries to convey.
>>>
>>> If this comment is valid, it's not about whether the OS itself supports
>>> ATS. instead it's a requirement for the OS to not manage ATS when
>>> it's already managed by HW.
>>>
>>> Unless there is a way to disable hw management with this change...
>>
>> This comment is not correct. The hardware automatic ATS is for older OS
>> compatible purposes, where the ATS is not aware of by the OS yet, but
>> ATS is functionally required for some SOC-integrated accelerators.
>>
>> The HAS specification for those platforms states that OSs supporting ATS
>> (so-called enlightened OSs) don't require automatic ATS anymore.
>>
>>   From the iommu driver's point of view, automatic ATS is not part of the
>> VT-d spec and also not enumerable, hence it should be transparent.
>>
> 
> I'm curious how automatic ATS can be disabled otherwise the old
> comment still makes sense as you will have both HW and SW
> managing ATS and then duplicated invalidations.
> 
> Is there a BIOS option to disable automatic ATS? Then the user will
> need to know which kernel version supports ATS to decide.
> 
> Or is it automatically enabled/disabled based on whether the
> IOMMU is in legacy or scalable mode? If yes then we may still want
> to disable SW-managed ATS when the IOMMU is in legacy mode to
> avoid duplicated invalidations.
> 
> btw ATS support was introduced long long time ago:
> 
> commit 93a23a7271dfb811b3adb72779054c3a24433112
> Author: Yu Zhao <yu.zhao@intel.com>
> Date:   Mon May 18 13:51:37 2009 +0800
> 
>      VT-d: support the device IOTLB
> 
>      Enable the device IOTLB (i.e. ATS) for both the bare metal and KVM
>      environments.
> 
>      Signed-off-by: Yu Zhao <yu.zhao@intel.com>
>      Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> while this hw-managed ATS was supported much later:
> 
> commit 97f2f2c5317f55ae3440733a090a96a251da222b
> Author: Yian Chen <yian.chen@intel.com>
> Date:   Tue Mar 1 10:01:59 2022 +0800
> 
>      iommu/vt-d: Enable ATS for the devices in SATC table
> 
>      Starting from Intel VT-d v3.2, Intel platform BIOS can provide additional
>      SATC table structure. SATC table includes a list of SoC integrated devices
>      that support ATC (Address translation cache).
> 
>      Enabling ATC (via ATS capability) can be a functional requirement for SATC
>      device operation or optional to enhance device performance/functionality.
>      This is determined by the bit of ATC_REQUIRED in SATC table. When IOMMU is
>      working in scalable mode, software chooses to always enable ATS for every
>      device in SATC table because Intel SoC devices in SATC table are trusted to
>      use ATS.
> 
>      On the other hand, if IOMMU is in legacy mode, ATS of SATC capable devices
>      can work transparently to software and be automatically enabled by IOMMU
>      hardware. As the result, there is no need for software to enable ATS on
>      these devices.
> 
>      This also removes dmar_find_matched_atsr_unit() helper as it becomes dead
>      code now.
> 
>      Signed-off-by: Yian Chen <yian.chen@intel.com>
>      Link: https://lore.kernel.org/r/20220222185416.1722611-1-yian.chen@intel.com
>      Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>      Link: https://lore.kernel.org/r/20220301020159.633356-13-baolu.lu@linux.inte
>      Signed-off-by: Joerg Roedel <jroedel@suse.de>
> 
> That history doesn't appear to support your argument that it's only
> for old OS which is not aware of ATS...

Software has no means to disable automatic ATS. Therefore it's not
expected to turn on the ATS in the device while the iommu is in legacy
mode. I will drop this change.

Best regards,
baolu