| Message ID | 167330628518.10497.13100425787268927786-1@git.sr.ht |
|---|---|
| State | New |
| Headers | show |
| Series | [qemu,1/3] target/arm: Unify checking for M Main Extension in MRS/MSR | expand |
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
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, + * 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. + */ +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. + */ + 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); + + 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); + } + base_reg++; + } + info->num = regs->len; + + g_string_append_printf(s, "</feature>"); + info->desc = g_string_free(s, false); + return info->num; +} + struct TypeSize { const char *gdb_type; int size; @@ -450,6 +565,8 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) return cpu->dyn_sysreg_xml.desc; } else if (strcmp(xmlname, "sve-registers.xml") == 0) { return cpu->dyn_svereg_xml.desc; + } else if (strcmp(xmlname, "arm-m-system.xml") == 0) { + return cpu->dyn_m_systemreg_xml.desc; } return NULL; } @@ -493,6 +610,14 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) */ gdb_register_coprocessor(cs, vfp_gdb_get_sysreg, vfp_gdb_set_sysreg, 2, "arm-vfp-sysregs.xml", 0); + } else { + /* M-profile coprocessors. */ + gdb_register_coprocessor(cs, + arm_gdb_get_m_systemreg, + arm_gdb_set_m_systemreg, + arm_gen_dynamic_m_systemreg_xml( + cs, cs->gdb_num_regs), + "arm-m-system.xml", 0); } } if (cpu_isar_feature(aa32_mve, cpu)) { diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c index e5ccfa9615..31d4dfba4b 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -69,7 +69,7 @@ static uint32_t v7m_mrs_xpsr(CPUARMState *env, uint32_t reg, unsigned el) return xpsr_read(env) & mask; } -static uint32_t v7m_mrs_control(CPUARMState *env, uint32_t secure) +uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure) { uint32_t value = env->v7m.control[secure]; @@ -106,7 +106,7 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) case 0 ... 7: /* xPSR sub-fields */ return v7m_mrs_xpsr(env, reg, 0); case 20: /* CONTROL */ - return v7m_mrs_control(env, 0); + return arm_v7m_mrs_control(env, 0); default: /* Unprivileged reads others as zero. */ return 0; @@ -2436,7 +2436,7 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) case 0 ... 7: /* xPSR sub-fields */ return v7m_mrs_xpsr(env, reg, el); case 20: /* CONTROL */ - return v7m_mrs_control(env, env->v7m.secure); + return arm_v7m_mrs_control(env, env->v7m.secure); case 0x94: /* CONTROL_NS */ /* * We have to handle this here because unprivileged Secure code