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 <address@hidden>
> Cc: Alexander Graf <address@hidden>
> Cc: David Gibson <address@hidden>
> ---
>  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, "address@hidden", 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