On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-m...@git.sr.ht> wrote:
>
> From: David Reiss <dre...@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 <dre...@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
