Re: [Xen-devel] [PATCH 5/6] xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.12.2019 23:30, Andrew Cooper wrote:
> Some hypercalls tasklets want to create a continuation, rather than fail the
> hypercall with a hard error.  By the time the tasklet is executing, it is too
> late to create the continuation, and even continue_hypercall_on_cpu() doesn't
> have enough state to do it correctly.

I think it would be quite nice if you made clear what piece of state
it is actually missing. To be honest, I don't recall anymore.

> All callers of continue_hypercall_on_cpu() have been updated to turn -ERESTART
> into a continuation, where appropriate modifications can be made to register
> and/or memory parameters.
> 
> This changes the continue_hypercall_on_cpu() behaviour to unconditionally
> create a hypercall continuation, in case the tasklet wants to use it, and then
> to have arch_hypercall_tasklet_result() cancel the continuation when a result
> is available.  None of these hypercalls are fast paths.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> 
> There is one RFC point.  The statement in the header file of "If this function
> returns 0 then the function is guaranteed to run at some point in the future."
> was never true.  In the case of a CPU miss, the hypercall would be blindly
> failed with -EINVAL.

"Was never true" sounds like "completely broken". Afaict it was true
in all cases except the purely hypothetical one of the tasklet ending
up executing on the wrong CPU.

> The current behaviour with this patch is to not cancel the continuation, which
> I think is less bad, but still not great.  Thoughts?

Well, that's a guest live lock then aiui. Is there any way for the
guest to make it out of there? If not, perhaps it'd be "better" to
crash the guest? (I don't suppose there's anything we can do to
still make the tasklet run on the intended CPU.)

> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1489,6 +1489,7 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long 
> res)
>  {
>      struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
>  
> +    regs->pc += 4;  /* Skip over 'hvc #XEN_HYPERCALL_TAG' */
>      HYPERCALL_RESULT_REG(regs) = res;
>  }
>  
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -170,6 +170,13 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long 
> res)
>  {
>      struct cpu_user_regs *regs = &v->arch.user_regs;
>  
> +    /*
> +     * PV hypercalls are all 2-byte instructions (INT $0x82, SYSCALL).  HVM
> +     * hypercalls are all 3-byte instructions (VMCALL, VMMCALL).
> +     *
> +     * Move %rip forwards to complete the continuation.
> +     */
> +    regs->rip += 2 + is_hvm_vcpu(v);
>      regs->rax = res;
>  }

To leave the system in consistent state, perhaps better to call
hypercall_cancel_continuation() along with the PC/RIP updating?

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -96,9 +96,11 @@ void domctl_lock_release(void);
>  
>  /*
>   * Continue the current hypercall via func(data) on specified cpu.
> - * If this function returns 0 then the function is guaranteed to run at some
> - * point in the future. If this function returns an error code then the
> - * function has not been and will not be executed.
> + *
> + * This function returns -ERESTART in the success case, and a higher level
> + * caller is required to set up a hypercall continuation.  func() will be run
> + * at some point in the future.  If this function returns any other error 
> code
> + * then func() has not, and will not be executed.
>   */
>  int continue_hypercall_on_cpu(
>      unsigned int cpu, long (*func)(void *data), void *data);

How is this comment any better wrt the "was never true" comment
you've made above? The function still wouldn't be invoked in
case of a CPU miss.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel