On 23 December 2011 11:40, Evgeny Voevodin <e.voevo...@samsung.com> wrote: > Secondary CPU bootloader enables interrupt and issues wfi until start address > is written to system controller. The position where to find this start > address is hardcoded to 0x10000030. This commit extends bootloader for > secondary CPU to allow a target board to cpecify a position where > to find start address. If target board doesn't specify start address then > default 0x10000030 is used.
I think this commit message could be slightly clearer: I suggest: hw/arm_boot.c: Make SMP boards specify address to poll in bootup loop The secondary CPU bootloader in arm_boot.c holds secondary CPUs in a pen until the primary CPU releases them. Make boards specify the address to be polled to determine whether to leave the pen (it was previously hardcoded to 0x10000030, which is a Versatile Express/ Realview specific system register address). [see below for why I say "Make" rather than "Allow".] > @@ -179,6 +179,8 @@ static void do_cpu_reset(void *opaque) > { > CPUState *env = opaque; > const struct arm_boot_info *info = env->boot_info; > + uint32_t smp_bootreg_addr = 0; > + target_phys_addr_t p = info->smp_bootreg_addr; > > cpu_reset(env); > if (info) { This is wrong because we dereference info before we check whether it is NULL. I apologise for not spotting earlier that WRITE_WORD() modifies its first argument, or I wouldn't have suggested it. Let's just do stl_phys_notdirty(info->smp_bootreg_addr, 0); in the else clause below; then we can drop both the local vars here. > @@ -197,6 +199,7 @@ static void do_cpu_reset(void *opaque) > info->loader_start); > } > } else { > + WRITE_WORD(p, smp_bootreg_addr); > env->regs[15] = info->smp_loader_start; > } > } > @@ -272,8 +275,12 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info > *info) > rom_add_blob_fixed("bootloader", bootloader, sizeof(bootloader), > info->loader_start); > if (info->nb_cpus > 1) { > - smpboot[10] = info->smp_priv_base; > - for (n = 0; n < sizeof(smpboot) / 4; n++) { > + if (!info->smp_bootreg_addr) { > + info->smp_bootreg_addr = 0x10000030; > + } I think we should just mandate that boards which set info->nb_cpus to something > 1 must also set info->smp_bootreg_addr. 0x10000030 is realview/vexpress/etc specific (it's the address of one of the flag registers in the sysregs). There are only two boards which this affects: hw/realview.c hw/vexpress.c (all the other ARM boards are single core). So it's not a huge effort to update those two boards and it means we don't need this bit of backcompat code. Otherwise OK. -- PMM