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

[Richard Henderson]

On 2/7/24 08:20, del...@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~