Re: [Qemu-devel] [PATCH for-2.5 v1 2/4] arm: boot: Add board specific setup code API

[Peter Crosthwaite]

On Tue, Oct 27, 2015 at 5:19 AM, Peter Maydell <address@hidden> wrote:

On 25 October 2015 at 23:13, Peter Crosthwaite
<address@hidden> wrote:
> Add an API for boards to inject their own preboot software (or
> firmware) seqeuence.
>
> The software then returns to bootloader via the link register. This
> allows boards to do their own little bits of firmware setup without
> needed to replace the bootloader completely (which is the requirement
> for existing firmware support).
>
> The blob is loaded by a callback if and only if doing a linux boot
> (similar to the existing write_secondary support).
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> Changed since RFC:
> Load blob via firmware.
> Remove un-needed 0-word in bootloader sequence.
> Remove "blob", just use "board setup" consistently
> Remove boolean for (just use a pointer NULL check on write_board_setup)
> Adjust comment about functionality of primary bootloader

Couple of comment tweaks, but otherwise
Reviewed-by: Peter Maydell <address@hidden>

Thanks.

 

>
>  hw/arm/boot.c        | 17 ++++++++++++++++-
>  include/hw/arm/arm.h | 10 ++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 2a151e2..5640989 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -31,6 +31,7 @@ typedef enum {
>      FIXUP_NONE = 0,     /* do nothing */
>      FIXUP_TERMINATOR,   /* end of insns */
>      FIXUP_BOARDID,      /* overwrite with board ID number */
> +    FIXUP_BOARD_SETUP,  /* overwrite with board specific setup code address */
>      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 */
> @@ -58,8 +59,14 @@ static const ARMInsnFixup bootloader_aarch64[] = {
>      { 0, FIXUP_TERMINATOR }
>  };
>
> -/* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  */
> +/* The worlds second smallest bootloader.  Call the board-setup code, Set r0-r2,
> + * then jump to kernel.
> + */

While we're editing this comment we might as well fix the grammar
error and remove the claim about size (since we're gradually increasing
it); a note about the fact the first part is not always used is
probably helpful too:

/* A very small bootloader: call the board-setup code (if needed),
 * set r0-r2, then jump to the kernel.
 * If we're not calling boot setup code then we don't copy across
 * the first BOOTLOADER_NO_BOARD_SETUP_OFFSET insns in this array.

 */

Will take verbatim.

 

>  static const ARMInsnFixup bootloader[] = {
> +    { 0xe28fe008 }, /* add     lr, pc, #8 */
> +    { 0xe51ff004 }, /* ldr     pc, [pc, #-4] */
> +    { 0, FIXUP_BOARD_SETUP },
> +#define BOOTLOADER_NO_BOARD_SETUP_OFFSET 3
>      { 0xe3a00000 }, /* mov     r0, #0 */
>      { 0xe59f1004 }, /* ldr     r1, [pc, #4] */
>      { 0xe59f2004 }, /* ldr     r2, [pc, #4] */
> @@ -131,6 +138,7 @@ static void write_bootloader(const char *name, hwaddr addr,
>          case FIXUP_NONE:
>              break;
>          case FIXUP_BOARDID:
> +        case FIXUP_BOARD_SETUP:
>          case FIXUP_ARGPTR:
>          case FIXUP_ENTRYPOINT:
>          case FIXUP_GIC_CPU_IF:
> @@ -640,6 +648,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>          elf_machine = EM_AARCH64;
>      } else {
>          primary_loader = bootloader;
> +        if (!info->write_board_setup) {
> +            primary_loader += BOOTLOADER_NO_BOARD_SETUP_OFFSET;
> +        }
>          kernel_load_offset = KERNEL_LOAD_ADDR;
>          elf_machine = EM_ARM;
>      }
> @@ -745,6 +756,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>          info->initrd_size = initrd_size;
>
>          fixupcontext[FIXUP_BOARDID] = info->board_id;
> +        fixupcontext[FIXUP_BOARD_SETUP] = info->board_setup_addr;
>
>          /* for device tree boot, we pass the DTB directly in r2. Otherwise
>           * we point to the kernel args.
> @@ -793,6 +805,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>          if (info->nb_cpus > 1) {
>              info->write_secondary_boot(cpu, info);
>          }
> +        if (info->write_board_setup) {
> +            info->write_board_setup(cpu, info);
> +        }
>
>          /* Notify devices which need to fake up firmware initialization
>           * that we're doing a direct kernel boot.
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 4dcd4f9..128a54e 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -87,6 +87,16 @@ struct arm_boot_info {
>       * -pflash. It also implies that fw_cfg_find() will succeed.
>       */
>      bool firmware_loaded;
> +
> +    /* Address at which board specific loader/setup code exists. If enabled,
> +     * this code-blob will run before anything else. It must return to the
> +     * caller via the link register. There is no stack setup. Enabled by

"set up".

Will fix.

Regards,

Peter

 

> +     * defining write_board_setup, which is responsible for loading the blob
> +     * to the specified address.
> +     */
> +    hwaddr board_setup_addr;
> +    void (*write_board_setup)(ARMCPU *cpu,
> +                              const struct arm_boot_info *info);
>  };
>
>  /**
> --
> 1.9.1

thanks
-- PMM