|
| From: | Helge Deller |
| Subject: | Re: [PATCH 3/5] target/hppa: Implement CPU diagnose registers for 64-bit HP-UX |
| Date: | Tue, 28 Jan 2025 19:37:44 +0100 |
| User-agent: | Mozilla Thunderbird |
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 0000Do 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
| [Prev in Thread] | Current Thread | [Next in Thread] |