diff mbox

[01/58] spapr: proper qdevification

Message ID 1315989802-18753-2-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf Sept. 14, 2011, 8:42 a.m. UTC
From: Paolo Bonzini <pbonzini@redhat.com>

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.

NOTE from agraf: added small compile fix

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/spapr.c       |   15 +++++----------
 hw/spapr.h       |    8 ++++++++
 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 +-------
 7 files changed, 25 insertions(+), 39 deletions(-)

Comments

David Gibson Sept. 15, 2011, 3:14 a.m. UTC | #1
On Wed, Sep 14, 2011 at 10:42:25AM +0200, Alexander Graf wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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.
> 
> NOTE from agraf: added small compile fix

Thanks for fixing this, Paolo.  Since writing that code, I've realised
it doesn't really fit the model correctly, but haven't gotten around
to fixing it yet.

I will make a later patch to move the irq allocation from the vio bus
to the xics itself, which will matter once we add PCI and/or other
busses.


A question about qdev stuff.  Under PAPR, there is generally only
supposed to be one SCSI target (disk / cd / whatever) per virtual scsi
bus.  But the generic qdev code will, by default, keep assigning
devices to the existing bus until it's full.  Any thoughts on how to
sanely change that behaviour on a per-machine basis?

We'll have a similar problem later on with PCI - PAPR machines usually
only have one device per host bridge, for better isolation.
Paolo Bonzini Sept. 15, 2011, 7:01 a.m. UTC | #2
On 09/15/2011 05:14 AM, David Gibson wrote:
> Under PAPR, there is generally only
> supposed to be one SCSI target (disk / cd / whatever) per virtual scsi
> bus.  But the generic qdev code will, by default, keep assigning
> devices to the existing bus until it's full.  Any thoughts on how to
> sanely change that behaviour on a per-machine basis?

You could change the if_max_devs array in blockdev.c to something 
provided by the machines.

However, I'm not sure about this, for two reasons:

1) do you mean, in Linux terms, one target per SCSI _host_ or one target 
per SCSI _channel_?  i.e. if you looks at /sys/bus/scsi/devices, right 
now it looks like

    0:0:0:0    0:0:1:0     (two targets on the same host and channel)

Should it be?

    0:0:0:0    0:1:0:0     (one target per channel)

or

    0:0:0:0    1:0:0:0     (one target per host)

If it is the former, then you are simply hitting a limitation of the 
SCSI layer in QEMU and I do have patches to make assignment more 
flexible.  Based on the Linux VSCSI driver, and based on what SLOF does, 
I'd guess that's what you mean.

2) does this matter at all?  First, when doing "real world" 
virtualization you won't use the legacy options (neither -hda/-cdrom nor 
"-drive ...,if=scsi"), you would use -device to manually assign the 
devices to their buses.  Second, why should you care in the case of 
SCSI?  It seems like a very hard limitation to me, and unlike the PCI 
case it doesn't buy you anything in terms of isolation.

Paolo
David Gibson Sept. 16, 2011, 3:06 a.m. UTC | #3
On Thu, Sep 15, 2011 at 09:01:38AM +0200, Paolo Bonzini wrote:
> On 09/15/2011 05:14 AM, David Gibson wrote:
> >Under PAPR, there is generally only
> >supposed to be one SCSI target (disk / cd / whatever) per virtual scsi
> >bus.  But the generic qdev code will, by default, keep assigning
> >devices to the existing bus until it's full.  Any thoughts on how to
> >sanely change that behaviour on a per-machine basis?
> 
> You could change the if_max_devs array in blockdev.c to something
> provided by the machines.
> 
> However, I'm not sure about this, for two reasons:
> 
> 1) do you mean, in Linux terms, one target per SCSI _host_ or one
> target per SCSI _channel_?  i.e. if you looks at
> /sys/bus/scsi/devices, right now it looks like
> 
>    0:0:0:0    0:0:1:0     (two targets on the same host and channel)
> 
> Should it be?
> 
>    0:0:0:0    0:1:0:0     (one target per channel)
> 
> or
> 
>    0:0:0:0    1:0:0:0     (one target per host)
> 
> If it is the former, then you are simply hitting a limitation of the
> SCSI layer in QEMU and I do have patches to make assignment more
> flexible.  Based on the Linux VSCSI driver, and based on what SLOF
> does, I'd guess that's what you mean.


Well, now I'm confused.  I had a look at a pHyp machine, and Linux
seemed to see it as multiple targets on a single channel, but I'm sure
the PAPR spec says you shouldn't have that.  So I'm going to have to
look closer now.

> 2) does this matter at all?  First, when doing "real world"
> virtualization you won't use the legacy options (neither -hda/-cdrom
> nor "-drive ...,if=scsi"), you would use -device to manually assign
> the devices to their buses.

Well, perhaps, but I really prefer to have sane defaults, rather than
having to build the machine myself on the command line.

>  Second, why should you care in the case
> of SCSI?  It seems like a very hard limitation to me, and unlike the
> PCI case it doesn't buy you anything in terms of isolation.

Ah, there is a good reason on this side.  I forget the exact details,
but due to the protocol it uses there's some blocksize limit that is
only advertised per vscsi adaptor, whereas it should really be a
per-target quantity (and is different in practice for cdroms and
disks).  Of course that's arguably a bug in the vscsi protocol, but we
can't fix that.
Paolo Bonzini Sept. 16, 2011, 10:41 a.m. UTC | #4
On 09/16/2011 05:06 AM, David Gibson wrote:
>> >
>> >  1) do you mean, in Linux terms, one target per SCSI _host_ or one
>> >  target per SCSI _channel_?  i.e. if you looks at
>> >  /sys/bus/scsi/devices, right now it looks like
>> >
>> >      0:0:0:0    0:0:1:0     (two targets on the same host and channel)
>> >
>> >  Should it be?
>> >
>> >      0:0:0:0    0:1:0:0     (one target per channel)
>> >
>> >  or
>> >
>> >      0:0:0:0    1:0:0:0     (one target per host)
>> >
>> >  If it is the former, then you are simply hitting a limitation of the
>> >  SCSI layer in QEMU and I do have patches to make assignment more
>> >  flexible.  Based on the Linux VSCSI driver, and based on what SLOF
>> >  does, I'd guess that's what you mean.
>
> Well, now I'm confused.  I had a look at a pHyp machine, and Linux
> seemed to see it as multiple targets on a single channel, but I'm sure
> the PAPR spec says you shouldn't have that.  So I'm going to have to
> look closer now.

If this is the case, there might be a bug in SLOF's probing of SCSI devices.

SLOF probes target 0/LUN 0 on eight channels, i.e. from 0:0:0 to 7:0:0. 
  Linux however shows them the same as pHyp, i.e. from 0:0:0 to 0:7:0.

The reason this works is because LUN parsing in QEMU is completely 
broken (by Ben's admission in spapr_vscsi.c :)) and so SLOF's x:0:0 and 
Linux's 0:x:0 end up referring to the same device.

Now, when implementing SCSI addressing I had two choices:

(1) leave them where Linux sees them.  This seems correct according to 
your experiments with pHyp, but then SLOF could only see 0:0:0;

(2) move the devices so that both SLOF and Linux see them as x:0:0 (one 
target per channel).  This would be inconsistent with pHyp, but it 
doesn't break either SLOF or Linux.

So, I would like to agree on a plan for merging the SCSI addressing 
series.  Right now I am doing (2), because it lets me use the current 
version of SLOF.  Is it okay for you to merge the feature with these 
semantics?

If you want to change to (1), that can be done easily.  However, it 
requires fixing SLOF, so it would have to go preferably through Alex's 
PPC tree.

(Again, that would be just the defaults---the addressing can always be 
overridden by using -device explicitly).

Paolo
Thomas Huth Sept. 16, 2011, 1:27 p.m. UTC | #5
Hi all!

Am Fri, 16 Sep 2011 12:41:40 +0200
schrieb Paolo Bonzini <pbonzini@redhat.com>:

> On 09/16/2011 05:06 AM, David Gibson wrote:
> >> >
> >> >  1) do you mean, in Linux terms, one target per SCSI _host_ or one
> >> >  target per SCSI _channel_?  i.e. if you looks at
> >> >  /sys/bus/scsi/devices, right now it looks like
> >> >
> >> >      0:0:0:0    0:0:1:0     (two targets on the same host and channel)
> >> >
> >> >  Should it be?
> >> >
> >> >      0:0:0:0    0:1:0:0     (one target per channel)
> >> >
> >> >  or
> >> >
> >> >      0:0:0:0    1:0:0:0     (one target per host)
> >> >
> >> >  If it is the former, then you are simply hitting a limitation of the
> >> >  SCSI layer in QEMU and I do have patches to make assignment more
> >> >  flexible.  Based on the Linux VSCSI driver, and based on what SLOF
> >> >  does, I'd guess that's what you mean.
> >
> > Well, now I'm confused.  I had a look at a pHyp machine, and Linux
> > seemed to see it as multiple targets on a single channel, but I'm sure
> > the PAPR spec says you shouldn't have that.  So I'm going to have to
> > look closer now.
> 
> If this is the case, there might be a bug in SLOF's probing of SCSI devices.
> 
> SLOF probes target 0/LUN 0 on eight channels, i.e. from 0:0:0 to 7:0:0. 
>   Linux however shows them the same as pHyp, i.e. from 0:0:0 to 0:7:0.
> 
> The reason this works is because LUN parsing in QEMU is completely 
> broken (by Ben's admission in spapr_vscsi.c :)) and so SLOF's x:0:0 and 
> Linux's 0:x:0 end up referring to the same device.


I've done some readings about this problem today, and I think I've got
an idea what might be wrong here - seems like a bug in SLOF to me.

First, according to the SLOF source code, it seems to me that its
intention is to to scan target IDs, not channels (but as I haven't
written that part, I am not 100% sure here).

Then I compared how Linux and SLOF fill the 64-bit LUN field in the
SRP_CMD request structure, and they both fill in the target ID at the
same location - but Linux is additionally setting an additional bit in
first byte (see the "lun_from_dev" function in ibmvscsi.c of the
kernel).

Looking at the "SCSI Architecture Model" specification, this additional
bit is used to select the "Logical unit addressing method" instead of
the "Peripheral device addressing method" that SLOF currently uses - and
the "Logical unit addressing method" sounds more reasonable to me when
looking at the places where SLOF tries to fill in the target ID.

So I suggest that I change SLOF to also use the "Logical unit
addressing method" like Linux does, which should result in the fact that
SLOF tries to scan the target IDs instead of the channels/bus IDs.

What do you think, does that sound ok?

 Regards,
  Thomas
Paolo Bonzini Sept. 16, 2011, 1:28 p.m. UTC | #6
On 09/16/2011 03:27 PM, Thomas Huth wrote:
> So I suggest that I change SLOF to also use the "Logical unit
> addressing method" like Linux does, which should result in the fact that
> SLOF tries to scan the target IDs instead of the channels/bus IDs.
>
> What do you think, does that sound ok?

Yes, that's perfect.

Paolo
David Gibson Sept. 16, 2011, 2:08 p.m. UTC | #7
On Fri, Sep 16, 2011 at 12:41:40PM +0200, Paolo Bonzini wrote:
> On 09/16/2011 05:06 AM, David Gibson wrote:
> >>>
> >>>  1) do you mean, in Linux terms, one target per SCSI _host_ or one
> >>>  target per SCSI _channel_?  i.e. if you looks at
> >>>  /sys/bus/scsi/devices, right now it looks like
> >>>
> >>>      0:0:0:0    0:0:1:0     (two targets on the same host and channel)
> >>>
> >>>  Should it be?
> >>>
> >>>      0:0:0:0    0:1:0:0     (one target per channel)
> >>>
> >>>  or
> >>>
> >>>      0:0:0:0    1:0:0:0     (one target per host)
> >>>
> >>>  If it is the former, then you are simply hitting a limitation of the
> >>>  SCSI layer in QEMU and I do have patches to make assignment more
> >>>  flexible.  Based on the Linux VSCSI driver, and based on what SLOF
> >>>  does, I'd guess that's what you mean.
> >
> >Well, now I'm confused.  I had a look at a pHyp machine, and Linux
> >seemed to see it as multiple targets on a single channel, but I'm sure
> >the PAPR spec says you shouldn't have that.  So I'm going to have to
> >look closer now.
> 
> If this is the case, there might be a bug in SLOF's probing of SCSI
> devices.

Um.. I'm confused.  This is a pHyp (aka PowerVM) machine, so there is
no SLOF.  What I'm seeing there seems to contradict the PAPR spec
which is supposed to describe it.  So I don't see how it has a bearing
on SLOF addressing.

> SLOF probes target 0/LUN 0 on eight channels, i.e. from 0:0:0 to
> 7:0:0.  Linux however shows them the same as pHyp, i.e. from 0:0:0
> to 0:7:0.
> 
> The reason this works is because LUN parsing in QEMU is completely
> broken (by Ben's admission in spapr_vscsi.c :)) and so SLOF's x:0:0
> and Linux's 0:x:0 end up referring to the same device.
> 
> Now, when implementing SCSI addressing I had two choices:
> 
> (1) leave them where Linux sees them.  This seems correct according
> to your experiments with pHyp, but then SLOF could only see 0:0:0;
> 
> (2) move the devices so that both SLOF and Linux see them as x:0:0
> (one target per channel).  This would be inconsistent with pHyp, but
> it doesn't break either SLOF or Linux.
> 
> So, I would like to agree on a plan for merging the SCSI addressing
> series.  Right now I am doing (2), because it lets me use the
> current version of SLOF.  Is it okay for you to merge the feature
> with these semantics?

(2) sounds like what PAPR describes to me, so that sounds fine to me.
But I still don't follow your reasoning leading up to that.

> If you want to change to (1), that can be done easily.  However, it
> requires fixing SLOF, so it would have to go preferably through
> Alex's PPC tree.
> 
> (Again, that would be just the defaults---the addressing can always
> be overridden by using -device explicitly).
> 
> Paolo
>
Benjamin Herrenschmidt Sept. 16, 2011, 3:51 p.m. UTC | #8
> I've done some readings about this problem today, and I think I've got
> an idea what might be wrong here - seems like a bug in SLOF to me.
> 
> First, according to the SLOF source code, it seems to me that its
> intention is to to scan target IDs, not channels (but as I haven't
> written that part, I am not 100% sure here).
> 
> Then I compared how Linux and SLOF fill the 64-bit LUN field in the
> SRP_CMD request structure, and they both fill in the target ID at the
> same location - but Linux is additionally setting an additional bit in
> first byte (see the "lun_from_dev" function in ibmvscsi.c of the
> kernel).
> 
> Looking at the "SCSI Architecture Model" specification, this additional
> bit is used to select the "Logical unit addressing method" instead of
> the "Peripheral device addressing method" that SLOF currently uses - and
> the "Logical unit addressing method" sounds more reasonable to me when
> looking at the places where SLOF tries to fill in the target ID.
> 
> So I suggest that I change SLOF to also use the "Logical unit
> addressing method" like Linux does, which should result in the fact that
> SLOF tries to scan the target IDs instead of the channels/bus IDs.

 .../...

Note that in addition to that, the PAPR spec specifies only one
"device" (whatever that means) per vscsi instance.

In fact, if we are ever to support proper sg, we need that, because the
way our vscsi client driver works in linux, we can only pass one max
request size down to the client from qemu, and without that information,
sg cannot be done reliably.

Cheers,
Ben.
Paolo Bonzini Sept. 19, 2011, 6:50 a.m. UTC | #9
On 09/16/2011 04:08 PM, David Gibson wrote:
> > > Well, now I'm confused.  I had a look at a pHyp machine, and Linux
> > > seemed to see it as multiple targets on a single channel, but I'm sure
> > > the PAPR spec says you shouldn't have that.  So I'm going to have to
> > > look closer now.
> >
> >  If this is the case, there might be a bug in SLOF's probing of SCSI
> >  devices.
>
> Um.. I'm confused.  This is a pHyp (aka PowerVM) machine, so there is
> no SLOF.  What I'm seeing there seems to contradict the PAPR spec
> which is supposed to describe it.  So I don't see how it has a bearing
> on SLOF addressing.

I meant "if we want to make QEMU present devices like pHyp, we cannot do 
that without fixing SLOF".

> >  (2) move the devices so that both SLOF and Linux see them as x:0:0
> >  (one target per channel).  This would be inconsistent with pHyp, but
> >  it doesn't break either SLOF or Linux.
> >
>
> (2) sounds like what PAPR describes to me, so that sounds fine to me.

No, PAPR describes one target per *host*, not channel.

Paolo
Thomas Huth Sept. 19, 2011, 6:55 a.m. UTC | #10
Am Fri, 16 Sep 2011 12:51:31 -0300
schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> 
> > I've done some readings about this problem today, and I think I've got
> > an idea what might be wrong here - seems like a bug in SLOF to me.
> > 
> > First, according to the SLOF source code, it seems to me that its
> > intention is to to scan target IDs, not channels (but as I haven't
> > written that part, I am not 100% sure here).
> > 
> > Then I compared how Linux and SLOF fill the 64-bit LUN field in the
> > SRP_CMD request structure, and they both fill in the target ID at the
> > same location - but Linux is additionally setting an additional bit in
> > first byte (see the "lun_from_dev" function in ibmvscsi.c of the
> > kernel).
> > 
> > Looking at the "SCSI Architecture Model" specification, this additional
> > bit is used to select the "Logical unit addressing method" instead of
> > the "Peripheral device addressing method" that SLOF currently uses - and
> > the "Logical unit addressing method" sounds more reasonable to me when
> > looking at the places where SLOF tries to fill in the target ID.
> > 
> > So I suggest that I change SLOF to also use the "Logical unit
> > addressing method" like Linux does, which should result in the fact that
> > SLOF tries to scan the target IDs instead of the channels/bus IDs.
> 
>  .../...
> 
> Note that in addition to that, the PAPR spec specifies only one
> "device" (whatever that means) per vscsi instance.

Really? In that case, I wonder why Linux is using the "Logical unit
addressing format" with target IDs and bus numbers instead of the
"Flat space addressing method" for vscsi ... according to
drivers/scsi/ibmvscsi/ibmvscsi.c :

static inline u16 lun_from_dev(struct scsi_device *dev)
{
	return (0x2 << 14) | (dev->id << 8) | (dev->channel << 5) | dev->lun;
}

In case there's really only one device per vscsi instance, shouldn't
that code use addressing method 0x1 instead of 0x2 here?

 Thomas
Paolo Bonzini Sept. 19, 2011, 6:59 a.m. UTC | #11
On 09/19/2011 08:55 AM, Thomas Huth wrote:
>> >  Note that in addition to that, the PAPR spec specifies only one
>> >  "device" (whatever that means) per vscsi instance.
> Really? In that case, I wonder why Linux is using the "Logical unit
> addressing format" with target IDs and bus numbers instead of the
> "Flat space addressing method" for vscsi ... according to
> drivers/scsi/ibmvscsi/ibmvscsi.c :
>
> static inline u16 lun_from_dev(struct scsi_device *dev)
> {
> 	return (0x2<<  14) | (dev->id<<  8) | (dev->channel<<  5) | dev->lun;
> }
>
> In case there's really only one device per vscsi instance, shouldn't
> that code use addressing method 0x1 instead of 0x2 here?

As long as dev->id == 0, dev->channel == 0, dev->lun < 31, the three 
addressing methods are all equivalent.

Some comments in ibmvscsi.c say that iOS needs non-zero channels, so 
there does seem to be someone else who doesn't follow the PAPR spec too 
well. :)

Paolo
diff mbox

Patch

diff --git a/hw/spapr.c b/hw/spapr.c
index 1265cee..8cf93fe 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 = g_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.h b/hw/spapr.h
index 263691b..009c459 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -1,6 +1,8 @@ 
 #if !defined(__HW_SPAPR_H__)
 #define __HW_SPAPR_H__
 
+#include "hw/xics.h"
+
 struct VIOsPAPRBus;
 struct icp_state;
 
@@ -278,6 +280,12 @@  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 qemu_irq spapr_find_qirq(sPAPREnvironment *spapr,
+                                        int irq_num)
+{
+    return xics_find_qirq(spapr->icp, irq_num);
+}
+
 static inline uint32_t rtas_ld(target_ulong phys, int n)
 {
     return ldl_be_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 ce6558b..ba2e1c1 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 = spapr_find_qirq(spapr, 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 fc9ac6a..d2d0415 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -893,20 +893,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 f5046d9..607b81b 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)