Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
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
> >> ---
> >> 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
aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
On 07/26/2017 12:47 AM, Alexey Kardashevskiy wrote: On 21/07/17 18:05, Philippe Mathieu-Daudé wrote: On 07/21/2017 04:30 AM, Alexey Kardashevskiy wrote: On 21/07/17 16:48, Philippe Mathieu-Daudé wrote: 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... https://www.quora.com/Why-do-the-French-complain-all-the-time :D Good one :) What about the reverting patch, is it going anywhere? Thanks. OK by me for 2.10. Now, is there someone from the industry willing to cleanup/unify the loader codes from the various archs during 2.11? Regards, Phil. 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
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
On 21/07/17 18:05, Philippe Mathieu-Daudé wrote: > On 07/21/2017 04:30 AM, Alexey Kardashevskiy wrote: >> On 21/07/17 16:48, Philippe Mathieu-Daudé wrote: >>> 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... > > https://www.quora.com/Why-do-the-French-complain-all-the-time > > :D Good one :) What about the reverting patch, is it going anywhere? Thanks. > >> >> >>> 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
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
On 07/21/2017 04:30 AM, Alexey Kardashevskiy wrote: On 21/07/17 16:48, Philippe Mathieu-Daudé wrote: 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... https://www.quora.com/Why-do-the-French-complain-all-the-time :D 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
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
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
>> ---
>> 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
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
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
---
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.
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.
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 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
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
Hi Jason, Michael, On 07/21/2017 01:55 AM, no-re...@patchew.org wrote: This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. [...] GTester: last random seed: R02Sd7d1d0ad279b9a1e69acb248301b1ab6 Vhost user backend fails to broadcast fake RARP make: *** [check-tests/test-replication] Error 1 Any idea about this error?
Re: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Type: series Subject: [Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness" Message-id: 20170721041952.45950-1-...@ozlabs.ru === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-build@min-glib time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 1ec97a9 Revert "elf-loader: warn about invalid endianness" === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-dkcfo8gx/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-dkcfo8gx/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 bison-2.4.1-5.el6.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch flex-2.5.35-9.el6.x86_64 gcc-4.4.7-18.el6.x86_64 git-1.7.1-8.el6.x86_64 glib2-devel-2.28.8-9.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ flex bison zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=847e13737f8c TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install No C++ compiler available; disabling C++ specific optional code Install prefix/var/tmp/qemu-build/install BIOS directory/var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install pythonpython -B smbd /usr/sbin/smbd module supportno host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu gprof enabled no sparse enabledno strip binariesyes profiler no static build no pixmansystem SDL support yes (1.2.14) GTK support no GTK GL supportno VTE support no TLS priority NORMAL GNUTLS supportno GNUTLS rndno libgcrypt no libgcrypt kdf no nettleno nettle kdfno libtasn1 no curses supportno virgl support no curl support no mingw32 support no Audio drivers oss Block whitelist (rw) Block whitelist (ro) VirtFS supportno VNC support yes VNC SASL support no VNC JPEG support no VNC PNG support no xen support no brlapi supportno bluez supportno Documentation no PIE yes vde support no netmap supportno Linux AIO support no ATTR/XATTR support yes Install blobs
[Qemu-devel] [PATCH qemu] Revert "elf-loader: warn about invalid endianness"
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
---
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;
}
--
2.11.0
