On Wed Jun 21, 2023 at 12:26 AM AEST, BALATON Zoltan wrote: > On Tue, 20 Jun 2023, Nicholas Piggin wrote: > > 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. > > > > Reviewed-by: Fabiano Rosas <faro...@suse.de> > > Signed-off-by: Nicholas Piggin <npig...@gmail.com> > > --- > > target/ppc/excp_helper.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > > index 12d8a7257b..a2801f6e6b 100644 > > --- a/target/ppc/excp_helper.c > > +++ b/target/ppc/excp_helper.c > > @@ -133,6 +133,26 @@ static void dump_hcall(CPUPPCState *env) > > env->nip); > > } > > > > +#ifdef CONFIG_TCG > > +/* Return true iff byteswap is needed to load instruction */ > > +static inline bool insn_need_byteswap(CPUArchState *env) > > +{ > > + /* SYSTEM builds TARGET_BIG_ENDIAN. Need to swap when MSR[LE] is set */ > > + return !!(env->msr & ((target_ulong)1 << MSR_LE)); > > +} > > Don't other places typically use FIELD_EX64 to test for msr bits now? If
Yeah I should use that, good point. There's at least another case in that file that doesn't use it but I probably added that too :/ > this really only tests for the LE bit and used only once do we need a new > function for that? I don't quite like trivial one line functions unless it > does something more complex Because if just makes code harder to read as I > have to look up what these do when I could just see it right away where it > used without these functions. It's based on mem_helper.c, which is familiar pattern/name so I might keep it. Maybe not, I'll check. I'm on the fence. > > + > > +static uint32_t ppc_ldl_code(CPUArchState *env, hwaddr addr) > > +{ > > + uint32_t insn = cpu_ldl_code(env, addr); > > + > > + if (insn_need_byteswap(env)) { > > + insn = bswap32(insn); > > + } > > + > > + return insn; > > +} > > +#endif > > Along the same lines I'm not sure this wrapper is needed unless this is a > recurring operation. Otherwise you could just add the if and the comment > below at the single place where this is needed. If this will be needed at > more places later then adding a function may make sense but otherwise I'd > avoid making code tangled with single line functions defined away from > where they are used as it's simpler to just have the if and swap at the > single place where it's needed than adding two new functions that I'd had > to look up and comprehend first to see what's happening. (It also would be > just 3 lines instead of 20 that way.) It does get used in a couple more places later. Few-line "abstraction" used once isn't necessarily wrong though. Thanks, Nick