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

[Christoffer Dall <christoffer.dall@linaro.org>]

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