| Message ID | 20230323022237.1807512-3-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] target/ppc: Fix width of some 32-bit SPRs | expand |
Hi Nick, > powerpc ifetch endianness depends on MSR[LE] so it has to byteswap > after cpu_ldl_code(). This corrects DSISR bits in alignment > interrupts when running in little endian mode. > Just a thought, we have these tests that perhaps could have caught this: https://github.com/legoater/pnv-test Despite the name they do have (some) support to pseries as well. Not sure how the P8 support is these days though. > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 287659c74d..5f0e363363 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env) > env->nip); > } > > +/* Return true iff byteswap is needed in a scalar memop */ > +static inline bool need_byteswap(CPUArchState *env) > +{ > +#if TARGET_BIG_ENDIAN TARGET_BIG_ENDIAN is always set for softmmu mode. See configs/targets/ppc64-softmmu.mak > + return !!(env->msr & ((target_ulong)1 << MSR_LE)); > +#else > + return !(env->msr & ((target_ulong)1 << MSR_LE)); > +#endif > +} > + > +static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr) > +{ > + uint32_t insn = cpu_ldl_code(env, addr); > +#if TARGET_BIG_ENDIAN > + if (env->msr & ((target_ulong)1 << MSR_LE)) { > + insn = bswap32(insn); > + } > +#else > + if (!(env->msr & ((target_ulong)1 << MSR_LE))) { > + insn = bswap32(insn); > + } > +#endif > + return insn; > +} > + > static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp) > { > const char *es; > @@ -3097,7 +3122,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, > > /* Restore state and reload the insn we executed, for filling in DSISR. */ > cpu_restore_state(cs, retaddr); > - insn = cpu_ldl_code(env, env->nip); > + insn = ppc_ldl_code(env, env->nip); > > switch (env->mmu_model) { > case POWERPC_MMU_SOFT_4xx:
On Fri Mar 24, 2023 at 11:30 PM AEST, Fabiano Rosas wrote: > Hi Nick, > > > powerpc ifetch endianness depends on MSR[LE] so it has to byteswap > > after cpu_ldl_code(). This corrects DSISR bits in alignment > > interrupts when running in little endian mode. > > > > Just a thought, we have these tests that perhaps could have caught > this: https://github.com/legoater/pnv-test > > Despite the name they do have (some) support to pseries as well. Not > sure how the P8 support is these days though. Hey Fabiano, Thanks for the review. Good thinking, and actually I did catch some of these (the SPR size one) when running kvm-unit-tests with TCG. I ported it to powernv too. I wonder if we should merge pnv-test into kvm-unit-tests. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++- > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > > index 287659c74d..5f0e363363 100644 > > --- a/target/ppc/excp_helper.c > > +++ b/target/ppc/excp_helper.c > > @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env) > > env->nip); > > } > > > > +/* Return true iff byteswap is needed in a scalar memop */ > > +static inline bool need_byteswap(CPUArchState *env) > > +{ > > +#if TARGET_BIG_ENDIAN > > TARGET_BIG_ENDIAN is always set for softmmu mode. See > configs/targets/ppc64-softmmu.mak I see, I just took it from translate.c and actually stupidly forgot to actually call it here :) I'll fix it up. Thanks, Nick
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 287659c74d..5f0e363363 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -133,6 +133,31 @@ static void dump_hcall(CPUPPCState *env) env->nip); } +/* Return true iff byteswap is needed in a scalar memop */ +static inline bool need_byteswap(CPUArchState *env) +{ +#if TARGET_BIG_ENDIAN + return !!(env->msr & ((target_ulong)1 << MSR_LE)); +#else + return !(env->msr & ((target_ulong)1 << MSR_LE)); +#endif +} + +static uint32_t ppc_ldl_code(CPUArchState *env, abi_ptr addr) +{ + uint32_t insn = cpu_ldl_code(env, addr); +#if TARGET_BIG_ENDIAN + if (env->msr & ((target_ulong)1 << MSR_LE)) { + insn = bswap32(insn); + } +#else + if (!(env->msr & ((target_ulong)1 << MSR_LE))) { + insn = bswap32(insn); + } +#endif + return insn; +} + static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp) { const char *es; @@ -3097,7 +3122,7 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, /* Restore state and reload the insn we executed, for filling in DSISR. */ cpu_restore_state(cs, retaddr); - insn = cpu_ldl_code(env, env->nip); + insn = ppc_ldl_code(env, env->nip); switch (env->mmu_model) { case POWERPC_MMU_SOFT_4xx:
powerpc ifetch endianness depends on MSR[LE] so it has to byteswap after cpu_ldl_code(). This corrects DSISR bits in alignment interrupts when running in little endian mode. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- target/ppc/excp_helper.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)