On Thu, 3 Jul 2025 16:17:27 -0600
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> On Thu, 3 Jul 2025 12:30:30 -0300
> Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> > On Tue, Jul 01, 2025 at 01:28:59PM -0600, Alex Williamson wrote:
> > > > +enum pci_bus_isolation pci_bus_isolated(struct pci_bus *bus)
> > > > +{
> > > > + struct pci_dev *bridge = bus->self;
> > > > + int type;
> > > > +
> > > > + /* Consider virtual busses isolated */
> > > > + if (!bridge)
> > > > + return PCIE_ISOLATED;
> > > > + if (pci_is_root_bus(bus))
> > > > + return PCIE_ISOLATED;
> > >
> > > How do we know the root bus isn't conventional?
> >
> > If I read this right this is dead code..
> >
> > /*
> > * Returns true if the PCI bus is root (behind host-PCI bridge),
> > * false otherwise
> > *
> > * Some code assumes that "bus->self == NULL" means that bus is a root bus.
> > * This is incorrect because "virtual" buses added for SR-IOV (via
> > * virtfn_add_bus()) have "bus->self == NULL" but are not root buses.
> > */
> > static inline bool pci_is_root_bus(struct pci_bus *pbus)
> > {
> > return !(pbus->parent);
> >
> > Looking at the call chain of pci_alloc_bus():
> > pci_alloc_child_bus() - Parent bus may not be NULL
> > pci_add_new_bus() - All callers pass !NULL bus
> > pci_register_host_bridge() - Sets self and parent to NULL
> >
> > Thus if pci_is_root() == true implies bus->self == NULL so we can't
> > get here.
>
> Yep, seems correct.
>
> > So I will change it to be like:
> >
> > /*
> > * This bus was created by pci_register_host_bridge(). There is nothing
> > * upstream of this, assume it contains the TA and that the root complex
> > * does not allow P2P without going through the IOMMU.
> > */
> > if (pci_is_root_bus(bus))
> > return PCIE_ISOLATED;
>
> Ok, but did we sidestep the question of whether the root bus can be
> conventional?
>
> >
> > /*
> > * Sometimes SRIOV VFs can have a "virtual" bus if the SRIOV RID's
> > * extend past the bus numbers of the parent. The spec says that SRIOV
> > * VFs and PFs should act the same as functions in a MFD. MFD isolation
> > * is handled outside this function.
> > */
> > if (!bridge)
> > return PCIE_ISOLATED;
> >
> > And now it seems we never took care with SRIOV, along with the PF
> > every SRIOV VF needs to have its ACS checked as though it was a MFD..
>
> There's actually evidence that we did take care to make sure VFs never
> flag themselves as multifunction in order to avoid the multifunction
> ACS tests. I think we'd see lots of devices suddenly unusable for one
> of their intended use cases if we grouped VFs that don't expose an ACS
> capability. Also VFs from multiple PFs exist on the same virtual bus,
> so I imagine if the PF supports ACS but the VF doesn't, you'd end up
> with multiple isolation domains on the same bus. Thus, we've so far
> take the approach that "surely the hw vendor intended these to be used
> independently", and only considered the isolation upstream from the VFs.
BTW, spec 6.0.1, section 6.12:
For ACS requirements, single-Function devices that are SR-IOV capable
must be handled as if they were Multi-Function Devices, since they
essentially behave as Multi-Function Devices after their Virtual
Functions (VFs) are enabled.
Also, section 7.7.11:
If an SR-IOV Capable Device other than one in a Root Complex
implements internal peer-to-peer transactions, ACS is required, and
ACS P2P Egress Control must be supported.
The latter says to me that a non root complex SR-IOV device that does
not implement ACS does not implement internal p2p routing. OTOH, the
former seems to suggest that we need to consider VFs as peers of the
PF, maybe even governed by ACS on the PF, relative to MF routing. I'm
not really finding anything that says the VF itself needs to implement
ACS. Thanks,
Alex
