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
