Re: [PATCH 1/4] gdbstub/aarch64: add CurrentEL register

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 8/8/25 5:26 AM, Manos Pitsidianakis wrote:
> On Fri, Aug 8, 2025 at 3:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 8 Aug 2025 at 12:30, Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>>>
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> ---
>>>   gdb-xml/aarch64-core.xml | 5 +++++
>>>   target/arm/cpu.h         | 1 +
>>>   target/arm/gdbstub64.c   | 6 ++++++
>>>   3 files changed, 12 insertions(+)
>>>
>>> diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
>>> index b8046510b9a085d30463d37b3ecc8d435f5fb7a4..19ad743dc5607b4021fb795bfb9b8e9cf0adef68 100644
>>> --- a/gdb-xml/aarch64-core.xml
>>> +++ b/gdb-xml/aarch64-core.xml
>>> @@ -91,4 +91,9 @@
>>>     </flags>
>>>     <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
>>>
>>> +  <flags id="current_el_flags" size="8">
>>> +    <!-- Exception Level.  -->
>>> +    <field name="EL" start="2" end="3"/>
>>> +  </flags>
>>> +  <reg name="CurrentEL" bitsize="64" type="current_el"/>
>>>   </feature>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index dc9b6dce4c92297b2636d0d7c0dce580f1806d5b..c3070cd9863381fac40f5640e0a7a84dfa1c6e06 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1473,6 +1473,7 @@ void pmu_init(ARMCPU *cpu);
>>>    * AArch32 mode SPSRs are basically CPSR-format.
>>>    */
>>>   #define PSTATE_SP (1U)
>>> +#define PSTATE_EL (3U << 2)
>>>   #define PSTATE_M (0xFU)
>>>   #define PSTATE_nRW (1U << 4)
>>>   #define PSTATE_F (1U << 6)
>>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
>>> index 08e28585396816ab90d6d8e460ff8171892fe8da..16b564e1a970cb5e854a705619f71ffc61545a73 100644
>>> --- a/target/arm/gdbstub64.c
>>> +++ b/target/arm/gdbstub64.c
>>> @@ -48,6 +48,9 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>>           return gdb_get_reg64(mem_buf, env->pc);
>>>       case 33:
>>>           return gdb_get_reg32(mem_buf, pstate_read(env));
>>> +    case 34:
>>> +        /* CurrentEL */
>>> +        return gdb_get_reg64(mem_buf, env->pstate & PSTATE_EL);
>>>       }
>>
>> The debugger already has this information in the 'cpsr'
>> register, so it could implement convenience views of
>> the subfields itself if it liked.
> 
> Yep, but consider: it is a register, architecturally, so it's nice to
> include it for consistency. It's redundant only because gdb has cpsr
> which is not a register. So this is about more about being technically
> correct than correcting an actual problem.
> 

I agree with Manos on this.
As mentioned on a previous thread, cpsr is not even supposed to exist 
for aarch64. So adding architecturally defined registers, even if data 
is redundant with cpsr, should not be a problem.
I'm sure gdb folks can understand this too.

> 
>>
>> If we're going to do this I would prefer it to be because
>> we've gained some consensus with e.g. the gdb maintainers
>> that this is the "preferred" way to expose the CPU state.
>> The XML config stuff lets us do it in our own way if we
>> want to, but I think there is value in consistency across
>> stubs here.
> 
> I plan to upstream the XML patches to gdb as well, separately. But
> since we use custom target xml it's not like we depend on gdb to
> change it.
> 
>>
>> thanks
>> -- PMM