[Qemu-devel] [PATCH v3 0/3] spapr qdevification

[Qemu-devel] [PATCH v3 0/3] spapr qdevification

[Paolo Bonzini]

This series fixes some problems with spapr's qdev interface.  Patch
1 is the important one, which makes it possible to use -device
to create vio devices.  The other two are cosmetic.

v1->v2:
        abstracted the call to xics_find_qirq behind spapr_find_qirq

v2->v3:
        undid v1->v2 change, introduced spapr_allocate_irq

Paolo Bonzini (3):
  spapr: proper qdevification
  spapr: prepare for qdevification of irq
  spapr: make irq customizable via qdev

 hw/spapr.c       |   16 ++++++----------
 hw/spapr.h       |    6 ++++++
 hw/spapr_llan.c  |   11 ++---------
 hw/spapr_vio.c   |    9 +++++++++
 hw/spapr_vio.h   |   18 +++++++++---------
 hw/spapr_vscsi.c |   12 ++----------
 hw/spapr_vty.c   |   10 ++--------
 7 files changed, 36 insertions(+), 46 deletions(-)

-- 
1.7.4.4

[Qemu-devel] [PATCH v3 1/3] spapr: proper qdevification

[Paolo Bonzini]

Right now the spapr devices cannot be instantiated with -device,
because the IRQs need to be passed to the spapr_*_create functions.
Do this instead in the bus's init wrapper.

This is particularly important with the conversion from scsi-disk
to scsi-{cd,hd} that Markus made.  After his patches, if you
specify a scsi-cd device attached to an if=none drive, the default
VSCSI controller will not be created and, without qdevification,
you will not be able to add yours.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/spapr.c       |   16 ++++++----------
 hw/spapr.h       |    6 ++++++
 hw/spapr_llan.c  |    7 +------
 hw/spapr_vio.c   |    3 +++
 hw/spapr_vio.h   |   13 ++++---------
 hw/spapr_vscsi.c |    8 +-------
 hw/spapr_vty.c   |    8 +-------
 7 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index 109b774..575dcaf 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -298,7 +298,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     long kernel_size, initrd_size, fw_size;
     long pteg_shift = 17;
     char *filename;
-    int irq = 16;
 
     spapr = qemu_malloc(sizeof(*spapr));
     cpu_ppc_hypercall = emulate_spapr_hypercall;
@@ -308,6 +307,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
      * necessary */
     spapr->fdt_addr = MIN(ram_size, 0x80000000) - FDT_MAX_SIZE;
     spapr->rtas_addr = spapr->fdt_addr - RTAS_MAX_SIZE;
+    spapr->next_irq = 16;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -360,15 +360,14 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     /* Set up VIO bus */
     spapr->vio_bus = spapr_vio_bus_init();
 
-    for (i = 0; i < MAX_SERIAL_PORTS; i++, irq++) {
+    for (i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
             spapr_vty_create(spapr->vio_bus, SPAPR_VTY_BASE_ADDRESS + i,
-                             serial_hds[i], xics_find_qirq(spapr->icp, irq),
-                             irq);
+                             serial_hds[i]);
         }
     }
 
-    for (i = 0; i < nb_nics; i++, irq++) {
+    for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
 
         if (!nd->model) {
@@ -376,8 +375,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         }
 
         if (strcmp(nd->model, "ibmveth") == 0) {
-            spapr_vlan_create(spapr->vio_bus, 0x1000 + i, nd,
-                              xics_find_qirq(spapr->icp, irq), irq);
+            spapr_vlan_create(spapr->vio_bus, 0x1000 + i, nd);
         } else {
             fprintf(stderr, "pSeries (sPAPR) platform does not support "
                     "NIC model '%s' (only ibmveth is supported)\n",
@@ -387,9 +385,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     }
 
     for (i = 0; i <= drive_get_max_bus(IF_SCSI); i++) {
-        spapr_vscsi_create(spapr->vio_bus, 0x2000 + i,
-                           xics_find_qirq(spapr->icp, irq), irq);
-        irq++;
+        spapr_vscsi_create(spapr->vio_bus, 0x2000 + i);
     }
 
     if (kernel_filename) {
diff --git a/hw/spapr.h b/hw/spapr.h
index b52133a..eb9e5a9 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -8,6 +8,7 @@ typedef struct sPAPREnvironment {
     struct VIOsPAPRBus *vio_bus;
     struct icp_state *icp;
 
+    int next_irq;
     void *htab;
     long htab_size;
     target_phys_addr_t fdt_addr, rtas_addr;
@@ -278,6 +279,11 @@ void spapr_register_hypercall(target_ulong opcode, spapr_hcall_fn fn);
 target_ulong spapr_hypercall(CPUState *env, target_ulong opcode,
                              target_ulong *args);
 
+static inline int spapr_allocate_irq(sPAPREnvironment *spapr)
+{
+    return spapr->next_irq++;
+}
+
 static inline uint32_t rtas_ld(target_ulong phys, int n)
 {
     return ldl_phys(phys + 4*n);
diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index c18efc7..2597748 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -195,11 +195,9 @@ static int spapr_vlan_init(VIOsPAPRDevice *sdev)
     return 0;
 }
 
-void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd,
-                       qemu_irq qirq, uint32_t vio_irq_num)
+void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd)
 {
     DeviceState *dev;
-    VIOsPAPRDevice *sdev;
 
     dev = qdev_create(&bus->bus, "spapr-vlan");
     qdev_prop_set_uint32(dev, "reg", reg);
@@ -207,9 +205,6 @@ void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd,
     qdev_set_nic_properties(dev, nd);
 
     qdev_init_nofail(dev);
-    sdev = (VIOsPAPRDevice *)dev;
-    sdev->qirq = qirq;
-    sdev->vio_irq_num = vio_irq_num;
 }
 
 static int spapr_vlan_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
index 481a804..6f34159 100644
--- a/hw/spapr_vio.c
+++ b/hw/spapr_vio.c
@@ -32,6 +32,7 @@
 
 #include "hw/spapr.h"
 #include "hw/spapr_vio.h"
+#include "hw/xics.h"
 
 #ifdef CONFIG_FDT
 #include <libfdt.h>
@@ -602,6 +603,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
     }
 
     dev->qdev.id = id;
+    dev->vio_irq_num = spapr_allocate_irq (spapr);
+    dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
 
     rtce_init(dev);
 
diff --git a/hw/spapr_vio.h b/hw/spapr_vio.h
index 603a8c4..faa5d94 100644
--- a/hw/spapr_vio.h
+++ b/hw/spapr_vio.h
@@ -62,6 +62,7 @@ typedef struct VIOsPAPRDevice {
 
 typedef struct VIOsPAPRBus {
     BusState bus;
+    int irq;
 } VIOsPAPRBus;
 
 typedef struct {
@@ -98,15 +99,9 @@ uint64_t ldq_tce(VIOsPAPRDevice *dev, uint64_t taddr);
 int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
 
 void vty_putchars(VIOsPAPRDevice *sdev, uint8_t *buf, int len);
-void spapr_vty_create(VIOsPAPRBus *bus,
-                      uint32_t reg, CharDriverState *chardev,
-                      qemu_irq qirq, uint32_t vio_irq_num);
-
-void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd,
-                       qemu_irq qirq, uint32_t vio_irq_num);
-
-void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg,
-                        qemu_irq qirq, uint32_t vio_irq_num);
+void spapr_vty_create(VIOsPAPRBus *bus, uint32_t reg, CharDriverState *chardev);
+void spapr_vlan_create(VIOsPAPRBus *bus, uint32_t reg, NICInfo *nd);
+void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg);
 
 int spapr_tce_set_bypass(uint32_t unit, uint32_t enable);
 void spapr_vio_quiesce(void);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 1c901ef..99a4b06 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -952,20 +952,14 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
     return 0;
 }
 
-void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg,
-                        qemu_irq qirq, uint32_t vio_irq_num)
+void spapr_vscsi_create(VIOsPAPRBus *bus, uint32_t reg)
 {
     DeviceState *dev;
-    VIOsPAPRDevice *sdev;
 
     dev = qdev_create(&bus->bus, "spapr-vscsi");
     qdev_prop_set_uint32(dev, "reg", reg);
 
     qdev_init_nofail(dev);
-
-    sdev = (VIOsPAPRDevice *)dev;
-    sdev->qirq = qirq;
-    sdev->vio_irq_num = vio_irq_num;
 }
 
 static int spapr_vscsi_devnode(VIOsPAPRDevice *dev, void *fdt, int node_off)
diff --git a/hw/spapr_vty.c b/hw/spapr_vty.c
index 6fc0105..fa97cf7 100644
--- a/hw/spapr_vty.c
+++ b/hw/spapr_vty.c
@@ -115,20 +115,14 @@ static target_ulong h_get_term_char(CPUState *env, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
-void spapr_vty_create(VIOsPAPRBus *bus,
-                      uint32_t reg, CharDriverState *chardev,
-                      qemu_irq qirq, uint32_t vio_irq_num)
+void spapr_vty_create(VIOsPAPRBus *bus, uint32_t reg, CharDriverState *chardev)
 {
     DeviceState *dev;
-    VIOsPAPRDevice *sdev;
 
     dev = qdev_create(&bus->bus, "spapr-vty");
     qdev_prop_set_uint32(dev, "reg", reg);
     qdev_prop_set_chr(dev, "chardev", chardev);
     qdev_init_nofail(dev);
-    sdev = (VIOsPAPRDevice *)dev;
-    sdev->qirq = qirq;
-    sdev->vio_irq_num = vio_irq_num;
 }
 
 static void vty_hcalls(VIOsPAPRBus *bus)
-- 
1.7.4.4

[Qemu-devel] [PATCH v3 2/3] spapr: prepare for qdevification of irq

[Paolo Bonzini]

Restructure common properties for sPAPR devices so that IRQ definitions
can be added in one place.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/spapr_llan.c  |    4 +---
 hw/spapr_vio.h   |    5 +++++
 hw/spapr_vscsi.c |    4 +---
 hw/spapr_vty.c   |    2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index 2597748..abe1297 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -495,9 +495,7 @@ static VIOsPAPRDeviceInfo spapr_vlan = {
     .qdev.name = "spapr-vlan",
     .qdev.size = sizeof(VIOsPAPRVLANDevice),
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("reg", VIOsPAPRDevice, reg, 0x1000),
-        DEFINE_PROP_UINT32("dma-window", VIOsPAPRDevice, rtce_window_size,
-                           0x10000000),
+        DEFINE_SPAPR_PROPERTIES(VIOsPAPRVLANDevice, sdev, 0x1000, 0x10000000),
         DEFINE_NIC_PROPERTIES(VIOsPAPRVLANDevice, nicconf),
         DEFINE_PROP_END_OF_LIST(),
     },
diff --git a/hw/spapr_vio.h b/hw/spapr_vio.h
index faa5d94..7eb5367 100644
--- a/hw/spapr_vio.h
+++ b/hw/spapr_vio.h
@@ -60,6 +60,11 @@ typedef struct VIOsPAPRDevice {
     VIOsPAPR_CRQ crq;
 } VIOsPAPRDevice;
 
+#define DEFINE_SPAPR_PROPERTIES(type, field, default_reg, default_dma_window) \
+        DEFINE_PROP_UINT32("reg", type, field.reg, default_reg), \
+        DEFINE_PROP_UINT32("dma-window", type, field.rtce_window_size, \
+                           default_dma_window)
+
 typedef struct VIOsPAPRBus {
     BusState bus;
     int irq;
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 99a4b06..194db23 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -989,9 +989,7 @@ static VIOsPAPRDeviceInfo spapr_vscsi = {
     .qdev.name = "spapr-vscsi",
     .qdev.size = sizeof(VSCSIState),
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("reg", VIOsPAPRDevice, reg, 0x2000),
-        DEFINE_PROP_UINT32("dma-window", VIOsPAPRDevice,
-                           rtce_window_size, 0x10000000),
+        DEFINE_SPAPR_PROPERTIES(VSCSIState, vdev, 0x2000, 0x10000000),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
diff --git a/hw/spapr_vty.c b/hw/spapr_vty.c
index fa97cf7..abc41f8 100644
--- a/hw/spapr_vty.c
+++ b/hw/spapr_vty.c
@@ -140,7 +140,7 @@ static VIOsPAPRDeviceInfo spapr_vty = {
     .qdev.name = "spapr-vty",
     .qdev.size = sizeof(VIOsPAPRVTYDevice),
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("reg", VIOsPAPRDevice, reg, 0),
+        DEFINE_SPAPR_PROPERTIES(VIOsPAPRVTYDevice, sdev, 0, 0),
         DEFINE_PROP_CHR("chardev", VIOsPAPRVTYDevice, chardev),
         DEFINE_PROP_END_OF_LIST(),
     },
-- 
1.7.4.4

[Qemu-devel] [PATCH v3 3/3] spapr: make irq customizable via qdev

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/spapr_vio.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
index 6f34159..a193caa 100644
--- a/hw/spapr_vio.c
+++ b/hw/spapr_vio.c
@@ -52,6 +52,10 @@
 static struct BusInfo spapr_vio_bus_info = {
     .name       = "spapr-vio",
     .size       = sizeof(VIOsPAPRBus),
+    .props = (Property[]) {
+        DEFINE_PROP_UINT32("irq", VIOsPAPRDevice, vio_irq_num, 0), \
+        DEFINE_PROP_END_OF_LIST(),
+    },
 };
 
 VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
@@ -603,7 +607,9 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
     }
 
     dev->qdev.id = id;
-    dev->vio_irq_num = spapr_allocate_irq (spapr);
+    if (!dev->vio_irq_num) {
+        dev->vio_irq_num = spapr_allocate_irq (spapr);
+    }
     dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
 
     rtce_init(dev);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH v3 3/3] spapr: make irq customizable via qdev

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/spapr_vio.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
> index 6f34159..a193caa 100644
> --- a/hw/spapr_vio.c
> +++ b/hw/spapr_vio.c
> @@ -52,6 +52,10 @@
>  static struct BusInfo spapr_vio_bus_info = {
>      .name       = "spapr-vio",
>      .size       = sizeof(VIOsPAPRBus),
> +    .props = (Property[]) {
> +        DEFINE_PROP_UINT32("irq", VIOsPAPRDevice, vio_irq_num, 0), \
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
>  };

Out of this patch's scope, but I need to ask anyway: do devices on a
spapr-vio bus have a unique address?  If yes, what is it?

>  
>  VIOsPAPRDevice *spapr_vio_find_by_reg(VIOsPAPRBus *bus, uint32_t reg)
> @@ -603,7 +607,9 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
>      }
>  
>      dev->qdev.id = id;
> -    dev->vio_irq_num = spapr_allocate_irq (spapr);
> +    if (!dev->vio_irq_num) {
> +        dev->vio_irq_num = spapr_allocate_irq (spapr);
> +    }
>      dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
>  
>      rtce_init(dev);

Is it okay to share interrupts?  I'm asking because spapr_allocate_irq()
will happily assign IRQs already used via property.  From PATCH 1/1:

static inline int spapr_allocate_irq(sPAPREnvironment *spapr)
{
    return spapr->next_irq++;
}

Re: [Qemu-devel] [PATCH v3 3/3] spapr: make irq customizable via qdev

[Paolo Bonzini]

On 06/07/2011 09:51 AM, Markus Armbruster wrote:
>> >  diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
>> >  index 6f34159..a193caa 100644
>> >  --- a/hw/spapr_vio.c
>> >  +++ b/hw/spapr_vio.c
>> >  @@ -52,6 +52,10 @@
>> >    static struct BusInfo spapr_vio_bus_info = {
>> >        .name       = "spapr-vio",
>> >        .size       = sizeof(VIOsPAPRBus),
>> >  +    .props = (Property[]) {
>> >  +        DEFINE_PROP_UINT32("irq", VIOsPAPRDevice, vio_irq_num, 0), \
>> >  +        DEFINE_PROP_END_OF_LIST(),
>> >  +    },
>> >    };
>
> Out of this patch's scope, but I need to ask anyway: do devices on a
> spapr-vio bus have a unique address?  If yes, what is it?

That would be the "reg" value in DEFINE_SPAPR_PROPERTIES (patch 2).  I 
didn't make it a bus property because the default values varies 
according to the device type.

> Is it okay to share interrupts?

Seems to work. :)

Paolo

Re: [Qemu-devel] [PATCH v3 3/3] spapr: make irq customizable via qdev

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/07/2011 09:51 AM, Markus Armbruster wrote:
>>> >  diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
>>> >  index 6f34159..a193caa 100644
>>> >  --- a/hw/spapr_vio.c
>>> >  +++ b/hw/spapr_vio.c
>>> >  @@ -52,6 +52,10 @@
>>> >    static struct BusInfo spapr_vio_bus_info = {
>>> >        .name       = "spapr-vio",
>>> >        .size       = sizeof(VIOsPAPRBus),
>>> >  +    .props = (Property[]) {
>>> >  +        DEFINE_PROP_UINT32("irq", VIOsPAPRDevice, vio_irq_num, 0), \
>>> >  +        DEFINE_PROP_END_OF_LIST(),
>>> >  +    },
>>> >    };
>>
>> Out of this patch's scope, but I need to ask anyway: do devices on a
>> spapr-vio bus have a unique address?  If yes, what is it?
>
> That would be the "reg" value in DEFINE_SPAPR_PROPERTIES (patch 2).  I
> didn't make it a bus property because the default values varies
> according to the device type.

Hmm.  Explain that in a comment where the bus property would go?

>> Is it okay to share interrupts?
>
> Seems to work. :)

Fair enough.

Re: [Qemu-devel] [PATCH v3 3/3] spapr: make irq customizable via qdev

[Paolo Bonzini]

On 06/07/2011 11:19 AM, Markus Armbruster wrote:
> >  That would be the "reg" value in DEFINE_SPAPR_PROPERTIES (patch 2).  I
> >  didn't make it a bus property because the default values varies
>
> Hmm.  Explain that in a comment where the bus property would go?

qdev.txt is probably a better place to document the possible approaches...

Paolo

Re: [Qemu-devel] [PATCH v3 0/3] spapr qdevification

[Paolo Bonzini]

On 06/06/2011 04:16 PM, Paolo Bonzini wrote:
> This series fixes some problems with spapr's qdev interface.  Patch
> 1 is the important one, which makes it possible to use -device
> to create vio devices.  The other two are cosmetic.
>
> v1->v2:
>          abstracted the call to xics_find_qirq behind spapr_find_qirq
>
> v2->v3:
>          undid v1->v2 change, introduced spapr_allocate_irq

Ping?

Re: [Qemu-devel] [PATCH v3 0/3] spapr qdevification

[Alexander Graf]

On 07/06/2011 01:32 PM, Paolo Bonzini wrote:
> On 06/06/2011 04:16 PM, Paolo Bonzini wrote:
>> This series fixes some problems with spapr's qdev interface.  Patch
>> 1 is the important one, which makes it possible to use -device
>> to create vio devices.  The other two are cosmetic.
>>
>> v1->v2:
>>          abstracted the call to xics_find_qirq behind spapr_find_qirq
>>
>> v2->v3:
>>          undid v1->v2 change, introduced spapr_allocate_irq
>
> Ping?

Thanks, applied to ppc-next :)


Alex