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

[Qemu-devel] [PATCH 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.

Paolo Bonzini (3):
  spapr: allow creating devices with -device
  spapr: prepare for qdevification of irq
  spapr: make irq customizable via qdev

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

-- 
1.7.4.4

[Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[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>
Cc: Alexander Graf <agraf@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr.c       |   15 +++++----------
 hw/spapr_llan.c  |    7 +------
 hw/spapr_vio.c   |    5 +++++
 hw/spapr_vio.h   |   13 ++++---------
 hw/spapr_vscsi.c |    8 +-------
 hw/spapr_vty.c   |    8 +-------
 6 files changed, 17 insertions(+), 39 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index 109b774..07b2165 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;
@@ -360,15 +359,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 +374,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 +384,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_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..be535d6 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>
@@ -595,6 +596,7 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
 {
     VIOsPAPRDeviceInfo *info = (VIOsPAPRDeviceInfo *)qinfo;
     VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
+    VIOsPAPRBus *bus = DO_UPCAST(VIOsPAPRBus, bus, dev->qdev.parent_bus);
     char *id;
 
     if (asprintf(&id, "%s@%x", info->dt_name, dev->reg) < 0) {
@@ -602,6 +604,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
     }
 
     dev->qdev.id = id;
+    dev->vio_irq_num = bus->irq++;
+    dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
 
     rtce_init(dev);
 
@@ -656,6 +660,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
 
     qbus = qbus_create(&spapr_vio_bus_info, dev, "spapr-vio");
     bus = DO_UPCAST(VIOsPAPRBus, bus, qbus);
+    bus->irq = 16;
 
     /* hcall-vio */
     spapr_register_hypercall(H_VIO_SIGNAL, h_vio_signal);
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 fdad3d2..89f7ce2 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -894,20 +894,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

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.

Really?  Hasn't that always been the case?

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[Paolo Bonzini]

On 05/24/2011 03:03 PM, Markus Armbruster wrote:
> >  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.
>
> Really?  Hasn't that always been the case?

What hasn't always been the case? :)

1) "the default VSCSI controller will not be created" -- no, this is new 
with scsi-cd: scsi-disk never was on the default_driver_table in vl.c, 
as you said in the commit message for af6bf13 (defaults: ide-cd, ide-hd 
and scsi-cd devices suppress default CD-ROM, 2011-05-18).  In fact, I 
believe you could add scsi-hd there too.

2) "without qdevification, you will not be able to add yours" -- that of 
course has always been the case.  But I never noticed because there was 
no way to avoid creating the default CD-ROM, and this in turn forced the 
non-qdev-clean creation of the VSCSI controller.

Paolo

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/24/2011 03:03 PM, Markus Armbruster wrote:
>> >  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.
>>
>> Really?  Hasn't that always been the case?
>
> What hasn't always been the case? :)
>
> 1) "the default VSCSI controller will not be created" -- no, this is
> new with scsi-cd: scsi-disk never was on the default_driver_table in
> vl.c, as you said in the commit message for af6bf13 (defaults: ide-cd,
> ide-hd and scsi-cd devices suppress default CD-ROM, 2011-05-18).  In
> fact, I believe you could add scsi-hd there too.

Aha.  The default CD-ROM also creates a default controller if
machine->use_scsi.  But as soon as you try -device scsi-cd, it
vanishes.  Hmm.

> 2) "without qdevification, you will not be able to add yours" -- that
> of course has always been the case.  But I never noticed because there
> was no way to avoid creating the default CD-ROM, and this in turn
> forced the non-qdev-clean creation of the VSCSI controller.

There's -nodefaults.

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[David Gibson]

On Tue, May 24, 2011 at 01:45:05PM +0200, Paolo Bonzini wrote:
> 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>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/spapr.c       |   15 +++++----------
>  hw/spapr_llan.c  |    7 +------
>  hw/spapr_vio.c   |    5 +++++
>  hw/spapr_vio.h   |   13 ++++---------
>  hw/spapr_vscsi.c |    8 +-------
>  hw/spapr_vty.c   |    8 +-------
>  6 files changed, 17 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 109b774..07b2165 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;
> @@ -360,15 +359,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]);

Yeah, I was passing these in in the hope of avoiding tying the VIO
code too strongly to the XICS.  That was probably a foolish goal,
since PAPR does specify the XICS.

>          }
>      }
>  
> -    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 +374,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 +384,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_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..be535d6 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>
> @@ -595,6 +596,7 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
>  {
>      VIOsPAPRDeviceInfo *info = (VIOsPAPRDeviceInfo *)qinfo;
>      VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
> +    VIOsPAPRBus *bus = DO_UPCAST(VIOsPAPRBus, bus, dev->qdev.parent_bus);
>      char *id;
>  
>      if (asprintf(&id, "%s@%x", info->dt_name, dev->reg) < 0) {
> @@ -602,6 +604,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
>      }
>  
>      dev->qdev.id = id;
> +    dev->vio_irq_num = bus->irq++;
> +    dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);

I'd prefer to see an spapr_allocate_irq() function, rather than open
coding this multiple times.

Since we're not trying to be PIC independent any more, I'm not sure
there's much point carrying both the irq number and the qirq around in
the device structure.  We could just to the xics_find_irq at the point
we need to issue the interrupt.  (I'd prefer to do it the other way
around, but there's no simple way to retrieve the irq number from the
qirq).

>      rtce_init(dev);
>  
> @@ -656,6 +660,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
>  
>      qbus = qbus_create(&spapr_vio_bus_info, dev, "spapr-vio");
>      bus = DO_UPCAST(VIOsPAPRBus, bus, qbus);
> +    bus->irq = 16;
>  
>      /* hcall-vio */
>      spapr_register_hypercall(H_VIO_SIGNAL, h_vio_signal);
> 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 fdad3d2..89f7ce2 100644
> --- a/hw/spapr_vscsi.c
> +++ b/hw/spapr_vscsi.c
> @@ -894,20 +894,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)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[Paolo Bonzini]

On 05/25/2011 12:12 AM, David Gibson wrote:
>> @@ -602,6 +604,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
>>       }
>>
>>       dev->qdev.id = id;
>> +    dev->vio_irq_num = bus->irq++;
>> +    dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
>
> I'd prefer to see an spapr_allocate_irq() function, rather than open
> coding this multiple times.

I don't understand.  This is the only call to xics_find_qirq, unlike 
before this patch.  It's not open coded multiple times.

I can surely add a spapr_allocate_irq() function that would keep the 
code independent of the particular interrupt controller.  Would you 
prefer something that gives back the virtual IRQ number as well:

     qemu_irq *spapr_allocate_irq(uint32_t *p_vio_irq_num)

or should I keep that in the VIOsPAPRBus, like this:

     qemu_irq *spapr_allocate_irq(uint32_t p_vio_irq_num)

?  Should I pass a sPAPREnvironment too, or should it just use the global?

Paolo

Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device

[David Gibson]

On Wed, May 25, 2011 at 09:29:26AM +0200, Paolo Bonzini wrote:
> On 05/25/2011 12:12 AM, David Gibson wrote:
> >>@@ -602,6 +604,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
> >>      }
> >>
> >>      dev->qdev.id = id;
> >>+    dev->vio_irq_num = bus->irq++;
> >>+    dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
> >
> >I'd prefer to see an spapr_allocate_irq() function, rather than open
> >coding this multiple times.
> 
> I don't understand.  This is the only call to xics_find_qirq, unlike
> before this patch.  It's not open coded multiple times.

Uh, sorry, I wasn't thinking it through when I assumed it was
duplicated.

Actually, in any case I wouldn't mind multiple calls to
xics_find_qirq(), it's the actual allocation - the irq++ - being
potentially duplicated which bothers me.  It's not now, but with this
approach it would need to be when we add non VIO devices to the system
which is coming (PCI, virtio, ..).

> I can surely add a spapr_allocate_irq() function that would keep the
> code independent of the particular interrupt controller.  Would you
> prefer something that gives back the virtual IRQ number as well:
> 
>     qemu_irq *spapr_allocate_irq(uint32_t *p_vio_irq_num)
> 
> or should I keep that in the VIOsPAPRBus, like this:
> 
>     qemu_irq *spapr_allocate_irq(uint32_t p_vio_irq_num)

In fact just returning the xics irq num and using xics_find_qirq() on
that would be ok by me.  The point is that I don't want the policy for
irq allocation on the global interrupt controller to be open coded
here at the bus level.

> ?  Should I pass a sPAPREnvironment too, or should it just use the global?

Passing it would be preferable.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[Qemu-devel] [PATCH 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>
Cc: Alexander Graf <agraf@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
---
 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 89f7ce2..bbe32f9 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -931,9 +931,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 3/3] spapr: make irq customizable via qdev

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
---
 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 be535d6..fee4c46 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)
@@ -604,7 +608,9 @@ static int spapr_vio_busdev_init(DeviceState *qdev, DeviceInfo *qinfo)
     }
 
     dev->qdev.id = id;
-    dev->vio_irq_num = bus->irq++;
+    if (!dev->vio_irq_num) {
+        dev->vio_irq_num = bus->irq++;
+    }
     dev->qirq = xics_find_qirq(spapr->icp, dev->vio_irq_num);
 
     rtce_init(dev);
-- 
1.7.4.4

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

[David Gibson]

On Tue, May 24, 2011 at 01:45:07PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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 be535d6..fee4c46 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(),
> +    },
>  };

I don't see what the point of this is.  Absolute irq numbers have no
special meaning in the XICS context, and the guest kernel will remap
them to virtual irqs anyway.


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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Paolo Bonzini]

On 05/25/2011 12:14 AM, David Gibson wrote:
> I don't see what the point of this is.  Absolute irq numbers have no
> special meaning in the XICS context, and the guest kernel will remap
> them to virtual irqs anyway.

It allows you to see the irq in "info qtree" for example.

Paolo

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

[David Gibson]

On Wed, May 25, 2011 at 09:30:22AM +0200, Paolo Bonzini wrote:
> On 05/25/2011 12:14 AM, David Gibson wrote:
> >I don't see what the point of this is.  Absolute irq numbers have no
> >special meaning in the XICS context, and the guest kernel will remap
> >them to virtual irqs anyway.
> 
> It allows you to see the irq in "info qtree" for example.

Hm, I see.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, May 24, 2011 at 01:45:07PM +0200, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  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 be535d6..fee4c46 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(),
>> +    },
>>  };
>
> I don't see what the point of this is.  Absolute irq numbers have no
> special meaning in the XICS context, and the guest kernel will remap
> them to virtual irqs anyway.

Are the irq numbers guest-visible?  If yes, a property may be required
to keep them stable across migration.  Especially when hot-plug comes
into play.

[...]

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

[David Gibson]

On Wed, May 25, 2011 at 05:13:40PM +0200, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Tue, May 24, 2011 at 01:45:07PM +0200, Paolo Bonzini wrote:
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  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 be535d6..fee4c46 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(),
> >> +    },
> >>  };
> >
> > I don't see what the point of this is.  Absolute irq numbers have no
> > special meaning in the XICS context, and the guest kernel will remap
> > them to virtual irqs anyway.
> 
> Are the irq numbers guest-visible?

Yes.

>  If yes, a property may be required
> to keep them stable across migration.  Especially when hot-plug comes
> into play.

Ah, yes, that's a point.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.

Looks good to spapr-ignorant me.