Re: [Qemu-devel] [for-2.12 4/7] pci: Simplify pci_bus_is_root()

[Marcel Apfelbaum <marcel@redhat.com>]

On 29/11/2017 15:12, David Gibson wrote:
> 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.
> 

OK

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

>>
>> Thanks,
>> Marcel
>>
>>> Signed-off-by: David Gibson <david@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,
>>>
>>
>