Re: [Qemu-devel] [PATCH v1 1/1] target_arm: Make the reset rom_ptr a property

[Alistair Francis <alistair23@gmail.com>]

On Thu, Aug 21, 2014 at 1:37 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Aug 21, 2014 at 11:40 AM, Alistair Francis <alistair23@gmail.com> wrote:
>> Add Konstanty
>>
>>
>> On Thu, Aug 21, 2014 at 11:36 AM, Alistair Francis <alistair23@gmail.com> wrote:
>>> This allows the board to set the reset address, which is required
>>> for some boards (the Netduino Plus 2 for example)
>>>
>
> The change is armv7m specific right? You should mention that in the
> commit message.
>
> This also obsoletes my attempt at correcting armv7m elf loading which
> was trying to solve this issue a different way.
>
>>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>
> Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>>> ---
>>> At the moment nothing requires this change, but I have a machine
>>> model that I'm working on that requires this
>>>
>>> Thanks to Peter C for spotting this issue
>>>
>>>  target-arm/cpu-qom.h | 1 +
>>>  target-arm/cpu.c     | 3 ++-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>>> index 07f3c9e..7e415f5 100644
>>> --- a/target-arm/cpu-qom.h
>>> +++ b/target-arm/cpu-qom.h
>>> @@ -138,6 +138,7 @@ typedef struct ARMCPU {
>>>      uint32_t id_isar3;
>>>      uint32_t id_isar4;
>>>      uint32_t id_isar5;
>>> +    uint32_t rom_address;
>
> Any reason for this to go in amongst the ID register fields? It seems
> in a class of its own, and the closest thing to it would be rvbar
> which is down the bottom. My gut says it should be last field in the
> struct.
>
>>>      uint64_t id_aa64pfr0;
>>>      uint64_t id_aa64pfr1;
>>>      uint64_t id_aa64dfr0;
>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>> index 8199f32..29f9473 100644
>>> --- a/target-arm/cpu.c
>>> +++ b/target-arm/cpu.c
>>> @@ -134,7 +134,7 @@ static void arm_cpu_reset(CPUState *s)
>>>          uint32_t pc;
>>>          uint8_t *rom;
>>>          env->daif &= ~PSTATE_I;
>>> -        rom = rom_ptr(0);
>>> +        rom = rom_ptr(cpu->rom_address);
>>>          if (rom) {
>>>              /* We should really use ldl_phys here, in case the guest
>>>                 modified flash and reset itself.  However images
>>> @@ -1020,6 +1020,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>>  static Property arm_cpu_properties[] = {
>>>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>>>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>>> +    DEFINE_PROP_UINT32("rom-address", ARMCPU, rom_address, 0),
>
> This should be an armv7m only property added in arm_cpu_post_init().
>
> Regards,
> Peter

I agree with all of your comments, will fix in the next version. I'll
leave it a day or so to see if anyone else has any input

Thanks,

Alistair

>
>>>      DEFINE_PROP_END_OF_LIST()
>>>  };
>>>
>>> --
>>> 1.9.1
>>>
>>