diff mbox series

[2/6] armv7m: Replace armv7m.hack with unassigned_access handler

Message ID 1484937883-1068-3-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series ARMv7M: some simple bugfixes and cleanups | expand

Commit Message

Peter Maydell Jan. 20, 2017, 6:44 p.m. UTC
From: Michael Davidsaver <mdavidsaver@gmail.com>


For v7m we need to catch attempts to execute from special
addresses at 0xfffffff0 and above. Previously we did this
with the aid of a hacky special purpose lump of memory
in the address space and a check in translate.c for whether
we were translating code at those addresses.

We can implement this more cleanly using a CPU
unassigned access handler which throws the exception
if the unassigned access is for one of the special addresses.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

[PMM:
 * drop the deletion of the "don't interrupt if PC is magic"
   code in arm_v7m_cpu_exec_interrupt() -- this is still
   required
 * don't generate an exception for unassigned accesses
   which aren't to the magic address -- although doing
   this is in theory correct in practice it will break
   currently working guests which rely on the RAZ/WI
   behaviour when they touch devices which we haven't
   modelled.
 * trigger EXCP_EXCEPTION_EXIT on is_exec, not !is_write
]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 hw/arm/armv7m.c        |  8 --------
 target/arm/cpu.c       | 28 ++++++++++++++++++++++++++++
 target/arm/translate.c | 12 ++++++------
 3 files changed, 34 insertions(+), 14 deletions(-)

-- 
2.7.4

Comments

Alex Bennée Jan. 24, 2017, 4:31 p.m. UTC | #1
Peter Maydell <peter.maydell@linaro.org> writes:

> From: Michael Davidsaver <mdavidsaver@gmail.com>

>

> For v7m we need to catch attempts to execute from special

> addresses at 0xfffffff0 and above. Previously we did this

> with the aid of a hacky special purpose lump of memory

> in the address space and a check in translate.c for whether

> we were translating code at those addresses.

>

> We can implement this more cleanly using a CPU

> unassigned access handler which throws the exception

> if the unassigned access is for one of the special addresses.

>

> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

> [PMM:

>  * drop the deletion of the "don't interrupt if PC is magic"

>    code in arm_v7m_cpu_exec_interrupt() -- this is still

>    required

>  * don't generate an exception for unassigned accesses

>    which aren't to the magic address -- although doing

>    this is in theory correct in practice it will break

>    currently working guests which rely on the RAZ/WI

>    behaviour when they touch devices which we haven't

>    modelled.

>  * trigger EXCP_EXCEPTION_EXIT on is_exec, not !is_write

> ]

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/arm/armv7m.c        |  8 --------

>  target/arm/cpu.c       | 28 ++++++++++++++++++++++++++++

>  target/arm/translate.c | 12 ++++++------

>  3 files changed, 34 insertions(+), 14 deletions(-)

>

> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c

> index 49d3078..0c9ca7b 100644

> --- a/hw/arm/armv7m.c

> +++ b/hw/arm/armv7m.c

> @@ -180,7 +180,6 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,

>      uint64_t entry;

>      uint64_t lowaddr;

>      int big_endian;

> -    MemoryRegion *hack = g_new(MemoryRegion, 1);

>

>      if (cpu_model == NULL) {

>  	cpu_model = "cortex-m3";

> @@ -225,13 +224,6 @@ DeviceState *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);

> -


What stops a model inadvertently registering a block of memory on-top?
Should we check the memory region really is unassigned?

>      qemu_register_reset(armv7m_reset, cpu);

>      return nvic;

>  }

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c

> index 3f2cdb6..47759c9 100644

> --- a/target/arm/cpu.c

> +++ b/target/arm/cpu.c

> @@ -292,6 +292,33 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)

>  }

>

>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)

> +static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,

> +                                      bool is_write, bool is_exec, int opaque,

> +                                      unsigned size)

> +{

> +    ARMCPU *arm = ARM_CPU(cpu);

> +    CPUARMState *env = &arm->env;

> +

> +    /* ARMv7-M interrupt return works by loading a magic value into the PC.

> +     * On real hardware the load causes the return to occur.  The qemu

> +     * implementation performs the jump normally, then does the exception

> +     * return by throwing a special exception when when the CPU tries to

> +     * execute code at the magic address.

> +     */

> +    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {

> +        cpu->exception_index = EXCP_EXCEPTION_EXIT;

> +        cpu_loop_exit(cpu);

> +    }

> +

> +    /* In real hardware an attempt to access parts of the address space

> +     * with nothing there will usually cause an external abort.

> +     * However our QEMU board models are often missing device models where

> +     * the guest can boot anyway with the default read-as-zero/writes-ignored

> +     * behaviour that you get without a QEMU unassigned_access hook.

> +     * So just return here to retain that default behaviour.

> +     */

> +}

> +

>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)

>  {

>      CPUClass *cc = CPU_GET_CLASS(cs);

> @@ -1016,6 +1043,7 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)

>      cc->do_interrupt = arm_v7m_cpu_do_interrupt;

>  #endif

>

> +    cc->do_unassigned_access = arm_v7m_unassigned_access;

>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;

>  }

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index c9186b6..a7c2abe 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -11719,12 +11719,12 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)

>              break;

>          }

>  #else

> -        if (dc->pc >= 0xfffffff0 && arm_dc_feature(dc, ARM_FEATURE_M)) {

> -            /* We always get here via a jump, so know we are not in a

> -               conditional execution block.  */

> -            gen_exception_internal(EXCP_EXCEPTION_EXIT);

> -            dc->is_jmp = DISAS_EXC;

> -            break;

> +        if (arm_dc_feature(dc, ARM_FEATURE_M)) {

> +            /* Branches to the magic exception-return addresses should

> +             * already have been caught via the arm_v7m_unassigned_access hook,

> +             * and never get here.

> +             */

> +            assert(dc->pc < 0xfffffff0);

>          }

>  #endif


Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


--
Alex Bennée
Peter Maydell Jan. 24, 2017, 4:53 p.m. UTC | #2
On 24 January 2017 at 16:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>

> Peter Maydell <peter.maydell@linaro.org> writes:

>> -    /* 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);

>> -

>

> What stops a model inadvertently registering a block of memory on-top?

> Should we check the memory region really is unassigned?


It would just be a board code bug, and a pretty implausible one
I think (at least no less plausible than any other typoing of
a device or memory address, 99% of which are uncheckable).

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 49d3078..0c9ca7b 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -180,7 +180,6 @@  DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
     uint64_t entry;
     uint64_t lowaddr;
     int big_endian;
-    MemoryRegion *hack = g_new(MemoryRegion, 1);
 
     if (cpu_model == NULL) {
 	cpu_model = "cortex-m3";
@@ -225,13 +224,6 @@  DeviceState *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 nvic;
 }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 3f2cdb6..47759c9 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -292,6 +292,33 @@  bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 }
 
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
+static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
+                                      bool is_write, bool is_exec, int opaque,
+                                      unsigned size)
+{
+    ARMCPU *arm = ARM_CPU(cpu);
+    CPUARMState *env = &arm->env;
+
+    /* ARMv7-M interrupt return works by loading a magic value into the PC.
+     * On real hardware the load causes the return to occur.  The qemu
+     * implementation performs the jump normally, then does the exception
+     * return by throwing a special exception when when the CPU tries to
+     * execute code at the magic address.
+     */
+    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {
+        cpu->exception_index = EXCP_EXCEPTION_EXIT;
+        cpu_loop_exit(cpu);
+    }
+
+    /* In real hardware an attempt to access parts of the address space
+     * with nothing there will usually cause an external abort.
+     * However our QEMU board models are often missing device models where
+     * the guest can boot anyway with the default read-as-zero/writes-ignored
+     * behaviour that you get without a QEMU unassigned_access hook.
+     * So just return here to retain that default behaviour.
+     */
+}
+
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
@@ -1016,6 +1043,7 @@  static void arm_v7m_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = arm_v7m_cpu_do_interrupt;
 #endif
 
+    cc->do_unassigned_access = arm_v7m_unassigned_access;
     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c9186b6..a7c2abe 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11719,12 +11719,12 @@  void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             break;
         }
 #else
-        if (dc->pc >= 0xfffffff0 && arm_dc_feature(dc, ARM_FEATURE_M)) {
-            /* We always get here via a jump, so know we are not in a
-               conditional execution block.  */
-            gen_exception_internal(EXCP_EXCEPTION_EXIT);
-            dc->is_jmp = DISAS_EXC;
-            break;
+        if (arm_dc_feature(dc, ARM_FEATURE_M)) {
+            /* Branches to the magic exception-return addresses should
+             * already have been caught via the arm_v7m_unassigned_access hook,
+             * and never get here.
+             */
+            assert(dc->pc < 0xfffffff0);
         }
 #endif