Re: [PATCH 1/4] target/ppc: Fix instruction loading endianness in alignment interrupt

[Nicholas Piggin]

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 <farosas@suse.de>
> > Signed-off-by: Nicholas Piggin <npiggin@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