Re: [Qemu-devel] [PATCH 2/2] pcie: Don't allow extended config space access via conventional PCI bridges

[David Gibson]

On Thu, Feb 14, 2019 at 05:04:03PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 14/02/2019 16:08, David Gibson wrote:
> > In hardware it's possible, if odd, to have a configuration like:
> > 
> > PCIe host bridge
> > \- PCIe to PCI bridge
> >    \- PCI to PCIe bridge
> >       \- PCIe device
> > 
> > The PCIe extended configuration space on the device won't be
> > accessible to the host, because the cycles can't traverse the
> > conventional PCI bus on the way there.
> > 
> > However, if we attempt to model that configuration under qemu,
> > extended config access on the device *will* work, because
> > pci_config_size() depends only on whether the device itself is PCIe
> > capable.
> > 
> > This patch fixes that modelling error by adding a flag to each
> > PCI/PCIe bus instance indicating whether extended config space
> > accesses are possible on it.  It will always be false for conventional
> > PCI buses, for PCIe buses it will be true if and only if the parent
> > bus also has the flag set.
> > 
> > AIUI earlier attempts to correct this have been rejected, because they
> > involved expensively traversing the whole bus hierarchy on each config
> > access.  This approach avoids that by computing the value as the bus
> > hierarchy is constructed, meaning we only need a single bit check when
> > we actually attempt the config access.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/pci/pci.c             | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci.h     |  4 +++-
> >  include/hw/pci/pci_bus.h |  2 ++
> >  3 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index f6d8b337db..f2d9dff9ee 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -120,6 +120,25 @@ static void pci_bus_realize(BusState *qbus, Error 
> > **errp)
> >      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> >  }
> >  
> > +static void pcie_bus_realize(BusState *qbus, Error **errp)
> > +{
> > +    PCIBus *bus = PCI_BUS(qbus);
> > +
> > +    pci_bus_realize(qbus, errp);
> > +
> > +    /* a PCI-E bus can supported extended config space if it's the
> > +     * root bus, or if the bus/bridge above it does as well */
> > +    if (pci_bus_is_root(bus)) {
> > +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +    } else {
> > +        PCIBus *parent_bus = pci_get_bus(bus->parent_dev);
> > +
> > +        if (pci_bus_extended_config_space(parent_bus)) {
> > +            bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +        }
> > +    }
> > +}
> > +
> >  static void pci_bus_unrealize(BusState *qbus, Error **errp)
> >  {
> >      PCIBus *bus = PCI_BUS(qbus);
> > @@ -166,6 +185,13 @@ static const TypeInfo pci_bus_info = {
> >      .class_init = pci_bus_class_init,
> >  };
> >  
> > +static void pcie_bus_class_init(ObjectClass *klass, void *data)
> > +{
> > +    BusClass *k = BUS_CLASS(klass);
> > +
> > +    k->realize = pcie_bus_realize;
> > +}
> > +
> >  static const TypeInfo pcie_interface_info = {
> >      .name          = INTERFACE_PCIE_DEVICE,
> >      .parent        = TYPE_INTERFACE,
> > @@ -174,6 +200,7 @@ static const TypeInfo pcie_interface_info = {
> >  static const TypeInfo conventional_pci_interface_info = {
> >      .name          = INTERFACE_CONVENTIONAL_PCI_DEVICE,
> >      .parent        = TYPE_INTERFACE,
> > +    .class_init    = pcie_bus_class_init,
> >  };
> >  
> >  static const TypeInfo pcie_bus_info = {
> > @@ -391,6 +418,11 @@ bool pci_bus_is_express(PCIBus *bus)
> >      return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
> >  }
> >  
> > +bool pci_bus_extended_config_space(PCIBus *bus)
> > +{
> > +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> > +}
> > +
> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState 
> > *parent,
> >                                const char *name,
> >                                MemoryRegion *address_space_mem,
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 1273deb740..919e8a6f5f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -395,6 +395,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, 
> > int pin);
> >  #define TYPE_PCIE_BUS "PCIE"
> >  
> >  bool pci_bus_is_express(PCIBus *bus);
> > +bool pci_bus_extended_config_space(PCIBus *bus);
> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState 
> > *parent,
> >                                const char *name,
> >                                MemoryRegion *address_space_mem,
> > @@ -754,7 +755,8 @@ static inline int pci_is_express_downstream_port(const 
> > PCIDevice *d)
> >  
> >  static inline uint32_t pci_config_size(const PCIDevice *d)
> >  {
> > -    return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : 
> > PCI_CONFIG_SPACE_SIZE;
> > +    return (pci_is_express(d) && 
> > pci_bus_extended_config_space(pci_get_bus(d)))
> > +        ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
> 
> 
> Since there is a selfnack anyway, I'll ask out of curiosity - can a
> device sit on PCIe bus and not be PCIe itself?

I believe so.  I think the most common case is plain-PCI integrated
devices in an otherwise PCI-E host bridge / root complex.

> The pci_is_express(d) check above just seems a little redundant,
> g_assert() could probably do just that.
> 
> 
> 
> 
> >  }
> >  
> >  static inline uint16_t pci_get_bdf(PCIDevice *dev)
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 3a4d599da3..8b1e849c34 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -23,6 +23,8 @@ typedef struct PCIBusClass {
> >  enum PCIBusFlags {
> >      /* This bus is the root of a PCI domain */
> >      PCI_BUS_IS_ROOT                                         = 0x0001,
> > +    /* PCIe extended configuration space is accessible on this bus */
> > +    PCI_BUS_EXTENDED_CONFIG_SPACE                           = 0x0002,
> >  };
> >  
> >  struct PCIBus {
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature