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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240820135153.GW3468552@ziepe.ca>
Date: Tue, 20 Aug 2024 10:51:53 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Vasant Hegde <vasant.hegde@....com>
Cc: Baolu Lu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>,
	Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
	Kevin Tian <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>,
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] iommu/vt-d: Move PCI PASID enablement to probe path

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