Re: [PATCH v6 18/20] target/arm: Rebuild hflags at CPSR writes

[Richard Henderson]

On 10/14/19 12:08 PM, Alex Bennée wrote:
> 
> Richard Henderson  writes:
> 
>> Continue setting, but not relying upon, env->hflags.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  target/arm/op_helper.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
>> index ccc2cecb46..b529d6c1bf 100644
>> --- a/target/arm/op_helper.c
>> +++ b/target/arm/op_helper.c
>> @@ -224,6 +224,7 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x, 
>> uint32_t shift)
>>  void HELPER(setend)(CPUARMState *env)
>>  {
>>  env->uncached_cpsr ^= CPSR_E;
>> +arm_rebuild_hflags(env);
>>  }
>>
>>  /* Function checks whether WFx (WFI/WFE) instructions are set up to be 
>> trapped.
>> @@ -387,6 +388,8 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
>>  void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
>>  {
>>  cpsr_write(env, val, mask, CPSRWriteByInstr);
>> +/* TODO: Not all cpsr bits are relevant to hflags.  */
> 
> Do you mean by this we could check which bits changed and avoid a
> re-compute if we wanted to? Is it likely to be anything other than the
> SS_ACTIVE bit?

Yes.  Well, there's also NZCV.  But neither case happens particularly often, so
it's probably not worth worrying about.


r~

Re: [PATCH v6 18/20] target/arm: Rebuild hflags at CPSR writes

[Alex Bennée]

Richard Henderson  writes:

> Continue setting, but not relying upon, env->hflags.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/op_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index ccc2cecb46..b529d6c1bf 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -224,6 +224,7 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x, 
> uint32_t shift)
>  void HELPER(setend)(CPUARMState *env)
>  {
>  env->uncached_cpsr ^= CPSR_E;
> +arm_rebuild_hflags(env);
>  }
>
>  /* Function checks whether WFx (WFI/WFE) instructions are set up to be 
> trapped.
> @@ -387,6 +388,8 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
>  void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
>  {
>  cpsr_write(env, val, mask, CPSRWriteByInstr);
> +/* TODO: Not all cpsr bits are relevant to hflags.  */

Do you mean by this we could check which bits changed and avoid a
re-compute if we wanted to? Is it likely to be anything other than the
SS_ACTIVE bit?

> +arm_rebuild_hflags(env);
>  }
>
>  /* Write the CPSR for a 32-bit exception return */

Anyway:

Reviewed-by: Alex Bennée 


--
Alex Bennée

[PATCH v6 18/20] target/arm: Rebuild hflags at CPSR writes

[Richard Henderson]

Continue setting, but not relying upon, env->hflags.

Signed-off-by: Richard Henderson 
---
 target/arm/op_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index ccc2cecb46..b529d6c1bf 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -224,6 +224,7 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x, 
uint32_t shift)
 void HELPER(setend)(CPUARMState *env)
 {
 env->uncached_cpsr ^= CPSR_E;
+arm_rebuild_hflags(env);
 }
 
 /* Function checks whether WFx (WFI/WFE) instructions are set up to be trapped.
@@ -387,6 +388,8 @@ uint32_t HELPER(cpsr_read)(CPUARMState *env)
 void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
 {
 cpsr_write(env, val, mask, CPSRWriteByInstr);
+/* TODO: Not all cpsr bits are relevant to hflags.  */
+arm_rebuild_hflags(env);
 }
 
 /* Write the CPSR for a 32-bit exception return */
-- 
2.17.1