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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Wei Liu <w...@xen.org>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Julien Grall <jul...@xen.org>
>> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>>
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
