Re: [Qemu-devel] [PATCH v5 06/11] hw/arm_boot.c: Extend secondary CPU bootloader.

[Peter Maydell]

On 23 December 2011 11:40, Evgeny Voevodin <address@hidden> 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