On Sat, Jan 30, 2021 at 01:44:15PM +0000, Marc Zyngier wrote:
> On Fri, 29 Jan 2021 21:43:25 +0000,
> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Tue, Jan 05, 2021 at 06:05:37PM +0000, David Brazdil wrote:
> > > Add a post-processing step to compilation of KVM nVHE hyp code which
> > > calls a custom host tool (gen-hyprel) on the partially linked object
> > > file (hyp sections' names prefixed).
> > >
> > > The tool lists all R_AARCH64_ABS64 data relocations targeting hyp
> > > sections and generates an assembly file that will form a new section
> > > .hyp.reloc in the kernel binary. The new section contains an array of
> > > 32-bit offsets to the positions targeted by these relocations.
> > >
> > > Since these addresses of those positions will not be determined until
> > > linking of `vmlinux`, each 32-bit entry carries a R_AARCH64_PREL32
> > > relocation with addend <section_base_sym> + <r_offset>. The linker of
> > > `vmlinux` will therefore fill the slot accordingly.
> > >
> > > This relocation data will be used at runtime to convert the kernel VAs
> > > at those positions to hyp VAs.
> > >
> > > Signed-off-by: David Brazdil <dbrazdil@xxxxxxxxxx>
> >
> > This patch results in the following error for me.
> >
> > error: arch/arm64/kvm/hyp/nvhe/kvm_nvhe.tmp.o: assertion elf.ehdr->e_ident[5] == 1 failed (lhs=2, rhs=1, line=250)
> >
> > The problem is seen when trying to build aarch64 images in big endian
> > mode.
> >
> > Te script used to reproduce the problem as well as bisect results are
> > attached.
>
> I came up with the following patch, which allows the kernel to link
> and boot. I don't have any BE userspace, so I didn't verify that I
> could boot a guest (the hypervisor does correctly initialise though).
>
> It's not exactly pretty, but it does the job...
>
> Thanks,
>
> M.
>
> From d80ca05b2ed90fc30d328041692fa80f525c8d12 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Date: Sat, 30 Jan 2021 13:07:51 +0000
> Subject: [PATCH] KVM: arm64: Make gen-hyprel endianness agnostic
>
> gen-hyprel is, for better or worse, a native-endian program:
> it assumes that the ELF data structures are in the host's
> endianness, and even assumes that the compiled kernel is
> little-endian in one particular case.
>
> None of these assumptions hold true though: people actually build
> (use?) BE arm64 kernels, and seem to avoid doing so on BE hosts.
> Madness!
>
> In order to solve this, wrap each access to the ELF data structures
> with the required byte-swapping magic. This requires to obtain
> the kernel data structure, and provide per-endianess wrappers.
>
> This result in a kernel that links and even boots in a model.
>
> Fixes: 8c49b5d43d4c ("KVM: arm64: Generate hyp relocation data")
> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Compiles and boots both big- and little-endian systems in qemu.
Guenter
> ---
> arch/arm64/kvm/hyp/nvhe/Makefile | 1 +
> arch/arm64/kvm/hyp/nvhe/gen-hyprel.c | 57 ++++++++++++++++++++--------
> 2 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index 268be1376f74..09d04dd50eb8 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -7,6 +7,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__
> ccflags-y := -D__KVM_NVHE_HYPERVISOR__
>
> hostprogs := gen-hyprel
> +HOST_EXTRACFLAGS += -I$(srctree)/include
>
> obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o \
> hyp-main.o hyp-smp.o psci-relay.o
> diff --git a/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c b/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
> index 58fe31fdba8e..ead02c6a7628 100644
> --- a/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
> +++ b/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
> @@ -25,6 +25,7 @@
> */
>
> #include <elf.h>
> +#include <endian.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <stdbool.h>
> @@ -36,6 +37,8 @@
> #include <sys/stat.h>
> #include <unistd.h>
>
> +#include <generated/autoconf.h>
> +
> #define HYP_SECTION_PREFIX ".hyp"
> #define HYP_RELOC_SECTION ".hyp.reloc"
> #define HYP_SECTION_SYMBOL_PREFIX "__hyp_section_"
> @@ -121,6 +124,28 @@ static struct {
> const char *sh_string;
> } elf;
>
> +#if defined(CONFIG_CPU_LITTLE_ENDIAN)
> +
> +#define elf16toh(x) le16toh(x)
> +#define elf32toh(x) le32toh(x)
> +#define elf64toh(x) le64toh(x)
> +
> +#define ELFENDIAN ELFDATA2LSB
> +
> +#elif defined(CONFIG_CPU_BIG_ENDIAN)
> +
> +#define elf16toh(x) be16toh(x)
> +#define elf32toh(x) be32toh(x)
> +#define elf64toh(x) be64toh(x)
> +
> +#define ELFENDIAN ELFDATA2MSB
> +
> +#else
> +
> +#error PDP-endian sadly unsupported...
> +
> +#endif
> +
> #define fatal_error(fmt, ...) \
> ({ \
> fprintf(stderr, "error: %s: " fmt "\n", \
> @@ -162,12 +187,12 @@ static struct {
>
> /* Iterate over all sections in the ELF. */
> #define for_each_section(var) \
> - for (var = elf.sh_table; var < elf.sh_table + elf.ehdr->e_shnum; ++var)
> + for (var = elf.sh_table; var < elf.sh_table + elf16toh(elf.ehdr->e_shnum); ++var)
>
> /* Iterate over all Elf64_Rela relocations in a given section. */
> #define for_each_rela(shdr, var) \
> - for (var = elf_ptr(Elf64_Rela, shdr->sh_offset); \
> - var < elf_ptr(Elf64_Rela, shdr->sh_offset + shdr->sh_size); var++)
> + for (var = elf_ptr(Elf64_Rela, elf64toh(shdr->sh_offset)); \
> + var < elf_ptr(Elf64_Rela, elf64toh(shdr->sh_offset) + elf64toh(shdr->sh_size)); var++)
>
> /* True if a string starts with a given prefix. */
> static inline bool starts_with(const char *str, const char *prefix)
> @@ -178,13 +203,13 @@ static inline bool starts_with(const char *str, const char *prefix)
> /* Returns a string containing the name of a given section. */
> static inline const char *section_name(Elf64_Shdr *shdr)
> {
> - return elf.sh_string + shdr->sh_name;
> + return elf.sh_string + elf32toh(shdr->sh_name);
> }
>
> /* Returns a pointer to the first byte of section data. */
> static inline const char *section_begin(Elf64_Shdr *shdr)
> {
> - return elf_ptr(char, shdr->sh_offset);
> + return elf_ptr(char, elf64toh(shdr->sh_offset));
> }
>
> /* Find a section by its offset from the beginning of the file. */
> @@ -247,13 +272,13 @@ static void init_elf(const char *path)
>
> /* Sanity check that this is an ELF64 relocatable object for AArch64. */
> assert_eq(elf.ehdr->e_ident[EI_CLASS], ELFCLASS64, "%u");
> - assert_eq(elf.ehdr->e_ident[EI_DATA], ELFDATA2LSB, "%u");
> - assert_eq(elf.ehdr->e_type, ET_REL, "%u");
> - assert_eq(elf.ehdr->e_machine, EM_AARCH64, "%u");
> + assert_eq(elf.ehdr->e_ident[EI_DATA], ELFENDIAN, "%u");
> + assert_eq(elf16toh(elf.ehdr->e_type), ET_REL, "%u");
> + assert_eq(elf16toh(elf.ehdr->e_machine), EM_AARCH64, "%u");
>
> /* Populate fields of the global struct. */
> - elf.sh_table = section_by_off(elf.ehdr->e_shoff);
> - elf.sh_string = section_begin(section_by_idx(elf.ehdr->e_shstrndx));
> + elf.sh_table = section_by_off(elf64toh(elf.ehdr->e_shoff));
> + elf.sh_string = section_begin(section_by_idx(elf16toh(elf.ehdr->e_shstrndx)));
> }
>
> /* Print the prologue of the output ASM file. */
> @@ -301,8 +326,8 @@ static void emit_rela_abs64(Elf64_Rela *rela, const char *sh_orig_name)
> * is `rela->r_offset`.
> */
> printf(".reloc %lu, R_AARCH64_PREL32, %s%s + 0x%lx\n",
> - reloc_offset, HYP_SECTION_SYMBOL_PREFIX, sh_orig_name,
> - rela->r_offset);
> + reloc_offset, HYP_SECTION_SYMBOL_PREFIX, sh_orig_name,
> + elf64toh(rela->r_offset));
>
> reloc_offset += 4;
> }
> @@ -322,7 +347,7 @@ static void emit_epilogue(void)
> */
> static void emit_rela_section(Elf64_Shdr *sh_rela)
> {
> - Elf64_Shdr *sh_orig = &elf.sh_table[sh_rela->sh_info];
> + Elf64_Shdr *sh_orig = &elf.sh_table[elf32toh(sh_rela->sh_info)];
> const char *sh_orig_name = section_name(sh_orig);
> Elf64_Rela *rela;
>
> @@ -333,10 +358,10 @@ static void emit_rela_section(Elf64_Shdr *sh_rela)
> emit_section_prologue(sh_orig_name);
>
> for_each_rela(sh_rela, rela) {
> - uint32_t type = (uint32_t)rela->r_info;
> + uint32_t type = (uint32_t)elf64toh(rela->r_info);
>
> /* Check that rela points inside the relocated section. */
> - assert_lt(rela->r_offset, sh_orig->sh_size, "0x%lx");
> + assert_lt(elf64toh(rela->r_offset), elf64toh(sh_orig->sh_size), "0x%lx");
>
> switch (type) {
> /*
> @@ -385,7 +410,7 @@ static void emit_all_relocs(void)
> Elf64_Shdr *shdr;
>
> for_each_section(shdr) {
> - switch (shdr->sh_type) {
> + switch (elf32toh(shdr->sh_type)) {
> case SHT_REL:
> fatal_error("Unexpected SHT_REL section \"%s\"",
> section_name(shdr));
> --
> 2.29.2
>
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
