> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Friday, August 9, 2024 9:29 PM
>
> On Fri, Aug 09, 2024 at 02:36:14AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Sent: Thursday, August 8, 2024 2:19 AM
> > >
> > > PCI ATS has a global Smallest Translation Unit field that is located in
> > > the PF but shared by all of the VFs.
> > >
> > > The expectation is that the STU will be set to the root port's global STU
> > > capability which is driven by the IO page table configuration of the iommu
> > > HW. Today it becomes set when the iommu driver first enables ATS.
> > >
> > > Thus, to enable ATS on the VF, the PF must have already had the correct
> > > STU programmed, even if ATS is off on the PF.
> > >
> > > Unfortunately the PF only programs the STU when the PF enables ATS.
> The
> > > iommu drivers tend to leave ATS disabled when IDENTITY translation is
> > > being used.
> >
> > Is there more context on this?
>
> How do you mean?
>
> > Looking at intel-iommu driver ATS is disabled for IDENETITY when
> > the iommu is in legacy mode:
> >
> > dmar_domain_attach_device()
> > {
> > ...
> > if (sm_supported(info->iommu) || !domain_type_is_si(info-
> >domain))
> > iommu_enable_pci_caps(info);
> > ...
> > }
> >
> > But this follows what VT-d spec says (section 9.3):
> >
> > TT: Translate Type
> > 10b: Untranslated requests are processed as pass-through. The SSPTPTR
> > field is ignored by hardware. Translated and Translation Requests are
> > blocked.
>
> Yes, HW like this is exactly the problem, it ends up not enabling ATS
> on the PF and then we don't have the STU programmed so the VF is
> effectively disabled too.
>
> Ideally iommus would continue to work with translated requests when
> ATS is enabled. Not supporting this configuration creates a nasty
> problem for devices that are using PASID.
>
> The PASID may require ATS to be enabled (ie SVA), but the RID may be
> IDENTITY for performance. The poor device has no idea it is not
> allowed to use ATS on the RID side :(
Okay, I see the problem now. 😊
>
> > > +/**
> > > + * pci_prepare_ats - Setup the PS for ATS
> > > + * @dev: the PCI device
> > > + * @ps: the IOMMU page shift
> > > + *
> > > + * This must be done by the IOMMU driver on the PF before any VFs are
> > > created to
> > > + * ensure that the VF can have ATS enabled.
> > > + *
> > > + * Returns 0 on success, or negative on failure.
> > > + */
> > > +int pci_prepare_ats(struct pci_dev *dev, int ps)
> > > +{
> > > + u16 ctrl;
> > > +
> > > + if (!pci_ats_supported(dev))
> > > + return -EINVAL;
> > > +
> > > + if (WARN_ON(dev->ats_enabled))
> > > + return -EBUSY;
> > > +
> > > + if (ps < PCI_ATS_MIN_STU)
> > > + return -EINVAL;
> > > +
> > > + if (dev->is_virtfn)
> > > + return 0;
> >
> > missed a check that 'ps' matches pf's ats_stu.
>
> Deliberate, that check is done when enabling ats:
>
> > > +
> > > + dev->ats_stu = ps;
> > > + ctrl = PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
> > > + pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_prepare_ats);
> > > +
> >
> > Then there is no need to keep the 'ps' parameter in pci_enable_ats().
>
> Which is why I left it here.
>
My initial thought was to save 'ps' to dev->ats_stu for both pf/vf
in pci_prepare_ats() then pci_enable_ats() can simply check
dev->ats_stu and dev->ats_enabled to avoid other two duplicated
checks and remove the 'ps' parameter.
But that comment is minor and if you still like the current way:
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
