Re: [PATCH 03/14] ARM: add exception handling support for PBL

[Marco Felsch <m.felsch@pengutronix.de>]

On 25-06-27, Sascha Hauer wrote:
> On Fri, Jun 27, 2025 at 05:45:09PM +0200, Marco Felsch wrote:
> > Hi Sascha,
> > 
> > On 25-06-27, Sascha Hauer wrote:
> > > Exception handling in PBL can be very useful for debugging PBL code.
> > > This patch adds support for it.
> > > 
> > > This is currently only implemented for ARMv7 and ARMv8. Only on these
> > > architectures we can tell the CPU where the exception table is. On ARMv6
> > > and older we would have to copy the exception table either to 0x0 or
> > > 0xffff0000. Not all SoCs have writable memory at these locations, so we
> > > would have to utilize the MMU to map writable memory there. We are not
> > > there yet, so for now skip exception handling support on these older
> > > architectures.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  arch/arm/Kconfig                   | 10 ++++++++++
> > >  arch/arm/cpu/Makefile              |  1 +
> > >  arch/arm/cpu/interrupts_32.c       | 14 +++++++++++++-
> > >  arch/arm/cpu/interrupts_64.c       | 10 +++++++++-
> > >  arch/arm/cpu/uncompress.c          |  2 ++
> > >  arch/arm/include/asm/barebox-arm.h |  8 ++++++++
> > >  arch/arm/lib/pbl.lds.S             |  4 ++++
> > >  7 files changed, 47 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 0800b15d784ca0ab975cf7ceb2f7b47ed10643b1..eaccee9f2f7a128a820e1c55fba816ec5ac4c02d 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -378,6 +378,16 @@ config ARM_EXCEPTIONS
> > >  	bool "enable arm exception handling support"
> > >  	default y
> > >  
> > > +config ARM_EXCEPTIONS_PBL
> > > +	select ARCH_HAS_DATA_ABORT_MASK_PBL
> > > +	depends on CPU_V7 || CPU_V8
> > > +	bool "enable arm exception handling support in PBL"
> > > +	help
> > > +	  Say yes here to enable exception handling in PBL. Note that the exception
> > > +	  table has to be initialized by calling arm_pbl_init_exceptions(). This is
> > > +	  done in barebox_pbl_start(). If you need exception handling earlier then
> > > +	  you have to call arm_pbl_init_exceptions() earlier from your board code.
> > > +
> > >  config ARM_UNWIND
> > >  	bool "enable stack unwinding support"
> > >  	depends on AEABI
> > > diff --git a/arch/arm/cpu/Makefile b/arch/arm/cpu/Makefile
> > > index 39e59c2a2f733d668d57a2f28bdd99f69a016229..9550592796702759f950a3bc4de385100ef2b2e8 100644
> > > --- a/arch/arm/cpu/Makefile
> > > +++ b/arch/arm/cpu/Makefile
> > > @@ -3,6 +3,7 @@
> > >  obj-pbl-y += cpu.o
> > >  
> > >  obj-$(CONFIG_ARM_EXCEPTIONS) += exceptions_$(S64_32).o interrupts_$(S64_32).o
> > > +pbl-$(CONFIG_ARM_EXCEPTIONS_PBL) += exceptions_$(S64_32).o interrupts_$(S64_32).o
> > >  obj-$(CONFIG_MMU) += mmu-common.o
> > >  obj-pbl-$(CONFIG_MMU) += mmu_$(S64_32).o
> > >  obj-$(CONFIG_MMU) += dma_$(S64_32).o
> > > diff --git a/arch/arm/cpu/interrupts_32.c b/arch/arm/cpu/interrupts_32.c
> > > index 623efb3966f0c34632e678d9e1edf2b6affcb4c5..cd503b38eeea551f27bbab0663bbfabdda7ffeea 100644
> > > --- a/arch/arm/cpu/interrupts_32.c
> > > +++ b/arch/arm/cpu/interrupts_32.c
> > > @@ -12,6 +12,7 @@
> > >  #include <asm/ptrace.h>
> > >  #include <asm/barebox-arm.h>
> > >  #include <asm/unwind.h>
> > > +#include <asm/system_info.h>
> > >  #include <init.h>
> > >  
> > >  /* Avoid missing prototype warning, called from assembly */
> > > @@ -61,7 +62,7 @@ void show_regs (struct pt_regs *regs)
> > >  		fast_interrupts_enabled (regs) ? "on" : "off",
> > >  		processor_modes[processor_mode (regs)],
> > >  		thumb_mode (regs) ? " (T)" : "");
> > > -#ifdef CONFIG_ARM_UNWIND
> > > +#if defined CONFIG_ARM_UNWIND && IN_PROPER
> > >  	unwind_backtrace(regs);
> > >  #endif
> > >  }
> > > @@ -181,3 +182,14 @@ int data_abort_unmask(void)
> > >  
> > >  	return arm_data_abort_occurred != 0;
> > >  }
> > > +
> > > +#if IS_ENABLED(CONFIG_ARM_EXCEPTIONS_PBL)
> > > +void arm_pbl_init_exceptions(void)
> > > +{
> > > +	if (cpu_architecture() < CPU_ARCH_ARMv7)
> > > +		return;
> > > +
> > > +	set_vbar((unsigned long)__exceptions_start);
> > > +	arm_fixup_vectors();
> > > +}
> > > +#endif
> > > diff --git a/arch/arm/cpu/interrupts_64.c b/arch/arm/cpu/interrupts_64.c
> > > index 4d4ef2bab88ef46a7be1cd4add8f3e51423a283b..574ab6a7ec220d2239b6e27c05426e2d4c67d426 100644
> > > --- a/arch/arm/cpu/interrupts_64.c
> > > +++ b/arch/arm/cpu/interrupts_64.c
> > > @@ -88,7 +88,8 @@ static void __noreturn do_exception(struct pt_regs *pt_regs)
> > >  {
> > >  	show_regs(pt_regs);
> > >  
> > > -	unwind_backtrace(pt_regs);
> > > +	if (IN_PROPER)
> > > +		unwind_backtrace(pt_regs);
> > >  
> > >  	panic_no_stacktrace("panic: unhandled exception");
> > >  }
> > > @@ -226,3 +227,10 @@ static int aarch64_init_vectors(void)
> > >  	return 0;
> > >  }
> > >  core_initcall(aarch64_init_vectors);
> > > +
> > > +#if IS_ENABLED(CONFIG_ARM_EXCEPTIONS_PBL)
> > > +void arm_pbl_init_exceptions(void)
> > > +{
> > > +	aarch64_init_vectors();
> > > +}
> > > +#endif
> > > diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
> > > index 4657a4828e67e1b0acfa9dec3aef33bc4c525468..4529ef5e3821e5b31a3673de6285d2f37e0ecba2 100644
> > > --- a/arch/arm/cpu/uncompress.c
> > > +++ b/arch/arm/cpu/uncompress.c
> > > @@ -63,6 +63,8 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
> > >  
> > >  	pr_debug("memory at 0x%08lx, size 0x%08lx\n", membase, memsize);
> > >  
> > > +	arm_pbl_init_exceptions();
> > > +
> > >  	if (IS_ENABLED(CONFIG_MMU))
> > >  		mmu_early_enable(membase, memsize);
> > >  	else if (IS_ENABLED(CONFIG_ARMV7R_MPU))
> > > diff --git a/arch/arm/include/asm/barebox-arm.h b/arch/arm/include/asm/barebox-arm.h
> > > index 7d35e88c812393d45e331f238baecfa91cbbe299..3ab442bd373e50a84b26145395e42879fc96757f 100644
> > > --- a/arch/arm/include/asm/barebox-arm.h
> > > +++ b/arch/arm/include/asm/barebox-arm.h
> > > @@ -52,6 +52,14 @@ static inline void arm_fixup_vectors(void)
> > >  }
> > >  #endif
> > >  
> > > +#if IS_ENABLED(CONFIG_ARM_EXCEPTIONS_PBL)
> > > +void arm_pbl_init_exceptions(void);
> > > +#else
> > > +static inline void arm_pbl_init_exceptions(void)
> > > +{
> > > +}
> > > +#endif
> > > +
> > >  void *barebox_arm_boot_dtb(void);
> > >  
> > >  /*
> > > diff --git a/arch/arm/lib/pbl.lds.S b/arch/arm/lib/pbl.lds.S
> > > index dad37c9e9bca98beb4f34360fa53a0421662f03c..9c51f5eb3a3d8256752a78e03fed851c84d92edb 100644
> > > --- a/arch/arm/lib/pbl.lds.S
> > > +++ b/arch/arm/lib/pbl.lds.S
> > > @@ -52,6 +52,10 @@ SECTIONS
> > >  		__bare_init_start = .;
> > >  		*(.text_bare_init*)
> > >  		__bare_init_end = .;
> > > +		. = ALIGN(0x20);
> > > +		__exceptions_start = .;
> > > +		KEEP(*(.text_exceptions*))
> > > +		__exceptions_stop = .;
> > 
> > Nit: We only need this in case of CONFIG_CPU_64, right?
> 
> s/CONFIG_CPU_64/CONFIG_CPU_32/
> 
> Yes, right.

right /o\

> > Maybe I
> > overlooked it but the only user of this section is
> > arch/arm/cpu/exceptions_32.S. Not sure why arch/arm/lib64/barebox.lds.S
> > has this section too.
> 
> Likely because the initial arm64 linker script started as a copy from
> the arm32 linker script.

Yes, I guess so too.

> I'll prepare a patch to remove the section from arch/arm/lib64/barebox.lds.S
> because it's indeed unused.
> 
> In arch/arm/lib/pbl.lds.S we could add an #ifdef CONFIG_CPU_64 around the

#ifdef CONFIG_CPU_32. yes.

> section, but it doesn't make a difference, the section will be empty
> anyway.

Sure, just the ALIGN() may add some more bytes but this can be ignored.

At least a comment would be nice :)

Regards,
  Marco


> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>