[Qemu-devel] [PATCH 1/6] ARM: add Cortex-M3/M4 exception configuration and status registers

[Qemu-devel] [PATCH 1/6] ARM: add Cortex-M3/M4 exception configuration and status registers

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 target-arm/cpu.h     |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/machine.c |    6 ++++++
 2 files changed, 57 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 80297b3..1089f63 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -387,6 +387,12 @@ typedef struct CPUARMState {
         uint32_t control;
         int current_sp;
         int exception;
+        uint32_t ccr;
+        uint32_t cfsr;
+        uint32_t hfsr;
+        uint32_t dfsr;
+        uint32_t mmfar;
+        uint32_t bfar;
     } v7m;
 
     /* Information associated with an exception about to be taken:
@@ -852,6 +858,51 @@ enum arm_cpu_mode {
 #define ARM_IWMMXT_wCGR2	10
 #define ARM_IWMMXT_wCGR3	11
 
+/* V7M CCSR bits */
+#define CCR_STKALIGN        0x00000200
+#define CCR_BFHFNMIGN       0x00000100
+#define CCR_DIV_0_TRP       0x00000010
+#define CCR_UNALIGN_TRP     0x00000008
+#define CCR_USERSETMPEND    0x00000002
+#define CCR_NONBASETHRDENA  0x00000001
+
+/* V7M CFSR bits for UFSR */
+#define CFSR_DIVBYZERO      0x02000000
+#define CFSR_UNALIGNED      0x01000000
+#define CFSR_NOCP           0x00080000
+#define CFSR_INVPC          0x00040000
+#define CFSR_INVSTATE       0x00020000
+#define CFSR_UNDEFINSTR     0x00010000
+
+/* V7M CFSR bits for BFSR */
+#define CFSR_BFARVALID      0x00008000
+#define CFSR_LSPERR         0x00002000
+#define CFSR_STKERR         0x00001000
+#define CFSR_UNSTKERR       0x00000800
+#define CFSR_IMPRECISERR    0x00000400
+#define CFSR_PRECISERR      0x00000200
+#define CFSR_IBUSERR        0x00000100
+
+/* V7M CFSR bits for MMFSR */
+#define CFSR_MMARVALID      0x00000080
+#define CFSR_MLSPERR        0x00000020
+#define CFSR_MSTKERR        0x00000010
+#define CFSR_MUNSTKERR      0x00000008
+#define CFSR_DACCVIOL       0x00000002
+#define CFSR_IACCVIOL       0x00000001
+
+/* V7M HFSR bits */
+#define HFSR_DEBUG_VT       0x80000000
+#define HFSR_FORCED         0x40000000
+#define HFSR_VECTTBL        0x00000002
+
+/* V7M DFSR bits */
+#define DFSR_EXTERNAL       0x00000010
+#define DFSR_VCATCH         0x00000008
+#define DFSR_DWTTRAP        0x00000004
+#define DFSR_BKPT           0x00000002
+#define DFSR_HALTED         0x00000001
+
 /* If adding a feature bit which corresponds to a Linux ELF
  * HWCAP bit, remember to update the feature-bit-to-hwcap
  * mapping in linux-user/elfload.c:get_elf_hwcap().
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 9eb51df..11dcf29 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -101,6 +101,12 @@ static const VMStateDescription vmstate_m = {
         VMSTATE_UINT32(env.v7m.control, ARMCPU),
         VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
+        VMSTATE_UINT32(env.v7m.ccr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
+        VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
+        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/6] ARM: add Cortex-M3/M4 exception configuration and status registers

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  target-arm/cpu.h     |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/machine.c |    6 ++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 80297b3..1089f63 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -387,6 +387,12 @@ typedef struct CPUARMState {
>          uint32_t control;
>          int current_sp;
>          int exception;
> +        uint32_t ccr;
> +        uint32_t cfsr;
> +        uint32_t hfsr;
> +        uint32_t dfsr;
> +        uint32_t mmfar;
> +        uint32_t bfar;
>      } v7m;
>
>      /* Information associated with an exception about to be taken:
> @@ -852,6 +858,51 @@ enum arm_cpu_mode {
>  #define ARM_IWMMXT_wCGR2       10
>  #define ARM_IWMMXT_wCGR3       11
>
> +/* V7M CCSR bits */

Typo, should read "CCR".

> +#define CCR_STKALIGN        0x00000200
> +#define CCR_BFHFNMIGN       0x00000100
> +#define CCR_DIV_0_TRP       0x00000010
> +#define CCR_UNALIGN_TRP     0x00000008
> +#define CCR_USERSETMPEND    0x00000002
> +#define CCR_NONBASETHRDENA  0x00000001
> +
> +/* V7M CFSR bits for UFSR */
> +#define CFSR_DIVBYZERO      0x02000000
> +#define CFSR_UNALIGNED      0x01000000
> +#define CFSR_NOCP           0x00080000
> +#define CFSR_INVPC          0x00040000
> +#define CFSR_INVSTATE       0x00020000
> +#define CFSR_UNDEFINSTR     0x00010000
> +
> +/* V7M CFSR bits for BFSR */
> +#define CFSR_BFARVALID      0x00008000
> +#define CFSR_LSPERR         0x00002000
> +#define CFSR_STKERR         0x00001000
> +#define CFSR_UNSTKERR       0x00000800
> +#define CFSR_IMPRECISERR    0x00000400
> +#define CFSR_PRECISERR      0x00000200
> +#define CFSR_IBUSERR        0x00000100
> +
> +/* V7M CFSR bits for MMFSR */
> +#define CFSR_MMARVALID      0x00000080
> +#define CFSR_MLSPERR        0x00000020
> +#define CFSR_MSTKERR        0x00000010
> +#define CFSR_MUNSTKERR      0x00000008
> +#define CFSR_DACCVIOL       0x00000002
> +#define CFSR_IACCVIOL       0x00000001
> +
> +/* V7M HFSR bits */
> +#define HFSR_DEBUG_VT       0x80000000

This bit is DEBUGEVT, not DEBUG_VT.

> +#define HFSR_FORCED         0x40000000
> +#define HFSR_VECTTBL        0x00000002
> +
> +/* V7M DFSR bits */
> +#define DFSR_EXTERNAL       0x00000010
> +#define DFSR_VCATCH         0x00000008
> +#define DFSR_DWTTRAP        0x00000004
> +#define DFSR_BKPT           0x00000002
> +#define DFSR_HALTED         0x00000001
> +
>  /* If adding a feature bit which corresponds to a Linux ELF
>   * HWCAP bit, remember to update the feature-bit-to-hwcap
>   * mapping in linux-user/elfload.c:get_elf_hwcap().
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9eb51df..11dcf29 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -101,6 +101,12 @@ static const VMStateDescription vmstate_m = {
>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
>          VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.ccr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
>          VMSTATE_END_OF_LIST()

If you're adding entries to this VMStateDescription you
also need to increment its version_id and minimum_version_id
fields.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/6] ARM: add Cortex-M3/M4 exception configuration and status registers

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  target-arm/cpu.h     |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/machine.c |    6 ++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 80297b3..1089f63 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -387,6 +387,12 @@ typedef struct CPUARMState {
>          uint32_t control;
>          int current_sp;
>          int exception;
> +        uint32_t ccr;
> +        uint32_t cfsr;
> +        uint32_t hfsr;
> +        uint32_t dfsr;
> +        uint32_t mmfar;
> +        uint32_t bfar;
>      } v7m;

Also, the reset value of the CCR (for M3 and M4, at least) is
0x00000200, not 0, so you need to put something in arm_cpu_reset().
Otherwise when you wire up the behaviour of CCR.STACKALIGN in
the later patch we'll break for guests which don't manually
set the bit but rely on it being set.

(Strictly speaking the reset value of the bit is IMPDEF, as
is whether it's RW or RO, but we can leave it common to all
the M profile cores we emulate until we add a core which
needs it to behave differently.)

thanks
-- PMM

[Qemu-devel] [PATCH 2/6] ARM: accessors to Cortex-M3/M4 exception configuration and status registers

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 hw/intc/armv7m_nvic.c |   70 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 9 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index e13b729..e6ae047 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -225,8 +225,8 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         /* TODO: Implement SLEEPONEXIT.  */
         return 0;
     case 0xd14: /* Configuration Control.  */
-        /* TODO: Implement Configuration Control bits.  */
-        return 0;
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.ccr;
     case 0xd24: /* System Handler Status.  */
         val = 0;
         if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
@@ -245,16 +245,23 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
         return val;
     case 0xd28: /* Configurable Fault Status.  */
-        /* TODO: Implement Fault Status.  */
-        qemu_log_mask(LOG_UNIMP, "Configurable Fault Status unimplemented\n");
-        return 0;
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.cfsr;
     case 0xd2c: /* Hard Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.hfsr;
     case 0xd30: /* Debug Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.dfsr;
     case 0xd34: /* Mem Manage Address.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.mmfar;
     case 0xd38: /* Bus Fault Address.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.bfar;
     case 0xd3c: /* Aux Fault Status.  */
         /* TODO: Implement fault status registers.  */
-        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
+        qemu_log_mask(LOG_UNIMP, "AUX fault status registers unimplemented\n");
         return 0;
     case 0xd40: /* PFR0.  */
         return 0x00000030;
@@ -361,9 +368,12 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         }
         break;
     case 0xd10: /* System Control.  */
-    case 0xd14: /* Configuration Control.  */
         /* TODO: Implement control registers.  */
-        qemu_log_mask(LOG_UNIMP, "NVIC: SCR and CCR unimplemented\n");
+        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
+        break;
+    case 0xd14: /* Configuration Control.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.ccr = value & 0; /* TODO: add used bits */
         break;
     case 0xd24: /* System Handler Control.  */
         /* TODO: Real hardware allows you to set/clear the active bits
@@ -373,13 +383,28 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
         break;
     case 0xd28: /* Configurable Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.cfsr &= ~value;
+        break;
     case 0xd2c: /* Hard Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.hfsr &= ~value;
+        break;
     case 0xd30: /* Debug Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.dfsr &= ~value;
+        break;
     case 0xd34: /* Mem Manage Address.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.mmfar = value;
+        break;
     case 0xd38: /* Bus Fault Address.  */
+        cpu = ARM_CPU(current_cpu);
+        cpu->env.v7m.bfar = value;
+        break;
     case 0xd3c: /* Aux Fault Status.  */
         qemu_log_mask(LOG_UNIMP,
-                      "NVIC: fault status registers unimplemented\n");
+                      "NVIC: AUX fault status registers unimplemented\n");
         break;
     case 0xf00: /* Software Triggered Interrupt Register */
         if ((value & 0x1ff) < s->num_irq) {
@@ -399,6 +424,7 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
     uint32_t offset = addr;
     int i;
     uint32_t val;
+    ARMCPU *cpu;
 
     switch (offset) {
     case 0xd18 ... 0xd23: /* System Handler Priority.  */
@@ -407,6 +433,18 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
             val |= s->gic.priority1[(offset - 0xd14) + i][0] << (i * 8);
         }
         return val;
+    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
+        cpu = ARM_CPU(current_cpu);
+        val = cpu->env.v7m.cfsr;
+        if (size == 1) {
+            val >>= (offset - 0xd28) * 8;
+            return val & 0xff;
+        }
+        if ((size == 2) && ((offset & 1) == 0)) {
+            val >>= (offset - 0xd28) * 8;
+            return val & 0xffff;
+        }
+        break;
     case 0xfe0 ... 0xfff: /* ID.  */
         if (offset & 3) {
             return 0;
@@ -436,6 +474,20 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
         }
         gic_update(&s->gic);
         return;
+    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
+        if (size == 1) {
+            value <<= (offset - 0xd28) * 8;
+            offset &= ~3;
+            size = 4;
+            break;
+        }
+        if ((size == 2) && ((offset & 1) == 0)) {
+            value <<= (offset - 0xd28) * 8;
+            offset &= ~3;
+            size = 4;
+            break;
+        }
+        break;
     }
     if (size == 4) {
         nvic_writel(s, offset, value);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/6] ARM: accessors to Cortex-M3/M4 exception configuration and status registers

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  hw/intc/armv7m_nvic.c |   70 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index e13b729..e6ae047 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -225,8 +225,8 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          /* TODO: Implement SLEEPONEXIT.  */
>          return 0;
>      case 0xd14: /* Configuration Control.  */
> -        /* TODO: Implement Configuration Control bits.  */
> -        return 0;
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.ccr;
>      case 0xd24: /* System Handler Status.  */
>          val = 0;
>          if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
> @@ -245,16 +245,23 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
>          return val;
>      case 0xd28: /* Configurable Fault Status.  */
> -        /* TODO: Implement Fault Status.  */
> -        qemu_log_mask(LOG_UNIMP, "Configurable Fault Status unimplemented\n");
> -        return 0;
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.cfsr;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.dfsr;
>      case 0xd34: /* Mem Manage Address.  */

This is MemManage Fault Address.

> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.mmfar;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.bfar;


>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> -        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP, "AUX fault status registers unimplemented\n");

"register".

>          return 0;
>      case 0xd40: /* PFR0.  */
>          return 0x00000030;
> @@ -361,9 +368,12 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          }
>          break;
>      case 0xd10: /* System Control.  */
> -    case 0xd14: /* Configuration Control.  */
>          /* TODO: Implement control registers.  */
> -        qemu_log_mask(LOG_UNIMP, "NVIC: SCR and CCR unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
> +        break;
> +    case 0xd14: /* Configuration Control.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.ccr = value & 0; /* TODO: add used bits */

This is weird and the compiler is likely to complain about that & 0.
Just let the guest write the value it wants to write. (You can
mask off the reserved bits if you like but you don't have to.)

>          break;
>      case 0xd24: /* System Handler Control.  */
>          /* TODO: Real hardware allows you to set/clear the active bits
> @@ -373,13 +383,28 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 0;
>          break;
>      case 0xd28: /* Configurable Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.cfsr &= ~value;
> +        break;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.hfsr &= ~value;
> +        break;
>      case 0xd30: /* Debug Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.dfsr &= ~value;
> +        break;
>      case 0xd34: /* Mem Manage Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.mmfar = value;
> +        break;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.bfar = value;
> +        break;
>      case 0xd3c: /* Aux Fault Status.  */
>          qemu_log_mask(LOG_UNIMP,
> -                      "NVIC: fault status registers unimplemented\n");
> +                      "NVIC: AUX fault status registers unimplemented\n");
>          break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < s->num_irq) {
> @@ -399,6 +424,7 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>      uint32_t offset = addr;
>      int i;
>      uint32_t val;
> +    ARMCPU *cpu;
>
>      switch (offset) {
>      case 0xd18 ... 0xd23: /* System Handler Priority.  */
> @@ -407,6 +433,18 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>              val |= s->gic.priority1[(offset - 0xd14) + i][0] << (i * 8);
>          }
>          return val;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        val = cpu->env.v7m.cfsr;
> +        if (size == 1) {
> +            val >>= (offset - 0xd28) * 8;
> +            return val & 0xff;
> +        }
> +        if ((size == 2) && ((offset & 1) == 0)) {
> +            val >>= (offset - 0xd28) * 8;
> +            return val & 0xffff;
> +        }
> +        break;

You can replace all this with:

    return extract32(cpu->env.v7m.cfsr, (offset - 0xd28) * 8, size * 8);

>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              return 0;
> @@ -436,6 +474,20 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>          }
>          gic_update(&s->gic);
>          return;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
> +        if (size == 1) {
> +            value <<= (offset - 0xd28) * 8;
> +            offset &= ~3;
> +            size = 4;
> +            break;
> +        }
> +        if ((size == 2) && ((offset & 1) == 0)) {
> +            value <<= (offset - 0xd28) * 8;
> +            offset &= ~3;
> +            size = 4;
> +            break;
> +        }
> +        break;

Both these if blocks have identical contents, and the
"break"s are unnecessary because we're about to do a
break anyway. This could also use a comment saying that
we can expand the write to a a 32-bit write because the
register has write-1-to-clear semantics.

>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value);
> --
> 1.7.9.5
>

thanks
-- PMM

[Qemu-devel] [PATCH 3/6] ARM: Cortex-M3/M4: honor STKALIGN in CCR

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 hw/intc/armv7m_nvic.c |    2 +-
 target-arm/helper.c   |    5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index e6ae047..369ef94 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -373,7 +373,7 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         break;
     case 0xd14: /* Configuration Control.  */
         cpu = ARM_CPU(current_cpu);
-        cpu->env.v7m.ccr = value & 0; /* TODO: add used bits */
+        cpu->env.v7m.ccr = value & CCR_STKALIGN;
         break;
     case 0xd24: /* System Handler Control.  */
         /* TODO: Real hardware allows you to set/clear the active bits
diff --git a/target-arm/helper.c b/target-arm/helper.c
index aa34159..812204f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4579,9 +4579,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     }
 
     /* Align stack pointer.  */
-    /* ??? Should only do this if Configuration Control Register
-       STACKALIGN bit is set.  */
-    if (env->regs[13] & 4) {
+    /* Do this only if Configuration Control Register STKALIGN bit is set. */
+    if ((env->v7m.ccr & CCR_STKALIGN) && (env->regs[13] & 4)) {
         env->regs[13] -= 4;
         xpsr |= 0x200;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/6] ARM: Cortex-M3/M4: honor STKALIGN in CCR

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  hw/intc/armv7m_nvic.c |    2 +-
>  target-arm/helper.c   |    5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index e6ae047..369ef94 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -373,7 +373,7 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          break;
>      case 0xd14: /* Configuration Control.  */
>          cpu = ARM_CPU(current_cpu);
> -        cpu->env.v7m.ccr = value & 0; /* TODO: add used bits */
> +        cpu->env.v7m.ccr = value & CCR_STKALIGN;

You can drop this part (see remarks on previous patch).

>          break;
>      case 0xd24: /* System Handler Control.  */
>          /* TODO: Real hardware allows you to set/clear the active bits
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index aa34159..812204f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4579,9 +4579,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      }
>
>      /* Align stack pointer.  */
> -    /* ??? Should only do this if Configuration Control Register
> -       STACKALIGN bit is set.  */
> -    if (env->regs[13] & 4) {
> +    /* Do this only if Configuration Control Register STKALIGN bit is set. */

You don't need this comment, it doesn't tell us anything that's
not obvious from the code.

> +    if ((env->v7m.ccr & CCR_STKALIGN) && (env->regs[13] & 4)) {
>          env->regs[13] -= 4;
>          xpsr |= 0x200;
>      }

thanks
-- PMM

[Qemu-devel] [PATCH 4/6] ARM: Cortex-M3/M4: on exception, set basic bits in exhandling registers

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 target-arm/helper.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 812204f..555bc5f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4541,6 +4541,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
        one we're raising.  */
     switch (cs->exception_index) {
     case EXCP_UDEF:
+        env->v7m.cfsr |= CFSR_UNDEFINSTR;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
         return;
     case EXCP_SWI:
@@ -4549,9 +4550,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return;
     case EXCP_PREFETCH_ABORT:
     case EXCP_DATA_ABORT:
-        /* TODO: if we implemented the MPU registers, this is where we
-         * should set the MMFAR, etc from exception.fsr and exception.vaddress.
-         */
+        env->v7m.mmfar = env->exception.vaddress;
+        env->v7m.cfsr |= CFSR_MMARVALID;
+        /* TODO: further decoding of exception.fsr */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
         return;
     case EXCP_BKPT:
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/6] ARM: Cortex-M3/M4: on exception, set basic bits in exhandling registers

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  target-arm/helper.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 812204f..555bc5f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4541,6 +4541,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>         one we're raising.  */
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
> +        env->v7m.cfsr |= CFSR_UNDEFINSTR;

I think you can also get here for attempts to use the
(nonexistent on M3) coprocessor instructions. Those should
set CFSR_NOCP, not CFSR_UNDEFINSTR.

>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>          return;
>      case EXCP_SWI:
> @@ -4549,9 +4550,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          return;
>      case EXCP_PREFETCH_ABORT:
>      case EXCP_DATA_ABORT:
> -        /* TODO: if we implemented the MPU registers, this is where we
> -         * should set the MMFAR, etc from exception.fsr and exception.vaddress.
> -         */
> +        env->v7m.mmfar = env->exception.vaddress;
> +        env->v7m.cfsr |= CFSR_MMARVALID;

Similarly, this can happen for both an MPU fault and a
BusFault, and the set of registers you need to update is different.
If you can't get enough information out of the exception.fsr
to determine the difference, you'll need to capture it earlier
(where we do for the other exception.* fields).

> +        /* TODO: further decoding of exception.fsr */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
>          return;
>      case EXCP_BKPT:
> --
> 1.7.9.5
>

thanks
-- PMM

[Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 hw/arm/armv7m.c     |   17 ++++++++++++++++-
 target-arm/cpu.c    |    2 ++
 target-arm/helper.c |   30 ++++++++++++++++++++++++++++--
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index c6eab6d..db6bc3c 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -179,15 +179,30 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
     int i;
     int big_endian;
     MemoryRegion *hack = g_new(MemoryRegion, 1);
+    ObjectClass *cpu_oc;
+    Error *err = NULL;
 
     if (cpu_model == NULL) {
 	cpu_model = "cortex-m3";
     }
-    cpu = cpu_arm_init(cpu_model);
+    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
+    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
+    /* On Cortex-M3/M4, the MPU has 8 windows */
+    object_property_set_int(OBJECT(cpu), 8, "pmsav7-dregion", &err);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
+
     env = &cpu->env;
 
     armv7m_bitband_init();
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 80669a6..d8cfbb1 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -832,6 +832,7 @@ static void cortex_m3_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
     cpu->midr = 0x410fc231;
 }
 
@@ -841,6 +842,7 @@ static void cortex_m4_initfn(Object *obj)
 
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_M);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
     set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
     cpu->midr = 0x410fc240; /* r0p0 */
 }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 555bc5f..637dbf6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5854,6 +5854,26 @@ static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
 
 }
 
+static inline void get_phys_addr_v7m_default(CPUARMState *env,
+                                             ARMMMUIdx mmu_idx,
+                                             int32_t address, int *prot)
+{
+    *prot = PAGE_READ | PAGE_WRITE;
+    switch (address) {
+    case 0xFFFFF000 ... 0xFFFFFFFF:
+        /* the special exception return address memory region is EXEC only */
+        *prot = PAGE_EXEC;
+        break;
+
+    case 0x00000000 ... 0x1FFFFFFF:
+    case 0x20000000 ... 0x3FFFFFFF:
+    case 0x60000000 ... 0x7FFFFFFF:
+    case 0x80000000 ... 0x9FFFFFFF:
+        *prot |= PAGE_EXEC;
+        break;
+    }
+}
+
 static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                                  int access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
@@ -5866,7 +5886,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
     *prot = 0;
 
     if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
-        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+        if (arm_feature(env, ARM_FEATURE_M))
+            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
+        else
+            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
     } else { /* MPU enabled */
         for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
             /* region search */
@@ -5944,7 +5967,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 *fsr = 0;
                 return true;
             }
-            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+            if (arm_feature(env, ARM_FEATURE_M))
+                get_phys_addr_v7m_default(env, mmu_idx, address, prot);
+            else
+                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
         } else { /* a MPU hit! */
             uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4

[Peter Crosthwaite]

On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>
> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  hw/arm/armv7m.c     |   17 ++++++++++++++++-
>  target-arm/cpu.c    |    2 ++
>  target-arm/helper.c |   30 ++++++++++++++++++++++++++++--
>  3 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index c6eab6d..db6bc3c 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -179,15 +179,30 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      int i;
>      int big_endian;
>      MemoryRegion *hack = g_new(MemoryRegion, 1);
> +    ObjectClass *cpu_oc;
> +    Error *err = NULL;
>
>      if (cpu_model == NULL) {
>         cpu_model = "cortex-m3";
>      }
> -    cpu = cpu_arm_init(cpu_model);
> +    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
> +    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));

Is this change in scope of the patch? why did you need it?

>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find CPU definition\n");
>          exit(1);
>      }
> +    /* On Cortex-M3/M4, the MPU has 8 windows */
> +    object_property_set_int(OBJECT(cpu), 8, "pmsav7-dregion", &err);
> +    if (err) {
> +        error_report_err(err);
> +        exit(1);
> +    }
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> +    if (err) {
> +        error_report_err(err);
> +        exit(1);
> +    }
> +
>      env = &cpu->env;
>
>      armv7m_bitband_init();
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 80669a6..d8cfbb1 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -832,6 +832,7 @@ static void cortex_m3_initfn(Object *obj)
>      ARMCPU *cpu = ARM_CPU(obj);
>      set_feature(&cpu->env, ARM_FEATURE_V7);
>      set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>      cpu->midr = 0x410fc231;
>  }
>
> @@ -841,6 +842,7 @@ static void cortex_m4_initfn(Object *obj)
>
>      set_feature(&cpu->env, ARM_FEATURE_V7);
>      set_feature(&cpu->env, ARM_FEATURE_M);
> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>      cpu->midr = 0x410fc240; /* r0p0 */
>  }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 555bc5f..637dbf6 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5854,6 +5854,26 @@ static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
>
>  }
>
> +static inline void get_phys_addr_v7m_default(CPUARMState *env,
> +                                             ARMMMUIdx mmu_idx,
> +                                             int32_t address, int *prot)

The fn name should include something about mpu or pmsa. v7m
unqualified is a little general. Does the V7M doc use "PMSA" or is
that an AR profile thing?

> +{
> +    *prot = PAGE_READ | PAGE_WRITE;
> +    switch (address) {
> +    case 0xFFFFF000 ... 0xFFFFFFFF:
> +        /* the special exception return address memory region is EXEC only */
> +        *prot = PAGE_EXEC;
> +        break;
> +
> +    case 0x00000000 ... 0x1FFFFFFF:
> +    case 0x20000000 ... 0x3FFFFFFF:
> +    case 0x60000000 ... 0x7FFFFFFF:
> +    case 0x80000000 ... 0x9FFFFFFF:
> +        *prot |= PAGE_EXEC;
> +        break;
> +    }
> +}
> +
>  static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                                   int access_type, ARMMMUIdx mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> @@ -5866,7 +5886,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>      *prot = 0;
>
>      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> -        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +        if (arm_feature(env, ARM_FEATURE_M))

Single line ifs still require { by coding style. scripts/checkpatch.pl
will catch these.

> +            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
> +        else
> +            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>      } else { /* MPU enabled */
>          for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
>              /* region search */
> @@ -5944,7 +5967,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                  *fsr = 0;
>                  return true;
>              }
> -            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +            if (arm_feature(env, ARM_FEATURE_M))
> +                get_phys_addr_v7m_default(env, mmu_idx, address, prot);
> +            else
> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);

This if-else can be consolidated with something like:

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 63f7e7b..bfe0afb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6573,10 +6573,9 @@ static int get_phys_addr_pmsav7(CPUARMState
*env, uint32_t address,
     int n;
     *phys_ptr = address;
     *prot = 0;
+    bool do_default = true;

-    if (!(sctlr & 0x1)) { /* MPU disabled */
-        get_phys_addr_pmsav7_default(env, address, prot);
-    } else { /* MPU enabled */
+    if (sctlr & 0x1) { /* MPU enabled */
         for (n = 15; n >= 0; n--) { /*region search */
             uint32_t base = env->cp15.c6_drbar[n];
             uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
@@ -6609,12 +6608,12 @@ static int get_phys_addr_pmsav7(CPUARMState
*env, uint32_t address,
             if (is_user || !(sctlr & 1 << 17)) { /* BR */
                 /* background fault */
                 return -1;
-            } else {
-                get_phys_addr_pmsav7_default(env, address, prot);
             }
         } else { /* a MPU hit! */
             uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);

+            do_default = false;
+
             if (is_user) { /* User mode AP bit decoding */
                 switch (ap) {
                 case 0:
@@ -6656,6 +6655,14 @@ static int get_phys_addr_pmsav7(CPUARMState
*env, uint32_t address,
         }
     }

+    if (do_default) {
+        if (arm_feature(env, ARM_FEATURE_M)) {
+            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
+        } else {
+            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+        }
+    }
+
     switch (access_type) {
     case 0:
         return *prot & PAGE_READ ? 0 : 0x405;

Regards,
Peter


>          } else { /* a MPU hit! */
>              uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
>
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4

[Alex Züpke]

Am 07.07.2015 um 22:50 schrieb Peter Crosthwaite:
> On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>>
>> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
>> ---
>>  hw/arm/armv7m.c     |   17 ++++++++++++++++-
>>  target-arm/cpu.c    |    2 ++
>>  target-arm/helper.c |   30 ++++++++++++++++++++++++++++--
>>  3 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
>> index c6eab6d..db6bc3c 100644
>> --- a/hw/arm/armv7m.c
>> +++ b/hw/arm/armv7m.c
>> @@ -179,15 +179,30 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>>      int i;
>>      int big_endian;
>>      MemoryRegion *hack = g_new(MemoryRegion, 1);
>> +    ObjectClass *cpu_oc;
>> +    Error *err = NULL;
>>
>>      if (cpu_model == NULL) {
>>         cpu_model = "cortex-m3";
>>      }
>> -    cpu = cpu_arm_init(cpu_model);
>> +    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
>> +    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
> 
> Is this change in scope of the patch? why did you need it?

I found no other way than this to change the "pmsav7-dregion" property from its default value.
Depending on this property, the existing constructors allocate memory for MPU window handling internally.

>>      if (cpu == NULL) {
>>          fprintf(stderr, "Unable to find CPU definition\n");
>>          exit(1);
>>      }
>> +    /* On Cortex-M3/M4, the MPU has 8 windows */
>> +    object_property_set_int(OBJECT(cpu), 8, "pmsav7-dregion", &err);
>> +    if (err) {
>> +        error_report_err(err);
>> +        exit(1);
>> +    }
>> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>> +    if (err) {
>> +        error_report_err(err);
>> +        exit(1);
>> +    }
>> +
>>      env = &cpu->env;
>>
>>      armv7m_bitband_init();
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 80669a6..d8cfbb1 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -832,6 +832,7 @@ static void cortex_m3_initfn(Object *obj)
>>      ARMCPU *cpu = ARM_CPU(obj);
>>      set_feature(&cpu->env, ARM_FEATURE_V7);
>>      set_feature(&cpu->env, ARM_FEATURE_M);
>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>>      cpu->midr = 0x410fc231;
>>  }
>>
>> @@ -841,6 +842,7 @@ static void cortex_m4_initfn(Object *obj)
>>
>>      set_feature(&cpu->env, ARM_FEATURE_V7);
>>      set_feature(&cpu->env, ARM_FEATURE_M);
>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>>      cpu->midr = 0x410fc240; /* r0p0 */
>>  }
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 555bc5f..637dbf6 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -5854,6 +5854,26 @@ static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
>>
>>  }
>>
>> +static inline void get_phys_addr_v7m_default(CPUARMState *env,
>> +                                             ARMMMUIdx mmu_idx,
>> +                                             int32_t address, int *prot)
> 
> The fn name should include something about mpu or pmsa. v7m
> unqualified is a little general. Does the V7M doc use "PMSA" or is
> that an AR profile thing?

The ARMv7-M docs describe the MPU as a PMSAv7 one, but the interface is different to the specification
in the ARMv7-A/M manual. Since the existing code already used a "v7m" prefix for v7-M, I tried to stick with that.

The original get_phys_add _pmsav7_default() function describes the default memory map for ARMv7-R CPUs,
so we should probably rename this function to get_phys_addr_v7r_default()? Is it OK to introduce "v7r"?


> 
>> +{
>> +    *prot = PAGE_READ | PAGE_WRITE;
>> +    switch (address) {
>> +    case 0xFFFFF000 ... 0xFFFFFFFF:
>> +        /* the special exception return address memory region is EXEC only */
>> +        *prot = PAGE_EXEC;
>> +        break;
>> +
>> +    case 0x00000000 ... 0x1FFFFFFF:
>> +    case 0x20000000 ... 0x3FFFFFFF:
>> +    case 0x60000000 ... 0x7FFFFFFF:
>> +    case 0x80000000 ... 0x9FFFFFFF:
>> +        *prot |= PAGE_EXEC;
>> +        break;
>> +    }
>> +}
>> +
>>  static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>                                   int access_type, ARMMMUIdx mmu_idx,
>>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>> @@ -5866,7 +5886,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>      *prot = 0;
>>
>>      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
>> -        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>> +        if (arm_feature(env, ARM_FEATURE_M))
> 
> Single line ifs still require { by coding style. scripts/checkpatch.pl
> will catch these.

OK.

> 
>> +            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
>> +        else
>> +            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>      } else { /* MPU enabled */
>>          for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
>>              /* region search */
>> @@ -5944,7 +5967,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>                  *fsr = 0;
>>                  return true;
>>              }
>> -            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>> +            if (arm_feature(env, ARM_FEATURE_M))
>> +                get_phys_addr_v7m_default(env, mmu_idx, address, prot);
>> +            else
>> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> 
> This if-else can be consolidated with something like:

Will do, thanks a lot!

Best regards
Alex

> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 63f7e7b..bfe0afb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6573,10 +6573,9 @@ static int get_phys_addr_pmsav7(CPUARMState
> *env, uint32_t address,
>      int n;
>      *phys_ptr = address;
>      *prot = 0;
> +    bool do_default = true;
> 
> -    if (!(sctlr & 0x1)) { /* MPU disabled */
> -        get_phys_addr_pmsav7_default(env, address, prot);
> -    } else { /* MPU enabled */
> +    if (sctlr & 0x1) { /* MPU enabled */
>          for (n = 15; n >= 0; n--) { /*region search */
>              uint32_t base = env->cp15.c6_drbar[n];
>              uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
> @@ -6609,12 +6608,12 @@ static int get_phys_addr_pmsav7(CPUARMState
> *env, uint32_t address,
>              if (is_user || !(sctlr & 1 << 17)) { /* BR */
>                  /* background fault */
>                  return -1;
> -            } else {
> -                get_phys_addr_pmsav7_default(env, address, prot);
>              }
>          } else { /* a MPU hit! */
>              uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
> 
> +            do_default = false;
> +
>              if (is_user) { /* User mode AP bit decoding */
>                  switch (ap) {
>                  case 0:
> @@ -6656,6 +6655,14 @@ static int get_phys_addr_pmsav7(CPUARMState
> *env, uint32_t address,
>          }
>      }
> 
> +    if (do_default) {
> +        if (arm_feature(env, ARM_FEATURE_M)) {
> +            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
> +        } else {
> +            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +        }
> +    }
> +
>      switch (access_type) {
>      case 0:
>          return *prot & PAGE_READ ? 0 : 0x405;
> 
> Regards,
> Peter
> 
> 
>>          } else { /* a MPU hit! */
>>              uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
>>
>> --
>> 1.7.9.5
>>
>>

Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4

[Peter Crosthwaite]

On Wed, Jul 8, 2015 at 12:56 AM, Alex Züpke <alexander.zuepke@hs-rm.de> wrote:
> Am 07.07.2015 um 22:50 schrieb Peter Crosthwaite:
>> On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
>>>
>>> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
>>> ---
>>>  hw/arm/armv7m.c     |   17 ++++++++++++++++-
>>>  target-arm/cpu.c    |    2 ++
>>>  target-arm/helper.c |   30 ++++++++++++++++++++++++++++--
>>>  3 files changed, 46 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
>>> index c6eab6d..db6bc3c 100644
>>> --- a/hw/arm/armv7m.c
>>> +++ b/hw/arm/armv7m.c
>>> @@ -179,15 +179,30 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>>>      int i;
>>>      int big_endian;
>>>      MemoryRegion *hack = g_new(MemoryRegion, 1);
>>> +    ObjectClass *cpu_oc;
>>> +    Error *err = NULL;
>>>
>>>      if (cpu_model == NULL) {
>>>         cpu_model = "cortex-m3";
>>>      }
>>> -    cpu = cpu_arm_init(cpu_model);
>>> +    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
>>> +    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
>>
>> Is this change in scope of the patch? why did you need it?
>
> I found no other way than this to change the "pmsav7-dregion" property from its default value.
> Depending on this property, the existing constructors allocate memory for MPU window handling internally.
>
>>>      if (cpu == NULL) {
>>>          fprintf(stderr, "Unable to find CPU definition\n");
>>>          exit(1);
>>>      }
>>> +    /* On Cortex-M3/M4, the MPU has 8 windows */
>>> +    object_property_set_int(OBJECT(cpu), 8, "pmsav7-dregion", &err);
>>> +    if (err) {
>>> +        error_report_err(err);
>>> +        exit(1);
>>> +    }
>>> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>>> +    if (err) {
>>> +        error_report_err(err);
>>> +        exit(1);
>>> +    }
>>> +
>>>      env = &cpu->env;
>>>
>>>      armv7m_bitband_init();
>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>> index 80669a6..d8cfbb1 100644
>>> --- a/target-arm/cpu.c
>>> +++ b/target-arm/cpu.c
>>> @@ -832,6 +832,7 @@ static void cortex_m3_initfn(Object *obj)
>>>      ARMCPU *cpu = ARM_CPU(obj);
>>>      set_feature(&cpu->env, ARM_FEATURE_V7);
>>>      set_feature(&cpu->env, ARM_FEATURE_M);
>>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>>>      cpu->midr = 0x410fc231;
>>>  }
>>>
>>> @@ -841,6 +842,7 @@ static void cortex_m4_initfn(Object *obj)
>>>
>>>      set_feature(&cpu->env, ARM_FEATURE_V7);
>>>      set_feature(&cpu->env, ARM_FEATURE_M);
>>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>>>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>>>      cpu->midr = 0x410fc240; /* r0p0 */
>>>  }
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 555bc5f..637dbf6 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -5854,6 +5854,26 @@ static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
>>>
>>>  }
>>>
>>> +static inline void get_phys_addr_v7m_default(CPUARMState *env,
>>> +                                             ARMMMUIdx mmu_idx,
>>> +                                             int32_t address, int *prot)
>>
>> The fn name should include something about mpu or pmsa. v7m
>> unqualified is a little general. Does the V7M doc use "PMSA" or is
>> that an AR profile thing?
>
> The ARMv7-M docs describe the MPU as a PMSAv7 one, but the interface is different to the specification
> in the ARMv7-A/M manual. Since the existing code already used a "v7m" prefix for v7-M, I tried to stick with that.
>
> The original get_phys_add _pmsav7_default() function describes the default memory map for ARMv7-R CPUs,
> so we should probably rename this function to get_phys_addr_v7r_default()? Is it OK to introduce "v7r"?
>

Yes, But we need to keep the "PMSA" in both R and M variants to make
it clear it is about MPU.

get_phys_add _pmsav7r_default()
get_phys_add _pmsav7m_default()

Regards,
Peter

>
>>
>>> +{
>>> +    *prot = PAGE_READ | PAGE_WRITE;
>>> +    switch (address) {
>>> +    case 0xFFFFF000 ... 0xFFFFFFFF:
>>> +        /* the special exception return address memory region is EXEC only */
>>> +        *prot = PAGE_EXEC;
>>> +        break;
>>> +
>>> +    case 0x00000000 ... 0x1FFFFFFF:
>>> +    case 0x20000000 ... 0x3FFFFFFF:
>>> +    case 0x60000000 ... 0x7FFFFFFF:
>>> +    case 0x80000000 ... 0x9FFFFFFF:
>>> +        *prot |= PAGE_EXEC;
>>> +        break;
>>> +    }
>>> +}
>>> +
>>>  static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>>                                   int access_type, ARMMMUIdx mmu_idx,
>>>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>>> @@ -5866,7 +5886,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>>      *prot = 0;
>>>
>>>      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
>>> -        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>> +        if (arm_feature(env, ARM_FEATURE_M))
>>
>> Single line ifs still require { by coding style. scripts/checkpatch.pl
>> will catch these.
>
> OK.
>
>>
>>> +            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
>>> +        else
>>> +            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>>      } else { /* MPU enabled */
>>>          for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
>>>              /* region search */
>>> @@ -5944,7 +5967,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>>                  *fsr = 0;
>>>                  return true;
>>>              }
>>> -            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>> +            if (arm_feature(env, ARM_FEATURE_M))
>>> +                get_phys_addr_v7m_default(env, mmu_idx, address, prot);
>>> +            else
>>> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>
>> This if-else can be consolidated with something like:
>
> Will do, thanks a lot!
>
> Best regards
> Alex
>
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 63f7e7b..bfe0afb 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -6573,10 +6573,9 @@ static int get_phys_addr_pmsav7(CPUARMState
>> *env, uint32_t address,
>>      int n;
>>      *phys_ptr = address;
>>      *prot = 0;
>> +    bool do_default = true;
>>
>> -    if (!(sctlr & 0x1)) { /* MPU disabled */
>> -        get_phys_addr_pmsav7_default(env, address, prot);
>> -    } else { /* MPU enabled */
>> +    if (sctlr & 0x1) { /* MPU enabled */
>>          for (n = 15; n >= 0; n--) { /*region search */
>>              uint32_t base = env->cp15.c6_drbar[n];
>>              uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
>> @@ -6609,12 +6608,12 @@ static int get_phys_addr_pmsav7(CPUARMState
>> *env, uint32_t address,
>>              if (is_user || !(sctlr & 1 << 17)) { /* BR */
>>                  /* background fault */
>>                  return -1;
>> -            } else {
>> -                get_phys_addr_pmsav7_default(env, address, prot);
>>              }
>>          } else { /* a MPU hit! */
>>              uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
>>
>> +            do_default = false;
>> +
>>              if (is_user) { /* User mode AP bit decoding */
>>                  switch (ap) {
>>                  case 0:
>> @@ -6656,6 +6655,14 @@ static int get_phys_addr_pmsav7(CPUARMState
>> *env, uint32_t address,
>>          }
>>      }
>>
>> +    if (do_default) {
>> +        if (arm_feature(env, ARM_FEATURE_M)) {
>> +            get_phys_addr_v7m_default(env, mmu_idx, address, prot);
>> +        } else {
>> +            get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>> +        }
>> +    }
>> +
>>      switch (access_type) {
>>      case 0:
>>          return *prot & PAGE_READ ? 0 : 0x405;
>>
>> Regards,
>> Peter
>>
>>
>>>          } else { /* a MPU hit! */
>>>              uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
>>>
>>> --
>>> 1.7.9.5
>>>
>>>
>
>
>

[Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Alex Zuepke]

Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
---
 hw/intc/armv7m_nvic.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/cpu.h      |    6 +++
 target-arm/helper.c   |    7 ++-
 target-arm/machine.c  |    1 +
 4 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 369ef94..5820414 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -289,6 +289,36 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         return 0x01111110;
     case 0xd70: /* ISAR4.  */
         return 0x01310102;
+    case 0xd90: /* MPU type register.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->pmsav7_dregion << 8;
+    case 0xd94: /* MPU control register.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.v7m.mpu_ctrl;
+    case 0xd98: /* MPU_RNR.  */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->env.cp15.c6_rgnr;
+    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
+    case 0xda4: /* MPU_RBAR_A1.  */
+    case 0xdac: /* MPU_RBAR_A2.  */
+    case 0xdb4: /* MPU_RBAR_A3.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            return 0;
+        val = cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr];
+        val |= cpu->env.cp15.c6_rgnr;
+        return val;
+    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
+    case 0xda8: /* MPU_RSAR_A1.  */
+    case 0xdb0: /* MPU_RSAR_A2.  */
+    case 0xdb8: /* MPU_RSAR_A3.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            return 0;
+        val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
+        val <<= 16;
+        val |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
+        return val;
     /* TODO: Implement debug registers.  */
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", offset);
@@ -406,6 +436,57 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         qemu_log_mask(LOG_UNIMP,
                       "NVIC: AUX fault status registers unimplemented\n");
         break;
+    case 0xd94: /* MPU control register.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            break;
+        cpu->env.v7m.mpu_ctrl = value & 0x7;
+        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_ENABLE)
+            cpu->env.cp15.sctlr_ns |= SCTLR_M;
+        else
+            cpu->env.cp15.sctlr_ns &= ~SCTLR_M;
+        /* TODO: mimic MPU_CTRL_HFNMIENA */
+        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_PRIVDEFENA)
+            cpu->env.cp15.sctlr_ns |= SCTLR_BR;
+        else
+            cpu->env.cp15.sctlr_ns &= ~SCTLR_BR;
+        /* This may enable/disable the MMU, so do a TLB flush.  */
+        tlb_flush(CPU(cpu), 1);
+        break;
+    case 0xd98: /* MPU_RNR.  */
+        cpu = ARM_CPU(current_cpu);
+        value &= 0xff;
+        if (value < cpu->pmsav7_dregion)
+            cpu->env.cp15.c6_rgnr = value;
+        break;
+    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
+    case 0xda4: /* MPU_RBAR_A1.  */
+    case 0xdac: /* MPU_RBAR_A2.  */
+    case 0xdb4: /* MPU_RBAR_A3.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            break;
+        if (value & 0x10) {
+            /* region update */
+            uint32_t region = value & 0x0f;
+            if (region < cpu->pmsav7_dregion)
+                cpu->env.cp15.c6_rgnr = region;
+        }
+        value &= ~0x1f;
+        cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr] = value;
+        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
+        break;
+    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
+    case 0xda8: /* MPU_RSAR_A1.  */
+    case 0xdb0: /* MPU_RSAR_A2.  */
+    case 0xdb8: /* MPU_RSAR_A3.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            break;
+        cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] = value >> 16;
+        cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr] = value & 0xffff;
+        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
+        break;
     case 0xf00: /* Software Triggered Interrupt Register */
         if ((value & 0x1ff) < s->num_irq) {
             gic_set_pending_private(&s->gic, 0, value & 0x1ff);
@@ -445,6 +526,19 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
             return val & 0xffff;
         }
         break;
+    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            break;
+        if ((size == 2) && (offset & 7) == 0) {
+            val = cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
+            return val & 0xffff;
+        }
+        if ((size == 2) && (offset & 7) == 2) {
+            val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
+            return val & 0xffff;
+        }
+        break;
     case 0xfe0 ... 0xfff: /* ID.  */
         if (offset & 3) {
             return 0;
@@ -465,6 +559,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
     nvic_state *s = (nvic_state *)opaque;
     uint32_t offset = addr;
     int i;
+    ARMCPU *cpu;
 
     switch (offset) {
     case 0xd18 ... 0xd23: /* System Handler Priority.  */
@@ -488,6 +583,24 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
             break;
         }
         break;
+    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
+        cpu = ARM_CPU(current_cpu);
+        if (cpu->pmsav7_dregion == 0)
+            break;
+        if ((size == 2) && (offset & 7) == 0) {
+            value |= cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] << 16;
+            offset &= ~2;
+            size = 4;
+            break;
+        }
+        if ((size == 2) && (offset & 7) == 2) {
+            value <<= 16;
+            value |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
+            offset &= ~2;
+            size = 4;
+            break;
+        }
+        break;
     }
     if (size == 4) {
         nvic_writel(s, offset, value);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1089f63..d603f71 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -393,6 +393,7 @@ typedef struct CPUARMState {
         uint32_t dfsr;
         uint32_t mmfar;
         uint32_t bfar;
+        uint32_t mpu_ctrl;
     } v7m;
 
     /* Information associated with an exception about to be taken:
@@ -903,6 +904,11 @@ enum arm_cpu_mode {
 #define DFSR_BKPT           0x00000002
 #define DFSR_HALTED         0x00000001
 
+/* V7M MPU_CTRL bits */
+#define MPU_CTRL_PRIVDEFENA 0x00000004
+#define MPU_CTRL_HFNMIENA   0x00000002
+#define MPU_CTRL_ENABLE     0x00000001
+
 /* If adding a feature bit which corresponds to a Linux ELF
  * HWCAP bit, remember to update the feature-bit-to-hwcap
  * mapping in linux-user/elfload.c:get_elf_hwcap().
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 637dbf6..542e075 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4552,7 +4552,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     case EXCP_DATA_ABORT:
         env->v7m.mmfar = env->exception.vaddress;
         env->v7m.cfsr |= CFSR_MMARVALID;
-        /* TODO: further decoding of exception.fsr */
+        if (cs->exception_index == EXCP_PREFETCH_ABORT)
+            env->v7m.cfsr |= CFSR_IACCVIOL;
+        else
+            env->v7m.cfsr |= CFSR_DACCVIOL;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
         return;
     case EXCP_BKPT:
@@ -5973,6 +5976,8 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
         } else { /* a MPU hit! */
             uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
+            if ((ap == 7) && arm_feature(env, ARM_FEATURE_M))
+                ap = 6;
 
             if (is_user) { /* User mode AP bit decoding */
                 switch (ap) {
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 11dcf29..0bdeaba 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -107,6 +107,7 @@ static const VMStateDescription vmstate_m = {
         VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
         VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
         VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
+        VMSTATE_UINT32(env.v7m.mpu_ctrl, ARMCPU),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:

A commit message that wasn't just the one-line summary would
be nice. Sometimes a patch really is trivial enough that it's
not worth describing in more than just a single line, but
those situations are the exception rather than the rule.

> Signed-off-by: Alex Zuepke <alexander.zuepke@hs-rm.de>
> ---
>  hw/intc/armv7m_nvic.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/cpu.h      |    6 +++
>  target-arm/helper.c   |    7 ++-
>  target-arm/machine.c  |    1 +
>  4 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 369ef94..5820414 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -289,6 +289,36 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          return 0x01111110;
>      case 0xd70: /* ISAR4.  */
>          return 0x01310102;
> +    case 0xd90: /* MPU type register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->pmsav7_dregion << 8;
> +    case 0xd94: /* MPU control register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.mpu_ctrl;
> +    case 0xd98: /* MPU_RNR.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.cp15.c6_rgnr;
> +    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
> +    case 0xda4: /* MPU_RBAR_A1.  */
> +    case 0xdac: /* MPU_RBAR_A2.  */
> +    case 0xdb4: /* MPU_RBAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            return 0;

QEMU coding style wants braces even for single-line if statements.
(You might find scripts/checkpatch.pl helpful for flagging up
some style violations.)

> +        val = cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr];
> +        val |= cpu->env.cp15.c6_rgnr;

You need to mask this, we only want the bottom 4 bits of
c6_rgnr.

> +        return val;
> +    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
> +    case 0xda8: /* MPU_RSAR_A1.  */
> +    case 0xdb0: /* MPU_RSAR_A2.  */
> +    case 0xdb8: /* MPU_RSAR_A3.  */

These are all "RASR", not "RSAR".


> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            return 0;
> +        val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
> +        val <<= 16;
> +        val |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +        return val;
>      /* TODO: Implement debug registers.  */
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", offset);
> @@ -406,6 +436,57 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          qemu_log_mask(LOG_UNIMP,
>                        "NVIC: AUX fault status registers unimplemented\n");
>          break;
> +    case 0xd94: /* MPU control register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        cpu->env.v7m.mpu_ctrl = value & 0x7;
> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_ENABLE)
> +            cpu->env.cp15.sctlr_ns |= SCTLR_M;
> +        else
> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_M;
> +        /* TODO: mimic MPU_CTRL_HFNMIENA */

That will be interesting to implement.

> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_PRIVDEFENA)
> +            cpu->env.cp15.sctlr_ns |= SCTLR_BR;
> +        else
> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_BR;

This is kind of ugly. Wouldn't it be nicer to make the code
which tests for MMU-enabled and privdefena be M-profile aware?

> +        /* This may enable/disable the MMU, so do a TLB flush.  */
> +        tlb_flush(CPU(cpu), 1);
> +        break;
> +    case 0xd98: /* MPU_RNR.  */
> +        cpu = ARM_CPU(current_cpu);
> +        value &= 0xff;
> +        if (value < cpu->pmsav7_dregion)
> +            cpu->env.cp15.c6_rgnr = value;
> +        break;
> +    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
> +    case 0xda4: /* MPU_RBAR_A1.  */
> +    case 0xdac: /* MPU_RBAR_A2.  */
> +    case 0xdb4: /* MPU_RBAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if (value & 0x10) {
> +            /* region update */
> +            uint32_t region = value & 0x0f;
> +            if (region < cpu->pmsav7_dregion)
> +                cpu->env.cp15.c6_rgnr = region;
> +        }
> +        value &= ~0x1f;
> +        cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr] = value;
> +        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +        break;
> +    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
> +    case 0xda8: /* MPU_RSAR_A1.  */
> +    case 0xdb0: /* MPU_RSAR_A2.  */
> +    case 0xdb8: /* MPU_RSAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] = value >> 16;
> +        cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr] = value & 0xffff;
> +        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +        break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < s->num_irq) {
>              gic_set_pending_private(&s->gic, 0, value & 0x1ff);
> @@ -445,6 +526,19 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>              return val & 0xffff;
>          }
>          break;
> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if ((size == 2) && (offset & 7) == 0) {
> +            val = cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +            return val & 0xffff;
> +        }
> +        if ((size == 2) && (offset & 7) == 2) {
> +            val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
> +            return val & 0xffff;
> +        }

RASR is UNPREDICTABLE for non-word-size access, so we don't need
this at all.

>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              return 0;
> @@ -465,6 +559,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>      nvic_state *s = (nvic_state *)opaque;
>      uint32_t offset = addr;
>      int i;
> +    ARMCPU *cpu;
>
>      switch (offset) {
>      case 0xd18 ... 0xd23: /* System Handler Priority.  */
> @@ -488,6 +583,24 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>              break;
>          }
>          break;
> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if ((size == 2) && (offset & 7) == 0) {
> +            value |= cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] << 16;
> +            offset &= ~2;
> +            size = 4;
> +            break;
> +        }
> +        if ((size == 2) && (offset & 7) == 2) {
> +            value <<= 16;
> +            value |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +            offset &= ~2;
> +            size = 4;
> +            break;
> +        }
> +        break;

Don't need this either.

>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1089f63..d603f71 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -393,6 +393,7 @@ typedef struct CPUARMState {
>          uint32_t dfsr;
>          uint32_t mmfar;
>          uint32_t bfar;
> +        uint32_t mpu_ctrl;
>      } v7m;
>
>      /* Information associated with an exception about to be taken:
> @@ -903,6 +904,11 @@ enum arm_cpu_mode {
>  #define DFSR_BKPT           0x00000002
>  #define DFSR_HALTED         0x00000001
>
> +/* V7M MPU_CTRL bits */
> +#define MPU_CTRL_PRIVDEFENA 0x00000004
> +#define MPU_CTRL_HFNMIENA   0x00000002
> +#define MPU_CTRL_ENABLE     0x00000001
> +
>  /* If adding a feature bit which corresponds to a Linux ELF
>   * HWCAP bit, remember to update the feature-bit-to-hwcap
>   * mapping in linux-user/elfload.c:get_elf_hwcap().
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 637dbf6..542e075 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4552,7 +4552,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      case EXCP_DATA_ABORT:
>          env->v7m.mmfar = env->exception.vaddress;
>          env->v7m.cfsr |= CFSR_MMARVALID;
> -        /* TODO: further decoding of exception.fsr */
> +        if (cs->exception_index == EXCP_PREFETCH_ABORT)
> +            env->v7m.cfsr |= CFSR_IACCVIOL;
> +        else
> +            env->v7m.cfsr |= CFSR_DACCVIOL;
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
>          return;
>      case EXCP_BKPT:
> @@ -5973,6 +5976,8 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                  get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>          } else { /* a MPU hit! */
>              uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
> +            if ((ap == 7) && arm_feature(env, ARM_FEATURE_M))
> +                ap = 6;
>
>              if (is_user) { /* User mode AP bit decoding */
>                  switch (ap) {
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 11dcf29..0bdeaba 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -107,6 +107,7 @@ static const VMStateDescription vmstate_m = {
>          VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
>          VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
>          VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.mpu_ctrl, ARMCPU),

Missing version bump. (You might want to put mpu_ctrl in the
list with the others, to save bumping it twice.)

>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
> 1.7.9.5
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Alex Züpke]

Am 14.07.2015 um 19:49 schrieb Peter Maydell:
> On 7 July 2015 at 19:25, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote:
> 
> A commit message that wasn't just the one-line summary would
> be nice. Sometimes a patch really is trivial enough that it's
> not worth describing in more than just a single line, but
> those situations are the exception rather than the rule.

OK.

> [snip]
>> @@ -406,6 +436,57 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>>          qemu_log_mask(LOG_UNIMP,
>>                        "NVIC: AUX fault status registers unimplemented\n");
>>          break;
>> +    case 0xd94: /* MPU control register.  */
>> +        cpu = ARM_CPU(current_cpu);
>> +        if (cpu->pmsav7_dregion == 0)
>> +            break;
>> +        cpu->env.v7m.mpu_ctrl = value & 0x7;
>> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_ENABLE)
>> +            cpu->env.cp15.sctlr_ns |= SCTLR_M;
>> +        else
>> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_M;
>> +        /* TODO: mimic MPU_CTRL_HFNMIENA */
> 
> That will be interesting to implement.
> 
>> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_PRIVDEFENA)
>> +            cpu->env.cp15.sctlr_ns |= SCTLR_BR;
>> +        else
>> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_BR;
> 
> This is kind of ugly. Wouldn't it be nicer to make the code
> which tests for MMU-enabled and privdefena be M-profile aware?

Yes, but I wanted to keep the impact as small as possible ...
As a general question: is it OK to (mis-)use the existing cp15 register infrastructure for that,
or should I put the MPU enable bits, region number, etc into dedicated MPU-related sub structures in CPUARMState?

> [snip]
>> @@ -445,6 +526,19 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>>              return val & 0xffff;
>>          }
>>          break;
>> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
>> +        cpu = ARM_CPU(current_cpu);
>> +        if (cpu->pmsav7_dregion == 0)
>> +            break;
>> +        if ((size == 2) && (offset & 7) == 0) {
>> +            val = cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
>> +            return val & 0xffff;
>> +        }
>> +        if ((size == 2) && (offset & 7) == 2) {
>> +            val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
>> +            return val & 0xffff;
>> +        }
> 
> RASR is UNPREDICTABLE for non-word-size access, so we don't need
> this at all.

It's from ARM recommended sample code:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0553a/BIHHHDDJ.html


Thanks a lot for the reviews!
I'll send a new set of patches next week.



Best regards
Alex

Re: [Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Peter Maydell]

On 15 July 2015 at 08:31, Alex Züpke <alexander.zuepke@hs-rm.de> wrote:
> Am 14.07.2015 um 19:49 schrieb Peter Maydell:
>> RASR is UNPREDICTABLE for non-word-size access, so we don't need
>> this at all.
>
> It's from ARM recommended sample code:
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0553a/BIHHHDDJ.html

Ah, thanks. It turns out that this is a difference between
implementations: earlier M profile cores (M3, M4) support
byte and halfword accesses here, but later ones don't, so
the architecture defines it as UNPREDICTABLE. Since we're
modelling an M3, it's probably safest to implement it.
You might find that extract32()/deposit32() make for a
cleaner way to write it, though.

-- PMM