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

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

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.

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

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.

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

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

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

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)

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

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)
> 

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