[Qemu-devel] [PATCH 0/3] armv7-m exit, exception fixes and add MPU

[Qemu-devel] [PATCH 0/3] armv7-m exit, exception fixes and add MPU

[Michael Davidsaver]

Please be aware that this work is based on my reading of ARM documentation, and hasn't yet been cross-checked with real hardware.  Patch #1 is (I think) fairly straightforward.  I'd like to get some comments on patches #2 and #3, which I don't consider ready for merge.

The first patch was previously sent on the thread "armv7-m: exit on external reset request", and included here with only a changed commit message.

I'm trying to add support for the MPU (PMSAv7) for cortex-m3 and cortex-m4.  In the process I found an issue with handling of exceptions other than IRQ.  In arm_v7m_cpu_do_interrupt() 'v7m.exception' is not being updated for UsageFault, SVC, and MemManage cases.  In the UsageFault case, the handler isn't invoked, and an invalid instruction is perpetually re-executed, resulting in a stuck state.

Patch #2 relaxes handling of mis-aligned handler functions (ie. missing '.thumb_func') to mirror what is already done on exception return.  While a guest_errors message will be helpful for people (like myself) who make this mistake, I'm not sure if automatically correcting the error is appropriate.

I'm having some difficulty in getting my test code loaded on a real cortex-m4 for a cross-check.  It may be some time before I succeed (might have to get a different board).  If anyone is interested in trying to do this test, please let me know as I'm happy to assist.

Michael Davidsaver (3):
  armv7-m: exit on external reset request
  armv7-m: fix non-IRQ exceptions
  armv7-m: add MPU to cortex-m3 and cortex-m4

 hw/arm/armv7m.c       |   8 ---
 hw/intc/armv7m_nvic.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/cpu-qom.h  |   4 ++
 target-arm/cpu.c      |  14 +++++
 target-arm/helper.c   |  34 +++++++++--
 5 files changed, 200 insertions(+), 20 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] armv7-m: exit on external reset request

[Michael Davidsaver]

Implement the SYSRESETREQ bit of the AIRCR register
for armv7-m (ie. cortex-m3).
---
 hw/intc/armv7m_nvic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 3ec8408..a671d84 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -15,6 +15,7 @@
 #include "hw/arm/arm.h"
 #include "exec/address-spaces.h"
 #include "gic_internal.h"
+#include "sysemu/sysemu.h"
 
 typedef struct {
     GICState gic;
@@ -348,10 +349,13 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
         break;
     case 0xd0c: /* Application Interrupt/Reset Control.  */
         if ((value >> 16) == 0x05fa) {
+            if (value & 4) {
+                qemu_system_reset_request();
+            }
             if (value & 2) {
                 qemu_log_mask(LOG_UNIMP, "VECTCLRACTIVE unimplemented\n");
             }
-            if (value & 5) {
+            if (value & 1) {
                 qemu_log_mask(LOG_UNIMP, "AIRCR system reset unimplemented\n");
             }
             if (value & 0x700) {
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] armv7-m: fix non-IRQ exceptions

[Michael Davidsaver]

Handlers will not be entered unless v7m.exception is updated.
For example, an invalid instruction won't invoke UsageError,
but rather re-executes the invalid instruction forever.

Add warn and fix of mis-aligned handlers.

Ensure exception return "addresses" always fault,
and trap them just before the EXCP_DATA_ABORT
handler would be invoked and execute return instead
of MemManage.
This removes the need for the "armv7m.hack" MemoryRegion.
---
 hw/arm/armv7m.c     |  8 --------
 target-arm/helper.c | 27 +++++++++++++++++++++------
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index eb214db..0fc95de 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -178,7 +178,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
     uint64_t lowaddr;
     int i;
     int big_endian;
-    MemoryRegion *hack = g_new(MemoryRegion, 1);
 
     if (cpu_model == NULL) {
 	cpu_model = "cortex-m3";
@@ -226,13 +225,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
         }
     }
 
-    /* Hack to map an additional page of ram at the top of the address
-       space.  This stops qemu complaining about executing code outside RAM
-       when returning from an exception.  */
-    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
-    vmstate_register_ram_global(hack);
-    memory_region_add_subregion(system_memory, 0xfffff000, hack);
-
     qemu_register_reset(armv7m_reset, cpu);
     return pic;
 }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8367997..56b238f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5346,18 +5346,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     switch (cs->exception_index) {
     case EXCP_UDEF:
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
-        return;
+        env->v7m.exception = ARMV7M_EXCP_USAGE;
+        break;
     case EXCP_SWI:
         /* The PC already points to the next instruction.  */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
-        return;
+        env->v7m.exception = ARMV7M_EXCP_SVC;
+        break;
     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.
-         */
+        if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0) {
+            /* this isn't a real fault, but rather a result of return from interrupt */
+            do_v7m_exception_exit(env);
+            return;
+        }
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
-        return;
+        env->v7m.exception = ARMV7M_EXCP_MEM;
+        break;
     case EXCP_BKPT:
         if (semihosting_enabled()) {
             int nr;
@@ -5407,6 +5412,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
     env->regs[15] = addr & 0xfffffffe;
     env->thumb = addr & 1;
+    if(!env->thumb) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "M profile interrupt handler with misaligned "
+                      "PC is UNPREDICTABLE\n");
+        env->thumb = 1;
+    }
 }
 
 /* Function used to synchronize QEMU's AArch64 register set with AArch32
@@ -6682,6 +6693,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
     *phys_ptr = address;
     *prot = 0;
 
+    /* ensure exception returns take precidence */
+    if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0)
+        return true;
+
     if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
         get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
     } else { /* MPU enabled */
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] armv7-m: add MPU to cortex-m3 and cortex-m4

[Michael Davidsaver]

The M series MPU is almost the same as the already
implemented R series MPU.  So use the M series
and translate as best we can.

The HFNMIENA bit in MPU_CTRL is not implemented.

Implement CFSR and MMFAR to report fault address
to MemManage handler.

Add MPU feature flag to cortex-m3 and -m4.
---
 hw/intc/armv7m_nvic.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/cpu-qom.h  |   4 ++
 target-arm/cpu.c      |  14 +++++
 target-arm/helper.c   |   7 +++
 4 files changed, 174 insertions(+), 5 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index a671d84..94011cf 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -245,12 +245,11 @@ 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;
+        return ARM_CPU(current_cpu)->pmsav7_cfsr;
+    case 0xd34: /* MMFAR MemManage Fault Address */
+        return ARM_CPU(current_cpu)->pmsav7_mmfar;
     case 0xd2c: /* Hard Fault Status.  */
     case 0xd30: /* Debug Fault Status.  */
-    case 0xd34: /* Mem Manage Address.  */
     case 0xd38: /* Bus Fault Address.  */
     case 0xd3c: /* Aux Fault Status.  */
         /* TODO: Implement fault status registers.  */
@@ -283,6 +282,55 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
     case 0xd70: /* ISAR4.  */
         return 0x01310102;
     /* TODO: Implement debug registers.  */
+    case 0xd90: /* MPU_TYPE */
+        cpu = ARM_CPU(current_cpu);
+        return cpu->has_mpu ? (cpu->pmsav7_dregion<<8) : 0;
+        break;
+    case 0xd94: /* MPU_CTRL */
+        val = 0;
+        cpu = ARM_CPU(current_cpu);
+        if(cpu->env.cp15.sctlr_el[0] & SCTLR_M)
+            val |= 1; /* ENABLE */
+        /* HFNMIENA not implemented, see nvic_writel() */
+        if(cpu->env.cp15.sctlr_el[0] & SCTLR_BR)
+            val |= 4; /* PRIVDEFENA */
+        return val;
+    case 0xd98: /* MPU_RNR */
+        return ARM_CPU(current_cpu)->env.cp15.c6_rgnr;
+    case 0xd9c: /* MPU_RBAR */
+    case 0xda4: /* MPU_RBAR_A1 */
+    case 0xdaC: /* MPU_RBAR_A2 */
+    case 0xdb4: /* MPU_RBAR_A3 */
+    {
+        uint32_t range;
+        cpu = ARM_CPU(current_cpu);
+        if(offset==0xd9c)
+            range = cpu->env.cp15.c6_rgnr;
+        else
+            range = (offset-0xda4)/8;
+
+        if(range>=cpu->pmsav7_dregion) return 0;
+
+        return (cpu->env.pmsav7.drbar[range]&(0x1f)) | (range&0xf);
+    }
+    case 0xda0: /* MPU_RASR */
+    case 0xda8: /* MPU_RASR_A1 */
+    case 0xdb0: /* MPU_RASR_A2 */
+    case 0xdb8: /* MPU_RASR_A3 */
+    {
+        uint32_t range;
+        cpu = ARM_CPU(current_cpu);
+
+        if(offset==0xda0)
+            range = cpu->env.cp15.c6_rgnr;
+        else
+            range = (offset-0xda8)/8;
+
+        if(range>=cpu->pmsav7_dregion) return 0;
+
+        return ((cpu->env.pmsav7.dracr[range]&0xffff)<<16)
+                | (cpu->env.pmsav7.drsr[range]&0xffff);
+    }
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", offset);
         return 0;
@@ -376,14 +424,110 @@ 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.  */
+    case 0xd34: /* Mem Manage Address.  */
+        return;
     case 0xd2c: /* Hard Fault Status.  */
     case 0xd30: /* Debug Fault Status.  */
-    case 0xd34: /* Mem Manage Address.  */
     case 0xd38: /* Bus Fault Address.  */
     case 0xd3c: /* Aux Fault Status.  */
         qemu_log_mask(LOG_UNIMP,
                       "NVIC: fault status registers unimplemented\n");
         break;
+    case 0xd90: /* MPU_TYPE (0xe000ed90) */
+        return; /* RO */
+    case 0xd94: /* MPU_CTRL */
+    {
+        unsigned q;
+        uint32_t tset=0, tclear=0;
+        if(value&1) {
+            tset |= SCTLR_M;
+        } else {
+            tclear |= SCTLR_M;
+        }
+        if(value&2) {
+            /* The M series MPU is almost functionally the same
+             * as the R series MPU, so we translate to the R series
+             * register format.
+             *
+             * One difference is that the R series MPU doesn't implement
+             * HFNMIENA (bypass MPU in some exception handlers).
+             */
+            qemu_log_mask(LOG_UNIMP, "M profile does not implement HFNMIENA bit in MPU_CTRL\n");
+        }
+        if(value&4) {
+            tset |= SCTLR_BR;
+        } else {
+            tclear |= SCTLR_BR;
+        }
+        cpu = ARM_CPU(current_cpu);
+        /* TODO, which sctlr(s) really need to be set? */
+        for(q=0; q<4; q++) {
+            cpu->env.cp15.sctlr_el[q] |= tset;
+            cpu->env.cp15.sctlr_el[q] &= ~tclear;
+        }
+        tlb_flush(CPU(cpu), 1);
+    }
+        break;
+    case 0xd98: /* MPU_RNR */
+        cpu = ARM_CPU(current_cpu);
+        if(value>=cpu->pmsav7_dregion) {
+            qemu_log_mask(LOG_GUEST_ERROR, "MPU region out of range %u/%u\n",
+                          (unsigned)value, (unsigned)cpu->pmsav7_dregion);
+        } else {
+            cpu->env.cp15.c6_rgnr = value;
+        }
+        tlb_flush(CPU(cpu), 1); /* necessary? */
+        break;
+    case 0xd9c: /* MPU_RBAR */
+    case 0xda4: /* MPU_RBAR_A1 */
+    case 0xdac: /* MPU_RBAR_A2 */
+    case 0xdb4: /* MPU_RBAR_A3 */
+    {
+        uint32_t range;
+        uint32_t base = value;
+        cpu = ARM_CPU(current_cpu);
+
+        if(offset==0xd9c)
+            range = cpu->env.cp15.c6_rgnr;
+        else
+            range = (offset-0xda4)/8;
+
+        if(value&(1<<4)) {
+            range = value&0xf;
+
+            if(range>=cpu->pmsav7_dregion) {
+                qemu_log_mask(LOG_GUEST_ERROR, "MPU region out of range %u/%u\n",
+                              (unsigned)range, (unsigned)cpu->pmsav7_dregion);
+                return;
+            }
+            cpu->env.cp15.c6_rgnr = range;
+            base &= ~0x1f;
+
+        } else if(range>=cpu->pmsav7_dregion)
+            return;
+
+        cpu->env.pmsav7.drbar[range] = base&~0x3;
+    }
+        tlb_flush(CPU(cpu), 1);
+        break;
+    case 0xda0: /* MPU_RASR */
+    case 0xda8: /* MPU_RASR_A1 */
+    case 0xdb0: /* MPU_RASR_A2 */
+    case 0xdb8: /* MPU_RASR_A3 */
+    {
+        uint32_t range;
+        cpu = ARM_CPU(current_cpu);
+
+        if(offset==0xda0)
+            range = cpu->env.cp15.c6_rgnr;
+        else
+            range = (offset-0xda8)/8;
+
+        cpu->env.pmsav7.drsr[range] = value&0xff3f;
+        cpu->env.pmsav7.dracr[range] = (value>>16)&0x173f;
+    }
+        tlb_flush(CPU(cpu), 1);
+        break;
     case 0xf00: /* Software Triggered Interrupt Register */
         if ((value & 0x1ff) < s->num_irq) {
             gic_set_pending_private(&s->gic, 0, value & 0x1ff);
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 25fb1ce..d75a496 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -107,6 +107,10 @@ typedef struct ARMCPU {
     bool has_mpu;
     /* PMSAv7 MPU number of supported regions */
     uint32_t pmsav7_dregion;
+    /* PMASv7 Fault status */
+    uint32_t pmsav7_cfsr;
+    /* PMASv7 Address of fault */
+    uint32_t pmsav7_mmfar;
 
     /* PSCI conduit used to invoke PSCI methods
      * 0 - disabled, 1 - smc, 2 - hvc
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d7b4445..deb5878 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -516,6 +516,12 @@ static Property arm_cpu_has_mpu_property =
 static Property arm_cpu_pmsav7_dregion_property =
             DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion, 16);
 
+static Property arm_cpu_pmsav7_cfsr_property =
+            DEFINE_PROP_UINT32("pmsav7-cfsr", ARMCPU, pmsav7_cfsr, 0);
+
+static Property arm_cpu_pmsav7_mmfar_property =
+            DEFINE_PROP_UINT32("pmsav7-mmfar", ARMCPU, pmsav7_mmfar, 0);
+
 static void arm_cpu_post_init(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -551,6 +557,12 @@ static void arm_cpu_post_init(Object *obj)
             qdev_property_add_static(DEVICE(obj),
                                      &arm_cpu_pmsav7_dregion_property,
                                      &error_abort);
+            qdev_property_add_static(DEVICE(obj),
+                                     &arm_cpu_pmsav7_cfsr_property,
+                                     &error_abort);
+            qdev_property_add_static(DEVICE(obj),
+                                     &arm_cpu_pmsav7_mmfar_property,
+                                     &error_abort);
         }
     }
 
@@ -889,6 +901,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;
 }
 
@@ -898,6 +911,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 56b238f..368cbc6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5357,11 +5357,15 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     case EXCP_DATA_ABORT:
         if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0) {
             /* this isn't a real fault, but rather a result of return from interrupt */
+            cpu->pmsav7_mmfar = 0;
+            cpu->pmsav7_cfsr = 0;
             do_v7m_exception_exit(env);
             return;
         }
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
         env->v7m.exception = ARMV7M_EXCP_MEM;
+        cpu->pmsav7_mmfar = env->exception.vaddress;
+        cpu->pmsav7_cfsr = (1<<1)|(1<<7);
         break;
     case EXCP_BKPT:
         if (semihosting_enabled()) {
@@ -5382,6 +5386,9 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         env->v7m.exception = armv7m_nvic_acknowledge_irq(env->nvic);
         break;
     case EXCP_EXCEPTION_EXIT:
+
+        cpu->pmsav7_mmfar = 0;
+        cpu->pmsav7_cfsr = 0;
         do_v7m_exception_exit(env);
         return;
     default:
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/3] armv7-m: add MPU to cortex-m3 and cortex-m4

[Peter Crosthwaite]

On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
> The M series MPU is almost the same as the already
> implemented R series MPU.  So use the M series
> and translate as best we can.
>

There is some work on list for this that never got a respin:

https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg01945.html

> The HFNMIENA bit in MPU_CTRL is not implemented.
>
> Implement CFSR and MMFAR to report fault address
> to MemManage handler.
>
> Add MPU feature flag to cortex-m3 and -m4.
> ---
>  hw/intc/armv7m_nvic.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  target-arm/cpu-qom.h  |   4 ++
>  target-arm/cpu.c      |  14 +++++
>  target-arm/helper.c   |   7 +++
>  4 files changed, 174 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index a671d84..94011cf 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -245,12 +245,11 @@ 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;
> +        return ARM_CPU(current_cpu)->pmsav7_cfsr;

You should avoid dereferenced inline QOM casts and create a local variable.

> +    case 0xd34: /* MMFAR MemManage Fault Address */
> +        return ARM_CPU(current_cpu)->pmsav7_mmfar;

Why reorder the addresses in the switch?

>      case 0xd2c: /* Hard Fault Status.  */
>      case 0xd30: /* Debug Fault Status.  */
> -    case 0xd34: /* Mem Manage Address.  */
>      case 0xd38: /* Bus Fault Address.  */
>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> @@ -283,6 +282,55 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>      case 0xd70: /* ISAR4.  */
>          return 0x01310102;
>      /* TODO: Implement debug registers.  */
> +    case 0xd90: /* MPU_TYPE */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->has_mpu ? (cpu->pmsav7_dregion<<8) : 0;
> +        break;
> +    case 0xd94: /* MPU_CTRL */
> +        val = 0;
> +        cpu = ARM_CPU(current_cpu);
> +        if(cpu->env.cp15.sctlr_el[0] & SCTLR_M)
> +            val |= 1; /* ENABLE */
> +        /* HFNMIENA not implemented, see nvic_writel() */
> +        if(cpu->env.cp15.sctlr_el[0] & SCTLR_BR)
> +            val |= 4; /* PRIVDEFENA */
> +        return val;
> +    case 0xd98: /* MPU_RNR */
> +        return ARM_CPU(current_cpu)->env.cp15.c6_rgnr;
> +    case 0xd9c: /* MPU_RBAR */
> +    case 0xda4: /* MPU_RBAR_A1 */
> +    case 0xdaC: /* MPU_RBAR_A2 */
> +    case 0xdb4: /* MPU_RBAR_A3 */
> +    {
> +        uint32_t range;
> +        cpu = ARM_CPU(current_cpu);
> +        if(offset==0xd9c)

spaces around ==

> +            range = cpu->env.cp15.c6_rgnr;
> +        else
> +            range = (offset-0xda4)/8;
> +
> +        if(range>=cpu->pmsav7_dregion) return 0;

{} for if body, return on new line. If you run your patch through
scripts/checkpatch.pl it will detect some of these conventions.

> +
> +        return (cpu->env.pmsav7.drbar[range]&(0x1f)) | (range&0xf);

Spaces around &, parentheses around hex constant not needed.

> +    }
> +    case 0xda0: /* MPU_RASR */
> +    case 0xda8: /* MPU_RASR_A1 */
> +    case 0xdb0: /* MPU_RASR_A2 */
> +    case 0xdb8: /* MPU_RASR_A3 */
> +    {
> +        uint32_t range;
> +        cpu = ARM_CPU(current_cpu);
> +
> +        if(offset==0xda0)
> +            range = cpu->env.cp15.c6_rgnr;
> +        else
> +            range = (offset-0xda8)/8;
> +
> +        if(range>=cpu->pmsav7_dregion) return 0;
> +
> +        return ((cpu->env.pmsav7.dracr[range]&0xffff)<<16)
> +                | (cpu->env.pmsav7.drsr[range]&0xffff);
> +    }

More style nits here.

Regards,
Peter

>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", offset);
>          return 0;
> @@ -376,14 +424,110 @@ 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;

Re: [Qemu-devel] [PATCH 2/3] armv7-m: fix non-IRQ exceptions

[Peter Crosthwaite]

On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
> Handlers will not be entered unless v7m.exception is updated.
> For example, an invalid instruction won't invoke UsageError,
> but rather re-executes the invalid instruction forever.
>
> Add warn and fix of mis-aligned handlers.
>
> Ensure exception return "addresses" always fault,
> and trap them just before the EXCP_DATA_ABORT
> handler would be invoked and execute return instead
> of MemManage.
> This removes the need for the "armv7m.hack" MemoryRegion.
> ---
>  hw/arm/armv7m.c     |  8 --------
>  target-arm/helper.c | 27 +++++++++++++++++++++------
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index eb214db..0fc95de 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -178,7 +178,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      uint64_t lowaddr;
>      int i;
>      int big_endian;
> -    MemoryRegion *hack = g_new(MemoryRegion, 1);
>
>      if (cpu_model == NULL) {
>         cpu_model = "cortex-m3";
> @@ -226,13 +225,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>          }
>      }
>
> -    /* Hack to map an additional page of ram at the top of the address
> -       space.  This stops qemu complaining about executing code outside RAM
> -       when returning from an exception.  */
> -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
> -    vmstate_register_ram_global(hack);
> -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
> -

CC PMM, Alistair and Marcin. They were discussing this recently.

Regards,
Peter

>      qemu_register_reset(armv7m_reset, cpu);
>      return pic;
>  }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..56b238f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5346,18 +5346,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_USAGE;
> +        break;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_SVC;
> +        break;
>      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.
> -         */
> +        if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0) {
> +            /* this isn't a real fault, but rather a result of return from interrupt */
> +            do_v7m_exception_exit(env);
> +            return;
> +        }
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_MEM;
> +        break;
>      case EXCP_BKPT:
>          if (semihosting_enabled()) {
>              int nr;
> @@ -5407,6 +5412,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
> +    if(!env->thumb) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "M profile interrupt handler with misaligned "
> +                      "PC is UNPREDICTABLE\n");
> +        env->thumb = 1;
> +    }
>  }
>
>  /* Function used to synchronize QEMU's AArch64 register set with AArch32
> @@ -6682,6 +6693,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>      *phys_ptr = address;
>      *prot = 0;
>
> +    /* ensure exception returns take precidence */
> +    if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0)
> +        return true;
> +
>      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
>          get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>      } else { /* MPU enabled */
> --
> 2.1.4
>

Re: [Qemu-devel] [PATCH 2/3] armv7-m: fix non-IRQ exceptions

[Michael Davidsaver]

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

I'm starting to doubt my diagnosis.  The bug may be in my understanding of
the interrupt priorities.  I'll have to do another test program.
On Oct 11, 2015 11:25 AM, "Peter Crosthwaite" <crosthwaitepeter@gmail.com>
wrote:

> On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
> <mdavidsaver@gmail.com> wrote:
> > Handlers will not be entered unless v7m.exception is updated.
> > For example, an invalid instruction won't invoke UsageError,
> > but rather re-executes the invalid instruction forever.
> >
> > Add warn and fix of mis-aligned handlers.
> >
> > Ensure exception return "addresses" always fault,
> > and trap them just before the EXCP_DATA_ABORT
> > handler would be invoked and execute return instead
> > of MemManage.
> > This removes the need for the "armv7m.hack" MemoryRegion.
> > ---
> >  hw/arm/armv7m.c     |  8 --------
> >  target-arm/helper.c | 27 +++++++++++++++++++++------
> >  2 files changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> > index eb214db..0fc95de 100644
> > --- a/hw/arm/armv7m.c
> > +++ b/hw/arm/armv7m.c
> > @@ -178,7 +178,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
> int mem_size, int num_irq,
> >      uint64_t lowaddr;
> >      int i;
> >      int big_endian;
> > -    MemoryRegion *hack = g_new(MemoryRegion, 1);
> >
> >      if (cpu_model == NULL) {
> >         cpu_model = "cortex-m3";
> > @@ -226,13 +225,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
> int mem_size, int num_irq,
> >          }
> >      }
> >
> > -    /* Hack to map an additional page of ram at the top of the address
> > -       space.  This stops qemu complaining about executing code outside
> RAM
> > -       when returning from an exception.  */
> > -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000,
> &error_fatal);
> > -    vmstate_register_ram_global(hack);
> > -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
> > -
>
> CC PMM, Alistair and Marcin. They were discussing this recently.
>
> Regards,
> Peter
>
> >      qemu_register_reset(armv7m_reset, cpu);
> >      return pic;
> >  }
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 8367997..56b238f 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -5346,18 +5346,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> >      switch (cs->exception_index) {
> >      case EXCP_UDEF:
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_USAGE;
> > +        break;
> >      case EXCP_SWI:
> >          /* The PC already points to the next instruction.  */
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_SVC;
> > +        break;
> >      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.
> > -         */
> > +        if(env->v7m.exception!=0 &&
> env->exception.vaddress>=0xfffffff0) {
> > +            /* this isn't a real fault, but rather a result of return
> from interrupt */
> > +            do_v7m_exception_exit(env);
> > +            return;
> > +        }
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_MEM;
> > +        break;
> >      case EXCP_BKPT:
> >          if (semihosting_enabled()) {
> >              int nr;
> > @@ -5407,6 +5412,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> >      addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
> >      env->regs[15] = addr & 0xfffffffe;
> >      env->thumb = addr & 1;
> > +    if(!env->thumb) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "M profile interrupt handler with misaligned "
> > +                      "PC is UNPREDICTABLE\n");
> > +        env->thumb = 1;
> > +    }
> >  }
> >
> >  /* Function used to synchronize QEMU's AArch64 register set with AArch32
> > @@ -6682,6 +6693,10 @@ static bool get_phys_addr_pmsav7(CPUARMState
> *env, uint32_t address,
> >      *phys_ptr = address;
> >      *prot = 0;
> >
> > +    /* ensure exception returns take precidence */
> > +    if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0)
> > +        return true;
> > +
> >      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> >          get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> >      } else { /* MPU enabled */
> > --
> > 2.1.4
> >
>

[-- Attachment #2: Type: text/html, Size: 6000 bytes --]

Re: [Qemu-devel] [PATCH 2/3] armv7-m: fix non-IRQ exceptions

[Peter Maydell]

On 11 October 2015 at 19:58, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> I'm starting to doubt my diagnosis.  The bug may be in my understanding of
> the interrupt priorities.  I'll have to do another test program.

Note that our handling of prioritization of the internal
exceptions is pretty badly broken. The fix for this probably
involves a redesign rather than a point fix :-/

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] armv7-m: add MPU to cortex-m3 and cortex-m4

[Michael Davidsaver]

On 10/11/2015 11:23 AM, Peter Crosthwaite wrote:
> On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
> <mdavidsaver@gmail.com> wrote:
>> The M series MPU is almost the same as the already
>> implemented R series MPU.  So use the M series
>> and translate as best we can.
>>
> There is some work on list for this that never got a respin:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg01945.html

Well, I totally missed that.  I'll have look.

> ...
>> +    case 0xd34: /* MMFAR MemManage Fault Address */
>> +        return ARM_CPU(current_cpu)->pmsav7_mmfar;
> Why reorder the addresses in the switch?

I was thinking to avoid duplicating the qemu_log_mask() for the
unimplemented registers.  I take it that this to you is not the lesser
evil :)

> ... If you run your patch through scripts/checkpatch.pl it will detect
> some of these conventions. 

Will do.

> ... More style nits here....

All noted.

> Regards, Peter 

Michael