Re: [PATCH] PCI/ATS: Allow to enable ATS on VFs even if it is not enabled on PF

[Ganapatrao Kulkarni]

Hi Bjorn,

On 16-02-2023 05:06 pm, Ganapatrao Kulkarni wrote:
> On 16-02-2023 04:42 pm, Jean-Philippe Brucker wrote:
> > On Wed, Feb 15, 2023 at 02:57:26PM -0600, Bjorn Helgaas wrote:
> > > [+cc Will, Robin, Joerg for arm-smmu-v3 page size question]
> > >
> > > On Sun, Feb 12, 2023 at 08:14:48PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Feb 08, 2023 at 10:43:21AM -0800, Ganapatrao Kulkarni wrote:
> > > > > As per PCIe specification(section 10.5), If a VF implements an
> > > > > ATS capability, its associated PF must implement an ATS capability.
> > > > > The ATS Capabilities in VFs and their associated PFs are permitted to
> > > > > be enabled independently.
> > > > > Also, it states that the Smallest Translation Unit (STU) for VFs 
> > > > > must be
> > > > > hardwired to Zero and the associated PF's value applies to VFs STU.
> > > > >
> > > > > The current code allows to enable ATS on VFs only if it is already
> > > > > enabled on associated PF, which is not necessary as per the 
> > > > > specification.
> > > > >
> > > > > It is only required to have valid STU programmed on PF to enable
> > > > > ATS on VFs. Adding code to write the first VFs STU to a PF's STU
> > > > > when PFs ATS is not enabled.
> > > >
> > > > Can you please add here quotes from the spec and its version? I 
> > > > don't see
> > > > anything like this in my version of PCIe specification.
> > >
> > > See PCIe r6.0, sec 10.5.1.
> > >
> > > > > Signed-off-by: Ganapatrao Kulkarni 
> > > > > <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >   drivers/pci/ats.c | 15 +++++++++++----
> > > > >   1 file changed, 11 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > > index f9cc2e10b676..a97ec67201d1 100644
> > > > > --- a/drivers/pci/ats.c
> > > > > +++ b/drivers/pci/ats.c
> > > > > @@ -67,13 +67,20 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > > > >       if (ps < PCI_ATS_MIN_STU)
> > > > >           return -EINVAL;
> > > > > -    /*
> > > > > -     * Note that enabling ATS on a VF fails unless it's already 
> > > > > enabled
> > > > > -     * with the same STU on the PF.
> > > > > -     */
> > > > >       ctrl = PCI_ATS_CTRL_ENABLE;
> > > > >       if (dev->is_virtfn) {
> > > > >           pdev = pci_physfn(dev);
> > > > > +
> > > > > +        if (!pdev->ats_enabled &&
> > > > > +                (pdev->ats_stu < PCI_ATS_MIN_STU)) {
> > > > > +            u16 ctrl2;
> > > > > +
> > > > > +            /* Associated PF's STU value applies to VFs. */
> > > > > +            pdev->ats_stu = ps;
> > > > > +            ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu - 
> > > > > PCI_ATS_MIN_STU);
> > > > > +            pci_write_config_word(pdev, pdev->ats_cap + 
> > > > > PCI_ATS_CTRL, ctrl2);
> > > > > +        }
> > >
> > > For reference, it is this way because of edc90fee916b ("PCI: Allocate
> > > ATS struct during enumeration").  The rationale was that since the PF
> > > STU applies to all VFs, we should require that the PF STU be
> > > programmed before enabling ATS on any of the VFs.
> > >
> > > This patch relaxes that so the PF STU would be set either by (a)
> > > enabling ATS on the PF or (b) enabling ATS on the first VF.
> > >
> > > This looks racy because theoretically drivers for VF A and VF B could
> > > independently call pci_enable_ats() with different IOMMU page sizes,
> > > and we don't know which will get there first.
> > >
> > > Most callers supply a compile-time constant (PAGE_SHIFT or
> > > VTD_PAGE_SHIFT), so it won't matter.  arm_smmu_enable_ats() is
> > > fancier, but I *assume* it would still supply the same IOMMU page size
> > > for all VFs of a given PF.
> > >
> > > But it's still kind of ugly to call pci_enable_ats(dev_A) and have it
> > > muck with the configuration of dev_B.  Maybe we should configure the
> > > PF STU (without enabling ATS) at enumeration-time in pci_ats_init()?
> > > Is there some way to get the IOMMU page size at that time?
> >
> > Not really, on Arm the supported page sizes are discovered when probing
> > the SMMU registers, which may happen later than enumeration, during 
> > module
> > loading.
> >
> > What this patch is trying to solve is:
> > * want the PF to bypass SMMU translation, and the VF to undergo SMMU
> >    translation (in order to assign it to a VM)
> > * SMMU forbids enabling ATS for a configuration that bypasses 
> > translation.
> >    So the PF ATS capability must be left disabled.
> >
> > For this situation I wonder if we could do: the SMMU driver, seeing that
> > the PF is configured to bypass, calls a new function 
> > "pci_configure_ats()"
>
> IMO, This seems to be feasible solution for this situation, which 
> addresses SMMU-ATS expectation in bypass and we could avoid PCI VFs 
> race. pci_configure_ats() can be called to program/configure STU of a PF 
> in smmu-bypass mode.

Can we add pci_configure_ats/pci_configure_ats_stu helper?

> > instead of pci_enable_ats(), which would only set the STU but leave the
> > cap disabled. Then when setting up translation for the VF, the SMMU 
> > driver
> > calls pci_enable_ats() as usual, which sees the PF's STU set 
> > appropriately
> > and succeeds.
> >
> > Thanks,
> > Jean
>
> Thanks,
> Ganapat

Thanks,
Ganapat