Re: [Qemu-devel] [PATCH 07/17] s390x/pci: introduce S390PCIBus

[Cornelia Huck]

On Tue, 28 Jun 2016 18:40:18 +0300
Marcel Apfelbaum <address@hidden> wrote:

> On 06/28/2016 06:20 PM, Cornelia Huck wrote:
> > On Tue, 28 Jun 2016 17:39:30 +0300
> > Marcel Apfelbaum <address@hidden> wrote:
> >
> >> On 06/24/2016 04:28 PM, Cornelia Huck wrote:
> >>> From: Yi Min Zhao <address@hidden>
> >>>
> >>> To enable S390PCIBusDevice as qdev, there should be a new bus to
> >>> plug and manage all instances of S390PCIBusDevice. Due to this,
> >>> S390PCIBus is introduced.
> >>>
> >>> Signed-off-by: Yi Min Zhao <address@hidden>
> >>> Reviewed-by: Pierre Morel <address@hidden>
> >>> Signed-off-by: Cornelia Huck <address@hidden>
> >>> ---
> >>>    hw/s390x/s390-pci-bus.c | 10 ++++++++++
> >>>    hw/s390x/s390-pci-bus.h |  8 ++++++++
> >>>    2 files changed, 18 insertions(+)
> >>>
> >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >>> index 0f6fcef..0c67c1e 100644
> >>> --- a/hw/s390x/s390-pci-bus.c
> >>> +++ b/hw/s390x/s390-pci-bus.c
> >>> @@ -527,6 +527,9 @@ static int s390_pcihost_init(SysBusDevice *dev)
> >>>        bus = BUS(b);
> >>>        qbus_set_hotplug_handler(bus, DEVICE(dev), NULL);
> >>>        phb->bus = b;
> >>> +
> >>> +    s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, DEVICE(s), 
> >>> NULL));
> >>> +
> >>>        QTAILQ_INIT(&s->pending_sei);
> >>>        return 0;
> >>>    }
> >>> @@ -636,9 +639,16 @@ static const TypeInfo s390_pcihost_info = {
> >>>        }
> >>>    };
> >>>
> >>> +static const TypeInfo s390_pcibus_info = {
> >>> +    .name = TYPE_S390_PCI_BUS,
> >>> +    .parent = TYPE_BUS,
> >>
> >> Hi,
> >>
> >> The type is named TYPE_S390_PCI_BUS, but does not
> >> derive from PCI_BUS. I find it a little confusing, anyway is just a 
> >> thought.
> >> Maybe you should go with TYPE_S390_BUS.
> >
> > I think that would be even more confusing, as this is not the only bus
> > on s390 :)
> 
> I suppose you mean S390 has a few bus types and this one is associated with 
> PCI.

Yes.

> 
> >
> > I have trouble thinking of a better name, though. TYPE_S390PCI_BUS?
> >
> 
> This would give a hint that we are referring to a special "S390PCI" bus, but 
> is less readable.
> I like the previous name better :)
> Maybe TYPE_ZPCI_BUS ? Anyway, maybe just being aware of it is enough.

Probably yes. I think most people tempted to look at this file already
understand the purpose.