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

[gaosong <gaosong@loongson.cn>]

Hi, Alex

在 2023/8/9 上午2:40, Alex Bennée 写道:
> 
> 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
> 
Thanks for you suggestions. we will use this method for vector extented.

For this patch:
Acked-by: Song Gao <gaosong@loongson.cn>

Thanks.
Song Gao

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