Re: [PATCH] hw/intc/arm_gicv3_cpuif: Make GIC maintenance interrupts work

[Luc Michel]

On 16:39 Fri 09 Oct     , Peter Maydell wrote:
> In gicv3_init_cpuif() we copy the ARMCPU gicv3_maintenance_interrupt
> into the GICv3CPUState struct's maintenance_irq field.  This will
> only work if the board happens to have already wired up the CPU
> maintenance IRQ before the GIC was realized.  Unfortunately this is
> not the case for the 'virt' board, and so the value that gets copied
> is NULL (since a qemu_irq is really a pointer to an IRQState struct
> under the hood).  The effect is that the CPU interface code never
> actually raises the maintenance interrupt line.
> 
> Instead, since the GICv3CPUState has a pointer to the CPUState, make
> the dereference at the point where we want to raise the interrupt, to
> avoid an implicit requirement on board code to wire things up in a
> particular order.
> 
> Reported-by: Jose Martins <josemartins90@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> 
> QEMU's implementation here is a bit odd because we've put all the
> logic into the "GIC" device where in real hardware it's split between
> a GIC device and the CPU interface part in the CPU.  If we had
> arranged it in that way then we wouldn't have this odd bit of code
> where the GIC device needs to raise an IRQ line that belongs to the
> CPU.
> 
> Not sure why we've never noticed this bug previously with KVM as a
> guest, you'd think we'd have spotted "maintenance interrupts just
> don't work"...
> ---
>  include/hw/intc/arm_gicv3_common.h | 1 -
>  hw/intc/arm_gicv3_cpuif.c          | 5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/intc/arm_gicv3_common.h 
> b/include/hw/intc/arm_gicv3_common.h
> index 0331b0ffdb8..91491a2f664 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -153,7 +153,6 @@ struct GICv3CPUState {
>      qemu_irq parent_fiq;
>      qemu_irq parent_virq;
>      qemu_irq parent_vfiq;
> -    qemu_irq maintenance_irq;
>  
>      /* Redistributor */
>      uint32_t level;                  /* Current IRQ level */
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 08e000e33c6..43ef1d7a840 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -399,6 +399,7 @@ static void gicv3_cpuif_virt_update(GICv3CPUState *cs)
>      int irqlevel = 0;
>      int fiqlevel = 0;
>      int maintlevel = 0;
> +    ARMCPU *cpu = ARM_CPU(cs->cpu);
>  
>      idx = hppvi_index(cs);
>      trace_gicv3_cpuif_virt_update(gicv3_redist_affid(cs), idx);
> @@ -424,7 +425,7 @@ static void gicv3_cpuif_virt_update(GICv3CPUState *cs)
>  
>      qemu_set_irq(cs->parent_vfiq, fiqlevel);
>      qemu_set_irq(cs->parent_virq, irqlevel);
> -    qemu_set_irq(cs->maintenance_irq, maintlevel);
> +    qemu_set_irq(cpu->gicv3_maintenance_interrupt, maintlevel);
>  }
>  
>  static uint64_t icv_ap_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -2624,8 +2625,6 @@ void gicv3_init_cpuif(GICv3State *s)
>              && cpu->gic_num_lrs) {
>              int j;
>  
> -            cs->maintenance_irq = cpu->gicv3_maintenance_interrupt;
> -
>              cs->num_list_regs = cpu->gic_num_lrs;
>              cs->vpribits = cpu->gic_vpribits;
>              cs->vprebits = cpu->gic_vprebits;
> -- 
> 2.20.1
> 
> 

--