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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 16/7/23 19:32, Peter Maydell wrote:
> 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).

Right, I missed the TARGET_LONG_BITS part.

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

OK.