Re: [Qemu-ppc] [PATCH 3/7] pseries: Add support for level interrupts to XICS

[Alexander Graf]

On 28.02.2012, at 04:18, David Gibson wrote:

> The pseries "xics" interrupt controller, like most interrupt
> controllers can support both message (i.e. edge sensitive) interrupts
> and level sensitive interrupts, but it needs to know which are which.
> 
> When I implemented the xics emulation for qemu, the only devices we
> supported were the PAPR virtual IO devices.  These devices only use
> message interrupts, so they were the only ones I implemented in xics.
> 
> Since then, however, we have added support for PCI devices, which use
> level sensitive interrupts.  It turns out the message interrupt logic
> still actually works most of the time for these, but there are
> circumstances where we can lost interrupts due to the incorrect
> interrupt logic.
> 
> This patch, therefore, implements the correct xics level-sensitive
> interrupt logic.  The type of the interrupt is set when a device
> allocates a new xics interrupt.
> 
> Signed-off-by: David Gibson <address@hidden@.dropbear.id.au>

This looks wrong

> ---
> hw/spapr.c     |    4 +-
> hw/spapr.h     |   12 +++++-
> hw/spapr_pci.c |    2 +-
> hw/spapr_vio.c |    2 +-
> hw/xics.c      |  122 +++++++++++++++++++++++++++++++++++++++++---------------
> hw/xics.h      |    2 +-
> 6 files changed, 106 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index dffb6a2..828bc53 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -83,7 +83,7 @@
> 
> sPAPREnvironment *spapr;
> 
> -qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num)
> +qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num, int lsi)
> {
>     uint32_t irq;
>     qemu_irq qirq;
> @@ -95,7 +95,7 @@ qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t 
> *irq_num)
>         irq = spapr->next_irq++;
>     }
> 
> -    qirq = xics_find_qirq(spapr->icp, irq);
> +    qirq = xics_assign_irq(spapr->icp, irq, lsi);

Find becomes assign? Is this on purpose?

While at it, could you make the "lsi" thing an enum? That'd make things easier 
to track :).


Alex