| Message ID | 20180713103059.12539-1-jusual@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 13, 2018 at 01:30:59PM +0300, Julia Suvorova wrote: > Forbid stack alignment change. (CCR) > Reserve FAULTMASK, BASEPRI registers. > Report any fault as HardFault. Disable MemManage, BusFault and > UsageFault, so they always escalated to HardFault. (SHCSR) > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > This is the last cortex-m0 patch. > > hw/intc/armv7m_nvic.c | 10 ++++++++++ > target/arm/cpu.c | 10 ++++++++++ > target/arm/helper.c | 13 +++++++++++-- > 3 files changed, 31 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 13 July 2018 at 11:30, Julia Suvorova <jusual@mail.ru> wrote: > Forbid stack alignment change. (CCR) > Reserve FAULTMASK, BASEPRI registers. > Report any fault as HardFault. Disable MemManage, BusFault and > UsageFault, so they always escalated to HardFault. (SHCSR) > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > This is the last cortex-m0 patch. > > hw/intc/armv7m_nvic.c | 10 ++++++++++ > target/arm/cpu.c | 10 ++++++++++ > target/arm/helper.c | 13 +++++++++++-- > 3 files changed, 31 insertions(+), 2 deletions(-) Most of this looks good; I have some comments on the reset value of CCR. > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index a914ce4e8c..3788cb773d 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -220,6 +220,11 @@ static void arm_cpu_reset(CPUState *s) > env->v7m.aircr = R_V7M_AIRCR_BFHFNMINS_MASK; > } > > + if (!arm_feature(env, ARM_FEATURE_V7)) { > + env->v7m.ccr[M_REG_NS] = 0x3f8; > + env->v7m.ccr[M_REG_S] = 0x3f8; This code will have no effect, because just below we already have an assignment to these fields: env->v7m.ccr[M_REG_NS] = R_V7M_CCR_STKALIGN_MASK; env->v7m.ccr[M_REG_S] = R_V7M_CCR_STKALIGN_MASK; > + } > + > /* In v7M the reset value of this bit is IMPDEF, but ARM recommends > * that it resets to 1, so QEMU always does that rather than making > * it dependent on CPU model. In v8M it is RES1. > @@ -230,6 +235,11 @@ static void arm_cpu_reset(CPUState *s) > /* in v8M the NONBASETHRDENA bit [0] is RES1 */ > env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_NONBASETHRDENA_MASK; > env->v7m.ccr[M_REG_S] |= R_V7M_CCR_NONBASETHRDENA_MASK; > + > + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { > + env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_UNALIGN_TRP_MASK; > + env->v7m.ccr[M_REG_S] |= R_V7M_CCR_UNALIGN_TRP_MASK; > + } This should be outside the "if v8" if(), because you also want it for v6M (giving you the v6M CCR value of STKALIGN and UNALIGN_TRP set and all other bits clear). thanks -- PMM
On 17.07.2018 16:09, Peter Maydell wrote: >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index a914ce4e8c..3788cb773d 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -220,6 +220,11 @@ static void arm_cpu_reset(CPUState *s) >> env->v7m.aircr = R_V7M_AIRCR_BFHFNMINS_MASK; >> } >> >> + if (!arm_feature(env, ARM_FEATURE_V7)) { >> + env->v7m.ccr[M_REG_NS] = 0x3f8; >> + env->v7m.ccr[M_REG_S] = 0x3f8; > > This code will have no effect, because just below we already have an > assignment to these fields: > env->v7m.ccr[M_REG_NS] = R_V7M_CCR_STKALIGN_MASK; > env->v7m.ccr[M_REG_S] = R_V7M_CCR_STKALIGN_MASK; My bad; I'll put the assignments that you mentioned into if/else block. >> + } >> + >> /* In v7M the reset value of this bit is IMPDEF, but ARM recommends >> * that it resets to 1, so QEMU always does that rather than making >> * it dependent on CPU model. In v8M it is RES1. >> @@ -230,6 +235,11 @@ static void arm_cpu_reset(CPUState *s) >> /* in v8M the NONBASETHRDENA bit [0] is RES1 */ >> env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_NONBASETHRDENA_MASK; >> env->v7m.ccr[M_REG_S] |= R_V7M_CCR_NONBASETHRDENA_MASK; >> + >> + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { >> + env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_UNALIGN_TRP_MASK; >> + env->v7m.ccr[M_REG_S] |= R_V7M_CCR_UNALIGN_TRP_MASK; >> + } > > This should be outside the "if v8" if(), because you also want it for v6M > (giving you the v6M CCR value of STKALIGN and UNALIGN_TRP set and all > other bits clear). This is the main problem. If I understand correctly, bits[4:8] also should be read-as-one (Table B3-4 ARMv6-M ARM). And I've already set them (with UNALIGN_TRP) before for v6m. Best regards, Julia Suvorova.
On 17 July 2018 at 14:42, Julia Suvorova <jusual@mail.ru> wrote: > On 17.07.2018 16:09, Peter Maydell wrote: >> This should be outside the "if v8" if(), because you also want it for v6M >> (giving you the v6M CCR value of STKALIGN and UNALIGN_TRP set and all >> other bits clear). > > > This is the main problem. If I understand correctly, bits[4:8] also > should be read-as-one (Table B3-4 ARMv6-M ARM). And I've already set them > (with UNALIGN_TRP) before for v6m. That is very odd. In table B3-10 bits [8:4] are documented as "reserved" which usually means reads-as-zero. I wonder if table B3-4 has a docs error and should really be saying "bits [9,3] = 0b11" rather than specifying [9:3]" ? Do you have access to a real hardware Cortex-M0 which you can read the CCR value from ? thanks -- PMM
On 17.07.2018 16:49, Peter Maydell wrote: > On 17 July 2018 at 14:42, Julia Suvorova <jusual@mail.ru> wrote: >> On 17.07.2018 16:09, Peter Maydell wrote: >>> This should be outside the "if v8" if(), because you also want it for v6M >>> (giving you the v6M CCR value of STKALIGN and UNALIGN_TRP set and all >>> other bits clear). >> >> >> This is the main problem. If I understand correctly, bits[4:8] also >> should be read-as-one (Table B3-4 ARMv6-M ARM). And I've already set them >> (with UNALIGN_TRP) before for v6m. > > That is very odd. In table B3-10 bits [8:4] are documented as > "reserved" which usually means reads-as-zero. I wonder if table > B3-4 has a docs error and should really be saying > "bits [9,3] = 0b11" rather than specifying [9:3]" ? > > Do you have access to a real hardware Cortex-M0 which you can read > the CCR value from ? It's a docs error. CCR is 520 (0b1000001000) on real micro:bit. Best regards, Julia Suvorova.
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index dbc0061b2d..5eec07342e 100644 --- a/hw/intc/armv7m_nvic.c +++ b/hw/intc/armv7m_nvic.c @@ -885,6 +885,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs) val |= cpu->env.v7m.ccr[M_REG_NS] & R_V7M_CCR_BFHFNMIGN_MASK; return val; case 0xd24: /* System Handler Control and State (SHCSR) */ + if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) { + goto bad_offset; + } val = 0; if (attrs.secure) { if (s->sec_vectors[ARMV7M_EXCP_MEM].active) { @@ -1322,6 +1325,10 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, cpu->env.v7m.scr[attrs.secure] = value; break; case 0xd14: /* Configuration Control. */ + if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) { + goto bad_offset; + } + /* Enforce RAZ/WI on reserved and must-RAZ/WI bits */ value &= (R_V7M_CCR_STKALIGN_MASK | R_V7M_CCR_BFHFNMIGN_MASK | @@ -1346,6 +1353,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, cpu->env.v7m.ccr[attrs.secure] = value; break; case 0xd24: /* System Handler Control and State (SHCSR) */ + if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) { + goto bad_offset; + } if (attrs.secure) { s->sec_vectors[ARMV7M_EXCP_MEM].active = (value & (1 << 0)) != 0; /* Secure HardFault active bit cannot be written */ diff --git a/target/arm/cpu.c b/target/arm/cpu.c index a914ce4e8c..3788cb773d 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -220,6 +220,11 @@ static void arm_cpu_reset(CPUState *s) env->v7m.aircr = R_V7M_AIRCR_BFHFNMINS_MASK; } + if (!arm_feature(env, ARM_FEATURE_V7)) { + env->v7m.ccr[M_REG_NS] = 0x3f8; + env->v7m.ccr[M_REG_S] = 0x3f8; + } + /* In v7M the reset value of this bit is IMPDEF, but ARM recommends * that it resets to 1, so QEMU always does that rather than making * it dependent on CPU model. In v8M it is RES1. @@ -230,6 +235,11 @@ static void arm_cpu_reset(CPUState *s) /* in v8M the NONBASETHRDENA bit [0] is RES1 */ env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_NONBASETHRDENA_MASK; env->v7m.ccr[M_REG_S] |= R_V7M_CCR_NONBASETHRDENA_MASK; + + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + env->v7m.ccr[M_REG_NS] |= R_V7M_CCR_UNALIGN_TRP_MASK; + env->v7m.ccr[M_REG_S] |= R_V7M_CCR_UNALIGN_TRP_MASK; + } } /* Unlike A/R profile, M profile defines the reset LR value */ diff --git a/target/arm/helper.c b/target/arm/helper.c index 2e45dda4e1..fdb481a51d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -10670,13 +10670,13 @@ 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) { + if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) { return; } env->v7m.basepri[M_REG_NS] = val & 0xff; return; case 0x93: /* FAULTMASK_NS */ - if (!env->v7m.secure) { + if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) { return; } env->v7m.faultmask[M_REG_NS] = val & 1; @@ -10760,9 +10760,15 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) env->v7m.primask[env->v7m.secure] = val & 1; break; case 17: /* BASEPRI */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } env->v7m.basepri[env->v7m.secure] = val & 0xff; break; case 18: /* BASEPRI_MAX */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } val &= 0xff; if (val != 0 && (val < env->v7m.basepri[env->v7m.secure] || env->v7m.basepri[env->v7m.secure] == 0)) { @@ -10770,6 +10776,9 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) } break; case 19: /* FAULTMASK */ + if (!arm_feature(env, ARM_FEATURE_M_MAIN)) { + goto bad_reg; + } env->v7m.faultmask[env->v7m.secure] = val & 1; break; case 20: /* CONTROL */
Forbid stack alignment change. (CCR) Reserve FAULTMASK, BASEPRI registers. Report any fault as HardFault. Disable MemManage, BusFault and UsageFault, so they always escalated to HardFault. (SHCSR) Signed-off-by: Julia Suvorova <jusual@mail.ru> --- This is the last cortex-m0 patch. hw/intc/armv7m_nvic.c | 10 ++++++++++ target/arm/cpu.c | 10 ++++++++++ target/arm/helper.c | 13 +++++++++++-- 3 files changed, 31 insertions(+), 2 deletions(-)