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

[David Gibson <david@gibson.dropbear.id.au>]

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