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 an 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.
Signed-off-by: Yian Chen <[email protected]>
---
drivers/iommu/intel/iommu.c | 53 ++++++++++++++++++++++++++++++++++---
include/linux/intel-iommu.h | 2 +-
2 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 92fea3fbbb11..58a80cec50bb 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -872,7 +872,6 @@ static bool iommu_is_dummy(struct intel_iommu *iommu, struct device *dev)
return false;
}
-
struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn)
{
struct dmar_drhd_unit *drhd = NULL;
@@ -2684,7 +2683,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (ecap_dev_iotlb_support(iommu->ecap) &&
pci_ats_supported(pdev) &&
- dmar_find_matched_atsr_unit(pdev))
+ dmar_ats_supported(pdev, iommu))
info->ats_supported = 1;
if (sm_supported(iommu)) {
@@ -4020,7 +4019,42 @@ static void intel_iommu_free_dmars(void)
}
}
-int dmar_find_matched_atsr_unit(struct pci_dev *dev)
+/* dev_satc_state - Find if dev is in a DMAR SATC table
+ *
+ * return value:
+ * 1: dev is in STAC table and ATS is required
+ * 0: dev is in SATC table and ATS is optional
+ * -1: dev isn't in SATC table
+ */
+static int dev_satc_state(struct pci_dev *dev)
+{
+ int i, ret = -1;
+ struct device *tmp;
+ struct dmar_satc_unit *satcu;
+ struct acpi_dmar_satc *satc;
+
+ dev = pci_physfn(dev);
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(satcu, &dmar_satc_units, list) {
+ satc = container_of(satcu->hdr, struct acpi_dmar_satc, header);
+ if (satc->segment != pci_domain_nr(dev->bus))
+ continue;
+ for_each_dev_scope(satcu->devices, satcu->devices_cnt, i, tmp)
+ if (to_pci_dev(tmp) == dev) {
+ if (satc->flags)
+ ret = 1;
+ else
+ ret = 0;
+ goto out;
+ }
+ }
+out:
+ rcu_read_unlock();
+ return ret;
+}
+
+int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
{
int i, ret = 1;
struct pci_bus *bus;
@@ -4030,6 +4064,19 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
struct dmar_atsr_unit *atsru;
dev = pci_physfn(dev);
+ i = dev_satc_state(dev);
+ if (i >= 0) {
+ /* This dev 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 (i && !sm_supported(iommu))
+ ret = 0;
+ return ret;
+ }
+
for (bus = dev->bus; bus; bus = bus->parent) {
bridge = bus->self;
/* If it's an integrated device, allow ATS */
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 69230fd695ea..fe9fd417d611 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -717,7 +717,7 @@ static inline int nr_pte_to_next_page(struct dma_pte *pte)
}
extern struct dmar_drhd_unit * dmar_find_matched_drhd_unit(struct pci_dev *dev);
-extern int dmar_find_matched_atsr_unit(struct pci_dev *dev);
+extern int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu);
extern int dmar_enable_qi(struct intel_iommu *iommu);
extern void dmar_disable_qi(struct intel_iommu *iommu);
--
2.25.1
Hi Yian,
On 2/17/22 3:36 AM, Yian Chen wrote:
> 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 an 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.
>
> Signed-off-by: Yian Chen <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 53 ++++++++++++++++++++++++++++++++++---
> include/linux/intel-iommu.h | 2 +-
> 2 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 92fea3fbbb11..58a80cec50bb 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -872,7 +872,6 @@ static bool iommu_is_dummy(struct intel_iommu *iommu, struct device *dev)
>
> return false;
> }
> -
Unnecessary.
> struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn)
> {
> struct dmar_drhd_unit *drhd = NULL;
> @@ -2684,7 +2683,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>
> if (ecap_dev_iotlb_support(iommu->ecap) &&
> pci_ats_supported(pdev) &&
> - dmar_find_matched_atsr_unit(pdev))
> + dmar_ats_supported(pdev, iommu))
> info->ats_supported = 1;
>
> if (sm_supported(iommu)) {
> @@ -4020,7 +4019,42 @@ static void intel_iommu_free_dmars(void)
> }
> }
>
> -int dmar_find_matched_atsr_unit(struct pci_dev *dev)
> +/* dev_satc_state - Find if dev is in a DMAR SATC table
> + *
> + * return value:
> + * 1: dev is in STAC table and ATS is required
> + * 0: dev is in SATC table and ATS is optional
> + * -1: dev isn't in SATC table
> + */
Judging the status of the device in table according to the hard coded
return value will make the code hard to read. How about using two
helpers with meaningful names, for example,
bool dmar_find_matched_satc_unit()
bool dmar_ats_required_in_satc()?
> +static int dev_satc_state(struct pci_dev *dev)
> +{
> + int i, ret = -1;
> + struct device *tmp;
> + struct dmar_satc_unit *satcu;
> + struct acpi_dmar_satc *satc;
> +
> + dev = pci_physfn(dev);
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(satcu, &dmar_satc_units, list) {
> + satc = container_of(satcu->hdr, struct acpi_dmar_satc, header);
> + if (satc->segment != pci_domain_nr(dev->bus))
> + continue;
> + for_each_dev_scope(satcu->devices, satcu->devices_cnt, i, tmp)
> + if (to_pci_dev(tmp) == dev) {
> + if (satc->flags)
Do you need to check the ATC_REQUIRED bit field?
> + ret = 1;
> + else
> + ret = 0;
> + goto out;
> + }
> + }
> +out:
> + rcu_read_unlock();
> + return ret;
> +}
> +
> +int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
> {
> int i, ret = 1;
> struct pci_bus *bus;
> @@ -4030,6 +4064,19 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
> struct dmar_atsr_unit *atsru;
>
> dev = pci_physfn(dev);
> + i = dev_satc_state(dev);
> + if (i >= 0) {
> + /* This dev 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 (i && !sm_supported(iommu))
> + ret = 0;
> + return ret;
> + }
> +
> for (bus = dev->bus; bus; bus = bus->parent) {
> bridge = bus->self;
> /* If it's an integrated device, allow ATS */
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 69230fd695ea..fe9fd417d611 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -717,7 +717,7 @@ static inline int nr_pte_to_next_page(struct dma_pte *pte)
> }
>
> extern struct dmar_drhd_unit * dmar_find_matched_drhd_unit(struct pci_dev *dev);
> -extern int dmar_find_matched_atsr_unit(struct pci_dev *dev);
> +extern int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu);
The dmar_find_matched_atsr_unit() is not used out of iommu.c. Can you
please make a patch to change it to a static one?
>
> extern int dmar_enable_qi(struct intel_iommu *iommu);
> extern void dmar_disable_qi(struct intel_iommu *iommu);
Best regards,
baolu
On 2/17/2022 6:53 PM, Lu Baolu wrote:
> Hi Yian,
>
> On 2/17/22 3:36 AM, Yian Chen wrote:
>> 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 an 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.
>>
>> Signed-off-by: Yian Chen <[email protected]>
>> ---
>> drivers/iommu/intel/iommu.c | 53 ++++++++++++++++++++++++++++++++++---
>> include/linux/intel-iommu.h | 2 +-
>> 2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 92fea3fbbb11..58a80cec50bb 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -872,7 +872,6 @@ static bool iommu_is_dummy(struct intel_iommu
>> *iommu, struct device *dev)
>> return false;
>> }
>> -
>
> Unnecessary.
sure, to remove this.
>
>> struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8
>> *devfn)
>> {
>> struct dmar_drhd_unit *drhd = NULL;
>> @@ -2684,7 +2683,7 @@ static struct dmar_domain
>> *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>> if (ecap_dev_iotlb_support(iommu->ecap) &&
>> pci_ats_supported(pdev) &&
>> - dmar_find_matched_atsr_unit(pdev))
>> + dmar_ats_supported(pdev, iommu))
>> info->ats_supported = 1;
>> if (sm_supported(iommu)) {
>> @@ -4020,7 +4019,42 @@ static void intel_iommu_free_dmars(void)
>> }
>> }
>> -int dmar_find_matched_atsr_unit(struct pci_dev *dev)
>> +/* dev_satc_state - Find if dev is in a DMAR SATC table
>> + *
>> + * return value:
>> + * 1: dev is in STAC table and ATS is required
>> + * 0: dev is in SATC table and ATS is optional
>> + * -1: dev isn't in SATC table
>> + */
>
> Judging the status of the device in table according to the hard coded
> return value will make the code hard to read. How about using two
> helpers with meaningful names, for example,
>
> bool dmar_find_matched_satc_unit()
> bool dmar_ats_required_in_satc()?
> sure, this is good. how about I add one function.
//return address of found satcu, otherwise null:
static struct dmar_satc_unit *dmar_find_matched_satc_unit(struct pci_dev *)
and check if ats is required through the returned satc unit(atc_required)
>> +static int dev_satc_state(struct pci_dev *dev)
>> +{
>> + int i, ret = -1;
>> + struct device *tmp;
>> + struct dmar_satc_unit *satcu;
>> + struct acpi_dmar_satc *satc;
>> +
>> + dev = pci_physfn(dev);
>> + rcu_read_lock();
>> +
>> + list_for_each_entry_rcu(satcu, &dmar_satc_units, list) {
>> + satc = container_of(satcu->hdr, struct acpi_dmar_satc, header);
>> + if (satc->segment != pci_domain_nr(dev->bus))
>> + continue;
>> + for_each_dev_scope(satcu->devices, satcu->devices_cnt, i, tmp)
>> + if (to_pci_dev(tmp) == dev) {
>> + if (satc->flags)
>
> Do you need to check the ATC_REQUIRED bit field?
>Thanks, I can update this upon above response.
>> + ret = 1;
>> + else
>> + ret = 0;
>> + goto out;
>> + }
>> + }
>> +out:
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>> +int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu *iommu)
>> {
>> int i, ret = 1;
>> struct pci_bus *bus;
>> @@ -4030,6 +4064,19 @@ int dmar_find_matched_atsr_unit(struct pci_dev
>> *dev)
>> struct dmar_atsr_unit *atsru;
>> dev = pci_physfn(dev);
>> + i = dev_satc_state(dev);
>> + if (i >= 0) {
>> + /* This dev 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 (i && !sm_supported(iommu))
>> + ret = 0;
>> + return ret;
>> + }
>> +
>> for (bus = dev->bus; bus; bus = bus->parent) {
>> bridge = bus->self;
>> /* If it's an integrated device, allow ATS */
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index 69230fd695ea..fe9fd417d611 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -717,7 +717,7 @@ static inline int nr_pte_to_next_page(struct
>> dma_pte *pte)
>> }
>> extern struct dmar_drhd_unit * dmar_find_matched_drhd_unit(struct
>> pci_dev *dev);
>> -extern int dmar_find_matched_atsr_unit(struct pci_dev *dev);
>> +extern int dmar_ats_supported(struct pci_dev *dev, struct intel_iommu
>> *iommu);
>
> The dmar_find_matched_atsr_unit() is not used out of iommu.c. Can you
> please make a patch to change it to a static one?
>
sure, i will replace "int dmar_find_matched_atsr_unit" as above response.
>> extern int dmar_enable_qi(struct intel_iommu *iommu);
>> extern void dmar_disable_qi(struct intel_iommu *iommu);
>
> Best regards,
> baolu
Thanks,
Yian