Message ID | 20210812191007.70331-1-matheus.ferst@eldorado.org.br |
---|---|
State | New |
Headers | show |
Series | [RFC] target/ppc: fix vector registers access in gdbstub for little-endian | expand |
On 8/12/21 9:10 AM, matheus.ferst@eldorado.org.br wrote: > static bool avr_need_swap(CPUPPCState *env) > { > + bool le; > +#if defined(CONFIG_USER_ONLY) > + le = false; > +#else > + le = msr_le; > +#endif It certainly doesn't seem like the right fix. My first guess was that MSR_LE wasn't being properly set up at cpu_reset for user-only, but it's there. r~
On Thu, 12 Aug 2021 at 21:07, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 8/12/21 9:10 AM, matheus.ferst@eldorado.org.br wrote: > > static bool avr_need_swap(CPUPPCState *env) > > { > > + bool le; > > +#if defined(CONFIG_USER_ONLY) > > + le = false; > > +#else > > + le = msr_le; > > +#endif > > It certainly doesn't seem like the right fix. > > My first guess was that MSR_LE wasn't being properly set up at cpu_reset for user-only, > but it's there. This code is confusing because there are multiple possible swaps happening: (1) gdb_get_regl() and friends assume they are passed a host-endian value and will tswap to get a value of TARGET_WORDS_BIGENDIAN endianness. (For the other direction, ldl_p() et al do target-endian accesses.) (2) for ppc softmmu, TARGET_WORDS_BIGENDIAN is always true, and so if the CPU is in LE mode then the ppc gdbstub code needs to swap (ppc_maybe_bswap_register() does this) (3) for ppc usermode, TARGET_WORDS_BIGENDIAN matches the actual binary's ordering, so the gdb_get_regl() etc swap is always correct and sufficient and ppc_maybe_bswap_register() does nothing (4) the data affected by this avr_need_swap() function is the 128 bit registers, and it has to do with whether we consider the two 64-bit halves as (high, low) or (low, high). (The swapping or not of each 64-bit half is done with the same steps 1 2 3 above as for a 64-bit value.) I haven't yet worked through the 128 bit case -- I'd need to look at whether we store the AVR data in the CPU struct as a pair of uint64 host-order values (Arm does this, it's always index 0 is lo, 1 is hi regardless of host endianness) or really as a host-order 128 bit integer. But I think the code is pretty confusing, and to make it a bit less so it would be useful to: * unify the "do we need to do an extra swap" logic that is currently split between avr_need_swap() and ppc_maybe_bswap_register() (assuming that the answer is really the same for both cases, of course...) * look at whether there is a nicer way to handle the 128 bit register case than "byteswap the two 64-bit halves and then decide which order to use them in" * write a good explanatory comment... -- PMM
On 13/08/2021 06:17, 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 Thu, 12 Aug 2021 at 21:07, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 8/12/21 9:10 AM, matheus.ferst@eldorado.org.br wrote: >>> static bool avr_need_swap(CPUPPCState *env) >>> { >>> + bool le; >>> +#if defined(CONFIG_USER_ONLY) >>> + le = false; >>> +#else >>> + le = msr_le; >>> +#endif >> >> It certainly doesn't seem like the right fix. >> >> My first guess was that MSR_LE wasn't being properly set up at cpu_reset for user-only, >> but it's there. > > This code is confusing because there are multiple possible swaps happening: > > (1) gdb_get_regl() and friends assume they are passed a host-endian value > and will tswap to get a value of TARGET_WORDS_BIGENDIAN endianness. > (For the other direction, ldl_p() et al do target-endian accesses.) > (2) for ppc softmmu, TARGET_WORDS_BIGENDIAN is always true, and so > if the CPU is in LE mode then the ppc gdbstub code needs to swap > (ppc_maybe_bswap_register() does this) > (3) for ppc usermode, TARGET_WORDS_BIGENDIAN matches the actual binary's > ordering, so the gdb_get_regl() etc swap is always correct and sufficient > and ppc_maybe_bswap_register() does nothing > (4) the data affected by this avr_need_swap() function is the 128 > bit registers, and it has to do with whether we consider the two > 64-bit halves as (high, low) or (low, high). (The swapping or not > of each 64-bit half is done with the same steps 1 2 3 above as for a > 64-bit value.) > Thanks for this explanation, I think I can better understand the problem now. > I haven't yet worked through the 128 bit case -- I'd need to look at > whether we store the AVR data in the CPU struct as a pair of uint64 > host-order values (Arm does this, it's always index 0 is lo, 1 is hi > regardless of host endianness) or really as a host-order 128 bit integer. I believe it's the latter. Looking at vsr64_offset in target/ppc/cpu.h, VsrD macro is used to determine the index of the high element, and the definition of this macro depends on HOST_WORDS_BIGENDIAN. > But I think the code is pretty confusing, and to make it a bit less > so it would be useful to: > * unify the "do we need to do an extra swap" logic that is currently > split between avr_need_swap() and ppc_maybe_bswap_register() (assuming > that the answer is really the same for both cases, of course...) I think we can remove avr_need_swap and handle everything in ppc_maybe_bswap_register. I'll provide another patch with this approach. > * look at whether there is a nicer way to handle the 128 bit > register case than "byteswap the two 64-bit halves and then decide > which order to use them in" We could use bswap128 from int128.h and something else to handle the !CONFIG_INT128 case. > * write a good explanatory comment... > > -- PMM > IIUC, usermode doesn't need any swap, and system does. What puzzles me is that the original commit (ea499e71506) mentions that the 64-bit elements need to be reordered "for both system and user mode". But that was in 2016, so maybe things have changed since then.
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c index 09ff1328d4..779e4a4e3f 100644 --- a/target/ppc/gdbstub.c +++ b/target/ppc/gdbstub.c @@ -391,10 +391,16 @@ const char *ppc_gdb_get_dynamic_xml(CPUState *cs, const char *xml_name) static bool avr_need_swap(CPUPPCState *env) { + bool le; +#if defined(CONFIG_USER_ONLY) + le = false; +#else + le = msr_le; +#endif #ifdef HOST_WORDS_BIGENDIAN - return msr_le; + return le; #else - return !msr_le; + return !le; #endif }