[Qemu-devel] [FOR 0.12][PATCH] Fix loading of ELF multiboot kernels

[Qemu-devel] [FOR 0.12][PATCH] Fix loading of ELF multiboot kernels

[Kevin Wolf]

The multiboot implementation assumed that there is only one program header
(which contains the entry point) and that the entry point is at the start of
the code. This doesn't hold true generally and caused too little data to be
loaded.

Fix the loading code to pass the whole loaded data to the Multiboot Option ROM.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/loader.c |    2 --
 hw/pc.c     |   10 ++++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 2d7a2c4..4c6981f 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -718,8 +718,6 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size)
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->max)
             continue;
-        if (rom->min > addr)
-            continue;
         if (rom->min + rom->romsize < addr)
             continue;
         if (rom->min > end)
diff --git a/hw/pc.c b/hw/pc.c
index 8c1b7ea..fcebe3d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -560,19 +560,21 @@ static int load_multiboot(void *fw_cfg,
     }
     if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
         uint64_t elf_entry;
+        uint64_t elf_low, elf_high;
         int kernel_size;
         fclose(f);
-        kernel_size = load_elf(kernel_filename, 0, &elf_entry, NULL, NULL,
+        kernel_size = load_elf(kernel_filename, 0, &elf_entry, &elf_low, &elf_high,
                                0, ELF_MACHINE, 0);
         if (kernel_size < 0) {
             fprintf(stderr, "Error while loading elf kernel\n");
             exit(1);
         }
-        mh_load_addr = mh_entry_addr = elf_entry;
-        mb_kernel_size = kernel_size;
+        mh_load_addr = elf_low;
+        mb_kernel_size = elf_high - elf_low;
+        mh_entry_addr = elf_entry;
 
         mb_kernel_data = qemu_malloc(mb_kernel_size);
-        if (rom_copy(mb_kernel_data, elf_entry, kernel_size) != kernel_size) {
+        if (rom_copy(mb_kernel_data, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
             fprintf(stderr, "Error while fetching elf kernel from rom\n");
             exit(1);
         }
-- 
1.6.2.5

Re: [Qemu-devel] [FOR 0.12][PATCH] Fix loading of ELF multiboot kernels

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The multiboot implementation assumed that there is only one program header
> (which contains the entry point) and that the entry point is at the start of
> the code. This doesn't hold true generally and caused too little data to be
> loaded.

Out of curiosity: does this affect images people actually use?
Examples?

> Fix the loading code to pass the whole loaded data to the Multiboot Option ROM.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/loader.c |    2 --
>  hw/pc.c     |   10 ++++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 2d7a2c4..4c6981f 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -718,8 +718,6 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size)
>      QTAILQ_FOREACH(rom, &roms, next) {
>          if (rom->max)
>              continue;
> -        if (rom->min > addr)
> -            continue;
>          if (rom->min + rom->romsize < addr)
>              continue;
>          if (rom->min > end)

I don't understand this hunk.

> diff --git a/hw/pc.c b/hw/pc.c
> index 8c1b7ea..fcebe3d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -560,19 +560,21 @@ static int load_multiboot(void *fw_cfg,
>      }
>      if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
>          uint64_t elf_entry;
> +        uint64_t elf_low, elf_high;
>          int kernel_size;
>          fclose(f);
> -        kernel_size = load_elf(kernel_filename, 0, &elf_entry, NULL, NULL,
> +        kernel_size = load_elf(kernel_filename, 0, &elf_entry, &elf_low, &elf_high,
>                                 0, ELF_MACHINE, 0);
>          if (kernel_size < 0) {
>              fprintf(stderr, "Error while loading elf kernel\n");
>              exit(1);
>          }
> -        mh_load_addr = mh_entry_addr = elf_entry;
> -        mb_kernel_size = kernel_size;
> +        mh_load_addr = elf_low;
> +        mb_kernel_size = elf_high - elf_low;
> +        mh_entry_addr = elf_entry;
>  
>          mb_kernel_data = qemu_malloc(mb_kernel_size);
> -        if (rom_copy(mb_kernel_data, elf_entry, kernel_size) != kernel_size) {
> +        if (rom_copy(mb_kernel_data, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
>              fprintf(stderr, "Error while fetching elf kernel from rom\n");
>              exit(1);
>          }

I get this part, and it looks good.

Re: [Qemu-devel] [FOR 0.12][PATCH] Fix loading of ELF multiboot kernels

[Kevin Wolf]

Am 16.12.2009 10:51, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> The multiboot implementation assumed that there is only one program header
>> (which contains the entry point) and that the entry point is at the start of
>> the code. This doesn't hold true generally and caused too little data to be
>> loaded.
> 
> Out of curiosity: does this affect images people actually use?
> Examples?

It hit me, so yes. Probably no widespread kernels as Alex' tests looked
fine (not sure what he tests, probably Xen and his OS X bootloader?). In
my case it was a simple test kernel. I'd expect the sum of unknown
research or hobby kernels to be a major use case for Multiboot support,
though.

>> Fix the loading code to pass the whole loaded data to the Multiboot Option ROM.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  hw/loader.c |    2 --
>>  hw/pc.c     |   10 ++++++----
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/loader.c b/hw/loader.c
>> index 2d7a2c4..4c6981f 100644
>> --- a/hw/loader.c
>> +++ b/hw/loader.c
>> @@ -718,8 +718,6 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size)
>>      QTAILQ_FOREACH(rom, &roms, next) {
>>          if (rom->max)
>>              continue;
>> -        if (rom->min > addr)
>> -            continue;
>>          if (rom->min + rom->romsize < addr)
>>              continue;
>>          if (rom->min > end)
> 
> I don't understand this hunk.

The original code assumes that there is only one ROM that contains the
entry point. In fact, each program header of an ELF file becomes it own
ROM, though. So if you have a first PH which contains the entry point
(or now the lowest loaded address) and a second PH at a higher address,
this test would fail for the second one even though we need to load it.

What we really need to test is if [addr, end] and [rom->min, rom->min +
rom->romsize] have an intersection. This is what the following two ifs
already check.

Kevin

Re: [Qemu-devel] [FOR 0.12][PATCH] Fix loading of ELF multiboot kernels

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 16.12.2009 10:51, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> The multiboot implementation assumed that there is only one program header
>>> (which contains the entry point) and that the entry point is at the start of
>>> the code. This doesn't hold true generally and caused too little data to be
>>> loaded.
>> 
>> Out of curiosity: does this affect images people actually use?
>> Examples?
>
> It hit me, so yes. Probably no widespread kernels as Alex' tests looked
> fine (not sure what he tests, probably Xen and his OS X bootloader?). In
> my case it was a simple test kernel. I'd expect the sum of unknown
> research or hobby kernels to be a major use case for Multiboot support,
> though.

Thanks.

>>> Fix the loading code to pass the whole loaded data to the Multiboot Option ROM.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  hw/loader.c |    2 --
>>>  hw/pc.c     |   10 ++++++----
>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/loader.c b/hw/loader.c
>>> index 2d7a2c4..4c6981f 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -718,8 +718,6 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size)
>>>      QTAILQ_FOREACH(rom, &roms, next) {
>>>          if (rom->max)
>>>              continue;
>>> -        if (rom->min > addr)
>>> -            continue;
>>>          if (rom->min + rom->romsize < addr)
>>>              continue;
>>>          if (rom->min > end)
>> 
>> I don't understand this hunk.
>
> The original code assumes that there is only one ROM that contains the
> entry point. In fact, each program header of an ELF file becomes it own
> ROM, though. So if you have a first PH which contains the entry point
> (or now the lowest loaded address) and a second PH at a higher address,
> this test would fail for the second one even though we need to load it.
>
> What we really need to test is if [addr, end] and [rom->min, rom->min +
> rom->romsize] have an intersection. This is what the following two ifs
> already check.

So rom_copy() is supposed to copy all fixed-address ROMs in the range
addr..addr+size?  Makes sense then.