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

[Matheus K. Ferst]

On 19/08/2021 09:42, 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 Wed, 18 Aug 2021 at 12:11, <matheus.ferst@eldorado.org.br> wrote:
> > From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> >
> > As vector registers are stored in host endianness, we shouldn't swap its
> > 64-bit elements in user mode. Add a 16-byte case in
> > ppc_maybe_bswap_register to handle the reordering of elements in softmmu
> > and remove avr_need_swap which is now unused.
> >
> > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > ---
> >   target/ppc/gdbstub.c | 32 +++++++-------------------------
> >   1 file changed, 7 insertions(+), 25 deletions(-)
> >
> > diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> > index 09ff1328d4..011016edfa 100644
> > --- a/target/ppc/gdbstub.c
> > +++ b/target/ppc/gdbstub.c
> > @@ -101,6 +101,8 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t 
> > *mem_buf, int len)
> >           bswap32s((uint32_t *)mem_buf);
> >       } else if (len == 8) {
> >           bswap64s((uint64_t *)mem_buf);
> > +    } else if (len == 16) {
> > +        bswap128s((Int128 *)mem_buf);
>
> This cast looks dubious. Int128 is not necessarily a 128-bit value
> in host byte order: if !CONFIG_INT128 then an Int128 is defined as:
> struct Int128 {
>      uint64_t lo;
>      int64_t hi;
> };
> > with the low half always first.
>
> So you can't cast your mem_buf* into an (Int128*) and then treat it
> like an Int128, I'm afraid.

That's a good point. I think that it's not a problem in practice for 
this particular case because bswap128 will swap the struct members. Even 
if we get the high part in Int128.lo, it should give the correct result.

However, the code may be conceptually wrong. I'm probably breaking the 
intended API of int128.h by relying on how the struct is defined, 
instead of sticking to the provided int128_* methods. If someone 
reproduces this in another context, it'll likely give the wrong answer.

Maybe I should use int128_{make128,getlo,gethi} inside a #ifdef 
HOST_WORDS_BIGENDIAN. Not so brief, but probably less wrong.

> This also means that the "Int128 s128" field in the ppc_vsr_t union
> seems dubious -- how is that intended to work ?

I'll look further into that. There are currently two uses of this 
member, one is just zeroing it, and the other is implementing the 
vextu[bhw][lr]x instructions.

> Maybe we should fix this by making the 'struct Int128' field order
> depend on HOST_WORDS_BIGENDIAN...
>
> -- PMM

I can make this change if you prefer, but I think I should change 
ppc_maybe_bswap_register to use int128_* methods anyway.

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