Re: [PATCH qemu] spapr: Use address from elf parser for kernel address

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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
>