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

["Alex Bennée" <alex.bennee@linaro.org>]

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

>      int grp;
> +    bool is_eoir0 = ri->crm == 8;
>  
> -    if (icv_access(env, ri->crm == 8 ? HCR_FMO : HCR_IMO)) {
> +    if (icv_access(env, is_eoir0 ? HCR_FMO : HCR_IMO)) {
>          icv_eoir_write(env, ri, value);
>          return;
>      }
>  
> -    trace_gicv3_icc_eoir_write(ri->crm == 8 ? 0 : 1,
> +    trace_gicv3_icc_eoir_write(is_eoir0 ? 0 : 1,
>                                 gicv3_redist_affid(cs), value);
>  
> -    if (ri->crm == 8) {
> -        /* EOIR0 */
> -        grp = GICV3_G0;
> -    } else {
> -        /* EOIR1 */
> -        if (arm_is_secure(env)) {
> -            grp = GICV3_G1;
> -        } else {
> -            grp = GICV3_G1NS;
> -        }
> -    }
> -
>      if (irq >= cs->gic->num_irq) {
>          /* This handles two cases:
>           * 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
> @@ -1340,8 +1329,35 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          return;
>      }
>  
> -    if (icc_highest_active_group(cs) != grp) {
> -        return;
> +    grp = icc_highest_active_group(cs);
> +    switch (grp) {
> +    case GICV3_G0:
> +        if (!is_eoir0) {
> +            return;
> +        }
> +        if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS)
> +            && arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
> +            return;
> +        }
> +        break;
> +    case GICV3_G1:
> +        if (is_eoir0) {
> +            return;
> +        }
> +        if (!arm_is_secure(env)) {
> +            return;
> +        }
> +        break;
> +    case GICV3_G1NS:
> +        if (is_eoir0) {
> +            return;
> +        }
> +        if (!arm_is_el3_or_mon(env) && arm_is_secure(env)) {
> +            return;
> +        }
> +        break;
> +    default:
> +        g_assert_not_reached();
>      }

From my reading of the spec it looks OK to me:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée