Messages in this thread | | | Date | Fri, 18 Feb 2022 13:50:01 -0800 | Subject | Re: [PATCH] iommu/vt-d: Enable ATS for the devices in SATC table | From | "Chen, Yian" <> |
| |
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 <yian.chen@intel.com> >> --- >> 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
| |