Re: [PATCH] target/arm: Do hflags rebuild in cpsr_write()

[Richard Henderson <richard.henderson@linaro.org>]

On 8/17/21 10:26 AM, Peter Maydell wrote:
> On Tue, 17 Aug 2021 at 21:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> Currently we rely on all the callsites of cpsr_write() to rebuild the
>> cached hflags if they change one of the CPSR bits which we use as a
>> TB flag and cache in hflags.  This is a bit awkward when we want to
>> change the set of CPSR bits that we cache, because it means we need
>> to re-audit all the cpsr_write() callsites to see which flags they
>> are writing and whether they now need to rebuild the hflags.
>>
>> Switch instead to making cpsr_write() call arm_rebuild_hflags()
>> itself if one of the bits being changed is a cached bit.
>>
>> We don't do the rebuild for the CPSRWriteRaw write type,
> 
> Doh. I said this, but then...
> 
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 201ecf8c67f..cdd6e0858fc 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -9215,6 +9215,7 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
>>                   CPSRWriteType write_type)
>>   {
>>       uint32_t changed_daif;
>> +    bool rebuild_hflags = mask & (CPSR_M | CPSR_E | CPSR_IL);
> 
> ...forgot to actually check the write type.
> 
> Should be:
> 
>      bool rebuild_hflags = (write_type != CPSRWriteRaw) &&
>          (mask & (CPSR_M | CPSR_E | CPSR_IL));

with the fix,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~