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

[Martin Galvan]

Once again, you're right. I'll fix that right away and send v3.

On Fri, Sep 5, 2014 at 2:43 PM, Peter Maydell <address@hidden> wrote:
> 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



-- 


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