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

[Yi Liu]

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