Re: [PATCH 4/5] xen/wait: Use relative stack adjustments

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 09:18, Andrew Cooper wrote:
> @@ -121,11 +121,11 @@ void wake_up_all(struct waitqueue_head *wq)
>  
>  static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>  {
> -    struct cpu_info *cpu_info = get_cpu_info();
>      struct vcpu *curr = current;
>      unsigned long dummy;
> +    unsigned int used;
>  
> -    ASSERT(wqv->esp == 0);
> +    ASSERT(wqv->used == 0);

Minor: Use ! like you do further down?

> @@ -154,24 +154,25 @@ static void __prepare_to_wait(struct waitqueue_vcpu 
> *wqv)
>          "push %%rbx; push %%rbp; push %%r12;"
>          "push %%r13; push %%r14; push %%r15;"
>  
> -        "sub %%esp,%%ecx;"
> +        "sub %%esp, %%ecx;" /* ecx = delta to cpu_info */
>          "cmp %[sz], %%ecx;"
>          "ja .L_skip;"       /* Bail if >4k */

According to the inputs, %eax is still 0 when bailing here, so the
check below won't find "used > PAGE_SIZE". I further wonder why you
don't store directly into wqv->used, and go through %eax instead.

> -        "mov %%rsp,%%rsi;"
> +
> +        "mov %%ecx, %%eax;"
> +        "mov %%rsp, %%rsi;" /* Copy from the stack, into wqv->stack */
>  
>          /* check_wakeup_from_wait() longjmp()'s to this point. */
>          ".L_wq_resume: rep movsb;"
> -        "mov %%rsp,%%rsi;"
>  
>          ".L_skip:"
>          "pop %%r15; pop %%r14; pop %%r13;"
>          "pop %%r12; pop %%rbp; pop %%rbx;"
> -        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> -        : "0" (0), "1" (cpu_info), "2" (wqv->stack),
> +        : "=a" (used), "=D" (dummy),     "=c" (dummy),         "=&S" (dummy)

You can't validly drop & from =D and =c. If you want to stick to
going through %eax, I think that one wants & added as well and ...

> +        : "a" (0),     "D" (wqv->stack), "c" (get_cpu_info()),

... the (unused) input here dropped.

> @@ -220,14 +224,22 @@ void check_wakeup_from_wait(void)
>       * the rep movs in __prepare_to_wait(), it copies from wqv->stack over 
> the
>       * active stack.
>       *
> +     * We are also bound by __prepare_to_wait()'s output constraints, so %eax
> +     * needs to be wqv->used.
> +     *
>       * All other GPRs are available for use; they're either restored from
>       * wqv->stack or explicitly clobbered.
>       */
> -    asm volatile ( "mov %%rdi, %%rsp;"
> +    asm volatile ( "sub %%esp, %k[var];" /* var = delta to cpu_info */
> +                   "neg %k[var];"
> +                   "add %%ecx, %k[var];" /* var = -delta + wqv->used */
> +
> +                   "sub %[var], %%rsp;"  /* Adjust %rsp down to make room */
> +                   "mov %%rsp, %%rdi;"   /* Copy from wqv->stack, into the 
> stack */
>                     "jmp .L_wq_resume;"
> -                   :
> -                   : "S" (wqv->stack), "D" (wqv->esp),
> -                     "c" ((char *)get_cpu_info() - (char *)wqv->esp)
> +                   : "=D" (tmp), [var] "=&r" (tmp)
> +                   : "S" (wqv->stack), "c" (wqv->used), "a" (wqv->used),

If you want to stick to going through %eax, then I think you need to
make it an output here: "+a" (wqv->used), so it is clear that the
register is blocked from any other use throughout the asm(). Or you
could use "=&a" and copy %ecx into %eax inside the asm(). Both may
cause the compiler to emit dead code updating wqv->used right after
the asm(), so I think not going through %eax is the more desirable
approach (but I may well be overlooking a reason why directly
dealing with wqv->used in __prepare_to_wait() isn't an option).

Strictly speaking (in particular if [right now] there wasn't just a
branch after updating %rdi) you also again want "=&D" here.

Jan