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

[Peter Maydell <peter.maydell@linaro.org>]

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