| Message ID | 20190812144442.30027-1-aaron@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | elf: Allow loading AArch64 ELF files | expand |
Patchew URL: https://patchew.org/QEMU/20190812144442.30027-1-aaron@os.amperecomputing.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] elf: Allow loading AArch64 ELF files Message-id: 20190812144442.30027-1-aaron@os.amperecomputing.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190812144442.30027-1-aaron@os.amperecomputing.com -> patchew/20190812144442.30027-1-aaron@os.amperecomputing.com Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3' Cloning into 'roms/edk2'... Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54' Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3' Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl' Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'... Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'CryptoPkg/Library/OpensslLib/openssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687' Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl' Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5' Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography' Cloning into 'boringssl'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f' Cloning into 'krb5'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168' Cloning into 'pyca-cryptography'... Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f' Cloning into 'roms/ipxe'... Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17' Cloning into 'roms/openbios'... Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174' Cloning into 'roms/openhackware'... Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5' Cloning into 'roms/opensbi'... Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6' Cloning into 'roms/qemu-palcode'... Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f' Cloning into 'roms/seabios'... Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4' Cloning into 'roms/seabios-hppa'... Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b' Cloning into 'roms/sgabios'... Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a' Cloning into 'roms/skiboot'... Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501' Cloning into 'roms/u-boot'... Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff' Cloning into 'roms/u-boot-sam460ex'... Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588' Cloning into 'slirp'... Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210' Cloning into 'tests/fp/berkeley-softfloat-3'... Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'tests/fp/berkeley-testfloat-3'... Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3' Cloning into 'ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' Switched to a new branch 'test' 4a7988f elf: Allow loading AArch64 ELF files === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Aaron Lindsay OS via Qemu-devel <qemu-devel@nongnu.org> total: 1 errors, 0 warnings, 12 lines checked Commit 4a7988f75dfa (elf: Allow loading AArch64 ELF files) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190812144442.30027-1-aaron@os.amperecomputing.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Mon, 12 Aug 2019 at 15:46, Aaron Lindsay OS via Qemu-arm <qemu-arm@nongnu.org> wrote: > > Treat EM_AARCH64 as a valid value when checking the ELF's machine-type > header. > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > --- > include/hw/elf_ops.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index 690f9238c8..f12faa90a1 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -381,6 +381,12 @@ static int glue(load_elf, SZ)(const char *name, int fd, > goto fail; > } > break; > + case EM_AARCH64: > + if (ehdr.e_machine != EM_AARCH64) { > + ret = ELF_LOAD_WRONG_ARCH; > + goto fail; > + } > + break; > default: > if (elf_machine != ehdr.e_machine) { > ret = ELF_LOAD_WRONG_ARCH; > -- > 2.17.1 What problem are we trying to solve here ? If I'm reading your patch correctly then it makes no difference to execution, because we're switching on 'elf_machine', and so this extra case is saying "if elf_machine is EM_AARCH64, insist that ehdr.e_machine is also EM_AARCH64", which is exactly what the default case would do anyway. The only reason to add extra cases in this switch is to handle the situation where a target's board/loader code says "I can handle elf files of type X" but actually this implicitly means it can handle both X and Y (so for EM_X86_64 we need to accept both EM_X86_64 and EM_386, for EM_PPC64 we need to accept both EM_PPC64 and EM_PPC, and so on). We don't have a case like that for aarch64/arm because the boot loader code has to deal with the 32 bit and 64 bit code separately anyway, so we can pass in the correct value depending on whether the CPU is 32-bit or 64-bit. The code in hw/arm/boot.c passes in an elf_machine value of EM_AARCH64 when it wants to load an AArch64 ELF file, so for that the default case is OK. The code in hw/core/generic-loader.c passes in 0 (EM_NONE) which will be handled by the check just before this switch statement, so the default case should work there too. Presumably there's some other code path for ELF file loading that doesn't work that you're trying to fix? thanks -- PMM
On Aug 12 16:02, Peter Maydell wrote: > On Mon, 12 Aug 2019 at 15:46, Aaron Lindsay OS via Qemu-arm > <qemu-arm@nongnu.org> wrote: > > > > Treat EM_AARCH64 as a valid value when checking the ELF's machine-type > > header. > > > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > > --- > > include/hw/elf_ops.h | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > > index 690f9238c8..f12faa90a1 100644 > > --- a/include/hw/elf_ops.h > > +++ b/include/hw/elf_ops.h > > @@ -381,6 +381,12 @@ static int glue(load_elf, SZ)(const char *name, int fd, > > goto fail; > > } > > break; > > + case EM_AARCH64: > > + if (ehdr.e_machine != EM_AARCH64) { > > + ret = ELF_LOAD_WRONG_ARCH; > > + goto fail; > > + } > > + break; > > default: > > if (elf_machine != ehdr.e_machine) { > > ret = ELF_LOAD_WRONG_ARCH; > > -- > > 2.17.1 > > What problem are we trying to solve here ? If I'm reading your patch > correctly then it makes no difference to execution, because we're > switching on 'elf_machine', and so this extra case is saying > "if elf_machine is EM_AARCH64, insist that ehdr.e_machine > is also EM_AARCH64", which is exactly what the default > case would do anyway. The only reason to add extra cases in > this switch is to handle the situation where a target's board/loader > code says "I can handle elf files of type X" but actually this > implicitly means it can handle both X and Y (so for EM_X86_64 we > need to accept both EM_X86_64 and EM_386, for EM_PPC64 we need to > accept both EM_PPC64 and EM_PPC, and so on). We don't have a > case like that for aarch64/arm because the boot loader code has > to deal with the 32 bit and 64 bit code separately anyway, so > we can pass in the correct value depending on whether the CPU > is 32-bit or 64-bit. > > The code in hw/arm/boot.c passes in an elf_machine value of > EM_AARCH64 when it wants to load an AArch64 ELF file, so > for that the default case is OK. The code in hw/core/generic-loader.c > passes in 0 (EM_NONE) which will be handled by the check just > before this switch statement, so the default case should > work there too. Presumably there's some other code path > for ELF file loading that doesn't work that you're trying to fix? Please disregard this patch. I'm sorry, upon closer inspection you are obviously correct... and I have no earthly idea what happened here. I hit the "goto fail" in the "default" case when debugging why I couldn't load an ELF on AArch64 last week. I was in a hurry and saw that there were other architectures in the switch/case and threw this code in there quickly without much thought, and after re-compiling, it worked! ...But after your email this morning, I'm completely unable to reproduce the failure case. I must have had another local issue which was coincidentally resolved at the same time, unbeknownst to me. -Aaron
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index 690f9238c8..f12faa90a1 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -381,6 +381,12 @@ static int glue(load_elf, SZ)(const char *name, int fd, goto fail; } break; + case EM_AARCH64: + if (ehdr.e_machine != EM_AARCH64) { + ret = ELF_LOAD_WRONG_ARCH; + goto fail; + } + break; default: if (elf_machine != ehdr.e_machine) { ret = ELF_LOAD_WRONG_ARCH;
Treat EM_AARCH64 as a valid value when checking the ELF's machine-type header. Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> --- include/hw/elf_ops.h | 6 ++++++ 1 file changed, 6 insertions(+)