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

[Ganapatrao Kulkarni]

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.

> 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