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

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

[Ben Dooks]

The ICC_PMR_ELx and ICV_PMR_ELx bit masks returned from
ic{c,v}_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.

This was noted during a run of a proprietary test system and
discused on the mailing list [1] and initially thought not to
be an issue due to RES0 being technically allowed to be
written to and read back as long as the implementation does
not use the RES0 bits. It is very possible that the values
are used in comparison without masking, as pointed out by
Peter in [2], if (cs->hppi.prio >= cs->icc_pmr_el1) may well
do the wrong thing.

Masking these values in ic{c,v}_fullprio_mask() should fix
this and prevent any future problems with playing with the
values.

[1]: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00607.html
[2]: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00737.html

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
v2:
 - fixes as suggested by Peter Maydell to include icv_fullprio_mask()
---
 hw/intc/arm_gicv3_cpuif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index d07b13eb27..ab1a00508e 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -146,7 +146,7 @@ static uint32_t icv_fullprio_mask(GICv3CPUState *cs)
      * with the group priority, whose mask depends on the value of VBPR
      * for the interrupt group.)
      */
-    return ~0U << (8 - cs->vpribits);
+    return (~0U << (8 - cs->vpribits)) & 0xff;
 }
 
 static int ich_highest_active_virt_prio(GICv3CPUState *cs)
@@ -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;
 }
 
 static inline int icc_min_bpr(GICv3CPUState *cs)
-- 
2.37.2.352.g3c44437643

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

[Peter Maydell]

On Thu, 16 Nov 2023 at 17:28, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>
> The ICC_PMR_ELx and ICV_PMR_ELx bit masks returned from
> ic{c,v}_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.
>
> This was noted during a run of a proprietary test system and
> discused on the mailing list [1] and initially thought not to
> be an issue due to RES0 being technically allowed to be
> written to and read back as long as the implementation does
> not use the RES0 bits. It is very possible that the values
> are used in comparison without masking, as pointed out by
> Peter in [2], if (cs->hppi.prio >= cs->icc_pmr_el1) may well
> do the wrong thing.
>
> Masking these values in ic{c,v}_fullprio_mask() should fix
> this and prevent any future problems with playing with the
> values.
>
> [1]: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00607.html
> [2]: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00737.html
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>



Applied to target-arm.next, thanks.

-- PMM