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

It seems that access to elements of ppc_avr_t should only depend on
msr_le when !CONFIG_USER_ONLY.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
To reproduce the problem, build the following program for ppc64le:

int main(void)
{
    __uint128_t a = 0x1122334455667788llu;
    a <<= 64;
    a |= 0xAABBCCDDEEFFAABBllu;
    asm volatile ("xxlor 4, %x0, %x0\n\t"
                  "xxlor 34, %x0, %x0\n\t"
                  :: "wa" (a) : "vs4", "vs34");
    return 0;
}

Run it with qemu-ppc64le, attach gdb, place a breakpoint after the
second xxlor and print vs4 and vs34:
Breakpoint 1, 0x0000000010000d88 in main ()
(gdb) p/x $vs4
$1 = {uint128 = 0x1122334455667788aabbccddeeffaabb, v2_double = {0x0,
    0x0}, v4_float = {0x0, 0x0, 0x78800000, 0x0}, v4_int32 = {
    0xeeffaabb, 0xaabbccdd, 0x55667788, 0x11223344}, v8_int16 = {
    0xaabb, 0xeeff, 0xccdd, 0xaabb, 0x7788, 0x5566, 0x3344, 0x1122},
  v16_int8 = {0xbb, 0xaa, 0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa, 0x88,
    0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11}}
(gdb) p/x $vs34
$2 = {uint128 = 0xaabbccddeeffaabb1122334455667788, v2_double = {0x0,
    0x0}, v4_float = {0x78800000, 0x0, 0x0, 0x0}, v4_int32 = {
    0x55667788, 0x11223344, 0xeeffaabb, 0xaabbccdd}, v8_int16 = {
    0x7788, 0x5566, 0x3344, 0x1122, 0xaabb, 0xeeff, 0xccdd, 0xaabb},
  v16_int8 = {0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11, 0xbb,
    0xaa, 0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa}}

RFC because I'm not quite sure about the fix.
---
 target/ppc/gdbstub.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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

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.

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