Re: [Qemu-ppc] [Qemu-devel] [RFC for-2.10 2/3] pci: Allow host bridges to override PCI/PCIe hybrid device behaviour

[Marcel Apfelbaum]

On 04/18/2017 05:33 PM, Eduardo Habkost wrote:
> On Tue, Apr 18, 2017 at 12:21:51PM +1000, David Gibson wrote:
> > On Mon, Apr 17, 2017 at 03:30:46PM -0300, Eduardo Habkost wrote:
> > > On Tue, Mar 28, 2017 at 01:16:50PM +1100, David Gibson wrote:
> > > > Currently PCI/PCIe hybrid devices - that is, devices which can appear as
> > > > either plain PCI or PCIe depending on where they're attached - will only
> > > > appear in PCIe mode if they're attached to a PCIe bus via a root port or
> > > > downstream port.
> > > >
> > > > This is correct for "standard" PCIe setups, but there are some platforms
> > > > which need different behaviour (notably "pseries" whose paravirtualized
> > > > PCI host bridges have some idiosyncracies).
> > > >
> > > > This patch allows the host bridge to override the normal behaviour.
> > > >
> > > > Signed-off-by: David Gibson <address@hidden>
> > > > ---
> > > >  hw/pci/pci.c              | 11 +++++++++--
> > > >  include/hw/pci/pci_host.h |  1 +
> > > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 779787b..ac68065 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -392,9 +392,16 @@ bool pci_bus_is_root(PCIBus *bus)
> > > >
> > > >  bool pci_allow_hybrid_pcie(PCIDevice *pci_dev)
> > >
> > > Why are we asking the device about allowing hybrid pcie, and not
> > > the bus itself? Shouldn't we be able to query this before the
> > > device is plugged, and before pci_dev->bus is even set?
> > >
> > > In other words, why pci_allow_hyberid_pcie() and
> > > pci_device_get_bus() get a PCIDevice* argument instead of a
> > > PCIBus* argument?
> >
> > Ah good point.  I made it a PCIDevice simply for convenience, but
> > you're right we should be able to query before the device is plugged.
>
> Understood.
>
> > > >  {
> > > > -    PCIBus *bus = pci_dev->bus;
> > > > +    PCIHostState *host_bridge = 
> > > > PCI_HOST_BRIDGE(pci_device_root_bus(pci_dev)->qbus.parent);
> > >
> > > There's something I don't understand completely: what exactly
> > > guarantees that pci_device_root_bus(...)->qbus.parent is always
> > > going to be a PCI_HOST_BRIDGE?
> >
> > Well, by definition whatever is above the root bus isn't PCI, which
> > pretty much means it has to be a PCI host bridge.  A machine could
> > break this assumption, but I think that would be a bug.  We use this
> > same pattern to find a PCI device's (or bus's) host bridge in other
> > places - there doesn't appear to be another way.

Agreed, the root bus is always exposed by (some kind of) a host bridge.

> After taking a better look at the way PCI buses are created, I
> agree it's OK. Maybe a
>   PCIHostState *pci_host_bridge(PCIBus *bus)
> helper would be useful?

For sure.

> Anyway, both pci_bus_root_bus() and pci_host_bridge() could be
> implemented as follow-ups if you prefer, so:
>
> Reviewed-by: Eduardo Habkost <address@hidden>

Thanks,
Marcel

> But I still have another suggestion:
>
> > > > +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
> > > > +
> > > > +    if (hc->allow_hybrid_pcie) {
> > > > +        return hc->allow_hybrid_pcie(host_bridge, pci_dev);
> > > > +    } else {
> > > > +        PCIBus *bus = pci_dev->bus;
> > > >
> > > > -    return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
> > > > +        return pci_bus_is_express(bus) && !pci_bus_is_root(bus);
>
> Maybe it's a matter of personal taste, but instead of requiring
> an explicit check for NULL hc->allow_hybrid_pcie, why not set a
> default hc->allow_hybrid_pcie() implementation on
> TYPE_PCI_HOST_BRIDGE that returns
> (pci_bus_is_express(bus) && !pci_bus_is_root(bus))?