Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"

[Alexey Kardashevskiy]

On 21/07/17 16:48, Philippe Mathieu-Daudé wrote:
> Hi Alexey,
> 
> On 07/21/2017 01:19 AM, Alexey Kardashevskiy wrote:
>> This reverts c8e1158cf611 "elf-loader: warn about invalid endianness"
>> as it produces a useless message every time an LE kernel image is
>> passed via -kernel on a ppc64-pseries machine. The pseries machine
>> already checks for ELF_LOAD_WRONG_ENDIAN and tries with big_endian=0.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>   hw/core/loader.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index c17ace0a2e..e5e8cbb638 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -480,7 +480,6 @@ int load_elf_ram(const char *filename,
>>       }
>>         if (target_data_order != e_ident[EI_DATA]) {
>> -        fprintf(stderr, "%s: wrong endianness\n", filename);
>>           ret = ELF_LOAD_WRONG_ENDIAN;
>>           goto fail;
>>       }
>>
> 
> I submitted this patch because I spent some time debugging while QEMU was
> failing silently using a MIPS kernel image which used to work, after
> realizing I was in an incorrect build_dir using qemu-system-mipsel to load
> a big endian image and felt stupid [1]. This dumb error can happen to other
> people so I added this warning here.

Been there too. This is why we try loading images twice in pseries.

> I was not aware of the ELF_LOAD_WRONG_ENDIAN related code, and at least the
> MIPS arch is not using it.
> As I can see in MAINTAINERS, sPAPR is "Supported" meaning "Someone is
> actually paid to look after this", while there is no such paid person for
> the MIPS part.
> It seems each arch had a different way to load images and hw/core/loader.c
> was an effort to merge common code but mostly "Supported" arch are using it.

afaict MIPS calls load_elf() in 4 places, each checks for the return value
and already prints the message, how come that that message is not enough?
What would probably make sense here is if MIPS also printed the return code
from load_elf() but in any case it is easily visible from gdb.


> While your revert does fixes your sPAPR warning issue, looking at the
> problem roots I think the correct fix is to improve the MIPS port and
> eventually the less loved archs to unify the loader.c calls and avoid such
> problems.
> I don't object reverting this patch for 2.10 and improve the loader.c usage
> during 2.11 cycle, I only wonder if this is another corporate/hobbyist> 
> conflict of interest with corporate crushing on hobbyist instead of

Come on mate...


> helping, motivating contribution improving common code usage.
> 
> Cc'ed MIPS and loader.c maintainers (both "Maintained" and not "Supported").
> 
> Phil.
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg05926.html


-- 
Alexey