| Message ID | 20250507165840.401623-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] target/arm: allow gdb to read ARM_CP_NORAW regs | expand |
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
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
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
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(¶m->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.
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(-)