| 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
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
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
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(-)