Re: [PATCH] hw/intc/arm_gicv3_cpuif: Fix EOIR write access check logic

[Peter Maydell]

On Mon, 24 May 2021 at 17:09, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@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 <ckim@etri.re.kr>
> > Signed-off-by: Peter Maydell <peter.maydell@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.bennee@linaro.org>

thanks
-- PMM