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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

See
https://lore.kernel.org/all/0-v1-0fb4d2ab6770+7e706-ats_vf_jgg@nvidia.com/
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.

Jason