Re: [PATCH v1 1/3] iommu/vt-d: Eliminate pci_physfn() in dmar_find_matched_satc_unit()

Re: [PATCH v1 1/3] iommu/vt-d: Eliminate pci_physfn() in dmar_find_matched_satc_unit()

[Yi Liu]

On 2025/5/9 22:00, Wei Wang wrote:
> The function dmar_find_matched_satc_unit() contains a duplicate call to
> pci_physfn(). This call is unnecessary as pci_physfn() has already been
> invoked by the caller. Removing the redundant call simplifies the code
> and improves efficiency a bit.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cb0b993bebb4..d8aa71305509 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2744,7 +2744,6 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
>   	struct device *tmp;
>   	int i;
>   
> -	dev = pci_physfn(dev);

better have a comment to highlight the input dev should be PF. also, can
add a WARN_ON(dev->is_virtfn);

>   	rcu_read_lock();
>   
>   	list_for_each_entry_rcu(satcu, &dmar_satc_units, list) {

-- 
Regards,
Yi Liu

Re: [PATCH v1 2/3] iommu/vt-d: Change dmar_ats_supported() to return boolean

[Yi Liu]

On 2025/5/9 22:00, Wei Wang wrote:
> According to "Function return values and names" in coding-style.rst, the
> dmar_ats_supported() function should return a boolean instead of an
> integer. Also, rename "ret" to "supported" to be more straightforward.
>

seems just a recommendation since this is just internal helper. The
function was indeed not well written anyhow. :) not sure if Baolu wants
take it or not. Taking it may make history tracking harder. Patch itself
looks good to me.

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

> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index d8aa71305509..2778bfe14f36 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2760,10 +2760,11 @@ 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;
> +	int i;
>   	struct pci_bus *bus;
> +	bool supported = true;
>   	struct pci_dev *bridge = NULL;
>   	struct device *tmp;
>   	struct acpi_dmar_atsr *atsr;

This list should have been in reverse Christmas tree order per the length. :)

> @@ -2786,11 +2787,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;
> @@ -2809,11 +2810,11 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>   		if (atsru->include_all)
>   			goto out;
>   	}
> -	ret = 0;
> +	supported = false;
>   out:
>   	rcu_read_unlock();
>   
> -	return ret;
> +	return supported;
>   }
>   
>   int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)

-- 
Regards,
Yi Liu

Re: [PATCH v1 3/3] iommu/vt-d: Fix ATS support check for integrated endpoints

[Yi Liu]

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

RE: [PATCH v1 3/3] iommu/vt-d: Fix ATS support check for integrated endpoints

[Wang, Wei W]

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.

RE: [PATCH v1 2/3] iommu/vt-d: Change dmar_ats_supported() to return boolean

[Wang, Wei W]

On Friday, May 9, 2025 5:27 PM, Liu, Yi L wrote:
/5/9 22:00, Wei Wang wrote:
> > According to "Function return values and names" in coding-style.rst,
> > the
> > dmar_ats_supported() function should return a boolean instead of an
> > integer. Also, rename "ret" to "supported" to be more straightforward.
> >
> 
> seems just a recommendation since this is just internal helper. The function
> was indeed not well written anyhow. :) not sure if Baolu wants take it or not.
> Taking it may make history tracking harder. Patch itself looks good to me.
> 
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>

It seems mandatory to me from coding-style.rst:
"
Mixing up these two sorts of representations is a fertile source of
difficult-to-find bugs.  If the C language included a strong distinction
between integers and booleans then the compiler would find these mistakes
for us... but it doesn't.  To help prevent such bugs, always follow this
convention::

        If the name of a function is an action or an imperative command,
        the function should return an error-code integer.  If the name
        is a predicate, the function should return a "succeeded" boolean.
"

Another reason for making this change is that when we try to add more checks
inside this function (e.g., integrated_device_ats_supported() in patch 3), the added
new function needs to continue following the incorrect style. So, I thought, why
not change it from the foundation 😊

RE: [PATCH v1 1/3] iommu/vt-d: Eliminate pci_physfn() in dmar_find_matched_satc_unit()

[Wang, Wei W]

On Friday, May 9, 2025 5:16 PM, Liu, Yi L wrote:
> On 2025/5/9 22:00, Wei Wang wrote:
> > The function dmar_find_matched_satc_unit() contains a duplicate call
> > to pci_physfn(). This call is unnecessary as pci_physfn() has already
> > been invoked by the caller. Removing the redundant call simplifies the
> > code and improves efficiency a bit.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index cb0b993bebb4..d8aa71305509 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -2744,7 +2744,6 @@ static struct dmar_satc_unit
> *dmar_find_matched_satc_unit(struct pci_dev *dev)
> >   	struct device *tmp;
> >   	int i;
> >
> > -	dev = pci_physfn(dev);
> 
> better have a comment to highlight the input dev should be PF. also, can add a
> WARN_ON(dev->is_virtfn);

How about changing the input to "struct pci_dev *pf_pdev " (like the one used
in device_lookup_iommu())?
This way, we probably don't need extra comments, and WARN_ON() might not be
necessary IMHO, as this is a static function with only one caller currently (already
clear that the input is a pf_dev)

[PATCH v1 0/3] Improve ATS Support Check in IOMMU/VT-d

[Wei Wang]

This patchset makes a few cleanups and fixes related to the check of ATS
support in iommu/vt-d. The first patch removes a duplicate call to
pci_physfn() in dmar_find_matched_satc_unit(). The second patch changes
dmar_ats_supported() to return boolean, aligning with the convention used
by other *_supported() functions. The third one addresses a corner case
where an integrated device, not recommended by the SATC structure, could
be incorrectly reported as "supported=true" by dmar_ats_supported().

Wei Wang (3):
  iommu/vt-d: Eliminate pci_physfn() in dmar_find_matched_satc_unit()
  iommu/vt-d: Change dmar_ats_supported() to return boolean
  iommu/vt-d: Fix ATS support check for integrated endpoints

 drivers/iommu/intel/iommu.c | 54 ++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] iommu/vt-d: Eliminate pci_physfn() in dmar_find_matched_satc_unit()

[Wei Wang]

The function dmar_find_matched_satc_unit() contains a duplicate call to
pci_physfn(). This call is unnecessary as pci_physfn() has already been
invoked by the caller. Removing the redundant call simplifies the code
and improves efficiency a bit.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/iommu/intel/iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cb0b993bebb4..d8aa71305509 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2744,7 +2744,6 @@ static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *dev)
 	struct device *tmp;
 	int i;
 
-	dev = pci_physfn(dev);
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(satcu, &dmar_satc_units, list) {
-- 
2.43.0

[PATCH v1 2/3] iommu/vt-d: Change dmar_ats_supported() to return boolean

[Wei Wang]

According to "Function return values and names" in coding-style.rst, the
dmar_ats_supported() function should return a boolean instead of an
integer. Also, rename "ret" to "supported" to be more straightforward.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d8aa71305509..2778bfe14f36 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2760,10 +2760,11 @@ 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;
+	int i;
 	struct pci_bus *bus;
+	bool supported = true;
 	struct pci_dev *bridge = NULL;
 	struct device *tmp;
 	struct acpi_dmar_atsr *atsr;
@@ -2786,11 +2787,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;
@@ -2809,11 +2810,11 @@ static int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
 		if (atsru->include_all)
 			goto out;
 	}
-	ret = 0;
+	supported = false;
 out:
 	rcu_read_unlock();
 
-	return ret;
+	return supported;
 }
 
 int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
-- 
2.43.0

[PATCH v1 3/3] iommu/vt-d: Fix ATS support check for integrated endpoints

[Wei Wang]

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

Re: [PATCH v1 3/3] iommu/vt-d: Fix ATS support check for integrated endpoints

[Baolu Lu]

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

RE: [PATCH v1 3/3] iommu/vt-d: Fix ATS support check for integrated endpoints

[Wang, Wei W]

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