Re: [PATCH] target/sparc: Handle FPRS correctly on big-endian hosts

[Peter Maydell]

On Fri, 14 Jul 2023 at 18:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 14/7/23 19:26, Peter Maydell wrote:
> > In CPUSparcState we define the fprs field as uint64_t.  However we
> > then refer to it in translate.c via a TCGv_i32 which we set up with
> > tcg_global_mem_new_ptr().  This means that on a big-endian host when
> > the guest does something to writo te the FPRS register this value
> > ends up in the wrong half of the uint64_t, and the QEMU C code that
> > refers to env->fprs sees the wrong value.  The effect of this is that
> > guest code that enables the FPU crashes with spurious FPU Disabled
> > exceptions.  In particular, this is why
> >   tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
> > times out on an s390 host.
> >
> > There are multiple ways we could fix this; since there are actually
> > only three bits in the FPRS register and the code in translate.c
> > would be a bit painful to convert to dealing with a TCGv_i64, change
> > the type of the CPU state struct field to match what translate.c is
> > expecting.
> >
> > (None of the other fields referenced by the r32[] array in
> > sparc_tcg_init() have the wrong type.)
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Another in my occasional series of "fix an avocado failure on
> > s390" Friday afternoon patches :-)
>
> :)
>
> > diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
> > index a1c8fdc4d55..bddb9609b7b 100644
> > --- a/target/sparc/gdbstub.c
> > +++ b/target/sparc/gdbstub.c
> > @@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray 
> > *mem_buf, int n)
> >       case 83:
> >           return gdb_get_regl(mem_buf, env->fsr);
> >       case 84:
> > -        return gdb_get_regl(mem_buf, env->fprs);
> > +    {
> > +        target_ulong fprs = env->fprs;
> > +        return gdb_get_regl(mem_buf, fprs);
>
> Why not return gdb_get_reg32() ?

Because that would cause different on-the-wire data to be
sent to gdb -- gdb_get_reg32() puts 4 bytes of data into
the gdb remote protocol packet, whereas gdb_get_regl() puts
either 4 or 8 bytes depending on TARGET_LONG_BITS (as
it happens, here we'll always send 8 because this register
is sparc64- specific).

Anyway, Richard is correct and we don't need to change this
at all, because gdb_get_regl() takes an integer argument,
it isn't a magic macro that implicitly takes the address
or looks at the type of what it gets passed. So passing
it env->fprs will zero-extend that and DTRT.

thanks
-- PMM