[PATCH 0/4] Other fixes from UBSAN enablement

[PATCH 0/4] Other fixes from UBSAN enablement

[Andrew Cooper]

Patch 1 is a long outstanding bug, and hopefully still qualifies for 4.20 at
this point.

Patches 2 and 3 probably don't, seeing as I can't seem to get UBSAN working
for PPC (for which patch 3 at least would be a prerequisite to compile).  They
can wait until 4.21 now.

Andrew Cooper (4):
  ARM32/traps: Fix do_trap_undefined_instruction()'s detection of kernel text
  ARM: Fix register constraints in run_in_exception_handler()
  xen: Centralise the declaration of dump_execution_state()
  [BROKEN] PPC: Activate UBSAN in testing

 automation/gitlab-ci/build.yaml        | 3 +++
 xen/arch/arm/arm32/traps.c             | 3 +--
 xen/arch/arm/include/asm/bug.h         | 6 +++---
 xen/arch/arm/include/asm/processor.h   | 3 ---
 xen/arch/ppc/Kconfig                   | 1 +
 xen/arch/ppc/stubs.c                   | 2 +-
 xen/arch/riscv/include/asm/processor.h | 2 --
 xen/arch/x86/include/asm/processor.h   | 1 -
 xen/common/ubsan/ubsan.c               | 1 -
 xen/include/xen/bug.h                  | 3 +++
 xen/include/xen/kernel.h               | 2 --
 11 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.39.5

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

[Andrew Cooper]

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

[PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Andrew Cooper]

Right now, run_in_exception_handler() takes an input in an arbitrary register,
and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
wrong regsiter.

Instead, use `register asm()` which is the normal way of tying register
constraints to exact registers.

Bloat-o-meter reports:

  ARM64:
    Function                                     old     new   delta
    dump_registers                               356     348      -8

  ARM32:
    ns16550_poll                                  52      48      -4
    dump_registers                               432     428      -4

The other instruction dropped in ARM64's dump_registers() is an alignment nop.

No functional change.

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>
---
 xen/arch/arm/include/asm/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index cacaf014ab09..8bf71587bea1 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -59,15 +59,15 @@ struct bug_frame {
  * be called function in a fixed register.
  */
 #define  run_in_exception_handler(fn) do {                                  \
-    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"                            \
-         "1:"BUG_INSTR"\n"                                                  \
+    register unsigned long _fn asm (STR(BUG_FN_REG)) = (unsigned long)(fn); \
+    asm ("1:"BUG_INSTR"\n"                                                  \
          ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","       \
          "             \"a\", %%progbits\n"                                 \
          "2:\n"                                                             \
          ".p2align 2\n"                                                     \
          ".long (1b - 2b)\n"                                                \
          ".long 0, 0, 0\n"                                                  \
-         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );             \
+         ".popsection" :: "r" (_fn) );                                      \
 } while (0)
 
 #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
-- 
2.39.5

[PATCH 3/4] xen: Centralise the declaration of dump_execution_state()

[Andrew Cooper]

Three architectures have an identical dump_execution_state(), and PPC has a
stub for show_execution_state() that just isn't wired up yet.

show_execution_state() is declared in a common header, meaning that
dump_execution_state() really ought to be too.  Move them both into xen/bug.h
as they're tightly tied to run_in_exception_handler().  Drop the include of
xen/kernel.h from ubsan.c which was required reviously for RISC-V to compile.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@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>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 xen/arch/arm/include/asm/processor.h   | 2 --
 xen/arch/riscv/include/asm/processor.h | 2 --
 xen/arch/x86/include/asm/processor.h   | 1 -
 xen/common/ubsan/ubsan.c               | 1 -
 xen/include/xen/bug.h                  | 3 +++
 xen/include/xen/kernel.h               | 2 --
 6 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index d80d44aeaa8f..f2c4d990c71c 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -577,8 +577,6 @@ 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 cpu_relax() barrier() /* Could yield? */
 
 /* All a bit UP for the moment */
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index 39696fb58dc6..90b800956303 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -91,8 +91,6 @@ static inline void sfence_vma(void)
     asm volatile ( "sfence.vma" ::: "memory" );
 }
 
-#define dump_execution_state() run_in_exception_handler(show_execution_state)
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* ASM__RISCV__PROCESSOR_H */
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index d247ef8dd226..c2eafaecfd40 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -405,7 +405,6 @@ static always_inline void rep_nop(void)
 void show_code(const struct cpu_user_regs *regs);
 void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs);
 void show_registers(const struct cpu_user_regs *regs);
-#define dump_execution_state() run_in_exception_handler(show_execution_state)
 void show_page_walk(unsigned long addr);
 void noreturn fatal_trap(const struct cpu_user_regs *regs, bool show_remote);
 
diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index e99370322b44..a96153c08078 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -11,7 +11,6 @@
  */
 
 #include <xen/bitops.h>
-#include <xen/kernel.h>
 #include <xen/lib.h>
 #include <xen/percpu.h>
 #include <xen/spinlock.h>
diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
index 99814c4bef36..2325a46e7f61 100644
--- a/xen/include/xen/bug.h
+++ b/xen/include/xen/bug.h
@@ -155,6 +155,9 @@ int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
 
 #endif /* CONFIG_GENERIC_BUG_FRAME */
 
+void cf_check show_execution_state(const struct cpu_user_regs *regs);
+#define dump_execution_state() run_in_exception_handler(show_execution_state)
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __XEN_BUG_H__ */
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index c5b6cc977772..57a1ef4e17b7 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -94,10 +94,8 @@ bool is_active_kernel_text(unsigned long addr);
 extern const char xen_config_data[];
 extern const unsigned int xen_config_data_size;
 
-struct cpu_user_regs;
 struct vcpu;
 
-void cf_check show_execution_state(const struct cpu_user_regs *regs);
 void vcpu_show_execution_state(struct vcpu *v);
 
 #endif /* _LINUX_KERNEL_H */
-- 
2.39.5

[PATCH 4/4] [BROKEN] PPC: Activate UBSAN in testing

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Shawn Anastasio <sanastasio@raptorengineering.com>

This compiles, but something is up with the console and nothing useful comes
out.

Sample:
  https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9079440897
---
 automation/gitlab-ci/build.yaml | 3 +++
 xen/arch/ppc/Kconfig            | 1 +
 xen/arch/ppc/stubs.c            | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 35e224366f62..6a2e491534d3 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -352,6 +352,9 @@ debian-12-ppc64le-gcc-debug:
     CONTAINER: debian:12-ppc64le
     KBUILD_DEFCONFIG: ppc64_defconfig
     HYPERVISOR_ONLY: y
+    EXTRA_XEN_CONFIG: |
+      CONFIG_UBSAN=y
+      CONFIG_UBSAN_FATAL=y
 
 debian-12-riscv64-gcc-debug:
   extends: .gcc-riscv64-cross-build-debug
diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
index 6db575a48d34..917f5d53a6c3 100644
--- a/xen/arch/ppc/Kconfig
+++ b/xen/arch/ppc/Kconfig
@@ -2,6 +2,7 @@ config PPC
 	def_bool y
 	select FUNCTION_ALIGNMENT_4B
 	select HAS_DEVICE_TREE
+	select HAS_UBSAN
 	select HAS_VMAP
 
 config PPC64
diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
index fff82f5cf3cc..671e71aa0a60 100644
--- a/xen/arch/ppc/stubs.c
+++ b/xen/arch/ppc/stubs.c
@@ -47,7 +47,7 @@ void send_timer_event(struct vcpu *v)
 
 void show_execution_state(const struct cpu_user_regs *regs)
 {
-    BUG_ON("unimplemented");
+    printk("TODO: Implement show_execution_state(regs)\n");
 }
 
 void arch_hypercall_tasklet_result(struct vcpu *v, long res)
-- 
2.39.5

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]


On 2/8/25 1:02 AM, Andrew Cooper wrote:
> Right now, run_in_exception_handler() takes an input in an arbitrary register,
> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
> wrong regsiter.

Probably, we should give a chance for the patch which suggests to use GENERIC_BUG_FRAME:
   https://lore.kernel.org/xen-devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/

~ Oleksii

> Instead, use `register asm()` which is the normal way of tying register
> constraints to exact registers.
>
> Bloat-o-meter reports:
>
>    ARM64:
>      Function                                     old     new   delta
>      dump_registers                               356     348      -8
>
>    ARM32:
>      ns16550_poll                                  52      48      -4
>      dump_registers                               432     428      -4
>
> The other instruction dropped in ARM64's dump_registers() is an alignment nop.
>
> No functional change.
>
> 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>
> ---
>   xen/arch/arm/include/asm/bug.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index cacaf014ab09..8bf71587bea1 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -59,15 +59,15 @@ struct bug_frame {
>    * be called function in a fixed register.
>    */
>   #define  run_in_exception_handler(fn) do {                                  \
> -    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"                            \
> -         "1:"BUG_INSTR"\n"                                                  \
> +    register unsigned long _fn asm (STR(BUG_FN_REG)) = (unsigned long)(fn); \
> +    asm ("1:"BUG_INSTR"\n"                                                  \
>            ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","       \
>            "             \"a\", %%progbits\n"                                 \
>            "2:\n"                                                             \
>            ".p2align 2\n"                                                     \
>            ".long (1b - 2b)\n"                                                \
>            ".long 0, 0, 0\n"                                                  \
> -         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );             \
> +         ".popsection" :: "r" (_fn) );                                      \
>   } while (0)
>   
>   #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")

[-- Attachment #2: Type: text/html, Size: 4050 bytes --]

Re: [PATCH 3/4] xen: Centralise the declaration of dump_execution_state()

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 4649 bytes --]


On 2/8/25 1:02 AM, Andrew Cooper wrote:
> Three architectures have an identical dump_execution_state(), and PPC has a
> stub for show_execution_state() that just isn't wired up yet.
>
> show_execution_state() is declared in a common header, meaning that
> dump_execution_state() really ought to be too.  Move them both into xen/bug.h
> as they're tightly tied to run_in_exception_handler().  Drop the include of
> xen/kernel.h from ubsan.c which was required reviously for RISC-V to compile.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich<JBeulich@suse.com>
> CC: Roger Pau Monné<roger.pau@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>
> CC: Shawn Anastasio<sanastasio@raptorengineering.com>
> ---
>   xen/arch/arm/include/asm/processor.h   | 2 --
>   xen/arch/riscv/include/asm/processor.h | 2 --

Reviewed-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.

Thanks.

~ Oleksii

>   xen/arch/x86/include/asm/processor.h   | 1 -
>   xen/common/ubsan/ubsan.c               | 1 -
>   xen/include/xen/bug.h                  | 3 +++
>   xen/include/xen/kernel.h               | 2 --
>   6 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
> index d80d44aeaa8f..f2c4d990c71c 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -577,8 +577,6 @@ 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 cpu_relax() barrier() /* Could yield? */
>   
>   /* All a bit UP for the moment */
> diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
> index 39696fb58dc6..90b800956303 100644
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -91,8 +91,6 @@ static inline void sfence_vma(void)
>       asm volatile ( "sfence.vma" ::: "memory" );
>   }
>   
> -#define dump_execution_state() run_in_exception_handler(show_execution_state)
> -
>   #endif /* __ASSEMBLY__ */
>   
>   #endif /* ASM__RISCV__PROCESSOR_H */
> diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> index d247ef8dd226..c2eafaecfd40 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -405,7 +405,6 @@ static always_inline void rep_nop(void)
>   void show_code(const struct cpu_user_regs *regs);
>   void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs);
>   void show_registers(const struct cpu_user_regs *regs);
> -#define dump_execution_state() run_in_exception_handler(show_execution_state)
>   void show_page_walk(unsigned long addr);
>   void noreturn fatal_trap(const struct cpu_user_regs *regs, bool show_remote);
>   
> diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
> index e99370322b44..a96153c08078 100644
> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -11,7 +11,6 @@
>    */
>   
>   #include <xen/bitops.h>
> -#include <xen/kernel.h>
>   #include <xen/lib.h>
>   #include <xen/percpu.h>
>   #include <xen/spinlock.h>
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> index 99814c4bef36..2325a46e7f61 100644
> --- a/xen/include/xen/bug.h
> +++ b/xen/include/xen/bug.h
> @@ -155,6 +155,9 @@ int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
>   
>   #endif /* CONFIG_GENERIC_BUG_FRAME */
>   
> +void cf_check show_execution_state(const struct cpu_user_regs *regs);
> +#define dump_execution_state() run_in_exception_handler(show_execution_state)
> +
>   #endif /* !__ASSEMBLY__ */
>   
>   #endif /* __XEN_BUG_H__ */
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index c5b6cc977772..57a1ef4e17b7 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -94,10 +94,8 @@ bool is_active_kernel_text(unsigned long addr);
>   extern const char xen_config_data[];
>   extern const unsigned int xen_config_data_size;
>   
> -struct cpu_user_regs;
>   struct vcpu;
>   
> -void cf_check show_execution_state(const struct cpu_user_regs *regs);
>   void vcpu_show_execution_state(struct vcpu *v);
>   
>   #endif /* _LINUX_KERNEL_H */

[-- Attachment #2: Type: text/html, Size: 5869 bytes --]

Re: [PATCH 3/4] xen: Centralise the declaration of dump_execution_state()

[Jan Beulich]

On 08.02.2025 01:02, Andrew Cooper wrote:
> Three architectures have an identical dump_execution_state(), and PPC has a
> stub for show_execution_state() that just isn't wired up yet.
> 
> show_execution_state() is declared in a common header, meaning that
> dump_execution_state() really ought to be too.  Move them both into xen/bug.h
> as they're tightly tied to run_in_exception_handler().

Hmm, show_execution_state() certainly has wider use than just with
run_in_exception_handler(). I don't think kernel.h was a great home for its
decl, but I'm thinking the same of bug.h. Nevertheless (not the least short
of having any better suggestion) ...

>  Drop the include of
> xen/kernel.h from ubsan.c which was required reviously for RISC-V to compile.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Orzel, Michal]

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
> 

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Orzel, Michal]

On 08/02/2025 01:02, Andrew Cooper wrote:
> 
> 
> Right now, run_in_exception_handler() takes an input in an arbitrary register,
> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
> wrong regsiter.
> 
> Instead, use `register asm()` which is the normal way of tying register
> constraints to exact registers.
> 
> Bloat-o-meter reports:
> 
>   ARM64:
>     Function                                     old     new   delta
>     dump_registers                               356     348      -8
> 
>   ARM32:
>     ns16550_poll                                  52      48      -4
>     dump_registers                               432     428      -4
> 
> The other instruction dropped in ARM64's dump_registers() is an alignment nop.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

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

[Julien Grall]

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

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Julien Grall]

Hi,

On 08/02/2025 00:02, Andrew Cooper wrote:
> Right now, run_in_exception_handler() takes an input in an arbitrary register,
> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
> wrong regsiter.

Just to confirm, you mean, the compiler is not clever enough to notice 
that the value should be in the register BUG_FN_REG and therefore, two 
registers will be clobbered. Is that correct?

 > > Instead, use `register asm()` which is the normal way of tying register
> constraints to exact registers.
> 
> Bloat-o-meter reports:
> 
>    ARM64:
>      Function                                     old     new   delta
>      dump_registers                               356     348      -8
> 
>    ARM32:
>      ns16550_poll                                  52      48      -4
>      dump_registers                               432     428      -4
> 
> The other instruction dropped in ARM64's dump_registers() is an alignment nop.
> 
> No functional change.
> 
> 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>
> ---
>   xen/arch/arm/include/asm/bug.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index cacaf014ab09..8bf71587bea1 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -59,15 +59,15 @@ struct bug_frame {
>    * be called function in a fixed register.
>    */
>   #define  run_in_exception_handler(fn) do {                                  \
> -    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"                            \
> -         "1:"BUG_INSTR"\n"                                                  \
> +    register unsigned long _fn asm (STR(BUG_FN_REG)) = (unsigned long)(fn); \
> +    asm ("1:"BUG_INSTR"\n"                                                  \
>            ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","       \
>            "             \"a\", %%progbits\n"                                 \
>            "2:\n"                                                             \
>            ".p2align 2\n"                                                     \
>            ".long (1b - 2b)\n"                                                \
>            ".long 0, 0, 0\n"                                                  \
> -         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );             \
> +         ".popsection" :: "r" (_fn) );                                      \
>   } while (0)
>   
>   #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")

-- 
Julien Grall

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Julien Grall]

On 10/02/2025 09:21, Oleksii Kurochko wrote:
> 
> On 2/8/25 1:02 AM, Andrew Cooper wrote:
>> Right now, run_in_exception_handler() takes an input in an arbitrary register,
>> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
>> wrong regsiter.
> 
> Probably, we should give a chance for the patch which suggests to use GENERIC_BUG_FRAME:
>    https://lore.kernel.org/xen- 
> devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/

That would be the ideal if someone has time for it. Otherwise, patch #3 
needs to be modified (see my answer on patch #2).

But I would also be ok with this as a stop-gap for the time being.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Andrew Cooper]

On 10/02/2025 9:29 pm, Julien Grall wrote:
> Hi,
>
> On 08/02/2025 00:02, Andrew Cooper wrote:
>> Right now, run_in_exception_handler() takes an input in an arbitrary
>> register,
>> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in
>> the
>> wrong regsiter.
>
> Just to confirm, you mean, the compiler is not clever enough to notice
> that the value should be in the register BUG_FN_REG and therefore, two
> registers will be clobbered. Is that correct?

Not quite.

The clobbered register set is always disjoint from inputs and outputs,
so the combination of one clobbered + one input always means two
different registers.

For "here's an input but it gets modified", you need to express that as
an output into a variable which isn't subsequently used.

For ARM, that is best spelt "+r" (foo) so it can also be used with
register asm() to tie to a single register.  On x86, you can use "=a"
(tmp) : "a" (input).  In principle you can do it with named parameters,
so [fn] "=r" (tmp) : "[fn]" (input) I believe works too.

Here is a contrived example https://godbolt.org/z/WjqTKjWWb showing how
the output (discard only) is forced into r0, causing the compiler to
copy a into r3 around the asm block.  Notice that GCC and Clang pick the
input operand differently, as both r0 and r3 are valid candidates in
this case.


However, for run_in_exception_handler(), "fn" isn't even modified
(AFAICT), so it's correct to describe it as an input only.

~Andrew

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Andrew Cooper]

On 10/02/2025 9:31 pm, Julien Grall wrote:
>
>
> On 10/02/2025 09:21, Oleksii Kurochko wrote:
>>
>> On 2/8/25 1:02 AM, Andrew Cooper wrote:
>>> Right now, run_in_exception_handler() takes an input in an arbitrary
>>> register,
>>> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn
>>> in the
>>> wrong regsiter.
>>
>> Probably, we should give a chance for the patch which suggests to use
>> GENERIC_BUG_FRAME:
>>    https://lore.kernel.org/xen-
>> devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/
>
> That would be the ideal if someone has time for it. Otherwise, patch
> #3 needs to be modified (see my answer on patch #2).
>
> But I would also be ok with this as a stop-gap for the time being.

Getting ARM onto GENERIC_BUG_FRAME would definitely be best all around,
but that is an almost-2-year-old patch with an open "it doesn't compile
on ARM32" issue.

I presume that all which is wanted is *a* solution that compiles (and
works) everywhere we support?

~Andrew

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

[Julien Grall]

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

Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Julien Grall]

Hi Andrew,

On 10/02/2025 22:41, Andrew Cooper wrote:
> On 10/02/2025 9:31 pm, Julien Grall wrote:
>>
>>
>> On 10/02/2025 09:21, Oleksii Kurochko wrote:
>>>
>>> On 2/8/25 1:02 AM, Andrew Cooper wrote:
>>>> Right now, run_in_exception_handler() takes an input in an arbitrary
>>>> register,
>>>> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn
>>>> in the
>>>> wrong regsiter.
>>>
>>> Probably, we should give a chance for the patch which suggests to use
>>> GENERIC_BUG_FRAME:
>>>     https://lore.kernel.org/xen-
>>> devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/
>>
>> That would be the ideal if someone has time for it. Otherwise, patch
>> #3 needs to be modified (see my answer on patch #2).
>>
>> But I would also be ok with this as a stop-gap for the time being.
> 
> Getting ARM onto GENERIC_BUG_FRAME would definitely be best all around,
> but that is an almost-2-year-old patch with an open "it doesn't compile
> on ARM32" issue.
> 
> I presume that all which is wanted is *a* solution that compiles (and
> works) everywhere we support?

Correct. Looking at the previous e-mail, it sounds like the patch was 
meant to work but it wasn't tested with older compilers and the commit 
message needed some rewording to mention what was tested.

Cheers,

-- 
Julien Grall

Re: [PATCH 4/4] [BROKEN] PPC: Activate UBSAN in testing

[Shawn Anastasio]

Hi Andrew,

On 2/7/25 6:02 PM, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
> 
> This compiles, but something is up with the console and nothing useful comes
> out.

I tracked this down to ubsan tripping due to an unaligned access in
opal.c, before the serial console is set up. I'll be sending a patch set
soon to the ML with the fix for this to enable ubsan on PPC.

Thanks,
Shawn