On 16 December 2013 23:40, Christoffer Dall <christoffer.dall@xxxxxxxxxx> 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 <peter.maydell@xxxxxxxxxx>
>
> Minor thing below, otherwise it looks quite nice:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>
>> ---
>> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
