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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 16.12.2009 10:51, schrieb Markus Armbruster:
>> Kevin Wolf <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.