Re: [Qemu-devel] [PATCH v11 4/6] arm: add secondary cpu book callbacks to arm_boot.c

[Peter Maydell]

On 19 January 2012 15:43, Mark Langsdorf <address@hidden> wrote:
> Re: [PATCH v11 4/6] arm: add secondary cpu book callbacks to arm_boot.c

Subject should say 'boot', not 'book'...

> Create two functions, write_secondary_boot() and secondary_cpu_reset_hook(),
> to allow platforms more control of how secondary CPUs are brought up. The
> new functions default to NULL and aren't called unless they are populated
> so there are no changes to existing platform models.
>
> Signed-off-by: Mark Langsdorf <address@hidden>
> ---
> Changes from v2-v10
>        Skipped
> Changes from v1
>        Added default versions of the functions
>        Created checks so that NULL function entries became default versions
>        Simplified calls to the secondary boot functions
>        Added comments on the use of these functions
>
>  hw/arm-misc.h |   15 +++++++++++++++
>  hw/arm_boot.c |   38 +++++++++++++++++++++++++++++---------
>  2 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index 6e8ae6b..9138eab 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -30,12 +30,27 @@ struct arm_boot_info {
>     const char *kernel_cmdline;
>     const char *initrd_filename;
>     target_phys_addr_t loader_start;
> +    /* multicore boards that use the default secondary core boot functions
> +     * need to put the address of the secondary boot code, the boot reg,
> +     * and the GIC address in the next 3 values, respectively. boards that
> +     * have their own boot functions can use these values as they want.
> +     */
>     target_phys_addr_t smp_loader_start;
>     target_phys_addr_t smp_bootreg_addr;
>     target_phys_addr_t smp_priv_base;
>     int nb_cpus;
>     int board_id;
>     int (*atag_board)(const struct arm_boot_info *info, void *p);
> +    /* multicore boards that use the default secondary core boot functions
> +     * can ignore these two function calls. If the default functions won't
> +     * work, then write_secondary_boot() should mimic the board's boot
> +     * loader code, and secondary_cpu_reset_hook() should operate handle any
> +     * polling response and reset functions.
> +     */

"then write_secondary_boot() should write a suitable blob of code mimicing
the secondary CPU startup process used by the board's boot loader/boot ROM
code, and secondary_cpu_reset_hook() should perform any necessary CPU reset
handling and set the PC for the secondary CPUs to point at this boot blob."

> +static void default_write_secondary(CPUState *env,
> +                                    const struct arm_boot_info *info)
> +{
> +    int n;
> +    smpboot[ARRAY_SIZE(smpboot) - 1] = info->smp_bootreg_addr;
> +    smpboot[ARRAY_SIZE(smpboot) - 2] = info->smp_priv_base;
> +    for (n = 0; n < ARRAY_SIZE(smpboot); n++) {
> +        smpboot[n] = tswap32(smpboot[n]);
> +    }
> +    rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
> +                       info->smp_loader_start);
> +}
> +
> +static void default_reset_secondary(CPUState *env,
> +                                    const struct arm_boot_info *info)
> +{
> +    stl_phys_notdirty(info->smp_bootreg_addr, 0);
> +    env->regs[15] = info->smp_loader_start;
> +}
> +

Can you put these two functions immediately below the smpboot[] array,
and replace the comment just above smpboot[] with this one:

/* Handling for secondary CPU boot in a multicore system.
 * Unlike the uniprocessor/primary CPU boot, this is platform
 * dependent. The default code here is based on the secondary
 * CPU boot protocol used on realview/vexpress boards, with
 * some parameterisation to increase its flexibility.
 * QEMU platform models for which this code is not appropriate
 * should override write_secondary_boot and secondary_cpu_reset_hook
 * instead.
 *
 * This code enables the interrupt controllers for the secondary
 * CPUs and then puts all the secondary CPUs into a loop waiting
 * for an interprocessor interrupt and polling a configurable
 * location for the kernel secondary CPU entry point.
 */

Otherwise looks good.

-- PMM