Re: [Qemu-arm] [PATCH v2 02/26] armv7m: Undo armv7m.hack

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <address@hidden> wrote:
> Add CPU unassigned access handler in place of special
> MemoryRegion to catch exception returns.
>
> The unassigned handler will signal other faults as either
> prefetch or data exceptions, with the FSR code 0x8 to
> distinguish them from memory translation faults (0xd).
> Future code will make use of this distinction when
> deciding to raise BusFault or MemManage exceptions.

This patch breaks my Stellaris test image -- instead of starting
it just sits there with a black screen.

I've put a copy of that test image up at
  http://people.linaro.org/~peter.maydell/stellaris.tgz
You can run it with path/to/stellaris/runme path/to/qemu-system-arm .

> ---
>  hw/arm/armv7m.c  |  8 --------
>  target-arm/cpu.c | 32 +++++++++++++++++++++-----------
>  2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index a80d2ad..68146de 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -176,7 +176,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";
> @@ -221,13 +220,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 30739fc..728854f 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -280,6 +280,25 @@ 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;
> +
> +    env->exception.vaddress = addr;
> +    env->exception.fsr = is_write ? 0x808 : 0x8; /* Precise External Abort */
> +    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && !is_write) {
> +        cpu->exception_index = EXCP_EXCEPTION_EXIT;

We could use a comment here (a) explaining what we're doing and (b)
mentioning that this isn't architecturally correct -- ideally we should
catch these exception exits on execution of the jump insn, not by
letting the jump execute and then trapping when we actually try to
execute at the magic addresses.

> +    } else if (is_exec) {
> +        cpu->exception_index = EXCP_PREFETCH_ABORT;
> +    } else {
> +        cpu->exception_index = EXCP_DATA_ABORT;
> +    }
> +    cpu_loop_exit(cpu);
> +}
> +
>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -294,19 +313,9 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>          cc->do_interrupt(cs);
>          ret = true;
>      }
> -    /* 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 when the
> -     * CPU tries to execute code at the magic address.
> -     * This will cause the magic PC value to be pushed to
> -     * the stack if an interrupt occurred at the wrong time.
> -     * We avoid this by disabling interrupts when
> -     * pc contains a magic address.
> -     */
>      if (interrupt_request & CPU_INTERRUPT_HARD
>          && !(env->daif & PSTATE_I)
> -        && (env->regs[15] < 0xfffffff0)) {
> +            ) {

Can we really drop this change? The thing it's guarding against
(interrupt comes in while the PC is this not-really-an-address
value) can still happen whether we catch the attempt to execute
in translate.c or via the unassigned-access hook.

(This isn't what's causing my test image to not run, though.)

>          cs->exception_index = EXCP_IRQ;
>          cc->do_interrupt(cs);
>          ret = true;
> @@ -909,6 +918,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;
>  }

thanks
-- PMM