From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53133) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QP0Ty-0004sK-5d for qemu-devel@nongnu.org; Tue, 24 May 2011 18:53:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QP0Tv-0004Ta-UA for qemu-devel@nongnu.org; Tue, 24 May 2011 18:53:46 -0400 Received: from ozlabs.org ([203.10.76.45]:41711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QP0Tv-0004TS-99 for qemu-devel@nongnu.org; Tue, 24 May 2011 18:53:43 -0400 Date: Wed, 25 May 2011 08:12:22 +1000 From: David Gibson Message-ID: <20110524221222.GA11809@yookeroo.fritz.box> References: <1306237507-19189-1-git-send-email-pbonzini@redhat.com> <1306237507-19189-2-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1306237507-19189-2-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/3] spapr: allow creating devices with -device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, agraf@suse.de 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 > Cc: Alexander Graf > Cc: David Gibson > --- > 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 > @@ -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