Re: [PATCH v2 1/1] PCI: Add translated request only flag for pci_enable_pasid()

[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@xxxxxxxxxxxxx/ 


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@xxxxxxxx>
> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxx/
>
> 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);