Re: [Qemu-devel] [RFC 4/5] pci: Simplify pci_bus_is_root()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2823 bytes --]

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,
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]