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

[PATCH for-6.2 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 | 16 +++++++++++-----
 target/ppc/gdbstub.c  | 32 +++++++-------------------------
 2 files changed, 18 insertions(+), 30 deletions(-)

-- 
2.25.1

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

[matheus.ferst]

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

Introduces bswap128s based on bswap128. Since bswap128 is defined using
int128_* methods available in either CONFIG_INT128 or !CONFIG_INT128
builds, place both outside of #ifdef CONFIG_INT128.

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

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 64500385e3..e0d385628c 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -153,11 +153,6 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
     *a -= b;
 }
 
-static inline Int128 bswap128(Int128 a)
-{
-    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
-}
-
 #else /* !CONFIG_INT128 */
 
 typedef struct Int128 Int128;
@@ -338,4 +333,15 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
 }
 
 #endif /* CONFIG_INT128 */
+
+static inline Int128 bswap128(Int128 a)
+{
+    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
+}
+
+static inline void bswap128s(Int128 *s)
+{
+    *s = bswap128(*s);
+}
+
 #endif /* INT128_H */
-- 
2.25.1

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

[Philippe Mathieu-Daudé]

On 8/16/21 9:13 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Introduces bswap128s based on bswap128. Since bswap128 is defined using
> int128_* methods available in either CONFIG_INT128 or !CONFIG_INT128
> builds, place both outside of #ifdef CONFIG_INT128.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>  include/qemu/int128.h | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index 64500385e3..e0d385628c 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -153,11 +153,6 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
>      *a -= b;
>  }
>  
> -static inline Int128 bswap128(Int128 a)
> -{
> -    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
> -}

Personally I'd move this one to the other #ifdef side,
and implement here with __builtin_bswap128().

>  #else /* !CONFIG_INT128 */
>  
>  typedef struct Int128 Int128;
> @@ -338,4 +333,15 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
>  }

> +static inline Int128 bswap128(Int128 a)
> +{
> +    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
> +}

   #endif /* CONFIG_INT128 */

And add this generic one here indeed:

> +static inline void bswap128s(Int128 *s)
> +{
> +    *s = bswap128(*s);
> +}
> +
>  #endif /* INT128_H */
> 

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

[Matheus K. Ferst]

On 17/08/2021 06:27, Philippe Mathieu-Daudé 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 8/16/21 9:13 PM, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> Introduces bswap128s based on bswap128. Since bswap128 is defined using
>> int128_* methods available in either CONFIG_INT128 or !CONFIG_INT128
>> builds, place both outside of #ifdef CONFIG_INT128.
>>
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>>   include/qemu/int128.h | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
>> index 64500385e3..e0d385628c 100644
>> --- a/include/qemu/int128.h
>> +++ b/include/qemu/int128.h
>> @@ -153,11 +153,6 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
>>       *a -= b;
>>   }
>>
>> -static inline Int128 bswap128(Int128 a)
>> -{
>> -    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
>> -}
> 
> Personally I'd move this one to the other #ifdef side,
> and implement here with __builtin_bswap128().
> 

I saw this builtin, but I couldn't test it on my system. It seems that 
Clang doesn't implement it, and it's only available on GCC 11: 
https://godbolt.org/z/T6vhd5a38 . I think we can use it, but I'd need to 
figure how to add a test for it in the configure script.

>>   #else /* !CONFIG_INT128 */
>>
>>   typedef struct Int128 Int128;
>> @@ -338,4 +333,15 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
>>   }
> 
>> +static inline Int128 bswap128(Int128 a)
>> +{
>> +    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
>> +}
> 
>     #endif /* CONFIG_INT128 */
> 
> And add this generic one here indeed:
> 
>> +static inline void bswap128s(Int128 *s)
>> +{
>> +    *s = bswap128(*s);
>> +}
>> +
>>   #endif /* INT128_H */
>>
> 

-- 
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 1/2] include/qemu/int128.h: introduce bswap128s

[Peter Maydell]

On Tue, 17 Aug 2021 at 13:09, Matheus K. Ferst
<matheus.ferst@eldorado.org.br> wrote:
>
> On 17/08/2021 06:27, Philippe Mathieu-Daudé wrote:
> > On 8/16/21 9:13 PM, matheus.ferst@eldorado.org.br wrote:
> >> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> >> -static inline Int128 bswap128(Int128 a)
> >> -{
> >> -    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
> >> -}
> >
> > Personally I'd move this one to the other #ifdef side,
> > and implement here with __builtin_bswap128().
> >
>
> I saw this builtin, but I couldn't test it on my system. It seems that
> Clang doesn't implement it, and it's only available on GCC 11:
> https://godbolt.org/z/T6vhd5a38 . I think we can use it, but I'd need to
> figure how to add a test for it in the configure script.

You should be able to get away without a configure script test --
#if __has_builtin(__builtin_bswap128)
/* version with the builtin here */
#else
/* fallback */
#endif

ought to work. (Any gcc new enough to have the builtin also has
__has_builtin; clang has had __has_builtin for ages; our compiler.h
defines a fallback "always 0" __has_builtin for older compilers.)

-- PMM

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

[Matheus K. Ferst]

On 17/08/2021 09:15, 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 Tue, 17 Aug 2021 at 13:09, Matheus K. Ferst
> <matheus.ferst@eldorado.org.br> wrote:
>>
>> On 17/08/2021 06:27, Philippe Mathieu-Daudé wrote:
>>> On 8/16/21 9:13 PM, matheus.ferst@eldorado.org.br wrote:
>>>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>>> -static inline Int128 bswap128(Int128 a)
>>>> -{
>>>> -    return int128_make128(bswap64(int128_gethi(a)), bswap64(int128_getlo(a)));
>>>> -}
>>>
>>> Personally I'd move this one to the other #ifdef side,
>>> and implement here with __builtin_bswap128().
>>>
>>
>> I saw this builtin, but I couldn't test it on my system. It seems that
>> Clang doesn't implement it, and it's only available on GCC 11:
>> https://godbolt.org/z/T6vhd5a38 . I think we can use it, but I'd need to
>> figure how to add a test for it in the configure script.
> 
> You should be able to get away without a configure script test --
> #if __has_builtin(__builtin_bswap128)
> /* version with the builtin here */
> #else
> /* fallback */
> #endif
> 
> ought to work. (Any gcc new enough to have the builtin also has
> __has_builtin; clang has had __has_builtin for ages; our compiler.h
> defines a fallback "always 0" __has_builtin for older compilers.)
> 
> -- PMM
> 

Nice, I'll prepare a v2.

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

[PATCH for-6.2 2/2] target/ppc: Don't swap 64-bit elements of AVR in gdbstub for user mode

[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 to call gdb_get_reg128. 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