| Message ID | 20160624132906.14446-8-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06/24/2016 04:28 PM, Cornelia Huck wrote: > From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > > 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 <zyimin@linux.vnet.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > 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. Thanks, Marcel > + .instance_size = sizeof(S390PCIBus), > +}; > + > static void s390_pci_register_types(void) > { > type_register_static(&s390_pcihost_info); > + type_register_static(&s390_pcibus_info); > } > > type_init(s390_pci_register_types) > diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h > index e332f6a..c4d4079 100644 > --- a/hw/s390x/s390-pci-bus.h > +++ b/hw/s390x/s390-pci-bus.h > @@ -21,6 +21,7 @@ > #include "hw/s390x/css.h" > > #define TYPE_S390_PCI_HOST_BRIDGE "s390-pcihost" > +#define TYPE_S390_PCI_BUS "s390-pcibus" > #define FH_MASK_ENABLE 0x80000000 > #define FH_MASK_INSTANCE 0x7f000000 > #define FH_MASK_SHM 0x00ff0000 > @@ -31,6 +32,8 @@ > > #define S390_PCI_HOST_BRIDGE(obj) \ > OBJECT_CHECK(S390pciState, (obj), TYPE_S390_PCI_HOST_BRIDGE) > +#define S390_PCI_BUS(obj) \ > + OBJECT_CHECK(S390PCIBus, (obj), TYPE_S390_PCI_BUS) > > #define HP_EVENT_TO_CONFIGURED 0x0301 > #define HP_EVENT_RESERVED_TO_STANDBY 0x0302 > @@ -267,8 +270,13 @@ typedef struct S390PCIBusDevice { > IndAddr *indicator; > } S390PCIBusDevice; > > +typedef struct S390PCIBus { > + BusState qbus; > +} S390PCIBus; > + > typedef struct S390pciState { > PCIHostState parent_obj; > + S390PCIBus *bus; > S390PCIBusDevice pbdev[PCI_SLOT_MAX]; > AddressSpace msix_notify_as; > MemoryRegion msix_notify_mr; >
On Tue, 28 Jun 2016 17:39:30 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 06/24/2016 04:28 PM, Cornelia Huck wrote: > > From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > > > > 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 <zyimin@linux.vnet.ibm.com> > > Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > 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 have trouble thinking of a better name, though. TYPE_S390PCI_BUS?
On 06/28/2016 06:20 PM, Cornelia Huck wrote: > On Tue, 28 Jun 2016 17:39:30 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 06/24/2016 04:28 PM, Cornelia Huck wrote: >>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >>> >>> 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 <zyimin@linux.vnet.ibm.com> >>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> >>> --- >>> 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. > > 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. Thanks, Marcel
On Tue, 28 Jun 2016 18:40:18 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 06/28/2016 06:20 PM, Cornelia Huck wrote: > > On Tue, 28 Jun 2016 17:39:30 +0300 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> On 06/24/2016 04:28 PM, Cornelia Huck wrote: > >>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > >>> > >>> 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 <zyimin@linux.vnet.ibm.com> > >>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > >>> --- > >>> 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.
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, + .instance_size = sizeof(S390PCIBus), +}; + static void s390_pci_register_types(void) { type_register_static(&s390_pcihost_info); + type_register_static(&s390_pcibus_info); } type_init(s390_pci_register_types) diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h index e332f6a..c4d4079 100644 --- a/hw/s390x/s390-pci-bus.h +++ b/hw/s390x/s390-pci-bus.h @@ -21,6 +21,7 @@ #include "hw/s390x/css.h" #define TYPE_S390_PCI_HOST_BRIDGE "s390-pcihost" +#define TYPE_S390_PCI_BUS "s390-pcibus" #define FH_MASK_ENABLE 0x80000000 #define FH_MASK_INSTANCE 0x7f000000 #define FH_MASK_SHM 0x00ff0000 @@ -31,6 +32,8 @@ #define S390_PCI_HOST_BRIDGE(obj) \ OBJECT_CHECK(S390pciState, (obj), TYPE_S390_PCI_HOST_BRIDGE) +#define S390_PCI_BUS(obj) \ + OBJECT_CHECK(S390PCIBus, (obj), TYPE_S390_PCI_BUS) #define HP_EVENT_TO_CONFIGURED 0x0301 #define HP_EVENT_RESERVED_TO_STANDBY 0x0302 @@ -267,8 +270,13 @@ typedef struct S390PCIBusDevice { IndAddr *indicator; } S390PCIBusDevice; +typedef struct S390PCIBus { + BusState qbus; +} S390PCIBus; + typedef struct S390pciState { PCIHostState parent_obj; + S390PCIBus *bus; S390PCIBusDevice pbdev[PCI_SLOT_MAX]; AddressSpace msix_notify_as; MemoryRegion msix_notify_mr;