On Aug 12 16:02, Peter Maydell wrote:
> On Mon, 12 Aug 2019 at 15:46, Aaron Lindsay OS via Qemu-arm
> <qemu-...@nongnu.org> wrote:
> >
> > Treat EM_AARCH64 as a valid value when checking the ELF's machine-type
> > header.
> >
> > Signed-off-by: Aaron Lindsay <aa...@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
