diff mbox series

[03/13] target/hppa: Fix PSW_W and PSW_E bits in rsm, ssm and mtsm

Message ID 20240207182023.36316-4-deller@kernel.org
State New
Headers show
Series target/hppa: Enhancements and fixes | expand

Commit Message

Helge Deller Feb. 7, 2024, 6:20 p.m. UTC 
From: Helge Deller <deller@gmx.de>

The ssm and rsm instructions number the PSW_W and PSW_E bits differently
than how they are actually in the PSW. Both bits are relevant on 64-bit
CPUs only.

Fix the existing ssm and rsm instructions to handle the bits correctly.
For that drop the swap_system_mask() helper function in favour of new
helper functions get_system_mask, set_system_mask and mtsm_system_mask.

get_system_mask() returns the PSW bits how they should be returned in
the target register of the ssm and rsm instructions.
set_system_mask() sets the PSW bits without any further modification.

Note that the a->i constant value of ssm and rsm have already been
converted to match the physical PSW bits by expand_sm_imm() in the
instruction decoder.

The mtsm instruction is different, as it takes the new PSW from a
register at runtime, and as such the PSW.E and PSW.W bits are moved at
runtime to the right bit positions before setting the PSW.

Signed-off-by: Helge Deller <deller@gmx.de>
---
 target/hppa/cpu.h        |  2 ++
 target/hppa/helper.h     |  4 +++-
 target/hppa/sys_helper.c | 46 +++++++++++++++++++++++++++++++++++++---
 target/hppa/translate.c  | 38 +++++++++++++++++++++------------
 4 files changed, 73 insertions(+), 17 deletions(-)

Comments

Richard Henderson Feb. 8, 2024, 8:43 p.m. UTC | #1 
On 2/7/24 08:20, deller@kernel.org wrote:
>   #define PSW_E            0x04000000
> +#define PSW_E_BIT                37 /* PA2.0 only */
>   #define PSW_W            0x08000000 /* PA2.0 only */
> +#define PSW_W_BIT                36 /* PA2.0 only */
...
> +target_ulong HELPER(get_system_mask)(CPUHPPAState *env)
> +{
> +    target_ulong psw = env->psw;
> +
> +    /* mask out invalid bits */
> +    target_ulong psw_new = psw & PSW_SM;
> +
> +    /* ssm/rsm instructions number PSW_W and PSW_E differently */
> +    psw_new &= ~PSW_W;
> +    if (psw & PSW_W) {
> +        psw_new |= 1ull << (63 - PSW_W_BIT);
> +    }

Um, this has changed nothing, since 1 << (63 - 36) == 0x8000000 == PSW_W.

The conversion of PSW_SM_W to PSW_W happens in expand_sm_imm().
There's a comment there about keeping unimplemented bits disabled, including PSW_E. 
Perhaps this is the wrong layer in which to do this?

In any case, what is the actual problem that you're seeing?  Because it *isn't* that we 
were not considering the different placement of the bits, as your commit message suggests.

> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 53ec57ee86..10fdc0813d 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2163,13 +2163,20 @@ static bool trans_rsm(DisasContext *ctx, arg_rsm *a)
>       nullify_over(ctx);
>   
>       tmp = tcg_temp_new_i64();
> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> -    tcg_gen_andi_i64(tmp, tmp, ~a->i);
> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
> -    save_gpr(ctx, a->t, tmp);
> +    if (a->t != 0) {
> +        gen_helper_get_system_mask(tmp, tcg_env);
> +        save_gpr(ctx, a->t, tmp);
> +    }

If a->t == 0, tmp is uninitialized...

> +
> +    if (a->i) {
> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));

... but read here.

> @@ -2183,11 +2190,17 @@ static bool trans_ssm(DisasContext *ctx, arg_ssm *a)
>       nullify_over(ctx);
>   
>       tmp = tcg_temp_new_i64();
> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> -    tcg_gen_ori_i64(tmp, tmp, a->i);
> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
> -    save_gpr(ctx, a->t, tmp);
> +    if (a->t != 0) {
> +        gen_helper_get_system_mask(tmp, tcg_env);
> +        save_gpr(ctx, a->t, tmp);
> +    }
> +
> +    if (a->i) {
> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> +        tcg_gen_ori_i64(tmp, tmp, a->i);

Likewise.


r~
Helge Deller Feb. 9, 2024, 8:55 a.m. UTC | #2 
On 2/8/24 21:43, Richard Henderson wrote:
> On 2/7/24 08:20, deller@kernel.org wrote:
>>   #define PSW_E            0x04000000
>> +#define PSW_E_BIT                37 /* PA2.0 only */
>>   #define PSW_W            0x08000000 /* PA2.0 only */
>> +#define PSW_W_BIT                36 /* PA2.0 only */
> ...
>> +target_ulong HELPER(get_system_mask)(CPUHPPAState *env)
>> +{
>> +    target_ulong psw = env->psw;
>> +
>> +    /* mask out invalid bits */
>> +    target_ulong psw_new = psw & PSW_SM;
>> +
>> +    /* ssm/rsm instructions number PSW_W and PSW_E differently */
>> +    psw_new &= ~PSW_W;
>> +    if (psw & PSW_W) {
>> +        psw_new |= 1ull << (63 - PSW_W_BIT);
>> +    }
>
> Um, this has changed nothing, since 1 << (63 - 36) == 0x8000000 == PSW_W.

Yep.
I seem to have mixed strange things when writing that.
I've dropped the patch for now.

Thanks!
Helge


> The conversion of PSW_SM_W to PSW_W happens in expand_sm_imm().
> There's a comment there about keeping unimplemented bits disabled, including PSW_E. Perhaps this is the wrong layer in which to do this?
>
> In any case, what is the actual problem that you're seeing?  Because it *isn't* that we were not considering the different placement of the bits, as your commit message suggests.
>
>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>> index 53ec57ee86..10fdc0813d 100644
>> --- a/target/hppa/translate.c
>> +++ b/target/hppa/translate.c
>> @@ -2163,13 +2163,20 @@ static bool trans_rsm(DisasContext *ctx, arg_rsm *a)
>>       nullify_over(ctx);
>>       tmp = tcg_temp_new_i64();
>> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
>> -    tcg_gen_andi_i64(tmp, tmp, ~a->i);
>> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
>> -    save_gpr(ctx, a->t, tmp);
>> +    if (a->t != 0) {
>> +        gen_helper_get_system_mask(tmp, tcg_env);
>> +        save_gpr(ctx, a->t, tmp);
>> +    }
>
> If a->t == 0, tmp is uninitialized...
>
>> +
>> +    if (a->i) {
>> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
>
> ... but read here.
>
>> @@ -2183,11 +2190,17 @@ static bool trans_ssm(DisasContext *ctx, arg_ssm *a)
>>       nullify_over(ctx);
>>       tmp = tcg_temp_new_i64();
>> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
>> -    tcg_gen_ori_i64(tmp, tmp, a->i);
>> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
>> -    save_gpr(ctx, a->t, tmp);
>> +    if (a->t != 0) {
>> +        gen_helper_get_system_mask(tmp, tcg_env);
>> +        save_gpr(ctx, a->t, tmp);
>> +    }
>> +
>> +    if (a->i) {
>> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
>> +        tcg_gen_ori_i64(tmp, tmp, a->i);
>
> Likewise.
>
>
> r~
diff mbox series

Patch

diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index 7a181e8f33..06b65f2258 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -122,7 +122,9 @@ 
 #define PSW_T            0x01000000
 #define PSW_S            0x02000000
 #define PSW_E            0x04000000
+#define PSW_E_BIT                37 /* PA2.0 only */
 #define PSW_W            0x08000000 /* PA2.0 only */
+#define PSW_W_BIT                36 /* PA2.0 only */
 #define PSW_Z            0x40000000 /* PA1.x only */
 #define PSW_Y            0x80000000 /* PA1.x only */
 
diff --git a/target/hppa/helper.h b/target/hppa/helper.h
index 1bdbcd8f98..abffd3f531 100644
--- a/target/hppa/helper.h
+++ b/target/hppa/helper.h
@@ -92,7 +92,9 @@  DEF_HELPER_1(rfi_r, void, env)
 DEF_HELPER_FLAGS_2(write_interval_timer, TCG_CALL_NO_RWG, void, env, tl)
 DEF_HELPER_FLAGS_2(write_eirr, TCG_CALL_NO_RWG, void, env, tl)
 DEF_HELPER_FLAGS_2(write_eiem, TCG_CALL_NO_RWG, void, env, tl)
-DEF_HELPER_FLAGS_2(swap_system_mask, TCG_CALL_NO_RWG, tl, env, tl)
+DEF_HELPER_FLAGS_1(get_system_mask, TCG_CALL_NO_RWG, tl, env)
+DEF_HELPER_FLAGS_2(set_system_mask, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_FLAGS_2(mtsm_system_mask, TCG_CALL_NO_RWG, void, env, tl)
 DEF_HELPER_FLAGS_3(itlba_pa11, TCG_CALL_NO_RWG, void, env, tl, tl)
 DEF_HELPER_FLAGS_3(itlbp_pa11, TCG_CALL_NO_RWG, void, env, tl, tl)
 DEF_HELPER_FLAGS_3(idtlbt_pa20, TCG_CALL_NO_RWG, void, env, tl, tl)
diff --git a/target/hppa/sys_helper.c b/target/hppa/sys_helper.c
index a59245eed3..88ba99f0d4 100644
--- a/target/hppa/sys_helper.c
+++ b/target/hppa/sys_helper.c
@@ -58,7 +58,27 @@  void HELPER(reset)(CPUHPPAState *env)
     helper_excp(env, EXCP_HLT);
 }
 
-target_ulong HELPER(swap_system_mask)(CPUHPPAState *env, target_ulong nsm)
+target_ulong HELPER(get_system_mask)(CPUHPPAState *env)
+{
+    target_ulong psw = env->psw;
+
+    /* mask out invalid bits */
+    target_ulong psw_new = psw & PSW_SM;
+
+    /* ssm/rsm instructions number PSW_W and PSW_E differently */
+    psw_new &= ~PSW_W;
+    if (psw & PSW_W) {
+        psw_new |= 1ull << (63 - PSW_W_BIT);
+    }
+    psw_new &= ~PSW_E;
+    if (psw & PSW_E) {
+        psw_new |= 1ull << (63 - PSW_E_BIT);
+    }
+
+    return psw_new;
+}
+
+void HELPER(set_system_mask)(CPUHPPAState *env, target_ulong nsm)
 {
     target_ulong psw = env->psw;
     /*
@@ -70,8 +90,28 @@  target_ulong HELPER(swap_system_mask)(CPUHPPAState *env, target_ulong nsm)
      * machines set the Q bit from 0 to 1 without an exception,
      * so let this go without comment.
      */
-    env->psw = (psw & ~PSW_SM) | (nsm & PSW_SM);
-    return psw & PSW_SM;
+
+    cpu_hppa_put_psw(env, (psw & ~PSW_SM) | (nsm & PSW_SM));
+}
+
+void HELPER(mtsm_system_mask)(CPUHPPAState *env, target_ulong nsm)
+{
+    target_ulong psw_new;
+
+    /* mask out invalid bits */
+    psw_new = nsm & PSW_SM;
+
+    /* set PSW_E and PSW_W */
+    psw_new &= ~PSW_W;
+    if (nsm & (1ull << (63 - PSW_W_BIT))) {
+        psw_new |= PSW_W;
+    }
+    psw_new &= ~PSW_E;
+    if (nsm & (1ull << (63 - PSW_E_BIT))) {
+        psw_new |= PSW_E;
+    }
+
+    helper_set_system_mask(env, psw_new);
 }
 
 void HELPER(rfi)(CPUHPPAState *env)
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 53ec57ee86..10fdc0813d 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2163,13 +2163,20 @@  static bool trans_rsm(DisasContext *ctx, arg_rsm *a)
     nullify_over(ctx);
 
     tmp = tcg_temp_new_i64();
-    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
-    tcg_gen_andi_i64(tmp, tmp, ~a->i);
-    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
-    save_gpr(ctx, a->t, tmp);
+    if (a->t != 0) {
+        gen_helper_get_system_mask(tmp, tcg_env);
+        save_gpr(ctx, a->t, tmp);
+    }
+
+    if (a->i) {
+        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
+        tcg_gen_andi_i64(tmp, tmp, ~a->i);
+        gen_helper_set_system_mask(tcg_env, tmp);
+
+        /* Exit, check e.g. for new interrupts */
+        ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
+    }
 
-    /* Exit the TB to recognize new interrupts, e.g. PSW_M.  */
-    ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
     return nullify_end(ctx);
 #endif
 }
@@ -2183,11 +2190,17 @@  static bool trans_ssm(DisasContext *ctx, arg_ssm *a)
     nullify_over(ctx);
 
     tmp = tcg_temp_new_i64();
-    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
-    tcg_gen_ori_i64(tmp, tmp, a->i);
-    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
-    save_gpr(ctx, a->t, tmp);
+    if (a->t != 0) {
+        gen_helper_get_system_mask(tmp, tcg_env);
+        save_gpr(ctx, a->t, tmp);
+    }
+
+    if (a->i) {
+        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
+        tcg_gen_ori_i64(tmp, tmp, a->i);
+        gen_helper_set_system_mask(tcg_env, tmp);
 
+    }
     /* Exit the TB to recognize new interrupts, e.g. PSW_I.  */
     ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
     return nullify_end(ctx);
@@ -2198,12 +2211,11 @@  static bool trans_mtsm(DisasContext *ctx, arg_mtsm *a)
 {
     CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
 #ifndef CONFIG_USER_ONLY
-    TCGv_i64 tmp, reg;
+    TCGv_i64 reg;
     nullify_over(ctx);
 
     reg = load_gpr(ctx, a->r);
-    tmp = tcg_temp_new_i64();
-    gen_helper_swap_system_mask(tmp, tcg_env, reg);
+    gen_helper_mtsm_system_mask(tcg_env, reg);
 
     /* Exit the TB to recognize new interrupts.  */
     ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;