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

[Peter Maydell]

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.
>
> Signed-off-by: David Reiss <dreiss@meta.com>

> +static struct v8m_stack_registers get_v8m_stack_registers(CPUARMState *env)
> +{
> +    uint32_t current_ss_msp;
> +    uint32_t current_ss_psp;
> +    if (v7m_using_psp(env)) {
> +        current_ss_msp = env->v7m.other_sp;
> +        current_ss_psp = env->regs[13];
> +    } else {
> +        current_ss_msp = env->regs[13];
> +        current_ss_psp = env->v7m.other_sp;
> +
> +    }
> +
> +    struct v8m_stack_registers ret;
> +    if (env->v7m.secure) {
> +        ret.msp_s = current_ss_msp;
> +        ret.psp_s = current_ss_psp;
> +        ret.msp_ns = env->v7m.other_ss_msp;
> +        ret.psp_ns = env->v7m.other_ss_psp;
> +    } else {
> +        ret.msp_s = env->v7m.other_ss_msp;
> +        ret.psp_s = env->v7m.other_ss_psp;
> +        ret.msp_ns = current_ss_msp;
> +        ret.psp_ns = current_ss_psp;
> +    }
> +
> +    return ret;
> +}

I wondered about using get_v7m_sp_ptr(), but this is fine I guess.

> +static int arm_gdb_get_m_secextreg(CPUARMState *env, GByteArray *buf, int 
> reg)
> +{
> +    switch (reg) {
> +    case  0:  /* MSP_S */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).msp_s);
> +    case  1:  /* PSP_S */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_s);
> +    case  2:  /* MSPLIM_S */
> +        return gdb_get_reg32(buf, env->v7m.msplim[M_REG_S]);
> +    case  3:  /* PSPLIM_S */
> +        return gdb_get_reg32(buf, env->v7m.psplim[M_REG_S]);
> +    case  4:  /* PRIMASK_S */
> +        return gdb_get_reg32(buf, env->v7m.primask[M_REG_S]);
> +    case  5:  /* BASEPRI_S */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.basepri[M_REG_S]);
> +    case  6:  /* FAULTMASK_S */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.faultmask[M_REG_S]);
> +    case  7:  /* CONTROL_S */
> +        return gdb_get_reg32(buf, env->v7m.control[M_REG_S]);
> +    case  8:  /* MSP_NS */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).msp_ns);
> +    case  9:  /* PSP_NS */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_ns);
> +    case 10:  /* MSPLIM_NS */
> +        return gdb_get_reg32(buf, env->v7m.msplim[M_REG_NS]);
> +    case 11:  /* PSPLIM_NS */
> +        return gdb_get_reg32(buf, env->v7m.psplim[M_REG_NS]);
> +    case 12:  /* PRIMASK_NS */
> +        return gdb_get_reg32(buf, env->v7m.primask[M_REG_NS]);
> +    case 13:  /* BASEPRI_NS */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.basepri[M_REG_NS]);
> +    case 14:  /* FAULTMASK_NS */
> +        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +            return 0;
> +        }
> +        return gdb_get_reg32(buf, env->v7m.faultmask[M_REG_NS]);
> +    case 15:  /* CONTROL_NS */
> +        return gdb_get_reg32(buf, env->v7m.control[M_REG_NS]);
> +    }
> +
> +    return 0;
> +}
> +
> +static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg)
> +{
> +    /* TODO: Implement. */
> +    return 0;
> +}
> +
> +int arm_gen_dynamic_m_securereg_xml(CPUState *cs, int base_reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    GString *s = g_string_new(NULL);
> +    DynamicGDBXMLInfo *info = &cpu->dyn_m_securereg_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.secext\">\n");
> +
> +    g_autoptr(GArray) regs = g_array_new(false, true, sizeof(const char *));
> +    /*  0 */ g_array_append_str_literal(regs, "msp_s");
> +    /*  1 */ g_array_append_str_literal(regs, "psp_s");
> +    /*  2 */ g_array_append_str_literal(regs, "msplim_s");
> +    /*  3 */ g_array_append_str_literal(regs, "psplim_s");
> +    /*  4 */ g_array_append_str_literal(regs, "primask_s");
> +    /*  5 */ g_array_append_str_literal(regs, "basepri_s");
> +    /*  6 */ g_array_append_str_literal(regs, "faultmask_s");
> +    /*  7 */ g_array_append_str_literal(regs, "control_s");
> +    /*  8 */ g_array_append_str_literal(regs, "msp_ns");
> +    /*  9 */ g_array_append_str_literal(regs, "psp_ns");
> +    /* 10 */ g_array_append_str_literal(regs, "msplim_ns");
> +    /* 11 */ g_array_append_str_literal(regs, "psplim_ns");
> +    /* 12 */ g_array_append_str_literal(regs, "primask_ns");
> +    /* 13 */ g_array_append_str_literal(regs, "basepri_ns");
> +    /* 14 */ g_array_append_str_literal(regs, "faultmask_ns");
> +    /* 15 */ g_array_append_str_literal(regs, "control_ns");

In patch 1 you skip the registers that don't exist without
the main extension, but here you throw them all in regardless.
Why the difference ?

(If we do want to have the xml be fixed and not dependent on
whether the main extension exists, we don't need to runtime
generate it, but can instead use a fixed xml file, as we do with
eg arm-m-profile-mve.xml.)

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

thanks
-- PMM