Re: [Qemu-devel] [PATCH 5/7] hw/arm/boot: Allow easier swapping in of different loader code

[Peter Maydell]

On 16 December 2013 23:40, Christoffer Dall <address@hidden> wrote:
> On Thu, Nov 28, 2013 at 01:33:20PM +0000, Peter Maydell wrote:
>> For AArch64 we will obviously require a different set of
>> primary and secondary boot loader code fragments. However currently
>> we hardcode the offsets into the loader code where we must write
>> the entrypoint and other data into arm_load_kernel(). This makes it
>> hard to substitute a different loader fragment, so switch to a more
>> flexible scheme where instead of a raw array of instructions we use
>> an array of (instruction, fixup-type) pairs that indicate which
>> words need special action or data written into them.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>
> Minor thing below, otherwise it looks quite nice:
>
> Reviewed-by: Christoffer Dall <address@hidden>
>
>> ---
>>  hw/arm/boot.c |  152 
>> ++++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 107 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 55d552f..77d29a8 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -20,15 +20,33 @@
>>  #define KERNEL_ARGS_ADDR 0x100
>>  #define KERNEL_LOAD_ADDR 0x00010000
>>
>> +typedef enum {
>> +    FIXUP_NONE = 0,   /* do nothing */
>> +    FIXUP_TERMINATOR, /* end of insns */
>> +    FIXUP_BOARDID,    /* overwrite with board ID number */
>> +    FIXUP_ARGPTR,     /* overwrite with pointer to kernel args */
>> +    FIXUP_ENTRYPOINT, /* overwrite with kernel entry point */
>> +    FIXUP_GIC_CPU_IF, /* overwrite with GIC CPU interface address */
>> +    FIXUP_BOOTREG,    /* overwrite with boot register address */
>> +    FIXUP_DSB,        /* overwrite with correct DSB insn for cpu */
>> +    FIXUP_MAX,
>> +} FixupType;
>> +
>> +typedef struct ARMInsnFixup {
>> +    uint32_t insn;
>> +    FixupType fixup;
>> +} ARMInsnFixup;
>> +
>>  /* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  
>> */
>> -static uint32_t bootloader[] = {
>> -  0xe3a00000, /* mov     r0, #0 */
>> -  0xe59f1004, /* ldr     r1, [pc, #4] */
>> -  0xe59f2004, /* ldr     r2, [pc, #4] */
>> -  0xe59ff004, /* ldr     pc, [pc, #4] */
>> -  0, /* Board ID */
>> -  0, /* Address of kernel args.  Set by integratorcp_init.  */
>> -  0  /* Kernel entry point.  Set by integratorcp_init.  */
>> +static const ARMInsnFixup bootloader[] = {
>> +    { 0xe3a00000 }, /* mov     r0, #0 */
>> +    { 0xe59f1004 }, /* ldr     r1, [pc, #4] */
>> +    { 0xe59f2004 }, /* ldr     r2, [pc, #4] */
>> +    { 0xe59ff004 }, /* ldr     pc, [pc, #4] */
>> +    { 0, FIXUP_BOARDID },
>> +    { 0, FIXUP_ARGPTR },
>> +    { 0, FIXUP_ENTRYPOINT },
>> +    { 0, FIXUP_TERMINATOR }
>>  };
>>
>>  /* Handling for secondary CPU boot in a multicore system.
>> @@ -48,39 +66,83 @@ static uint32_t bootloader[] = {
>>  #define DSB_INSN 0xf57ff04f
>>  #define CP15_DSB_INSN 0xee070f9a /* mcr cp15, 0, r0, c7, c10, 4 */
>>
>> -static uint32_t smpboot[] = {
>> -  0xe59f2028, /* ldr r2, gic_cpu_if */
>> -  0xe59f0028, /* ldr r0, startaddr */
>> -  0xe3a01001, /* mov r1, #1 */
>> -  0xe5821000, /* str r1, [r2] - set GICC_CTLR.Enable */
>> -  0xe3a010ff, /* mov r1, #0xff */
>> -  0xe5821004, /* str r1, [r2, 4] - set GIC_PMR.Priority to 0xff */
>> -  DSB_INSN,   /* dsb */
>> -  0xe320f003, /* wfi */
>> -  0xe5901000, /* ldr     r1, [r0] */
>> -  0xe1110001, /* tst     r1, r1 */
>> -  0x0afffffb, /* beq     <wfi> */
>> -  0xe12fff11, /* bx      r1 */
>> -  0,          /* gic_cpu_if: base address of GIC CPU interface */
>> -  0           /* bootreg: Boot register address is held here */
>> +static const ARMInsnFixup smpboot[] = {
>> +    { 0xe59f2028 }, /* ldr r2, gic_cpu_if */
>> +    { 0xe59f0028 }, /* ldr r0, startaddr */
>> +    { 0xe3a01001 }, /* mov r1, #1 */
>> +    { 0xe5821000 }, /* str r1, [r2] - set GICC_CTLR.Enable */
>> +    { 0xe3a010ff }, /* mov r1, #0xff */
>> +    { 0xe5821004 }, /* str r1, [r2, 4] - set GIC_PMR.Priority to 0xff */
>> +    { 0, FIXUP_DSB },   /* dsb */
>> +    { 0xe320f003 }, /* wfi */
>> +    { 0xe5901000 }, /* ldr     r1, [r0] */
>> +    { 0xe1110001 }, /* tst     r1, r1 */
>> +    { 0x0afffffb }, /* beq     <wfi> */
>> +    { 0xe12fff11 }, /* bx      r1 */
>> +    { 0, FIXUP_GIC_CPU_IF },
>> +    { 0, FIXUP_BOOTREG },
>
> couldn't we add the gic_cpu_if and startaddr "labels" as comments to the
> two lines above?  (alternatively also rename the reference to startaddr
> to bootret in the second instruction comment).

Yeah, I figured there wasn't any need to comment since the FIXUP
constant name made the meaning obvious but I forgot about the
reference to the labels in the code comments. I'll change the
startaddr reference to bootreg.

thanks
-- PMM