diff mbox

[07/17] s390x/pci: introduce S390PCIBus

Message ID 20160624132906.14446-8-cornelia.huck@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck June 24, 2016, 1:28 p.m. UTC 
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(+)

Comments

Marcel Apfelbaum June 28, 2016, 2:39 p.m. UTC | #1 
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;
>
Cornelia Huck June 28, 2016, 3:20 p.m. UTC | #2 
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?
Marcel Apfelbaum June 28, 2016, 3:40 p.m. UTC | #3 
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
Cornelia Huck June 28, 2016, 5:15 p.m. UTC | #4 
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 mbox

Patch

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;