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

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

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

PPC gdbstub code has two possible swaps of the 64-bit elements of AVR
registers: in gdb_get_avr_reg/gdb_set_avr_reg (based on msr_le) and in
gdb_get_reg128/ldq_p (based on TARGET_WORDS_BIGENDIAN).

In softmmu, only the first is done, because TARGET_WORDS_BIGENDIAN is
always true. In user mode, both are being done, resulting in swapped
high and low doublewords of AVR registers in little-endian binaries.

We fix this by moving the first swap to ppc_maybe_bswap_register, which
already handles the endianness swap of each element's value in softmmu
and does nothing in user mode.

Matheus Ferst (2):
  include/qemu/int128.h: introduce bswap128s
  target/ppc: fix vector registers access in gdbstub for little-endian

 include/qemu/int128.h | 15 +++++++++++++++
 target/ppc/gdbstub.c  | 32 +++++++-------------------------
 2 files changed, 22 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH for-6.2 v2 1/2] include/qemu/int128.h: introduce bswap128s

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Changes the current bswap128 implementation to use __builtin_bswap128
when available, adds a bswap128 implementation for !CONFIG_INT128
builds, and introduces bswap128s based on bswap128.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 include/qemu/int128.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 64500385e3..8d6ee5203f 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -155,7 +155,11 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
 
 static inline Int128 bswap128(Int128 a)
 {
+#if __has_builtin(__builtin_bswap128)
+    return __builtin_bswap128(a);
+#else
     return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
+#endif
 }
 
 #else /* !CONFIG_INT128 */
@@ -337,5 +341,16 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
     *a = int128_sub(*a, b);
 }
 
+static inline Int128 bswap128(Int128 a)
+{
+    return int128_make128(bswap64(a.hi), bswap64(a.lo));
+}
+
 #endif /* CONFIG_INT128 */
+
+static inline void bswap128s(Int128 *s)
+{
+    *s = bswap128(*s);
+}
+
 #endif /* INT128_H */
-- 
2.25.1

Re: [PATCH for-6.2 v2 1/2] include/qemu/int128.h: introduce bswap128s

[Philippe Mathieu-Daudé]

On 8/18/21 1:06 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Changes the current bswap128 implementation to use __builtin_bswap128
> when available, adds a bswap128 implementation for !CONFIG_INT128
> builds, and introduces bswap128s based on bswap128.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>  include/qemu/int128.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-6.2 v2 1/2] include/qemu/int128.h: introduce bswap128s

[Peter Maydell]

On Wed, 18 Aug 2021 at 12:11, <matheus.ferst@eldorado.org.br> wrote:
>
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>
> Changes the current bswap128 implementation to use __builtin_bswap128
> when available, adds a bswap128 implementation for !CONFIG_INT128
> builds, and introduces bswap128s based on bswap128.
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[matheus.ferst]

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);
     } else {
         g_assert_not_reached();
     }
@@ -389,15 +391,6 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name)
 }
 #endif
 
-static bool avr_need_swap(CPUPPCState *env)
-{
-#ifdef HOST_WORDS_BIGENDIAN
-    return msr_le;
-#else
-    return !msr_le;
-#endif
-}
-
 #if !defined(CONFIG_USER_ONLY)
 static int gdb_find_spr_idx(CPUPPCState *env, int n)
 {
@@ -486,14 +479,9 @@ static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
 
     if (n < 32) {
         ppc_avr_t *avr = cpu_avr_ptr(env, n);
-        if (!avr_need_swap(env)) {
-            gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
-        } else {
-            gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
-        }
+        gdb_get_reg128(buf, avr->VsrD(0) , avr->VsrD(1));
         mem_buf = gdb_get_reg_ptr(buf, 16);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
+        ppc_maybe_bswap_register(env, mem_buf, 16);
         return 16;
     }
     if (n == 32) {
@@ -515,15 +503,9 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
 {
     if (n < 32) {
         ppc_avr_t *avr = cpu_avr_ptr(env, n);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf + 8, 8);
-        if (!avr_need_swap(env)) {
-            avr->u64[0] = ldq_p(mem_buf);
-            avr->u64[1] = ldq_p(mem_buf + 8);
-        } else {
-            avr->u64[1] = ldq_p(mem_buf);
-            avr->u64[0] = ldq_p(mem_buf + 8);
-        }
+        ppc_maybe_bswap_register(env, mem_buf, 16);
+        avr->VsrD(0) = ldq_p(mem_buf);
+        avr->VsrD(1) = ldq_p(mem_buf + 8);
         return 16;
     }
     if (n == 32) {
-- 
2.25.1

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

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

[Luis Fernando Fujita Pires]

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

+1 for that.

--
Luis Pires
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

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>

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

[Richard Henderson]

On 8/19/21 2:42 AM, Peter Maydell wrote:
> Maybe we should fix this by making the 'struct Int128' field order
> depend on HOST_WORDS_BIGENDIAN...

Yes, I think so.

At some point I had a notion of supporting Int128 natively in TCG, at least as far as data 
movement and the host function call abi.


r~

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(-)
> @@ -486,14 +479,9 @@ static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
>
>      if (n < 32) {
>          ppc_avr_t *avr = cpu_avr_ptr(env, n);
> -        if (!avr_need_swap(env)) {
> -            gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
> -        } else {
> -            gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
> -        }
> +        gdb_get_reg128(buf, avr->VsrD(0) , avr->VsrD(1));

Stray space before comma.

Otherwise if we first fix up the Int128 field order then:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM