[6/7] hw/arm/boot: Add boot support for AArch64 processor

From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>

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 <m.hamayun@virtualopensystems.com>
[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 <peter.maydell@linaro.org>
---
 hw/arm/boot.c |   40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

On Thu, Nov 28, 2013 at 01:33:21PM +0000, Peter Maydell wrote:
> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
> 
> 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 <m.hamayun@virtualopensystems.com>
> [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 <peter.maydell@linaro.org>
> ---
>  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
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Otherwise looks good to me:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

On 16 December 2013 23:40, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Nov 28, 2013 at 01:33:21PM +0000, Peter Maydell wrote:
>> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
>>
>> 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 <m.hamayun@virtualopensystems.com>
>> [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 <peter.maydell@linaro.org>
>> ---
>>  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...

Yeah, we could put the references to the document names in.
I don't see the point repeating the 0x80000 figure in the comment though.

>>
>>  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"...?

referring down to a label or something would be better. This is probably
just a copy of output from a disassembler of a binary blob with assumed
offset zero.

thanks
-- PMM

On Tue, Dec 17, 2013 at 12:25:43AM +0000, Peter Maydell wrote:
> On 16 December 2013 23:40, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > On Thu, Nov 28, 2013 at 01:33:21PM +0000, Peter Maydell wrote:
> >> From: "Mian M. Hamayun" <m.hamayun@virtualopensystems.com>
> >>
> >> 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 <m.hamayun@virtualopensystems.com>
> >> [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 <peter.maydell@linaro.org>
> >> ---
> >>  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...
> 
> Yeah, we could put the references to the document names in.
> I don't see the point repeating the 0x80000 figure in the comment though.
> 

I didn't mean it that literal, just that now it's sort of weird that
there's no comment for 32-bit, but something for the aarch64, which sort
of suggests that any braindead monkey of course knows the 32-bit
details:)

But it's also something that can be fixed later if it creates a
rebasing headache for you at this point.

> >>
> >>  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"...?
> 
> referring down to a label or something would be better. This is probably
> just a copy of output from a disassembler of a binary blob with assumed
> offset zero.
> 
Yeah, I actually dumped your binary values and did a raw disassembly to
check that you weren't lying in the comments, and my disassembler gave
me the hex versions so I sort of figured, but that could be made easier
for the readers who don't want to do that or possess an intimate
knowledge of the aarch64 opcodes in their head.

-Christoffer