The VT-d spec states that "SATC reporting structure identifies devices
that have address translation cache and that is validated per requirements
described in the 'Device TLB in System-on-Chip (SoC) Integrated Devices'
section. It is recommended that system software enable ATC for this
device". It is possible for an integrated device to have PCI ATC
capability implemented but not validated per the requirements, and thus
not appear in the SATC structure as recommended for ATS enablement.

The current implementation checks ATS support for integrated endpoints in
two places. First, it verifies if the integrated endpoint device is listed
in SATC. If not, it proceeds to the second check that always returns true
for integrated devices. This could result in endpoint devices not
recommended in SATC presenting "supported = true" to the caller.

Add integrated_device_ats_supported() for the integrated device ATS check
in a single location, which improves readability. The above issue is
also fixed in the function via returning false in that case.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/iommu/intel/iommu.c | 42 +++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2778bfe14f36..39abcf4e0f8f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2760,6 +2760,34 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
 	return satcu;
 }
 
+static bool integrated_device_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
+{
+	struct dmar_satc_unit *satcu = dmar_find_matched_satc_unit(dev);
+
+	/*
+	 * 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.
+	 */
+	if (satcu)
+		return !(satcu->atc_required && !sm_supported(iommu));
+
+	/*
+	 * The integrated device isn't enumerated in the SATC structure. For
+	 * example, it has ATS PCI capability implemented but not validated per
+	 * the requirements described in the VT-d specification, specifically
+	 * in the "Device TLB in System-on-Chip (SoC) Integrated Devices"
+	 * section. Therefore, it does not appear in the SATC structure. Return
+	 * false in this case.
+	 *
+	 * On older machines that do not support SATC (i.e., no SATC structure
+	 * present), ATS is considered to be "always" supported for integrated
+	 * endpoints.
+	 */
+	return !list_empty(&dmar_satc_units);
+}
+
 static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 {
 	int i;
@@ -2769,25 +2797,13 @@ static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 	struct device *tmp;
 	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
-	struct dmar_satc_unit *satcu;
 
 	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));
 
 	for (bus = dev->bus; bus; bus = bus->parent) {
 		bridge = bus->self;
-		/* If it's an integrated device, allow ATS */
 		if (!bridge)
-			return true;
+			return integrated_device_ats_supported(dev, iommu);
 		/* Connected via non-PCIe: no ATS */
 		if (!pci_is_pcie(bridge) ||
 		    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
-- 
2.43.0

On 5/9/25 22:00, Wei Wang wrote:
> The VT-d spec states that "SATC reporting structure identifies devices
> that have address translation cache and that is validated per requirements
> described in the 'Device TLB in System-on-Chip (SoC) Integrated Devices'

This is a spec recommendation for hardware implementation of the trusted
ATS. I recommend supporting it alongside HPT support in the mainline
kernel.

This 1/3 and 2/3 look good to me. Thank you and I will queue them for
next.

> section. It is recommended that system software enable ATC for this
> device". It is possible for an integrated device to have PCI ATC
> capability implemented but not validated per the requirements, and thus
> not appear in the SATC structure as recommended for ATS enablement.
> 
> The current implementation checks ATS support for integrated endpoints in
> two places. First, it verifies if the integrated endpoint device is listed
> in SATC. If not, it proceeds to the second check that always returns true
> for integrated devices. This could result in endpoint devices not
> recommended in SATC presenting "supported = true" to the caller.
> 
> Add integrated_device_ats_supported() for the integrated device ATS check
> in a single location, which improves readability. The above issue is
> also fixed in the function via returning false in that case.
> 
> Signed-off-by: Wei Wang<wei.w.wang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 42 +++++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 13 deletions(-)

Thanks,
baolu

On Monday, May 12, 2025 10:43 AM, Baolu Lu wrote:
> On 5/9/25 22:00, Wei Wang wrote:
> > The VT-d spec states that "SATC reporting structure identifies devices
> > that have address translation cache and that is validated per
> > requirements described in the 'Device TLB in System-on-Chip (SoC)
> Integrated Devices'
> 
> This is a spec recommendation for hardware implementation of the trusted
> ATS. I recommend supporting it alongside HPT support in the mainline kernel.

Sounds good, thanks.

> 
> This 1/3 and 2/3 look good to me. Thank you and I will queue them for next.
> 
> > section. It is recommended that system software enable ATC for this
> > device". It is possible for an integrated device to have PCI ATC
> > capability implemented but not validated per the requirements, and
> > thus not appear in the SATC structure as recommended for ATS enablement.
> >
> > The current implementation checks ATS support for integrated endpoints
> > in two places. First, it verifies if the integrated endpoint device is
> > listed in SATC. If not, it proceeds to the second check that always
> > returns true for integrated devices. This could result in endpoint
> > devices not recommended in SATC presenting "supported = true" to the
> caller.
> >
> > Add integrated_device_ats_supported() for the integrated device ATS
> > check in a single location, which improves readability. The above
> > issue is also fixed in the function via returning false in that case.
> >
> > Signed-off-by: Wei Wang<wei.w.wang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 42 +++++++++++++++++++++++++------------
> >   1 file changed, 29 insertions(+), 13 deletions(-)
> 
> Thanks,
> baolu

On 2025/5/9 22:00, Wei Wang wrote:
> The VT-d spec states that "SATC reporting structure identifies devices
> that have address translation cache and that is validated per requirements
> described in the 'Device TLB in System-on-Chip (SoC) Integrated Devices'
> section. It is recommended that system software enable ATC for this
> device". It is possible for an integrated device to have PCI ATC
> capability implemented but not validated per the requirements, and thus
> not appear in the SATC structure as recommended for ATS enablement.
> 
> The current implementation checks ATS support for integrated endpoints in
> two places. First, it verifies if the integrated endpoint device is listed
> in SATC. If not, it proceeds to the second check that always returns true
> for integrated devices. This could result in endpoint devices not
> recommended in SATC presenting "supported = true" to the caller.
> 
> Add integrated_device_ats_supported() for the integrated device ATS check
> in a single location, which improves readability. The above issue is
> also fixed in the function via returning false in that case.

if it is a fix. A Fixes tag is needed.

> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 42 +++++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 2778bfe14f36..39abcf4e0f8f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2760,6 +2760,34 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
>   	return satcu;
>   }
>   
> +static bool integrated_device_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
> +{
> +	struct dmar_satc_unit *satcu = dmar_find_matched_satc_unit(dev);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (satcu)
> +		return !(satcu->atc_required && !sm_supported(iommu));
> +
> +	/*
> +	 * The integrated device isn't enumerated in the SATC structure. For
> +	 * example, it has ATS PCI capability implemented but not validated per
> +	 * the requirements described in the VT-d specification, specifically
> +	 * in the "Device TLB in System-on-Chip (SoC) Integrated Devices"
> +	 * section. Therefore, it does not appear in the SATC structure. Return
> +	 * false in this case.
> +	 *
> +	 * On older machines that do not support SATC (i.e., no SATC structure
> +	 * present), ATS is considered to be "always" supported for integrated
> +	 * endpoints.
> +	 */
> +	return !list_empty(&dmar_satc_units);

shouldn't it be "return list_empty(&dmar_satc_units);"?

> +}
> +
>   static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   {
>   	int i;
> @@ -2769,25 +2797,13 @@ static bool dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   	struct device *tmp;
>   	struct acpi_dmar_atsr *atsr;
>   	struct dmar_atsr_unit *atsru;
> -	struct dmar_satc_unit *satcu;
>   
>   	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));
>   
>   	for (bus = dev->bus; bus; bus = bus->parent) {
>   		bridge = bus->self;
> -		/* If it's an integrated device, allow ATS */
>   		if (!bridge)
> -			return true;
> +			return integrated_device_ats_supported(dev, iommu);
>   		/* Connected via non-PCIe: no ATS */
>   		if (!pci_is_pcie(bridge) ||
>   		    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)

-- 
Regards,
Yi Liu

On Friday, May 9, 2025 5:32 PM, Liu, Yi L wrote:
2025/5/9 22:00, Wei Wang wrote:
> > The VT-d spec states that "SATC reporting structure identifies devices
> > that have address translation cache and that is validated per
> > requirements described in the 'Device TLB in System-on-Chip (SoC)
> Integrated Devices'
> > section. It is recommended that system software enable ATC for this
> > device". It is possible for an integrated device to have PCI ATC
> > capability implemented but not validated per the requirements, and
> > thus not appear in the SATC structure as recommended for ATS enablement.
> >
> > The current implementation checks ATS support for integrated endpoints
> > in two places. First, it verifies if the integrated endpoint device is
> > listed in SATC. If not, it proceeds to the second check that always
> > returns true for integrated devices. This could result in endpoint
> > devices not recommended in SATC presenting "supported = true" to the
> caller.
> >
> > Add integrated_device_ats_supported() for the integrated device ATS
> > check in a single location, which improves readability. The above
> > issue is also fixed in the function via returning false in that case.
> 
> if it is a fix. A Fixes tag is needed.

Yeah, will add it.

> 
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 42 +++++++++++++++++++++++++------------
> >   1 file changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 2778bfe14f36..39abcf4e0f8f 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -2760,6 +2760,34 @@ static struct dmar_satc_unit
> *dmar_find_matched_satc_unit(struct pci_dev *dev)
> >   	return satcu;
> >   }
> >
> > +static bool integrated_device_ats_supported(struct pci_dev *dev,
> > +struct intel_iommu *iommu) {
> > +	struct dmar_satc_unit *satcu = dmar_find_matched_satc_unit(dev);
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> > +	if (satcu)
> > +		return !(satcu->atc_required && !sm_supported(iommu));
> > +
> > +	/*
> > +	 * The integrated device isn't enumerated in the SATC structure. For
> > +	 * example, it has ATS PCI capability implemented but not validated
> per
> > +	 * the requirements described in the VT-d specification, specifically
> > +	 * in the "Device TLB in System-on-Chip (SoC) Integrated Devices"
> > +	 * section. Therefore, it does not appear in the SATC structure. Return
> > +	 * false in this case.
> > +	 *
> > +	 * On older machines that do not support SATC (i.e., no SATC structure
> > +	 * present), ATS is considered to be "always" supported for integrated
> > +	 * endpoints.
> > +	 */
> > +	return !list_empty(&dmar_satc_units);
> 
> shouldn't it be "return list_empty(&dmar_satc_units);"?

Right, thanks for the catch up.