Re: [PATCH] hw/intc/arm_gicv3: ICC_PMR_EL1 high bits should be RAZ

[Peter Maydell]

On Tue, 14 Nov 2023 at 16:54, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>
> The ICC_PMR_ELx bit msak returned from icc_fullprio_mask
> should technically also remove any bit above 7 as these
> are marked reserved (read 0) and should therefore should
> not be written as anything other than 0.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index d07b13eb27..986044df79 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -803,7 +803,7 @@ static uint32_t icc_fullprio_mask(GICv3CPUState *cs)
>       * with the group priority, whose mask depends on the value of BPR
>       * for the interrupt group.)
>       */
> -    return ~0U << (8 - cs->pribits);
> +    return (~0U << (8 - cs->pribits)) & 0xff;
>  }

The upper bits of ICC_PMR_ELx are defined as RES0, which has a
complicated technical definition which you can find in the GIC
architecture specification glossary. It's valid for RES0 bits to
be implemented as reads-as-written, which is the way our current
implementation works. Valid guest code should never be writing
any non-zero value into those bits.

What problem are you running into that you're trying to fix
with this patch? If our implementation misbehaves as a result
of letting these high bits through into cs->icc_pmr_el1 that
would be a good reason for making the change.

If we do want to change this, for consistency we'd want
to change icv_fullprio_mask() too.

thanks
-- PMM