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
