Re: [Qemu-ppc] [RFC for-4.1 5/5] pcie: Simplify pci_adjust_config_limit()

[David Gibson]

On Tue, Apr 02, 2019 at 03:53:05PM +0200, Greg Kurz wrote:
> On Tue,  2 Apr 2019 16:40:28 +1100
> David Gibson <address@hidden> wrote:
> 
> > Since c2077e2c "pci: Adjust PCI config limit based on bus topology",
> > pci_adjust_config_limit() has been used in the config space read and write
> > paths to only permit access to extended config space on buses which permit
> > it.  Specifically it prevents access on devices below a vanilla-PCI bus via
> > some combination of bridges, even if both the host bridge and the device
> > itself are PCI-E.
> > 
> > It accomplishes this with a somewhat complex call up the chain of bridges
> > to see if any of them prohibit extended config space access.  This is
> > overly complex, since we can always know if the bus will support such
> > access at the point it is constructed.
> > 
> > This patch simplifies the test by using a flag in the PCIBus instance
> > indicating wither extended configuration space is accessible.  It is always
> > false for vanilla PCI buses.  For PCI-E buses, it is true for root buses
> > and equal to the parent bus's's capability otherwise.
> > 
> > This should cause no behavioural change.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> 
> LGTM. Some remarks below.
> 
> >  hw/pci/pci.c             | 36 ++++++++++++++++++++++--------------
> >  hw/pci/pci_host.c        | 13 +++----------
> >  hw/ppc/spapr_pci.c       | 24 +++++++++---------------
> >  include/hw/pci/pci_bus.h |  3 ++-
> >  4 files changed, 36 insertions(+), 40 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index ea5941fb22..420496571e 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
> 
> s/supported/support

Corrected, along with the block comment formatting.

> > +     * 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_allows_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);
> > @@ -142,11 +161,6 @@ static uint16_t pcibus_numa_node(PCIBus *bus)
> >      return NUMA_NODE_UNASSIGNED;
> >  }
> >  
> > -static bool pcibus_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    return false;
> > -}
> > -
> >  static void pci_bus_class_init(ObjectClass *klass, void *data)
> >  {
> >      BusClass *k = BUS_CLASS(klass);
> > @@ -161,7 +175,6 @@ static void pci_bus_class_init(ObjectClass *klass, void 
> > *data)
> >  
> >      pbc->bus_num = pcibus_num;
> >      pbc->numa_node = pcibus_numa_node;
> > -    pbc->allows_extended_config_space = 
> > pcibus_allows_extended_config_space;
> >  }
> >  
> >  static const TypeInfo pci_bus_info = {
> > @@ -182,16 +195,11 @@ static const TypeInfo conventional_pci_interface_info 
> > = {
> >      .parent        = TYPE_INTERFACE,
> >  };
> >  
> > -static bool pciebus_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    return true;
> > -}
> > -
> >  static void pcie_bus_class_init(ObjectClass *klass, void *data)
> >  {
> > -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> > +    BusClass *k = BUS_CLASS(klass);
> >  
> > -    pbc->allows_extended_config_space = 
> > pciebus_allows_extended_config_space;
> > +    k->realize = pcie_bus_realize;
> >  }
> >  
> >  static const TypeInfo pcie_bus_info = {
> > @@ -412,7 +420,7 @@ bool pci_bus_is_express(PCIBus *bus)
> >  
> >  bool pci_bus_allows_extended_config_space(PCIBus *bus)
> >  {
> > -    return PCI_BUS_GET_CLASS(bus)->allows_extended_config_space(bus);
> > +    return !!(bus->flags & PCI_BUS_EXTENDED_CONFIG_SPACE);
> >  }
> >  
> 
> Maybe make this a static inline in pci_bus.h like you already did with
> pci_bus_is_root() in the previous patch ?

I thought I'd tried that earlier and run into include hell because of
the target vs. non-target distinction.  But I must have been mistaken
because I tried it again and it worked fine.

> >  void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState 
> > *parent,
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 9d64b2e12f..5f3497256c 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -53,16 +53,9 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus 
> > *bus, uint32_t addr)
> >  
> >  static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
> >  {
> > -    if (*limit > PCI_CONFIG_SPACE_SIZE) {
> > -        if (!pci_bus_allows_extended_config_space(bus)) {
> > -            *limit = PCI_CONFIG_SPACE_SIZE;
> > -            return;
> > -        }
> > -
> > -        if (!pci_bus_is_root(bus)) {
> > -            PCIDevice *bridge = pci_bridge_get_device(bus);
> > -            pci_adjust_config_limit(pci_get_bus(bridge), limit);
> > -        }
> > +    if ((*limit > PCI_CONFIG_SPACE_SIZE) &&
> 
> parenthesitis ? ;-)
> 
> > +        !pci_bus_allows_extended_config_space(bus)) {
> > +        *limit = PCI_CONFIG_SPACE_SIZE;
> >      }
> >  }
> >  
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 2e76d8cbd8..23d70ca6fe 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1638,26 +1638,11 @@ static void spapr_phb_unrealize(DeviceState *dev, 
> > Error **errp)
> >      memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
> >  }
> >  
> > -static bool spapr_phb_allows_extended_config_space(PCIBus *bus)
> > -{
> > -    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(BUS(bus)->parent);
> > -
> > -    return sphb->pcie_ecs;
> > -}
> > -
> > -static void spapr_phb_root_bus_class_init(ObjectClass *klass, void *data)
> > -{
> > -    PCIBusClass *pbc = PCI_BUS_CLASS(klass);
> > -
> > -    pbc->allows_extended_config_space = 
> > spapr_phb_allows_extended_config_space;
> > -}
> > -
> >  #define TYPE_SPAPR_PHB_ROOT_BUS "spapr-pci-host-bridge-root-bus"
> >  
> >  static const TypeInfo spapr_phb_root_bus_info = {
> >      .name = TYPE_SPAPR_PHB_ROOT_BUS,
> >      .parent = TYPE_PCI_BUS,
> > -    .class_init = spapr_phb_root_bus_class_init,
> 
> The type is quite useless now, it should be dropped I guess, ie.
> git revert of "spapr_pci: Fix extended config space accesses".

Ah, good point.

> >  };
> >  
> >  static void spapr_phb_realize(DeviceState *dev, Error **errp)
> > @@ -1766,6 +1751,15 @@ static void spapr_phb_realize(DeviceState *dev, 
> > Error **errp)
> >                                  &sphb->memspace, &sphb->iospace,
> >                                  PCI_DEVFN(0, 0), PCI_NUM_PINS,
> >                                  TYPE_SPAPR_PHB_ROOT_BUS);
> > +
> > +    /*
> > +     * Despite resembling a vanilla PCI bus in most ways, the PAPR
> > +     * para-virtualized PCI bus *does* permit PCI-E extended config
> > +     * space access
> > +     */
> > +    if (sphb->pcie_ecs) {
> > +        bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +    }
> >      phb->bus = bus;
> >      qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
> >  
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index aea98d5040..3c0be4c420 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -17,12 +17,13 @@ typedef struct PCIBusClass {
> >  
> >      int (*bus_num)(PCIBus *bus);
> >      uint16_t (*numa_node)(PCIBus *bus);
> > -    bool (*allows_extended_config_space)(PCIBus *bus);
> >  } 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