On Wed, Nov 29, 2017 at 12:45:28PM +0200, Marcel Apfelbaum wrote: > On 29/11/2017 10:46, David Gibson wrote: > > pci_bus_is_root() currently relies on a method in the PCIBusClass. > > But it's always known if a PCI bus is a root bus when we create it, so > > using a dynamic method is overkill. > > > > This replaces it with an IS_ROOT bit in a new flags field, which is set on > > root buses and otherwise clear. As a bonus this removes the special > > is_root logic from pci_expander_bridge, since it already creates its bus > > as a root bus. > > > > I agree it makes the code simpler, I am not sure about > the enum. Why not make it a boolean field and when will need > more flags just convert it to an enum (or not)?
Well, I have some patches I'm working on (rather slowly) which add a
second flag.
>
> Thanks,
> Marcel
>
> > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> > ---
> > hw/pci-bridge/pci_expander_bridge.c | 6 ------
> > hw/pci/pci.c | 14 ++------------
> > include/hw/pci/pci.h | 12 +++++++++++-
> > 3 files changed, 13 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 5652cf06e9..11dfa9258e 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -65,11 +65,6 @@ static int pxb_bus_num(PCIBus *bus)
> > return pxb->bus_nr;
> > }
> > -static bool pxb_is_root(PCIBus *bus)
> > -{
> > - return true; /* by definition */
> > -}
> > -
> > static uint16_t pxb_bus_numa_node(PCIBus *bus)
> > {
> > PXBDev *pxb = convert_to_pxb(bus->parent_dev);
> > @@ -82,7 +77,6 @@ static void pxb_bus_class_init(ObjectClass *class, void
> > *data)
> > PCIBusClass *pbc = PCI_BUS_CLASS(class);
> > pbc->bus_num = pxb_bus_num;
> > - pbc->is_root = pxb_is_root;
> > pbc->numa_node = pxb_bus_numa_node;
> > }
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6e11dc2fec..5fab7f23b3 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -126,14 +126,9 @@ static void pci_bus_unrealize(BusState *qbus, Error
> > **errp)
> > vmstate_unregister(NULL, &vmstate_pcibus, bus);
> > }
> > -static bool pcibus_is_root(PCIBus *bus)
> > -{
> > - return !bus->parent_dev;
> > -}
> > -
> > static int pcibus_num(PCIBus *bus)
> > {
> > - if (pcibus_is_root(bus)) {
> > + if (pci_bus_is_root(bus)) {
> > return 0; /* pci host bridge */
> > }
> > return bus->parent_dev->config[PCI_SECONDARY_BUS];
> > @@ -156,7 +151,6 @@ static void pci_bus_class_init(ObjectClass *klass, void
> > *data)
> > k->unrealize = pci_bus_unrealize;
> > k->reset = pcibus_reset;
> > - pbc->is_root = pcibus_is_root;
> > pbc->bus_num = pcibus_num;
> > pbc->numa_node = pcibus_numa_node;
> > }
> > @@ -385,6 +379,7 @@ static void pci_root_bus_init(PCIBus *bus, DeviceState
> > *parent,
> > bus->slot_reserved_mask = 0x0;
> > bus->address_space_mem = address_space_mem;
> > bus->address_space_io = address_space_io;
> > + bus->flags |= PCI_BUS_IS_ROOT;
> > /* host bridge */
> > QLIST_INIT(&bus->child);
> > @@ -397,11 +392,6 @@ bool pci_bus_is_express(PCIBus *bus)
> > return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
> > }
> > -bool pci_bus_is_root(PCIBus *bus)
> > -{
> > - return PCI_BUS_GET_CLASS(bus)->is_root(bus);
> > -}
> > -
> > 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 77d92a3dc4..cbb3386207 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -404,6 +404,11 @@ typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void
> > *, int);
> > #define PCI_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(PCIBusClass, (obj),
> > TYPE_PCI_BUS)
> > #define TYPE_PCIE_BUS "PCIE"
> > +enum PCIBusFlags {
> > + /* This bus is the root of a PCI domain */
> > + PCI_BUS_IS_ROOT = 0x0001,
> > +};
> > +
> > typedef struct PCIBusClass {
> > /*< private >*/
> > BusClass parent_class;
> > @@ -416,6 +421,7 @@ typedef struct PCIBusClass {
> > struct PCIBus {
> > BusState qbus;
> > + enum PCIBusFlags flags;
> > PCIIOMMUFunc iommu_fn;
> > void *iommu_opaque;
> > uint8_t devfn_min;
> > @@ -440,8 +446,12 @@ struct PCIBus {
> > Notifier machine_done;
> > };
> > +static inline bool pci_bus_is_root(PCIBus *bus)
> > +{
> > + return !!(bus->flags & PCI_BUS_IS_ROOT);
> > +}
> > +
> > bool pci_bus_is_express(PCIBus *bus);
> > -bool pci_bus_is_root(PCIBus *bus);
> > void pci_root_bus_new_inplace(PCIBus *bus, size_t bus_size, DeviceState
> > *parent,
> > const char *name,
> > MemoryRegion *address_space_mem,
> >
>
--
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
