On Mon, 16 Jun 2025 at 21:10, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> If the user writes a large value to the register but with the bottom
> bits unset we could end up with something illegal. By clamping ahead
> of the check we at least assure we won't assert(bpr > 0) later in the
> GIC interface code.
>
> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 4b4cf09157..165f7e9c2f 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -1797,6 +1797,9 @@ static void icc_bpr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> trace_gicv3_icc_bpr_write(ri->crm == 8 ? 0 : 1,
> gicv3_redist_affid(cs), value);
>
> + /* clamp the value to 2:0, the rest os RES0 */
> + value = deposit64(0, 0, 3, value);
Should be extract64(), as RTH notes (or just &= 7 if you like).
> +
> if (grp == GICV3_G1 && gicv3_use_ns_bank(env)) {
> grp = GICV3_G1NS;
> }
> @@ -1820,7 +1823,7 @@ static void icc_bpr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> value = minval;
> }
>
> - cs->icc_bpr[grp] = value & 7;
> + cs->icc_bpr[grp] = value;
> gicv3_cpuif_update(cs);
Yes, I agree we should do the "work only on the 3 bit field"
part before we do the "enforce the minimum value" logic.
The handling in icv_bpr_write() has a similar issue.
(Why was your guest writing garbage to this register?)
-- PMM
