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

[Peter Maydell <peter.maydell@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