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.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@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

Hi Andrew,

On 08/02/2025 00: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.

This was done on purpose. If you look at the current implementation of 
run_in_exception_handler(), it will clobber some registers.

With your patch #2, the function should only clobber one. It is a bit 
better, but it still not great. So I think we need to stick with WARN() 
on Arm (+ maybe a comment explaning why it is implemented differently).

Cheers,

-- 
Julien Grall

On 10/02/2025 9:23 pm, Julien Grall wrote:
> Hi Andrew,
>
> On 08/02/2025 00: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.
>
> This was done on purpose. If you look at the current implementation of
> run_in_exception_handler(), it will clobber some registers.
>
> With your patch #2, the function should only clobber one. It is a bit
> better, but it still not great. So I think we need to stick with
> WARN() on Arm (+ maybe a comment explaning why it is implemented
> differently).

I'm sorry but I don't follow.

run_in_exception_handler() only uses 1 register (after patch 2), but
it's fully described to the invoking context, so nothing is clobbered
from the compilers point of view.

Are you concerned about losing r0/x0 in the resulting trace?

I can certainly split the patch in half.  The
do_trap_undefined_instruction() change isn't related, although the
second hunk is needed for patch 3 to consolidate dump_execution_state()
across architectures.

~Andrew

Hi Andrew,

On 10/02/2025 22:23, Andrew Cooper wrote:
> On 10/02/2025 9:23 pm, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 08/02/2025 00: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.
>>
>> This was done on purpose. If you look at the current implementation of
>> run_in_exception_handler(), it will clobber some registers.
>>
>> With your patch #2, the function should only clobber one. It is a bit
>> better, but it still not great. So I think we need to stick with
>> WARN() on Arm (+ maybe a comment explaning why it is implemented
>> differently).
> 
> I'm sorry but I don't follow.
> 
> run_in_exception_handler() only uses 1 register (after patch 2), but
> it's fully described to the invoking context, so nothing is clobbered
> from the compilers point of view.

Maybe "clobbered" was the wrong word. I was comparing the existing 
implementation (WARN()) with your proposed one. You don't mention in the 
commit message that r0/x0 will be missing. It wasn't clear to me whether 
this was intended.

> 
> Are you concerned about losing r0/x0 in the resulting trace?

I think the consolidation is not a strong enough reason to end up losing 
some registers in the dump. See below a proposal.

> 
> I can certainly split the patch in half.  The
> do_trap_undefined_instruction() change isn't related, although the
> second hunk is needed for patch 3 to consolidate dump_execution_state()
> across architectures.

What about checking if the arch is already providing a 
dump_execution_state() helper? This should allow the consolidation until 
we managed to convert Arm over to the generic bug infrastructure.

Cheers,

-- 
Julien Grall

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.cooper3@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-4c497d960059@gmail.com/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d

With this handled:
Reviewed-by: Michal Orzel <michal.orzel@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 <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@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
>

On 10/02/2025 10:13 am, Orzel, Michal wrote:
>
> 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.cooper3@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-4c497d960059@gmail.com/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d

That is unintentional.

I'm going to split this patch in half, because it's clear that the
run_in_exception_handler() problems are more complicated than I expected.

The fix in do_trap_undefined_instruction() genuinely is entirely
independent of UBSAN.

~Andrew