Re: [Qemu-devel] [PATCH] target-arm: Fix resetting issues on ARMv7-M CPUs

[Peter Maydell]

On 11 August 2014 17:50, Martin Galvan
<address@hidden> wrote:
> When calling qemu_system_reset after startup on a Cortex-M CPU, the initial
> values of PC, MSP and the Thumb bit weren't set correctly. In particular,
> since Thumb was 0, an Usage Fault would arise immediately after trying to
> excecute any instruction on a Cortex-M.
>
> Signed-off-by: Martin Galvan <address@hidden>
> ---
>  target-arm/cpu.c | 19 ++++++++++++++-----
>  target-arm/cpu.h |  4 ++++
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 7cebb76..d436b59 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -131,7 +131,6 @@ static void arm_cpu_reset(CPUState *s)
>      /* On ARMv7-M the CPSR_I is the value of the PRIMASK register, and is
>         clear at reset.  Initial SP and PC are loaded from ROM.  */
>      if (IS_M(env)) {
> -        uint32_t pc;
>          uint8_t *rom;
>          env->daif &= ~PSTATE_I;
>          rom = rom_ptr(0);
> @@ -140,11 +139,21 @@ static void arm_cpu_reset(CPUState *s)
>                 modified flash and reset itself.  However images
>                 loaded via -kernel have not been copied yet, so load the
>                 values directly from there.  */
> -            env->regs[13] = ldl_p(rom) & 0xFFFFFFFC;
> -            pc = ldl_p(rom + 4);
> -            env->thumb = pc & 1;
> -            env->regs[15] = pc & ~1;
> +            env->initial_MSP = ldl_p(rom) & 0xFFFFFFFC;
> +            env->initial_PC = ldl_p(rom + 4);
> +            env->initial_PC &= ~1;
>          }
> +
> +        /* If we do a system reset, rom will be NULL since its data
> +            was zeroed when calling cpu_flush_icache_range at startup. Set
> +            the initial registers here using the values we loaded from ROM
> +            at startup. */
> +        env->regs[13] = env->initial_MSP;
> +        env->regs[15] = env->initial_PC;

I'm afraid this looks like the wrong fix for the problem you're seeing.
The bug you need to fix is that the ROM contents got zeroed.
The reset code is correct to reload SP and PC from memory --
this is what the hardware does.

> +
> +        /* ARMv7-M only supports Thumb instructions. If this isn't
> +           set we'll get an Usage Fault. */
> +        env->thumb = 1;

It's true that if the thumb bit isn't set we get a usage fault, but
that is correct behaviour if the PC value in the vector table
doesn't have its low bit set. (See the TakeReset() pseudocode
in the ARMv7M ARM ARM.)

>      }
>
>      if (env->cp15.c1_sys & SCTLR_V) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 79205ba..a56aebd 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -330,6 +330,10 @@ typedef struct CPUARMState {
>
>      void *nvic;
>      const struct arm_boot_info *boot_info;
> +
> +    /* Initial MSP and PC for ARMv7-M CPUs */
> +    uint32_t initial_MSP; /* Stored in 0x0 inside the guest ROM */
> +    uint32_t initial_PC; /* Stored in 0x4 inside the guest ROM */
>  } CPUARMState;

This definitely looks wrong. The starting PC and SP are not CPU state.

thanks
-- PMM