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

[Peter Crosthwaite]

CC Alistair who is into ARMv7M

On Sat, Sep 6, 2014 at 4:39 AM, 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 being set
> correctly if the vector table was in ROM. In particular, since Thumb was 0, a
> Usage Fault would arise immediately after trying to execute any instruction
> on a Cortex-M.
>
> Signed-off-by: Martin Galvan <address@hidden>
> ---
>
> Changed in v3: Fixed a few style issues.
>
> Changed in v2: We're not adding initial_MSP and
> initial_PC to the CPU state. Instead, we're loading the initial values
> using ldl_phys.
>
>  target-arm/cpu.c | 34 +++++++++++++++++++++++-------
> ----
>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 8199f32..8e9f203 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -129,26 +129,38 @@ static void arm_cpu_reset(CPUState *s)
>      env->uncached_cpsr = ARM_CPU_MODE_SVC;
>      env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
>      /* 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.  */
> +     * clear at reset. Initial SP and PC are loaded from ROM.
> +     */
>      if (IS_M(env)) {
> -        uint32_t pc;
> +        uint32_t initial_msp; /* Loaded from 0x0 */
> +        uint32_t initial_pc; /* Loaded from 0x4 */
>          uint8_t *rom;
> +
>          env->daif &= ~PSTATE_I;
>          rom = rom_ptr(0);
>          if (rom) {
> -            /* We should really use ldl_phys here, in case the guest
> -               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;
> +            /* Address zero is covered by ROM which hasn't yet been
> +             * copied into physical memory.
> +             */

Longer term (not this series) this could perhaps be better handled by
reset callback order dependance. I.e. this reset handler is dependant
on the ROM loader reset handler. We have similar issues with CPU reset
ordering around the bootloader objects (such as hw/arm/boot.c) so I
think this is more fuel on the fire for properly handling of reset
deps.

> +            initial_msp = ldl_p(rom);
> +            initial_pc = ldl_p(rom + 4);
> +        } else {
> +            /* Address zero not covered by a ROM blob, or the ROM blob
> +             * is in non-modifiable memory and this is a second reset after
> +             * it got copied into memory. In the latter case, rom_ptr
> +             * will return a NULL pointer and we should use ldl_phys instead.
> +             */
> +            initial_msp = ldl_phys(s->as, 0);
> +            initial_pc = ldl_phys(s->as, 4);
>          }
> +
> +        env->regs[13] = initial_msp & 0xFFFFFFFC;
> +        env->regs[15] = initial_pc & ~1;
> +        env->thumb = initial_pc & 1; /* Must always be 1 for ARMv7-M */

I though some M profile variants supported non-thumb too? If it is a
must, then is should report an error of some form.

>      }
>
>      if (env->cp15.c1_sys & SCTLR_V) {
> -            env->regs[15] = 0xFFFF0000;
> +        env->regs[15] = 0xFFFF0000;

Out of scope change.

Regards,
Peter

>      }
>
>      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> --
> 1.9.1
>
> --
>
> Martín Galván
>
> Software Engineer
>
> Taller Technologies Argentina
>
>
> San Lorenzo 47, 3rd Floor, Office 5
>
> Córdoba, Argentina
>
> Phone: 54 351 4217888 / +54 351 4218211
>