[Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Julia Suvorova]

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(-)

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 */
-- 
2.17.1

Re: [Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Julia Suvorova]

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.

Re: [Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] arm: Add ARMv6-M programmer's model support

[Julia Suvorova]

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.