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

["Alex Bennée" <alex.bennee@linaro.org>]

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