Re: [Qemu-devel] [PATCH v3 07/11] AARCH64: Add boot support for aarch64 processor

[Peter Maydell]

On 27 September 2013 11:10, Mian M. Hamayun
<address@hidden> wrote:
> From: "Mian M. Hamayun" <address@hidden>
>
> This commit adds support for booting a single AArch64 CPU by setting
> appropriate registers. The bootloader includes placehoders for Board-ID
> that are used to implement uniform indexing across different bootloaders.
> We also introduce Cortex-A57 to virt platform.

Hi; apologies for not getting round to a closer look at this patch earlier.
The changes are mostly fairly minor so my current plan is to put a version
of this patch in the set I'm hoping to send out later this week...

> Signed-off-by: Mian M. Hamayun <address@hidden>
> ---
>  hw/arm/boot.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/virt.c |  8 ++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 4c1170e..0471eb8 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -19,6 +19,23 @@
>
>  #define KERNEL_ARGS_ADDR 0x100
>
> +#ifdef TARGET_AARCH64
> +static uint32_t bootloader_arm64[] = {
> +    0x580000c0,        /* ldr  x0, 18 ; Load the lower 32-bits of DTB */
> +    0xaa1f03e1,        /* mov  x1, xzr */
> +    0xaa1f03e2,        /* mov  x2, xzr */
> +    0xaa1f03e3,        /* mov  x3, xzr */
> +    0x58000084,        /* ldr  x4, 20 ; Load the lower 32-bits of kernel 
> entry */
> +    0xd61f0080,        /* br   x4     ; Jump to the kernel entry point */
> +    0x00000000,        /* .word @DTB Lower 32-bits */
> +    0x00000000,        /* .word @DTB Higher 32-bits */
> +    0x00000000,        /* .word @Kernel Entry Lower 32-bits */
> +    0x00000000,        /* .word @Kernel Entry Higher 32-bits */
> +    0x00000000,        /* .word @Board ID Lower 32-bits -- Placeholder */
> +    0x00000000         /* .word @Board ID Higher 32-bits -- Placeholder */
> +};
> +#endif
> +
>  /* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  
> */
>  static uint32_t bootloader_arm32[] = {
>    0xe3a00000, /* mov     r0, #0 */
> @@ -103,11 +120,37 @@ static void setup_boot_env_32(void)
>      return;
>  }
>
> +#ifdef TARGET_AARCH64
> +static void setup_boot_env_64(void)
> +{
> +    bootloader = bootloader_arm64;
> +    bootloader_array_size = ARRAY_SIZE(bootloader_arm64);
> +
> +    kernel_args_index    = bootloader_array_size - 6;
> +    kernel_entry_index   = bootloader_array_size - 4;
> +    kernel_boardid_index = bootloader_array_size - 2;
> +    return;
> +}
> +#endif
> +
>  static void setup_boot_env(ARMCPU *cpu)
>  {
> +#ifdef TARGET_AARCH64

You don't need this ifdef -- env->aarch64 exists on 32 bit builds
and is always zero, so just doing a runtime check should be fine.

> +    CPUARMState *env = &cpu->env;
> +    if(env->aarch64) {
> +        /* AARCH64 Mode */
> +        kernel_load_addr = 0x00080000;

This offset should ideally be pulled out of the decompressed image header.

> +        setup_boot_env_64();
> +    }
> +    else {
> +        /* AARCH32 Mode */
> +        /* TODO: Specify Kernel Load Address for AARCH32 */
> +    }
> +#else
>      /* ARMv7 */
>      kernel_load_addr = 0x00010000;
>      setup_boot_env_32();
> +#endif
>      return;
>  }
>
> @@ -380,8 +423,22 @@ static void do_cpu_reset(void *opaque)
>              env->regs[15] = info->entry & 0xfffffffe;
>              env->thumb = info->entry & 1;
>          } else {
> +#ifdef TARGET_AARCH64
> +            if(env->aarch64) {
> +                env->pstate = PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT 
> | PSR_MODE_EL1h;
> +            } else {
> +                hw_error("AArch32 mode is currently not supported\n");

TARGET_AARCH64 doesn't necessarily mean this is a 64
bit CPU; this ifdef breaks 32 bit CPUs in an aarch64-softmmu binary.

> +            }
> +            env->xregs[0] =  0;
> +            env->xregs[1] = -1;
> +#endif
>              if (CPU(cpu) == first_cpu) {
> +#ifdef TARGET_AARCH64
> +                env->xregs[2] = bootloader[kernel_args_index];

Why are we setting X[2] here? The bootloader code is going
to immediately clear it... Similarly X[0] and X[1] above.

> +                env->pc = info->loader_start;
> +#else
>                  env->regs[15] = info->loader_start;

This ifdef should be an "if (aarch64)" runtime check too.

> +#endif
>                  if (!info->dtb_filename) {
>                      if (old_param) {
>                          set_kernel_args_old(info);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 448a0e5..8043fd4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -118,6 +118,14 @@ static VirtBoardInfo machines[] = {
>          .memmap = a15memmap,
>          .irqmap = a15irqmap,
>      },
> +    {
> +        .cpu_model = "cortex-a57",
> +        .cpu_compatible = "arm,arm-v8",
> +        .qdevname = "a57mpcore_priv",
> +        .gic_compatible = "arm,cortex-a15-gic",
> +        .memmap = a15memmap,
> +        .irqmap = a15irqmap,
> +    },
>  };

This hunk shouldn't really be in this patch I think.

thanks
-- PMM