* [PATCH] riscv: add backtrace support
@ 2023-05-15 13:03 Ben Dooks
2023-05-25 11:05 ` Ben Dooks
0 siblings, 1 reply; 6+ messages in thread
From: Ben Dooks @ 2023-05-15 13:03 UTC (permalink / raw)
To: u-boot; +Cc: ben.dooks, rick, Ben Dooks
When debugging, it is useful to have a backtrace to find
out what is in the call stack as the previous function (RA)
may not have been the culprit.
Since this adds size to the build, do not add it by default
and avoid putting it in the SPL build if not needed.
Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
arch/riscv/Kconfig | 10 ++++++++++
arch/riscv/Makefile | 4 ++++
arch/riscv/cpu/start.S | 1 +
arch/riscv/lib/interrupts.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 50 insertions(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index f6ed05906a..3f2316cfb5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -98,6 +98,16 @@ config ARCH_RV64I
endchoice
+config FRAMEPOINTER
+ bool "Build with frame pointer for stack unwinding"
+ help
+ Choose this option to use the frame pointer so the stack can be
+ unwound if needed. This is useful for tracing where faults came
+ from as the source may be several functions back
+
+ If you say Y here, then the code size will be increased due to
+ having to store the fp.
+
choice
prompt "Code Model"
default CMODEL_MEDLOW
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 4963b5109b..0cb60c7c7e 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -45,6 +45,10 @@ endif
ARCH_FLAGS = -march=$(RISCV_MARCH) -mabi=$(ABI) \
-mcmodel=$(CMODEL)
+ifeq ($(CONFIG_$(SPL_)FRAMEPOINTER),y)
+ ARCH_FLAGS += -fno-omit-frame-pointer
+endif
+
PLATFORM_CPPFLAGS += $(ARCH_FLAGS)
CFLAGS_EFI += $(ARCH_FLAGS)
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index dad22bfea8..3d13722615 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -396,6 +396,7 @@ call_board_init_r:
*/
mv a0, s3 /* gd_t */
mv a1, s4 /* dest_addr */
+ mv s0, zero /* fp == NULL */
/*
* jump to it ...
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index e966afa7e3..db3d7e294b 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -53,6 +53,40 @@ static void show_regs(struct pt_regs *regs)
#endif
}
+#if defined(CONFIG_FRAMEPOINTER) || defined(CONFIG_SPL_FRAMEPOINTER)
+static void show_backtrace(struct pt_regs *regs)
+{
+ uintptr_t *fp = (uintptr_t *)regs->s0;
+ unsigned count = 0;
+ ulong ra;
+
+ printf("backtrace:\n");
+
+ /* there are a few entry points where the s0 register is
+ * set to gd, so to avoid changing those, just abort if
+ * the value is the same */
+ while (fp != NULL && fp != (uintptr_t *)gd) {
+ ra = fp[-1];
+ printf("backtrace %2d: FP: " REG_FMT " RA: " REG_FMT,
+ count, (ulong)fp, ra);
+
+ if (gd && gd->flags & GD_FLG_RELOC)
+ printf(" - RA: " REG_FMT " reloc adjusted\n",
+ ra - gd->reloc_off);
+ else
+ printf("\n");
+
+ fp = (uintptr_t *)fp[-2];
+ count++;
+ }
+}
+#else
+static void show_backtrace(struct pt_regs *regs)
+{
+ printf("No backtrace support enabled\n");
+}
+#endif
+
/**
* instr_len() - get instruction length
*
@@ -119,6 +153,7 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs)
epc - gd->reloc_off, regs->ra - gd->reloc_off);
show_regs(regs);
+ show_backtrace(regs);
show_code(epc);
show_efi_loaded_images(epc);
panic("\n");
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] riscv: add backtrace support
2023-05-15 13:03 [PATCH] riscv: add backtrace support Ben Dooks
@ 2023-05-25 11:05 ` Ben Dooks
2023-06-14 6:25 ` Bo Gan
0 siblings, 1 reply; 6+ messages in thread
From: Ben Dooks @ 2023-05-25 11:05 UTC (permalink / raw)
To: Ben Dooks, u-boot, Ben Dooks; +Cc: rick
On 15/05/2023 14:03, Ben Dooks wrote:
> When debugging, it is useful to have a backtrace to find
> out what is in the call stack as the previous function (RA)
> may not have been the culprit.
>
> Since this adds size to the build, do not add it by default
> and avoid putting it in the SPL build if not needed.
Hi, has anyone had time to review this?
As a note, my sifive.com address may go soon, so please
add ben.dooks@codethink.co.uk to any followups.
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> arch/riscv/Kconfig | 10 ++++++++++
> arch/riscv/Makefile | 4 ++++
> arch/riscv/cpu/start.S | 1 +
> arch/riscv/lib/interrupts.c | 35 +++++++++++++++++++++++++++++++++++
> 4 files changed, 50 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index f6ed05906a..3f2316cfb5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -98,6 +98,16 @@ config ARCH_RV64I
>
> endchoice
>
> +config FRAMEPOINTER
> + bool "Build with frame pointer for stack unwinding"
> + help
> + Choose this option to use the frame pointer so the stack can be
> + unwound if needed. This is useful for tracing where faults came
> + from as the source may be several functions back
> +
> + If you say Y here, then the code size will be increased due to
> + having to store the fp.
> +
> choice
> prompt "Code Model"
> default CMODEL_MEDLOW
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 4963b5109b..0cb60c7c7e 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -45,6 +45,10 @@ endif
> ARCH_FLAGS = -march=$(RISCV_MARCH) -mabi=$(ABI) \
> -mcmodel=$(CMODEL)
>
> +ifeq ($(CONFIG_$(SPL_)FRAMEPOINTER),y)
> + ARCH_FLAGS += -fno-omit-frame-pointer
> +endif
> +
> PLATFORM_CPPFLAGS += $(ARCH_FLAGS)
> CFLAGS_EFI += $(ARCH_FLAGS)
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index dad22bfea8..3d13722615 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -396,6 +396,7 @@ call_board_init_r:
> */
> mv a0, s3 /* gd_t */
> mv a1, s4 /* dest_addr */
> + mv s0, zero /* fp == NULL */
>
> /*
> * jump to it ...
> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
> index e966afa7e3..db3d7e294b 100644
> --- a/arch/riscv/lib/interrupts.c
> +++ b/arch/riscv/lib/interrupts.c
> @@ -53,6 +53,40 @@ static void show_regs(struct pt_regs *regs)
> #endif
> }
>
> +#if defined(CONFIG_FRAMEPOINTER) || defined(CONFIG_SPL_FRAMEPOINTER)
> +static void show_backtrace(struct pt_regs *regs)
> +{
> + uintptr_t *fp = (uintptr_t *)regs->s0;
> + unsigned count = 0;
> + ulong ra;
> +
> + printf("backtrace:\n");
> +
> + /* there are a few entry points where the s0 register is
> + * set to gd, so to avoid changing those, just abort if
> + * the value is the same */
> + while (fp != NULL && fp != (uintptr_t *)gd) {
> + ra = fp[-1];
> + printf("backtrace %2d: FP: " REG_FMT " RA: " REG_FMT,
> + count, (ulong)fp, ra);
> +
> + if (gd && gd->flags & GD_FLG_RELOC)
> + printf(" - RA: " REG_FMT " reloc adjusted\n",
> + ra - gd->reloc_off);
> + else
> + printf("\n");
> +
> + fp = (uintptr_t *)fp[-2];
> + count++;
> + }
> +}
> +#else
> +static void show_backtrace(struct pt_regs *regs)
> +{
> + printf("No backtrace support enabled\n");
> +}
> +#endif
> +
> /**
> * instr_len() - get instruction length
> *
> @@ -119,6 +153,7 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs)
> epc - gd->reloc_off, regs->ra - gd->reloc_off);
>
> show_regs(regs);
> + show_backtrace(regs);
> show_code(epc);
> show_efi_loaded_images(epc);
> panic("\n");
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] riscv: add backtrace support
2023-05-25 11:05 ` Ben Dooks
@ 2023-06-14 6:25 ` Bo Gan
2023-06-14 17:15 ` Ben Dooks
0 siblings, 1 reply; 6+ messages in thread
From: Bo Gan @ 2023-06-14 6:25 UTC (permalink / raw)
To: Ben Dooks, Ben Dooks, u-boot; +Cc: rick
On 5/25/23 4:05 AM, Ben Dooks wrote:
> On 15/05/2023 14:03, Ben Dooks wrote:
>> When debugging, it is useful to have a backtrace to find
>> out what is in the call stack as the previous function (RA)
>> may not have been the culprit.
>>
>> Since this adds size to the build, do not add it by default
>> and avoid putting it in the SPL build if not needed.
>
> Hi, has anyone had time to review this?
>
Hi Ben, this looks really useful. I'd like to use it in SPL,
but I'm unable to enable CONFIG_SPL_FRAMEPOINTER=y. It's likely
that you need to add a SPL_FRAMEPOINTER entry to Kconfig as well.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: add backtrace support
2023-06-14 6:25 ` Bo Gan
@ 2023-06-14 17:15 ` Ben Dooks
2023-06-15 19:01 ` Bo Gan
0 siblings, 1 reply; 6+ messages in thread
From: Ben Dooks @ 2023-06-14 17:15 UTC (permalink / raw)
To: Bo Gan, Ben Dooks, u-boot; +Cc: rick
On 14/06/2023 07:25, Bo Gan wrote:
> On 5/25/23 4:05 AM, Ben Dooks wrote:
>> On 15/05/2023 14:03, Ben Dooks wrote:
>>> When debugging, it is useful to have a backtrace to find
>>> out what is in the call stack as the previous function (RA)
>>> may not have been the culprit.
>>>
>>> Since this adds size to the build, do not add it by default
>>> and avoid putting it in the SPL build if not needed.
>>
>> Hi, has anyone had time to review this?
>>
>
> Hi Ben, this looks really useful. I'd like to use it in SPL,
> but I'm unable to enable CONFIG_SPL_FRAMEPOINTER=y. It's likely
> that you need to add a SPL_FRAMEPOINTER entry to Kconfig as well.
I will have a look at this, but testing may not be easy as the
setup we're using has limited SPL space.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: add backtrace support
2023-06-14 17:15 ` Ben Dooks
@ 2023-06-15 19:01 ` Bo Gan
2023-06-16 8:55 ` Ben Dooks
0 siblings, 1 reply; 6+ messages in thread
From: Bo Gan @ 2023-06-15 19:01 UTC (permalink / raw)
To: Ben Dooks, Ben Dooks, u-boot; +Cc: rick
On 6/14/23 10:15 AM, Ben Dooks wrote:
> On 14/06/2023 07:25, Bo Gan wrote:
>> On 5/25/23 4:05 AM, Ben Dooks wrote:
>>> On 15/05/2023 14:03, Ben Dooks wrote:
>>>> When debugging, it is useful to have a backtrace to find
>>>> out what is in the call stack as the previous function (RA)
>>>> may not have been the culprit.
>>>>
>>>> Since this adds size to the build, do not add it by default
>>>> and avoid putting it in the SPL build if not needed.
>>>
>>> Hi, has anyone had time to review this?
>>>
>>
>> Hi Ben, this looks really useful. I'd like to use it in SPL,
>> but I'm unable to enable CONFIG_SPL_FRAMEPOINTER=y. It's likely
>> that you need to add a SPL_FRAMEPOINTER entry to Kconfig as well.
>
> I will have a look at this, but testing may not be easy as the
> setup we're using has limited SPL space.
>
I did a hack (duplicate `config FRAMEPOINTER` to `config SPL_FRAMEPOINTER`)
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -63,6 +63,24 @@ config SPL_SYS_DCACHE_OFF
+config SPL_FRAMEPOINTER
+ bool "Build with frame pointer for stack unwinding"
+ depends on SPL
+ help
+ Choose this option to use the frame pointer so the stack can be
+ unwound if needed. This is useful for tracing where faults came
+ from as the source may be several functions back
+
+ If you say Y here, then the code size will be increased due to
+ having to store the fp.
+
This is sufficient for enabling CONFIG_SPL_FRAMEPOINTER=y. With this, I
tested SPL on JH7110 (VisionFive 2), and observed your patch's working:
Unhandled exception: Load access fault
EPC: 000000000800335e RA: 00000000080033e8 TVAL: 0000000000000040
SP: 00000000080ff5b0 GP: 00000000080dbda0 TP: 0000000000000000
T0: 00000000080ffb20 T1: 0000000000000020 T2: 0000000000000000
S0: 00000000080ffb20 S1: 00000000080ff6d0 A0: 0000000000000000
A1: 00000000080ff6d0 A2: 0000000008020d83 A3: 00000000080ffb20
A4: 0000000000000025 A5: 0000000008040218 A6: 0000000000000020
A7: 0000000008000000 S2: 0000000000000013 S3: 0000000000000001
S4: 0000000040000000 S5: 0000000000000001 S6: 000000000000000a
S7: 00000000080ffb88 S8: 0000000002000000 S9: 000000000801bc60
S10: 00000000080fff38 S11: 0000000000000000 T3: 0000000000000023
T4: 0000000000000006 T5: 000000000001869f T6: 00000000080dd138
backtrace:
backtrace 0: FP: 00000000080ffb20 RA: 0000000008005888
backtrace 1: FP: 00000000080ffb80 RA: 0000000008007046
backtrace 2: FP: 00000000080ffc50 RA: 00000000080024ec
backtrace 3: FP: 00000000080ffd00 RA: 00000000080028e0
backtrace 4: FP: 00000000080ffe00 RA: 0000000008002cf6
backtrace 5: FP: 00000000080fff30 RA: 0000000008002144
backtrace 6: FP: 0000000008100000 RA: 0000000008000178
Code: 6797 0002 b783 1ba7 050e 953e 6108 6422 (613c)
Looks like you just need a `config SPL_FRAMEPOINTER` entry in Kconfig.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] riscv: add backtrace support
2023-06-15 19:01 ` Bo Gan
@ 2023-06-16 8:55 ` Ben Dooks
0 siblings, 0 replies; 6+ messages in thread
From: Ben Dooks @ 2023-06-16 8:55 UTC (permalink / raw)
To: Bo Gan, Ben Dooks, u-boot; +Cc: rick
On 15/06/2023 20:01, Bo Gan wrote:
> On 6/14/23 10:15 AM, Ben Dooks wrote:
>> On 14/06/2023 07:25, Bo Gan wrote:
>>> On 5/25/23 4:05 AM, Ben Dooks wrote:
>>>> On 15/05/2023 14:03, Ben Dooks wrote:
>>>>> When debugging, it is useful to have a backtrace to find
>>>>> out what is in the call stack as the previous function (RA)
>>>>> may not have been the culprit.
>>>>>
>>>>> Since this adds size to the build, do not add it by default
>>>>> and avoid putting it in the SPL build if not needed.
>>>>
>>>> Hi, has anyone had time to review this?
>>>>
>>>
>>> Hi Ben, this looks really useful. I'd like to use it in SPL,
>>> but I'm unable to enable CONFIG_SPL_FRAMEPOINTER=y. It's likely
>>> that you need to add a SPL_FRAMEPOINTER entry to Kconfig as well.
>>
>> I will have a look at this, but testing may not be easy as the
>> setup we're using has limited SPL space.
>>
>
> I did a hack (duplicate `config FRAMEPOINTER` to `config SPL_FRAMEPOINTER`)
>
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -63,6 +63,24 @@ config SPL_SYS_DCACHE_OFF
> +config SPL_FRAMEPOINTER
> + bool "Build with frame pointer for stack unwinding"
I think this should have been "Build SPL with...
> + depends on SPL
> + help
> + Choose this option to use the frame pointer so the stack can be
> + unwound if needed. This is useful for tracing where faults came
> + from as the source may be several functions back
> +
> + If you say Y here, then the code size will be increased due to
> + having to store the fp.
> +
>
> This is sufficient for enabling CONFIG_SPL_FRAMEPOINTER=y. With this, I
> tested SPL on JH7110 (VisionFive 2), and observed your patch's working:
Ok, thank you for testing.
If I put this into v2 of the patch, do you want crediting with
a co-author tag?
> Unhandled exception: Load access fault
> EPC: 000000000800335e RA: 00000000080033e8 TVAL: 0000000000000040
>
> SP: 00000000080ff5b0 GP: 00000000080dbda0 TP: 0000000000000000
> T0: 00000000080ffb20 T1: 0000000000000020 T2: 0000000000000000
> S0: 00000000080ffb20 S1: 00000000080ff6d0 A0: 0000000000000000
> A1: 00000000080ff6d0 A2: 0000000008020d83 A3: 00000000080ffb20
> A4: 0000000000000025 A5: 0000000008040218 A6: 0000000000000020
> A7: 0000000008000000 S2: 0000000000000013 S3: 0000000000000001
> S4: 0000000040000000 S5: 0000000000000001 S6: 000000000000000a
> S7: 00000000080ffb88 S8: 0000000002000000 S9: 000000000801bc60
> S10: 00000000080fff38 S11: 0000000000000000 T3: 0000000000000023
> T4: 0000000000000006 T5: 000000000001869f T6: 00000000080dd138
> backtrace:
> backtrace 0: FP: 00000000080ffb20 RA: 0000000008005888
> backtrace 1: FP: 00000000080ffb80 RA: 0000000008007046
> backtrace 2: FP: 00000000080ffc50 RA: 00000000080024ec
> backtrace 3: FP: 00000000080ffd00 RA: 00000000080028e0
> backtrace 4: FP: 00000000080ffe00 RA: 0000000008002cf6
> backtrace 5: FP: 00000000080fff30 RA: 0000000008002144
> backtrace 6: FP: 0000000008100000 RA: 0000000008000178
>
> Code: 6797 0002 b783 1ba7 050e 953e 6108 6422 (613c)
>
> Looks like you just need a `config SPL_FRAMEPOINTER` entry in Kconfig.
>
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-16 8:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-15 13:03 [PATCH] riscv: add backtrace support Ben Dooks
2023-05-25 11:05 ` Ben Dooks
2023-06-14 6:25 ` Bo Gan
2023-06-14 17:15 ` Ben Dooks
2023-06-15 19:01 ` Bo Gan
2023-06-16 8:55 ` Ben Dooks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox