Re: [PATCH] target/loongarch: Split fcc register to fcc0-7 in gdbstub

["Alex Bennée" <alex.bennee@linaro.org>]

bibo mao <maobibo@loongson.cn> writes:

> I think that it is problem of loongarch gdb, rather qemu.
> If so, everytime when gdb changes register layout, qemu need modify.
> There should be compatible requirements between gdb client and gdb server.
>
> Tiezhu,
>
> what is your opition?

You can always register additional custom regsets which is what we do
for the extended Aarch64 regs. See ->gdb_get_dynamic_xml

>
> Regards
> Bibo Mao
>
> 在 2023/8/8 18:03, Jiajie Chen 写道:
>> 
>> On 2023/8/8 17:55, Jiajie Chen wrote:
>>>
>>> On 2023/8/8 14:10, bibo mao wrote:
>>>> I am not familiar with gdb, is there  abi breakage?
>>>> I do not know how gdb client works with gdb server with different versions.
>> There seemed no versioning in the process, but rather in-code xml
>> validation. In gdb, the code only allows new xml (fcc0-7) and
>> rejects old one (fcc), so gdb breaks qemu first and do not consider
>> backward compatibility with qemu.
>>>
>>> Not abi breakage, but gdb will complain:
>>>
>>> warning: while parsing target description (at line 1): Target
>>> description specified unknown architecture "loongarch64"
>>> warning: Could not load XML target description; ignoring
>>> warning: No executable has been specified and target does not support
>>> determining executable automatically.  Try using the "file" command.
>>> Truncated register 38 in remote 'g' packet
>> 
>> Sorry, to be clear, the actual error message is:
>> 
>> (gdb) target extended-remote localhost:1234
>> Remote debugging using localhost:1234
>> warning: Architecture rejected target-supplied description
>> warning: No executable has been specified and target does not support
>> 
>> It rejects the target description xml given by qemu, thus using the
>> builtin one. However, there is a mismatch in fcc registers, so it
>> will not work if we list floating point registers.
>> 
>> At the same time, if we are using loongarch32 target(I recently
>> posted patches to support this), it will reject the target
>> description and fallback to loongarch64, making gcc not usable.
>> 
>>>
>>> And gdb can no longer debug kernel running in qemu. You can
>>> reproduce this error using latest qemu(without this patch) and
>>> gdb(13.1 or later).
>>>
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>
>>>>
>>>> 在 2023/8/8 13:42, Jiajie Chen 写道:
>>>>> Since GDB 13.1(GDB commit ea3352172), GDB LoongArch changed to use
>>>>> fcc0-7 instead of fcc register. This commit partially reverts commit
>>>>> 2f149c759 (`target/loongarch: Update gdb_set_fpu() and gdb_get_fpu()`)
>>>>> to match the behavior of GDB.
>>>>>
>>>>> Note that it is a breaking change for GDB 13.0 or earlier, but it is
>>>>> also required for GDB 13.1 or later to work.
>>>>>
>>>>> Signed-off-by: Jiajie Chen <c@jia.je>
>>>>> ---
>>>>>   gdb-xml/loongarch-fpu.xml  |  9 ++++++++-
>>>>>   target/loongarch/gdbstub.c | 16 +++++++---------
>>>>>   2 files changed, 15 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/gdb-xml/loongarch-fpu.xml b/gdb-xml/loongarch-fpu.xml
>>>>> index 78e42cf5dd..e81e3382e7 100644
>>>>> --- a/gdb-xml/loongarch-fpu.xml
>>>>> +++ b/gdb-xml/loongarch-fpu.xml
>>>>> @@ -45,6 +45,13 @@
>>>>>     <reg name="f29" bitsize="64" type="fputype" group="float"/>
>>>>>     <reg name="f30" bitsize="64" type="fputype" group="float"/>
>>>>>     <reg name="f31" bitsize="64" type="fputype" group="float"/>
>>>>> -  <reg name="fcc" bitsize="64" type="uint64" group="float"/>
>>>>> +  <reg name="fcc0" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc1" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc2" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc3" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc4" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc5" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc6" bitsize="8" type="uint8" group="float"/>
>>>>> +  <reg name="fcc7" bitsize="8" type="uint8" group="float"/>
>>>>>     <reg name="fcsr" bitsize="32" type="uint32" group="float"/>
>>>>>   </feature>
>>>>> diff --git a/target/loongarch/gdbstub.c b/target/loongarch/gdbstub.c
>>>>> index 0752fff924..15ad6778f1 100644
>>>>> --- a/target/loongarch/gdbstub.c
>>>>> +++ b/target/loongarch/gdbstub.c
>>>>> @@ -70,10 +70,9 @@ static int loongarch_gdb_get_fpu(CPULoongArchState *env,
>>>>>   {
>>>>>       if (0 <= n && n < 32) {
>>>>>           return gdb_get_reg64(mem_buf, env->fpr[n].vreg.D(0));
>>>>> -    } else if (n == 32) {
>>>>> -        uint64_t val = read_fcc(env);
>>>>> -        return gdb_get_reg64(mem_buf, val);
>>>>> -    } else if (n == 33) {
>>>>> +    } else if (32 <= n && n < 40) {
>>>>> +        return gdb_get_reg8(mem_buf, env->cf[n - 32]);
>>>>> +    } else if (n == 40) {
>>>>>           return gdb_get_reg32(mem_buf, env->fcsr0);
>>>>>       }
>>>>>       return 0;
>>>>> @@ -87,11 +86,10 @@ static int loongarch_gdb_set_fpu(CPULoongArchState *env,
>>>>>       if (0 <= n && n < 32) {
>>>>>           env->fpr[n].vreg.D(0) = ldq_p(mem_buf);
>>>>>           length = 8;
>>>>> -    } else if (n == 32) {
>>>>> -        uint64_t val = ldq_p(mem_buf);
>>>>> -        write_fcc(env, val);
>>>>> -        length = 8;
>>>>> -    } else if (n == 33) {
>>>>> +    } else if (32 <= n && n < 40) {
>>>>> +        env->cf[n - 32] = ldub_p(mem_buf);
>>>>> +        length = 1;
>>>>> +    } else if (n == 40) {
>>>>>           env->fcsr0 = ldl_p(mem_buf);
>>>>>           length = 4;
>>>>>       }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro