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

[Martin Galvan]

On Sat, Sep 6, 2014 at 8:00 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Sat, Sep 6, 2014 at 8:42 PM, Peter Maydell <address@hidden> wrote:
>> On 6 September 2014 03:45, Peter Crosthwaite
>> <address@hidden> wrote:
>>> CC Alistair who is into ARMv7M
>>>
>>> On Sat, Sep 6, 2014 at 4:39 AM, Martin Galvan
>>> <address@hidden> 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?

-- 

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