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

[Vasant Hegde]

Jason,


On 8/20/2024 7:21 PM, Jason Gunthorpe wrote:
> On Tue, Aug 20, 2024 at 02:00:08PM +0530, Vasant Hegde wrote:
> 
>>> 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. 
> 
> ARM has a similar issue.
> 
> ATS enablement should be done when the domain is attached in those
> cases.
> 
> Arguably you don't want to turn ATS on anyhow for pure IDENTITY with
> no PASID because it is just pointless.
> 
>> 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 :
> 
> It makes sense.
> 
> I don't see any ordering restriction in the PCI specification.
> 
> Notice that PASID does have a specific called out restriction:
> 
> 	/*
> 	 * Note that PASID must be enabled before, and disabled after ATS:
> 	 * PCI Express Base 4.0r1.0 - 10.5.1.3 ATS Control Register
> 	 *
> 	 *   Behavior is undefined if this bit is Set and the value of the PASID
> 	 *   Enable, Execute Requested Enable, or Privileged Mode Requested bits
> 	 *   are changed.
> 	 */
> 
>> [I am assuming ops->dev_enable_feat() interface is going away]
> 
> Is the plan
>  
>>   - Enable device side PASID/PRI during ops->probe_device()
> 
> Yes
> 
>>   - 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.
> 
> From a PCI perspective only ATS can be changed at this point..
> 
> The SW construct of IOPF can be changed during domain attachment.
> 
> Everything that is PF-only must be setup during probe_device only
> otherwise SRIOV VFs will be broken insome cases.

Makes sense. I will modify AMD driver to enable PRI/PASID in probe() path.


> 
> See
> https://lore.kernel.org/all/0-v1-0fb4d2ab6770+7e706-ats_vf_jgg@xxxxxxxxxx/
> for this concept applied to ATS.
> 
> This means probe_device() has to do:
> 
>  - ATS properties 
>  - PRI
>  - PASID properties
> 
> At a minimum.
> 
> It would be nice if the iommu core code did this setup in one place
> immediately after calling probe_device() but before attaching a
> domain.
> 
> There is no particularly good reason to have this coded in all the
> iommu drivers.

Yeah. that makes sense. We may have to adjust few things in driver (at least AMD
driver). But its doable.

-Vasant