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

[Julien Grall <julien@xxxxxxx>]

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