[PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX

deller@kernel.org posted 5 patches 1 year ago
[PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX
Posted by deller@kernel.org 1 year ago
From: Helge Deller <deller@gmx.de>

PA-RISC CPUs have diagnose registers (%dr). Those are mostly
undocumented and control cache behaviour, memory behaviour, reset button
management and many other related internal CPU things.

The Linux kernel turns space-register hashing off unconditionally at
bootup.  That code was provided by HP at the beginning of the PA-RISC
Linux porting effort, and I don't know why it was decided then why Linux
should not use space register hashing.
32-bit HP-UX versions seem to not use space register hashing either.
But for 64-bit HP-UX versions, Sven Schnelle noticed that space register
hashing needs to be enabled and is required, otherwise the HP-UX kernel
will crash badly.

On 64-bit CPUs space register hashing is controlled by a bit in diagnose
register %dr2.  Since we want to support Linux and 32- and 64-bit HP-UX,
we need to fully emulate the diagnose registers and handle specifically
the content of %dr2.

Furthermore it turned out that commit 3bdf20819e68 seems wrong and
conflicts with the general space register diagnose register, so it is
partly reverted here.

Signed-off-by: Helge Deller <deller@gmx.de>
Suggested-by: Sven Schnelle <svens@stackframe.org>
Fixes: 3bdf20819e68 ("target/hppa: Add diag instructions to set/restore shadow registers")
---
 hw/hppa/machine.c        |  5 +++++
 target/hppa/cpu.c        |  3 ++-
 target/hppa/cpu.h        | 24 ++++++++++++++++--------
 target/hppa/helper.c     |  4 ++--
 target/hppa/insns.decode |  6 ++++--
 target/hppa/int_helper.c |  6 +++---
 target/hppa/machine.c    |  5 +++--
 target/hppa/sys_helper.c |  2 +-
 target/hppa/translate.c  | 24 +++++++++++++++++-------
 9 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 0dd1908214..d5de793b62 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -662,6 +662,11 @@ static void hppa_machine_reset(MachineState *ms, ResetType type)
         cpu_set_pc(cs, firmware_entry);
         cpu[i]->env.psw = PSW_Q;
         cpu[i]->env.gr[5] = CPU_HPA + i * 0x1000;
+
+        /* 64-bit machines start with space-register hashing enabled in %dr2 */
+        if (hppa_is_pa20(&cpu[0]->env)) {
+            cpu[i]->env.dr[2] = HPPA64_DIAG_SPHASH_ENABLE; /* (bit 54) */
+        }
     }
 
     cpu[0]->env.gr[26] = ms->ram_size;
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index b0bc9d35e4..9a83910cae 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -44,7 +44,8 @@ static vaddr hppa_cpu_get_pc(CPUState *cs)
 {
     CPUHPPAState *env = cpu_env(cs);
 
-    return hppa_form_gva_psw(env->psw, (env->psw & PSW_C ? env->iasq_f : 0),
+    return hppa_form_gva_psw(env, env->psw,
+                            (env->psw & PSW_C ? env->iasq_f : 0),
                              env->iaoq_f & -4);
 }
 
diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index beea42d105..64e60a3980 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -25,6 +25,7 @@
 #include "qemu/cpu-float.h"
 #include "qemu/interval-tree.h"
 #include "hw/registerfields.h"
+#include "hw/hppa/hppa_hardware.h"
 
 #define MMU_ABS_W_IDX     6
 #define MMU_ABS_IDX       7
@@ -232,6 +233,7 @@ typedef struct CPUArchState {
     target_ulong cr[32];     /* control registers */
     target_ulong cr_back[2]; /* back of cr17/cr18 */
     target_ulong shadow[7];  /* shadow registers */
+    target_ulong dr[32];     /* diagnose registers */
 
     /*
      * During unwind of a memory insn, the base register of the address.
@@ -319,27 +321,33 @@ void hppa_translate_code(CPUState *cs, TranslationBlock *tb,
 
 #define CPU_RESOLVING_TYPE TYPE_HPPA_CPU
 
-static inline uint64_t gva_offset_mask(target_ulong psw)
+static inline uint64_t gva_offset_mask(CPUHPPAState *env, target_ulong psw)
 {
-    return (psw & PSW_W
-            ? MAKE_64BIT_MASK(0, 62)
-            : MAKE_64BIT_MASK(0, 32));
+    if (psw & PSW_W) {
+        return (env->dr[2] & HPPA64_DIAG_SPHASH_ENABLE)
+            ? MAKE_64BIT_MASK(0, 62) &
+                ~((uint64_t)HPPA64_PDC_CACHE_RET_SPID_VAL << 48)
+            : MAKE_64BIT_MASK(0, 62);
+    } else {
+        return MAKE_64BIT_MASK(0, 32);
+    }
 }
 
-static inline target_ulong hppa_form_gva_psw(target_ulong psw, uint64_t spc,
-                                             target_ulong off)
+static inline target_ulong hppa_form_gva_psw(CPUHPPAState *env,
+                                             target_ulong psw,
+                                             uint64_t spc, target_ulong off)
 {
 #ifdef CONFIG_USER_ONLY
     return off & gva_offset_mask(psw);
 #else
-    return spc | (off & gva_offset_mask(psw));
+    return spc | (off & gva_offset_mask(env, psw));
 #endif
 }
 
 static inline target_ulong hppa_form_gva(CPUHPPAState *env, uint64_t spc,
                                          target_ulong off)
 {
-    return hppa_form_gva_psw(env->psw, spc, off);
+    return hppa_form_gva_psw(env, env->psw, spc, off);
 }
 
 hwaddr hppa_abs_to_phys_pa2_w0(vaddr addr);
diff --git a/target/hppa/helper.c b/target/hppa/helper.c
index d4b1a3cd5a..cd7eeb5dfa 100644
--- a/target/hppa/helper.c
+++ b/target/hppa/helper.c
@@ -133,9 +133,9 @@ void hppa_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     qemu_fprintf(f, "IA_F %08" PRIx64 ":%0*" PRIx64 " (" TARGET_FMT_lx ")\n"
                     "IA_B %08" PRIx64 ":%0*" PRIx64 " (" TARGET_FMT_lx ")\n",
                  env->iasq_f >> 32, w, m & env->iaoq_f,
-                 hppa_form_gva_psw(psw, env->iasq_f, env->iaoq_f),
+                 hppa_form_gva_psw(env, psw, env->iasq_f, env->iaoq_f),
                  env->iasq_b >> 32, w, m & env->iaoq_b,
-                 hppa_form_gva_psw(psw, env->iasq_b, env->iaoq_b));
+                 hppa_form_gva_psw(env, psw, env->iasq_b, env->iaoq_b));
 
     psw_c[0]  = (psw & PSW_W ? 'W' : '-');
     psw_c[1]  = (psw & PSW_E ? 'E' : '-');
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index 71074a64c1..4eaac750ea 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -644,10 +644,12 @@ xmpyu           001110 ..... ..... 010 .0111 .00 t:5    r1=%ra64 r2=%rb64
     # For 32-bit PA-7300LC (PCX-L2)
     diag_getshadowregs_pa1  000101 00 0000 0000 0001 1010 0000 0000
     diag_putshadowregs_pa1  000101 00 0000 0000 0001 1010 0100 0000
+    diag_mfdiag             000101 dr:5  rt:5   0000 0110 0000 0000
+    diag_mtdiag             000101 dr:5  r1:5   0001 0110 0000 0000
 
     # For 64-bit PA8700 (PCX-W2)
-    diag_getshadowregs_pa2  000101 00 0111 1000 0001 1000 0100 0000
-    diag_putshadowregs_pa2  000101 00 0111 0000 0001 1000 0100 0000
+    diag_mfdiag             000101 dr:5  0 0000 0000 1000 101  rt:5
+    diag_mtdiag             000101 dr:5  r1:5   0001 1000 0100 0000
   ]
   diag_unimp                000101 i:26
 }
diff --git a/target/hppa/int_helper.c b/target/hppa/int_helper.c
index 58695def82..5aefb3ade4 100644
--- a/target/hppa/int_helper.c
+++ b/target/hppa/int_helper.c
@@ -112,9 +112,9 @@ void hppa_cpu_do_interrupt(CPUState *cs)
      */
     if (old_psw & PSW_C) {
         env->cr[CR_IIASQ] =
-            hppa_form_gva_psw(old_psw, env->iasq_f, env->iaoq_f) >> 32;
+            hppa_form_gva_psw(env, old_psw, env->iasq_f, env->iaoq_f) >> 32;
         env->cr_back[0] =
-            hppa_form_gva_psw(old_psw, env->iasq_b, env->iaoq_b) >> 32;
+            hppa_form_gva_psw(env, old_psw, env->iasq_b, env->iaoq_b) >> 32;
     } else {
         env->cr[CR_IIASQ] = 0;
         env->cr_back[0] = 0;
@@ -165,7 +165,7 @@ void hppa_cpu_do_interrupt(CPUState *cs)
                 if (old_psw & PSW_C) {
                     int prot, t;
 
-                    vaddr = hppa_form_gva_psw(old_psw, env->iasq_f, vaddr);
+                    vaddr = hppa_form_gva_psw(env, old_psw, env->iasq_f, vaddr);
                     t = hppa_get_physical_address(env, vaddr, MMU_KERNEL_IDX,
                                                   0, 0, &paddr, &prot);
                     if (t >= 0) {
diff --git a/target/hppa/machine.c b/target/hppa/machine.c
index 211bfcf640..bb47a2e689 100644
--- a/target/hppa/machine.c
+++ b/target/hppa/machine.c
@@ -198,6 +198,7 @@ static const VMStateField vmstate_env_fields[] = {
     VMSTATE_UINT64(iasq_b, CPUHPPAState),
 
     VMSTATE_UINT32(fr0_shadow, CPUHPPAState),
+    VMSTATE_UINT64_ARRAY(dr, CPUHPPAState, 32),
     VMSTATE_END_OF_LIST()
 };
 
@@ -208,8 +209,8 @@ static const VMStateDescription * const vmstate_env_subsections[] = {
 
 static const VMStateDescription vmstate_env = {
     .name = "env",
-    .version_id = 3,
-    .minimum_version_id = 3,
+    .version_id = 4,
+    .minimum_version_id = 4,
     .fields = vmstate_env_fields,
     .subsections = vmstate_env_subsections,
 };
diff --git a/target/hppa/sys_helper.c b/target/hppa/sys_helper.c
index da5b569de8..72bb9ea7b5 100644
--- a/target/hppa/sys_helper.c
+++ b/target/hppa/sys_helper.c
@@ -88,7 +88,7 @@ void HELPER(rfi)(CPUHPPAState *env)
      * To recreate the space identifier, remove the offset bits.
      * For pa1.x, the mask reduces to no change to space.
      */
-    mask = gva_offset_mask(env->psw);
+    mask = gva_offset_mask(env, env->psw);
 
     env->iaoq_f = env->cr[CR_IIAOQ];
     env->iaoq_b = env->cr_back[1];
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index dc04f9f3c0..45a40d2c5e 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -1577,7 +1577,7 @@ static void form_gva(DisasContext *ctx, TCGv_i64 *pgva, TCGv_i64 *pofs,
     *pofs = ofs;
     *pgva = addr = tcg_temp_new_i64();
     tcg_gen_andi_i64(addr, modify <= 0 ? ofs : base,
-                     gva_offset_mask(ctx->tb_flags));
+                     gva_offset_mask(cpu_env(ctx->cs), ctx->tb_flags));
 #ifndef CONFIG_USER_ONLY
     if (!is_phys) {
         tcg_gen_or_i64(addr, addr, space_select(ctx, sp, base));
@@ -4593,19 +4593,29 @@ static bool trans_diag_getshadowregs_pa1(DisasContext *ctx, arg_empty *a)
     return !ctx->is_pa20 && do_getshadowregs(ctx);
 }
 
-static bool trans_diag_getshadowregs_pa2(DisasContext *ctx, arg_empty *a)
+static bool trans_diag_putshadowregs_pa1(DisasContext *ctx, arg_empty *a)
 {
-    return ctx->is_pa20 && do_getshadowregs(ctx);
+    return !ctx->is_pa20 && do_putshadowregs(ctx);
 }
 
-static bool trans_diag_putshadowregs_pa1(DisasContext *ctx, arg_empty *a)
+static bool trans_diag_mfdiag(DisasContext *ctx, arg_diag_mfdiag *a)
 {
-    return !ctx->is_pa20 && do_putshadowregs(ctx);
+    CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
+    nullify_over(ctx);
+    TCGv_i64 dest = dest_gpr(ctx, a->rt);
+    tcg_gen_ld_i64(dest, tcg_env,
+                       offsetof(CPUHPPAState, dr[a->dr]));
+    save_gpr(ctx, a->rt, dest);
+    return nullify_end(ctx);
 }
 
-static bool trans_diag_putshadowregs_pa2(DisasContext *ctx, arg_empty *a)
+static bool trans_diag_mtdiag(DisasContext *ctx, arg_diag_mtdiag *a)
 {
-    return ctx->is_pa20 && do_putshadowregs(ctx);
+    CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
+    nullify_over(ctx);
+    tcg_gen_st_i64(load_gpr(ctx, a->r1), tcg_env,
+                        offsetof(CPUHPPAState, dr[a->dr]));
+    return nullify_end(ctx);
 }
 
 static bool trans_diag_unimp(DisasContext *ctx, arg_diag_unimp *a)
-- 
2.47.0
Re: [PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX
Posted by Richard Henderson 1 year ago
On 1/28/25 08:14, deller@kernel.org wrote:
> From: Helge Deller <deller@gmx.de>
> 
> PA-RISC CPUs have diagnose registers (%dr). Those are mostly
> undocumented and control cache behaviour, memory behaviour, reset button
> management and many other related internal CPU things.
> 
> The Linux kernel turns space-register hashing off unconditionally at
> bootup.  That code was provided by HP at the beginning of the PA-RISC
> Linux porting effort, and I don't know why it was decided then why Linux
> should not use space register hashing.
> 32-bit HP-UX versions seem to not use space register hashing either.
> But for 64-bit HP-UX versions, Sven Schnelle noticed that space register
> hashing needs to be enabled and is required, otherwise the HP-UX kernel
> will crash badly.
> 
> On 64-bit CPUs space register hashing is controlled by a bit in diagnose
> register %dr2.  Since we want to support Linux and 32- and 64-bit HP-UX,
> we need to fully emulate the diagnose registers and handle specifically
> the content of %dr2.
> 
> Furthermore it turned out that commit 3bdf20819e68 seems wrong and
> conflicts with the general space register diagnose register, so it is
> partly reverted here.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Suggested-by: Sven Schnelle <svens@stackframe.org>
> Fixes: 3bdf20819e68 ("target/hppa: Add diag instructions to set/restore shadow registers")
> ---
>   hw/hppa/machine.c        |  5 +++++
>   target/hppa/cpu.c        |  3 ++-
>   target/hppa/cpu.h        | 24 ++++++++++++++++--------
>   target/hppa/helper.c     |  4 ++--
>   target/hppa/insns.decode |  6 ++++--
>   target/hppa/int_helper.c |  6 +++---
>   target/hppa/machine.c    |  5 +++--
>   target/hppa/sys_helper.c |  2 +-
>   target/hppa/translate.c  | 24 +++++++++++++++++-------
>   9 files changed, 53 insertions(+), 26 deletions(-)

This does too much at once.

Adding the dr registers should be separate from any of the space hashing.  Translator 
changes can be separate from everything else.  Etc.

> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 0dd1908214..d5de793b62 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -662,6 +662,11 @@ static void hppa_machine_reset(MachineState *ms, ResetType type)
>           cpu_set_pc(cs, firmware_entry);
>           cpu[i]->env.psw = PSW_Q;
>           cpu[i]->env.gr[5] = CPU_HPA + i * 0x1000;
> +
> +        /* 64-bit machines start with space-register hashing enabled in %dr2 */
> +        if (hppa_is_pa20(&cpu[0]->env)) {
> +            cpu[i]->env.dr[2] = HPPA64_DIAG_SPHASH_ENABLE; /* (bit 54) */
> +        }
>       }

Why in hw/?  I would expect this in hppa_cpu_reset_hold.

> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
> index beea42d105..64e60a3980 100644
> --- a/target/hppa/cpu.h
> +++ b/target/hppa/cpu.h
> @@ -25,6 +25,7 @@
>   #include "qemu/cpu-float.h"
>   #include "qemu/interval-tree.h"
>   #include "hw/registerfields.h"
> +#include "hw/hppa/hppa_hardware.h"

Ideally this would never be in cpu.h.

> @@ -319,27 +321,33 @@ void hppa_translate_code(CPUState *cs, TranslationBlock *tb,
>   
>   #define CPU_RESOLVING_TYPE TYPE_HPPA_CPU
>   
> -static inline uint64_t gva_offset_mask(target_ulong psw)
> +static inline uint64_t gva_offset_mask(CPUHPPAState *env, target_ulong psw)
>   {
> -    return (psw & PSW_W
> -            ? MAKE_64BIT_MASK(0, 62)
> -            : MAKE_64BIT_MASK(0, 32));
> +    if (psw & PSW_W) {
> +        return (env->dr[2] & HPPA64_DIAG_SPHASH_ENABLE)
> +            ? MAKE_64BIT_MASK(0, 62) &
> +                ~((uint64_t)HPPA64_PDC_CACHE_RET_SPID_VAL << 48)
> +            : MAKE_64BIT_MASK(0, 62);
> +    } else {
> +        return MAKE_64BIT_MASK(0, 32);
> +    }
>   }

This is getting pretty complicated now.  I think perhaps we should cache the mask based on 
current cpu state (updated with writes to psw or dr2).

This would also move the HPPA64_DIAG_SPHASH_ENABLE reference out of cpu.h.

>   
> -static inline target_ulong hppa_form_gva_psw(target_ulong psw, uint64_t spc,
> -                                             target_ulong off)
> +static inline target_ulong hppa_form_gva_psw(CPUHPPAState *env,
> +                                             target_ulong psw,
> +                                             uint64_t spc, target_ulong off)

Which means this would take the cached mask as argument, not env + psw.
Might rename as hppa_form_gva_mask() at the same time to emphasize the
change in arguments.

> diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
> index 71074a64c1..4eaac750ea 100644
> --- a/target/hppa/insns.decode
> +++ b/target/hppa/insns.decode
> @@ -644,10 +644,12 @@ xmpyu           001110 ..... ..... 010 .0111 .00 t:5    r1=%ra64 r2=%rb64
>       # For 32-bit PA-7300LC (PCX-L2)
>       diag_getshadowregs_pa1  000101 00 0000 0000 0001 1010 0000 0000
>       diag_putshadowregs_pa1  000101 00 0000 0000 0001 1010 0100 0000
> +    diag_mfdiag             000101 dr:5  rt:5   0000 0110 0000 0000
> +    diag_mtdiag             000101 dr:5  r1:5   0001 0110 0000 0000
>   
>       # For 64-bit PA8700 (PCX-W2)
> -    diag_getshadowregs_pa2  000101 00 0111 1000 0001 1000 0100 0000
> -    diag_putshadowregs_pa2  000101 00 0111 0000 0001 1000 0100 0000
> +    diag_mfdiag             000101 dr:5  0 0000 0000 1000 101  rt:5
> +    diag_mtdiag             000101 dr:5  r1:5   0001 1000 0100 0000

Do we not want to distinguish the two different diag instructions?
Did you really want to remove get/put_shadowregs_pa2?

> diff --git a/target/hppa/int_helper.c b/target/hppa/int_helper.c
> index 58695def82..5aefb3ade4 100644
> --- a/target/hppa/int_helper.c
> +++ b/target/hppa/int_helper.c
> @@ -112,9 +112,9 @@ void hppa_cpu_do_interrupt(CPUState *cs)
>        */
>       if (old_psw & PSW_C) {
>           env->cr[CR_IIASQ] =
> -            hppa_form_gva_psw(old_psw, env->iasq_f, env->iaoq_f) >> 32;
> +            hppa_form_gva_psw(env, old_psw, env->iasq_f, env->iaoq_f) >> 32;
>           env->cr_back[0] =
> -            hppa_form_gva_psw(old_psw, env->iasq_b, env->iaoq_b) >> 32;
> +            hppa_form_gva_psw(env, old_psw, env->iasq_b, env->iaoq_b) >> 32;
>       } else {
>           env->cr[CR_IIASQ] = 0;
>           env->cr_back[0] = 0;
> @@ -165,7 +165,7 @@ void hppa_cpu_do_interrupt(CPUState *cs)
>                   if (old_psw & PSW_C) {
>                       int prot, t;
>   
> -                    vaddr = hppa_form_gva_psw(old_psw, env->iasq_f, vaddr);
> +                    vaddr = hppa_form_gva_psw(env, old_psw, env->iasq_f, vaddr);
>                       t = hppa_get_physical_address(env, vaddr, MMU_KERNEL_IDX,
>                                                     0, 0, &paddr, &prot);
>                       if (t >= 0) {

Here we'd need to cache the old mask, since PSW_W is changed in step 2, which would 
compute a new mask.


r~
Re: [PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX
Posted by Helge Deller 1 year ago
On 1/28/25 19:19, Richard Henderson wrote:
> On 1/28/25 08:14, deller@kernel.org wrote:
>> From: Helge Deller <deller@gmx.de>
>>
>> PA-RISC CPUs have diagnose registers (%dr). Those are mostly
>> undocumented and control cache behaviour, memory behaviour, reset button
>> management and many other related internal CPU things.
>>
>> The Linux kernel turns space-register hashing off unconditionally at
>> bootup.  That code was provided by HP at the beginning of the PA-RISC
>> Linux porting effort, and I don't know why it was decided then why Linux
>> should not use space register hashing.
>> 32-bit HP-UX versions seem to not use space register hashing either.
>> But for 64-bit HP-UX versions, Sven Schnelle noticed that space register
>> hashing needs to be enabled and is required, otherwise the HP-UX kernel
>> will crash badly.
>>
>> On 64-bit CPUs space register hashing is controlled by a bit in diagnose
>> register %dr2.  Since we want to support Linux and 32- and 64-bit HP-UX,
>> we need to fully emulate the diagnose registers and handle specifically
>> the content of %dr2.
>>
>> Furthermore it turned out that commit 3bdf20819e68 seems wrong and
>> conflicts with the general space register diagnose register, so it is
>> partly reverted here.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Suggested-by: Sven Schnelle <svens@stackframe.org>
>> Fixes: 3bdf20819e68 ("target/hppa: Add diag instructions to set/restore shadow registers")
>> ---
>>   hw/hppa/machine.c        |  5 +++++
>>   target/hppa/cpu.c        |  3 ++-
>>   target/hppa/cpu.h        | 24 ++++++++++++++++--------
>>   target/hppa/helper.c     |  4 ++--
>>   target/hppa/insns.decode |  6 ++++--
>>   target/hppa/int_helper.c |  6 +++---
>>   target/hppa/machine.c    |  5 +++--
>>   target/hppa/sys_helper.c |  2 +-
>>   target/hppa/translate.c  | 24 +++++++++++++++++-------
>>   9 files changed, 53 insertions(+), 26 deletions(-)
> 
> This does too much at once.
> 
> Adding the dr registers should be separate from any of the space hashing.  Translator changes can be separate from everything else.  Etc.

Ok.


>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index 0dd1908214..d5de793b62 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -662,6 +662,11 @@ static void hppa_machine_reset(MachineState *ms, ResetType type)
>>           cpu_set_pc(cs, firmware_entry);
>>           cpu[i]->env.psw = PSW_Q;
>>           cpu[i]->env.gr[5] = CPU_HPA + i * 0x1000;
>> +
>> +        /* 64-bit machines start with space-register hashing enabled in %dr2 */
>> +        if (hppa_is_pa20(&cpu[0]->env)) {
>> +            cpu[i]->env.dr[2] = HPPA64_DIAG_SPHASH_ENABLE; /* (bit 54) */
>> +        }
>>       }
> 
> Why in hw/?  I would expect this in hppa_cpu_reset_hold.

Ok.
  
>> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
>> index beea42d105..64e60a3980 100644
>> --- a/target/hppa/cpu.h
>> +++ b/target/hppa/cpu.h
>> @@ -25,6 +25,7 @@
>>   #include "qemu/cpu-float.h"
>>   #include "qemu/interval-tree.h"
>>   #include "hw/registerfields.h"
>> +#include "hw/hppa/hppa_hardware.h"
> 
> Ideally this would never be in cpu.h.
> 
>> @@ -319,27 +321,33 @@ void hppa_translate_code(CPUState *cs, TranslationBlock *tb,
>>   #define CPU_RESOLVING_TYPE TYPE_HPPA_CPU
>> -static inline uint64_t gva_offset_mask(target_ulong psw)
>> +static inline uint64_t gva_offset_mask(CPUHPPAState *env, target_ulong psw)
>>   {
>> -    return (psw & PSW_W
>> -            ? MAKE_64BIT_MASK(0, 62)
>> -            : MAKE_64BIT_MASK(0, 32));
>> +    if (psw & PSW_W) {
>> +        return (env->dr[2] & HPPA64_DIAG_SPHASH_ENABLE)
>> +            ? MAKE_64BIT_MASK(0, 62) &
>> +                ~((uint64_t)HPPA64_PDC_CACHE_RET_SPID_VAL << 48)
>> +            : MAKE_64BIT_MASK(0, 62);
>> +    } else {
>> +        return MAKE_64BIT_MASK(0, 32);
>> +    }
>>   }
> 
> This is getting pretty complicated now.  I think perhaps we should cache the mask based on current cpu state (updated with writes to psw or dr2).

Good idea. Will try to rewrite.

> This would also move the HPPA64_DIAG_SPHASH_ENABLE reference out of cpu.h.
> 
>> -static inline target_ulong hppa_form_gva_psw(target_ulong psw, uint64_t spc,
>> -                                             target_ulong off)
>> +static inline target_ulong hppa_form_gva_psw(CPUHPPAState *env,
>> +                                             target_ulong psw,
>> +                                             uint64_t spc, target_ulong off)
> 
> Which means this would take the cached mask as argument, not env + psw.
> Might rename as hppa_form_gva_mask() at the same time to emphasize the
> change in arguments.
> 
>> diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
>> index 71074a64c1..4eaac750ea 100644
>> --- a/target/hppa/insns.decode
>> +++ b/target/hppa/insns.decode
>> @@ -644,10 +644,12 @@ xmpyu           001110 ..... ..... 010 .0111 .00 t:5    r1=%ra64 r2=%rb64
>>       # For 32-bit PA-7300LC (PCX-L2)
>>       diag_getshadowregs_pa1  000101 00 0000 0000 0001 1010 0000 0000
>>       diag_putshadowregs_pa1  000101 00 0000 0000 0001 1010 0100 0000
>> +    diag_mfdiag             000101 dr:5  rt:5   0000 0110 0000 0000
>> +    diag_mtdiag             000101 dr:5  r1:5   0001 0110 0000 0000
>>       # For 64-bit PA8700 (PCX-W2)
>> -    diag_getshadowregs_pa2  000101 00 0111 1000 0001 1000 0100 0000
>> -    diag_putshadowregs_pa2  000101 00 0111 0000 0001 1000 0100 0000
>> +    diag_mfdiag             000101 dr:5  0 0000 0000 1000 101  rt:5
>> +    diag_mtdiag             000101 dr:5  r1:5   0001 1000 0100 0000
> 
> Do we not want to distinguish the two different diag instructions?

I did not see a need, as currently they behave the same.
Additionally I did not found any other uses in Linux or HP-UX, so I'm
not sure if we would benefit about the differentiation in the future.
Maybe we will change it when we look at ODE again, but for now I think
it's ok.

> Did you really want to remove get/put_shadowregs_pa2?

Yes. I think the previous patch is wrong.


>> diff --git a/target/hppa/int_helper.c b/target/hppa/int_helper.c
>> index 58695def82..5aefb3ade4 100644
>> --- a/target/hppa/int_helper.c
>> +++ b/target/hppa/int_helper.c
>> @@ -112,9 +112,9 @@ void hppa_cpu_do_interrupt(CPUState *cs)
>>        */
>>       if (old_psw & PSW_C) {
>>           env->cr[CR_IIASQ] =
>> -            hppa_form_gva_psw(old_psw, env->iasq_f, env->iaoq_f) >> 32;
>> +            hppa_form_gva_psw(env, old_psw, env->iasq_f, env->iaoq_f) >> 32;
>>           env->cr_back[0] =
>> -            hppa_form_gva_psw(old_psw, env->iasq_b, env->iaoq_b) >> 32;
>> +            hppa_form_gva_psw(env, old_psw, env->iasq_b, env->iaoq_b) >> 32;
>>       } else {
>>           env->cr[CR_IIASQ] = 0;
>>           env->cr_back[0] = 0;
>> @@ -165,7 +165,7 @@ void hppa_cpu_do_interrupt(CPUState *cs)
>>                   if (old_psw & PSW_C) {
>>                       int prot, t;
>> -                    vaddr = hppa_form_gva_psw(old_psw, env->iasq_f, vaddr);
>> +                    vaddr = hppa_form_gva_psw(env, old_psw, env->iasq_f, vaddr);
>>                       t = hppa_get_physical_address(env, vaddr, MMU_KERNEL_IDX,
>>                                                     0, 0, &paddr, &prot);
>>                       if (t >= 0) {
> 
> Here we'd need to cache the old mask, since PSW_W is changed in step 2, which would compute a new mask.

Ok.

Helge