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

[Yi Liu <yi.l.liu@intel.com>]

On 2024/8/20 16:30, Vasant Hegde wrote:
> 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.

Are we sure that it is ok to enable PRI before enabling ATS?

-- 
Regards,
Yi Liu