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

[Aurelien Jarno]

On 2017-07-21 17:30, Alexey Kardashevskiy wrote:
> 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.

Indeed. For a better understanding of the issue, it should be noted that
the pseries platform can dynamically change its endianness at runtime
and thus can load both little and big endian kernel. This is done by
calling load_elf twice with both endianness. Therefore the fact that the
endianness *does not* match is not an issue in that case.

> > 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.

It display an error message, but this message doesn't display why the
ELF kernel failed to be loaded.
 
> > 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...

I would not call it a conflict, but it's definitely a corporte/hobbyist
issue. The corporate people designed a nice load_elf_strerror function
to report error and the hobbyist people failed to use it.

Anyway I have just sent a simple patch series improving the MIPS error
reporting. That should make everybody happy.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net