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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@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.

The current behaviour with this patch is to not cancel the continuation, which
I think is less bad, but still not great.  Thoughts?
---
 xen/arch/arm/traps.c     |  1 +
 xen/arch/x86/hypercall.c |  7 +++++++
 xen/common/domain.c      |  9 +++++----
 xen/include/xen/domain.h | 11 ++++++++---
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a20474f87c..5d35d2b7e9 100644
--- 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;
 }
 
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 7f299d45c6..42d95f9b9a 100644
--- 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;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ab7e4d09c0..eb69db3078 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1665,7 +1665,7 @@ static void continue_hypercall_tasklet_handler(void *data)
 {
     struct migrate_info *info = data;
     struct vcpu *v = info->vcpu;
-    long res = -EINVAL;
+    long res = -ERESTART;
 
     /* Wait for vcpu to sleep so that we can access its register state. */
     vcpu_sleep_sync(v);
@@ -1675,7 +1675,8 @@ static void continue_hypercall_tasklet_handler(void *data)
     if ( likely(info->cpu == smp_processor_id()) )
         res = info->func(info->data);
 
-    arch_hypercall_tasklet_result(v, res);
+    if ( res != -ERESTART )
+        arch_hypercall_tasklet_result(v, res);
 
     this_cpu(continue_info) = NULL;
 
@@ -1726,8 +1727,8 @@ int continue_hypercall_on_cpu(
 
     tasklet_schedule_on_cpu(&info->vcpu->continue_hypercall_tasklet, cpu);
 
-    /* Dummy return value will be overwritten by tasklet. */
-    return 0;
+    /* Start a continuation.  Value will be overwritten by the tasklet. */
+    return -ERESTART;
 }
 
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1cb205d977..83c737bca4 100644
--- 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);
@@ -106,6 +108,9 @@ int continue_hypercall_on_cpu(
 /*
  * Companion to continue_hypercall_on_cpu(), to feed func()'s result back into
  * vcpu regsiter state.
+ *
+ * Must undo the effects of the hypercall continuation created by
+ * continue_hypercall_on_cpu()'s caller.
  */
 void arch_hypercall_tasklet_result(struct vcpu *v, long res);
 
-- 
2.11.0


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