Re: [Qemu-devel] [PATCH 6/7] hw/arm/boot: Add boot support for AArch64 processor

[Christoffer Dall]

On Thu, Nov 28, 2013 at 01:33:21PM +0000, Peter Maydell 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.
> 
> Signed-off-by: Mian M. Hamayun <address@hidden>
> [PMM:
>  * updated to use ARMInsnFixup style bootloader fragments
>  * dropped virt.c additions
>  * use runtime checks for "is this an AArch64 core" rather than ifdefs
>  * drop some unnecessary setting of registers in reset hook
> ]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/arm/boot.c |   40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 77d29a8..b6b04b7 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -19,6 +19,8 @@
>  
>  #define KERNEL_ARGS_ADDR 0x100
>  #define KERNEL_LOAD_ADDR 0x00010000
> +/* The AArch64 kernel boot protocol specifies a different preferred address 
> */
> +#define KERNEL64_LOAD_ADDR 0x00080000

I assume you referring to Documentation/arm/Booting and
Documentation/arm64/booting.txt here?  Perhaps it would be nicer to
refer to that and state how we reach the address for the two archs
instead of having the aarch64 specific note here, e.g. "The kernel
recommends booting at offset 0x80000 from system RAM" or something like
that...

>  
>  typedef enum {
>      FIXUP_NONE = 0,   /* do nothing */
> @@ -37,6 +39,20 @@ typedef struct ARMInsnFixup {
>      FixupType fixup;
>  } ARMInsnFixup;
>  
> +static const ARMInsnFixup bootloader_aarch64[] = {
> +    { 0x580000c0 }, /* ldr x0, 18 ; Load the lower 32-bits of DTB */

so by 18 you mean the label 0x18 assuming the instruction above has the
label 0 or something like that?  Is this an accepted/familiar notation
or shoudl we do something like the arm32 bootloaders and "define a
label in the comments"...?

> +    { 0xaa1f03e1 }, /* mov x1, xzr */
> +    { 0xaa1f03e2 }, /* mov x2, xzr */
> +    { 0xaa1f03e3 }, /* mov x3, xzr */
> +    { 0x58000084 }, /* ldr x4, 20 ; Load the lower 32-bits of kernel entry */

same as above

> +    { 0xd61f0080 }, /* br x4      ; Jump to the kernel entry point */
> +    { 0, FIXUP_ARGPTR }, /* .word @DTB Lower 32-bits */
> +    { 0 }, /* .word @DTB Higher 32-bits */
> +    { 0, FIXUP_ENTRYPOINT }, /* .word @Kernel Entry Lower 32-bits */
> +    { 0 }, /* .word @Kernel Entry Higher 32-bits */
> +    { 0, FIXUP_TERMINATOR }
> +};
> +
>  /* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  
> */
>  static const ARMInsnFixup bootloader[] = {
>      { 0xe3a00000 }, /* mov     r0, #0 */
> @@ -396,7 +412,12 @@ static void do_cpu_reset(void *opaque)
>              env->thumb = info->entry & 1;
>          } else {
>              if (CPU(cpu) == first_cpu) {
> -                env->regs[15] = info->loader_start;
> +                if (env->aarch64) {
> +                    env->pc = info->loader_start;
> +                } else {
> +                    env->regs[15] = info->loader_start;
> +                }
> +
>                  if (!info->dtb_filename) {
>                      if (old_param) {
>                          set_kernel_args_old(info);
> @@ -418,8 +439,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>      int initrd_size;
>      int is_linux = 0;
>      uint64_t elf_entry;
> -    hwaddr entry;
> +    hwaddr entry, kernel_load_offset;
>      int big_endian;
> +    static const ARMInsnFixup *primary_loader;
>  
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
> @@ -429,6 +451,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>          return;
>      }
>  
> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +        primary_loader = bootloader_aarch64;
> +        kernel_load_offset = KERNEL64_LOAD_ADDR;
> +    } else {
> +        primary_loader = bootloader;
> +        kernel_load_offset = KERNEL_LOAD_ADDR;
> +    }
> +
>      info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
>  
>      if (!info->secondary_cpu_reset_hook) {
> @@ -469,9 +499,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>                                    &is_linux);
>      }
>      if (kernel_size < 0) {
> -        entry = info->loader_start + KERNEL_LOAD_ADDR;
> +        entry = info->loader_start + kernel_load_offset;
>          kernel_size = load_image_targphys(info->kernel_filename, entry,
> -                                          info->ram_size - KERNEL_LOAD_ADDR);
> +                                          info->ram_size - 
> kernel_load_offset);
>          is_linux = 1;
>      }
>      if (kernel_size < 0) {
> @@ -532,7 +562,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>          fixupcontext[FIXUP_ENTRYPOINT] = entry;
>  
>          write_bootloader("bootloader", info->loader_start,
> -                         bootloader, fixupcontext);
> +                         primary_loader, fixupcontext);
>  
>          if (info->nb_cpus > 1) {
>              info->write_secondary_boot(cpu, info);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> kvmarm mailing list
> address@hidden
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Otherwise looks good to me:

Reviewed-by: Christoffer Dall <address@hidden>