[RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Alex Bennée]

Before this we suppress all ARM_CP_NORAW registers being listed under
GDB. This includes useful registers like CurrentEL which gets tagged
as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
registers. These are registers TCG can directly compute because we
have the information at compile time but until now with no readfn.

Add a .readfn to return the CurrentEL and then loosen the restrictions
in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
be read if there is a readfn available.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Julian Armistead <julian.armistead@linaro.org>
---
 target/arm/gdbstub.c |  6 +++++-
 target/arm/helper.c  | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index ce4497ad7c..029678ac9a 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -282,7 +282,11 @@ static void arm_register_sysreg_for_feature(gpointer key, gpointer value,
     CPUARMState *env = &cpu->env;
     DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature;
 
-    if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
+    if (!(ri->type & ARM_CP_NO_GDB)) {
+        /* skip ARM_CP_NO_RAW if there are no helper functions */
+        if ((ri->type & ARM_CP_NO_RAW) && !ri->readfn) {
+            return;
+        }
         if (arm_feature(env, ARM_FEATURE_AARCH64)) {
             if (ri->state == ARM_CP_STATE_AA64) {
                 arm_gen_one_feature_sysreg(&param->builder, dyn_feature,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7fb6e88630..7ea1307c20 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4993,6 +4993,17 @@ static void ic_ivau_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 #endif
 
+/*
+ * Normally the current_el is known at translation time and we can
+ * emit the result directly in TCG code. However this helper exists
+ * only so we can also expose CURRENTEL to gdb.
+ */
+static uint64_t aa64_currentel_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    int el = arm_current_el(env);
+    return el;
+}
+
 static const ARMCPRegInfo v8_cp_reginfo[] = {
     /*
      * Minimal set of EL0-visible registers. This will need to be expanded
@@ -5031,7 +5042,9 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     },
     { .name = "CURRENTEL", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .opc2 = 2, .crn = 4, .crm = 2,
-      .access = PL1_R, .type = ARM_CP_CURRENTEL },
+      .access = PL1_R, .type = ARM_CP_CURRENTEL,
+      .readfn = aa64_currentel_read
+    },
     /*
      * Instruction cache ops. All of these except `IC IVAU` NOP because we
      * don't emulate caches.
-- 
2.39.5

Re: [RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Peter Maydell]

On Wed, 7 May 2025 at 17:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Before this we suppress all ARM_CP_NORAW registers being listed under
> GDB. This includes useful registers like CurrentEL which gets tagged
> as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
> registers. These are registers TCG can directly compute because we
> have the information at compile time but until now with no readfn.
>
> Add a .readfn to return the CurrentEL and then loosen the restrictions
> in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
> be read if there is a readfn available.

The primary use case for NO_RAW is "system instructions" like
the TLB maintenance insns. These don't make sense to expose
to a debugger.

If we want the gdbstub access to system registers to be
more than our current "we provide the ones that are easy",
then I think I'd like to see a bit more up-front analysis of
what the gdbstub needs and whether we've got into a bit of
a mess with our ARM_CP_* flags that we could straighten out.

thanks
-- PMM

Re: [RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 7 May 2025 at 17:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Before this we suppress all ARM_CP_NORAW registers being listed under
>> GDB. This includes useful registers like CurrentEL which gets tagged
>> as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
>> registers. These are registers TCG can directly compute because we
>> have the information at compile time but until now with no readfn.
>>
>> Add a .readfn to return the CurrentEL and then loosen the restrictions
>> in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
>> be read if there is a readfn available.
>
> The primary use case for NO_RAW is "system instructions" like
> the TLB maintenance insns. These don't make sense to expose
> to a debugger.

I think we could re-think the logic:

    /*
     * By convention, for wildcarded registers only the first
     * entry is used for migration; the others are marked as
     * ALIAS so we don't try to transfer the register
     * multiple times. Special registers (ie NOP/WFI) are
     * never migratable and not even raw-accessible.
     */
    if (r2->type & ARM_CP_SPECIAL_MASK) {
        r2->type |= ARM_CP_NO_RAW;
    }

> If we want the gdbstub access to system registers to be
> more than our current "we provide the ones that are easy",
> then I think I'd like to see a bit more up-front analysis of
> what the gdbstub needs and whether we've got into a bit of
> a mess with our ARM_CP_* flags that we could straighten out.

Yeah - hence the RFC. CurrentEL is a super useful one to expose though
when you are debugging complex hypervisor setups.

>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Peter Maydell]

On Thu, 8 May 2025 at 12:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Wed, 7 May 2025 at 17:58, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Before this we suppress all ARM_CP_NORAW registers being listed under
> >> GDB. This includes useful registers like CurrentEL which gets tagged
> >> as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
> >> registers. These are registers TCG can directly compute because we
> >> have the information at compile time but until now with no readfn.
> >>
> >> Add a .readfn to return the CurrentEL and then loosen the restrictions
> >> in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
> >> be read if there is a readfn available.
> >
> > The primary use case for NO_RAW is "system instructions" like
> > the TLB maintenance insns. These don't make sense to expose
> > to a debugger.
>
> I think we could re-think the logic:
>
>     /*
>      * By convention, for wildcarded registers only the first
>      * entry is used for migration; the others are marked as
>      * ALIAS so we don't try to transfer the register
>      * multiple times. Special registers (ie NOP/WFI) are
>      * never migratable and not even raw-accessible.
>      */
>     if (r2->type & ARM_CP_SPECIAL_MASK) {
>         r2->type |= ARM_CP_NO_RAW;
>     }

Well, we definitely don't want WFI or the DC ZVA etc
"registers" to be exposed to GDB or for us to try to handle
them in KVM state sync or migration... ARM_CP_NOP is less
clear because we use it pretty widely for more than one
purpose. The main one is "system instruction that we don't
need to implement". (CP_NOP + a readable register is a
questionable combination since the guest will expect it to
update the general-purpose destreg, not leave it untouched,
but we do have some.)

> > If we want the gdbstub access to system registers to be
> > more than our current "we provide the ones that are easy",
> > then I think I'd like to see a bit more up-front analysis of
> > what the gdbstub needs and whether we've got into a bit of
> > a mess with our ARM_CP_* flags that we could straighten out.
>
> Yeah - hence the RFC. CurrentEL is a super useful one to expose though
> when you are debugging complex hypervisor setups.

One problem with this patch is the one that the reporter of
https://gitlab.com/qemu-project/qemu/-/issues/2760 noted
in the conversation there: it will allow the debugger to
read registers which have a side-effect on read, like
ICC_IAR1_EL1: we almost certainly do not want to allow
the debugger to acknowledge an interrupt by doing a sysreg
read.

thanks
-- PMM

Re: [RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 8 May 2025 at 12:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Wed, 7 May 2025 at 17:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >>
>> >> Before this we suppress all ARM_CP_NORAW registers being listed under
>> >> GDB. This includes useful registers like CurrentEL which gets tagged
>> >> as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
>> >> registers. These are registers TCG can directly compute because we
>> >> have the information at compile time but until now with no readfn.
>> >>
>> >> Add a .readfn to return the CurrentEL and then loosen the restrictions
>> >> in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
>> >> be read if there is a readfn available.
>> >
>> > The primary use case for NO_RAW is "system instructions" like
>> > the TLB maintenance insns. These don't make sense to expose
>> > to a debugger.
>>
>> I think we could re-think the logic:
>>
>>     /*
>>      * By convention, for wildcarded registers only the first
>>      * entry is used for migration; the others are marked as
>>      * ALIAS so we don't try to transfer the register
>>      * multiple times. Special registers (ie NOP/WFI) are
>>      * never migratable and not even raw-accessible.
>>      */
>>     if (r2->type & ARM_CP_SPECIAL_MASK) {
>>         r2->type |= ARM_CP_NO_RAW;
>>     }
>
> Well, we definitely don't want WFI or the DC ZVA etc
> "registers" to be exposed to GDB or for us to try to handle
> them in KVM state sync or migration... ARM_CP_NOP is less
> clear because we use it pretty widely for more than one
> purpose. The main one is "system instruction that we don't
> need to implement". (CP_NOP + a readable register is a
> questionable combination since the guest will expect it to
> update the general-purpose destreg, not leave it untouched,
> but we do have some.)
>
>> > If we want the gdbstub access to system registers to be
>> > more than our current "we provide the ones that are easy",
>> > then I think I'd like to see a bit more up-front analysis of
>> > what the gdbstub needs and whether we've got into a bit of
>> > a mess with our ARM_CP_* flags that we could straighten out.
>>
>> Yeah - hence the RFC. CurrentEL is a super useful one to expose though
>> when you are debugging complex hypervisor setups.
>
> One problem with this patch is the one that the reporter of
> https://gitlab.com/qemu-project/qemu/-/issues/2760 noted
> in the conversation there: it will allow the debugger to
> read registers which have a side-effect on read, like
> ICC_IAR1_EL1: we almost certainly do not want to allow
> the debugger to acknowledge an interrupt by doing a sysreg
> read.

Doesn't raw_readfn offer these semantics?

    /*
     * Function for doing a "raw" read; used when we need to copy
     * coprocessor state to the kernel for KVM or out for
     * migration. This only needs to be provided if there is also a
     * readfn and it has side effects (for instance clear-on-read bits).
     */
    CPReadFn *raw_readfn;

So maybe:

        /* We can only read ARM_CP_NO_RAW regs without side effects */
        if ((ri->type & ARM_CP_NO_RAW) && !ri->raw_readfn) {
            return;
        }

And I guess we can strengthen the gdb helper to NOP any writes to such
registers.

>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] target/arm: allow gdb to read ARM_CP_NORAW regs

[Peter Maydell]

On Thu, 8 May 2025 at 14:37, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 8 May 2025 at 12:50, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >> > On Wed, 7 May 2025 at 17:58, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> >>
> >> >> Before this we suppress all ARM_CP_NORAW registers being listed under
> >> >> GDB. This includes useful registers like CurrentEL which gets tagged
> >> >> as ARM_CP_NO_RAW because it is one of the ARM_CP_SPECIAL_MASK
> >> >> registers. These are registers TCG can directly compute because we
> >> >> have the information at compile time but until now with no readfn.
> >> >>
> >> >> Add a .readfn to return the CurrentEL and then loosen the restrictions
> >> >> in arm_register_sysreg_for_feature to allow ARM_CP_NORAW registers to
> >> >> be read if there is a readfn available.
> >> >
> >> > The primary use case for NO_RAW is "system instructions" like
> >> > the TLB maintenance insns. These don't make sense to expose
> >> > to a debugger.
> >>
> >> I think we could re-think the logic:
> >>
> >>     /*
> >>      * By convention, for wildcarded registers only the first
> >>      * entry is used for migration; the others are marked as
> >>      * ALIAS so we don't try to transfer the register
> >>      * multiple times. Special registers (ie NOP/WFI) are
> >>      * never migratable and not even raw-accessible.
> >>      */
> >>     if (r2->type & ARM_CP_SPECIAL_MASK) {
> >>         r2->type |= ARM_CP_NO_RAW;
> >>     }
> >
> > Well, we definitely don't want WFI or the DC ZVA etc
> > "registers" to be exposed to GDB or for us to try to handle
> > them in KVM state sync or migration... ARM_CP_NOP is less
> > clear because we use it pretty widely for more than one
> > purpose. The main one is "system instruction that we don't
> > need to implement". (CP_NOP + a readable register is a
> > questionable combination since the guest will expect it to
> > update the general-purpose destreg, not leave it untouched,
> > but we do have some.)
> >
> >> > If we want the gdbstub access to system registers to be
> >> > more than our current "we provide the ones that are easy",
> >> > then I think I'd like to see a bit more up-front analysis of
> >> > what the gdbstub needs and whether we've got into a bit of
> >> > a mess with our ARM_CP_* flags that we could straighten out.
> >>
> >> Yeah - hence the RFC. CurrentEL is a super useful one to expose though
> >> when you are debugging complex hypervisor setups.
> >
> > One problem with this patch is the one that the reporter of
> > https://gitlab.com/qemu-project/qemu/-/issues/2760 noted
> > in the conversation there: it will allow the debugger to
> > read registers which have a side-effect on read, like
> > ICC_IAR1_EL1: we almost certainly do not want to allow
> > the debugger to acknowledge an interrupt by doing a sysreg
> > read.
>
> Doesn't raw_readfn offer these semantics?
>
>     /*
>      * Function for doing a "raw" read; used when we need to copy
>      * coprocessor state to the kernel for KVM or out for
>      * migration. This only needs to be provided if there is also a
>      * readfn and it has side effects (for instance clear-on-read bits).
>      */
>     CPReadFn *raw_readfn;
>
> So maybe:
>
>         /* We can only read ARM_CP_NO_RAW regs without side effects */
>         if ((ri->type & ARM_CP_NO_RAW) && !ri->raw_readfn) {
>             return;
>         }
>
> And I guess we can strengthen the gdb helper to NOP any writes to such
> registers.

This seems to be continuing down the path of "oh, if we just
tweak this condition here it seems to whack this particular
mole on the head". What I'm asking for is a more holistic
view of what we're trying to achieve and what the "right"
design for that ought to be, not for small patches that
add more ad-hoc adjustments to where we are currently.

thanks
-- PMM