On 5 September 2016 at 15:09, Alex Bennée <[email protected]> wrote: > I noticed while testing with modern kernels and -d guest_errors warnings > about invalid writes to the GIC. For GICv2 these registers certainly > should work so I've implemented both. As the code is common between all > the various GICs writes to GICD_ISACTIVERn is checked to ensure it is > not a RO register for v1 GICs.
This is definitely a bug, and also the right way to fix it, so you don't need to mark your patch 'RFC' :-) Some minor review issues below. > Signed-off-by: Alex Bennée <[email protected]> > --- > hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index b30cc91..423a4ae 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -972,9 +972,38 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, > GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); > } > } > + } else if (offset < 0x380) { > + /* Interrupt Set-Active */ > + irq = (offset - 0x300) * 8 + GIC_BASE_IRQ; > + if (irq >= s->num_irq || s->revision < 2) Better to check "s->revision != 2" -- we still have the NVIC code tangled up with the GIC, and on the NVIC these are R/O. > + goto bad_reg; > + > + for (i = 0; i < 8; i++) { > + if (s->security_extn && !attrs.secure && > + !GIC_TEST_GROUP(irq + i, 1 << cpu)) { > + continue; /* Ignore Non-secure access of Group0 IRQ */ > + } > + > + if (value & (1 << i)) { > + GIC_SET_ACTIVE(irq + i, 1 << cpu); The mask parameter to GIC_SET_ACTIVE/GIC_CLEAR_ACTIVE should be calculated like int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK; -- compare the set-enable, clear-enable, etc write code. I'm fairly sure that just setting the active bit (ie not also trying to update the active-priority registers) is the correct behaviour here, though the GIC spec is not clear to me on this point. > + } > + } > } else if (offset < 0x400) { > - /* Interrupt Active. */ > - goto bad_reg; > + /* Interrupt Clear-Active */ > + irq = (offset - 0x380) * 8 + GIC_BASE_IRQ; > + if (irq >= s->num_irq) > + goto bad_reg; This is missing the check against s->revision. > + > + for (i = 0; i < 8; i++) { > + if (s->security_extn && !attrs.secure && > + !GIC_TEST_GROUP(irq + i, 1 << cpu)) { > + continue; /* Ignore Non-secure access of Group0 IRQ */ > + } > + > + if (value & (1 << i)) { > + GIC_CLEAR_ACTIVE(irq + i, 1 << cpu); > + } > + } > } else if (offset < 0x800) { > /* Interrupt Priority. */ > irq = (offset - 0x400) + GIC_BASE_IRQ; > -- > 2.9.3 It looks like we don't implement reads of clear-active correctly either. thanks -- PMM
