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

[Peter Maydell]

On 17 December 2013 13:04, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Dec 17, 2013 at 10:15 PM, Peter Maydell
> <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
>
> "placeholders"

Fixed (not going to respin just for that though :-))

>> that are used to implement uniform indexing across different bootloaders.
>>
>> Signed-off-by: Mian M. Hamayun <address@hidden>
>> Signed-off-by: Peter Maydell <address@hidden>
>> Message-id: 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>
>> Reviewed-by: Christoffer Dall <address@hidden>
>> ---
>>  hw/arm/boot.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 0c05a64..90e9534 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -17,8 +17,13 @@
>>  #include "sysemu/device_tree.h"
>>  #include "qemu/config-file.h"
>>
>> +/* Kernel boot protocol is specified in the kernel docs
>> + * Documentation/arm/Booting and Documentation/arm64/booting.txt
>> + * They have different preferred image load offsets from system RAM base.
>> + */
>>  #define KERNEL_ARGS_ADDR 0x100
>>  #define KERNEL_LOAD_ADDR 0x00010000
>
> So out of context, but I have been booting an ARMv7 multi defconfig on
> a few qemu platforms recently (spec. allwinner, highbank and zynq) and
> I found I had to patch this to 0x8000 due to some image alignment
> expectations of head.S. Could we patch this to 0x8000 -  is there this
> same sense of preferred image offset in KERNEL32_LOAD_ADDR?

Hmm. Are you booting non-zImage kernels? The booting doc says

# When booting a raw (non-zImage) kernel the constraints are tighter.
# In this case the kernel must be loaded at an offset into system equal
# to TEXT_OFFSET - PAGE_OFFSET.

so maybe that's what we're not getting right there? I practically
always use a zImage so I wouldn't ever see that.

In any case if you can nail down whether that's the problem and
submit a patch that would be good, though we'll have to test
on a bunch of boards that we haven't broken something else.
(I don't particularly want to change to 0x8000 because it "happens
to work", but the same change with a rationale attached is fine :-))

>> +#define KERNEL64_LOAD_ADDR 0x00080000
>>
>>  typedef enum {
>>      FIXUP_NONE = 0,   /* do nothing */
>> @@ -37,6 +42,20 @@ typedef struct ARMInsnFixup {
>>      FixupType fixup;
>>  } ARMInsnFixup;
>>
>> +static const ARMInsnFixup bootloader_aarch64[] = {
>> +    { 0x580000c0 }, /* ldr x0, arg ; Load the lower 32-bits of DTB */
>> +    { 0xaa1f03e1 }, /* mov x1, xzr */
>> +    { 0xaa1f03e2 }, /* mov x2, xzr */
>> +    { 0xaa1f03e3 }, /* mov x3, xzr */
>> +    { 0x58000084 }, /* ldr x4, entry ; Load the lower 32-bits of kernel 
>> entry */
>> +    { 0xd61f0080 }, /* br x4      ; Jump to the kernel entry point */
>> +    { 0, FIXUP_ARGPTR }, /* arg: .word @DTB Lower 32-bits */
>> +    { 0 }, /* .word @DTB Higher 32-bits */
>> +    { 0, FIXUP_ENTRYPOINT }, /* entry: .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 +415,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) {
>
> Curious, why does this 'if' directly deref env, while the one below
> (for primary_loader selection) uses ARM_FEATURE?

When we're picking the bootloader the CPU isn't started, so
its internal state (including the aarch64 flag, which is a reflection
of PSTATE.nRW) isn't valid. We want to pick a bootloader on
the basis of "if this CPU can do AArch64 then use the AArch64
loader". This code however runs after reset and so we can say
"if you're currently in AArch64 mode then this is how the PC is
set, otherwise like this".

I'm mulling a cleanup at some point that makes the AArch32
also use env->pc, at which point this if() will just evaporate anyway.

thanks
-- PMM