On Tue, 1 Sept 2026 at 18:52, Alex Bennée <[email protected]> wrote: > > Gilles Grimaud <[email protected]> writes: > > > From: gilles grimaud <[email protected]> > > > > Cortex-M0+ implementations may provide an Armv6 PMSA MPU with eight > > regions. QEMU already marks M-profile CPUs as PMSA, but only exposes and > > allocates the existing region model for Armv7 CPUs. This prevents a > > Cortex-M0+ board from enabling its optional MPU. > > > > Just a comment for all patches, you should ensure your editor wraps at > 80 columns (72 for title) as patches are often read on terminals. > > It's good habit to run through checkpatch.pl to pick up the minor > oft-forgotten stuff. > > > Use QEMU's PMSAv7 state and translation path for the compatible > > Armv6-M MPU register layout. Permit CONTROL.nPRIV changes when the > > optional MPU is present and clear the region state on reset. > > Hmm will need to think about this - the file is explicitly targeting the > armv7_nvic so we might want to consider renaming if indeed special > casing the differences is the right thing to do.
All the M-profile code is shot through with "v7m" naming for historical reasons. It is all intended to cover v6M, v7M and v8M. v6M is awkward because its naming is more marketing driven than related to the progression of A-profile versions. Really v6M is more like "v7M without the Main Extension" -- it's a cut-down v7M, not a predecessor to it. v8M tidies up the naming here by making "is cut-down" orthogonal to the architectural version number. It is tempting to wonder if we should model v6M CPUs by having them actually set ARM_FEATURE_M + ARM_FEATURE_V7 but not ARM_FEATURE_M_MAIN. But that could quite easily have just as many annoying corner cases only the other way around... > > Signed-off-by: gilles grimaud <[email protected]> > > --- > > hw/intc/armv7m_nvic.c | 3 +++ > > target/arm/cpu.c | 12 +++++++++--- > > target/arm/ptw.c | 3 ++- > > target/arm/tcg/m_helper.c | 10 +++++++++- > > 4 files changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > > index a7651f831e..3140599ac1 100644 > > --- a/hw/intc/armv7m_nvic.c > > +++ b/hw/intc/armv7m_nvic.c > > @@ -1626,6 +1626,9 @@ static void nvic_writel(NVICState *s, uint32_t > > offset, uint32_t value, > > break; > > case 0xd08: /* Vector Table Offset. */ > > cpu->env.v7m.vecbase[attrs.secure] = value & 0xffffff80; > > + if (!arm_feature(&cpu->env, ARM_FEATURE_M_SECURITY)) { > > + cpu->env.v7m.vecbase[!attrs.secure] = value & 0xffffff80; > > + } This looks dubious and also unrelated to what the commit message says the change is doing. The VTOR exists whether the CPU has the Security extension or not. > > diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c > > index f4ba93b291..667b2a3cbd 100644 > > --- a/target/arm/tcg/m_helper.c > > +++ b/target/arm/tcg/m_helper.c > > @@ -16,6 +16,7 @@ > > #include "qemu/bitops.h" > > #include "qemu/log.h" > > #include "exec/page-protection.h" > > +#include "exec/cputlb.h" > > #ifdef CONFIG_TCG > > #include "accel/tcg/cpu-ldst-common.h" > > #include "semihosting/common-semi.h" > > @@ -2775,9 +2776,16 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t > > maskreg, uint32_t val) > > !arm_v7m_is_handler_mode(env))) { > > write_v7m_control_spsel(env, (val & R_V7M_CONTROL_SPSEL_MASK) > > != 0); > > } > > - if (cur_el > 0 && arm_feature(env, ARM_FEATURE_M_MAIN)) { > > + if (cur_el > 0 && (arm_feature(env, ARM_FEATURE_M_MAIN) || > > + env_archcpu(env)->has_mpu)) { Why are we testing against has_mpu here ? > > + uint32_t old_control = env->v7m.control[env->v7m.secure]; > > + > > env->v7m.control[env->v7m.secure] &= ~R_V7M_CONTROL_NPRIV_MASK; > > env->v7m.control[env->v7m.secure] |= val & > > R_V7M_CONTROL_NPRIV_MASK; > > + if ((old_control ^ env->v7m.control[env->v7m.secure]) & > > + R_V7M_CONTROL_NPRIV_MASK) { > > + tlb_flush(env_cpu(env)); Where has this tlb_flush() appeared from, and why? > > + } > > } > > if (cpu_isar_feature(aa32_vfp_simd, env_archcpu(env))) { > > /* -- PMM
