[Qemu-devel] [PATCH target-arm] arm: armv7m: Respect elf entry point

[Qemu-devel] [PATCH target-arm] arm: armv7m: Respect elf entry point

[Peter Crosthwaite]

ARMv7M has it's own bootloader (separate from the regular ARM
bootloader) that is elf aware. It is able to load elfs but it does
not set the program counter to the elf entry point. Make it more
consistent with the regular ARM bootloader by setting the program
counter to the given elf entry point.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 hw/arm/armv7m.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 397e8df..d1b983f 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -155,11 +155,18 @@ static void armv7m_bitband_init(void)
 
 /* Board init.  */
 
+typedef struct ARMV7MResetArgs {
+    ARMCPU *cpu;
+    uint32_t reset_pc;
+} ARMV7MResetArgs;
+
 static void armv7m_reset(void *opaque)
 {
-    ARMCPU *cpu = opaque;
+    ARMV7MResetArgs *args = opaque;
 
-    cpu_reset(CPU(cpu));
+    cpu_reset(CPU(args->cpu));
+    args->cpu->env.regs[15] = args->reset_pc;
+    args->cpu->env.thumb = args->reset_pc & 1;
 }
 
 /* Init CPU and memory for a v7-M based board.
@@ -183,6 +190,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
     MemoryRegion *sram = g_new(MemoryRegion, 1);
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *hack = g_new(MemoryRegion, 1);
+    ARMV7MResetArgs reset_args;
 
     flash_size *= 1024;
     sram_size *= 1024;
@@ -259,7 +267,12 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
     vmstate_register_ram_global(hack);
     memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
 
-    qemu_register_reset(armv7m_reset, cpu);
+    reset_args = (ARMV7MResetArgs) {
+        .cpu = cpu,
+        .reset_pc = entry,
+    };
+    qemu_register_reset(armv7m_reset,
+                        g_memdup(&reset_args, sizeof(reset_args)));
     return pic;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH target-arm] arm: armv7m: Respect elf entry point

[Alistair Francis]

On Sat, Aug 2, 2014 at 9:41 AM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> ARMv7M has it's own bootloader (separate from the regular ARM
> bootloader) that is elf aware. It is able to load elfs but it does
> not set the program counter to the elf entry point. Make it more
> consistent with the regular ARM bootloader by setting the program
> counter to the given elf entry point.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  hw/arm/armv7m.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 397e8df..d1b983f 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -155,11 +155,18 @@ static void armv7m_bitband_init(void)
>
>  /* Board init.  */
>
> +typedef struct ARMV7MResetArgs {
> +    ARMCPU *cpu;
> +    uint32_t reset_pc;
> +} ARMV7MResetArgs;
> +
>  static void armv7m_reset(void *opaque)
>  {
> -    ARMCPU *cpu = opaque;
> +    ARMV7MResetArgs *args = opaque;
>
> -    cpu_reset(CPU(cpu));
> +    cpu_reset(CPU(args->cpu));
> +    args->cpu->env.regs[15] = args->reset_pc;
> +    args->cpu->env.thumb = args->reset_pc & 1;
>  }

This breaks a simple test case that I have. It now starts at a an
address one one bit larger but causes the program to hang.
If the 'cpu_reset(CPU(args->cpu))' is moved to the end of the function
the test boots (the same as it did before).

>
>  /* Init CPU and memory for a v7-M based board.
> @@ -183,6 +190,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>      MemoryRegion *sram = g_new(MemoryRegion, 1);
>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>      MemoryRegion *hack = g_new(MemoryRegion, 1);
> +    ARMV7MResetArgs reset_args;
>
>      flash_size *= 1024;
>      sram_size *= 1024;
> @@ -259,7 +267,12 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>      vmstate_register_ram_global(hack);
>      memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
>
> -    qemu_register_reset(armv7m_reset, cpu);
> +    reset_args = (ARMV7MResetArgs) {
> +        .cpu = cpu,
> +        .reset_pc = entry,
> +    };
> +    qemu_register_reset(armv7m_reset,
> +                        g_memdup(&reset_args, sizeof(reset_args)));
>      return pic;
>  }
>
> --
> 1.9.1
>

Re: [Qemu-devel] [PATCH target-arm] arm: armv7m: Respect elf entry point

[Peter Crosthwaite]

On Mon, Aug 4, 2014 at 5:54 PM, Alistair Francis <alistair23@gmail.com> wrote:
> On Sat, Aug 2, 2014 at 9:41 AM, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> ARMv7M has it's own bootloader (separate from the regular ARM
>> bootloader) that is elf aware. It is able to load elfs but it does
>> not set the program counter to the elf entry point. Make it more
>> consistent with the regular ARM bootloader by setting the program
>> counter to the given elf entry point.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>  hw/arm/armv7m.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
>> index 397e8df..d1b983f 100644
>> --- a/hw/arm/armv7m.c
>> +++ b/hw/arm/armv7m.c
>> @@ -155,11 +155,18 @@ static void armv7m_bitband_init(void)
>>
>>  /* Board init.  */
>>
>> +typedef struct ARMV7MResetArgs {
>> +    ARMCPU *cpu;
>> +    uint32_t reset_pc;
>> +} ARMV7MResetArgs;
>> +
>>  static void armv7m_reset(void *opaque)
>>  {
>> -    ARMCPU *cpu = opaque;
>> +    ARMV7MResetArgs *args = opaque;
>>
>> -    cpu_reset(CPU(cpu));
>> +    cpu_reset(CPU(args->cpu));
>> +    args->cpu->env.regs[15] = args->reset_pc;
>> +    args->cpu->env.thumb = args->reset_pc & 1;
>>  }
>
> This breaks a simple test case that I have.

Can you send me a replicator? I'll give it a go.

> It now starts at a an
> address one one bit larger but causes the program to hang.
> If the 'cpu_reset(CPU(args->cpu))' is moved to the end of the function
> the test boots (the same as it did before).
>

Hmm but I think that loses you the elf entry point again.

Regards,
Peter

>>
>>  /* Init CPU and memory for a v7-M based board.
>> @@ -183,6 +190,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>>      MemoryRegion *sram = g_new(MemoryRegion, 1);
>>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>>      MemoryRegion *hack = g_new(MemoryRegion, 1);
>> +    ARMV7MResetArgs reset_args;
>>
>>      flash_size *= 1024;
>>      sram_size *= 1024;
>> @@ -259,7 +267,12 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>>      vmstate_register_ram_global(hack);
>>      memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
>>
>> -    qemu_register_reset(armv7m_reset, cpu);
>> +    reset_args = (ARMV7MResetArgs) {
>> +        .cpu = cpu,
>> +        .reset_pc = entry,
>> +    };
>> +    qemu_register_reset(armv7m_reset,
>> +                        g_memdup(&reset_args, sizeof(reset_args)));
>>      return pic;
>>  }
>>
>> --
>> 1.9.1
>>
>

Re: [Qemu-devel] [PATCH target-arm] arm: armv7m: Respect elf entry point

[Peter Maydell]

On 2 August 2014 00:41, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> ARMv7M has it's own bootloader (separate from the regular ARM
> bootloader) that is elf aware. It is able to load elfs but it does
> not set the program counter to the elf entry point. Make it more
> consistent with the regular ARM bootloader by setting the program
> counter to the given elf entry point.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>  hw/arm/armv7m.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 397e8df..d1b983f 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -155,11 +155,18 @@ static void armv7m_bitband_init(void)
>
>  /* Board init.  */
>
> +typedef struct ARMV7MResetArgs {
> +    ARMCPU *cpu;
> +    uint32_t reset_pc;
> +} ARMV7MResetArgs;
> +
>  static void armv7m_reset(void *opaque)
>  {
> -    ARMCPU *cpu = opaque;
> +    ARMV7MResetArgs *args = opaque;
>
> -    cpu_reset(CPU(cpu));
> +    cpu_reset(CPU(args->cpu));
> +    args->cpu->env.regs[15] = args->reset_pc;
> +    args->cpu->env.thumb = args->reset_pc & 1;

This looks odd. If the entry point has bit 0 being the
Thumb bit, then shouldn't we be masking it out the
same way we do in the A-profile do_cpu_reset() ?

>  }
>
>  /* Init CPU and memory for a v7-M based board.
> @@ -183,6 +190,7 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>      MemoryRegion *sram = g_new(MemoryRegion, 1);
>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>      MemoryRegion *hack = g_new(MemoryRegion, 1);
> +    ARMV7MResetArgs reset_args;
>
>      flash_size *= 1024;
>      sram_size *= 1024;
> @@ -259,7 +267,12 @@ qemu_irq *armv7m_init(MemoryRegion *address_space_mem,
>      vmstate_register_ram_global(hack);
>      memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
>
> -    qemu_register_reset(armv7m_reset, cpu);
> +    reset_args = (ARMV7MResetArgs) {
> +        .cpu = cpu,
> +        .reset_pc = entry,
> +    };
> +    qemu_register_reset(armv7m_reset,
> +                        g_memdup(&reset_args, sizeof(reset_args)));

Why the local variable and memdup rather than just
g_new0() and set the fields in the allocated struct?

thanks
-- PMM