lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/1] PCI: Add translated request only flag for pci_enable_pasid()
From
Baolu,

Yes, you can add the tested-by tag for me.

Thanks,

Matt

On 1/13/23 03:49, Baolu Lu wrote:
> 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@bell.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@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);
>

\
 
 \ /
  Last update: 2023-03-26 23:39    [W:0.175 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site