Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb

[Peter Maydell]

On Tue, 14 Feb 2023 at 16:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: David Reiss <dreiss@meta.com>
>
> Follows a fairly similar pattern to the existing special register
> debug support.  Only reading is implemented, but it should be
> possible to implement writes.
>
> Signed-off-by: David Reiss <dreiss@meta.com>
> [rth: Split out from two other patches;
>  Use an enumeration to locally number the registers.
>  Use a structure to list and control runtime visibility.
>  Handle security extension with the same code.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h     |   1 +
>  target/arm/gdbstub.c | 169 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 170 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index c9f768f945..536e60d48c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -867,6 +867,7 @@ struct ArchCPU {
>
>      DynamicGDBXMLInfo dyn_sysreg_xml;
>      DynamicGDBXMLInfo dyn_svereg_xml;
> +    DynamicGDBXMLInfo dyn_m_systemreg_xml;
>
>      /* Timers used by the generic (architected) timer */
>      QEMUTimer *gt_timer[NUM_GTIMERS];
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 062c8d447a..a8848c7fee 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -322,6 +322,167 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int 
> base_reg)
>      return cpu->dyn_sysreg_xml.num;
>  }
>
> +enum {
> +    M_SYSREG_MSP        = 0,
> +    M_SYSREG_PSP        = 1,
> +    M_SYSREG_PRIMASK    = 2,
> +    M_SYSREG_CONTROL    = 3,
> +    M_SYSREG_BASEPRI    = 4,
> +    M_SYSREG_FAULTMASK  = 5,
> +    M_SYSREG_MSPLIM     = 6,
> +    M_SYSREG_PSPLIM     = 7,
> +    M_SYSREG_REG_MASK   = 7,
> +
> +    /*
> +     * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the
> +     * secure extension is present (replaced by MSP_S, MSP_NS, et al).
> +     * However, the MRS instruction is still allowed to read from MSP and 
> PSP,
> +     * and will return the value associated with the current security state.

What's "don't exist" based on here? Architecturally MSPLIM, PSPLIM etc
are banked registers; MRS/MSR let you access the one for the current
security state, and (if you are Secure) the one for the NS state.

> +     * We replicate this behavior for the convenience of users, who will see
> +     * GDB behave similarly to their assembly code, even if they are 
> oblivious
> +     * to the security extension.
> +     */
> +    M_SYSREG_CURRENT    = 0 << 3,
> +    M_SYSREG_NONSECURE  = 1 << 3,
> +    M_SYSREG_SECURE     = 2 << 3,
> +    M_SYSREG_MODE_MASK  = 3 << 3,
> +};
> +

> +static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    GString *s = g_string_new(NULL);
> +    int i, ret;
> +
> +    g_string_printf(s, "<?xml version=\"1.0\"?>");
> +    g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
> +    g_string_append_printf(s, "<feature 
> name=\"org.gnu.gdb.arm.m-system\">\n");

Half of these need to be in org.gnu.gdb.arm.secext.
These aren't our own XML features we're making up (if they
were then they would be in org.qemu.something), so we should
follow the existing precedent about what registers go in them.

thanks
-- PMM