| Message ID | 20210818110656.1993090-3-matheus.ferst@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Fix vector registers access in gdbstub for little-endian | expand |
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
> 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>
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
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.
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~
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) {