[kvm-unit-tests PATCH 0/6] arm64: improve debuggability

[kvm-unit-tests PATCH 0/6] arm64: improve debuggability

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

My recent experience in debugging ARM64 tests on EFI was not as fun as I
expected it to be.

There were several reasons for that besides the questionable definition
of "fun":

1. ARM64 is not compiled with frame pointers and there is no stack
   unwinder when the stack is dumped.

2. Building an EFI drops the debug information.

3. The addresses that are printed on dump_stack() and the use of GDB
   are hard because taking code relocation into account is non trivial.

The patches help both ARM64 and EFI for this matter. The image address
is printed when EFI is used to allow the use of GDB. Symbols are emitted
into a separate debug file. The frame pointer is included and special
entry is added upon an exception to allow backtracing across
exceptions.

Nadav Amit (6):
  arm: keep efi debug information in a separate file
  lib/stack: print base addresses on efi
  arm64: enable frame pointer and support stack unwinding
  arm64: stack: update trace stack on exception
  efi: Print address of image
  arm64: dump stack on bad exception

 arm/Makefile.arm        |  3 ---
 arm/Makefile.arm64      |  1 +
 arm/Makefile.common     |  8 ++++++-
 arm/cstart64.S          | 13 ++++++++++
 lib/arm64/asm-offsets.c |  3 ++-
 lib/arm64/asm/stack.h   |  3 +++
 lib/arm64/processor.c   |  1 +
 lib/arm64/stack.c       | 53 +++++++++++++++++++++++++++++++++++++++++
 lib/efi.c               |  3 +++
 lib/stack.c             | 31 ++++++++++++++++++++++--
 10 files changed, 112 insertions(+), 7 deletions(-)
 create mode 100644 lib/arm64/stack.c

-- 
2.34.1

[kvm-unit-tests PATCH 1/6] arm: keep efi debug information in a separate file

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Debugging tests that run on EFI is hard because the debug information is
not included in the EFI file. Dump it into a separeate .debug file to
allow the use of gdb or pretty_print_stacks script.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arm/Makefile.common | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arm/Makefile.common b/arm/Makefile.common
index d60cf8c..f904702 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -69,7 +69,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
 ifeq ($(CONFIG_EFI),y)
 %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
 %.so: %.o $(FLATLIBS) $(SRCDIR)/arm/efi/elf_aarch64_efi.lds $(cstart.o)
-	$(CC) $(CFLAGS) -c -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
+	$(CC) $(CFLAGS) -c -g -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
 		-DPROGNAME=\"$(@:.so=.efi)\" -DAUXFLAGS=$(AUXFLAGS)
 	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/arm/efi/elf_aarch64_efi.lds \
 		$(filter %.o, $^) $(FLATLIBS) $(@:.so=.aux.o) \
@@ -78,6 +78,9 @@ ifeq ($(CONFIG_EFI),y)
 
 %.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 .dynamic -j .dynsym \
 		-j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \
-- 
2.34.1

Re: [kvm-unit-tests PATCH 1/6] arm: keep efi debug information in a separate file

[Andrew Jones]

On Sat, Jun 17, 2023 at 01:49:25AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Debugging tests that run on EFI is hard because the debug information is
> not included in the EFI file. Dump it into a separeate .debug file to
> allow the use of gdb or pretty_print_stacks script.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arm/Makefile.common | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index d60cf8c..f904702 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -69,7 +69,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
>  ifeq ($(CONFIG_EFI),y)
>  %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
>  %.so: %.o $(FLATLIBS) $(SRCDIR)/arm/efi/elf_aarch64_efi.lds $(cstart.o)
> -	$(CC) $(CFLAGS) -c -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
> +	$(CC) $(CFLAGS) -c -g -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
>  		-DPROGNAME=\"$(@:.so=.efi)\" -DAUXFLAGS=$(AUXFLAGS)
>  	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/arm/efi/elf_aarch64_efi.lds \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.so=.aux.o) \
> @@ -78,6 +78,9 @@ ifeq ($(CONFIG_EFI),y)
>  
>  %.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 .dynamic -j .dynsym \
>  		-j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \
> -- 
> 2.34.1
>

Any reason to not also do this for x86?

One reason I ask, is that in order for pretty_print_stacks.py to make
use of this from run_tests.sh we need a patch like

diff --git a/run_tests.sh b/run_tests.sh
index f61e0057b537..67b239f1adc7 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -125,8 +125,14 @@ fi
 RUNTIME_log_stderr () { process_test_output "$1"; }
 RUNTIME_log_stdout () {
     local testname="$1"
+    local kernel
+
     if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
-        local kernel="$2"
+        if [ "$CONFIG_EFI" = "y" ]; then
+            kernel="${TEST_DIR}/${2}.efi.debug"
+        else
+            kernel="$2"
+        fi
         ./scripts/pretty_print_stacks.py "$kernel" | process_test_output "$testname"
     else
         process_test_output "$testname"


We'd have to special-case that CONFIG_EFI condition for arm64 if we don't
also create .efi.debug files for x86.

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/6] arm: keep efi debug information in a separate file

[Andrew Jones]

On Sat, Jun 17, 2023 at 01:49:25AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Debugging tests that run on EFI is hard because the debug information is
> not included in the EFI file. Dump it into a separeate .debug file to
> allow the use of gdb or pretty_print_stacks script.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arm/Makefile.common | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index d60cf8c..f904702 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -69,7 +69,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
>  ifeq ($(CONFIG_EFI),y)
>  %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
>  %.so: %.o $(FLATLIBS) $(SRCDIR)/arm/efi/elf_aarch64_efi.lds $(cstart.o)
> -	$(CC) $(CFLAGS) -c -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
> +	$(CC) $(CFLAGS) -c -g -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
>  		-DPROGNAME=\"$(@:.so=.efi)\" -DAUXFLAGS=$(AUXFLAGS)
>  	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/arm/efi/elf_aarch64_efi.lds \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.so=.aux.o) \
> @@ -78,6 +78,9 @@ ifeq ($(CONFIG_EFI),y)
>  
>  %.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 .dynamic -j .dynsym \
>  		-j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \
> -- 
> 2.34.1
>

We also need a change to arm_clean to clean these new files up. Or, since
we probably want them for x86 as well and we already have other efi
cleanup to do, then maybe we should have a common efi_clean in the top-
level Makefile which x86's and arm's clean use to clean up all efi related
files.

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/6] arm: keep efi debug information in a separate file

[Nadav Amit]

> On Jun 24, 2023, at 3:31 AM, Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> !! External Email
> 
> On Sat, Jun 17, 2023 at 01:49:25AM +0000, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> 
>> Debugging tests that run on EFI is hard because the debug information is
>> not included in the EFI file. Dump it into a separeate .debug file to
>> allow the use of gdb or pretty_print_stacks script.
>> 
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>> arm/Makefile.common | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index d60cf8c..f904702 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -69,7 +69,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
>> ifeq ($(CONFIG_EFI),y)
>> %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
>> %.so: %.o $(FLATLIBS) $(SRCDIR)/arm/efi/elf_aarch64_efi.lds $(cstart.o)
>> -     $(CC) $(CFLAGS) -c -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
>> +     $(CC) $(CFLAGS) -c -g -o $(@:.so=.aux.o) $(SRCDIR)/lib/auxinfo.c \
>>              -DPROGNAME=\"$(@:.so=.efi)\" -DAUXFLAGS=$(AUXFLAGS)
>>      $(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/arm/efi/elf_aarch64_efi.lds \
>>              $(filter %.o, $^) $(FLATLIBS) $(@:.so=.aux.o) \
>> @@ -78,6 +78,9 @@ ifeq ($(CONFIG_EFI),y)
>> 
>> %.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 .dynamic -j .dynsym \
>>              -j .rel -j .rela -j .rel.* -j .rela.* -j .rel* -j .rela* \
>> --
>> 2.34.1
>> 
> 
> We also need a change to arm_clean to clean these new files up. Or, since
> we probably want them for x86 as well and we already have other efi
> cleanup to do, then maybe we should have a common efi_clean in the top-
> level Makefile which x86's and arm's clean use to clean up all efi related
> files.

[ I aggregate my response to both of your emails ]

Thanks!

I will create .debug files for x86, but I am going to remove the ‘-g’ option.
If anyone wants it, he would need to provide CFLAGS during configure.

I will cleanup the .debug files for each arch separately since other files
are cleanup’d that way.

[kvm-unit-tests PATCH 2/6] lib/stack: print base addresses on efi

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Making sense from dumped stacks when running EFI tests is very hard due
to the relocation. Fix it by adjusting the address back to the original
address.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 lib/stack.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/lib/stack.c b/lib/stack.c
index bdb23fd..c3c7c24 100644
--- a/lib/stack.c
+++ b/lib/stack.c
@@ -6,13 +6,38 @@
  */
 
 #include <libcflat.h>
+#include <stdbool.h>
 #include <stack.h>
 
 #define MAX_DEPTH 20
 
+#ifdef CONFIG_EFI
+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)
+{
+	*addr = (unsigned long)rebased_addr;
+	return true;
+}
+#endif
+
 static void print_stack(const void **return_addrs, int depth,
 			bool top_is_return_address)
 {
+	unsigned long addr;
 	int i = 0;
 
 	printf("\tSTACK:");
@@ -20,12 +45,14 @@ static void print_stack(const void **return_addrs, int depth,
 	/* @addr indicates a non-return address, as expected by the stack
 	 * pretty printer script. */
 	if (depth > 0 && !top_is_return_address) {
-		printf(" @%lx", (unsigned long) return_addrs[0]);
+		if (base_address(return_addrs[0], &addr))
+			printf(" @%lx", addr);
 		i++;
 	}
 
 	for (; i < depth; i++) {
-		printf(" %lx", (unsigned long) return_addrs[i]);
+		if (base_address(return_addrs[i], &addr))
+			printf(" %lx", addr);
 	}
 	printf("\n");
 }
-- 
2.34.1

Re: [kvm-unit-tests PATCH 2/6] lib/stack: print base addresses on efi

[Andrew Jones]

On Sat, Jun 17, 2023 at 01:49:26AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Making sense from dumped stacks when running EFI tests is very hard due
> to the relocation. Fix it by adjusting the address back to the original
> address.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  lib/stack.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/stack.c b/lib/stack.c
> index bdb23fd..c3c7c24 100644
> --- a/lib/stack.c
> +++ b/lib/stack.c
> @@ -6,13 +6,38 @@
>   */
>  
>  #include <libcflat.h>
> +#include <stdbool.h>
>  #include <stack.h>
>  
>  #define MAX_DEPTH 20
>  
> +#ifdef CONFIG_EFI

We also relocate when building non-efi arm64 unit tests, so this should be

#if defined(CONFIG_EFI) || defined(__aarch64__)

Or, we could create a new config, CONFIG_RELOC, which gets set by
--enable-efi and --arch=arm64.

> +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)
> +{
> +	*addr = (unsigned long)rebased_addr;
> +	return true;
> +}
> +#endif
> +
>  static void print_stack(const void **return_addrs, int depth,
>  			bool top_is_return_address)
>  {
> +	unsigned long addr;
>  	int i = 0;
>  
>  	printf("\tSTACK:");
> @@ -20,12 +45,14 @@ static void print_stack(const void **return_addrs, int depth,
>  	/* @addr indicates a non-return address, as expected by the stack
>  	 * pretty printer script. */
>  	if (depth > 0 && !top_is_return_address) {
> -		printf(" @%lx", (unsigned long) return_addrs[0]);
> +		if (base_address(return_addrs[0], &addr))
> +			printf(" @%lx", addr);
>  		i++;
>  	}
>  
>  	for (; i < depth; i++) {
> -		printf(" %lx", (unsigned long) return_addrs[i]);
> +		if (base_address(return_addrs[i], &addr))
> +			printf(" %lx", addr);
>  	}
>  	printf("\n");
>  }
> -- 
> 2.34.1
>

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/6] lib/stack: print base addresses on efi

[Nadav Amit]

> On Jun 24, 2023, at 3:13 AM, Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> We also relocate when building non-efi arm64 unit tests, so this should be
> 
> #if defined(CONFIG_EFI) || defined(__aarch64__)
> 
> Or, we could create a new config, CONFIG_RELOC, which gets set by
> --enable-efi and --arch=arm64.

Will do. Thanks.

[kvm-unit-tests PATCH 3/6] arm64: enable frame pointer and support stack unwinding

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Enable frame pointers for arm64 and perform stack unwinding based on
arm64 convention.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arm/Makefile.arm      |  3 ---
 arm/Makefile.arm64    |  1 +
 arm/Makefile.common   |  3 +++
 lib/arm64/asm/stack.h |  3 +++
 lib/arm64/stack.c     | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 lib/arm64/stack.c

diff --git a/arm/Makefile.arm b/arm/Makefile.arm
index 2ce00f5..7fd39f3 100644
--- a/arm/Makefile.arm
+++ b/arm/Makefile.arm
@@ -11,9 +11,6 @@ ifeq ($(CONFIG_EFI),y)
 $(error Cannot build arm32 tests as EFI apps)
 endif
 
-# stack.o relies on frame pointers.
-KEEP_FRAME_POINTER := y
-
 CFLAGS += $(machine)
 CFLAGS += -mcpu=$(PROCESSOR)
 CFLAGS += -mno-unaligned-access
diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
index eada7f9..60385e2 100644
--- a/arm/Makefile.arm64
+++ b/arm/Makefile.arm64
@@ -21,6 +21,7 @@ define arch_elf_check =
 endef
 
 cstart.o = $(TEST_DIR)/cstart64.o
+cflatobjs += lib/arm64/stack.o
 cflatobjs += lib/arm64/processor.o
 cflatobjs += lib/arm64/spinlock.o
 cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o
diff --git a/arm/Makefile.common b/arm/Makefile.common
index f904702..7fecfb3 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -22,6 +22,9 @@ $(TEST_DIR)/sieve.elf: AUXFLAGS = 0x1
 ##################################################################
 AUXFLAGS ?= 0x0
 
+# stack.o relies on frame pointers.
+KEEP_FRAME_POINTER := y
+
 CFLAGS += -std=gnu99
 CFLAGS += -ffreestanding
 CFLAGS += -O2
diff --git a/lib/arm64/asm/stack.h b/lib/arm64/asm/stack.h
index d000624..be486cf 100644
--- a/lib/arm64/asm/stack.h
+++ b/lib/arm64/asm/stack.h
@@ -5,4 +5,7 @@
 #error Do not directly include <asm/stack.h>. Just use <stack.h>.
 #endif
 
+#define HAVE_ARCH_BACKTRACE_FRAME
+#define HAVE_ARCH_BACKTRACE
+
 #endif
diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
new file mode 100644
index 0000000..1e2568a
--- /dev/null
+++ b/lib/arm64/stack.c
@@ -0,0 +1,37 @@
+/*
+ * backtrace support (this is a modified lib/x86/stack.c)
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <stack.h>
+
+extern char vector_stub_start, vector_stub_end;
+
+int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) {
+	const void *fp = frame;
+	void *lr;
+	int depth;
+
+	/*
+	 * ARM64 stack grows down. fp points to the previous fp on the stack,
+	 * and lr is just above it
+	 */
+	for (depth = 0; fp && depth < max_depth; ++depth) {
+
+		asm volatile ("ldp %0, %1, [%2]"
+				  : "=r" (fp), "=r" (lr)
+				  : "r" (fp)
+				  : );
+
+		return_addrs[depth] = lr;
+	}
+
+	return depth;
+}
+
+int backtrace(const void **return_addrs, int max_depth)
+{
+	return backtrace_frame(__builtin_frame_address(0),
+			       return_addrs, max_depth);
+}
-- 
2.34.1

Re: [kvm-unit-tests PATCH 3/6] arm64: enable frame pointer and support stack unwinding

[Andrew Jones]

On Sat, Jun 17, 2023 at 01:49:27AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Enable frame pointers for arm64 and perform stack unwinding based on
> arm64 convention.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arm/Makefile.arm      |  3 ---
>  arm/Makefile.arm64    |  1 +
>  arm/Makefile.common   |  3 +++
>  lib/arm64/asm/stack.h |  3 +++
>  lib/arm64/stack.c     | 37 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 44 insertions(+), 3 deletions(-)
>  create mode 100644 lib/arm64/stack.c
> 
> diff --git a/arm/Makefile.arm b/arm/Makefile.arm
> index 2ce00f5..7fd39f3 100644
> --- a/arm/Makefile.arm
> +++ b/arm/Makefile.arm
> @@ -11,9 +11,6 @@ ifeq ($(CONFIG_EFI),y)
>  $(error Cannot build arm32 tests as EFI apps)
>  endif
>  
> -# stack.o relies on frame pointers.
> -KEEP_FRAME_POINTER := y
> -
>  CFLAGS += $(machine)
>  CFLAGS += -mcpu=$(PROCESSOR)
>  CFLAGS += -mno-unaligned-access
> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> index eada7f9..60385e2 100644
> --- a/arm/Makefile.arm64
> +++ b/arm/Makefile.arm64
> @@ -21,6 +21,7 @@ define arch_elf_check =
>  endef
>  
>  cstart.o = $(TEST_DIR)/cstart64.o
> +cflatobjs += lib/arm64/stack.o
>  cflatobjs += lib/arm64/processor.o
>  cflatobjs += lib/arm64/spinlock.o
>  cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f904702..7fecfb3 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -22,6 +22,9 @@ $(TEST_DIR)/sieve.elf: AUXFLAGS = 0x1
>  ##################################################################
>  AUXFLAGS ?= 0x0
>  
> +# stack.o relies on frame pointers.
> +KEEP_FRAME_POINTER := y
> +
>  CFLAGS += -std=gnu99
>  CFLAGS += -ffreestanding
>  CFLAGS += -O2
> diff --git a/lib/arm64/asm/stack.h b/lib/arm64/asm/stack.h
> index d000624..be486cf 100644
> --- a/lib/arm64/asm/stack.h
> +++ b/lib/arm64/asm/stack.h
> @@ -5,4 +5,7 @@
>  #error Do not directly include <asm/stack.h>. Just use <stack.h>.
>  #endif
>  
> +#define HAVE_ARCH_BACKTRACE_FRAME
> +#define HAVE_ARCH_BACKTRACE
> +
>  #endif
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> new file mode 100644
> index 0000000..1e2568a
> --- /dev/null
> +++ b/lib/arm64/stack.c
> @@ -0,0 +1,37 @@
> +/*
> + * backtrace support (this is a modified lib/x86/stack.c)
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <stack.h>
> +
> +extern char vector_stub_start, vector_stub_end;

These aren't used until the next patch.

> +
> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) {

'{' should be on its own line. I usually try to run the kernel's
checkpatch since we use the same style (except we're even more forgiving
for long lines).

> +	const void *fp = frame;
> +	void *lr;
> +	int depth;
> +
> +	/*
> +	 * ARM64 stack grows down. fp points to the previous fp on the stack,
> +	 * and lr is just above it
> +	 */
> +	for (depth = 0; fp && depth < max_depth; ++depth) {
> +
> +		asm volatile ("ldp %0, %1, [%2]"
> +				  : "=r" (fp), "=r" (lr)
> +				  : "r" (fp)
> +				  : );
> +
> +		return_addrs[depth] = lr;
> +	}
> +
> +	return depth;
> +}
> +
> +int backtrace(const void **return_addrs, int max_depth)
> +{
> +	return backtrace_frame(__builtin_frame_address(0),
> +			       return_addrs, max_depth);
> +}
> -- 
> 2.34.1
> 

Thanks,
drew

Re: [kvm-unit-tests PATCH 3/6] arm64: enable frame pointer and support stack unwinding

[Nadav Amit]

> On Jun 24, 2023, at 3:13 AM, Andrew Jones <andrew.jones@linux.dev> wrote:
> 
>> +extern char vector_stub_start, vector_stub_end;
> 
> These aren't used until the next patch.
> 
>> +
>> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) {
> 
> '{' should be on its own line. I usually try to run the kernel's
> checkpatch since we use the same style (except we're even more forgiving
> for long lines).

I usually do use checkpatch. I guess I got sloppy. I will fix these 2 issues.

BTW: I used the get_maintainer script to get who to send the patches to and it
included the depreciated kvmarm@lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu> .. Ugh.

Re: [kvm-unit-tests PATCH 3/6] arm64: enable frame pointer and support stack unwinding

[Andrew Jones]

On Sun, Jun 25, 2023 at 12:22:15PM -0700, Nadav Amit wrote:
> 
> 
> > On Jun 24, 2023, at 3:13 AM, Andrew Jones <andrew.jones@linux.dev> wrote:
> > 
> >> +extern char vector_stub_start, vector_stub_end;
> > 
> > These aren't used until the next patch.
> > 
> >> +
> >> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) {
> > 
> > '{' should be on its own line. I usually try to run the kernel's
> > checkpatch since we use the same style (except we're even more forgiving
> > for long lines).
> 
> I usually do use checkpatch. I guess I got sloppy. I will fix these 2 issues.
> 
> BTW: I used the get_maintainer script to get who to send the patches to and it
> included the depreciated kvmarm@lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu> .. Ugh.

Ah, thanks for pointing that out. It's safe to declare the depreciation
peroid over. I just pushed a patch dropping it now.

drew

[kvm-unit-tests PATCH 4/6] arm64: stack: update trace stack on exception

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Using gdb for backtracing or dumping the stack following an exception is
not very helpful as the exact location of the exception is not saved.

Add an additional frame to save the location of the exception.

One delicate point is dealing with the pretty_print_stacks script. When
the stack is dumped, the script would not print the right address for
the exception address: for every return address it deducts "1" before
looking for the instruction location in the code (using addr2line). As a
somewhat hacky solution add "1" for the exception address when dumping
the stack.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arm/cstart64.S          | 13 +++++++++++++
 lib/arm64/asm-offsets.c |  3 ++-
 lib/arm64/stack.c       | 16 ++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arm/cstart64.S b/arm/cstart64.S
index cbd6b51..61e27d3 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -314,6 +314,13 @@ exceptions_init:
 	mrs	x2, spsr_el1
 	stp	x1, x2, [sp, #S_PC]
 
+	/*
+	 * Save a frame pointer using the link to allow unwinding of
+	 * exceptions.
+	 */
+	stp	x29, x1, [sp, #S_FP]
+	add 	x29, sp, #S_FP
+
 	mov	x0, \vec
 	mov	x1, sp
 	mrs	x2, esr_el1
@@ -349,6 +356,9 @@ exceptions_init:
 	eret
 .endm
 
+vector_stub_start:
+.globl vector_stub_start
+
 vector_stub	el1t_sync,     0
 vector_stub	el1t_irq,      1
 vector_stub	el1t_fiq,      2
@@ -369,6 +379,9 @@ vector_stub	el0_irq_32,   13
 vector_stub	el0_fiq_32,   14
 vector_stub	el0_error_32, 15
 
+vector_stub_end:
+.globl vector_stub_end
+
 .section .text.ex
 
 .macro ventry, label
diff --git a/lib/arm64/asm-offsets.c b/lib/arm64/asm-offsets.c
index 53a1277..7b8bffb 100644
--- a/lib/arm64/asm-offsets.c
+++ b/lib/arm64/asm-offsets.c
@@ -25,6 +25,7 @@ int main(void)
 	OFFSET(S_PSTATE, pt_regs, pstate);
 	OFFSET(S_ORIG_X0, pt_regs, orig_x0);
 	OFFSET(S_SYSCALLNO, pt_regs, syscallno);
-	DEFINE(S_FRAME_SIZE, sizeof(struct pt_regs));
+	DEFINE(S_FRAME_SIZE, (sizeof(struct pt_regs) + 16));
+	DEFINE(S_FP, sizeof(struct pt_regs));
 	return 0;
 }
diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
index 1e2568a..a48ecbb 100644
--- a/lib/arm64/stack.c
+++ b/lib/arm64/stack.c
@@ -12,6 +12,8 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	const void *fp = frame;
 	void *lr;
 	int depth;
+	bool is_exception = false;
+	unsigned long addr;
 
 	/*
 	 * ARM64 stack grows down. fp points to the previous fp on the stack,
@@ -25,6 +27,20 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 				  : );
 
 		return_addrs[depth] = lr;
+
+		/*
+		 * If this is an exception, add 1 to the pointer so when the
+		 * pretty_print_stacks script is run it would get the right
+		 * address (it deducts 1 to find the call address, but we want
+		 * the actual address).
+		 */
+		if (is_exception)
+			return_addrs[depth] += 1;
+
+		/* Check if we are in the exception handlers for the next entry */
+		addr = (unsigned long)lr;
+		is_exception = (addr >= (unsigned long)&vector_stub_start &&
+				addr < (unsigned long)&vector_stub_end);
 	}
 
 	return depth;
-- 
2.34.1

Re: [kvm-unit-tests PATCH 4/6] arm64: stack: update trace stack on exception

[Andrew Jones]

On Sat, Jun 17, 2023 at 01:49:28AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Using gdb for backtracing or dumping the stack following an exception is
> not very helpful as the exact location of the exception is not saved.
> 
> Add an additional frame to save the location of the exception.
> 
> One delicate point is dealing with the pretty_print_stacks script. When
> the stack is dumped, the script would not print the right address for
> the exception address: for every return address it deducts "1" before
> looking for the instruction location in the code (using addr2line). As a
> somewhat hacky solution add "1" for the exception address when dumping
> the stack.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arm/cstart64.S          | 13 +++++++++++++
>  lib/arm64/asm-offsets.c |  3 ++-
>  lib/arm64/stack.c       | 16 ++++++++++++++++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index cbd6b51..61e27d3 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -314,6 +314,13 @@ exceptions_init:
>  	mrs	x2, spsr_el1
>  	stp	x1, x2, [sp, #S_PC]
>  
> +	/*
> +	 * Save a frame pointer using the link to allow unwinding of
> +	 * exceptions.
> +	 */
> +	stp	x29, x1, [sp, #S_FP]
> +	add 	x29, sp, #S_FP
> +
>  	mov	x0, \vec
>  	mov	x1, sp
>  	mrs	x2, esr_el1
> @@ -349,6 +356,9 @@ exceptions_init:
>  	eret
>  .endm
>  
> +vector_stub_start:
> +.globl vector_stub_start

nit: I'd prefer the .globl directives above the labels to match the
rest of the file.

> +
>  vector_stub	el1t_sync,     0
>  vector_stub	el1t_irq,      1
>  vector_stub	el1t_fiq,      2
> @@ -369,6 +379,9 @@ vector_stub	el0_irq_32,   13
>  vector_stub	el0_fiq_32,   14
>  vector_stub	el0_error_32, 15
>  
> +vector_stub_end:
> +.globl vector_stub_end
> +
>  .section .text.ex
>  
>  .macro ventry, label
> diff --git a/lib/arm64/asm-offsets.c b/lib/arm64/asm-offsets.c
> index 53a1277..7b8bffb 100644
> --- a/lib/arm64/asm-offsets.c
> +++ b/lib/arm64/asm-offsets.c
> @@ -25,6 +25,7 @@ int main(void)
>  	OFFSET(S_PSTATE, pt_regs, pstate);
>  	OFFSET(S_ORIG_X0, pt_regs, orig_x0);
>  	OFFSET(S_SYSCALLNO, pt_regs, syscallno);
> -	DEFINE(S_FRAME_SIZE, sizeof(struct pt_regs));
> +	DEFINE(S_FRAME_SIZE, (sizeof(struct pt_regs) + 16));
> +	DEFINE(S_FP, sizeof(struct pt_regs));

It'd be good to comment this, something like

  ...
  OFFSET(S_ORIG_X0, pt_regs, orig_x0);
  OFFSET(S_SYSCALLNO, pt_regs, syscallno);

  /* FP and LR (16 bytes) go on the frame above pt_regs */
  DEFINE(S_FP, sizeof(struct pt_regs));
  DEFINE(S_FRAME_SIZE, (sizeof(struct pt_regs) + 16));

  return 0;

>  	return 0;
>  }
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> index 1e2568a..a48ecbb 100644
> --- a/lib/arm64/stack.c
> +++ b/lib/arm64/stack.c
> @@ -12,6 +12,8 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  	const void *fp = frame;
>  	void *lr;
>  	int depth;
> +	bool is_exception = false;
> +	unsigned long addr;
>  
>  	/*
>  	 * ARM64 stack grows down. fp points to the previous fp on the stack,
> @@ -25,6 +27,20 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>  				  : );
>  
>  		return_addrs[depth] = lr;
> +
> +		/*
> +		 * If this is an exception, add 1 to the pointer so when the
> +		 * pretty_print_stacks script is run it would get the right
> +		 * address (it deducts 1 to find the call address, but we want
> +		 * the actual address).
> +		 */
> +		if (is_exception)
> +			return_addrs[depth] += 1;
> +
> +		/* Check if we are in the exception handlers for the next entry */
> +		addr = (unsigned long)lr;
> +		is_exception = (addr >= (unsigned long)&vector_stub_start &&
> +				addr < (unsigned long)&vector_stub_end);
>  	}
>  
>  	return depth;
> -- 
> 2.34.1
>

Thanks,
drew

[kvm-unit-tests PATCH 5/6] efi: Print address of image

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Using gdb to debug tests that are run from efi is very hard without
knowing the base address in which the image was loaded. Print the
address so it can be used while debugging.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 lib/efi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/efi.c b/lib/efi.c
index 2e127a4..f42edd4 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -18,6 +18,7 @@
 extern int __argc, __envc;
 extern char *__argv[100];
 extern char *__environ[200];
+extern char _text;
 
 extern int main(int argc, char **argv, char **envp);
 
@@ -363,6 +364,8 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
 		goto efi_main_error;
 	}
 
+	printf("Address of image is: 0x%lx\n", (unsigned long)&_text);
+
 	/* Run the test case */
 	ret = main(__argc, __argv, __environ);
 
-- 
2.34.1

[kvm-unit-tests PATCH 6/6] arm64: dump stack on bad exception

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 lib/arm64/processor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
index 831207c..5bcad67 100644
--- a/lib/arm64/processor.c
+++ b/lib/arm64/processor.c
@@ -130,6 +130,7 @@ static void bad_exception(enum vector v, struct pt_regs *regs,
 	printf("Vector: %d (%s)\n", v, vector_names[v]);
 	printf("ESR_EL1: %8s%08x, ec=%#x (%s)\n", "", esr, ec, ec_names[ec]);
 	printf("FAR_EL1: %016lx (%svalid)\n", far, far_valid ? "" : "not ");
+	dump_stack();
 	printf("Exception frame registers:\n");
 	show_regs(regs);
 	abort();
-- 
2.34.1

Re: [kvm-unit-tests PATCH 6/6] arm64: dump stack on bad exception

[Nadav Amit]

> On Jun 16, 2023, at 6:49 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
> From: Nadav Amit <namit@vmware.com>
> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>

Ugh.

Signed-off-by: Nadav Amit <namit@vmware.com <mailto:namit@vmware.com>>