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] [day] [month] [year] [list]

Message-ID: <0f80d53f-ea8b-4f16-acf8-aa1c99966dfa@amd.com>
Date: Mon, 26 Aug 2024 14:41:18 +0530
From: Vasant Hegde <vasant.hegde@....com>
To: Jason Gunthorpe <jgg@...pe.ca>
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

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@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.

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

-Vasant