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 <peter.mayd...@linaro.org> wrote:
> On 4 September 2014 19:12, Martin Galvan
> <martin.gal...@tallertechnologies.com> 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 <martin.gal...@tallertechnologies.com>
>> ---
>>
>> 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
