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

[Martin Galvan]

On Tue, Aug 19, 2014 at 10:06 AM, Peter Maydell
<address@hidden> wrote:
>
> 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.

Indeed, but aren't the ROM contents supposed to get zeroed? Otherwise,
why would we call cpu_flish_icache_range? I'm afraid "fixing" that may
have some unwanted side effects.

> > +
> > +        /* 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.)

Alright. I suppose env->thumb = pc & 1 should do the trick?

> >      }
> >
> >      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.

The alternative was to use static variables which would get set on
startup, but that was even uglier. It' doesn't really matter now,
though, since the problem lies somewhere else.