On Mon, 24 May 2021 at 17:09, Alex Bennée <alex.ben...@linaro.org> wrote: > > > Peter Maydell <peter.mayd...@linaro.org> writes: > > > In icc_eoir_write() we assume that we can identify the group of the > > IRQ being completed based purely on which register is being written > > to and the current CPU state, and that "CPU state matches group > > indicated by register" is the only necessary access check. > > > > This isn't correct: if the CPU is not in Secure state then EOIR1 will > > only complete Group 1 NS IRQs, but if the CPU is in EL3 it can > > complete both Group 1 S and Group 1 NS IRQs. (The pseudocode > > ICC_EOIR1_EL1 makes this clear.) We were also missing the logic to > > prevent EOIR0 writes completing G0 IRQs when they should not. > > > > Rearrange the logic to first identify the group of the current > > highest priority interrupt and then look at whether we should > > complete it or ignore the access based on which register was accessed > > and the state of the CPU. The resulting behavioural change is: > > * EL3 can now complete G1NS interrupts > > * G0 interrupt completion is now ignored if the GIC > > and the CPU have the security extension enabled and > > the CPU is not secure > > > > Reported-by: Chan Kim <c...@etri.re.kr> > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > > --- > > hw/intc/arm_gicv3_cpuif.c | 48 ++++++++++++++++++++++++++------------- > > 1 file changed, 32 insertions(+), 16 deletions(-) > > > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > > index 43ef1d7a840..81f94c7f4ad 100644 > > --- a/hw/intc/arm_gicv3_cpuif.c > > +++ b/hw/intc/arm_gicv3_cpuif.c > > @@ -1307,27 +1307,16 @@ static void icc_eoir_write(CPUARMState *env, const > > ARMCPRegInfo *ri, > > GICv3CPUState *cs = icc_cs_from_env(env); > > int irq = value & 0xffffff; > > Un-related but the docs do say that: > > This field has either 16 or 24 bits implemented. The number of > implemented bits can be found in ICC_CTLR_EL1.IDbits and > ICC_CTLR_EL3.IDbits. If only 16 bits are implemented, bits [23:16] of > this register are RES0
Yeah, this is one of those implementation-choice areas: you can implement either 16 or 24 bits. QEMU chooses 24 bits and correspondingly we set the ICC_CTLR_ELx.IDbits field to 1 in icc_reset(). > From my reading of the spec it looks OK to me: > > Reviewed-by: Alex Bennée <alex.ben...@linaro.org> thanks -- PMM
