| Message ID | 20230112084629.737653-1-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] PCI: Add translated request only opt-in for pci_enable_pasid() | expand |
On Thu, Jan 12, 2023 at 04:46:29PM +0800, Lu Baolu wrote: > -int pci_enable_pasid(struct pci_dev *pdev, int features); > +int pci_enable_pasid(struct pci_dev *pdev, int features, bool > transled_only); Please use a named flag so we can grep for it.. Discuss in the commit message that this is a temporary step and that pci_enable_pasid() needs to be moved to the drivers Jason
On 2023/1/12 20:38, Jason Gunthorpe wrote: > On Thu, Jan 12, 2023 at 04:46:29PM +0800, Lu Baolu wrote: > >> -int pci_enable_pasid(struct pci_dev *pdev, int features); >> +int pci_enable_pasid(struct pci_dev *pdev, int features, bool >> transled_only); > > Please use a named flag so we can grep for it.. Sure. What do you think of this naming? + * @flags: device-specific flags + * - PCI_PASID_TRANSLED_REQ_ONLY: The PCI device only issues PASID + * memory requests of translated type. > Discuss in the commit message that this is a temporary step and that > pci_enable_pasid() needs to be moved to the drivers I will add below in commit message: 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. -- Best regards, baolu
On Thu, Jan 12, 2023 at 09:25:25PM +0800, Baolu Lu wrote: > On 2023/1/12 20:38, Jason Gunthorpe wrote: > > On Thu, Jan 12, 2023 at 04:46:29PM +0800, Lu Baolu wrote: > > > > > -int pci_enable_pasid(struct pci_dev *pdev, int features); > > > +int pci_enable_pasid(struct pci_dev *pdev, int features, bool > > > transled_only); > > > > Please use a named flag so we can grep for it.. > > Sure. What do you think of this naming? > > + * @flags: device-specific flags > + * - PCI_PASID_TRANSLED_REQ_ONLY: The PCI device only issues PASID > + * memory requests of translated type. Yes > > Discuss in the commit message that this is a temporary step and that > > pci_enable_pasid() needs to be moved to the drivers > > I will add below in commit message: > > 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. Yes Jason
On Thu, Jan 12, 2023 at 09:25:25PM +0800, Baolu Lu wrote: > On 2023/1/12 20:38, Jason Gunthorpe wrote: > > On Thu, Jan 12, 2023 at 04:46:29PM +0800, Lu Baolu wrote: > > > > > -int pci_enable_pasid(struct pci_dev *pdev, int features); > > > +int pci_enable_pasid(struct pci_dev *pdev, int features, bool > > > transled_only); ^^^^^^^^ > + * @flags: device-specific flags > + * - PCI_PASID_TRANSLED_REQ_ONLY: The PCI device only issues PASID > + * memory requests of translated type. ^^^^^^^^ I don't like "transled" since it's not a word itself or an obvious combination of "translated" and something else. Not sure whether you need to abbreviate it, but if you do, "xlate" is a common shortening of "translate". Bjorn
On 2023/1/13 2:34, Bjorn Helgaas wrote: > On Thu, Jan 12, 2023 at 09:25:25PM +0800, Baolu Lu wrote: >> On 2023/1/12 20:38, Jason Gunthorpe wrote: >>> On Thu, Jan 12, 2023 at 04:46:29PM +0800, Lu Baolu wrote: >>> >>>> -int pci_enable_pasid(struct pci_dev *pdev, int features); >>>> +int pci_enable_pasid(struct pci_dev *pdev, int features, bool >>>> transled_only); > ^^^^^^^^ > >> + * @flags: device-specific flags >> + * - PCI_PASID_TRANSLED_REQ_ONLY: The PCI device only issues PASID >> + * memory requests of translated type. > ^^^^^^^^ > > I don't like "transled" since it's not a word itself or an obvious > combination of "translated" and something else. > > Not sure whether you need to abbreviate it, but if you do, "xlate" is > a common shortening of "translate". Yes. "xlate" looks better. I will replace with it. I've updated this patch with a v2. https://lore.kernel.org/linux-iommu/20230113014409.752405-1-baolu.lu@linux.intel.com/ Thank you both for the comments. -- Best regards, baolu
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h index df54cd5b15db..78a37821fff4 100644 --- a/include/linux/pci-ats.h +++ b/include/linux/pci-ats.h @@ -35,12 +35,13 @@ 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, bool transled_only); 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, + bool transled_only) { 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..a89734b30947 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, true); 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..f5f224f023ba 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, false); 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 317af67b6098..2dd7773ce5c0 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1424,7 +1424,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, false)) info->pasid_enabled = 1; if (info->pri_supported && diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index f9cc2e10b676..eb14ca8ec4e6 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -353,12 +353,14 @@ void pci_pasid_init(struct pci_dev *pdev) * pci_enable_pasid - Enable the PASID capability * @pdev: PCI device structure * @features: Features to enable + * @transled_only: True if the device issues MemRd/Wr TLPs with a PASID TLP + * prefix only when the AT field is set to translated. * * 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, bool transled_only) { u16 control, supported; int pasid = pdev->pasid_cap; @@ -382,7 +384,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 (!transled_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);
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 an extra 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 Refer to kernel bugzilla through below link for more details. 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 | 5 +++-- 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 | 7 +++++-- 5 files changed, 12 insertions(+), 7 deletions(-)