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

[Peter Maydell]

On 4 September 2014 19:12, Martin Galvan
<address@hidden> wrote:

Thanks for this patch. I think it's generally right
but could use a little tweaking for style issues.

> 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

Add "if the vector table was in ROM".

>. In
> particular, since Thumb was 0, a Usage Fault would arise immediately
> after trying to excecute any instruction on a Cortex-M.

"execute"

>
> Signed-off-by: Martin Galvan <address@hidden>
> ---
>
> Changed since previous version: 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 | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 5ac1bf9..9dfa50e 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -136,14 +136,22 @@ static void arm_cpu_reset(CPUState *s)
>          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.  */
> +            /* Address zero is covered by ROM which hasn't yet been
> +               copied into physical memory. */

Since we're touching these lines anyway we might as well
bring them into line with the comment style used in most
of the rest of the file for multiline comments:
     /* line one
      * line two
      */

>              env->regs[13] = ldl_p(rom) & 0xFFFFFFFC;
>              pc = ldl_p(rom + 4);
>              env->thumb = pc & 1;
>              env->regs[15] = pc & ~1;
> +        } 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

Something in your email patch submission path is folding
long lines, which means the resulting patch doesn't
apply cleanly at my end. I can fix this up, but if you're
thinking about sending more patches in future you might
want to investigate this. I recommend the 'git send-email'
tool, though it requires a little bit of configuration.

> +               a NULL pointer and we should use ldl_phys instead.
> +            */
> +            env->regs[13] = ldl_phys(s->as, 0) & 0xFFFFFFFC;
> +            pc = ldl_phys(s->as, 4);
> +            env->thumb = pc & 1;
> +            env->regs[15] = pc & ~1;

There's a lot of duplication here with the other half of the
if(); it would be nicer to have the code inside the if()
just do the work of loading the pc and sp words out of the
vector table, and then do the masking and setting of
env fields once outside it.

thanks
-- PMM