[v3] target-arm: Fix resetting issues on ARMv7-M CPUs

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 <martin.galvan@tallertechnologies.com>
---

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(-)

--
1.9.1

CC Alistair who is into ARMv7M

On Sat, Sep 6, 2014 at 4:39 AM, 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 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 <martin.galvan@tallertechnologies.com>
> ---
>
> 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
>

On 6 September 2014 03:45, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> CC Alistair who is into ARMv7M
>
> On Sat, Sep 6, 2014 at 4:39 AM, 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 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.

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

In general I dislike how we handle M-profile reset of SP/PC, because
really what the hardware does is "CPU reads the SP and PC as
the first thing it does *after* reset", not "reads at point of reset".
The distinction starts to matter if you consider situations like
connecting with a gdb stub and using gdb to change the vector
table contents. But M-profile doesn't really rank high enough
in my list of priorities for me to think about a better approach.

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

No, all M profile cores have Thumb only, but the architecture
specifies the behaviour you get if a vector table entry doesn't
have the lowest bit set: the CPU reads the PC and clears the
Thumb bit in its PSR, then on the first attempt to execute an
instruction it takes a UsageFault (as it would for any attempt
to execute an instruction when the Thumb mode bit is clear).
You can think of it as the CPU knowing about the concept of
ARM vs Thumb mode but having no actual instructions in the
ARM decoder (though the detail of exactly what exception
we take is not quite what that would imply).

[A UsageFault taken immediately out of reset is going to be
escalated to a HardFault, but that's just following the usual
priority escalation rules, or at least it would be if we actually
implemented them...]

So the comment is actually incorrect and we should probably
delete it.

thanks
-- PMM

On Sat, Sep 6, 2014 at 8:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 September 2014 03:45, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> CC Alistair who is into ARMv7M
>>
>> On Sat, Sep 6, 2014 at 4:39 AM, 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 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.
>
>> 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.
>
> In general I dislike how we handle M-profile reset of SP/PC, because
> really what the hardware does is "CPU reads the SP and PC as
> the first thing it does *after* reset", not "reads at point of reset".
> The distinction starts to matter if you consider situations like
> connecting with a gdb stub and using gdb to change the vector
> table contents. But M-profile doesn't really rank high enough
> in my list of priorities for me to think about a better approach.
>

Does scheduling this post-reset stuff in a one-shot expired timer solve this?

By default the ARM CPU would need to be ->halted, then on reset() you
set an expired timer with handler which does SP, PC etc and clears
->halted. This if-else ROM awareness should then evaporate.

Getting slightly more radical, that may be applicable to the arm/boot
bootloader too.

>>> +            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.
>
> No, all M profile cores have Thumb only, but the architecture
> specifies the behaviour you get if a vector table entry doesn't
> have the lowest bit set: the CPU reads the PC and clears the
> Thumb bit in its PSR, then on the first attempt to execute an
> instruction it takes a UsageFault (as it would for any attempt
> to execute an instruction when the Thumb mode bit is clear).
> You can think of it as the CPU knowing about the concept of
> ARM vs Thumb mode but having no actual instructions in the
> ARM decoder (though the detail of exactly what exception
> we take is not quite what that would imply).
>
> [A UsageFault taken immediately out of reset is going to be
> escalated to a HardFault, but that's just following the usual
> priority escalation rules, or at least it would be if we actually
> implemented them...]
>
> So the comment is actually incorrect and we should probably
> delete it.

Yep, thats what I was thinking but your reasonings are more correct than mine.

Regards,
Peter

>
> thanks
> -- PMM
>

On Sat, Sep 6, 2014 at 8:00 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sat, Sep 6, 2014 at 8:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 6 September 2014 03:45, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> CC Alistair who is into ARMv7M
>>>
>>> On Sat, Sep 6, 2014 at 4:39 AM, Martin Galvan
>>> <martin.galvan@tallertechnologies.com> wrote:
>>>>      }
>>>>>
>>>>>      if (env->cp15.c1_sys & SCTLR_V) {
>>>>> -            env->regs[15] = 0xFFFF0000;
>>>>> +        env->regs[15] = 0xFFFF0000;
>>>
>>> Out of scope change.

Oh, I must've changed that and forgotten it was gonna be included on
the patch. Still, quoting
http://wiki.qemu.org/Contribute/SubmitAPatch:

"It's OK to fix coding style issues in the immediate area (few lines)
of the lines you're changing. "

>>>> +            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.
>>
>> No, all M profile cores have Thumb only, but the architecture
>> specifies the behaviour you get if a vector table entry doesn't
>> have the lowest bit set: the CPU reads the PC and clears the
>> Thumb bit in its PSR, then on the first attempt to execute an
>> instruction it takes a UsageFault (as it would for any attempt
>> to execute an instruction when the Thumb mode bit is clear).
>> You can think of it as the CPU knowing about the concept of
>> ARM vs Thumb mode but having no actual instructions in the
>> ARM decoder (though the detail of exactly what exception
>> we take is not quite what that would imply).
>>
>> [A UsageFault taken immediately out of reset is going to be
>> escalated to a HardFault, but that's just following the usual
>> priority escalation rules, or at least it would be if we actually
>> implemented them...]
>>
>> So the comment is actually incorrect and we should probably
>> delete it.
>
> Yep, thats what I was thinking but your reasonings are more correct than mine.

Should I send a new patch version, just to remove that one comment?

On 8 September 2014 13:37, Martin Galvan
<martin.galvan@tallertechnologies.com> wrote:
> On Sat, Sep 6, 2014 at 8:00 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Sat, Sep 6, 2014 at 8:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 6 September 2014 03:45, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> CC Alistair who is into ARMv7M
>>>>
>>>> On Sat, Sep 6, 2014 at 4:39 AM, Martin Galvan
>>>> <martin.galvan@tallertechnologies.com> wrote:
>>>>>      }
>>>>>>
>>>>>>      if (env->cp15.c1_sys & SCTLR_V) {
>>>>>> -            env->regs[15] = 0xFFFF0000;
>>>>>> +        env->regs[15] = 0xFFFF0000;
>>>>
>>>> Out of scope change.
>
> Oh, I must've changed that and forgotten it was gonna be included on
> the patch. Still, quoting
> http://wiki.qemu.org/Contribute/SubmitAPatch:
>
> "It's OK to fix coding style issues in the immediate area (few lines)
> of the lines you're changing. "

Yeah, this kind of thing is a judgement call. In this
case I kind of agree with Peter so I'll just split it
out into a separate patch for you when I apply this
to my target-arm tree.

>>> So the comment is actually incorrect and we should probably
>>> delete it.
>>
>> Yep, thats what I was thinking but your reasonings are more correct than mine.
>
> Should I send a new patch version, just to remove that one comment?

No, don't bother, I'll just remove it when I apply this
to target-arm.

(with the tweaks I'm going to make as above)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM