On Tue, Dec 17, 2013 at 11:14 PM, Peter Maydell
<peter.mayd...@linaro.org> wrote:
> On 17 December 2013 13:04, Peter Crosthwaite
> <peter.crosthwa...@xilinx.com> wrote:
>> On Tue, Dec 17, 2013 at 10:15 PM, Peter Maydell
>> <peter.mayd...@linaro.org> wrote:
>>> From: "Mian M. Hamayun" <m.hama...@virtualopensystems.com>
>>>
>>> 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 <m.hama...@virtualopensystems.com>
>>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
>>> Message-id: 1385645602-18662-7-git-send-email-peter.mayd...@linaro.org
>>> [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.mayd...@linaro.org>
>>> Reviewed-by: Christoffer Dall <christoffer.d...@linaro.org>
>>> ---
>>> 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
>
Yep.
> # 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.
>
Yes it'll probably that, although my trace through head.S had
significant variation with different Kconfigs and normal Image has
booted in the past. Would have to get back with specifics.
> 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".
>
But isn't the selection of which primary bootloader pre-decided by
arm_feature? This would lead to an error if (somehow) the bootloader
blobbed in the 64 bit version and the CPU reset into 32 (via this
aarch64 flag). Perhaps this check should also be arm_feature, and then
the code can addtionally assert correctness of this ->aarch64 CPU
state rather than trusting in coherency between the two stages.
Regards,
Peter
> 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
>64
