|
From: | Daniel Henrique Barboza |
Subject: | Re: [PATCH qemu] spapr: Use address from elf parser for kernel address |
Date: | Wed, 18 May 2022 07:07:00 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 |
On 5/17/22 23:51, Alexey Kardashevskiy wrote:
On 5/18/22 04:58, Daniel Henrique Barboza wrote:Alexey, I had to amend your commit due to Gitlab CI complaining about ... On 5/4/22 03:55, Alexey Kardashevskiy wrote:tl;dr: This allows Big Endian zImage booting via -kernel + x-vof=on. QEMU loads the kernel at 0x400000 by default which works most of the time as Linux kernels are relocatable, 64bit and compiled with "-pie" (position independent code). This works for a little endian zImage too. However a big endian zImage is compiled without -pie, is 32bit, linked to 0x4000000 so current QEMU ends up loading it at 0x4400000 but keeps spapr->kernel_addr unchanged so booting fails. This uses the kernel address returned from load_elf(). If the default kernel_addr is used, there is no change in behavior (as translate_kernel_address() takes care of this), which is: LE/BE vmlinux and LE zImage boot, BE zImage does not. If the VM created with "-machine kernel-addr=0,x-vof=on", then QEMU prints a warning and BE zImage boots. Note #1: SLOF (x-vof=off) still cannot boot a big endian zImage as SLOF enables MSR_SF for everything loaded by QEMU and this leads to early crash of 32bit zImage. Note #2: BE/LE vmlinux images set MSR_SF in early boot so these just work; a LE zImage restores MSR_SF after every CI call and we are lucky enough not to crash before the first CI call. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- We could probably change SLOF to always clear MSR_SF before jumping to the kernel but this is 1) SLOF fix 2) not quite sure if it brings lots of value. I really wish I had this when tested this fix: https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220406070038.3704604-1-aik@ozlabs.ru/ --- hw/ppc/spapr.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a4372ba1891e..89f18f6564bd 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2971,14 +2971,16 @@ static void spapr_machine_init(MachineState *machine) } if (kernel_filename) { + uint64_t loaded_addr = 0; + spapr->kernel_size = load_elf(kernel_filename, NULL, translate_kernel_address, spapr, - NULL, NULL, NULL, NULL, 1, + NULL, &loaded_addr, NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0); if (spapr->kernel_size == ELF_LOAD_WRONG_ENDIAN) { spapr->kernel_size = load_elf(kernel_filename, NULL, translate_kernel_address, spapr, - NULL, NULL, NULL, NULL, 0, + NULL, &loaded_addr, NULL, NULL, 0, PPC_ELF_MACHINE, 0, 0); spapr->kernel_le = spapr->kernel_size > 0; } @@ -2988,6 +2990,12 @@ static void spapr_machine_init(MachineState *machine) exit(1); } + if (spapr->kernel_addr != loaded_addr) { + warn_report("spapr: kernel_addr changed from 0x%lx to 0x%lx", + spapr->kernel_addr, loaded_addr);... this code. This is problematic when compiling in a 32 bit environment because the definition of long (long) unsigned differs from the usual 64 bit env we use: ../hw/ppc/spapr.c: In function 'spapr_machine_init': ../hw/ppc/spapr.c:2998:25: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uint64_t' {aka 'long long unsigned int'} [-Werror=format=] 2998 | warn_report("spapr: kernel_addr changed from 0x%lx to 0x%lx", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2999 | spapr->kernel_addr, loaded_addr); | ~~~~~~~~~~~~~~~~~~ | | | uint64_t {aka long long unsigned int} ../hw/ppc/spapr.c:2998:25: error: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned int'} [-Werror=format=] 2998 | warn_report("spapr: kernel_addr changed from 0x%lx to 0x%lx", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2999 | spapr->kernel_addr, loaded_addr); | ~~~~~~~~~~~ | | | uint64_t {aka long long unsigned int} cc1: all warnings being treated as errors I've fixed it by doing the following: diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 156e799ae9..8d5bdfc20f 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2995,7 +2995,7 @@ static void spapr_machine_init(MachineState *machine) } if (spapr->kernel_addr != loaded_addr) { - warn_report("spapr: kernel_addr changed from 0x%lx to 0x%lx", + warn_report("spapr: kernel_addr changed from 0x%"PRIx64 + " to 0x%"PRIx64, spapr->kernel_addr, loaded_addr); spapr->kernel_addr = loaded_addr; } If you're ok with this fixup we can keep it as is. Otherwise feel free to send another version.I am totally fine with this change, sorry I have not compile tested it, just assumed this cannot fail :-/
Nah it's fine. You would only be able to see this error if you happen to compile it with a 32 bit environment. Not sure if this is something you use to do here and there. I sure don't. Daniel
Thanks, Daniel+ spapr->kernel_addr = loaded_addr; + } + /* load initrd */ if (initrd_filename) { /* Try to locate the initrd in the gap between the kernel
[Prev in Thread] | Current Thread | [Next in Thread] |