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

[Matheus K. Ferst]

On 13/08/2021 06:17, Peter Maydell wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa 
> confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail 
> suspeito entre imediatamente em contato com o DTI.
>
> 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.)

Thanks for this explanation, I think I can better understand the problem 
now.

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

I believe it's the latter. Looking at vsr64_offset in target/ppc/cpu.h, 
VsrD macro is used to determine the index of the high element, and the 
definition of this macro depends on HOST_WORDS_BIGENDIAN.

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

I think we can remove avr_need_swap and handle everything in 
ppc_maybe_bswap_register. I'll provide another patch with this approach.

>   * 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"

We could use bswap128 from int128.h and something else to handle the 
!CONFIG_INT128 case.

>   * write a good explanatory comment...
>
> -- PMM

IIUC, usermode doesn't need any swap, and system does. What puzzles me 
is that the original commit (ea499e71506) mentions that the 64-bit 
elements need to be reordered "for both system and user mode". But that 
was in 2016, so maybe things have changed since then.

--
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>