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

[Kevin Wolf <kwolf@redhat.com>]

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