| Message ID | 167330628518.10497.13100425787268927786-0@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> > > BASEPRI, FAULTMASK, and their _NS equivalents only exist on devices with > the Main Extension. However, the MRS instruction did not check this, > and the MSR instruction handled it inconsistently (warning BASEPRI, but > silently ignoring writes to BASEPRI_NS). Unify this behavior and always > warn when reading or writing any of these registers if the extension is > not present. > > Signed-off-by: David Reiss <dreiss@meta.com> > --- Hi; it looks like you didn't send a cover letter for this patchset. If you're making contributions in future, if you could send a cover letter for a multi-patch submission, that's helpful because our automated tooling tends to get confused by patchsets which don't have one. (A single standalone patch doesn't need a cover letter.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Tue, 17 Jan 2023 at 11:35, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> wrote: > > > > From: David Reiss <dreiss@meta.com> > > > > BASEPRI, FAULTMASK, and their _NS equivalents only exist on devices with > > the Main Extension. However, the MRS instruction did not check this, > > and the MSR instruction handled it inconsistently (warning BASEPRI, but > > silently ignoring writes to BASEPRI_NS). Unify this behavior and always > > warn when reading or writing any of these registers if the extension is > > not present. > > > > Signed-off-by: David Reiss <dreiss@meta.com> > > --- > > Hi; it looks like you didn't send a cover letter for this patchset. > If you're making contributions in future, if you could send a cover > letter for a multi-patch submission, that's helpful because our > automated tooling tends to get confused by patchsets which don't > have one. (A single standalone patch doesn't need a cover letter.) > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Since this patch is OK and not really related to 2 and 3, I'm going to apply this to target-arm.next. I've left review comments on the other two patches. thanks -- PMM
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c index 355cd4d60a..e5ccfa9615 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -2481,11 +2481,17 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) } return env->v7m.primask[M_REG_NS]; case 0x91: /* BASEPRI_NS */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } if (!env->v7m.secure) { return 0; } return env->v7m.basepri[M_REG_NS]; case 0x93: /* FAULTMASK_NS */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } if (!env->v7m.secure) { return 0; } @@ -2531,8 +2537,14 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) return env->v7m.primask[env->v7m.secure]; case 17: /* BASEPRI */ case 18: /* BASEPRI_MAX */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } return env->v7m.basepri[env->v7m.secure]; case 19: /* FAULTMASK */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } return env->v7m.faultmask[env->v7m.secure]; default: bad_reg: @@ -2597,13 +2609,19 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) env->v7m.primask[M_REG_NS] = val & 1; return; case 0x91: /* BASEPRI_NS */ - if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) { + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } + if (!env->v7m.secure) { return; } env->v7m.basepri[M_REG_NS] = val & 0xff; return; case 0x93: /* FAULTMASK_NS */ - if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) { + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } + if (!env->v7m.secure) { return; } env->v7m.faultmask[M_REG_NS] = val & 1;