Re: [RFC PATCH] target/ppc: fix vector registers access in gdbstub for little-endian

[Peter Maydell]

On Thu, 12 Aug 2021 at 21:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/12/21 9:10 AM, matheus.ferst@eldorado.org.br wrote:
> >   static bool avr_need_swap(CPUPPCState *env)
> >   {
> > +    bool le;
> > +#if defined(CONFIG_USER_ONLY)
> > +    le = false;
> > +#else
> > +    le = msr_le;
> > +#endif
>
> It certainly doesn't seem like the right fix.
>
> My first guess was that MSR_LE wasn't being properly set up at cpu_reset for 
> user-only,
> but it's there.

This code is confusing because there are multiple possible swaps happening:

(1) gdb_get_regl() and friends assume they are passed a host-endian value
and will tswap to get a value of TARGET_WORDS_BIGENDIAN endianness.
(For the other direction, ldl_p() et al do target-endian accesses.)
(2) for ppc softmmu, TARGET_WORDS_BIGENDIAN is always true, and so
if the CPU is in LE mode then the ppc gdbstub code needs to swap
(ppc_maybe_bswap_register() does this)
(3) for ppc usermode, TARGET_WORDS_BIGENDIAN matches the actual binary's
ordering, so the gdb_get_regl() etc swap is always correct and sufficient
and ppc_maybe_bswap_register() does nothing
(4) the data affected by this avr_need_swap() function is the 128
bit registers, and it has to do with whether we consider the two
64-bit halves as (high, low) or (low, high). (The swapping or not
of each 64-bit half is done with the same steps 1 2 3 above as for a
64-bit value.)

I haven't yet worked through the 128 bit case -- I'd need to look at
whether we store the AVR data in the CPU struct as a pair of uint64
host-order values (Arm does this, it's always index 0 is lo, 1 is hi
regardless of host endianness) or really as a host-order 128 bit integer.
But I think the code is pretty confusing, and to make it a bit less
so it would be useful to:
 * unify the "do we need to do an extra swap" logic that is currently
split between avr_need_swap() and ppc_maybe_bswap_register() (assuming
that the answer is really the same for both cases, of course...)
 * look at whether there is a nicer way to handle the 128 bit
register case than "byteswap the two 64-bit halves and then decide
which order to use them in"
 * write a good explanatory comment...

-- PMM