| Message ID | 20230327131218.2721044-6-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/6] target/ppc: Fix width of some 32-bit SPRs | expand |
Nick, > + case POWERPC_EXCP_HV_EMU: > + env->spr[SPR_HEIR] = insn; > + if (is_prefix_excp(env, insn)) { > + uint32_t insn2 = ppc_ldl_code(env, env->nip + 4); > + env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32; Are inst and inst2 in the right locations here? I think you might need insn in the top half and insn2 in the bottom. I wrote the little test case below. I'd expect GPR0 and GPR1 to end up with the same value, but they don't with this code qemu correctly sees the bad prefix instruction as HSRR1[34] is set. Mikey % cat heir.S #define SPR_HEIR (0x153) #define SPR_HSRR0 (0x13a) start: . = 0x10 .long (1<<26) | 0 .long 0x0 . = 0xe40 illegal: mfspr 0, SPR_HEIR mfspr 2, SPR_HSRR0 ld 1, 0(2) b . % powerpc64-linux-gnu-gcc -o heir.o -c heir.S % powerpc64-linux-gnu-objcopy -O binary heir.o heir.stripped % qemu-system-ppc64 -nographic-machine powernv10 -cpu POWER10 -display none -vga none -m 1g -accel tcg -serial mon:stdio -bios /home/mikey/devel/test/heir.stripped QEMU 7.2.91 monitor - type 'help' for more information (qemu) info registers CPU#0 NIP 0000000000000e4c LR 0000000000000000 CTR 0000000000000000 XER 0000000000000000 CPU#0 MSR 9000000000000000 HID0 0000000000000000 HF fc000006 iidx 7 didx 7 TB 00000000 2494783394 DECR 1800184060 GPR00 0000000004000000 0400000000000000 0000000000000010 0000000001000000 GPR04 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR08 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR12 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR16 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR24 0000000000000000 0000000000000000 0000000000000000 0000000000000000 GPR28 0000000000000000 0000000000000000 0000000000000000 0000000000000000 CR 00000000 [ - - - - - - - - ] RES ffffffffffffffff FPR00 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR04 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR08 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR12 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR16 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR24 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPR28 0000000000000000 0000000000000000 0000000000000000 0000000000000000 FPSCR 0000000000000000 SRR0 0000000000000000 SRR1 0000000000000000 PVR 0000000000800200 VRSAVE 0000000000000000 SPRG0 0000000000000000 SPRG1 0000000000000000 SPRG2 0000000000000000 SPRG3 0000000000000000 SPRG4 0000000000000000 SPRG5 0000000000000000 SPRG6 0000000000000000 SPRG7 0000000000000000 HSRR0 0000000000000010 HSRR1 9000000020000000 CFAR 0000000000000e4c LPCR 000000000000000c PTCR 0000000000000000 DAR 0000000000000000 DSISR 0000000000000000 (qemu)
On Wed Mar 29, 2023 at 3:51 PM AEST, Michael Neuling wrote: > Nick, > > > + case POWERPC_EXCP_HV_EMU: > > + env->spr[SPR_HEIR] = insn; > > + if (is_prefix_excp(env, insn)) { > > + uint32_t insn2 = ppc_ldl_code(env, env->nip + 4); > > + env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32; > > Are inst and inst2 in the right locations here? I think you might need > insn in the top half and insn2 in the bottom. > > I wrote the little test case below. I'd expect GPR0 and GPR1 to end up > with the same value, but they don't with this code You're right. I was a bit confused becaue the prefix instructions are treated as two words, so ld (insn) in little endian doesn't match HEIR, for example. The ISA uses the term "image", but that's only really defined for 4 byte instructions AFAIKS. You can deduce though, There may be circumstances in which the suffix cannot be loaded [...] In such circumstances, bits 32:63 are set to 0s. So prefix word goes in the high bits. Real P10 agrees, so does systemsim. I'll fix and re-send. Is there any better semantics in the ISA or should I raise an issue to clarify instruction image for prefix? Thanks, Nick
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 557d736dab..8c4a203ecb 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1653,6 +1653,7 @@ void ppc_compat_add_property(Object *obj, const char *name, #define SPR_HMER (0x150) #define SPR_HMEER (0x151) #define SPR_PCR (0x152) +#define SPR_HEIR (0x153) #define SPR_BOOKE_LPIDR (0x152) #define SPR_BOOKE_TCR (0x154) #define SPR_BOOKE_TLB0PS (0x158) diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index 5aa0b3f0f1..ff73be1812 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -1629,6 +1629,7 @@ static void register_8xx_sprs(CPUPPCState *env) * HSRR0 => SPR 314 (Power 2.04 hypv) * HSRR1 => SPR 315 (Power 2.04 hypv) * LPIDR => SPR 317 (970) + * HEIR => SPR 339 (Power 2.05 hypv) (64-bit reg from 3.1) * EPR => SPR 702 (Power 2.04 emb) * perf => 768-783 (Power 2.04) * perf => 784-799 (Power 2.04) @@ -5522,6 +5523,24 @@ static void register_power6_common_sprs(CPUPPCState *env) 0x00000000); } +static void register_HEIR32_spr(CPUPPCState *env) +{ + spr_register_hv(env, SPR_HEIR, "HEIR", + SPR_NOACCESS, SPR_NOACCESS, + SPR_NOACCESS, SPR_NOACCESS, + &spr_read_generic, &spr_write_generic32, + 0x00000000); +} + +static void register_HEIR64_spr(CPUPPCState *env) +{ + spr_register_hv(env, SPR_HEIR, "HEIR", + SPR_NOACCESS, SPR_NOACCESS, + SPR_NOACCESS, SPR_NOACCESS, + &spr_read_generic, &spr_write_generic, + 0x00000000); +} + static void register_power8_tce_address_control_sprs(CPUPPCState *env) { spr_register_kvm(env, SPR_TAR, "TAR", @@ -5950,6 +5969,7 @@ static void init_proc_POWER7(CPUPPCState *env) register_power5p_ear_sprs(env); register_power5p_tb_sprs(env); register_power6_common_sprs(env); + register_HEIR32_spr(env); register_power6_dbg_sprs(env); register_power7_book4_sprs(env); @@ -6072,6 +6092,7 @@ static void init_proc_POWER8(CPUPPCState *env) register_power5p_ear_sprs(env); register_power5p_tb_sprs(env); register_power6_common_sprs(env); + register_HEIR32_spr(env); register_power6_dbg_sprs(env); register_power8_tce_address_control_sprs(env); register_power8_ids_sprs(env); @@ -6234,6 +6255,7 @@ static void init_proc_POWER9(CPUPPCState *env) register_power5p_ear_sprs(env); register_power5p_tb_sprs(env); register_power6_common_sprs(env); + register_HEIR32_spr(env); register_power6_dbg_sprs(env); register_power8_tce_address_control_sprs(env); register_power8_ids_sprs(env); @@ -6409,6 +6431,7 @@ static void init_proc_POWER10(CPUPPCState *env) register_power5p_ear_sprs(env); register_power5p_tb_sprs(env); register_power6_common_sprs(env); + register_HEIR64_spr(env); register_power6_dbg_sprs(env); register_power8_tce_address_control_sprs(env); register_power8_ids_sprs(env); diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 4e119c4dfc..84f222ba1d 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1596,13 +1596,23 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp) case POWERPC_EXCP_HDECR: /* Hypervisor decrementer exception */ case POWERPC_EXCP_HDSI: /* Hypervisor data storage exception */ case POWERPC_EXCP_SDOOR_HV: /* Hypervisor Doorbell interrupt */ - case POWERPC_EXCP_HV_EMU: case POWERPC_EXCP_HVIRT: /* Hypervisor virtualization */ srr0 = SPR_HSRR0; srr1 = SPR_HSRR1; new_msr |= (target_ulong)MSR_HVB; new_msr |= env->msr & ((target_ulong)1 << MSR_RI); break; + case POWERPC_EXCP_HV_EMU: + env->spr[SPR_HEIR] = insn; + if (is_prefix_excp(env, insn)) { + uint32_t insn2 = ppc_ldl_code(env, env->nip + 4); + env->spr[SPR_HEIR] |= (uint64_t)insn2 << 32; + } + srr0 = SPR_HSRR0; + srr1 = SPR_HSRR1; + new_msr |= (target_ulong)MSR_HVB; + new_msr |= env->msr & ((target_ulong)1 << MSR_RI); + break; case POWERPC_EXCP_VPU: /* Vector unavailable exception */ case POWERPC_EXCP_VSXU: /* VSX unavailable exception */ case POWERPC_EXCP_FU: /* Facility unavailable exception */
The hypervisor emulation assistance interrupt modifies HEIR to contain the value of the instruction which caused the exception. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- target/ppc/cpu.h | 1 + target/ppc/cpu_init.c | 23 +++++++++++++++++++++++ target/ppc/excp_helper.c | 12 +++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-)