Re: [RFC PATCH] target/arm: clamp value to account for RES0 fields

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

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 16 Jun 2025 at 21:10, Alex Bennée <alex.bennee@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.bennee@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?)

Heh, it was writing the SP instead of the XR, this is how I found out I
was getting it wrong ;-)

>
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro