Re: [PATCH 3/5] xen/wait: Minor asm improvements

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 09:18, Andrew Cooper wrote:
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -151,13 +151,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu 
> *wqv)
>       * copies in from wqv->stack over the active stack.
>       */
>      asm volatile (
> -        "push %%rax; push %%rbx; push %%rdx; push %%rbp;"
> -        "push %%r8;  push %%r9;  push %%r10; push %%r11;"
> -        "push %%r12; push %%r13; push %%r14; push %%r15;"
> +        "push %%rbx; push %%rbp; push %%r12;"
> +        "push %%r13; push %%r14; push %%r15;"
>  
>          "sub %%esp,%%ecx;"
> -        "cmp %3,%%ecx;"
> -        "ja .L_skip;"
> +        "cmp %[sz], %%ecx;"
> +        "ja .L_skip;"       /* Bail if >4k */
>          "mov %%rsp,%%rsi;"
>  
>          /* check_wakeup_from_wait() longjmp()'s to this point. */
> @@ -165,12 +164,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu 
> *wqv)
>          "mov %%rsp,%%rsi;"
>  
>          ".L_skip:"
> -        "pop %%r15; pop %%r14; pop %%r13; pop %%r12;"
> -        "pop %%r11; pop %%r10; pop %%r9;  pop %%r8;"
> -        "pop %%rbp; pop %%rdx; pop %%rbx; pop %%rax"
> +        "pop %%r15; pop %%r14; pop %%r13;"
> +        "pop %%r12; pop %%rbp; pop %%rbx;"
>          : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
> -        : "i" (PAGE_SIZE), "0" (0), "1" (cpu_info), "2" (wqv->stack)
> -        : "memory" );
> +        : "0" (0), "1" (cpu_info), "2" (wqv->stack),
> +          [sz] "i" (PAGE_SIZE)
> +        : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
>  
>      if ( unlikely(wqv->esp == 0) )
>      {
> @@ -224,11 +223,12 @@ void check_wakeup_from_wait(void)
>       * All other GPRs are available for use; they're either restored from
>       * wqv->stack or explicitly clobbered.
>       */

Ah, this is where the comment stops being misleading. I think it would
be better if you had it correct there and adjusted it here.

> -    asm volatile (
> -        "mov %1,%%"__OP"sp; jmp .L_wq_resume;"
> -        : : "S" (wqv->stack), "D" (wqv->esp),
> -          "c" ((char *)get_cpu_info() - (char *)wqv->esp)
> -        : "memory" );
> +    asm volatile ( "mov %%rdi, %%rsp;"
> +                   "jmp .L_wq_resume;"

Minor remark: No need for trailing semicolons like this one. Anyway:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan