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
