[PATCH 0/7] x86/ubsan: fix ubsan on clang + code fixes

[PATCH 0/7] x86/ubsan: fix ubsan on clang + code fixes

[Roger Pau Monne]

Hello,

This started as a series to fix UBSAN when using clang, and the first 2
patches do fix that.  Patch 3 fix an issue when using UBSAN on gcc 12.
Patch 4 improves the reporting of clang UB pointer arithmetics.

Finally patches 5 and 6 fix bugs in the code highlighted by the clang UB
pointer arithmetic detection: offset additions to NULL pointers and too
early usage of ioremap_wc().

Patch 7 was the original goal of the series: be able to enable UBSAN for
randconfig.

Thanks, Roger.

Roger Pau Monne (7):
  xen/ubsan: provide helper for clang's -fsanitize=function
  x86/wait: prevent duplicated assembly labels
  x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST
  xen/ubsan: expand pointer overflow message printing
  x86/ioremap: prevent additions against the NULL pointer
  x86/vga: fix mapping of the VGA text buffer
  kconfig/randconfig: enable UBSAN for randconfig

 xen/Kconfig                        |   4 ++
 xen/Kconfig.debug                  |   2 +-
 xen/arch/x86/boot/x86_64.S         |  10 ++-
 xen/arch/x86/dmi_scan.c            |   7 +-
 xen/arch/x86/include/asm/config.h  |   5 ++
 xen/arch/x86/mm.c                  |   6 +-
 xen/arch/x86/setup.c               |   2 +-
 xen/common/ubsan/ubsan.c           |  31 +++++++-
 xen/common/ubsan/ubsan.h           |   5 ++
 xen/common/wait.c                  | 111 ++++++++++++++++++++---------
 xen/drivers/video/vga.c            |  11 ++-
 xen/tools/kconfig/allrandom.config |   1 -
 12 files changed, 144 insertions(+), 51 deletions(-)

-- 
2.48.1

[PATCH 1/7] xen/ubsan: provide helper for clang's -fsanitize=function

[Roger Pau Monne]

clang's -fsanitize=function relies on the presence of
__ubsan_handle_function_type_mismatch() to print the detection of indirect
calls of a function through a function pointer of the wrong type.

Implement the helper, inspired on the llvm ubsan lib implementation.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/ubsan/ubsan.c | 16 ++++++++++++++++
 xen/common/ubsan/ubsan.h |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index e99370322b44..7ebe4bfc14dc 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -546,3 +546,19 @@ void __ubsan_handle_invalid_builtin(struct invalid_builtin_data *data)
 
 	ubsan_epilogue(&flags);
 }
+
+void __ubsan_handle_function_type_mismatch(
+	struct function_type_mismatch_data *data, unsigned long val)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	pr_err("call to function %ps through pointer to incorrect function type %s\n",
+		(void *)val, data->type->type_name);
+
+	ubsan_epilogue(&flags);
+}
diff --git a/xen/common/ubsan/ubsan.h b/xen/common/ubsan/ubsan.h
index 9c7f3b9b6c07..8987f9d45397 100644
--- a/xen/common/ubsan/ubsan.h
+++ b/xen/common/ubsan/ubsan.h
@@ -95,6 +95,11 @@ enum {
 	kind_clz,
 };
 
+struct function_type_mismatch_data {
+	struct source_location location;
+	struct type_descriptor *type;
+};
+
 #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
 typedef __int128 s_max;
 typedef unsigned __int128 u_max;
-- 
2.48.1

Re: [PATCH 1/7] xen/ubsan: provide helper for clang's -fsanitize=function

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> clang's -fsanitize=function relies on the presence of
> __ubsan_handle_function_type_mismatch() to print the detection of indirect
> calls of a function through a function pointer of the wrong type.
>
> Implement the helper, inspired on the llvm ubsan lib implementation.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

It's weird, but we're now ahead of Linux by two sanitisers (this, and
invalid_builtin visible in context).

~Andrew

[PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Roger Pau Monne]

When enabling UBSAN with clang, the following error is triggered during the
build:

common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
  154 |         "push %%rbx; push %%rbp; push %%r12;"
      |         ^
<inline asm>:1:121: note: instantiated into assembly here
    1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
      |                                                                                                                                ^
common/wait.c:154:9: error: symbol '.L_skip' is already defined
  154 |         "push %%rbx; push %%rbp; push %%r12;"
      |         ^
<inline asm>:1:159: note: instantiated into assembly here
    1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
      |                                                                                                                                                                      ^
2 errors generated.

The inline assembly block in __prepare_to_wait() is duplicated, thus
leading to multiple definitions of the otherwise unique labels inside the
assembly block.  GCC extended-asm documentation notes the possibility of
duplicating asm blocks:

> Under certain circumstances, GCC may duplicate (or remove duplicates of)
> your assembly code when optimizing. This can lead to unexpected duplicate
> symbol errors during compilation if your asm code defines symbols or
> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.

Move the assembly blocks that deal with saving and restoring the current
CPU context into it's own explicitly non-inline functions.  This prevents
clang from duplicating the assembly blocks.  Just using noinline attribute
seems to be enough to prevent assembly duplication, in the future noclone
might also be required if asm block duplication issues arise again.

Additionally, add a small self-test to ensure the consistency of the
context save and restore logic.

Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
Link: https://github.com/llvm/llvm-project/issues/92161
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/wait.c | 111 +++++++++++++++++++++++++++++++---------------
 1 file changed, 76 insertions(+), 35 deletions(-)

diff --git a/xen/common/wait.c b/xen/common/wait.c
index cb6f5ff3c20a..2fcbbe8d0c71 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -119,24 +119,16 @@ void wake_up_all(struct waitqueue_head *wq)
 
 #ifdef CONFIG_X86
 
-static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
+/*
+ * context_save() must strictly be noinline, as to avoid multiple callers from
+ * inlining the code, thus duplicating the label and triggering an assembler
+ * error about duplicated labels.
+ */
+static void noinline context_save(struct waitqueue_vcpu *wqv)
 {
     struct cpu_info *cpu_info = get_cpu_info();
-    struct vcpu *curr = current;
     unsigned long dummy;
 
-    ASSERT(wqv->esp == NULL);
-
-    /* Save current VCPU affinity; force wakeup on *this* CPU only. */
-    if ( vcpu_temporary_affinity(curr, smp_processor_id(), VCPU_AFFINITY_WAIT) )
-    {
-        gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
-        domain_crash(curr->domain);
-
-        for ( ; ; )
-            do_softirq();
-    }
-
     /*
      * Hand-rolled setjmp().
      *
@@ -170,6 +162,54 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
         : "0" (0), "1" (cpu_info), "2" (wqv->stack),
           [sz] "i" (PAGE_SIZE)
         : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
+}
+
+/*
+ * Since context_save() is noinline, context_restore() must also be noinline,
+ * to balance the RET vs CALL instructions.
+ */
+static void noinline noreturn context_restore(struct waitqueue_vcpu *wqv)
+{
+    /*
+     * Hand-rolled longjmp().
+     *
+     * check_wakeup_from_wait() is always called with a shallow stack,
+     * immediately after the vCPU has been rescheduled.
+     *
+     * Adjust %rsp to be the correct depth for the (deeper) stack we want to
+     * restore, then prepare %rsi, %rdi and %rcx such that when we rejoin the
+     * rep movs in __prepare_to_wait(), it copies from wqv->stack over the
+     * active stack.
+     *
+     * All other GPRs are available for use; They're restored from the stack,
+     * or explicitly clobbered.
+     */
+    asm volatile ( "mov %%rdi, %%rsp;"
+                   "jmp .L_wq_resume"
+                   :
+                   : "S" (wqv->stack), "D" (wqv->esp),
+                     "c" ((char *)get_cpu_info() - (char *)wqv->esp)
+                   : "memory" );
+    unreachable();
+}
+
+static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
+{
+    struct vcpu *curr = current;
+
+    ASSERT(wqv->esp == NULL);
+
+    /* Save current VCPU affinity; force wakeup on *this* CPU only. */
+    if ( vcpu_temporary_affinity(curr, smp_processor_id(), VCPU_AFFINITY_WAIT) )
+    {
+        gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
+        domain_crash(curr->domain);
+
+        for ( ; ; )
+            do_softirq();
+    }
+
+    context_save(wqv);
 
     if ( unlikely(wqv->esp == NULL) )
     {
@@ -229,30 +269,31 @@ void check_wakeup_from_wait(void)
      *
      * Therefore, no actions are necessary here to maintain RSB safety.
      */
-
-    /*
-     * Hand-rolled longjmp().
-     *
-     * check_wakeup_from_wait() is always called with a shallow stack,
-     * immediately after the vCPU has been rescheduled.
-     *
-     * Adjust %rsp to be the correct depth for the (deeper) stack we want to
-     * restore, then prepare %rsi, %rdi and %rcx such that when we rejoin the
-     * rep movs in __prepare_to_wait(), it copies from wqv->stack over the
-     * active stack.
-     *
-     * All other GPRs are available for use; They're restored from the stack,
-     * or explicitly clobbered.
-     */
-    asm volatile ( "mov %%rdi, %%rsp;"
-                   "jmp .L_wq_resume"
-                   :
-                   : "S" (wqv->stack), "D" (wqv->esp),
-                     "c" ((char *)get_cpu_info() - (char *)wqv->esp)
-                   : "memory" );
+    context_restore(wqv);
     unreachable();
 }
 
+#ifdef CONFIG_SELF_TESTS
+static void __init __constructor test_save_restore_ctx(void)
+{
+    static unsigned int __initdata count;
+    struct waitqueue_vcpu wqv = {};
+
+    wqv.stack = alloc_xenheap_page();
+    if ( !wqv.stack )
+        panic("unable to allocate memory for context selftest\n");
+
+    context_save(&wqv);
+    if ( !count++ )
+        context_restore(&wqv);
+
+    if ( count != 2 )
+        panic("context save and restore not working as expected\n");
+
+    free_xenheap_page(wqv.stack);
+}
+#endif
+
 #else /* !CONFIG_X86 */
 
 #define __prepare_to_wait(wqv) ((void)0)
-- 
2.48.1

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> diff --git a/xen/common/wait.c b/xen/common/wait.c
> index cb6f5ff3c20a..2fcbbe8d0c71 100644
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -170,6 +162,54 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          : "0" (0), "1" (cpu_info), "2" (wqv->stack),
>            [sz] "i" (PAGE_SIZE)
>          : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
> +}
> +
> +/*
> + * Since context_save() is noinline, context_restore() must also be noinline,
> + * to balance the RET vs CALL instructions.

Why are you caring about balancing CALLs and RETs?

This infrastructure exists for cases which don't.

> +#ifdef CONFIG_SELF_TESTS
> +static void __init __constructor test_save_restore_ctx(void)
> +{
> +    static unsigned int __initdata count;
> +    struct waitqueue_vcpu wqv = {};
> +
> +    wqv.stack = alloc_xenheap_page();
> +    if ( !wqv.stack )
> +        panic("unable to allocate memory for context selftest\n");
> +
> +    context_save(&wqv);
> +    if ( !count++ )
> +        context_restore(&wqv);
> +
> +    if ( count != 2 )
> +        panic("context save and restore not working as expected\n");
> +
> +    free_xenheap_page(wqv.stack);
> +}
> +#endif

The wait infrastructure is incompatible with CET-SS.  (yet another
reason why I want to delete it.)

The only reason this wont blow up in CI because shadow stacks are
enabled later in boot, but I was hoping to change this with FRED.

~Andrew

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Jan Beulich]

On 13.03.2025 16:30, Roger Pau Monne wrote:
> When enabling UBSAN with clang, the following error is triggered during the
> build:
> 
> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>       |         ^
> <inline asm>:1:121: note: instantiated into assembly here
>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>       |                                                                                                                                ^
> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>       |         ^
> <inline asm>:1:159: note: instantiated into assembly here
>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>       |                                                                                                                                                                      ^
> 2 errors generated.
> 
> The inline assembly block in __prepare_to_wait() is duplicated, thus
> leading to multiple definitions of the otherwise unique labels inside the
> assembly block.  GCC extended-asm documentation notes the possibility of
> duplicating asm blocks:
> 
>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>> your assembly code when optimizing. This can lead to unexpected duplicate
>> symbol errors during compilation if your asm code defines symbols or
>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
> 
> Move the assembly blocks that deal with saving and restoring the current
> CPU context into it's own explicitly non-inline functions.  This prevents
> clang from duplicating the assembly blocks.  Just using noinline attribute
> seems to be enough to prevent assembly duplication, in the future noclone
> might also be required if asm block duplication issues arise again.

Wouldn't it be a far easier / less intrusive change to simply append %= to
the label names?

Jan

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
> On 13.03.2025 16:30, Roger Pau Monne wrote:
> > When enabling UBSAN with clang, the following error is triggered during the
> > build:
> > 
> > common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
> >   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >       |         ^
> > <inline asm>:1:121: note: instantiated into assembly here
> >     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >       |                                                                                                                                ^
> > common/wait.c:154:9: error: symbol '.L_skip' is already defined
> >   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >       |         ^
> > <inline asm>:1:159: note: instantiated into assembly here
> >     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >       |                                                                                                                                                                      ^
> > 2 errors generated.
> > 
> > The inline assembly block in __prepare_to_wait() is duplicated, thus
> > leading to multiple definitions of the otherwise unique labels inside the
> > assembly block.  GCC extended-asm documentation notes the possibility of
> > duplicating asm blocks:
> > 
> >> Under certain circumstances, GCC may duplicate (or remove duplicates of)
> >> your assembly code when optimizing. This can lead to unexpected duplicate
> >> symbol errors during compilation if your asm code defines symbols or
> >> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
> > 
> > Move the assembly blocks that deal with saving and restoring the current
> > CPU context into it's own explicitly non-inline functions.  This prevents
> > clang from duplicating the assembly blocks.  Just using noinline attribute
> > seems to be enough to prevent assembly duplication, in the future noclone
> > might also be required if asm block duplication issues arise again.
> 
> Wouldn't it be a far easier / less intrusive change to simply append %= to
> the label names?

That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
won't be able to make a jump to the .L_wq_resume label defined in the
__prepare_to_wait() assembly block if the label is declared as
.L_wq_resume%=.

Also we want to make sure there's a single .L_wq_resume seeing how
check_wakeup_from_wait() uses it as the restore entry point?

Thanks, Roger.

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Jan Beulich]

On 14.03.2025 09:30, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>> When enabling UBSAN with clang, the following error is triggered during the
>>> build:
>>>
>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>       |         ^
>>> <inline asm>:1:121: note: instantiated into assembly here
>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>       |                                                                                                                                ^
>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>       |         ^
>>> <inline asm>:1:159: note: instantiated into assembly here
>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>       |                                                                                                                                                                      ^
>>> 2 errors generated.
>>>
>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
>>> leading to multiple definitions of the otherwise unique labels inside the
>>> assembly block.  GCC extended-asm documentation notes the possibility of
>>> duplicating asm blocks:
>>>
>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>>>> your assembly code when optimizing. This can lead to unexpected duplicate
>>>> symbol errors during compilation if your asm code defines symbols or
>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
>>>
>>> Move the assembly blocks that deal with saving and restoring the current
>>> CPU context into it's own explicitly non-inline functions.  This prevents
>>> clang from duplicating the assembly blocks.  Just using noinline attribute
>>> seems to be enough to prevent assembly duplication, in the future noclone
>>> might also be required if asm block duplication issues arise again.
>>
>> Wouldn't it be a far easier / less intrusive change to simply append %= to
>> the label names?
> 
> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
> won't be able to make a jump to the .L_wq_resume label defined in the
> __prepare_to_wait() assembly block if the label is declared as
> .L_wq_resume%=.
> 
> Also we want to make sure there's a single .L_wq_resume seeing how
> check_wakeup_from_wait() uses it as the restore entry point?

Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
understanding why there is this duplication? The breaking out of the asm()
that you do isn't going to be reliable, as in principle the compiler is
still permitted to duplicate stuff. Afaict the only reliable way is to move
the code to a separate assembly file (with the asm() merely JMPing there,
providing a pseudo-return-address by some custom means). Or to a file-scope
asm(), as those can't be duplicated.

Jan

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Andrew Cooper]

On 14/03/2025 8:44 am, Jan Beulich wrote:
> On 14.03.2025 09:30, Roger Pau Monné wrote:
>> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
>>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>>> When enabling UBSAN with clang, the following error is triggered during the
>>>> build:
>>>>
>>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>       |         ^
>>>> <inline asm>:1:121: note: instantiated into assembly here
>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>       |                                                                                                                                ^
>>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>       |         ^
>>>> <inline asm>:1:159: note: instantiated into assembly here
>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>       |                                                                                                                                                                      ^
>>>> 2 errors generated.
>>>>
>>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
>>>> leading to multiple definitions of the otherwise unique labels inside the
>>>> assembly block.  GCC extended-asm documentation notes the possibility of
>>>> duplicating asm blocks:
>>>>
>>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>>>>> your assembly code when optimizing. This can lead to unexpected duplicate
>>>>> symbol errors during compilation if your asm code defines symbols or
>>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
>>>> Move the assembly blocks that deal with saving and restoring the current
>>>> CPU context into it's own explicitly non-inline functions.  This prevents
>>>> clang from duplicating the assembly blocks.  Just using noinline attribute
>>>> seems to be enough to prevent assembly duplication, in the future noclone
>>>> might also be required if asm block duplication issues arise again.
>>> Wouldn't it be a far easier / less intrusive change to simply append %= to
>>> the label names?
>> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
>> won't be able to make a jump to the .L_wq_resume label defined in the
>> __prepare_to_wait() assembly block if the label is declared as
>> .L_wq_resume%=.
>>
>> Also we want to make sure there's a single .L_wq_resume seeing how
>> check_wakeup_from_wait() uses it as the restore entry point?
> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
> understanding why there is this duplication? The breaking out of the asm()
> that you do isn't going to be reliable, as in principle the compiler is
> still permitted to duplicate stuff. Afaict the only reliable way is to move
> the code to a separate assembly file (with the asm() merely JMPing there,
> providing a pseudo-return-address by some custom means). Or to a file-scope
> asm(), as those can't be duplicated.

See the simplified example in
https://github.com/llvm/llvm-project/issues/92161

When I debugged this a while back, The multiple uses of wqv->esp (one
explicit after the asm, one as an asm parameter) gain pointer
sanitisation, so the structure looks like:

    ...
    if ( bad pointer )
        __ubsan_report();
    asm volatile (...);
    if ( bad pointer )
        __ubsan_report();
    ...

which then got transformed to:

    if ( bad pointer )
    {
        __ubsan_report();
        asm volatile (...);
        __ubsan_report();
    }
    else
        asm volatile (...);

~Andrew

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Jan Beulich]

On 14.03.2025 10:05, Andrew Cooper wrote:
> On 14/03/2025 8:44 am, Jan Beulich wrote:
>> On 14.03.2025 09:30, Roger Pau Monné wrote:
>>> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
>>>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>>>> When enabling UBSAN with clang, the following error is triggered during the
>>>>> build:
>>>>>
>>>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>       |         ^
>>>>> <inline asm>:1:121: note: instantiated into assembly here
>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>       |                                                                                                                                ^
>>>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>       |         ^
>>>>> <inline asm>:1:159: note: instantiated into assembly here
>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>       |                                                                                                                                                                      ^
>>>>> 2 errors generated.
>>>>>
>>>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
>>>>> leading to multiple definitions of the otherwise unique labels inside the
>>>>> assembly block.  GCC extended-asm documentation notes the possibility of
>>>>> duplicating asm blocks:
>>>>>
>>>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>>>>>> your assembly code when optimizing. This can lead to unexpected duplicate
>>>>>> symbol errors during compilation if your asm code defines symbols or
>>>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
>>>>> Move the assembly blocks that deal with saving and restoring the current
>>>>> CPU context into it's own explicitly non-inline functions.  This prevents
>>>>> clang from duplicating the assembly blocks.  Just using noinline attribute
>>>>> seems to be enough to prevent assembly duplication, in the future noclone
>>>>> might also be required if asm block duplication issues arise again.
>>>> Wouldn't it be a far easier / less intrusive change to simply append %= to
>>>> the label names?
>>> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
>>> won't be able to make a jump to the .L_wq_resume label defined in the
>>> __prepare_to_wait() assembly block if the label is declared as
>>> .L_wq_resume%=.
>>>
>>> Also we want to make sure there's a single .L_wq_resume seeing how
>>> check_wakeup_from_wait() uses it as the restore entry point?
>> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
>> understanding why there is this duplication? The breaking out of the asm()
>> that you do isn't going to be reliable, as in principle the compiler is
>> still permitted to duplicate stuff. Afaict the only reliable way is to move
>> the code to a separate assembly file (with the asm() merely JMPing there,
>> providing a pseudo-return-address by some custom means). Or to a file-scope
>> asm(), as those can't be duplicated.
> 
> See the simplified example in
> https://github.com/llvm/llvm-project/issues/92161
> 
> When I debugged this a while back, The multiple uses of wqv->esp (one
> explicit after the asm, one as an asm parameter) gain pointer
> sanitisation, so the structure looks like:
> 
>     ...
>     if ( bad pointer )
>         __ubsan_report();
>     asm volatile (...);
>     if ( bad pointer )
>         __ubsan_report();
>     ...
> 
> which then got transformed to:
> 
>     if ( bad pointer )
>     {
>         __ubsan_report();
>         asm volatile (...);
>         __ubsan_report();
>     }
>     else
>         asm volatile (...);

But isn't it then going to be enough to latch &wqv->esp into a local variable,
and use that in the asm() and in the subsequent if()?

Jan

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 10:13:07AM +0100, Jan Beulich wrote:
> On 14.03.2025 10:05, Andrew Cooper wrote:
> > On 14/03/2025 8:44 am, Jan Beulich wrote:
> >> On 14.03.2025 09:30, Roger Pau Monné wrote:
> >>> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
> >>>> On 13.03.2025 16:30, Roger Pau Monne wrote:
> >>>>> When enabling UBSAN with clang, the following error is triggered during the
> >>>>> build:
> >>>>>
> >>>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
> >>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >>>>>       |         ^
> >>>>> <inline asm>:1:121: note: instantiated into assembly here
> >>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >>>>>       |                                                                                                                                ^
> >>>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
> >>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >>>>>       |         ^
> >>>>> <inline asm>:1:159: note: instantiated into assembly here
> >>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >>>>>       |                                                                                                                                                                      ^
> >>>>> 2 errors generated.
> >>>>>
> >>>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
> >>>>> leading to multiple definitions of the otherwise unique labels inside the
> >>>>> assembly block.  GCC extended-asm documentation notes the possibility of
> >>>>> duplicating asm blocks:
> >>>>>
> >>>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
> >>>>>> your assembly code when optimizing. This can lead to unexpected duplicate
> >>>>>> symbol errors during compilation if your asm code defines symbols or
> >>>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
> >>>>> Move the assembly blocks that deal with saving and restoring the current
> >>>>> CPU context into it's own explicitly non-inline functions.  This prevents
> >>>>> clang from duplicating the assembly blocks.  Just using noinline attribute
> >>>>> seems to be enough to prevent assembly duplication, in the future noclone
> >>>>> might also be required if asm block duplication issues arise again.
> >>>> Wouldn't it be a far easier / less intrusive change to simply append %= to
> >>>> the label names?
> >>> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
> >>> won't be able to make a jump to the .L_wq_resume label defined in the
> >>> __prepare_to_wait() assembly block if the label is declared as
> >>> .L_wq_resume%=.
> >>>
> >>> Also we want to make sure there's a single .L_wq_resume seeing how
> >>> check_wakeup_from_wait() uses it as the restore entry point?
> >> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
> >> understanding why there is this duplication? The breaking out of the asm()
> >> that you do isn't going to be reliable, as in principle the compiler is
> >> still permitted to duplicate stuff. Afaict the only reliable way is to move
> >> the code to a separate assembly file (with the asm() merely JMPing there,
> >> providing a pseudo-return-address by some custom means). Or to a file-scope
> >> asm(), as those can't be duplicated.
> > 
> > See the simplified example in
> > https://github.com/llvm/llvm-project/issues/92161
> > 
> > When I debugged this a while back, The multiple uses of wqv->esp (one
> > explicit after the asm, one as an asm parameter) gain pointer
> > sanitisation, so the structure looks like:
> > 
> >     ...
> >     if ( bad pointer )
> >         __ubsan_report();
> >     asm volatile (...);
> >     if ( bad pointer )
> >         __ubsan_report();
> >     ...
> > 
> > which then got transformed to:
> > 
> >     if ( bad pointer )
> >     {
> >         __ubsan_report();
> >         asm volatile (...);
> >         __ubsan_report();
> >     }
> >     else
> >         asm volatile (...);
> 
> But isn't it then going to be enough to latch &wqv->esp into a local variable,
> and use that in the asm() and in the subsequent if()?

I have the following diff which seems to prevent the duplication,
would you both be OK with this approach?

Thanks, Roger.
---
diff --git a/xen/common/wait.c b/xen/common/wait.c
index cb6f5ff3c20a..60ebd58a0abd 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -124,6 +124,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
     struct cpu_info *cpu_info = get_cpu_info();
     struct vcpu *curr = current;
     unsigned long dummy;
+    void *esp = NULL;
 
     ASSERT(wqv->esp == NULL);
 
@@ -166,12 +167,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
         ".L_skip:"
         "pop %%r15; pop %%r14; pop %%r13;"
         "pop %%r12; pop %%rbp; pop %%rbx"
-        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
+        : "=&S" (esp), "=&c" (dummy), "=&D" (dummy)
         : "0" (0), "1" (cpu_info), "2" (wqv->stack),
           [sz] "i" (PAGE_SIZE)
         : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
 
-    if ( unlikely(wqv->esp == NULL) )
+    if ( unlikely(esp == NULL) )
     {
         gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
         domain_crash(curr->domain);
@@ -179,6 +180,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
         for ( ; ; )
             do_softirq();
     }
+    wqv->esp = esp;
 }
 
 static void __finish_wait(struct waitqueue_vcpu *wqv)

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Jan Beulich]

On 14.03.2025 11:12, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 10:13:07AM +0100, Jan Beulich wrote:
>> On 14.03.2025 10:05, Andrew Cooper wrote:
>>> On 14/03/2025 8:44 am, Jan Beulich wrote:
>>>> On 14.03.2025 09:30, Roger Pau Monné wrote:
>>>>> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
>>>>>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>>>>>> When enabling UBSAN with clang, the following error is triggered during the
>>>>>>> build:
>>>>>>>
>>>>>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>>>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>>>       |         ^
>>>>>>> <inline asm>:1:121: note: instantiated into assembly here
>>>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>>>       |                                                                                                                                ^
>>>>>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>>>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>>>       |         ^
>>>>>>> <inline asm>:1:159: note: instantiated into assembly here
>>>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>>>       |                                                                                                                                                                      ^
>>>>>>> 2 errors generated.
>>>>>>>
>>>>>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
>>>>>>> leading to multiple definitions of the otherwise unique labels inside the
>>>>>>> assembly block.  GCC extended-asm documentation notes the possibility of
>>>>>>> duplicating asm blocks:
>>>>>>>
>>>>>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>>>>>>>> your assembly code when optimizing. This can lead to unexpected duplicate
>>>>>>>> symbol errors during compilation if your asm code defines symbols or
>>>>>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
>>>>>>> Move the assembly blocks that deal with saving and restoring the current
>>>>>>> CPU context into it's own explicitly non-inline functions.  This prevents
>>>>>>> clang from duplicating the assembly blocks.  Just using noinline attribute
>>>>>>> seems to be enough to prevent assembly duplication, in the future noclone
>>>>>>> might also be required if asm block duplication issues arise again.
>>>>>> Wouldn't it be a far easier / less intrusive change to simply append %= to
>>>>>> the label names?
>>>>> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
>>>>> won't be able to make a jump to the .L_wq_resume label defined in the
>>>>> __prepare_to_wait() assembly block if the label is declared as
>>>>> .L_wq_resume%=.
>>>>>
>>>>> Also we want to make sure there's a single .L_wq_resume seeing how
>>>>> check_wakeup_from_wait() uses it as the restore entry point?
>>>> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
>>>> understanding why there is this duplication? The breaking out of the asm()
>>>> that you do isn't going to be reliable, as in principle the compiler is
>>>> still permitted to duplicate stuff. Afaict the only reliable way is to move
>>>> the code to a separate assembly file (with the asm() merely JMPing there,
>>>> providing a pseudo-return-address by some custom means). Or to a file-scope
>>>> asm(), as those can't be duplicated.
>>>
>>> See the simplified example in
>>> https://github.com/llvm/llvm-project/issues/92161
>>>
>>> When I debugged this a while back, The multiple uses of wqv->esp (one
>>> explicit after the asm, one as an asm parameter) gain pointer
>>> sanitisation, so the structure looks like:
>>>
>>>     ...
>>>     if ( bad pointer )
>>>         __ubsan_report();
>>>     asm volatile (...);
>>>     if ( bad pointer )
>>>         __ubsan_report();
>>>     ...
>>>
>>> which then got transformed to:
>>>
>>>     if ( bad pointer )
>>>     {
>>>         __ubsan_report();
>>>         asm volatile (...);
>>>         __ubsan_report();
>>>     }
>>>     else
>>>         asm volatile (...);
>>
>> But isn't it then going to be enough to latch &wqv->esp into a local variable,
>> and use that in the asm() and in the subsequent if()?
> 
> I have the following diff which seems to prevent the duplication,
> would you both be OK with this approach?

Yes (with a brief comment added as to the need for the local). And thanks.

Jan

> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -124,6 +124,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>      struct cpu_info *cpu_info = get_cpu_info();
>      struct vcpu *curr = current;
>      unsigned long dummy;
> +    void *esp = NULL;
>  
>      ASSERT(wqv->esp == NULL);
>  
> @@ -166,12 +167,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          ".L_skip:"
>          "pop %%r15; pop %%r14; pop %%r13;"
>          "pop %%r12; pop %%rbp; pop %%rbx"
> -        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> +        : "=&S" (esp), "=&c" (dummy), "=&D" (dummy)
>          : "0" (0), "1" (cpu_info), "2" (wqv->stack),
>            [sz] "i" (PAGE_SIZE)
>          : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
>  
> -    if ( unlikely(wqv->esp == NULL) )
> +    if ( unlikely(esp == NULL) )
>      {
>          gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
>          domain_crash(curr->domain);
> @@ -179,6 +180,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          for ( ; ; )
>              do_softirq();
>      }
> +    wqv->esp = esp;
>  }
>  
>  static void __finish_wait(struct waitqueue_vcpu *wqv)
> 

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Andrew Cooper]

On 14/03/2025 10:12 am, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 10:13:07AM +0100, Jan Beulich wrote:
>> But isn't it then going to be enough to latch &wqv->esp into a local variable,
>> and use that in the asm() and in the subsequent if()?
> I have the following diff which seems to prevent the duplication,
> would you both be OK with this approach?
>
> Thanks, Roger.
> ---
> diff --git a/xen/common/wait.c b/xen/common/wait.c
> index cb6f5ff3c20a..60ebd58a0abd 100644
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -124,6 +124,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>      struct cpu_info *cpu_info = get_cpu_info();
>      struct vcpu *curr = current;
>      unsigned long dummy;
> +    void *esp = NULL;
>  
>      ASSERT(wqv->esp == NULL);
>  
> @@ -166,12 +167,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          ".L_skip:"
>          "pop %%r15; pop %%r14; pop %%r13;"
>          "pop %%r12; pop %%rbp; pop %%rbx"
> -        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> +        : "=&S" (esp), "=&c" (dummy), "=&D" (dummy)
>          : "0" (0), "1" (cpu_info), "2" (wqv->stack),
>            [sz] "i" (PAGE_SIZE)
>          : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
>  
> -    if ( unlikely(wqv->esp == NULL) )
> +    if ( unlikely(esp == NULL) )
>      {
>          gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
>          domain_crash(curr->domain);
> @@ -179,6 +180,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          for ( ; ; )
>              do_softirq();
>      }
> +    wqv->esp = esp;
>  }
>  
>  static void __finish_wait(struct waitqueue_vcpu *wqv)
>

If that works, then fine.  It's certainly less invasive.

The moment I actually get around to (or persuade someone else to) switch
the introspection mappings over to acquire_resource, then wait.c is
going to be deleted.

~Andrew

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 09:44:10AM +0100, Jan Beulich wrote:
> On 14.03.2025 09:30, Roger Pau Monné wrote:
> > On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
> >> On 13.03.2025 16:30, Roger Pau Monne wrote:
> >>> When enabling UBSAN with clang, the following error is triggered during the
> >>> build:
> >>>
> >>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
> >>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >>>       |         ^
> >>> <inline asm>:1:121: note: instantiated into assembly here
> >>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >>>       |                                                                                                                                ^
> >>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
> >>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
> >>>       |         ^
> >>> <inline asm>:1:159: note: instantiated into assembly here
> >>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
> >>>       |                                                                                                                                                                      ^
> >>> 2 errors generated.
> >>>
> >>> The inline assembly block in __prepare_to_wait() is duplicated, thus
> >>> leading to multiple definitions of the otherwise unique labels inside the
> >>> assembly block.  GCC extended-asm documentation notes the possibility of
> >>> duplicating asm blocks:
> >>>
> >>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
> >>>> your assembly code when optimizing. This can lead to unexpected duplicate
> >>>> symbol errors during compilation if your asm code defines symbols or
> >>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
> >>>
> >>> Move the assembly blocks that deal with saving and restoring the current
> >>> CPU context into it's own explicitly non-inline functions.  This prevents
> >>> clang from duplicating the assembly blocks.  Just using noinline attribute
> >>> seems to be enough to prevent assembly duplication, in the future noclone
> >>> might also be required if asm block duplication issues arise again.
> >>
> >> Wouldn't it be a far easier / less intrusive change to simply append %= to
> >> the label names?
> > 
> > That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
> > won't be able to make a jump to the .L_wq_resume label defined in the
> > __prepare_to_wait() assembly block if the label is declared as
> > .L_wq_resume%=.
> > 
> > Also we want to make sure there's a single .L_wq_resume seeing how
> > check_wakeup_from_wait() uses it as the restore entry point?
> 
> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
> understanding why there is this duplication?

Not anything else than what Andrew found in:

https://github.com/llvm/llvm-project/issues/92161

> The breaking out of the asm()
> that you do isn't going to be reliable, as in principle the compiler is
> still permitted to duplicate stuff.

I know.  That's why I mention in the commit message that "... asm
block duplication issues arise again."

> Afaict the only reliable way is to move
> the code to a separate assembly file (with the asm() merely JMPing there,
> providing a pseudo-return-address by some custom means). Or to a file-scope
> asm(), as those can't be duplicated.

Moving to a separate file was my first thought, but it seemed more
intrusive that strictly needed to workaround the issue at hand.

I can take a look at what I can do, if the proposed approach is not
suitable.

Thanks, Roger.

Re: [PATCH 2/7] x86/wait: prevent duplicated assembly labels

[Jan Beulich]

On 14.03.2025 10:06, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 09:44:10AM +0100, Jan Beulich wrote:
>> On 14.03.2025 09:30, Roger Pau Monné wrote:
>>> On Fri, Mar 14, 2025 at 09:24:09AM +0100, Jan Beulich wrote:
>>>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>>>> When enabling UBSAN with clang, the following error is triggered during the
>>>>> build:
>>>>>
>>>>> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>       |         ^
>>>>> <inline asm>:1:121: note: instantiated into assembly here
>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>       |                                                                                                                                ^
>>>>> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>>>>>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>>>>>       |         ^
>>>>> <inline asm>:1:159: note: instantiated into assembly here
>>>>>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>>>>>       |                                                                                                                                                                      ^
>>>>> 2 errors generated.
>>>>>
>>>>> The inline assembly block in __prepare_to_wait() is duplicated, thus
>>>>> leading to multiple definitions of the otherwise unique labels inside the
>>>>> assembly block.  GCC extended-asm documentation notes the possibility of
>>>>> duplicating asm blocks:
>>>>>
>>>>>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>>>>>> your assembly code when optimizing. This can lead to unexpected duplicate
>>>>>> symbol errors during compilation if your asm code defines symbols or
>>>>>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
>>>>>
>>>>> Move the assembly blocks that deal with saving and restoring the current
>>>>> CPU context into it's own explicitly non-inline functions.  This prevents
>>>>> clang from duplicating the assembly blocks.  Just using noinline attribute
>>>>> seems to be enough to prevent assembly duplication, in the future noclone
>>>>> might also be required if asm block duplication issues arise again.
>>>>
>>>> Wouldn't it be a far easier / less intrusive change to simply append %= to
>>>> the label names?
>>>
>>> That won't work AFAICT, as the inline asm in check_wakeup_from_wait()
>>> won't be able to make a jump to the .L_wq_resume label defined in the
>>> __prepare_to_wait() assembly block if the label is declared as
>>> .L_wq_resume%=.
>>>
>>> Also we want to make sure there's a single .L_wq_resume seeing how
>>> check_wakeup_from_wait() uses it as the restore entry point?
>>
>> Hmm, yes on both points; the %= would only work for .Lskip. Have you gained
>> understanding why there is this duplication?
> 
> Not anything else than what Andrew found in:
> 
> https://github.com/llvm/llvm-project/issues/92161
> 
>> The breaking out of the asm()
>> that you do isn't going to be reliable, as in principle the compiler is
>> still permitted to duplicate stuff.
> 
> I know.  That's why I mention in the commit message that "... asm
> block duplication issues arise again."
> 
>> Afaict the only reliable way is to move
>> the code to a separate assembly file (with the asm() merely JMPing there,
>> providing a pseudo-return-address by some custom means). Or to a file-scope
>> asm(), as those can't be duplicated.
> 
> Moving to a separate file was my first thought, but it seemed more
> intrusive that strictly needed to workaround the issue at hand.

Maybe the file-scope asm() approach would be less intrusive overall,
compared to the separate-.S-file one. Plus it may allow keeping labels
non-global.

> I can take a look at what I can do, if the proposed approach is not
> suitable.

I've made yet another suggestion in reply to Andrew's response.

Jan

[PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Roger Pau Monne]

When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
compiler emits the following errors:

arch/x86/setup.c: In function '__start_xen':
arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
 1504 |             end = consider_modules(s, e, reloc_size + mask,
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1505 |                                    bi->mods, bi->nr_modules, -1);
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
  686 | static uint64_t __init consider_modules(
      |                        ^~~~~~~~~~~~~~~~
arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
 1535 |             end = consider_modules(s, e, size, bi->mods,
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1536 |                                    bi->nr_modules + relocated, j);
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
  686 | static uint64_t __init consider_modules(
      |                        ^~~~~~~~~~~~~~~~

This seems to be the result of some function manipulation done by UBSAN
triggering GCC stringops related errors.  Placate the errors by declaring
the function parameter as `const struct *boot_module` instead of `const
struct boot_module[]`.

Note that GCC 13 seems to be fixed, and doesn't trigger the error when
using `[]`.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4a32d8491186..bde5d75ea6ab 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
 #undef BOOTSTRAP_MAP_LIMIT
 
 static uint64_t __init consider_modules(
-    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
+    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
     unsigned int nr_mods, unsigned int this_mod)
 {
     unsigned int i;
-- 
2.48.1

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> compiler emits the following errors:
>
> arch/x86/setup.c: In function '__start_xen':
> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1505 |                                    bi->mods, bi->nr_modules, -1);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1535 |             end = consider_modules(s, e, size, bi->mods,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1536 |                                    bi->nr_modules + relocated, j);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
>
> This seems to be the result of some function manipulation done by UBSAN
> triggering GCC stringops related errors.  Placate the errors by declaring
> the function parameter as `const struct *boot_module` instead of `const
> struct boot_module[]`.
>
> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> using `[]`.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

(I swear I've seen this before, and already fixed it once by switching
to a pointer...)

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Jan Beulich]

On 13.03.2025 16:30, Roger Pau Monne wrote:
> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> compiler emits the following errors:
> 
> arch/x86/setup.c: In function '__start_xen':
> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1505 |                                    bi->mods, bi->nr_modules, -1);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1535 |             end = consider_modules(s, e, size, bi->mods,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1536 |                                    bi->nr_modules + relocated, j);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> 
> This seems to be the result of some function manipulation done by UBSAN
> triggering GCC stringops related errors.  Placate the errors by declaring
> the function parameter as `const struct *boot_module` instead of `const
> struct boot_module[]`.
> 
> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> using `[]`.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4a32d8491186..bde5d75ea6ab 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
>  #undef BOOTSTRAP_MAP_LIMIT
>  
>  static uint64_t __init consider_modules(
> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
>      unsigned int nr_mods, unsigned int this_mod)
>  {
>      unsigned int i;

While I'm okay-ish with the change, how are we going to make sure it won't be
re-introduced? Or something similar be introduced elsewhere?

Jan

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
> On 13.03.2025 16:30, Roger Pau Monne wrote:
> > When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> > compiler emits the following errors:
> > 
> > arch/x86/setup.c: In function '__start_xen':
> > arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >  1504 |             end = consider_modules(s, e, reloc_size + mask,
> >       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  1505 |                                    bi->mods, bi->nr_modules, -1);
> >       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >   686 | static uint64_t __init consider_modules(
> >       |                        ^~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >  1535 |             end = consider_modules(s, e, size, bi->mods,
> >       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  1536 |                                    bi->nr_modules + relocated, j);
> >       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >   686 | static uint64_t __init consider_modules(
> >       |                        ^~~~~~~~~~~~~~~~
> > 
> > This seems to be the result of some function manipulation done by UBSAN
> > triggering GCC stringops related errors.  Placate the errors by declaring
> > the function parameter as `const struct *boot_module` instead of `const
> > struct boot_module[]`.
> > 
> > Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> > using `[]`.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/setup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 4a32d8491186..bde5d75ea6ab 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
> >  #undef BOOTSTRAP_MAP_LIMIT
> >  
> >  static uint64_t __init consider_modules(
> > -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> > +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
> >      unsigned int nr_mods, unsigned int this_mod)
> >  {
> >      unsigned int i;
> 
> While I'm okay-ish with the change, how are we going to make sure it won't be
> re-introduced? Or something similar be introduced elsewhere?

I'm afraid I don't have a good response, as I don't even know exactly
why the error triggers.  We will rely on the CI to start doing
randconfig builds with UBSAN enabled (see patch 7/7).

Thanks, Roger.

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Jan Beulich]

On 14.03.2025 09:27, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
>>> compiler emits the following errors:
>>>
>>> arch/x86/setup.c: In function '__start_xen':
>>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>>>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>  1505 |                                    bi->mods, bi->nr_modules, -1);
>>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
>>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>>>   686 | static uint64_t __init consider_modules(
>>>       |                        ^~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>>>  1535 |             end = consider_modules(s, e, size, bi->mods,
>>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>  1536 |                                    bi->nr_modules + relocated, j);
>>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
>>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>>>   686 | static uint64_t __init consider_modules(
>>>       |                        ^~~~~~~~~~~~~~~~
>>>
>>> This seems to be the result of some function manipulation done by UBSAN
>>> triggering GCC stringops related errors.  Placate the errors by declaring
>>> the function parameter as `const struct *boot_module` instead of `const
>>> struct boot_module[]`.
>>>
>>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
>>> using `[]`.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/setup.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 4a32d8491186..bde5d75ea6ab 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
>>>  #undef BOOTSTRAP_MAP_LIMIT
>>>  
>>>  static uint64_t __init consider_modules(
>>> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
>>> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
>>>      unsigned int nr_mods, unsigned int this_mod)
>>>  {
>>>      unsigned int i;
>>
>> While I'm okay-ish with the change, how are we going to make sure it won't be
>> re-introduced? Or something similar be introduced elsewhere?
> 
> I'm afraid I don't have a good response, as I don't even know exactly
> why the error triggers.

One option might be to amend ./CODING_STYLE for dis-encourage [] notation
in function parameters. I wouldn't be happy about us doing so, as I think
that serves a documentation purpose, but compiler deficiencies getting in
the way is certainly higher priority here.

Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh,
wouldn't be desirable imo, as it would still lose the doc effect, at least
to some degree.

>  We will rely on the CI to start doing
> randconfig builds with UBSAN enabled (see patch 7/7).

Right. Just that randconfig is, well, random in what it covers.

Jan

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 09:33:01AM +0100, Jan Beulich wrote:
> On 14.03.2025 09:27, Roger Pau Monné wrote:
> > On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
> >> On 13.03.2025 16:30, Roger Pau Monne wrote:
> >>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> >>> compiler emits the following errors:
> >>>
> >>> arch/x86/setup.c: In function '__start_xen':
> >>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >>>  1504 |             end = consider_modules(s, e, reloc_size + mask,
> >>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>  1505 |                                    bi->mods, bi->nr_modules, -1);
> >>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >>>   686 | static uint64_t __init consider_modules(
> >>>       |                        ^~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >>>  1535 |             end = consider_modules(s, e, size, bi->mods,
> >>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>  1536 |                                    bi->nr_modules + relocated, j);
> >>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >>>   686 | static uint64_t __init consider_modules(
> >>>       |                        ^~~~~~~~~~~~~~~~
> >>>
> >>> This seems to be the result of some function manipulation done by UBSAN
> >>> triggering GCC stringops related errors.  Placate the errors by declaring
> >>> the function parameter as `const struct *boot_module` instead of `const
> >>> struct boot_module[]`.
> >>>
> >>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> >>> using `[]`.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>>  xen/arch/x86/setup.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> >>> index 4a32d8491186..bde5d75ea6ab 100644
> >>> --- a/xen/arch/x86/setup.c
> >>> +++ b/xen/arch/x86/setup.c
> >>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
> >>>  #undef BOOTSTRAP_MAP_LIMIT
> >>>  
> >>>  static uint64_t __init consider_modules(
> >>> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> >>> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
> >>>      unsigned int nr_mods, unsigned int this_mod)
> >>>  {
> >>>      unsigned int i;
> >>
> >> While I'm okay-ish with the change, how are we going to make sure it won't be
> >> re-introduced? Or something similar be introduced elsewhere?
> > 
> > I'm afraid I don't have a good response, as I don't even know exactly
> > why the error triggers.
> 
> One option might be to amend ./CODING_STYLE for dis-encourage [] notation
> in function parameters. I wouldn't be happy about us doing so, as I think
> that serves a documentation purpose, but compiler deficiencies getting in
> the way is certainly higher priority here.
> 
> Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh,
> wouldn't be desirable imo, as it would still lose the doc effect, at least
> to some degree.

This is a very specific case, I don't think we should change our
coding style based on it.  I think our only option is to deal with
such compiler bugs when we detect them.

Thanks, Roger.

[PATCH 4/7] xen/ubsan: expand pointer overflow message printing

[Roger Pau Monne]

Add messages about operations against the NULL pointer, or that result in
a NULL pointer.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/ubsan/ubsan.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index 7ebe4bfc14dc..20aa0cb598e1 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -517,9 +517,18 @@ void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
 
 	ubsan_prologue(&data->location, &flags);
 
-	pr_err("pointer operation %s %p to %p\n",
-	       base > result ? "overflowed" : "underflowed",
-	       _p(base), _p(result));
+	if (!base && !result)
+		pr_err("applying zero offset to null pointer\n");
+	else if (!base && result)
+		pr_err("applying non-zero offset %p to null pointer\n",
+			_p(result));
+	else if (base && !result)
+		pr_err("applying non-zero offset to non-null pointer %p produced null pointer\n",
+			_p(base));
+	else
+		pr_err("pointer operation %s %p to %p\n",
+			base > result ? "overflowed" : "underflowed",
+			_p(base), _p(result));
 
 	ubsan_epilogue(&flags);
 }
-- 
2.48.1

Re: [PATCH 4/7] xen/ubsan: expand pointer overflow message printing

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> Add messages about operations against the NULL pointer, or that result in
> a NULL pointer.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 5/7] x86/ioremap: prevent additions against the NULL pointer

[Roger Pau Monne]

This was reported by clang UBSAN as:

UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
applying zero offset to null pointer
[...]
Xen call trace:
    [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
    [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
    [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
    [<ffff82d0406c3728>] F video_init+0xd0/0x180
    [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
    [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
    [<ffff82d04020482e>] F __high_start+0x8e/0x90

Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
pointer from __vmap() is NULL.

Fixes: d0d4635d034f ('implement vmap()')
Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/dmi_scan.c | 7 +++++--
 xen/arch/x86/mm.c       | 6 ++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index 2fcc485295eb..a05492037519 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -112,6 +112,7 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
 {
     mfn_t mfn = _mfn(PFN_DOWN(addr));
     unsigned int offs = PAGE_OFFSET(addr);
+    void *va;
 
     if ( addr + len <= MB(1) )
         return __va(addr);
@@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
     if ( system_state < SYS_STATE_boot )
         return __acpi_map_table(addr, len);
 
-    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
-                  VMAP_DEFAULT) + offs;
+    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
+	        VMAP_DEFAULT);
+
+    return va ? va + offs : NULL;
 }
 
 static void __init bt_iounmap(const void *ptr, unsigned int len)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index bfdc8fb01949..03b8319f7a9d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6277,7 +6277,9 @@ void __iomem *ioremap(paddr_t pa, size_t len)
         unsigned int offs = pa & (PAGE_SIZE - 1);
         unsigned int nr = PFN_UP(offs + len);
 
-        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_UCMINUS, VMAP_DEFAULT) + offs;
+        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_UCMINUS, VMAP_DEFAULT);
+        if ( va )
+            va += offs;
     }
 
     return (void __force __iomem *)va;
@@ -6294,7 +6296,7 @@ void __iomem *__init ioremap_wc(paddr_t pa, size_t len)
 
     va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_WC, VMAP_DEFAULT);
 
-    return (void __force __iomem *)(va + offs);
+    return (void __force __iomem *)(va ? va + offs : NULL);
 }
 
 int create_perdomain_mapping(struct domain *d, unsigned long va,
-- 
2.48.1

Re: [PATCH 5/7] x86/ioremap: prevent additions against the NULL pointer

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> This was reported by clang UBSAN as:
>
> UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
> applying zero offset to null pointer
> [...]
> Xen call trace:
>     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
>     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
>     [<ffff82d0406c3728>] F video_init+0xd0/0x180
>     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
>     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
>     [<ffff82d04020482e>] F __high_start+0x8e/0x90
>
> Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
> pointer from __vmap() is NULL.
>
> Fixes: d0d4635d034f ('implement vmap()')
> Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.

It's unfortunate, because C23 makes this one case (add 0 to NULL
pointer) explicitly well defined to avoid corner cases like this.  Oh well.

> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index 2fcc485295eb..a05492037519 100644
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
>      if ( system_state < SYS_STATE_boot )
>          return __acpi_map_table(addr, len);
>  
> -    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> -                  VMAP_DEFAULT) + offs;
> +    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> +	        VMAP_DEFAULT);

You've got mixed tabs/spaces here.

~Andrew

Re: [PATCH 5/7] x86/ioremap: prevent additions against the NULL pointer

[Roger Pau Monné]

On Thu, Mar 13, 2025 at 05:21:13PM +0000, Andrew Cooper wrote:
> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> > This was reported by clang UBSAN as:
> >
> > UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
> > applying zero offset to null pointer
> > [...]
> > Xen call trace:
> >     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
> >     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
> >     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
> >     [<ffff82d0406c3728>] F video_init+0xd0/0x180
> >     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
> >     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
> >     [<ffff82d04020482e>] F __high_start+0x8e/0x90
> >
> > Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
> > pointer from __vmap() is NULL.
> >
> > Fixes: d0d4635d034f ('implement vmap()')
> > Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
> > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.
> 
> It's unfortunate, because C23 makes this one case (add 0 to NULL
> pointer) explicitly well defined to avoid corner cases like this.  Oh well.

Interesting, so they added a new type (nullptr_t) that has a single
possible value (nullptr), and hence arithmetic operations against it
always result in nullptr.  That's helpful to prevent this kind of
bugs.

> > diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> > index 2fcc485295eb..a05492037519 100644
> > --- a/xen/arch/x86/dmi_scan.c
> > +++ b/xen/arch/x86/dmi_scan.c
> > @@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
> >      if ( system_state < SYS_STATE_boot )
> >          return __acpi_map_table(addr, len);
> >  
> > -    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> > -                  VMAP_DEFAULT) + offs;
> > +    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> > +	        VMAP_DEFAULT);
> 
> You've got mixed tabs/spaces here.

Thanks, vim autodetection is a bit confused with this file because it
uses both hard and soft tabs, fixed now.

Roger.

Re: [PATCH 5/7] x86/ioremap: prevent additions against the NULL pointer

[Andrew Cooper]

On 14/03/2025 8:43 am, Roger Pau Monné wrote:
> On Thu, Mar 13, 2025 at 05:21:13PM +0000, Andrew Cooper wrote:
>> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
>>> This was reported by clang UBSAN as:
>>>
>>> UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
>>> applying zero offset to null pointer
>>> [...]
>>> Xen call trace:
>>>     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>>>     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
>>>     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
>>>     [<ffff82d0406c3728>] F video_init+0xd0/0x180
>>>     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
>>>     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
>>>     [<ffff82d04020482e>] F __high_start+0x8e/0x90
>>>
>>> Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
>>> pointer from __vmap() is NULL.
>>>
>>> Fixes: d0d4635d034f ('implement vmap()')
>>> Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
>>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.
>>
>> It's unfortunate, because C23 makes this one case (add 0 to NULL
>> pointer) explicitly well defined to avoid corner cases like this.  Oh well.
> Interesting, so they added a new type (nullptr_t) that has a single
> possible value (nullptr), and hence arithmetic operations against it
> always result in nullptr.  That's helpful to prevent this kind of
> bugs.

nullptr_t is unrelated.  That's for _Generic() and friends.

I'm struggling to find the reference to NULL + 0 being made well
defined.  It was in the context of library implementations of memset()/etc.

Nevertheless, we've got to cope with it, given our current -std.

~Andrew

[PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monne]

The call to ioremap_wc() in video_init() will always fail, because
video_init() is called ahead of vm_init_type(), and so the underlying
__vmap() call will fail to allocate the linear address space.

Fix by reverting to the previous behavior and using the directmap entries
in the low 1MB.  Note the VGA text buffer directmap entries are also
adjusted to map the VGA text buffer as WC instead of UC-.

Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/x86_64.S        | 10 +++++++---
 xen/arch/x86/include/asm/config.h |  5 +++++
 xen/drivers/video/vga.c           | 11 ++++++++---
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 26b9d1c2df9a..07f4bdf46e31 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -84,15 +84,19 @@ ENTRY(__high_start)
 /*
  * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
  * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
- * of physical memory. In any case the VGA hole should be mapped with type UC.
+ * of physical memory. VGA hole should be mapped with type UC, with the
+ * exception of the text buffer that uses WC.
  * Uses 1x 4k page.
  */
 l1_directmap:
         pfn = 0
         .rept L1_PAGETABLE_ENTRIES
-        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
-        .if pfn >= 0xa0 && pfn < 0xc0
+        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
+        .if pfn >= 0xa0 && pfn < 0xb8
         .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
+        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
+        .elseif pfn >= 0xb8 && pfn < 0xc0
+        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL | MAP_SMALL_PAGES
         .else
         .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
         .endif
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index 19746f956ec3..a455bfb0df65 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -208,6 +208,11 @@
 #endif
 #define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)
 
+#define IS_DIRECTMAP_ADDR(x) ({                                         \
+    unsigned long addr = (unsigned long)(x);                            \
+    addr >= DIRECTMAP_VIRT_START && addr < DIRECTMAP_VIRT_END;          \
+})
+
 #ifndef __ASSEMBLY__
 
 #ifdef CONFIG_PV32
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index b4d018326128..704d00034658 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -70,9 +70,13 @@ void __init video_init(void)
     switch ( vga_console_info.video_type )
     {
     case XEN_VGATYPE_TEXT_MODE_3:
-        if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) ||
-             ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) )
+        if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) )
             return;
+        /*
+         * The first MB is always mapped, and the VGA text buffer uses the WC
+         * cache attribute.
+         */
+        video = __va(0xB8000);
         outw(0x200a, 0x3d4); /* disable cursor */
         columns = vga_console_info.u.text_mode_3.columns;
         lines   = vga_console_info.u.text_mode_3.rows;
@@ -158,7 +162,8 @@ void __init video_endboot(void)
         if ( !vgacon_keep )
         {
             memset(video, 0, columns * lines * 2);
-            iounmap(video);
+            /* VGA text buffer uses a directmap mapping, don't try to unmap. */
+            ASSERT(IS_DIRECTMAP_ADDR(video));
             video = ZERO_BLOCK_PTR;
         }
         break;
-- 
2.48.1

Re: [PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Andrew Cooper]

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> The call to ioremap_wc() in video_init() will always fail, because
> video_init() is called ahead of vm_init_type(), and so the underlying
> __vmap() call will fail to allocate the linear address space.
>
> Fix by reverting to the previous behavior and using the directmap entries
> in the low 1MB.  Note the VGA text buffer directmap entries are also
> adjusted to map the VGA text buffer as WC instead of UC-.
>
> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/boot/x86_64.S        | 10 +++++++---
>  xen/arch/x86/include/asm/config.h |  5 +++++
>  xen/drivers/video/vga.c           | 11 ++++++++---
>  3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> index 26b9d1c2df9a..07f4bdf46e31 100644
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -84,15 +84,19 @@ ENTRY(__high_start)
>  /*
>   * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
>   * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
> - * of physical memory. In any case the VGA hole should be mapped with type UC.
> + * of physical memory. VGA hole should be mapped with type UC, with the
> + * exception of the text buffer that uses WC.
>   * Uses 1x 4k page.
>   */
>  l1_directmap:
>          pfn = 0
>          .rept L1_PAGETABLE_ENTRIES
> -        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
> -        .if pfn >= 0xa0 && pfn < 0xc0
> +        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
> +        .if pfn >= 0xa0 && pfn < 0xb8
>          .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
> +        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
> +        .elseif pfn >= 0xb8 && pfn < 0xc0
> +        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL | MAP_SMALL_PAGES
>          .else
>          .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
>          .endif

We have to be careful doing this.

It probably is safe to use WC in the pagetables.  We don't start using
the pagetables until after we're sure we're on a 64bit CPU, which means
WC is available.

However, doing so now means that we need explicit SFENCE's when using
this, even in places like early_error.  The IN/OUT instructions do flush
WC buffers, but the UART is written to before the screen, so there's a
chance that you'll lose the final character of the message on the screen.

~Andrew

Re: [PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monné]

(resending because I seem to have inadvertently corrupted the Cc field)

On Thu, Mar 13, 2025 at 07:39:58PM +0000, Andrew Cooper wrote:
> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> > The call to ioremap_wc() in video_init() will always fail, because
> > video_init() is called ahead of vm_init_type(), and so the underlying
> > __vmap() call will fail to allocate the linear address space.
> >
> > Fix by reverting to the previous behavior and using the directmap entries
> > in the low 1MB.  Note the VGA text buffer directmap entries are also
> > adjusted to map the VGA text buffer as WC instead of UC-.
> >
> > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/boot/x86_64.S        | 10 +++++++---
> >  xen/arch/x86/include/asm/config.h |  5 +++++
> >  xen/drivers/video/vga.c           | 11 ++++++++---
> >  3 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> > index 26b9d1c2df9a..07f4bdf46e31 100644
> > --- a/xen/arch/x86/boot/x86_64.S
> > +++ b/xen/arch/x86/boot/x86_64.S
> > @@ -84,15 +84,19 @@ ENTRY(__high_start)
> >  /*
> >   * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
> >   * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
> > - * of physical memory. In any case the VGA hole should be mapped with type UC.
> > + * of physical memory. VGA hole should be mapped with type UC, with the
> > + * exception of the text buffer that uses WC.
> >   * Uses 1x 4k page.
> >   */
> >  l1_directmap:
> >          pfn = 0
> >          .rept L1_PAGETABLE_ENTRIES
> > -        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
> > -        .if pfn >= 0xa0 && pfn < 0xc0
> > +        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
> > +        .if pfn >= 0xa0 && pfn < 0xb8
> >          .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
> > +        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
> > +        .elseif pfn >= 0xb8 && pfn < 0xc0
> > +        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL | MAP_SMALL_PAGES
> >          .else
> >          .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
> >          .endif
> 
> We have to be careful doing this.
> 
> It probably is safe to use WC in the pagetables.  We don't start using
> the pagetables until after we're sure we're on a 64bit CPU, which means
> WC is available.
> 
> However, doing so now means that we need explicit SFENCE's when using
> this, even in places like early_error.  The IN/OUT instructions do flush
> WC buffers, but the UART is written to before the screen, so there's a
> chance that you'll lose the final character of the message on the screen.

I don't think early_error will ever use this mapping.

`vga_text_buffer` contains the address 0xb8000, and AFAICT it's
exclusively used with paging disabled (as the multiboot2 efi path
explicitly sets vga_text_buffer = 0).  The WC mapping created above is
on the directmap, so va > DIRECTMAP_VIRT_START.

vga_text_puts() might need such SFENCE, but arguably that should be a
different patch IMO.  Might be best to ask Jan whether this is on
purpose?

My hypothesis is that the SFENCE might only be needed in
video_endboot() and before reboot if Xen crashed ahead of
relinquishing the VGA console.

Thanks, Roger.

Re: [PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Jan Beulich]

On 14.03.2025 11:39, Roger Pau Monné wrote:
> (resending because I seem to have inadvertently corrupted the Cc field)
> 
> On Thu, Mar 13, 2025 at 07:39:58PM +0000, Andrew Cooper wrote:
>> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
>>> The call to ioremap_wc() in video_init() will always fail, because
>>> video_init() is called ahead of vm_init_type(), and so the underlying
>>> __vmap() call will fail to allocate the linear address space.
>>>
>>> Fix by reverting to the previous behavior and using the directmap entries
>>> in the low 1MB.  Note the VGA text buffer directmap entries are also
>>> adjusted to map the VGA text buffer as WC instead of UC-.
>>>
>>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/boot/x86_64.S        | 10 +++++++---
>>>  xen/arch/x86/include/asm/config.h |  5 +++++
>>>  xen/drivers/video/vga.c           | 11 ++++++++---
>>>  3 files changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
>>> index 26b9d1c2df9a..07f4bdf46e31 100644
>>> --- a/xen/arch/x86/boot/x86_64.S
>>> +++ b/xen/arch/x86/boot/x86_64.S
>>> @@ -84,15 +84,19 @@ ENTRY(__high_start)
>>>  /*
>>>   * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
>>>   * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
>>> - * of physical memory. In any case the VGA hole should be mapped with type UC.
>>> + * of physical memory. VGA hole should be mapped with type UC, with the
>>> + * exception of the text buffer that uses WC.
>>>   * Uses 1x 4k page.
>>>   */
>>>  l1_directmap:
>>>          pfn = 0
>>>          .rept L1_PAGETABLE_ENTRIES
>>> -        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>> +        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
>>> +        .if pfn >= 0xa0 && pfn < 0xb8
>>>          .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
>>> +        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
>>> +        .elseif pfn >= 0xb8 && pfn < 0xc0
>>> +        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL | MAP_SMALL_PAGES
>>>          .else
>>>          .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
>>>          .endif
>>
>> We have to be careful doing this.
>>
>> It probably is safe to use WC in the pagetables.  We don't start using
>> the pagetables until after we're sure we're on a 64bit CPU, which means
>> WC is available.
>>
>> However, doing so now means that we need explicit SFENCE's when using
>> this, even in places like early_error.  The IN/OUT instructions do flush
>> WC buffers, but the UART is written to before the screen, so there's a
>> chance that you'll lose the final character of the message on the screen.
> 
> I don't think early_error will ever use this mapping.
> 
> `vga_text_buffer` contains the address 0xb8000, and AFAICT it's
> exclusively used with paging disabled (as the multiboot2 efi path
> explicitly sets vga_text_buffer = 0).  The WC mapping created above is
> on the directmap, so va > DIRECTMAP_VIRT_START.
> 
> vga_text_puts() might need such SFENCE, but arguably that should be a
> different patch IMO.  Might be best to ask Jan whether this is on
> purpose?

I think that was wrongly omitted before already.

> My hypothesis is that the SFENCE might only be needed in
> video_endboot() and before reboot if Xen crashed ahead of
> relinquishing the VGA console.

This might suffice for being able to see the final picture, but it may
result in display artifacts earlier on.

Question is whether simply undoing the ioremap_wc() (for not functioning
correctly) isn't going to be good enough. Prior to the change to use that,
we had been using UC- quite okay (even if a bit slow). An option may be
to make a WC mapping a little later, when __vmap() is usable.

Jan

Re: [PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 12:23:32PM +0100, Jan Beulich wrote:
> On 14.03.2025 11:39, Roger Pau Monné wrote:
> > (resending because I seem to have inadvertently corrupted the Cc field)
> > 
> > On Thu, Mar 13, 2025 at 07:39:58PM +0000, Andrew Cooper wrote:
> >> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> >>> The call to ioremap_wc() in video_init() will always fail, because
> >>> video_init() is called ahead of vm_init_type(), and so the underlying
> >>> __vmap() call will fail to allocate the linear address space.
> >>>
> >>> Fix by reverting to the previous behavior and using the directmap entries
> >>> in the low 1MB.  Note the VGA text buffer directmap entries are also
> >>> adjusted to map the VGA text buffer as WC instead of UC-.
> >>>
> >>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>>  xen/arch/x86/boot/x86_64.S        | 10 +++++++---
> >>>  xen/arch/x86/include/asm/config.h |  5 +++++
> >>>  xen/drivers/video/vga.c           | 11 ++++++++---
> >>>  3 files changed, 20 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> >>> index 26b9d1c2df9a..07f4bdf46e31 100644
> >>> --- a/xen/arch/x86/boot/x86_64.S
> >>> +++ b/xen/arch/x86/boot/x86_64.S
> >>> @@ -84,15 +84,19 @@ ENTRY(__high_start)
> >>>  /*
> >>>   * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
> >>>   * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
> >>> - * of physical memory. In any case the VGA hole should be mapped with type UC.
> >>> + * of physical memory. VGA hole should be mapped with type UC, with the
> >>> + * exception of the text buffer that uses WC.
> >>>   * Uses 1x 4k page.
> >>>   */
> >>>  l1_directmap:
> >>>          pfn = 0
> >>>          .rept L1_PAGETABLE_ENTRIES
> >>> -        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
> >>> -        .if pfn >= 0xa0 && pfn < 0xc0
> >>> +        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
> >>> +        .if pfn >= 0xa0 && pfn < 0xb8
> >>>          .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES
> >>> +        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
> >>> +        .elseif pfn >= 0xb8 && pfn < 0xc0
> >>> +        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL | MAP_SMALL_PAGES
> >>>          .else
> >>>          .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
> >>>          .endif
> >>
> >> We have to be careful doing this.
> >>
> >> It probably is safe to use WC in the pagetables.  We don't start using
> >> the pagetables until after we're sure we're on a 64bit CPU, which means
> >> WC is available.
> >>
> >> However, doing so now means that we need explicit SFENCE's when using
> >> this, even in places like early_error.  The IN/OUT instructions do flush
> >> WC buffers, but the UART is written to before the screen, so there's a
> >> chance that you'll lose the final character of the message on the screen.
> > 
> > I don't think early_error will ever use this mapping.
> > 
> > `vga_text_buffer` contains the address 0xb8000, and AFAICT it's
> > exclusively used with paging disabled (as the multiboot2 efi path
> > explicitly sets vga_text_buffer = 0).  The WC mapping created above is
> > on the directmap, so va > DIRECTMAP_VIRT_START.
> > 
> > vga_text_puts() might need such SFENCE, but arguably that should be a
> > different patch IMO.  Might be best to ask Jan whether this is on
> > purpose?
> 
> I think that was wrongly omitted before already.

OK, so we should likely have an SFENCE in vga_text_puts() then.

> > My hypothesis is that the SFENCE might only be needed in
> > video_endboot() and before reboot if Xen crashed ahead of
> > relinquishing the VGA console.
> 
> This might suffice for being able to see the final picture, but it may
> result in display artifacts earlier on.
> 
> Question is whether simply undoing the ioremap_wc() (for not functioning
> correctly) isn't going to be good enough.

Yeah, that was indeed my first approach, as I was under the impression
that not use WC wouldn't make that much of a difference in the text
buffer (as opposed to not using WC for the frame buffer).  But then I
saw that making the directmap mappings of the text buffer WC wasn't
that complicated.

> Prior to the change to use that,
> we had been using UC- quite okay (even if a bit slow). An option may be
> to make a WC mapping a little later, when __vmap() is usable.

Hm, yes, that's another possibility, albeit it seems to add even more
complexity, for a display mode that I assume is not that used anymore.
Otherwise someone would have complained of not getting any output
since 81d195c6c0e2?

If there are no further objections I will just revert to use ioremap()
and leave it like that.

Thanks, Roger.

[PATCH 7/7] kconfig/randconfig: enable UBSAN for randconfig

[Roger Pau Monne]

Introduce an additional Kconfig check to only offer the option if the
compiler supports -fsanitize=undefined.

We no longer use Travis CI, so the original motivation for not enabling
UBSAN might no longer present.  Regardless, the option won't be present in
the first place if the compiler doesn't support -fsanitize=undefined.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/Kconfig                        | 4 ++++
 xen/Kconfig.debug                  | 2 +-
 xen/tools/kconfig/allrandom.config | 1 -
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 72fdb8376087..2128f0ccfc0b 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,10 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
 config CC_SPLIT_SECTIONS
 	bool
 
+# Compiler supports -fsanitize=undefined
+config CC_HAS_UBSAN
+	def_bool $(cc-option,-fsanitize=undefined)
+
 # Set code alignment.
 #
 # Allow setting on a boolean basis, and then convert such selection to an
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index c4a8d86912e0..f7cc5ffaabd7 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -98,7 +98,7 @@ config SCRUB_DEBUG
 
 config UBSAN
 	bool "Undefined behaviour sanitizer"
-	depends on HAS_UBSAN
+	depends on HAS_UBSAN && CC_HAS_UBSAN
 	help
 	  Enable undefined behaviour sanitizer. It uses compiler to insert code
 	  snippets so that undefined behaviours in C are detected during runtime.
diff --git a/xen/tools/kconfig/allrandom.config b/xen/tools/kconfig/allrandom.config
index 76f74320b5b0..c7753ac4addb 100644
--- a/xen/tools/kconfig/allrandom.config
+++ b/xen/tools/kconfig/allrandom.config
@@ -1,4 +1,3 @@
 # Explicit option choices not subject to regular RANDCONFIG
 
 CONFIG_GCOV_FORMAT_AUTODETECT=y
-CONFIG_UBSAN=n
-- 
2.48.1