| Message ID | 1385645602-18662-6-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 28, 2013 at 11:33 PM, Peter Maydell <peter.maydell@linaro.org> 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. > Why do we need blobs at all? Cant we just fix arm/boot to directly setup the CPU state to the desired? Rather than complex blobs that execute ARM instructions just manipulate the regs directly. The booloader already directly accesses r15 for setting the boot blob entry point so I don't see whats wrong with setting board-id and dtb directly either. See the microblaze bootloader for an example. Regards, Peter > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > 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 }, > + { 0, FIXUP_TERMINATOR } > }; > > +static void write_bootloader(const char *name, hwaddr addr, > + const ARMInsnFixup *insns, uint32_t *fixupcontext) > +{ > + /* Fix up the specified bootloader fragment and write it into > + * guest memory using rom_add_blob_fixed(). fixupcontext is > + * an array giving the values to write in for the fixup types > + * which write a value into the code array. > + */ > + int i, len; > + uint32_t *code; > + > + len = 0; > + while (insns[len].fixup != FIXUP_TERMINATOR) { > + len++; > + } > + > + code = g_new0(uint32_t, len); > + > + for (i = 0; i < len; i++) { > + uint32_t insn = insns[i].insn; > + FixupType fixup = insns[i].fixup; > + > + switch (fixup) { > + case FIXUP_NONE: > + break; > + case FIXUP_BOARDID: > + case FIXUP_ARGPTR: > + case FIXUP_ENTRYPOINT: > + case FIXUP_GIC_CPU_IF: > + case FIXUP_BOOTREG: > + case FIXUP_DSB: > + insn = fixupcontext[fixup]; > + break; > + default: > + abort(); > + } > + code[i] = tswap32(insn); > + } > + > + rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr); > + > + g_free(code); > +} > + > static void default_write_secondary(ARMCPU *cpu, > const struct arm_boot_info *info) > { > - int n; > - smpboot[ARRAY_SIZE(smpboot) - 1] = info->smp_bootreg_addr; > - smpboot[ARRAY_SIZE(smpboot) - 2] = info->gic_cpu_if_addr; > - for (n = 0; n < ARRAY_SIZE(smpboot); n++) { > - /* Replace DSB with the pre-v7 DSB if necessary. */ > - if (!arm_feature(&cpu->env, ARM_FEATURE_V7) && > - smpboot[n] == DSB_INSN) { > - smpboot[n] = CP15_DSB_INSN; > - } > - smpboot[n] = tswap32(smpboot[n]); > + uint32_t fixupcontext[FIXUP_MAX]; > + > + fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr; > + fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr; > + if (arm_feature(&cpu->env, ARM_FEATURE_V7)) { > + fixupcontext[FIXUP_DSB] = DSB_INSN; > + } else { > + fixupcontext[FIXUP_DSB] = CP15_DSB_INSN; > } > - rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot), > - info->smp_loader_start); > + > + write_bootloader("smpboot", info->smp_loader_start, > + smpboot, fixupcontext); > } > > static void default_reset_secondary(ARMCPU *cpu, > @@ -354,7 +416,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > CPUState *cs = CPU(cpu); > int kernel_size; > int initrd_size; > - int n; > int is_linux = 0; > uint64_t elf_entry; > hwaddr entry; > @@ -420,6 +481,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > } > info->entry = entry; > if (is_linux) { > + uint32_t fixupcontext[FIXUP_MAX]; > + > if (info->initrd_filename) { > initrd_size = load_ramdisk(info->initrd_filename, > info->initrd_start, > @@ -441,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > } > info->initrd_size = initrd_size; > > - bootloader[4] = info->board_id; > + fixupcontext[FIXUP_BOARDID] = info->board_id; > > /* for device tree boot, we pass the DTB directly in r2. Otherwise > * we point to the kernel args. > @@ -456,9 +519,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > if (load_dtb(dtb_start, info)) { > exit(1); > } > - bootloader[5] = dtb_start; > + fixupcontext[FIXUP_ARGPTR] = dtb_start; > } else { > - bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; > + fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR; > if (info->ram_size >= (1ULL << 32)) { > fprintf(stderr, "qemu: RAM size must be less than 4GB to boot" > " Linux kernel using ATAGS (try passing a device tree" > @@ -466,12 +529,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > exit(1); > } > } > - bootloader[6] = entry; > - for (n = 0; n < sizeof(bootloader) / 4; n++) { > - bootloader[n] = tswap32(bootloader[n]); > - } > - rom_add_blob_fixed("bootloader", bootloader, sizeof(bootloader), > - info->loader_start); > + fixupcontext[FIXUP_ENTRYPOINT] = entry; > + > + write_bootloader("bootloader", info->loader_start, > + bootloader, fixupcontext); > + > if (info->nb_cpus > 1) { > info->write_secondary_boot(cpu, info); > } > -- > 1.7.9.5 > >
On 13 December 2013 03:19, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Why do we need blobs at all? Cant we just fix arm/boot to directly > setup the CPU state to the desired? Rather than complex blobs that > execute ARM instructions just manipulate the regs directly. We could in theory do this for the primary bootloader, but the secondary CPU blob has to have a loop in it so we can sit around waiting for the guest code running in the primary to tell us it's time to go: >> +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 }, >> + { 0, FIXUP_TERMINATOR } >> }; We're also writing to devices here, and it's cleaner to do that by running a guest code instruction than by somehow having the boot code ferret around inside the device's implementation pre-start, I think. thanks -- PMM
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@linaro.org> Minor thing below, otherwise it looks quite nice: Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > 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). > + { 0, FIXUP_TERMINATOR } > }; > > +static void write_bootloader(const char *name, hwaddr addr, > + const ARMInsnFixup *insns, uint32_t *fixupcontext) > +{ > + /* Fix up the specified bootloader fragment and write it into > + * guest memory using rom_add_blob_fixed(). fixupcontext is > + * an array giving the values to write in for the fixup types > + * which write a value into the code array. > + */ > + int i, len; > + uint32_t *code; > + > + len = 0; > + while (insns[len].fixup != FIXUP_TERMINATOR) { > + len++; > + } > + > + code = g_new0(uint32_t, len); > + > + for (i = 0; i < len; i++) { > + uint32_t insn = insns[i].insn; > + FixupType fixup = insns[i].fixup; > + > + switch (fixup) { > + case FIXUP_NONE: > + break; > + case FIXUP_BOARDID: > + case FIXUP_ARGPTR: > + case FIXUP_ENTRYPOINT: > + case FIXUP_GIC_CPU_IF: > + case FIXUP_BOOTREG: > + case FIXUP_DSB: > + insn = fixupcontext[fixup]; > + break; > + default: > + abort(); > + } > + code[i] = tswap32(insn); > + } > + > + rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr); > + > + g_free(code); > +} > + > static void default_write_secondary(ARMCPU *cpu, > const struct arm_boot_info *info) > { > - int n; > - smpboot[ARRAY_SIZE(smpboot) - 1] = info->smp_bootreg_addr; > - smpboot[ARRAY_SIZE(smpboot) - 2] = info->gic_cpu_if_addr; > - for (n = 0; n < ARRAY_SIZE(smpboot); n++) { > - /* Replace DSB with the pre-v7 DSB if necessary. */ > - if (!arm_feature(&cpu->env, ARM_FEATURE_V7) && > - smpboot[n] == DSB_INSN) { > - smpboot[n] = CP15_DSB_INSN; > - } > - smpboot[n] = tswap32(smpboot[n]); > + uint32_t fixupcontext[FIXUP_MAX]; > + > + fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr; > + fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr; > + if (arm_feature(&cpu->env, ARM_FEATURE_V7)) { > + fixupcontext[FIXUP_DSB] = DSB_INSN; > + } else { > + fixupcontext[FIXUP_DSB] = CP15_DSB_INSN; > } > - rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot), > - info->smp_loader_start); > + > + write_bootloader("smpboot", info->smp_loader_start, > + smpboot, fixupcontext); > } > > static void default_reset_secondary(ARMCPU *cpu, > @@ -354,7 +416,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > CPUState *cs = CPU(cpu); > int kernel_size; > int initrd_size; > - int n; > int is_linux = 0; > uint64_t elf_entry; > hwaddr entry; > @@ -420,6 +481,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > } > info->entry = entry; > if (is_linux) { > + uint32_t fixupcontext[FIXUP_MAX]; > + > if (info->initrd_filename) { > initrd_size = load_ramdisk(info->initrd_filename, > info->initrd_start, > @@ -441,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > } > info->initrd_size = initrd_size; > > - bootloader[4] = info->board_id; > + fixupcontext[FIXUP_BOARDID] = info->board_id; > > /* for device tree boot, we pass the DTB directly in r2. Otherwise > * we point to the kernel args. > @@ -456,9 +519,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > if (load_dtb(dtb_start, info)) { > exit(1); > } > - bootloader[5] = dtb_start; > + fixupcontext[FIXUP_ARGPTR] = dtb_start; > } else { > - bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; > + fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR; > if (info->ram_size >= (1ULL << 32)) { > fprintf(stderr, "qemu: RAM size must be less than 4GB to boot" > " Linux kernel using ATAGS (try passing a device tree" > @@ -466,12 +529,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > exit(1); > } > } > - bootloader[6] = entry; > - for (n = 0; n < sizeof(bootloader) / 4; n++) { > - bootloader[n] = tswap32(bootloader[n]); > - } > - rom_add_blob_fixed("bootloader", bootloader, sizeof(bootloader), > - info->loader_start); > + fixupcontext[FIXUP_ENTRYPOINT] = entry; > + > + write_bootloader("bootloader", info->loader_start, > + bootloader, fixupcontext); > + > if (info->nb_cpus > 1) { > info->write_secondary_boot(cpu, info); > } > -- > 1.7.9.5 > >
On 16 December 2013 23:40, Christoffer Dall <christoffer.dall@linaro.org> 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@linaro.org> > > Minor thing below, otherwise it looks quite nice: > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > >> --- >> 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
On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 December 2013 03:19, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> Why do we need blobs at all? Cant we just fix arm/boot to directly >> setup the CPU state to the desired? Rather than complex blobs that >> execute ARM instructions just manipulate the regs directly. > > We could in theory do this for the primary bootloader, but > the secondary CPU blob has to have a loop in it so we > can sit around waiting for the guest code running in the > primary to tell us it's time to go: > >>> +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 }, Reading up on Christopher's Kernel documentation link: Documentation/arm64/booting.txt Documentation/arm/Booting I can't see any reference to GIC, where are these GICisms coming from? >>> + { 0, FIXUP_BOOTREG }, >>> + { 0, FIXUP_TERMINATOR } >>> }; > > We're also writing to devices here, and it's cleaner to do that > by running a guest code instruction than by somehow having > the boot code ferret around inside the device's implementation > pre-start, I think. dma_memory_write(&address_space_memory, ... Its a level above ferreting, and a level below the machine code blob. Regards, Peter > > thanks > -- PMM >
On 17 December 2013 00:52, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 13 December 2013 03:19, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> Why do we need blobs at all? Cant we just fix arm/boot to directly >>> setup the CPU state to the desired? Rather than complex blobs that >>> execute ARM instructions just manipulate the regs directly. >> >> We could in theory do this for the primary bootloader, but >> the secondary CPU blob has to have a loop in it so we >> can sit around waiting for the guest code running in the >> primary to tell us it's time to go: >> >>>> +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 }, > > > Reading up on Christopher's Kernel documentation link: > > Documentation/arm64/booting.txt > Documentation/arm/Booting > > I can't see any reference to GIC, where are these GICisms coming from? v7 secondary CPU boot protocol is platform specific, though most boards seem to do something vaguely vexpress like. The kernel expects that we've set the GIC up so that the primary CPU can do an IPI to get the secondary out of the holding pen loop (that's the "wfi / ldr /tst / beq" sequence, which loops waiting for a CPU interrupt). >>>> + { 0, FIXUP_BOOTREG }, >>>> + { 0, FIXUP_TERMINATOR } >>>> }; >> >> We're also writing to devices here, and it's cleaner to do that >> by running a guest code instruction than by somehow having >> the boot code ferret around inside the device's implementation >> pre-start, I think. > > dma_memory_write(&address_space_memory, ... > > Its a level above ferreting, and a level below the machine code blob. This doesn't work in the reset hook because you can't guarantee that this reset hook gets run after the device resets itself rather than before... thanks -- PMM
On Tue, Dec 17, 2013 at 10:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 December 2013 00:52, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 13 December 2013 03:19, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> Why do we need blobs at all? Cant we just fix arm/boot to directly >>>> setup the CPU state to the desired? Rather than complex blobs that >>>> execute ARM instructions just manipulate the regs directly. >>> >>> We could in theory do this for the primary bootloader, but >>> the secondary CPU blob has to have a loop in it so we >>> can sit around waiting for the guest code running in the >>> primary to tell us it's time to go: >>> >>>>> +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 }, >> >> >> Reading up on Christopher's Kernel documentation link: >> >> Documentation/arm64/booting.txt >> Documentation/arm/Booting >> >> I can't see any reference to GIC, where are these GICisms coming from? > > v7 secondary CPU boot protocol is platform specific, > though most boards seem to do something vaguely > vexpress like. So Zynq is very different. It just rewrites the vector table and resets the secondarys from peripherals rst controller. The kernel expects that we've set the > GIC up so that the primary CPU can do an IPI to get > the secondary out of the holding pen loop (that's the > "wfi / ldr /tst / beq" sequence, which loops waiting > for a CPU interrupt). > It seems a bit board-specific and an obstacle to ultimately sanitizing the embedded bootloaders across the architectures (I hope to one day boot MB and ARM with one BL once I get the arch-isms out of the boot flow). I have another problem while we are on the bootstrap discussion - In Zynq we have some Linux specific bootstrap issues out in device land. Our clock driver expects the bootloader to setup some state: https://github.com/Xilinx/meta-xilinx/blob/master/recipes-devtools/qemu/files/HACK_zynq_slcr_Bring_SLCR_out_of_reset_in_kernel_state.patch This seems similar to the Vexpress GIC requirement - peripherals needing linux specific setup. Can we solve both problems here with a bit or framework allowing devs to have an alternate Linux-specific reset state? Then we can ditch on the machine code too :) Regards, Peter >>>>> + { 0, FIXUP_BOOTREG }, >>>>> + { 0, FIXUP_TERMINATOR } >>>>> }; >>> >>> We're also writing to devices here, and it's cleaner to do that >>> by running a guest code instruction than by somehow having >>> the boot code ferret around inside the device's implementation >>> pre-start, I think. >> >> dma_memory_write(&address_space_memory, ... >> >> Its a level above ferreting, and a level below the machine code blob. > > This doesn't work in the reset hook because you can't guarantee > that this reset hook gets run after the device resets itself rather > than before... > > thanks > -- PMM >
On Tue, Dec 17, 2013 at 11:24:45AM +1000, Peter Crosthwaite wrote: > On Tue, Dec 17, 2013 at 10:58 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: > > On 17 December 2013 00:52, Peter Crosthwaite > > <peter.crosthwaite@xilinx.com> wrote: > >> On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > >>> On 13 December 2013 03:19, Peter Crosthwaite > >>> <peter.crosthwaite@xilinx.com> wrote: > >>>> Why do we need blobs at all? Cant we just fix arm/boot to directly > >>>> setup the CPU state to the desired? Rather than complex blobs that > >>>> execute ARM instructions just manipulate the regs directly. > >>> > >>> We could in theory do this for the primary bootloader, but > >>> the secondary CPU blob has to have a loop in it so we > >>> can sit around waiting for the guest code running in the > >>> primary to tell us it's time to go: > >>> > >>>>> +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 }, > >> > >> > >> Reading up on Christopher's Kernel documentation link: > >> > >> Documentation/arm64/booting.txt > >> Documentation/arm/Booting > >> > >> I can't see any reference to GIC, where are these GICisms coming from? > > > > v7 secondary CPU boot protocol is platform specific, > > though most boards seem to do something vaguely > > vexpress like. > > So Zynq is very different. It just rewrites the vector table and > resets the secondarys from peripherals rst controller. > > > The kernel expects that we've set the > > GIC up so that the primary CPU can do an IPI to get > > the secondary out of the holding pen loop (that's the > > "wfi / ldr /tst / beq" sequence, which loops waiting > > for a CPU interrupt). > > > > It seems a bit board-specific and an obstacle to ultimately sanitizing > the embedded bootloaders across the architectures (I hope to one day > boot MB and ARM with one BL once I get the arch-isms out of the boot > flow). ARM is already making a good effort at this with PSCI. However, supporting this in TCG requires some secure state / hyp mode emulation that we don't have currently, afaik. > > I have another problem while we are on the bootstrap discussion - In > Zynq we have some Linux specific bootstrap issues out in device land. > Our clock driver expects the bootloader to setup some state: > > https://github.com/Xilinx/meta-xilinx/blob/master/recipes-devtools/qemu/files/HACK_zynq_slcr_Bring_SLCR_out_of_reset_in_kernel_state.patch > > This seems similar to the Vexpress GIC requirement - peripherals > needing linux specific setup. Can we solve both problems here with a > bit or framework allowing devs to have an alternate Linux-specific > reset state? > > Then we can ditch on the machine code too :) > > Regards, > Peter > > >>>>> + { 0, FIXUP_BOOTREG }, > >>>>> + { 0, FIXUP_TERMINATOR } > >>>>> }; > >>> > >>> We're also writing to devices here, and it's cleaner to do that > >>> by running a guest code instruction than by somehow having > >>> the boot code ferret around inside the device's implementation > >>> pre-start, I think. > >> > >> dma_memory_write(&address_space_memory, ... > >> > >> Its a level above ferreting, and a level below the machine code blob. > > > > This doesn't work in the reset hook because you can't guarantee > > that this reset hook gets run after the device resets itself rather > > than before... > > > > thanks > > -- PMM > > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
On 17 December 2013 01:24, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Dec 17, 2013 at 10:58 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 17 December 2013 00:52, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 13 December 2013 03:19, Peter Crosthwaite >>>> <peter.crosthwaite@xilinx.com> wrote: >>>>> Why do we need blobs at all? Cant we just fix arm/boot to directly >>>>> setup the CPU state to the desired? Rather than complex blobs that >>>>> execute ARM instructions just manipulate the regs directly. >>>> >>>> We could in theory do this for the primary bootloader, but >>>> the secondary CPU blob has to have a loop in it so we >>>> can sit around waiting for the guest code running in the >>>> primary to tell us it's time to go: >>>> >>>>>> +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 }, >>> >>> >>> Reading up on Christopher's Kernel documentation link: >>> >>> Documentation/arm64/booting.txt >>> Documentation/arm/Booting >>> >>> I can't see any reference to GIC, where are these GICisms coming from? >> >> v7 secondary CPU boot protocol is platform specific, >> though most boards seem to do something vaguely >> vexpress like. > > So Zynq is very different. It just rewrites the vector table and > resets the secondarys from peripherals rst controller. Yeah. This is why boot.c supports letting the board provide its own secondary boot code string (used by exynos and highbank). > The kernel expects that we've set the >> GIC up so that the primary CPU can do an IPI to get >> the secondary out of the holding pen loop (that's the >> "wfi / ldr /tst / beq" sequence, which loops waiting >> for a CPU interrupt). >> > > It seems a bit board-specific and an obstacle to ultimately sanitizing > the embedded bootloaders across the architectures (I hope to one day > boot MB and ARM with one BL once I get the arch-isms out of the boot > flow). > > I have another problem while we are on the bootstrap discussion - In > Zynq we have some Linux specific bootstrap issues out in device land. > Our clock driver expects the bootloader to setup some state: > > https://github.com/Xilinx/meta-xilinx/blob/master/recipes-devtools/qemu/files/HACK_zynq_slcr_Bring_SLCR_out_of_reset_in_kernel_state.patch > > This seems similar to the Vexpress GIC requirement - peripherals > needing linux specific setup. Can we solve both problems here with a > bit or framework allowing devs to have an alternate Linux-specific > reset state? > > Then we can ditch on the machine code too :) I'm not convinced about this at all -- this would be letting the "I know about booting Linux" code spread out from boot.c where it currently is into every device. That sounds like a bad idea. -- PMM
On Tue, Dec 17, 2013 at 8:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 December 2013 01:24, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Dec 17, 2013 at 10:58 AM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 17 December 2013 00:52, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> On Fri, Dec 13, 2013 at 8:05 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>> On 13 December 2013 03:19, Peter Crosthwaite >>>>> <peter.crosthwaite@xilinx.com> wrote: >>>>>> Why do we need blobs at all? Cant we just fix arm/boot to directly >>>>>> setup the CPU state to the desired? Rather than complex blobs that >>>>>> execute ARM instructions just manipulate the regs directly. >>>>> >>>>> We could in theory do this for the primary bootloader, but >>>>> the secondary CPU blob has to have a loop in it so we >>>>> can sit around waiting for the guest code running in the >>>>> primary to tell us it's time to go: >>>>> >>>>>>> +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 }, >>>> >>>> >>>> Reading up on Christopher's Kernel documentation link: >>>> >>>> Documentation/arm64/booting.txt >>>> Documentation/arm/Booting >>>> >>>> I can't see any reference to GIC, where are these GICisms coming from? >>> >>> v7 secondary CPU boot protocol is platform specific, >>> though most boards seem to do something vaguely >>> vexpress like. >> >> So Zynq is very different. It just rewrites the vector table and >> resets the secondarys from peripherals rst controller. > > Yeah. This is why boot.c supports letting the board provide its > own secondary boot code string (used by exynos and highbank). > >> The kernel expects that we've set the >>> GIC up so that the primary CPU can do an IPI to get >>> the secondary out of the holding pen loop (that's the >>> "wfi / ldr /tst / beq" sequence, which loops waiting >>> for a CPU interrupt). >>> >> >> It seems a bit board-specific and an obstacle to ultimately sanitizing >> the embedded bootloaders across the architectures (I hope to one day >> boot MB and ARM with one BL once I get the arch-isms out of the boot >> flow). >> >> I have another problem while we are on the bootstrap discussion - In >> Zynq we have some Linux specific bootstrap issues out in device land. >> Our clock driver expects the bootloader to setup some state: >> >> https://github.com/Xilinx/meta-xilinx/blob/master/recipes-devtools/qemu/files/HACK_zynq_slcr_Bring_SLCR_out_of_reset_in_kernel_state.patch >> >> This seems similar to the Vexpress GIC requirement - peripherals >> needing linux specific setup. Can we solve both problems here with a >> bit or framework allowing devs to have an alternate Linux-specific >> reset state? >> >> Then we can ditch on the machine code too :) > > I'm not convinced about this at all -- this would be letting the > "I know about booting Linux" code spread out from boot.c > where it currently is into every device. That sounds like a bad idea. - > So the "I know about boot linux" code already has to span multiple architectures it's already reasonably 'spread out' just looking at the zoo of Linux bootloaders. Its also a contributor to the much disliked inconsistencies in boot flow between archs. If we can outsource arch/board specific bringup to device land it would allow consolidation of all the different Linux bootloaders. Or at least across embedded - E.g. Microblaze, ARM, some of the board level PPC bootloaders O.R. and the newly posted Moxie machine. ARM_CPU / MICROBLAZE_CPU would be responsible for setting dtb reg ptr / kernel args, program entry point. ARM_GIC would set itself up. ZYNQ_SLCR would set itself up. QOM interfaces can make this purely additive and unobtrusive. E.G. interface TYPE_LINUX_RESET implements a reset hook that runs iff the system is booting Linux. Anyways, we are not going to solve this today and at the end of the day, the patch does make this much more self documenting and flexible for ARM, so: Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Regards, Peter > -- PMM >
On 17 December 2013 11:36, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Dec 17, 2013 at 8:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> I'm not convinced about this at all -- this would be letting the >> "I know about booting Linux" code spread out from boot.c >> where it currently is into every device. That sounds like a bad idea. - >> > > So the "I know about boot linux" code already has to span multiple > architectures it's already reasonably 'spread out' just looking at the > zoo of Linux bootloaders. Its also a contributor to the much disliked > inconsistencies in boot flow between archs. > > If we can outsource arch/board specific bringup to device land it > would allow consolidation of all the different Linux bootloaders. Or > at least across embedded - E.g. Microblaze, ARM, some of the board > level PPC bootloaders O.R. and the newly posted Moxie machine. > > ARM_CPU / MICROBLAZE_CPU would be responsible for setting dtb reg ptr > / kernel args, program entry point. > ARM_GIC would set itself up. ...but how? What the GIC state needs to be depends on the board. It's times like this I see the appeal of pushing the whole thing off onto a custom "firmware blob" which does the work :-) > we are not going to solve this today and at the end of the day, the > patch does make this much more self documenting and flexible for ARM, > so: > > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Thanks. -- PMM
On Tue, Dec 17, 2013 at 9:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 December 2013 11:36, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Dec 17, 2013 at 8:31 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> I'm not convinced about this at all -- this would be letting the >>> "I know about booting Linux" code spread out from boot.c >>> where it currently is into every device. That sounds like a bad idea. - >>> >> >> So the "I know about boot linux" code already has to span multiple >> architectures it's already reasonably 'spread out' just looking at the >> zoo of Linux bootloaders. Its also a contributor to the much disliked >> inconsistencies in boot flow between archs. >> >> If we can outsource arch/board specific bringup to device land it >> would allow consolidation of all the different Linux bootloaders. Or >> at least across embedded - E.g. Microblaze, ARM, some of the board >> level PPC bootloaders O.R. and the newly posted Moxie machine. >> >> ARM_CPU / MICROBLAZE_CPU would be responsible for setting dtb reg ptr >> / kernel args, program entry point. >> ARM_GIC would set itself up. > > ...but how? What the GIC state needs to be depends on the board. > In such cases, boards could trivially define a lightweight QOM child class (of the device in question) that implements the linux_reset() hook their specific way: TypeInfo vexpress_gic_info { .parent = TYPE_ARM_GIC, .interfaces = { { TYPE_LINUX_RESET }, {} }, .class_init = vexpress_gic_class_init, } Regards, Peter > It's times like this I see the appeal of pushing the whole thing off onto > a custom "firmware blob" which does the work :-) > >> we are not going to solve this today and at the end of the day, the >> patch does make this much more self documenting and flexible for ARM, >> so: >> >> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > Thanks. > > -- PMM >
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 }, + { 0, FIXUP_TERMINATOR } }; +static void write_bootloader(const char *name, hwaddr addr, + const ARMInsnFixup *insns, uint32_t *fixupcontext) +{ + /* Fix up the specified bootloader fragment and write it into + * guest memory using rom_add_blob_fixed(). fixupcontext is + * an array giving the values to write in for the fixup types + * which write a value into the code array. + */ + int i, len; + uint32_t *code; + + len = 0; + while (insns[len].fixup != FIXUP_TERMINATOR) { + len++; + } + + code = g_new0(uint32_t, len); + + for (i = 0; i < len; i++) { + uint32_t insn = insns[i].insn; + FixupType fixup = insns[i].fixup; + + switch (fixup) { + case FIXUP_NONE: + break; + case FIXUP_BOARDID: + case FIXUP_ARGPTR: + case FIXUP_ENTRYPOINT: + case FIXUP_GIC_CPU_IF: + case FIXUP_BOOTREG: + case FIXUP_DSB: + insn = fixupcontext[fixup]; + break; + default: + abort(); + } + code[i] = tswap32(insn); + } + + rom_add_blob_fixed(name, code, len * sizeof(uint32_t), addr); + + g_free(code); +} + static void default_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info) { - int n; - smpboot[ARRAY_SIZE(smpboot) - 1] = info->smp_bootreg_addr; - smpboot[ARRAY_SIZE(smpboot) - 2] = info->gic_cpu_if_addr; - for (n = 0; n < ARRAY_SIZE(smpboot); n++) { - /* Replace DSB with the pre-v7 DSB if necessary. */ - if (!arm_feature(&cpu->env, ARM_FEATURE_V7) && - smpboot[n] == DSB_INSN) { - smpboot[n] = CP15_DSB_INSN; - } - smpboot[n] = tswap32(smpboot[n]); + uint32_t fixupcontext[FIXUP_MAX]; + + fixupcontext[FIXUP_GIC_CPU_IF] = info->gic_cpu_if_addr; + fixupcontext[FIXUP_BOOTREG] = info->smp_bootreg_addr; + if (arm_feature(&cpu->env, ARM_FEATURE_V7)) { + fixupcontext[FIXUP_DSB] = DSB_INSN; + } else { + fixupcontext[FIXUP_DSB] = CP15_DSB_INSN; } - rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot), - info->smp_loader_start); + + write_bootloader("smpboot", info->smp_loader_start, + smpboot, fixupcontext); } static void default_reset_secondary(ARMCPU *cpu, @@ -354,7 +416,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) CPUState *cs = CPU(cpu); int kernel_size; int initrd_size; - int n; int is_linux = 0; uint64_t elf_entry; hwaddr entry; @@ -420,6 +481,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) } info->entry = entry; if (is_linux) { + uint32_t fixupcontext[FIXUP_MAX]; + if (info->initrd_filename) { initrd_size = load_ramdisk(info->initrd_filename, info->initrd_start, @@ -441,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) } info->initrd_size = initrd_size; - bootloader[4] = info->board_id; + fixupcontext[FIXUP_BOARDID] = info->board_id; /* for device tree boot, we pass the DTB directly in r2. Otherwise * we point to the kernel args. @@ -456,9 +519,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) if (load_dtb(dtb_start, info)) { exit(1); } - bootloader[5] = dtb_start; + fixupcontext[FIXUP_ARGPTR] = dtb_start; } else { - bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; + fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR; if (info->ram_size >= (1ULL << 32)) { fprintf(stderr, "qemu: RAM size must be less than 4GB to boot" " Linux kernel using ATAGS (try passing a device tree" @@ -466,12 +529,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) exit(1); } } - bootloader[6] = entry; - for (n = 0; n < sizeof(bootloader) / 4; n++) { - bootloader[n] = tswap32(bootloader[n]); - } - rom_add_blob_fixed("bootloader", bootloader, sizeof(bootloader), - info->loader_start); + fixupcontext[FIXUP_ENTRYPOINT] = entry; + + write_bootloader("bootloader", info->loader_start, + bootloader, fixupcontext); + if (info->nb_cpus > 1) { info->write_secondary_boot(cpu, info); }
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@linaro.org> --- hw/arm/boot.c | 152 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 107 insertions(+), 45 deletions(-)