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

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

Posted by matheus.ferst@eldorado.org.br 4 years, 5 months ago

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

Posted by Peter Maydell 4 years, 5 months ago

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

Posted by Luis Fernando Fujita Pires 4 years, 5 months ago

> 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

Posted by Matheus K. Ferst 4 years, 5 months ago

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

Posted by Richard Henderson 4 years, 5 months ago

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

Posted by Peter Maydell 4 years, 5 months ago

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