Re: [PATCH for-4.20? 1/4] ARM32/traps: Fix do_trap_undefined_instruction()'s detection of kernel text

[Andrew Cooper <andrew.cooper3@citrix.com>]

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