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
