[v1] target/sparc: Cleanup condition codes etc

[PATCH 01/20] target/sparc: Introduce cpu_put_psr_icc

Posted by Richard Henderson 2 years, 3 months ago

Isolate linux-user from changes to icc representation.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/sparc/cpu.h        | 1 +
 linux-user/sparc/signal.c | 2 +-
 target/sparc/win_helper.c | 7 ++++++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 758a4e8aaa..955329f6c9 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -619,6 +619,7 @@ void sparc_restore_state_to_opc(CPUState *cs,
 /* win_helper.c */
 target_ulong cpu_get_psr(CPUSPARCState *env1);
 void cpu_put_psr(CPUSPARCState *env1, target_ulong val);
+void cpu_put_psr_icc(CPUSPARCState *env1, target_ulong val);
 void cpu_put_psr_raw(CPUSPARCState *env1, target_ulong val);
 #ifdef TARGET_SPARC64
 void cpu_change_pstate(CPUSPARCState *env1, uint32_t new_pstate);
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index 2be9000b9e..dfcae707e0 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -164,7 +164,7 @@ static void restore_pt_regs(struct target_pt_regs *regs, CPUSPARCState *env)
      */
     uint32_t psr;
     __get_user(psr, &regs->psr);
-    env->psr = (psr & PSR_ICC) | (env->psr & ~PSR_ICC);
+    cpu_put_psr_icc(env, psr);
 #endif
 
     /* Note that pc and npc are handled in the caller. */
diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
index 3a7c0ff943..bf2c90c780 100644
--- a/target/sparc/win_helper.c
+++ b/target/sparc/win_helper.c
@@ -67,9 +67,14 @@ target_ulong cpu_get_psr(CPUSPARCState *env)
 #endif
 }
 
-void cpu_put_psr_raw(CPUSPARCState *env, target_ulong val)
+void cpu_put_psr_icc(CPUSPARCState *env, target_ulong val)
 {
     env->psr = val & PSR_ICC;
+}
+
+void cpu_put_psr_raw(CPUSPARCState *env, target_ulong val)
+{
+    cpu_put_psr_icc(env, val);
 #if !defined(TARGET_SPARC64)
     env->psref = (val & PSR_EF) ? 1 : 0;
     env->psrpil = (val & PSR_PIL) >> 8;
-- 
2.34.1

Re: [PATCH 01/20] target/sparc: Introduce cpu_put_psr_icc

Posted by Philippe Mathieu-Daudé 2 years, 3 months ago

Hi Richard,

On 17/10/23 08:40, Richard Henderson wrote:
> Isolate linux-user from changes to icc representation.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/sparc/cpu.h        | 1 +
>   linux-user/sparc/signal.c | 2 +-
>   target/sparc/win_helper.c | 7 ++++++-
>   3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index 758a4e8aaa..955329f6c9 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -619,6 +619,7 @@ void sparc_restore_state_to_opc(CPUState *cs,
>   /* win_helper.c */
>   target_ulong cpu_get_psr(CPUSPARCState *env1);
>   void cpu_put_psr(CPUSPARCState *env1, target_ulong val);
> +void cpu_put_psr_icc(CPUSPARCState *env1, target_ulong val);
>   void cpu_put_psr_raw(CPUSPARCState *env1, target_ulong val);
>   #ifdef TARGET_SPARC64
>   void cpu_change_pstate(CPUSPARCState *env1, uint32_t new_pstate);
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index 2be9000b9e..dfcae707e0 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -164,7 +164,7 @@ static void restore_pt_regs(struct target_pt_regs *regs, CPUSPARCState *env)
>        */
>       uint32_t psr;
>       __get_user(psr, &regs->psr);
> -    env->psr = (psr & PSR_ICC) | (env->psr & ~PSR_ICC);

This keeps the non-PSR_ICC fields from env->psr, ...

> +    cpu_put_psr_icc(env, psr);
>   #endif
>   
>       /* Note that pc and npc are handled in the caller. */
> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
> index 3a7c0ff943..bf2c90c780 100644
> --- a/target/sparc/win_helper.c
> +++ b/target/sparc/win_helper.c
> @@ -67,9 +67,14 @@ target_ulong cpu_get_psr(CPUSPARCState *env)
>   #endif
>   }
>   
> -void cpu_put_psr_raw(CPUSPARCState *env, target_ulong val)
> +void cpu_put_psr_icc(CPUSPARCState *env, target_ulong val)
>   {
>       env->psr = val & PSR_ICC;

... while this zeroes the non-PSR_ICC fields. Is that expected?

> +}
> +
> +void cpu_put_psr_raw(CPUSPARCState *env, target_ulong val)
> +{
> +    cpu_put_psr_icc(env, val);
>   #if !defined(TARGET_SPARC64)
>       env->psref = (val & PSR_EF) ? 1 : 0;
>       env->psrpil = (val & PSR_PIL) >> 8;

Re: [PATCH 01/20] target/sparc: Introduce cpu_put_psr_icc

Posted by Richard Henderson 2 years, 3 months ago

On 10/19/23 07:11, Philippe Mathieu-Daudé wrote:
>>       uint32_t psr;
>>       __get_user(psr, &regs->psr);
>> -    env->psr = (psr & PSR_ICC) | (env->psr & ~PSR_ICC);
> 
> This keeps the non-PSR_ICC fields from env->psr, ...
> 
>> +    cpu_put_psr_icc(env, psr);
>>   #endif
>>       /* Note that pc and npc are handled in the caller. */
>> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
>> index 3a7c0ff943..bf2c90c780 100644
>> --- a/target/sparc/win_helper.c
>> +++ b/target/sparc/win_helper.c
>> @@ -67,9 +67,14 @@ target_ulong cpu_get_psr(CPUSPARCState *env)
>>   #endif
>>   }
>> -void cpu_put_psr_raw(CPUSPARCState *env, target_ulong val)
>> +void cpu_put_psr_icc(CPUSPARCState *env, target_ulong val)
>>   {
>>       env->psr = val & PSR_ICC;
> 
> ... while this zeroes the non-PSR_ICC fields. Is that expected?

The only field in env->psr is ICC.
The other fields are in env->psr{s,ps,et,pil,ef}.

This is a bit of old linux-user confusion, which apparently presumed more.
Anyway, the situation is improved with this patch, and further in the next by removing 
"env->psr" entirely.

r~

PS: The handling of PSR could probably be cleaned up.  I don't think there is a real need 
for the fields to be split apart like this.  A test of e.g. (env->psr & PSR_S) should be 
more or less identical in performance to (env->psrs != 0).  It's only the PSR_ICC 
subfields that get heavy use within translation.