Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

[Vasant Hegde <vasant.hegde@amd.com>]

Hi All,



On 8/20/2024 9:40 AM, Baolu Lu wrote:
> On 2024/8/19 20:34, Jason Gunthorpe wrote:
>> On Mon, Aug 19, 2024 at 03:09:00PM +0800, Baolu Lu wrote:
>>> On 2024/8/19 14:34, Vasant Hegde wrote:
>>>> On 8/16/2024 6:39 PM, Baolu Lu wrote:
>>>>> On 2024/8/16 20:16, Vasant Hegde wrote:
>>>>>> On 8/16/2024 4:19 PM, Lu Baolu wrote:
>>>>>>> Currently, PCI PASID is enabled alongside PCI ATS when an iommu domain is
>>>>>>> attached to the device and disabled when the device transitions to block
>>>>>>> translation mode. This approach is inappropriate as PCI PASID is a device
>>>>>>> feature independent of the type of the attached domain.
>>>>>> Reading through other thread, I thought we want to enable both PASID and
>>>>>> PRI in
>>>>>> device probe path. Did I miss something?
>>>>> PRI is different. PRI should be enabled when the first iopf-capable
>>>>> domain is attached to device or its PASID, and disabled when the last
>>>>> such domain is detached.
>>>> Right. That's what AMD driver also does (We enable it when we attach IOPF
>>>> capable domain). But looking into pci_enable_pri() :
>>>>
>>>>
>>>> 202         /*
>>>> 203          * VFs must not implement the PRI Capability.  If their PF
>>>> 204          * implements PRI, it is shared by the VFs, so if the PF PRI is
>>>> 205          * enabled, it is also enabled for the VF.
>>>> 206          */
>>>> 207         if (pdev->is_virtfn) {
>>>> 208                 if (pci_physfn(pdev)->pri_enabled)
>>>> 209                         return 0;
>>>> 210                 return -EINVAL;
>>>> 211         }
>>>> 212
>>>>
>>>>
>>>> If we try to enable PRI for VF without first enabling it in PF it will fail
>>>> right?
>>>>
>>>> Now if PF is attached to non-IOPF capable domain (like in AMD case attaching to
>>>> domain with V1 page table) and we try to attach VF to IOPF capable domain  (say
>>>> AMD v2 page table -OR- nested domain) it will fail right?
>>> Yeah! So, the iommu driver should basically control the PRI switch on
>>> the PF whenever someone wants to use it on a VF.
>> PRI enable sounds like PASID enable to me.
>>
>> The ATS control is per VF/PF, and PRI does nothing unless ATS returns
>> a non-present indication.
>>
>> Like PASID, it seems the purpose of PRI caps is to negotiate if the
>> CPU can process PRI TLPs globally.
>>
>> So, I'd guess that just like PASID we should turn it on at PF probe
>> time if the IOMMU can globall handle PRI.
>>
>> Enabling ATS will cause PRI TLPs to be sent.
>>
>> Probably more of this code should be lifted out of the iommu drivers..
> 
> Some architectures, including VT-d non-scalable mode, doesn't support
> ATS translation and translated requests when it is working in the
> IDENTITY domain mode. In that case, probably PCI ATS still need to be
> disabled when such domain is attached and re-enabled when the domain is
> detached.

Does it make sense to move both PASID/PRI enablement to probe() path? something
like below :

[I am assuming ops->dev_enable_feat() interface is going away]

  - Enable device side PASID/PRI during ops->probe_device()
  - In device attach path (ops->attach_dev()), depending on IOMMU, device and
domain capability configure the features like PASID, IOPF and ATS. That means
ATS enablement is still done at attach device path.


-Vasant