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: <95473081-db0b-4802-b875-24605ab2ef37@intel.com>
Date: Tue, 20 Aug 2024 16:55:26 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: Vasant Hegde <vasant.hegde@....com>, Baolu Lu <baolu.lu@...ux.intel.com>,
	Jason Gunthorpe <jgg@...pe.ca>
CC: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, "Robin
 Murphy" <robin.murphy@....com>, Kevin Tian <kevin.tian@...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 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