Re: [PATCH] PCI: s390: Handle ARI on bus without associated struct pci_dev

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Thu, 2024-08-01 at 11:59 -0500, Bjorn Helgaas wrote:
> On Tue, Jul 30, 2024 at 09:59:13PM +0200, Niklas Schnelle wrote:
> > On Tue, 2024-07-30 at 21:36 +0200, Niklas Schnelle wrote:
> > > On s390 PCI busses are virtualized and the downstream ports are
> > > invisible to the OS and struct pci_bus::self is NULL. This associated
> > > struct pci_dev is however relied upon in pci_ari_enabled() to check
> > > whether ARI is enabled for the bus. ARI is therefor always detected as
> > > disabled.
> > > 
> > > At the same time firmware on s390 always enables and relies upon ARI
> > > thus causing a mismatch. Moreover with per-PCI function pass-through
> > > there may exist busses with no function with devfn 0. For example
> > > a SR-IOV capable device with two PFs may have separate function
> > > dependency link chains for each of the PFs and their child VFs. In this
> > > case the OS may only see the second PF and its child VFs on a bus
> > > without a devfn 0 function. A situation which is also not supported by
> > > the common pci_configure_ari() code.
> > > 
> > > Dispite simply being a mismatch this causes problems as some PCI devices
> > > present a different SR-IOV topology depending on PCI_SRIOV_CTRL_ARI.
> > > 
> > > A similar mismatch may occur with SR-IOV when virtfn_add_bus() creates new
> > > busses with no associated struct pci_dev. Here too pci_ari_enabled()
> > > on these busses would return false even if ARI is actually used.
> > > 
> > > Prevent both mismatches by moving the ari_enabled flag from struct
> > > pci_dev to struct pci_bus making it independent from struct pci_bus::
> > > self. Let the bus inherit the ari_enabled state from its parent bus when
> > > there is no bridge device such that busses added by virtfn_add_bus()
> > > match their parent. For s390 set ari_enabled when the device supports
> > > ARI in the awareness that all PCIe ports on s390 systems are ARI
> > > capable.
> > > 
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > ---
---8<---
> > @@ -3523,12 +3524,18 @@ void pci_configure_ari(struct pci_dev *dev)
> >         u32 cap;
> >         struct pci_dev *bridge;
> > 
> > -       if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
> > +       if (pcie_ari_disabled || !pci_is_pcie(dev))
> > +               return;
> > +
> > +       if (dev->devfn && !hypervisor_isolated_pci_functions())
> >                 return;
> > 
> >         bridge = dev->bus->self;
> > -       if (!bridge)
> > +       if (!bridge) {
> > +               if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> > +                       dev->bus->ari_enabled = 1;
> 
> In the generic case here, how do we know whether the invisible bridge
> leading here has ARI enabled?  If that's known to always be the case
> for s390, I understand that, but I don't understand the other cases
> (jailhouse, passthrough, etc).

Good point! Yes this is probably not correct if Jailhouse doesn't also
guarantee ARI. I guess if we really want the generic solution, and I'm
fine with an s390 specific one too, then we would need to add some
indication that the invisible bridges support ARI. Honestly I'm not
even entirely sure if the bridge is even NULL on jailhouse too. In
QEMU/KVM for example I think everyone besides s390 emulates bridges.

> 
> >                 return;
> > +       }
> > 
> >         pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
> >         if (!(cap & PCI_EXP_DEVCAP2_ARI))
> > 
>