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

[Peter Maydell]

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.

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

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

-- PMM