Ping for code review, please? thanks -- PMM
On Fri, 9 Oct 2020 at 16:39, Peter Maydell <[email protected]> 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 <[email protected]> > Signed-off-by: Peter Maydell <[email protected]> > --- > > 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 >
