Messages in this thread | | | Date | Fri, 13 Jan 2023 03:15:00 -0500 | Subject | Re: [PATCH v2 1/1] PCI: Add translated request only flag for pci_enable_pasid() | From | Matt Fagnani <> |
| |
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/
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@bell.net> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Suggested-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.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);
| |