On 08/02/2025 01:02, Andrew Cooper wrote:
>
>
> While fixing some common/arch boundaries for UBSAN support on other
> architectures, the following debugging patch:
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index c1f2d1b89d43..58d1d048d339 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -504,6 +504,8 @@ void asmlinkage __init start_xen(unsigned long
> fdt_paddr)
>
> system_state = SYS_STATE_active;
>
> + dump_execution_state();
> +
> for_each_domain( d )
> domain_unpause_by_systemcontroller(d);
>
> fails with:
>
> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
> (XEN) CPU0: Unexpected Trap: Undefined Instruction
> (XEN) ----[ Xen-4.20-rc arm32 debug=n Not tainted ]----
> (XEN) CPU: 0
> <snip>
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Undefined Instruction
> (XEN) ****************************************
>
> This is because the condition for init text is wrong. While there's nothing
> interesting from that point onwards in start_xen(), it's also wrong for any
> livepatch which brings in an adjusted BUG_FRAME().
>
> Use is_active_kernel_text() which is the correct test for this purpose, and is
> aware of init and livepatch regions too.
>
> Commit c8d4b6304a5e ("xen/arm: add support for run_in_exception_handler()"),
> made run_in_exception_handler() work, but didn't complete the TODO left in
> commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, to make
> ARM consistent with other architectures.
>
> Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON")
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
You should have mentioned that this patch requires [1] as a prerequisite.
Otherwise this patch fails to build on both arm64 and arm32 with UBSAN enabled.
[1]
https://lore.kernel.org/xen-devel/359347d3-9a5f-4672-98d6-4c497d960...@gmail.com/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d
With this handled:
Reviewed-by: Michal Orzel <michal.or...@amd.com>
As for taking this patch into 4.20, I don't think this qualifies as a serious
bug. At the same time I don't see how it could cause issues, so I'd be ok to
take it in. That said, at least one more Arm maintainer should take a vote.
~Michal
> ---
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Julien Grall <jul...@xen.org>
> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
> CC: Bertrand Marquis <bertrand.marq...@arm.com>
> CC: Michal Orzel <michal.or...@amd.com>
> CC: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
>
> Sample run going wrong:
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9078570105
>
> Sample run with dump_execution_state() working:
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9079185111
> ---
> xen/arch/arm/arm32/traps.c | 3 +--
> xen/arch/arm/include/asm/processor.h | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index a2fc1c22cbc9..b88d41811b49 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -36,8 +36,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs
> *regs)
> uint32_t pc = regs->pc;
> uint32_t instr;
>
> - if ( !is_kernel_text(pc) &&
> - (system_state >= SYS_STATE_active || !is_kernel_inittext(pc)) )
> + if ( !is_active_kernel_text(pc) )
> goto die;
>
> /* PC should be always a multiple of 4, as Xen is using ARM instruction
> set */
> diff --git a/xen/arch/arm/include/asm/processor.h
> b/xen/arch/arm/include/asm/processor.h
> index 60b587db697f..d80d44aeaa8f 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -577,8 +577,7 @@ void panic_PAR(uint64_t par);
> void show_registers(const struct cpu_user_regs *regs);
> void show_stack(const struct cpu_user_regs *regs);
>
> -//#define dump_execution_state()
> run_in_exception_handler(show_execution_state)
> -#define dump_execution_state() WARN()
> +#define dump_execution_state() run_in_exception_handler(show_execution_state)
>
> #define cpu_relax() barrier() /* Could yield? */
>
> --
> 2.39.5
>
