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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/12/2019 16:52, Jan Beulich wrote:
> 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.

How to correctly mutate the registers and/or memory (which is specific
to the hypercall subop in some cases).

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

There is nothing hypothetical about it.  It really will go wrong when a
CPU gets offlined.

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

It simply continues again.  It will livelock only if the hypercall picks
a bad cpu all the time.

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

That is why I implemented it like this.  If it is a stray interaction
with a CPU offline, the next time around it will work fine.

Anything else is rather more broken.

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

Probably, yes.

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

Depends now the miss came about.  It certainly stands a far better
chance now of actually executing.

~Andrew

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