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

[BALATON Zoltan]

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 
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.

> +
> +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.)

Regards,
BALATON Zoltan

> +
> static void ppc_excp_debug_sw_tlb(CPUPPCState *env, int excp)
> {
>     const char *es;
> @@ -3104,7 +3124,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: