diff mbox series

[RFC,4/5] pci: Simplify pci_bus_is_root()

Message ID 20171003091423.28704-5-david@gibson.dropbear.id.au
State New
Headers show
Series Assorted PCI/PCIe cleanups cleanups | expand

Commit Message

David Gibson Oct. 3, 2017, 9:14 a.m. UTC 
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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

# Conflicts:
#	include/hw/pci/pci_bus.h
---
 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(-)

Comments

Eduardo Habkost Oct. 3, 2017, 2:42 p.m. UTC | #1 
On Tue, Oct 03, 2017 at 08:14:22PM +1100, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> # Conflicts:
> #	include/hw/pci/pci_bus.h

Should this be part of the commit message?

> ---
[...]
> 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;

Why not a simple boolean field?  If we want to keep the struct
size smaller when adding more flags, we can use bit-fields.

>      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,
> -- 
> 2.13.6
>
David Gibson Oct. 4, 2017, 6:03 a.m. UTC | #2 
On Tue, Oct 03, 2017 at 11:42:59AM -0300, Eduardo Habkost wrote:
> On Tue, Oct 03, 2017 at 08:14:22PM +1100, 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > # Conflicts:
> > #	include/hw/pci/pci_bus.h
> 
> Should this be part of the commit message?

Oops.  I'll fix that for the next spin.

> > ---
> [...]
> > 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;
> 
> Why not a simple boolean field?  If we want to keep the struct
> size smaller when adding more flags, we can use bit-fields.

Well, mst suggested "flags", so this is what came to mind.  I guess
being a past kernel dev has given be a tendency to avoid bitfields,
though I don't think any of the problems they can cause would apply
here.

Note that I do add a second flags bit in patch 5/5.

> 
> >      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,
>
diff mbox series

Patch

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 ffaa449ec1..4baf591562 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,