Re: [Qemu-devel] [PATCH v3 06/11] target-arm: Parameterize the bootloader selection and setup mechanism

[Peter Maydell]

On 27 September 2013 11:10, Mian M. Hamayun
<address@hidden> wrote:
> From: "Mian M. Hamayun" <address@hidden>
>
> This commit replaces the constant indices used in bootloaders, such as for
> specifying the Board ID and kernel arguments with variable parameters.
> This change is used as mechanism to minimize code changes for different
> bootloaders, for example different bootloaders will be used for different
> architectures (ARMv7 vs. ARMv8).
>
> Similary pointers are introduced to select appropriate bootloaders for boot
> and secondary cpus.

So I think parameterizing this a bit is good, but I don't think this
code is quite the right approach...

> Signed-off-by: Mian M. Hamayun <address@hidden>
> ---
>  hw/arm/boot.c | 81 
> ++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 967397b..4c1170e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -18,10 +18,9 @@
>  #include "qemu/config-file.h"
>
>  #define KERNEL_ARGS_ADDR 0x100
> -#define KERNEL_LOAD_ADDR 0x00010000
>
>  /* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  
> */
> -static uint32_t bootloader[] = {
> +static uint32_t bootloader_arm32[] = {
>    0xe3a00000, /* mov     r0, #0 */
>    0xe59f1004, /* ldr     r1, [pc, #4] */
>    0xe59f2004, /* ldr     r2, [pc, #4] */
> @@ -48,7 +47,7 @@ static uint32_t bootloader[] = {
>  #define DSB_INSN 0xf57ff04f
>  #define CP15_DSB_INSN 0xee070f9a /* mcr cp15, 0, r0, c7, c10, 4 */
>
> -static uint32_t smpboot[] = {
> +static uint32_t smpboot_arm32[] = {
>    0xe59f2028, /* ldr r2, gic_cpu_if */
>    0xe59f0028, /* ldr r0, startaddr */
>    0xe3a01001, /* mov r1, #1 */
> @@ -65,13 +64,60 @@ static uint32_t smpboot[] = {
>    0           /* bootreg: Boot register address is held here */
>  };
>
> +/*
> + * The bootloaders to be used are referenced by the following pointers
> + * An appropriate bootloader is selected depending on the architecture
> + * i.e. ARMv7, ARMv8 (AARCH64 and AARCH32)
> + */
> +static uint32_t *bootloader = NULL;
> +static uint32_t  bootloader_array_size = 0;
> +
> +static uint32_t *smpboot = NULL;
> +static uint32_t  smpboot_array_size = 0;
> +
> +/*
> + * An index gives the location in the bootloader array, where we put the 
> board
> + * ID, kernel arguments and kernel entry addresses. These are different for
> + * ARMv7 and ARMv8 bootloaders defined above.
> + */
> +static uint32_t kernel_boardid_index = 0;
> +static uint32_t kernel_args_index    = 0;
> +static uint32_t kernel_entry_index   = 0;
> +
> +/*
> + * Similarly, the kernel loading address also depends on the architecture,
> + * i.e. its different for ARMv7, ARMv8 (AARCH64 and AARCH32)
> + */
> +static uint32_t kernel_load_addr = 0x0;
> +
> +static void setup_boot_env_32(void)
> +{
> +    bootloader = bootloader_arm32;
> +    bootloader_array_size = ARRAY_SIZE(bootloader_arm32);
> +    smpboot = smpboot_arm32;
> +    smpboot_array_size = ARRAY_SIZE(smpboot_arm32);
> +
> +    kernel_boardid_index = bootloader_array_size - 3;
> +    kernel_args_index    = bootloader_array_size - 2;
> +    kernel_entry_index   = bootloader_array_size - 1;
> +    return;
> +}

...you wind up with a per-loader function which sets a bunch
of globals, so the information relating to that loader is more
scattered than necessary. Also, if we find that we need something
other than boardid/args/entrypoint to be patched in, we need to
update every loader. And we're fixing up the secondary boot
loader separately from the primary cpu loader, just because
it happens to need slightly different things doing to it.

My suggestion would be that we have a struct defining
a loader:

typedef struct ARMInsnList {
    uint32_t insn;
    uint32_t fixup;
};

and then have each loader be an array ARMInsnList[];

fixup is one of a set of constants defining what if any action
to take on that word:
#define FIXUP_NONE 0      /* do nothing */
#define FIXUP_TERMINATOR 1 /* end of list */
#define FIXUP_BOARDID 2   /* replace with board ID */
#define FIXUP_ARGPTR 3    /* replace with arg pointer */
#define FIXUP_ENTRYPOINT 4  /* replace with entry point */
#define FIXUP_DSB 5           /* if pre-v7, replace with cp15 DSB */
#define FIXUP_GIC_CPUIF 6 /* replace with GIC CPU i/f address */

and so on. The 32 bit loader then looks something like:

static const ARMInsnList bootloader_arm32[] = {
  { 0xe3a00000 } , /* mov     r0, #0 */
  { 0xe59f1004 }, /* ldr     r1, [pc, #4] */
  { 0xe59f2004 }, /* ldr     r2, [pc, #4] */
  { 0xe59ff004 }, /* ldr     pc, [pc, #4] */
  { 0, FIXUP_BOARDID },
  { 0, FIXUP_ARGPTR },
  { 0, FIXUP_ENTRYPOINT },
  { 0, FIXUP_TERMINATOR }
};

(note that since we made FIXUP_NONE 0 we can omit it as
the struct initialization will default to that.)

Then we can have a common bit of code that does the
"create the fixed up instructions based on the list of
insns and fixups"; it can figure out the length from the
presence of the terminator, and it will be able to handle
both the primary and secondary loaders, with scope for
more flexibility if we need it via further fixup types. (It'll
need to allocate an array to fill in since the insns aren't
consecutive in an ARMInsnList but that's not a big deal.)

I'll have a go at writing up a patch for this this evening...

-- PMM