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

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

[Martin Galvan]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

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 <martin.galvan@tallertechnologies.com>
---
 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;
+
+        /* ARMv7-M only supports Thumb instructions. If this isn't
+           set we'll get an Usage Fault. */
+        env->thumb = 1;
     }

     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;

 #include "cpu-qom.h"
--
1.9.1

[-- Attachment #2: Type: text/html, Size: 3028 bytes --]

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

[Peter Maydell]

On 11 August 2014 17:50, Martin Galvan
<martin.galvan@tallertechnologies.com> 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 <martin.galvan@tallertechnologies.com>
> ---
>  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

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
<peter.maydell@linaro.org> wrote:
>
> On 11 August 2014 17:50, Martin Galvan
> <martin.galvan@tallertechnologies.com> 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 <martin.galvan@tallertechnologies.com>
> > ---
> >  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.

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

[Peter Maydell]

On 19 August 2014 14:25, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Tue, Aug 19, 2014 at 10:06 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> 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.

Why do you think cpu_flush_icache_range has anything to do
with this? All that does is ensure that the host's instruction
cache has no stale contents for the ROM region. It doesn't
zero anything. (It's mostly there for the benefit of KVM, not TCG.)

>> > +
>> > +        /* 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?

Yes. You'll notice that this is exactly what the current code does...

thanks
-- PMM

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

[Martin Galvan]

On Tue, Aug 19, 2014 at 11:16 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 19 August 2014 14:25, Martin Galvan
> <martin.galvan@tallertechnologies.com> wrote:
>> On Tue, Aug 19, 2014 at 10:06 AM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> 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.
>
> Why do you think cpu_flush_icache_range has anything to do
> with this? All that does is ensure that the host's instruction
> cache has no stale contents for the ROM region. It doesn't
> zero anything. (It's mostly there for the benefit of KVM, not TCG.)
>

Indeed, I missed the g_free just above the call to
cpu_flush_icache_range in rom_reset. Is there any particular reason
why we're doing that g_free?

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

[Martin Galvan]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

Ping http://patchwork.ozlabs.org/patch/379134/


On Mon, Aug 11, 2014 at 1:50 PM, Martin Galvan <
martin.galvan@tallertechnologies.com> 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 <martin.galvan@tallertechnologies.com>
> ---
>  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;
> +
> +        /* ARMv7-M only supports Thumb instructions. If this isn't
> +           set we'll get an Usage Fault. */
> +        env->thumb = 1;
>      }
>
>      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;
>
>  #include "cpu-qom.h"
> --
> 1.9.1
>



-- 

[image: http://www.tallertechnologies.com]
<http://www.tallertechnologies.com>

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

[image: http://www.linkedin.com/company/taller-technologies]
<http://www.linkedin.com/company/taller-technologies>[image:
https://www.facebook.com/tallertechnologies]
<https://www.facebook.com/tallertechnologies>

[-- Attachment #2: Type: text/html, Size: 7506 bytes --]