qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX
Date: Tue, 28 Jan 2025 10:19:08 -0800
User-agent: Mozilla Thunderbird

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~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]