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

[Fabiano Rosas <farosas@linux.ibm.com>]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 5/5/22 05:16, Fabiano Rosas wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>> 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.
>> 
>> I think we can fix this without needing a different command line for BE
>> zImage (apart from x-vof, which is a separate matter).
>> 
>> If you look at translate_kernel_address, it cannot really work when the
>> ELF PhysAddr is != 0. We would always hit this sort of 0x4400000 issue,
>> so if we fix that function like this...
>> 
>> static uint64_t translate_kernel_address(void *opaque, uint64_t addr)
>> {
>>      SpaprMachineState *spapr = opaque;
>> 
>>      return addr ? addr : spapr->kernel_addr;
>> }
>
>
> The qemu elf loader is supposed to handle relocations which should be 
> calling this hook more than once, now I wonder why it is not doing so.

For relocations, it seems to only call translate_fn for s390x.

>> ...then we could always use the ELF PhysAddr if it is different from 0
>> and only use the default load addr if the ELF PhysAddr is 0. If the user
>> gives kernel_addr on the cmdline, we honor that, even if puts the kernel
>> over the firmware (we have code to detect that).
>
>
> ELF address is 0 for LE zImage only, vmlinux BE/LE uses 
> 0xc000000000000000. And we are already chopping all these tops bits off 
> in translate_kernel_address() and I do not really know why _exactly_ it 
> is 0x0fffffff and not let's say 0x7fffffff.

Oh, I am not talking about the ELF entry point. And that is not what
load_elf passes to translate_kernel_address either. It is the ELF
PhysAddr:

$ powerpc64le-buildroot-linux-gnu-readelf -We vmlinux | tail

Program Headers:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  LOAD           0x010000 0xc000000000000000 0x0000000000000000 0x28d84d4 0x2a54ea8 RWE 0x10000

So it is 0x400000 for BE zImage and 0 for vmlinux.

>> 
>>> @@ -2988,6 +2990,12 @@ static void spapr_machine_init(MachineState *machine)
>>>               exit(1);
>>>           }
>>>   
>>> +        if (spapr->kernel_addr != loaded_addr) {
>> 
>> This could be:
>> 
>>          if (spapr->kernel_addr == KERNEL_LOAD_ADDR &&
>> 	    spapr->kernel_addr != loaded_addr) {
>> 
>> So the precedence would be:
>> 
>> 1- ELF PhysAddr, if != 0. After all, that is what it's for. BE zImage
>>     falls here;
>>      
>> 2- KERNEL_LOAD_ADDR. Via translate_kernel_address, LE/BE vmlinux fall
>>     here;
>> 
>> 3- kernel_addr. The user is probably hacking something, just use what
>>     they gave us. QEMU will yell if they load the kernel over the fw.
>
>
> imho too complicated.
>
> What if the user runs QEMU with kernel-addr=0x400000? (0x400000 is 
> KERNEL_LOAD_ADDR noooow but not necessarily forever). Is it 2) or 3)?

Good point. It should always be 3. It is a bad user interface to have a
command line option and arbitrarily ignore it. Be it 0x0 or 0x400000.

> I am basically fixing a bug when we ignore where load_elf() loaded the 
> ELF and just assume it is KERNEL_LOAD_ADDR. Now the code checks if the 
> ELF was loaded where we want it to be.

That bug is already accounted for, that is why we have
translate_kernel_address:

  /* address_offset is hack for kernel images that are
     linked at the wrong physical address.  */
  if (translate_fn) {
      addr = translate_fn(translate_opaque, ph->p_paddr);

So we're actively telling load_elf to load the kernel at the wrong place
when we do:

(ph->p_addr & 0x0fffffff) + spapr->kernel_addr

It just happened to work so far because the vmlinux PhysAddr is 0.

When you set kernel-addr=0 you are simply working around this other bug
because:

(ph->p_addr & 0x0fffffff) + 0 == ph->p_addr

I just want to remove this indirection and use the ELF PhysAddr
directly.

> Everything else can be done but on top of this.

If you want to merge this I could send another patch on top of it later,
no worries. I realise that this a larger change that will need more
testing.

>>> +            warn_report("spapr: kernel_addr changed from 0x%lx to 0x%lx",
>>> +                        spapr->kernel_addr, loaded_addr);
>>> +            spapr->kernel_addr = loaded_addr;
>>> +        }
>>> +
>>>           /* load initrd */
>>>           if (initrd_filename) {
>>>               /* Try to locate the initrd in the gap between the kernel