| 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
| ||
|
Message-ID: <e40bacb0-701c-5373-3890-d037f668829f@linux.intel.com>
Date: Fri, 13 Jan 2023 16:49:21 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Matt Fagnani <matt.fagnani@...l.net>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Joerg Roedel <jroedel@...e.de>
Cc: baolu.lu@...ux.intel.com,
Christian König <christian.koenig@....com>,
Jason Gunthorpe <jgg@...dia.com>,
Kevin Tian <kevin.tian@...el.com>,
Vasant Hegde <vasant.hegde@....com>,
Tony Zhu <tony.zhu@...el.com>, linux-pci@...r.kernel.org,
iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] PCI: Add translated request only flag for
pci_enable_pasid()
On 2023/1/13 16:15, Matt Fagnani wrote:
> Baolu,
>
> 6.2-rc3 with v2 of this patch applied booted normally without the black
> screen problem. regzbot by Thorsten Leemhuis recommends adding the
> following link tag to the previous discussion at
> https://linux-regtracking.leemhuis.info/regzbot/mainline/ "When fixing,
> add this to the commit message to make regzbot notice patch postings and
> commits to resolve the issue:" Link:
> https://lore.kernel.org/r/15d0f9ff-2a56-b3e9-5b45-e6b23300ae3b@leemhuis.info/
Sure. I will add below:
Fixes: 201007ef707a ("PCI: Enable PASID only when ACS RR & UF enabled on
upstream path")
Reported-by: Matt Fagnani <matt.fagnani@...l.net>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216865
Link:
https://lore.kernel.org/r/15d0f9ff-2a56-b3e9-5b45-e6b23300ae3b@leemhuis.info/
By the way, can I add your tested-by?
--
Best regards,
baolu
>
> Thanks,
>
> Matt
>
> On 1/12/23 20:44, Lu Baolu wrote:
>> The PCIe fabric routes Memory Requests based on the TLP address, ignoring
>> the PASID. In order to ensure system integrity, commit 201007ef707a
>> ("PCI:
>> Enable PASID only when ACS RR & UF enabled on upstream path") requires
>> some ACS features being supported on device's upstream path when enabling
>> PCI/PASID.
>>
>> One alternative is ATS which lets the device resolve the PASID+addr pair
>> before a memory request is made into a routeable TLB address through the
>> TA. Those resolved addresses are then cached on the device instead of
>> in the IOMMU TLB and the device always sets the translated bit for PASID.
>> One example of those devices are AMD graphic devices that always have ACS
>> or ATS enabled together with PASID.
>>
>> This adds a flag parameter in the pci_enable_pasid() helper, with which
>> the device driver could opt-in the fact that device always sets the
>> translated bit for PASID.
>>
>> It also applies this opt-in for AMD graphic devices. Without this change,
>> kernel boots to black screen on a system with below AMD graphic device:
>>
>> 00:01.0 VGA compatible controller: Advanced Micro Devices, Inc.
>> [AMD/ATI] Wani [Radeon R5/R6/R7 Graphics] (rev ca)
>> (prog-if 00 [VGA controller])
>> DeviceName: ATI EG BROADWAY
>> Subsystem: Hewlett-Packard Company Device 8332
>>
>> At present, it is a common practice to enable/disable PCI PASID in the
>> iommu drivers. Considering that the device driver knows more about the
>> specific device, we will follow up by moving pci_enable_pasid() into
>> the specific device drivers.
>>
>> Fixes: 201007ef707a ("PCI: Enable PASID only when ACS RR & UF enabled
>> on upstream path")
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216865
>> Reported-by: Matt Fagnani <matt.fagnani@...l.net>
>> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
>> Suggested-by: Christian König <christian.koenig@....com>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>> include/linux/pci-ats.h | 6 ++++--
>> drivers/iommu/amd/iommu.c | 2 +-
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
>> drivers/iommu/intel/iommu.c | 3 ++-
>> drivers/pci/ats.c | 8 ++++++--
>> 5 files changed, 14 insertions(+), 7 deletions(-)
>>
>> Change log:
>> v2:
>> - Convert the bool to a named flag;
>> - Convert TRANSLED to XLATED.
>>
>> v1:
>> -
>> https://lore.kernel.org/linux-iommu/20230112084629.737653-1-baolu.lu@linux.intel.com/
>>
>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>> index df54cd5b15db..750fca736ef4 100644
>> --- a/include/linux/pci-ats.h
>> +++ b/include/linux/pci-ats.h
>> @@ -4,6 +4,8 @@
>> #include <linux/pci.h>
>> +#define PCI_PASID_XLATED_REQ_ONLY BIT(0)
>> +
>> #ifdef CONFIG_PCI_ATS
>> /* Address Translation Service */
>> bool pci_ats_supported(struct pci_dev *dev);
>> @@ -35,12 +37,12 @@ static inline bool pci_pri_supported(struct
>> pci_dev *pdev)
>> #endif /* CONFIG_PCI_PRI */
>> #ifdef CONFIG_PCI_PASID
>> -int pci_enable_pasid(struct pci_dev *pdev, int features);
>> +int pci_enable_pasid(struct pci_dev *pdev, int features, int flags);
>> void pci_disable_pasid(struct pci_dev *pdev);
>> int pci_pasid_features(struct pci_dev *pdev);
>> int pci_max_pasids(struct pci_dev *pdev);
>> #else /* CONFIG_PCI_PASID */
>> -static inline int pci_enable_pasid(struct pci_dev *pdev, int features)
>> +static inline int pci_enable_pasid(struct pci_dev *pdev, int
>> features, int flags)
>> { return -EINVAL; }
>> static inline void pci_disable_pasid(struct pci_dev *pdev) { }
>> static inline int pci_pasid_features(struct pci_dev *pdev)
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index cbeaab55c0db..64a8c03d7dfa 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1700,7 +1700,7 @@ static int pdev_pri_ats_enable(struct pci_dev
>> *pdev)
>> int ret;
>> /* Only allow access to user-accessible pages */
>> - ret = pci_enable_pasid(pdev, 0);
>> + ret = pci_enable_pasid(pdev, 0, PCI_PASID_XLATED_REQ_ONLY);
>> if (ret)
>> goto out_err;
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index ab160198edd6..891bf53c45dc 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2350,7 +2350,7 @@ static int arm_smmu_enable_pasid(struct
>> arm_smmu_master *master)
>> if (num_pasids <= 0)
>> return num_pasids;
>> - ret = pci_enable_pasid(pdev, features);
>> + ret = pci_enable_pasid(pdev, features, 0);
>> if (ret) {
>> dev_err(&pdev->dev, "Failed to enable PASID\n");
>> return ret;
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 59df7e42fd53..5cc13f02a5ac 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1425,7 +1425,8 @@ static void iommu_enable_pci_caps(struct
>> device_domain_info *info)
>> undefined. So always enable PASID support on devices which
>> have it, even if we can't yet know if we're ever going to
>> use it. */
>> - if (info->pasid_supported && !pci_enable_pasid(pdev,
>> info->pasid_supported & ~1))
>> + if (info->pasid_supported &&
>> + !pci_enable_pasid(pdev, info->pasid_supported & ~1, 0))
>> info->pasid_enabled = 1;
>> if (info->pri_supported &&
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index f9cc2e10b676..3a2d9e0ba1d8 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -353,12 +353,15 @@ void pci_pasid_init(struct pci_dev *pdev)
>> * pci_enable_pasid - Enable the PASID capability
>> * @pdev: PCI device structure
>> * @features: Features to enable
>> + * @flags: device-specific flags
>> + * - PCI_PASID_XLATED_REQ_ONLY: The PCI device only issues PASID
>> + * memory requests of translated type.
>> *
>> * Returns 0 on success, negative value on error. This function checks
>> * whether the features are actually supported by the device and
>> returns
>> * an error if not.
>> */
>> -int pci_enable_pasid(struct pci_dev *pdev, int features)
>> +int pci_enable_pasid(struct pci_dev *pdev, int features, int flags)
>> {
>> u16 control, supported;
>> int pasid = pdev->pasid_cap;
>> @@ -382,7 +385,8 @@ int pci_enable_pasid(struct pci_dev *pdev, int
>> features)
>> if (!pasid)
>> return -EINVAL;
>> - if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
>> + if (!(flags & PCI_PASID_XLATED_REQ_ONLY) &&
>> + !pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
>> return -EINVAL;
>> pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
Powered by blists - more mailing lists