Re: [PATCH qemu 2/3] target/arm/gdbstub: Support reading M system registers from GDB

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

On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> 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.
>
> `v7m_mrs_control` was renamed `arm_v7m_mrs_control` and made
> non-static so this logic could be shared between the MRS instruction and
> the GDB stub.
>
> Signed-off-by: David Reiss <dreiss@meta.com>
> ---
>  target/arm/cpu.h      |  11 +++-
>  target/arm/gdbstub.c  | 125 ++++++++++++++++++++++++++++++++++++++++++
>  target/arm/m_helper.c |   6 +-
>  3 files changed, 137 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 2b4bd20f9d..5cf86cf2d7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -852,6 +852,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];
> @@ -1094,11 +1095,13 @@ int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>
>  /*
> - * Helpers to dynamically generates XML descriptions of the sysregs
> - * and SVE registers. Returns the number of registers in each set.
> + * Helpers to dynamically generates XML descriptions of the sysregs,

we can fix the typo while we're changing this line: "dynamically generate"

> + * SVE registers, and M-profile system registers.
> + * Returns the number of registers in each set.
>   */
>  int arm_gen_dynamic_sysreg_xml(CPUState *cpu, int base_reg);
>  int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
> +int arm_gen_dynamic_m_systemreg_xml(CPUState *cpu, int base_reg);
>
>  /* Returns the dynamically generated XML for the gdb stub.
>   * Returns a pointer to the XML contents for the specified XML file or NULL
> @@ -1490,6 +1493,10 @@ FIELD(SVCR, ZA, 1, 1)
>  FIELD(SMCR, LEN, 0, 4)
>  FIELD(SMCR, FA64, 31, 1)
>
> +/* Read the CONTROL register as the MRS instruction would.
> + */

QEMU comment style is either
/* one line comment */
or
/*
 * multiline comment, with the opening and closing
 * slash-star and star-slash on lines of their own
 */

We do still have some older parts of the codebase with different
styles, but new code should follow the coding style.
scripts/checkpatch.pl usually but doesn't always catch this.

> +uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure);
> +
>  /* Write a new value to v7m.exception, thus transitioning into or out
>   * of Handler mode; this may result in a change of active stack pointer.
>   */
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 2f806512d0..4456892e91 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -322,6 +322,121 @@ int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
>      return cpu->dyn_sysreg_xml.num;
>  }
>
> +/*
> + * Helper required because g_array_append_val is a macro
> + * that cannot handle string literals.
> + */
> +static inline void g_array_append_str_literal(GArray *array, const char *str)
> +{
> +    g_array_append_val(array, str);
> +}
> +
> +static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg)
> +{
> +    /* NOTE: This implementation shares a lot of logic with v7m_mrs. */
> +    switch (reg) {
> +
> +    /*
> +     * NOTE: MSP and PSP technically don't exist if the secure extension
> +     * is present (replaced by MSP_S, MSP_NS, PSP_S, PSP_NS).  Similar for
> +     * MSPLIM and PSPLIM.
> +     * However, the MRS instruction is still allowed to read from MSP and PSP,
> +     * and will return the value associated with the current security 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.
> +     */
> +    case 0: /* MSP */
> +        return gdb_get_reg32(buf,
> +            v7m_using_psp(env) ? env->v7m.other_sp : env->regs[13]);
> +    case 1: /* PSP */
> +        return gdb_get_reg32(buf,
> +            v7m_using_psp(env) ? env->regs[13] : env->v7m.other_sp);
> +    case 6: /* MSPLIM */
> +        if (!arm_feature(env, ARM_FEATURE_V8)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.msplim[env->v7m.secure]);
> +    case 7: /* PSPLIM */
> +        if (!arm_feature(env, ARM_FEATURE_V8)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.psplim[env->v7m.secure]);
> +
> +    /*
> +     * NOTE: PRIMAKS, BASEPRI, and FAULTMASK are defined a bit differently
> +     * from the SP family, but have similar banking behavior.
> +     */
> +    case 2: /* PRIMASK */
> +        return gdb_get_reg32(buf, env->v7m.primask[env->v7m.secure]);
> +    case 3: /* BASEPRI */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.basepri[env->v7m.secure]);
> +    case 4: /* FAULTMASK */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.faultmask[env->v7m.secure]);
> +
> +    /*
> +     * NOTE: CONTROL has a mix of banked and non-banked bits.  We continue
> +     * to emulate the MRS instruction.  Unfortunately, this gives GDB no way
> +     * to read the SFPA bit when the CPU is in a non-secure state.
> +     */

Indent on this comment seems odd.

> +    case 5: /* CONTROL */
> +        return gdb_get_reg32(buf, arm_v7m_mrs_control(env, env->v7m.secure));
> +    }
> +
> +    return 0;
> +}
> +
> +static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
> +{
> +    /* TODO: Implement. */
> +    return 0;
> +}
> +
> +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);
> +    DynamicGDBXMLInfo *info = &cpu->dyn_m_systemreg_xml;
> +    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");
> +
> +    int is_v8 = arm_feature(env, ARM_FEATURE_V8);
> +    int is_main = arm_feature(env, ARM_FEATURE_M_MAIN);

Use bool for these, please. Also, coding style says don't declare
variables in the middle of a block.

> +
> +    g_autoptr(GArray) regs = g_array_new(false, true, sizeof(const char *));
> +    /* 0 */ g_array_append_str_literal(regs, "msp");
> +    /* 1 */ g_array_append_str_literal(regs, "psp");
> +    /* 2 */ g_array_append_str_literal(regs, "primask");
> +    /* 3 */ g_array_append_str_literal(regs, is_main ? "basepri" : "");
> +    /* 4 */ g_array_append_str_literal(regs, is_main ? "faultmask" : "");
> +    /* 5 */ g_array_append_str_literal(regs, "control");
> +    /* 6 */ g_array_append_str_literal(regs, is_v8 ? "msplim" : "");
> +    /* 7 */ g_array_append_str_literal(regs, is_v8 ? "psplim" : "");
> +
> +    for (int idx = 0; idx < regs->len; idx++) {
> +        const char *name = g_array_index(regs, const char *, idx);
> +        if (*name != '\0') {
> +            g_string_append_printf(s,
> +                        "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
> +                        name, base_reg);

Opinion seems to be divided on whether the registers here that
don't define all 32 bits should be reported as bitsize="32" or
not, eg OpenOCD reports primask etc as bitsize="1". But I found
at least one other generator of this XML which uses bitsize=32
throughout, so I guess we're good to do so also.

> +        }
> +        base_reg++;
> +    }
> +    info->num = regs->len;
> +
> +    g_string_append_printf(s, "</feature>");
> +    info->desc = g_string_free(s, false);
> +    return info->num;
> +}

thanks
-- PMM