| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Feb 2022 13:50:01 -0800
From: "Chen, Yian" <yian.chen@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>,
David Woodhouse <dwmw2@...radead.org>,
Ashok Raj <ashok.raj@...el.com>,
Sohil Mehta <sohil.mehta@...el.com>,
Jacob jun Pan <jacob.jun.pan@...el.com>,
<iommu@...ts.linux-foundation.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iommu/vt-d: Enable ATS for the devices in SATC table
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@...el.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
Powered by blists - more mailing lists