Re: [PATCH v2 4/8] KVM: arm64: Generate hyp relocation data

[Marc Zyngier <maz@kernel.org>]

On Fri, 29 Jan 2021 21:43:25 +0000,
Guenter Roeck <linux@roeck-us.net> 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@google.com>
> 
> 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@kernel.org>
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@roeck-us.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 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.