[kvm-unit-tests PATCH 00/13] Enable EFI support

[kvm-unit-tests PATCH 00/13] Enable EFI support

[Andrew Jones]

This series starts with some fixes for backtraces for bugs found
when tracing with riscv EFI builds. The series then brings EFI
support to riscv, basing the approach heavily on arm64's support
(including arm64's improvements[1]). It should now be possible
to launch tests from EFI-capable bootloaders.

[1] https://lore.kernel.org/all/20240227192109.487402-20-andrew.jones@linux.dev/

Thanks,
drew

Andrew Jones (13):
  riscv: Call abort instead of assert on unhandled exceptions
  riscv: show_regs: Prepare for EFI images
  treewide: lib/stack: Fix backtrace
  treewide: lib/stack: Make base_address arch specific
  riscv: Import gnu-efi files
  riscv: Tweak the gnu-efi imported code
  riscv: Enable building for EFI
  riscv: efi: Switch stack in _start
  efi: Add support for obtaining the boot hartid
  riscv: Refactor setup code
  riscv: Enable EFI boot
  riscv: efi: Add run script
  riscv: efi: Use efi-direct by default

 configure                     |  12 +-
 lib/arm/stack.c               |  13 +--
 lib/arm64/stack.c             |  29 +++--
 lib/efi.c                     |  33 ++++++
 lib/elf.h                     |   5 +
 lib/riscv/asm/setup.h         |   5 +
 lib/riscv/processor.c         |  11 +-
 lib/riscv/setup.c             | 170 +++++++++++++++++++++-------
 lib/riscv/stack.c             |  30 +++--
 lib/s390x/stack.c             |  12 +-
 lib/stack.c                   |  19 +---
 lib/stack.h                   |  26 +++--
 lib/x86/stack.c               |  29 +++--
 riscv/Makefile                |  24 +++-
 riscv/cstart.S                |   4 +
 riscv/efi/crt0-efi-riscv64.S  | 205 ++++++++++++++++++++++++++++++++++
 riscv/efi/elf_riscv64_efi.lds | 142 +++++++++++++++++++++++
 riscv/efi/reloc_riscv64.c     |  91 +++++++++++++++
 riscv/efi/run                 | 106 ++++++++++++++++++
 riscv/flat.lds                |   1 +
 riscv/run                     |   2 +-
 21 files changed, 859 insertions(+), 110 deletions(-)
 create mode 100644 riscv/efi/crt0-efi-riscv64.S
 create mode 100644 riscv/efi/elf_riscv64_efi.lds
 create mode 100644 riscv/efi/reloc_riscv64.c
 create mode 100755 riscv/efi/run

-- 
2.43.0

[kvm-unit-tests PATCH 01/13] riscv: Call abort instead of assert on unhandled exceptions

[Andrew Jones]

We should call abort() instead of assert() on an unhandled
exception since assert() calls abort() anyway after a useless
"assert failed: 0" message. We can also skip dumping the
exception stack and just unwind from the stack frame where
the exception occurred.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/processor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
index e0904209c0da..6c868b805cf7 100644
--- a/lib/riscv/processor.c
+++ b/lib/riscv/processor.c
@@ -43,7 +43,8 @@ void do_handle_exception(struct pt_regs *regs)
 	}
 
 	show_regs(regs);
-	assert(0);
+	dump_frame_stack((void *)regs->epc, (void *)regs->s0);
+	abort();
 }
 
 void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *))
-- 
2.43.0

[kvm-unit-tests PATCH 02/13] riscv: show_regs: Prepare for EFI images

[Andrew Jones]

EFI images start with a header page and then _text, so the load
address should use 'ImageBase' instead of _text. Just add the
ImageBase symbol to the non-efi build too and then change show_regs()
to use it instead. While there, add a couple convenience calculations
for the PC and return address (pre-subtract the load address from
them) in order to make it quicker for looking them up in an objdump
(Note, for EFI, one must dump the .so file, which isn't preserved by
default, but can be by uncommenting out the '.PRECIOUS: %.so' line in
the makefile.)

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/processor.c | 8 ++++----
 riscv/flat.lds        | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
index 6c868b805cf7..ece7cbffc6dd 100644
--- a/lib/riscv/processor.c
+++ b/lib/riscv/processor.c
@@ -8,20 +8,20 @@
 #include <asm/processor.h>
 #include <asm/setup.h>
 
-extern unsigned long _text;
+extern unsigned long ImageBase;
 
 void show_regs(struct pt_regs *regs)
 {
 	struct thread_info *info = current_thread_info();
-	uintptr_t text = (uintptr_t)&_text;
+	uintptr_t loadaddr = (uintptr_t)&ImageBase;
 	unsigned int w = __riscv_xlen / 4;
 
-	printf("Load address: %" PRIxPTR "\n", text);
+	printf("Load address: %" PRIxPTR "\n", loadaddr);
 	printf("CPU%3d : hartid=%lx\n", info->cpu, info->hartid);
 	printf("status : %.*lx\n", w, regs->status);
 	printf("cause  : %.*lx\n", w, regs->cause);
 	printf("badaddr: %.*lx\n", w, regs->badaddr);
-	printf("pc: %.*lx ra: %.*lx\n", w, regs->epc, w, regs->ra);
+	printf("pc: %.*lx (%lx) ra: %.*lx (%lx)\n", w, regs->epc, regs->epc - loadaddr, w, regs->ra, regs->ra - loadaddr);
 	printf("sp: %.*lx gp: %.*lx tp : %.*lx\n", w, regs->sp, w, regs->gp, w, regs->tp);
 	printf("a0: %.*lx a1: %.*lx a2 : %.*lx a3 : %.*lx\n", w, regs->a0, w, regs->a1, w, regs->a2, w, regs->a3);
 	printf("a4: %.*lx a5: %.*lx a6 : %.*lx a7 : %.*lx\n", w, regs->a4, w, regs->a5, w, regs->a6, w, regs->a7);
diff --git a/riscv/flat.lds b/riscv/flat.lds
index d4853f82ba1c..1ca501e6593b 100644
--- a/riscv/flat.lds
+++ b/riscv/flat.lds
@@ -30,6 +30,7 @@ PHDRS
 
 SECTIONS
 {
+    PROVIDE(ImageBase = .);
     PROVIDE(_text = .);
     .text : { *(.init) *(.text) *(.text.*) } :text
     . = ALIGN(4K);
-- 
2.43.0

[kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace

[Andrew Jones]

We should never pass the result of __builtin_frame_address(0) to
another function since the compiler is within its rights to pop the
frame to which it points before making the function call, as may be
done for tail calls. Nobody has complained about backtrace(), so
likely all compilations have been inlining backtrace_frame(), not
dropping the frame on the tail call, or nobody is looking at traces.
However, for riscv, when built for EFI, it does drop the frame on the
tail call, and it was noticed. Preemptively fix backtrace() for all
architectures.

Fixes: 52266791750d ("lib: backtrace printing")
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/arm/stack.c   | 13 +++++--------
 lib/arm64/stack.c | 12 +++++-------
 lib/riscv/stack.c | 12 +++++-------
 lib/s390x/stack.c | 12 +++++-------
 lib/stack.h       | 24 +++++++++++++++++-------
 lib/x86/stack.c   | 12 +++++-------
 6 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/lib/arm/stack.c b/lib/arm/stack.c
index 7d081be7c6d0..66d18b47ea53 100644
--- a/lib/arm/stack.c
+++ b/lib/arm/stack.c
@@ -8,13 +8,16 @@
 #include <libcflat.h>
 #include <stack.h>
 
-int backtrace_frame(const void *frame, const void **return_addrs,
-		    int max_depth)
+int arch_backtrace_frame(const void *frame, const void **return_addrs,
+			 int max_depth, bool current_frame)
 {
 	static int walking;
 	int depth;
 	const unsigned long *fp = (unsigned long *)frame;
 
+	if (current_frame)
+		fp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs,
 	walking = 0;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0),
-			       return_addrs, max_depth);
-}
diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
index 82611f4b1815..f5eb57fd8892 100644
--- a/lib/arm64/stack.c
+++ b/lib/arm64/stack.c
@@ -8,7 +8,8 @@
 
 extern char vector_stub_start, vector_stub_end;
 
-int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
+int arch_backtrace_frame(const void *frame, const void **return_addrs,
+			 int max_depth, bool current_frame)
 {
 	const void *fp = frame;
 	static bool walking;
@@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	bool is_exception = false;
 	unsigned long addr;
 
+	if (current_frame)
+		fp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	walking = false;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0),
-			       return_addrs, max_depth);
-}
diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
index 712a5478d547..d865594b9671 100644
--- a/lib/riscv/stack.c
+++ b/lib/riscv/stack.c
@@ -2,12 +2,16 @@
 #include <libcflat.h>
 #include <stack.h>
 
-int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
+int arch_backtrace_frame(const void *frame, const void **return_addrs,
+			 int max_depth, bool current_frame)
 {
 	static bool walking;
 	const unsigned long *fp = (unsigned long *)frame;
 	int depth;
 
+	if (current_frame)
+		fp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	walking = false;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0),
-			       return_addrs, max_depth);
-}
diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
index 9f234a12adf6..d194f654e94d 100644
--- a/lib/s390x/stack.c
+++ b/lib/s390x/stack.c
@@ -14,11 +14,15 @@
 #include <stack.h>
 #include <asm/arch_def.h>
 
-int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
+int arch_backtrace_frame(const void *frame, const void **return_addrs,
+			 int max_depth, bool current_frame)
 {
 	int depth = 0;
 	struct stack_frame *stack = (struct stack_frame *)frame;
 
+	if (current_frame)
+		stack = __builtin_frame_address(0);
+
 	for (depth = 0; stack && depth < max_depth; depth++) {
 		return_addrs[depth] = (void *)stack->grs[8];
 		stack = stack->back_chain;
@@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0),
-			       return_addrs, max_depth);
-}
diff --git a/lib/stack.h b/lib/stack.h
index 10fc2f793354..6edc84344b51 100644
--- a/lib/stack.h
+++ b/lib/stack.h
@@ -11,17 +11,27 @@
 #include <asm/stack.h>
 
 #ifdef HAVE_ARCH_BACKTRACE_FRAME
-extern int backtrace_frame(const void *frame, const void **return_addrs,
-			   int max_depth);
+extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
+				int max_depth, bool current_frame);
+
+static inline int backtrace_frame(const void *frame, const void **return_addrs,
+				  int max_depth)
+{
+	return arch_backtrace_frame(frame, return_addrs, max_depth, false);
+}
+
+static inline int backtrace(const void **return_addrs, int max_depth)
+{
+	return arch_backtrace_frame(NULL, return_addrs, max_depth, true);
+}
 #else
-static inline int
-backtrace_frame(const void *frame __unused, const void **return_addrs __unused,
-		int max_depth __unused)
+extern int backtrace(const void **return_addrs, int max_depth);
+
+static inline int backtrace_frame(const void *frame, const void **return_addrs,
+				  int max_depth)
 {
 	return 0;
 }
 #endif
 
-extern int backtrace(const void **return_addrs, int max_depth);
-
 #endif
diff --git a/lib/x86/stack.c b/lib/x86/stack.c
index 5ecd97ce90b9..58ab6c4b293a 100644
--- a/lib/x86/stack.c
+++ b/lib/x86/stack.c
@@ -1,12 +1,16 @@
 #include <libcflat.h>
 #include <stack.h>
 
-int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
+int arch_backtrace_frame(const void *frame, const void **return_addrs,
+			 int max_depth, bool current_frame)
 {
 	static int walking;
 	int depth = 0;
 	const unsigned long *bp = (unsigned long *) frame;
 
+	if (current_frame)
+		bp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	walking = 0;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0), return_addrs,
-			       max_depth);
-}
-- 
2.43.0

[kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific

[Andrew Jones]

Calculating the offset of an address is image specific, which is
architecture specific. Until now, all architectures and architecture
configurations which select CONFIG_RELOC were able to subtract
_etext, but the EFI configuration of riscv cannot (it must subtract
ImageBase). Make this function architecture specific, since the
architecture's image layout already is.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/arm64/stack.c | 17 +++++++++++++++++
 lib/riscv/stack.c | 18 ++++++++++++++++++
 lib/stack.c       | 19 ++-----------------
 lib/stack.h       |  2 ++
 lib/x86/stack.c   | 17 +++++++++++++++++
 5 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
index f5eb57fd8892..3369031a74f7 100644
--- a/lib/arm64/stack.c
+++ b/lib/arm64/stack.c
@@ -6,6 +6,23 @@
 #include <stdbool.h>
 #include <stack.h>
 
+#ifdef CONFIG_RELOC
+extern char _text, _etext;
+
+bool base_address(const void *rebased_addr, unsigned long *addr)
+{
+	unsigned long ra = (unsigned long)rebased_addr;
+	unsigned long start = (unsigned long)&_text;
+	unsigned long end = (unsigned long)&_etext;
+
+	if (ra < start || ra >= end)
+		return false;
+
+	*addr = ra - start;
+	return true;
+}
+#endif
+
 extern char vector_stub_start, vector_stub_end;
 
 int arch_backtrace_frame(const void *frame, const void **return_addrs,
diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
index d865594b9671..a143c22a570a 100644
--- a/lib/riscv/stack.c
+++ b/lib/riscv/stack.c
@@ -2,6 +2,24 @@
 #include <libcflat.h>
 #include <stack.h>
 
+#ifdef CONFIG_RELOC
+extern char ImageBase, _text, _etext;
+
+bool base_address(const void *rebased_addr, unsigned long *addr)
+{
+	unsigned long ra = (unsigned long)rebased_addr;
+	unsigned long base = (unsigned long)&ImageBase;
+	unsigned long start = (unsigned long)&_text;
+	unsigned long end = (unsigned long)&_etext;
+
+	if (ra < start || ra >= end)
+		return false;
+
+	*addr = ra - base;
+	return true;
+}
+#endif
+
 int arch_backtrace_frame(const void *frame, const void **return_addrs,
 			 int max_depth, bool current_frame)
 {
diff --git a/lib/stack.c b/lib/stack.c
index dd6bfa8dac6e..e5099e207388 100644
--- a/lib/stack.c
+++ b/lib/stack.c
@@ -11,23 +11,8 @@
 
 #define MAX_DEPTH 20
 
-#ifdef CONFIG_RELOC
-extern char _text, _etext;
-
-static bool base_address(const void *rebased_addr, unsigned long *addr)
-{
-	unsigned long ra = (unsigned long)rebased_addr;
-	unsigned long start = (unsigned long)&_text;
-	unsigned long end = (unsigned long)&_etext;
-
-	if (ra < start || ra >= end)
-		return false;
-
-	*addr = ra - start;
-	return true;
-}
-#else
-static bool base_address(const void *rebased_addr, unsigned long *addr)
+#ifndef CONFIG_RELOC
+bool base_address(const void *rebased_addr, unsigned long *addr)
 {
 	*addr = (unsigned long)rebased_addr;
 	return true;
diff --git a/lib/stack.h b/lib/stack.h
index 6edc84344b51..f8def4ad4d49 100644
--- a/lib/stack.h
+++ b/lib/stack.h
@@ -10,6 +10,8 @@
 #include <libcflat.h>
 #include <asm/stack.h>
 
+bool base_address(const void *rebased_addr, unsigned long *addr);
+
 #ifdef HAVE_ARCH_BACKTRACE_FRAME
 extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
 				int max_depth, bool current_frame);
diff --git a/lib/x86/stack.c b/lib/x86/stack.c
index 58ab6c4b293a..7ba73becbd69 100644
--- a/lib/x86/stack.c
+++ b/lib/x86/stack.c
@@ -1,6 +1,23 @@
 #include <libcflat.h>
 #include <stack.h>
 
+#ifdef CONFIG_RELOC
+extern char _text, _etext;
+
+bool base_address(const void *rebased_addr, unsigned long *addr)
+{
+	unsigned long ra = (unsigned long)rebased_addr;
+	unsigned long start = (unsigned long)&_text;
+	unsigned long end = (unsigned long)&_etext;
+
+	if (ra < start || ra >= end)
+		return false;
+
+	*addr = ra - start;
+	return true;
+}
+#endif
+
 int arch_backtrace_frame(const void *frame, const void **return_addrs,
 			 int max_depth, bool current_frame)
 {
-- 
2.43.0

[kvm-unit-tests PATCH 05/13] riscv: Import gnu-efi files

[Andrew Jones]

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/efi/crt0-efi-riscv64.S  | 187 ++++++++++++++++++++++++++++++++++
 riscv/efi/elf_riscv64_efi.lds | 142 ++++++++++++++++++++++++++
 riscv/efi/reloc_riscv64.c     |  90 ++++++++++++++++
 3 files changed, 419 insertions(+)
 create mode 100644 riscv/efi/crt0-efi-riscv64.S
 create mode 100644 riscv/efi/elf_riscv64_efi.lds
 create mode 100644 riscv/efi/reloc_riscv64.c

diff --git a/riscv/efi/crt0-efi-riscv64.S b/riscv/efi/crt0-efi-riscv64.S
new file mode 100644
index 000000000000..712ed03fc06e
--- /dev/null
+++ b/riscv/efi/crt0-efi-riscv64.S
@@ -0,0 +1,187 @@
+/* SPDX-License-Identifier: GPL-2.0+ OR BSD-2-Clause */
+/*
+ * Copright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ * Copright (C) 2018 Alexander Graf <agraf@suse.de>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice and this list of conditions, without modification.
+ * 2. The name of the author may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License as published by the Free Software Foundation;
+ * either version 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef EFI_SUBSYSTEM
+#define EFI_SUBSYSTEM 10
+#endif
+
+	.section	.text.head
+
+	/*
+	 * Magic "MZ" signature for PE/COFF
+	 */
+	.globl	ImageBase
+ImageBase:
+	.ascii	"MZ"
+	.skip	58				// 'MZ' + pad + offset == 64
+	.4byte	pe_header - ImageBase		// Offset to the PE header.
+pe_header:
+	.ascii	"PE"
+	.2byte 	0
+coff_header:
+	.2byte	0x5064				// riscv64
+	.2byte	4				// nr_sections
+	.4byte	0 				// TimeDateStamp
+	.4byte	0				// PointerToSymbolTable
+	.4byte	0				// NumberOfSymbols
+	.2byte	section_table - optional_header	// SizeOfOptionalHeader
+	.2byte	0x206				// Characteristics.
+						// IMAGE_FILE_DEBUG_STRIPPED |
+						// IMAGE_FILE_EXECUTABLE_IMAGE |
+						// IMAGE_FILE_LINE_NUMS_STRIPPED
+optional_header:
+	.2byte	0x20b				// PE32+ format
+	.byte	0x02				// MajorLinkerVersion
+	.byte	0x14				// MinorLinkerVersion
+	.4byte	_text_size - ImageBase		// SizeOfCode
+	.4byte	_alldata_size - ImageBase		// SizeOfInitializedData
+	.4byte	0				// SizeOfUninitializedData
+	.4byte	_start - ImageBase		// AddressOfEntryPoint
+	.4byte	_text - ImageBase		// BaseOfCode
+
+extra_header_fields:
+	.8byte	0				// ImageBase
+	.4byte	0x1000				// SectionAlignment
+	.4byte	0x1000				// FileAlignment
+	.2byte	0				// MajorOperatingSystemVersion
+	.2byte	0				// MinorOperatingSystemVersion
+	.2byte	0				// MajorImageVersion
+	.2byte	0				// MinorImageVersion
+	.2byte	0				// MajorSubsystemVersion
+	.2byte	0				// MinorSubsystemVersion
+	.4byte	0				// Win32VersionValue
+
+	.4byte	_image_end - ImageBase		// SizeOfImage
+
+	// Everything before the kernel image is considered part of the header
+	.4byte	_text - ImageBase		// SizeOfHeaders
+	.4byte	0				// CheckSum
+	.2byte	EFI_SUBSYSTEM			// Subsystem
+	.2byte	0				// DllCharacteristics
+	.8byte	0				// SizeOfStackReserve
+	.8byte	0				// SizeOfStackCommit
+	.8byte	0				// SizeOfHeapReserve
+	.8byte	0				// SizeOfHeapCommit
+	.4byte	0				// LoaderFlags
+	.4byte	0x10				// NumberOfRvaAndSizes
+
+	.8byte	0				// ExportTable
+	.8byte	0				// ImportTable
+	.8byte	0				// ResourceTable
+	.8byte	0				// ExceptionTable
+	.8byte	0				// CertificationTable
+	.4byte	_reloc - ImageBase				// BaseRelocationTable (VirtualAddress)
+	.4byte	_reloc_vsize - ImageBase				// BaseRelocationTable (Size)
+	.8byte	0				// Debug
+	.8byte	0				// Architecture
+	.8byte	0				// Global Ptr
+	.8byte	0				// TLS Table
+	.8byte	0				// Load Config Table
+	.8byte	0				// Bound Import
+	.8byte	0				// IAT
+	.8byte	0				// Delay Import Descriptor
+	.8byte	0				// CLR Runtime Header
+	.8byte	0				// Reserved, must be zero
+
+	// Section table
+section_table:
+
+	.ascii	".text\0\0\0"
+	.4byte	_text_vsize - ImageBase		// VirtualSize
+	.4byte	_text - ImageBase	// VirtualAddress
+	.4byte	_text_size - ImageBase		// SizeOfRawData
+	.4byte	_text - ImageBase	// PointerToRawData
+	.4byte	0		// PointerToRelocations (0 for executables)
+	.4byte	0		// PointerToLineNumbers (0 for executables)
+	.2byte	0		// NumberOfRelocations  (0 for executables)
+	.2byte	0		// NumberOfLineNumbers  (0 for executables)
+	.4byte	0x60000020	// Characteristics (section flags)
+
+	/*
+	 * The EFI application loader requires a relocation section
+	 * because EFI applications must be relocatable.  This is a
+	 * dummy section as far as we are concerned.
+	 */
+	.ascii	".reloc\0\0"
+	.4byte	_reloc_vsize - ImageBase			// VirtualSize
+	.4byte	_reloc - ImageBase			// VirtualAddress
+	.4byte	_reloc_size - ImageBase			// SizeOfRawData
+	.4byte	_reloc - ImageBase			// PointerToRawData
+	.4byte	0			// PointerToRelocations
+	.4byte	0			// PointerToLineNumbers
+	.2byte	0			// NumberOfRelocations
+	.2byte	0			// NumberOfLineNumbers
+	.4byte	0x42000040		// Characteristics (section flags)
+
+	.ascii	".data\0\0\0"
+	.4byte	_data_vsize - ImageBase			// VirtualSize
+	.4byte	_data - ImageBase			// VirtualAddress
+	.4byte	_data_size - ImageBase			// SizeOfRawData
+	.4byte	_data - ImageBase			// PointerToRawData
+	.4byte	0			// PointerToRelocations
+	.4byte	0			// PointerToLineNumbers
+	.2byte	0			// NumberOfRelocations
+	.2byte	0			// NumberOfLineNumbers
+	.4byte	0xC0000040		// Characteristics (section flags)
+
+	.ascii	".rodata\0"
+	.4byte	_rodata_vsize - ImageBase			// VirtualSize
+	.4byte	_rodata - ImageBase			// VirtualAddress
+	.4byte	_rodata_size - ImageBase			// SizeOfRawData
+	.4byte	_rodata - ImageBase			// PointerToRawData
+	.4byte	0			// PointerToRelocations
+	.4byte	0			// PointerToLineNumbers
+	.2byte	0			// NumberOfRelocations
+	.2byte	0			// NumberOfLineNumbers
+	.4byte	0x40000040		// Characteristics (section flags)
+
+	.text
+	.globl _start
+	.type _start,%function
+_start:
+	addi		sp, sp, -24
+	sd		a0, 0(sp)
+	sd		a1, 8(sp)
+	sd		ra, 16(sp)
+	lla		a0, ImageBase
+	lla		a1, _DYNAMIC
+	call		_relocate
+	bne		a0, zero, 0f
+	ld		a1, 8(sp)
+	ld		a0, 0(sp)
+	call		_entry
+	ld		ra, 16(sp)
+0:	addi		sp, sp, 24
+	ret
+
+// hand-craft a dummy .reloc section so EFI knows it's a relocatable executable:
+ 
+ 	.data
+dummy:	.4byte	0
+
+#define IMAGE_REL_ABSOLUTE	0
+ 	.section .reloc, "a"
+label1:
+	.4byte	dummy-label1				// Page RVA
+	.4byte	12					// Block Size (2*4+2*2), must be aligned by 32 Bits
+	.2byte	(IMAGE_REL_ABSOLUTE<<12) +  0		// reloc for dummy
+	.2byte	(IMAGE_REL_ABSOLUTE<<12) +  0		// reloc for dummy
+
+#if defined(__ELF__) && defined(__linux__)
+	.section .note.GNU-stack,"",%progbits
+#endif
diff --git a/riscv/efi/elf_riscv64_efi.lds b/riscv/efi/elf_riscv64_efi.lds
new file mode 100644
index 000000000000..ac7055a5619b
--- /dev/null
+++ b/riscv/efi/elf_riscv64_efi.lds
@@ -0,0 +1,142 @@
+/* SPDX-License-Identifier: GPL-2.0+ OR BSD-2-Clause */
+
+OUTPUT_FORMAT("elf64-littleriscv", "elf64-littleriscv", "elf64-littleriscv")
+OUTPUT_ARCH(riscv)
+ENTRY(_start)
+SECTIONS
+{
+  .text 0 : {
+    *(.text.head)
+    . = ALIGN(4096);
+    _text = .;
+    *(.text)
+    *(.text.*)
+    *(.gnu.linkonce.t.*)
+    *(.plt)
+    . = ALIGN(16);
+    _evtext = .;
+    . = ALIGN(4096);
+    _etext = .;
+  } =0
+  _text_vsize = _evtext - _text;
+  _text_size = _etext - _text;
+  . = ALIGN(4096);
+  _reloc = .;
+  .reloc : {
+    *(.reloc)
+    _evreloc = .;
+    . = ALIGN(4096);
+    _ereloc = .;
+  } =0
+  _reloc_vsize = _evreloc - _reloc;
+  _reloc_size = _ereloc - _reloc;
+  . = ALIGN(4096);
+  _data = .;
+  .dynamic  : { *(.dynamic) }
+  . = ALIGN(4096);
+  .data :
+  {
+   *(.sdata)
+   *(.data)
+   *(.data1)
+   *(.data.*)
+   *(.got.plt)
+   *(.got)
+
+   /*
+    * Note that these aren't the using the GNU "CONSTRUCTOR" output section
+    * command, so they don't start with a size.  Because of p2align and the
+    * end/END definitions, and the fact that they're mergeable, they can also
+    * have NULLs which aren't guaranteed to be at the end.
+    */
+   . = ALIGN(16);
+   __init_array_start = .;
+   *(SORT(.init_array.*))
+   *(.init_array)
+   __init_array_end = .;
+  . = ALIGN(16);
+   __CTOR_LIST__ = .;
+   *(SORT(.ctors.*))
+   *(.ctors)
+   __CTOR_END__ = .;
+  . = ALIGN(16);
+   __DTOR_LIST__ = .;
+   *(SORT(.dtors.*))
+   *(.dtors)
+   __DTOR_END__ = .;
+   . = ALIGN(16);
+   __fini_array_start = .;
+   *(SORT(.fini_array.*))
+   *(.fini_array)
+   __fini_array_end = .;
+
+   /* the EFI loader doesn't seem to like a .bss section, so we stick
+      it all into .data: */
+   . = ALIGN(16);
+   _bss = .;
+   *(.sbss)
+   *(.scommon)
+   *(.dynbss)
+   *(.bss)
+   *(.bss.*)
+   *(COMMON)
+   . = ALIGN(16);
+   _bss_end = .;
+   _evdata = .;
+   . = ALIGN(4096);
+   _edata = .;
+  } =0
+  _data_vsize = _evdata - _data;
+  _data_size = _edata - _data;
+
+  . = ALIGN(4096);
+  _rodata = .;
+  .rela :
+  {
+    *(.rela.text*)
+    *(.rela.data*)
+    *(.rela.got)
+    *(.rela.dyn)
+    *(.rela.stab)
+    *(.rela.init_array*)
+    *(.rela.fini_array*)
+    *(.rela.ctors*)
+    *(.rela.dtors*)
+
+  }
+  . = ALIGN(4096);
+  .rela.plt : { *(.rela.plt) }
+  . = ALIGN(4096);
+  .rodata : {
+    *(.rodata*)
+    _evrodata = .;
+    . = ALIGN(4096);
+    _erodata = .;
+  } =0
+  _rodata_vsize = _evrodata - _rodata;
+  _rodata_size = _erodata - _rodata;
+  _image_end = .;
+  _alldata_size = _image_end - _reloc;
+
+  . = ALIGN(4096);
+  .dynsym   : { *(.dynsym) }
+  . = ALIGN(4096);
+  .dynstr   : { *(.dynstr) }
+  . = ALIGN(4096);
+  .note.gnu.build-id : { *(.note.gnu.build-id) }
+  . = ALIGN(4096);
+  .hash : { *(.hash) }
+  . = ALIGN(4096);
+  .gnu.hash : { *(.gnu.hash) }
+  . = ALIGN(4096);
+  .eh_frame : { *(.eh_frame) }
+  . = ALIGN(4096);
+  .gcc_except_table : { *(.gcc_except_table*) }
+  /DISCARD/ :
+  {
+    *(.rela.reloc)
+    *(.note.GNU-stack)
+  }
+  .comment 0 : { *(.comment) }
+}
+
diff --git a/riscv/efi/reloc_riscv64.c b/riscv/efi/reloc_riscv64.c
new file mode 100644
index 000000000000..e4296026e2a4
--- /dev/null
+++ b/riscv/efi/reloc_riscv64.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
+/* reloc_riscv.c - position independent ELF shared object relocator
+   Copyright (C) 2018 Alexander Graf <agraf@suse.de>
+   Copyright (C) 2014 Linaro Ltd. <ard.biesheuvel@linaro.org>
+   Copyright (C) 1999 Hewlett-Packard Co.
+	Contributed by David Mosberger <davidm@hpl.hp.com>.
+
+    All rights reserved.
+
+    Redistribution and use in source and binary forms, with or without
+    modification, are permitted provided that the following conditions
+    are met:
+
+    * Redistributions of source code must retain the above copyright
+      notice, this list of conditions and the following disclaimer.
+    * Redistributions in binary form must reproduce the above
+      copyright notice, this list of conditions and the following
+      disclaimer in the documentation and/or other materials
+      provided with the distribution.
+    * Neither the name of Hewlett-Packard Co. nor the names of its
+      contributors may be used to endorse or promote products derived
+      from this software without specific prior written permission.
+
+    THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+    CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+    INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+    MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+    DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS
+    BE LIABLE FOR ANYDIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
+    OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+    PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+    PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+    THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
+    TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+    THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+    SUCH DAMAGE.
+*/
+
+#include <efi.h>
+
+#include <elf.h>
+
+#define Elf_Dyn		Elf64_Dyn
+#define Elf_Rela	Elf64_Rela
+#define ELF_R_TYPE	ELF64_R_TYPE
+
+EFI_STATUS EFIAPI _relocate(long ldbase, Elf_Dyn *dyn)
+{
+	long relsz = 0, relent = 0;
+	Elf_Rela *rel = NULL;
+	unsigned long *addr;
+	int i;
+
+	for (i = 0; dyn[i].d_tag != DT_NULL; ++i) {
+		switch (dyn[i].d_tag) {
+		case DT_RELA:
+			rel = (Elf_Rela *)((unsigned long)dyn[i].d_un.d_ptr + ldbase);
+			break;
+		case DT_RELASZ:
+			relsz = dyn[i].d_un.d_val;
+			break;
+		case DT_RELAENT:
+			relent = dyn[i].d_un.d_val;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!rel && relent == 0)
+		return EFI_SUCCESS;
+
+	if (!rel || relent == 0)
+		return EFI_LOAD_ERROR;
+
+	while (relsz > 0) {
+		/* apply the relocs */
+		switch (ELF_R_TYPE(rel->r_info)) {
+		case R_RISCV_RELATIVE:
+			addr = (unsigned long *)(ldbase + rel->r_offset);
+			*addr = ldbase + rel->r_addend;
+			break;
+		default:
+				break;
+		}
+		rel = (Elf_Rela *)((char *)rel + relent);
+		relsz -= relent;
+	}
+	return EFI_SUCCESS;
+}
-- 
2.43.0

[kvm-unit-tests PATCH 06/13] riscv: Tweak the gnu-efi imported code

[Andrew Jones]

Change _relocate to match the prototype in efi.h (it doesn't
matter that 'handle' and 'sys_tab' are unused). Also add
R_RISCV_RELATIVE to efi.h and replace '_entry' with 'efi_main'.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/elf.h                    | 5 +++++
 riscv/efi/crt0-efi-riscv64.S | 2 +-
 riscv/efi/reloc_riscv64.c    | 3 ++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/elf.h b/lib/elf.h
index 7a7db57774cd..fee20289e1fc 100644
--- a/lib/elf.h
+++ b/lib/elf.h
@@ -65,4 +65,9 @@ typedef struct elf64_rela {
 /* The following are used with relocations */
 #define ELF64_R_TYPE(i)		((i) & 0xffffffff)
 
+/*
+ * riscv static relocation types.
+ */
+#define R_RISCV_RELATIVE	3
+
 #endif /* _ELF_H_ */
diff --git a/riscv/efi/crt0-efi-riscv64.S b/riscv/efi/crt0-efi-riscv64.S
index 712ed03fc06e..cc8551a43c6a 100644
--- a/riscv/efi/crt0-efi-riscv64.S
+++ b/riscv/efi/crt0-efi-riscv64.S
@@ -164,7 +164,7 @@ _start:
 	bne		a0, zero, 0f
 	ld		a1, 8(sp)
 	ld		a0, 0(sp)
-	call		_entry
+	call		efi_main
 	ld		ra, 16(sp)
 0:	addi		sp, sp, 24
 	ret
diff --git a/riscv/efi/reloc_riscv64.c b/riscv/efi/reloc_riscv64.c
index e4296026e2a4..8504ad595e51 100644
--- a/riscv/efi/reloc_riscv64.c
+++ b/riscv/efi/reloc_riscv64.c
@@ -44,7 +44,8 @@
 #define Elf_Rela	Elf64_Rela
 #define ELF_R_TYPE	ELF64_R_TYPE
 
-EFI_STATUS EFIAPI _relocate(long ldbase, Elf_Dyn *dyn)
+efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t handle,
+		       efi_system_table_t *sys_tab)
 {
 	long relsz = 0, relent = 0;
 	Elf_Rela *rel = NULL;
-- 
2.43.0

[kvm-unit-tests PATCH 07/13] riscv: Enable building for EFI

[Andrew Jones]

Mimicking arm64 support, add configure and makefile changes to build
for EFI. Since the linker script is replaced also replace the initial
cstart code (also done like arm64). Finally, provide a stub for
setup_efi() in order to allow compiling to complete (even though
tests can't yet run).

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 configure             |  3 ++-
 lib/riscv/asm/setup.h |  5 +++++
 riscv/Makefile        | 24 +++++++++++++++++++++++-
 riscv/cstart.S        |  4 ++++
 4 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 283c959973fd..cb1718ce12e6 100755
--- a/configure
+++ b/configure
@@ -230,7 +230,8 @@ else
     fi
 fi
 
-if [ "$efi" ] && [ "$arch" != "x86_64" ] && [ "$arch" != "arm64" ]; then
+if [ "$efi" ] && [ "$arch" != "x86_64" ] &&
+   [ "$arch" != "arm64" ] && [ "$arch" != "riscv64" ]; then
     echo "--[enable|disable]-efi is not supported for $arch"
     usage
 fi
diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
index e58dd53071ae..dfc8875fbb3b 100644
--- a/lib/riscv/asm/setup.h
+++ b/lib/riscv/asm/setup.h
@@ -12,4 +12,9 @@ int hartid_to_cpu(unsigned long hartid);
 void io_init(void);
 void setup(const void *fdt, phys_addr_t freemem_start);
 
+#ifdef CONFIG_EFI
+#include <efi.h>
+static inline efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) { return 0; }
+#endif
+
 #endif /* _ASMRISCV_SETUP_H_ */
diff --git a/riscv/Makefile b/riscv/Makefile
index 85bbca151739..e1803c133e71 100644
--- a/riscv/Makefile
+++ b/riscv/Makefile
@@ -85,7 +85,29 @@ include $(SRCDIR)/scripts/asm-offsets.mak
 		-DPROGNAME=\"$(notdir $(@:.aux.o=.$(exe)))\" -DAUXFLAGS=$(AUXFLAGS)
 
 ifeq ($(CONFIG_EFI),y)
-	# TODO
+# avoid jump tables before all relocations have been processed
+riscv/efi/reloc_riscv64.o: CFLAGS += -fno-jump-tables
+cflatobjs += riscv/efi/reloc_riscv64.o
+cflatobjs += lib/acpi.o
+cflatobjs += lib/efi.o
+
+#.PRECIOUS: %.so
+
+%.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
+%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o
+	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \
+		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
+
+%.efi: %.so
+	$(call arch_elf_check, $^)
+	$(OBJCOPY) --only-keep-debug $^ $@.debug
+	$(OBJCOPY) --strip-debug $^
+	$(OBJCOPY) --add-gnu-debuglink=$@.debug $^
+	$(OBJCOPY) \
+		-j .text -j .sdata -j .data -j .rodata -j .dynamic -j .dynsym \
+		-j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \
+		-j .reloc \
+		-O binary $^ $@
 else
 %.elf: LDFLAGS += -pie -n -z notext
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o
diff --git a/riscv/cstart.S b/riscv/cstart.S
index c935467ff6a1..10b5da5779b0 100644
--- a/riscv/cstart.S
+++ b/riscv/cstart.S
@@ -42,6 +42,9 @@
 9997:
 .endm
 
+#ifdef CONFIG_EFI
+#include "efi/crt0-efi-riscv64.S"
+#else
 	.section .init
 
 /*
@@ -109,6 +112,7 @@ start:
 	call	exit
 	j	halt
 
+#endif /* !CONFIG_EFI */
 	.text
 
 .balign 4
-- 
2.43.0

[kvm-unit-tests PATCH 08/13] riscv: efi: Switch stack in _start

[Andrew Jones]

Modify gnu-efi's _start to switch the stack. This allows us to not
map memory regions which have EFI memory type EFI_BOOT_SERVICES_DATA,
as the stack will be in the EFI_LOADER_CODE region instead. We'll
still map the stack as R/W instead of R/X because we'll split the
EFI_LOADER_CODE region on the _etext boundary and map addresses
before _etext as R/X and the rest as R/W.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/efi/crt0-efi-riscv64.S | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/riscv/efi/crt0-efi-riscv64.S b/riscv/efi/crt0-efi-riscv64.S
index cc8551a43c6a..4ed82b14a1d6 100644
--- a/riscv/efi/crt0-efi-riscv64.S
+++ b/riscv/efi/crt0-efi-riscv64.S
@@ -164,7 +164,20 @@ _start:
 	bne		a0, zero, 0f
 	ld		a1, 8(sp)
 	ld		a0, 0(sp)
+
+	/* Switch to our own stack */
+	mv		a2, sp
+	la		sp, stacktop
+	mv		fp, zero
+	push_fp zero
+	addi		sp, sp, -16
+	sd		a2, 0(sp)
+
 	call		efi_main
+
+	/* Restore sp */
+	ld		sp, 0(sp)
+
 	ld		ra, 16(sp)
 0:	addi		sp, sp, 24
 	ret
@@ -172,6 +185,11 @@ _start:
 // hand-craft a dummy .reloc section so EFI knows it's a relocatable executable:
  
  	.data
+
+.balign 16384
+.space 16384
+stacktop:
+
 dummy:	.4byte	0
 
 #define IMAGE_REL_ABSOLUTE	0
-- 
2.43.0

[kvm-unit-tests PATCH 09/13] efi: Add support for obtaining the boot hartid

[Andrew Jones]

riscv needs to use an EFI protocol to get the boot hartid.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/efi.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/efi.c b/lib/efi.c
index edfcc80ef114..77cfbfac50ed 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -29,6 +29,31 @@ extern int main(int argc, char **argv, char **envp);
 
 efi_system_table_t *efi_system_table = NULL;
 
+#ifdef __riscv
+#define RISCV_EFI_BOOT_PROTOCOL_GUID EFI_GUID(0xccd15fec, 0x6f73, 0x4eec,  0x83, 0x95, 0x3e, 0x69, 0xe4, 0xb9, 0x40, 0xbf)
+
+unsigned long boot_hartid;
+
+struct riscv_efi_boot_protocol {
+	u64 revision;
+	efi_status_t (*get_boot_hartid)(struct riscv_efi_boot_protocol *,
+		      unsigned long *boot_hartid);
+};
+
+static efi_status_t efi_get_boot_hartid(void)
+{
+	efi_guid_t boot_protocol_guid = RISCV_EFI_BOOT_PROTOCOL_GUID;
+	struct riscv_efi_boot_protocol *boot_protocol;
+	efi_status_t status;
+
+	status = efi_bs_call(locate_protocol, &boot_protocol_guid, NULL,
+			     (void **)&boot_protocol);
+	if (status != EFI_SUCCESS)
+		return status;
+	return efi_call_proto(boot_protocol, get_boot_hartid, &boot_hartid);
+}
+#endif
+
 static void efi_free_pool(void *ptr)
 {
 	efi_bs_call(free_pool, ptr);
@@ -422,6 +447,14 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
 		goto efi_main_error;
 	}
 
+#ifdef __riscv
+	status = efi_get_boot_hartid();
+	if (status != EFI_SUCCESS) {
+		printf("Failed to get boot haritd\n");
+		goto efi_main_error;
+	}
+#endif
+
 	/* 
 	 * Exit EFI boot services, let kvm-unit-tests take full control of the
 	 * guest
-- 
2.43.0

[kvm-unit-tests PATCH 10/13] riscv: Refactor setup code

[Andrew Jones]

To prepare for EFI setup, move code that will be shared into
functions. This is the same type of code and the exact same function
names which were created when refactoring Arm's EFI setup, so riscv
setup is still following Arm's setup patterns.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/setup.c | 109 +++++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 41 deletions(-)

diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
index 40ff26a24cfc..f721d81192ac 100644
--- a/lib/riscv/setup.c
+++ b/lib/riscv/setup.c
@@ -31,6 +31,8 @@
 #define MAX_DT_MEM_REGIONS	16
 #define NR_MEM_REGIONS		(MAX_DT_MEM_REGIONS + 16)
 
+extern unsigned long _etext;
+
 char *initrd;
 u32 initrd_size;
 
@@ -81,25 +83,12 @@ static void cpu_init(void)
 	cpu0_calls_idle = true;
 }
 
-extern unsigned long _etext;
-
-static void mem_init(phys_addr_t freemem_start)
+static void mem_allocator_init(phys_addr_t freemem_start, phys_addr_t freemem_end)
 {
-	struct mem_region *freemem, *code, *data;
-	phys_addr_t freemem_end, base, top;
-
-	memregions_init(riscv_mem_regions, NR_MEM_REGIONS);
-	memregions_add_dt_regions(MAX_DT_MEM_REGIONS);
+	phys_addr_t base, top;
 
-	/* Split the region with the code into two regions; code and data */
-	memregions_split((unsigned long)&_etext, &code, &data);
-	assert(code);
-	code->flags |= MR_F_CODE;
-
-	freemem = memregions_find(freemem_start);
-	assert(freemem && !(freemem->flags & (MR_F_IO | MR_F_CODE)));
-
-	freemem_end = freemem->end & PAGE_MASK;
+	freemem_start = PAGE_ALIGN(freemem_start);
+	freemem_end &= PAGE_MASK;
 
 	/*
 	 * The assert below is mostly checking that the free memory doesn't
@@ -129,6 +118,64 @@ static void mem_init(phys_addr_t freemem_start)
 	page_alloc_ops_enable();
 }
 
+static void mem_init(phys_addr_t freemem_start)
+{
+	struct mem_region *freemem, *code, *data;
+
+	memregions_init(riscv_mem_regions, NR_MEM_REGIONS);
+	memregions_add_dt_regions(MAX_DT_MEM_REGIONS);
+
+	/* Split the region with the code into two regions; code and data */
+	memregions_split((unsigned long)&_etext, &code, &data);
+	assert(code);
+	code->flags |= MR_F_CODE;
+
+	freemem = memregions_find(freemem_start);
+	assert(freemem && !(freemem->flags & (MR_F_IO | MR_F_CODE)));
+
+	mem_allocator_init(freemem_start, freemem->end);
+}
+
+static void freemem_push_fdt(void **freemem, const void *fdt)
+{
+	u32 fdt_size;
+	int ret;
+
+	fdt_size = fdt_totalsize(fdt);
+	ret = fdt_move(fdt, *freemem, fdt_size);
+	assert(ret == 0);
+	ret = dt_init(*freemem);
+	assert(ret == 0);
+	*freemem += fdt_size;
+}
+
+static void freemem_push_dt_initrd(void **freemem)
+{
+	const char *tmp;
+	int ret;
+
+	ret = dt_get_initrd(&tmp, &initrd_size);
+	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
+	if (ret == 0) {
+		initrd = *freemem;
+		memmove(initrd, tmp, initrd_size);
+		*freemem += initrd_size;
+	}
+}
+
+static void initrd_setup(void)
+{
+	char *env;
+
+	if (!initrd)
+		return;
+
+	/* environ is currently the only file in the initrd */
+	env = malloc(initrd_size);
+	memcpy(env, initrd, initrd_size);
+	setup_env(env, initrd_size);
+}
+
 static void banner(void)
 {
 	puts("\n");
@@ -141,29 +188,14 @@ static void banner(void)
 void setup(const void *fdt, phys_addr_t freemem_start)
 {
 	void *freemem;
-	const char *bootargs, *tmp;
-	u32 fdt_size;
+	const char *bootargs;
 	int ret;
 
 	assert(sizeof(long) == 8 || freemem_start < VA_BASE);
 	freemem = __va(freemem_start);
 
-	/* Move the FDT to the base of free memory */
-	fdt_size = fdt_totalsize(fdt);
-	ret = fdt_move(fdt, freemem, fdt_size);
-	assert(ret == 0);
-	ret = dt_init(freemem);
-	assert(ret == 0);
-	freemem += fdt_size;
-
-	/* Move the initrd to the top of the FDT */
-	ret = dt_get_initrd(&tmp, &initrd_size);
-	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
-	if (ret == 0) {
-		initrd = freemem;
-		memmove(initrd, tmp, initrd_size);
-		freemem += initrd_size;
-	}
+	freemem_push_fdt(&freemem, fdt);
+	freemem_push_dt_initrd(&freemem);
 
 	mem_init(PAGE_ALIGN(__pa(freemem)));
 	cpu_init();
@@ -174,12 +206,7 @@ void setup(const void *fdt, phys_addr_t freemem_start)
 	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
 	setup_args_progname(bootargs);
 
-	if (initrd) {
-		/* environ is currently the only file in the initrd */
-		char *env = malloc(initrd_size);
-		memcpy(env, initrd, initrd_size);
-		setup_env(env, initrd_size);
-	}
+	initrd_setup();
 
 	if (!(auxinfo.flags & AUXINFO_MMU_OFF))
 		setup_vm();
-- 
2.43.0

[kvm-unit-tests PATCH 11/13] riscv: Enable EFI boot

[Andrew Jones]

Mimicking Arm's setup_efi() and duplicating some code from riscv's
setup(), add the EFI setup code needed to boot unit tests from EFI-
capable bootloaders. The selftest unit test can now be run with

  qemu-system-riscv64 \
    -nodefaults -nographic -serial mon:stdio \
    -accel tcg -cpu max \
    -machine virt,pflash0=pflash0 \
    -blockdev node-name=pflash0,driver=file,read-only=on,filename=RISCV_VIRT_CODE.fd \
    -smp 16 \
    -kernel riscv/selftest.efi \
    -initrd test-env \
    -append 'selftest.efi foo bar baz' \
    -machine acpi=off

where test-env has the environment variables
  $ cat test-env
  FOO=foo
  BAR=bar
  BAZ=baz

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/setup.h |  2 +-
 lib/riscv/setup.c     | 63 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
index dfc8875fbb3b..7f81a705ca4f 100644
--- a/lib/riscv/asm/setup.h
+++ b/lib/riscv/asm/setup.h
@@ -14,7 +14,7 @@ void setup(const void *fdt, phys_addr_t freemem_start);
 
 #ifdef CONFIG_EFI
 #include <efi.h>
-static inline efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) { return 0; }
+efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo);
 #endif
 
 #endif /* _ASMRISCV_SETUP_H_ */
diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
index f721d81192ac..7c681ea3a13c 100644
--- a/lib/riscv/setup.c
+++ b/lib/riscv/setup.c
@@ -213,3 +213,66 @@ void setup(const void *fdt, phys_addr_t freemem_start)
 
 	banner();
 }
+
+#ifdef CONFIG_EFI
+#include <efi.h>
+
+extern unsigned long exception_vectors;
+extern unsigned long boot_hartid;
+
+static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
+{
+	struct mem_region *freemem_mr = NULL, *code, *data;
+	void *freemem;
+
+	memregions_init(riscv_mem_regions, NR_MEM_REGIONS);
+
+	memregions_efi_init(&efi_bootinfo->mem_map, &freemem_mr);
+	if (!freemem_mr)
+		return EFI_OUT_OF_RESOURCES;
+
+	memregions_split((unsigned long)&_etext, &code, &data);
+	assert(code && (code->flags & MR_F_CODE));
+	if (data)
+		data->flags &= ~MR_F_CODE;
+
+	for (struct mem_region *m = mem_regions; m->end; ++m)
+		assert(m == code || !(m->flags & MR_F_CODE));
+
+	freemem = (void *)PAGE_ALIGN(freemem_mr->start);
+
+	if (efi_bootinfo->fdt_valid)
+		freemem_push_fdt(&freemem, efi_bootinfo->fdt);
+
+	mmu_disable();
+	mem_allocator_init((unsigned long)freemem, freemem_mr->end);
+
+	return EFI_SUCCESS;
+}
+
+efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
+{
+	efi_status_t status;
+
+	csr_write(CSR_STVEC, (unsigned long)&exception_vectors);
+	csr_write(CSR_SSCRATCH, boot_hartid);
+
+	status = efi_mem_init(efi_bootinfo);
+	if (status != EFI_SUCCESS) {
+		printf("Failed to initialize memory\n");
+		return status;
+	}
+
+	cpu_init();
+	thread_info_init();
+	io_init();
+	initrd_setup();
+
+	if (!(auxinfo.flags & AUXINFO_MMU_OFF))
+		setup_vm();
+
+	banner();
+
+	return EFI_SUCCESS;
+}
+#endif /* CONFIG_EFI */
-- 
2.43.0

[kvm-unit-tests PATCH 12/13] riscv: efi: Add run script

[Andrew Jones]

Adapt Arm's efi run script to riscv. We can now run efi tests with
run_tests.sh.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 configure     |   2 +-
 riscv/efi/run | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++
 riscv/run     |   2 +-
 3 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100755 riscv/efi/run

diff --git a/configure b/configure
index cb1718ce12e6..94b243fd1b26 100755
--- a/configure
+++ b/configure
@@ -94,7 +94,7 @@ usage() {
 	                           Select whether to run EFI tests directly with QEMU's -kernel
 	                           option. When not enabled, tests will be placed in an EFI file
 	                           system and run from the UEFI shell. Ignored when efi isn't enabled.
-	                           (arm64 only)
+	                           (arm64 and riscv64 only)
 EOF
     exit 1
 }
diff --git a/riscv/efi/run b/riscv/efi/run
new file mode 100755
index 000000000000..982b8b9c455a
--- /dev/null
+++ b/riscv/efi/run
@@ -0,0 +1,106 @@
+#!/bin/bash
+
+if [ $# -eq 0 ]; then
+	echo "Usage $0 TEST_CASE [QEMU_ARGS]"
+	exit 2
+fi
+
+if [ ! -f config.mak ]; then
+	echo "run './configure --enable-efi && make' first. See ./configure -h"
+	exit 2
+fi
+source config.mak
+source scripts/arch-run.bash
+
+if [ -f RISCV_VIRT_CODE.fd ]; then
+	DEFAULT_UEFI=RISCV_VIRT_CODE.fd
+fi
+
+KERNEL_NAME=$1
+
+: "${EFI_SRC:=$TEST_DIR}"
+: "${EFI_UEFI:=$DEFAULT_UEFI}"
+: "${EFI_TEST:=efi-tests}"
+: "${EFI_CASE:=$(basename $KERNEL_NAME .efi)}"
+: "${EFI_TESTNAME:=$TESTNAME}"
+: "${EFI_TESTNAME:=$EFI_CASE}"
+: "${EFI_CASE_DIR:="$EFI_TEST/$EFI_TESTNAME"}"
+: "${EFI_VAR_GUID:=97ef3e03-7329-4a6a-b9ba-6c1fdcc5f823}"
+
+if [ ! -f "$EFI_UEFI" ]; then
+	echo "UEFI firmware not found."
+	echo "Please specify the path with the env variable EFI_UEFI"
+	exit 2
+fi
+
+if [ "$EFI_USE_ACPI" = "y" ]; then
+	echo "ACPI not available"
+	exit 2
+fi
+
+# Remove the TEST_CASE from $@
+shift 1
+
+# Fish out the arguments for the test, they should be the next string
+# after the "-append" option
+qemu_args=()
+cmd_args=()
+while (( "$#" )); do
+	if [ "$1" = "-append" ]; then
+		cmd_args=$2
+		shift 2
+	else
+		qemu_args+=("$1")
+		shift 1
+	fi
+done
+
+if [ "$EFI_CASE" = "_NO_FILE_4Uhere_" ]; then
+	EFI_CASE_DIR="$EFI_TEST/dummy"
+	mkdir -p "$EFI_CASE_DIR"
+	$TEST_DIR/run \
+		$EFI_CASE \
+		-machine pflash0=pflash0 \
+		-blockdev node-name=pflash0,driver=file,read-only=on,filename="$EFI_UEFI" \
+		-drive file.dir="$EFI_CASE_DIR/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
+		"${qemu_args[@]}"
+	exit
+fi
+
+uefi_shell_run()
+{
+	mkdir -p "$EFI_CASE_DIR"
+	cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_CASE_DIR/"
+	echo "@echo -off" > "$EFI_CASE_DIR/startup.nsh"
+	if [ "$EFI_USE_ACPI" != "y" ]; then
+		qemu_args+=(-machine acpi=off)
+		FDT_BASENAME="dtb"
+		UEFI_SHELL_RUN=y $TEST_DIR/run \
+			-machine pflash0=pflash0 \
+			-blockdev node-name=pflash0,driver=file,read-only=on,filename="$EFI_UEFI" \
+			-machine dumpdtb="$EFI_CASE_DIR/$FDT_BASENAME" \
+			"${qemu_args[@]}"
+		echo "setvar fdtfile -guid $EFI_VAR_GUID -rt =L\"$FDT_BASENAME\""  >> "$EFI_CASE_DIR/startup.nsh"
+	fi
+	echo "$EFI_CASE.efi" "${cmd_args[@]}" >> "$EFI_CASE_DIR/startup.nsh"
+
+	UEFI_SHELL_RUN=y $TEST_DIR/run \
+		-machine pflash0=pflash0 \
+		-blockdev node-name=pflash0,driver=file,read-only=on,filename="$EFI_UEFI" \
+		-drive file.dir="$EFI_CASE_DIR/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
+		"${qemu_args[@]}"
+}
+
+if [ "$EFI_DIRECT" = "y" ]; then
+	if [ "$EFI_USE_ACPI" != "y" ]; then
+		qemu_args+=(-machine acpi=off)
+	fi
+	$TEST_DIR/run \
+		$KERNEL_NAME \
+		-append "$(basename $KERNEL_NAME) ${cmd_args[@]}" \
+		-machine pflash0=pflash0 \
+		-blockdev node-name=pflash0,driver=file,read-only=on,filename="$EFI_UEFI" \
+		"${qemu_args[@]}"
+else
+	uefi_shell_run
+fi
diff --git a/riscv/run b/riscv/run
index cbe5dd792dcd..73f2bf54dc32 100755
--- a/riscv/run
+++ b/riscv/run
@@ -33,7 +33,7 @@ command="$qemu -nodefaults -nographic -serial mon:stdio"
 command+=" $mach $acc $firmware -cpu $processor "
 command="$(migration_cmd) $(timeout_cmd) $command"
 
-if [ "$EFI_RUN" = "y" ]; then
+if [ "$UEFI_SHELL_RUN" = "y" ]; then
 	ENVIRON_DEFAULT=n run_qemu_status $command "$@"
 else
 	# We return the exit code via stdout, not via the QEMU return code
-- 
2.43.0

[kvm-unit-tests PATCH 13/13] riscv: efi: Use efi-direct by default

[Andrew Jones]

efi-direct is much faster and riscv makes a lot of use of environment
variables, which can't be as easily used without '-initrd'. When efi
is configured for riscv, assume efi-direct is also desired. Users
can switch to running from the UEFI shell by configuring with
--disable-efi-direct.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 configure | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 94b243fd1b26..10728773eb3b 100755
--- a/configure
+++ b/configure
@@ -93,7 +93,8 @@ usage() {
 	    --[enable|disable]-efi-direct
 	                           Select whether to run EFI tests directly with QEMU's -kernel
 	                           option. When not enabled, tests will be placed in an EFI file
-	                           system and run from the UEFI shell. Ignored when efi isn't enabled.
+	                           system and run from the UEFI shell. Ignored when efi isn't enabled
+	                           and defaults to enabled when efi is enabled for riscv64.
 	                           (arm64 and riscv64 only)
 EOF
     exit 1
@@ -236,6 +237,10 @@ if [ "$efi" ] && [ "$arch" != "x86_64" ] &&
     usage
 fi
 
+if [ "$efi" ] && [ "$arch" = "riscv64" ] && [ -z "$efi_direct" ]; then
+    efi_direct=y
+fi
+
 if [ -z "$page_size" ]; then
     if [ "$efi" = 'y' ] && [ "$arch" = "arm64" ]; then
         page_size="4096"
-- 
2.43.0

Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace

[Claudio Imbrenda]

On Wed, 28 Feb 2024 16:04:19 +0100
Andrew Jones <andrew.jones@linux.dev> wrote:

> We should never pass the result of __builtin_frame_address(0) to
> another function since the compiler is within its rights to pop the
> frame to which it points before making the function call, as may be
> done for tail calls. Nobody has complained about backtrace(), so
> likely all compilations have been inlining backtrace_frame(), not
> dropping the frame on the tail call, or nobody is looking at traces.
> However, for riscv, when built for EFI, it does drop the frame on the
> tail call, and it was noticed. Preemptively fix backtrace() for all
> architectures.
> 
> Fixes: 52266791750d ("lib: backtrace printing")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/arm/stack.c   | 13 +++++--------
>  lib/arm64/stack.c | 12 +++++-------
>  lib/riscv/stack.c | 12 +++++-------
>  lib/s390x/stack.c | 12 +++++-------
>  lib/stack.h       | 24 +++++++++++++++++-------
>  lib/x86/stack.c   | 12 +++++-------
>  6 files changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/lib/arm/stack.c b/lib/arm/stack.c
> index 7d081be7c6d0..66d18b47ea53 100644
> --- a/lib/arm/stack.c
> +++ b/lib/arm/stack.c
> @@ -8,13 +8,16 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs,
> -		    int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	static int walking;
>  	int depth;
>  	const unsigned long *fp = (unsigned long *)frame;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs,
>  	walking = 0;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> index 82611f4b1815..f5eb57fd8892 100644
> --- a/lib/arm64/stack.c
> +++ b/lib/arm64/stack.c
> @@ -8,7 +8,8 @@
>  
>  extern char vector_stub_start, vector_stub_end;
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	const void *fp = frame;
>  	static bool walking;
> @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	bool is_exception = false;
>  	unsigned long addr;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	walking = false;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
> index 712a5478d547..d865594b9671 100644
> --- a/lib/riscv/stack.c
> +++ b/lib/riscv/stack.c
> @@ -2,12 +2,16 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	static bool walking;
>  	const unsigned long *fp = (unsigned long *)frame;
>  	int depth;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	walking = false;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
> index 9f234a12adf6..d194f654e94d 100644
> --- a/lib/s390x/stack.c
> +++ b/lib/s390x/stack.c
> @@ -14,11 +14,15 @@
>  #include <stack.h>
>  #include <asm/arch_def.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	int depth = 0;
>  	struct stack_frame *stack = (struct stack_frame *)frame;
>  
> +	if (current_frame)
> +		stack = __builtin_frame_address(0);
> +
>  	for (depth = 0; stack && depth < max_depth; depth++) {
>  		return_addrs[depth] = (void *)stack->grs[8];
>  		stack = stack->back_chain;
> @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/stack.h b/lib/stack.h
> index 10fc2f793354..6edc84344b51 100644
> --- a/lib/stack.h
> +++ b/lib/stack.h
> @@ -11,17 +11,27 @@
>  #include <asm/stack.h>
>  
>  #ifdef HAVE_ARCH_BACKTRACE_FRAME
> -extern int backtrace_frame(const void *frame, const void **return_addrs,
> -			   int max_depth);
> +extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +				int max_depth, bool current_frame);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> +				  int max_depth)
> +{
> +	return arch_backtrace_frame(frame, return_addrs, max_depth, false);
> +}
> +
> +static inline int backtrace(const void **return_addrs, int max_depth)
> +{
> +	return arch_backtrace_frame(NULL, return_addrs, max_depth, true);
> +}
>  #else
> -static inline int
> -backtrace_frame(const void *frame __unused, const void **return_addrs __unused,
> -		int max_depth __unused)
> +extern int backtrace(const void **return_addrs, int max_depth);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> +				  int max_depth)
>  {
>  	return 0;
>  }
>  #endif
>  
> -extern int backtrace(const void **return_addrs, int max_depth);
> -
>  #endif
> diff --git a/lib/x86/stack.c b/lib/x86/stack.c
> index 5ecd97ce90b9..58ab6c4b293a 100644
> --- a/lib/x86/stack.c
> +++ b/lib/x86/stack.c
> @@ -1,12 +1,16 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	static int walking;
>  	int depth = 0;
>  	const unsigned long *bp = (unsigned long *) frame;
>  
> +	if (current_frame)
> +		bp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	walking = 0;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0), return_addrs,
> -			       max_depth);
> -}

Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace

[Nicholas Piggin]

On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote:
> We should never pass the result of __builtin_frame_address(0) to
> another function since the compiler is within its rights to pop the
> frame to which it points before making the function call, as may be
> done for tail calls. Nobody has complained about backtrace(), so
> likely all compilations have been inlining backtrace_frame(), not
> dropping the frame on the tail call, or nobody is looking at traces.
> However, for riscv, when built for EFI, it does drop the frame on the
> tail call, and it was noticed. Preemptively fix backtrace() for all
> architectures.
>
> Fixes: 52266791750d ("lib: backtrace printing")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/arm/stack.c   | 13 +++++--------
>  lib/arm64/stack.c | 12 +++++-------
>  lib/riscv/stack.c | 12 +++++-------
>  lib/s390x/stack.c | 12 +++++-------
>  lib/stack.h       | 24 +++++++++++++++++-------
>  lib/x86/stack.c   | 12 +++++-------
>  6 files changed, 42 insertions(+), 43 deletions(-)
>
> diff --git a/lib/arm/stack.c b/lib/arm/stack.c
> index 7d081be7c6d0..66d18b47ea53 100644
> --- a/lib/arm/stack.c
> +++ b/lib/arm/stack.c
> @@ -8,13 +8,16 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs,
> -		    int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	static int walking;
>  	int depth;
>  	const unsigned long *fp = (unsigned long *)frame;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs,
>  	walking = 0;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> index 82611f4b1815..f5eb57fd8892 100644
> --- a/lib/arm64/stack.c
> +++ b/lib/arm64/stack.c
> @@ -8,7 +8,8 @@
>  
>  extern char vector_stub_start, vector_stub_end;
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	const void *fp = frame;
>  	static bool walking;
> @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	bool is_exception = false;
>  	unsigned long addr;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	walking = false;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
> index 712a5478d547..d865594b9671 100644
> --- a/lib/riscv/stack.c
> +++ b/lib/riscv/stack.c
> @@ -2,12 +2,16 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	static bool walking;
>  	const unsigned long *fp = (unsigned long *)frame;
>  	int depth;
>  
> +	if (current_frame)
> +		fp = __builtin_frame_address(0);
> +
>  	if (walking) {
>  		printf("RECURSIVE STACK WALK!!!\n");
>  		return 0;
> @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	walking = false;
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
> index 9f234a12adf6..d194f654e94d 100644
> --- a/lib/s390x/stack.c
> +++ b/lib/s390x/stack.c
> @@ -14,11 +14,15 @@
>  #include <stack.h>
>  #include <asm/arch_def.h>
>  
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +			 int max_depth, bool current_frame)
>  {
>  	int depth = 0;
>  	struct stack_frame *stack = (struct stack_frame *)frame;
>  
> +	if (current_frame)
> +		stack = __builtin_frame_address(0);
> +
>  	for (depth = 0; stack && depth < max_depth; depth++) {
>  		return_addrs[depth] = (void *)stack->grs[8];
>  		stack = stack->back_chain;
> @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  
>  	return depth;
>  }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> -	return backtrace_frame(__builtin_frame_address(0),
> -			       return_addrs, max_depth);
> -}
> diff --git a/lib/stack.h b/lib/stack.h
> index 10fc2f793354..6edc84344b51 100644
> --- a/lib/stack.h
> +++ b/lib/stack.h
> @@ -11,17 +11,27 @@
>  #include <asm/stack.h>
>  
>  #ifdef HAVE_ARCH_BACKTRACE_FRAME
> -extern int backtrace_frame(const void *frame, const void **return_addrs,
> -			   int max_depth);
> +extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
> +				int max_depth, bool current_frame);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> +				  int max_depth)
> +{
> +	return arch_backtrace_frame(frame, return_addrs, max_depth, false);
> +}
> +
> +static inline int backtrace(const void **return_addrs, int max_depth)
> +{
> +	return arch_backtrace_frame(NULL, return_addrs, max_depth, true);
> +}
>  #else
> -static inline int
> -backtrace_frame(const void *frame __unused, const void **return_addrs __unused,
> -		int max_depth __unused)
> +extern int backtrace(const void **return_addrs, int max_depth);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> +				  int max_depth)
>  {
>  	return 0;
>  }
>  #endif
>  
> -extern int backtrace(const void **return_addrs, int max_depth);
> -
>  #endif

Is there a reason to add the inline wrappers rather than just externs
and drop the arch_ prefix?

Do we want to just generally have all arch specific functions have an
arch_ prefix? Fine by me.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

I'm fine to rebase the powerpc patch on top of this if it goes in first.
Thanks for the heads up.

Thanks,
Nick

Re: [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific

[Nicholas Piggin]

On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote:
> Calculating the offset of an address is image specific, which is
> architecture specific. Until now, all architectures and architecture
> configurations which select CONFIG_RELOC were able to subtract
> _etext, but the EFI configuration of riscv cannot (it must subtract
> ImageBase). Make this function architecture specific, since the
> architecture's image layout already is.

arch_base_address()?

How about a default implementation unlesss HAVE_ARCH_BASE_ADDRESS?

Thanks,
Nick

>
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/arm64/stack.c | 17 +++++++++++++++++
>  lib/riscv/stack.c | 18 ++++++++++++++++++
>  lib/stack.c       | 19 ++-----------------
>  lib/stack.h       |  2 ++
>  lib/x86/stack.c   | 17 +++++++++++++++++
>  5 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> index f5eb57fd8892..3369031a74f7 100644
> --- a/lib/arm64/stack.c
> +++ b/lib/arm64/stack.c
> @@ -6,6 +6,23 @@
>  #include <stdbool.h>
>  #include <stack.h>
>  
> +#ifdef CONFIG_RELOC
> +extern char _text, _etext;
> +
> +bool base_address(const void *rebased_addr, unsigned long *addr)
> +{
> +	unsigned long ra = (unsigned long)rebased_addr;
> +	unsigned long start = (unsigned long)&_text;
> +	unsigned long end = (unsigned long)&_etext;
> +
> +	if (ra < start || ra >= end)
> +		return false;
> +
> +	*addr = ra - start;
> +	return true;
> +}
> +#endif
> +
>  extern char vector_stub_start, vector_stub_end;
>  
>  int arch_backtrace_frame(const void *frame, const void **return_addrs,
> diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
> index d865594b9671..a143c22a570a 100644
> --- a/lib/riscv/stack.c
> +++ b/lib/riscv/stack.c
> @@ -2,6 +2,24 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> +#ifdef CONFIG_RELOC
> +extern char ImageBase, _text, _etext;
> +
> +bool base_address(const void *rebased_addr, unsigned long *addr)
> +{
> +	unsigned long ra = (unsigned long)rebased_addr;
> +	unsigned long base = (unsigned long)&ImageBase;
> +	unsigned long start = (unsigned long)&_text;
> +	unsigned long end = (unsigned long)&_etext;
> +
> +	if (ra < start || ra >= end)
> +		return false;
> +
> +	*addr = ra - base;
> +	return true;
> +}
> +#endif
> +
>  int arch_backtrace_frame(const void *frame, const void **return_addrs,
>  			 int max_depth, bool current_frame)
>  {
> diff --git a/lib/stack.c b/lib/stack.c
> index dd6bfa8dac6e..e5099e207388 100644
> --- a/lib/stack.c
> +++ b/lib/stack.c
> @@ -11,23 +11,8 @@
>  
>  #define MAX_DEPTH 20
>  
> -#ifdef CONFIG_RELOC
> -extern char _text, _etext;
> -
> -static bool base_address(const void *rebased_addr, unsigned long *addr)
> -{
> -	unsigned long ra = (unsigned long)rebased_addr;
> -	unsigned long start = (unsigned long)&_text;
> -	unsigned long end = (unsigned long)&_etext;
> -
> -	if (ra < start || ra >= end)
> -		return false;
> -
> -	*addr = ra - start;
> -	return true;
> -}
> -#else
> -static bool base_address(const void *rebased_addr, unsigned long *addr)
> +#ifndef CONFIG_RELOC
> +bool base_address(const void *rebased_addr, unsigned long *addr)
>  {
>  	*addr = (unsigned long)rebased_addr;
>  	return true;
> diff --git a/lib/stack.h b/lib/stack.h
> index 6edc84344b51..f8def4ad4d49 100644
> --- a/lib/stack.h
> +++ b/lib/stack.h
> @@ -10,6 +10,8 @@
>  #include <libcflat.h>
>  #include <asm/stack.h>
>  
> +bool base_address(const void *rebased_addr, unsigned long *addr);
> +
>  #ifdef HAVE_ARCH_BACKTRACE_FRAME
>  extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
>  				int max_depth, bool current_frame);
> diff --git a/lib/x86/stack.c b/lib/x86/stack.c
> index 58ab6c4b293a..7ba73becbd69 100644
> --- a/lib/x86/stack.c
> +++ b/lib/x86/stack.c
> @@ -1,6 +1,23 @@
>  #include <libcflat.h>
>  #include <stack.h>
>  
> +#ifdef CONFIG_RELOC
> +extern char _text, _etext;
> +
> +bool base_address(const void *rebased_addr, unsigned long *addr)
> +{
> +	unsigned long ra = (unsigned long)rebased_addr;
> +	unsigned long start = (unsigned long)&_text;
> +	unsigned long end = (unsigned long)&_etext;
> +
> +	if (ra < start || ra >= end)
> +		return false;
> +
> +	*addr = ra - start;
> +	return true;
> +}
> +#endif
> +
>  int arch_backtrace_frame(const void *frame, const void **return_addrs,
>  			 int max_depth, bool current_frame)
>  {

Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace

[Andrew Jones]

On Thu, Feb 29, 2024 at 01:31:52PM +1000, Nicholas Piggin wrote:
> On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote:
...
> > diff --git a/lib/stack.h b/lib/stack.h
> > index 10fc2f793354..6edc84344b51 100644
> > --- a/lib/stack.h
> > +++ b/lib/stack.h
> > @@ -11,17 +11,27 @@
> >  #include <asm/stack.h>
> >  
> >  #ifdef HAVE_ARCH_BACKTRACE_FRAME
> > -extern int backtrace_frame(const void *frame, const void **return_addrs,
> > -			   int max_depth);
> > +extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
> > +				int max_depth, bool current_frame);
> > +
> > +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> > +				  int max_depth)
> > +{
> > +	return arch_backtrace_frame(frame, return_addrs, max_depth, false);
> > +}
> > +
> > +static inline int backtrace(const void **return_addrs, int max_depth)
> > +{
> > +	return arch_backtrace_frame(NULL, return_addrs, max_depth, true);
> > +}
> >  #else
> > -static inline int
> > -backtrace_frame(const void *frame __unused, const void **return_addrs __unused,
> > -		int max_depth __unused)
> > +extern int backtrace(const void **return_addrs, int max_depth);
> > +
> > +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> > +				  int max_depth)
> >  {
> >  	return 0;
> >  }
> >  #endif
> >  
> > -extern int backtrace(const void **return_addrs, int max_depth);
> > -
> >  #endif
> 
> Is there a reason to add the inline wrappers rather than just externs
> and drop the arch_ prefix?

Only reason is to avoid duplicating the functions in each arch, but
since they're oneliners which won't likely change, then we could
duplicate them instead, if preferred, but I'm not sure what the
benefit of that over the static inlines would be.

> 
> Do we want to just generally have all arch specific functions have an
> arch_ prefix? Fine by me.

We've been slowly doing that over in 'KVM selftests', which has improved
readability, so slowly adopting it here too in kvm-unit-tests would be
nice.

> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 
> I'm fine to rebase the powerpc patch on top of this if it goes in first.
> Thanks for the heads up.
> 

Thanks,
drew

Re: [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific

[Andrew Jones]

On Thu, Feb 29, 2024 at 01:49:58PM +1000, Nicholas Piggin wrote:
> On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote:
> > Calculating the offset of an address is image specific, which is
> > architecture specific. Until now, all architectures and architecture
> > configurations which select CONFIG_RELOC were able to subtract
> > _etext, but the EFI configuration of riscv cannot (it must subtract
> > ImageBase). Make this function architecture specific, since the
> > architecture's image layout already is.
> 
> arch_base_address()?

Yeah, I should have added that prefix.

> 
> How about a default implementation unlesss HAVE_ARCH_BASE_ADDRESS?

We have a default implementation for !CONFIG_RELOC, but if an arch
selects RELOC it must have an implementation of base_address(), so
I wouldn't introduce a HAVE_ARCH_BASE_ADDRESS type of config since
it would just always be selected when RELOC is selected. It occurred
to me after posting that I probably should have just made the current
base_address() implementation weak and then only introduced the new
riscv one. I'll do that for v2.

Thanks,
drew