[Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Andrew Cooper]

This is the Post "x86/smt: Support for enabling/disabling SMT at runtime" work
which stumbled into discovering XSA-296, for the purpose of making
continuations more efficient.

From testing this series, I have re-confirmed the previous reported
observation that:

  # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done

in dom0 eventually causes the serial console to cease working (wedge midway
through printing a line).

There are sporadic "Broke affinity for IRQ26, new: ffff" messages, but the
serial always seems to break shortly after the first "Broke affinity for
IRQ30, new: ffff".  Both IRQs are non-descript PCI-MSI/-X interrupts bound to
dom0.

Andrew Cooper (6):
  xen/tasklet: Fix return value truncation on arm64
  xen/tasklet: Switch data parameter from unsigned long to void *.
  xen/domctl: Consolidate hypercall continuation handling at the top level
  xen/hypercall: Cope with -ERESTART on more hypercall paths
  xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu()
  x86/smt: Don't use -EBUSY for smt_up_down_helper() continuations

 xen/arch/arm/traps.c                      |  8 +++++++
 xen/arch/x86/domctl.c                     |  5 ++++-
 xen/arch/x86/hvm/hvm.c                    |  6 ++---
 xen/arch/x86/hvm/vlapic.c                 |  8 +++----
 xen/arch/x86/hypercall.c                  | 14 ++++++++++++
 xen/arch/x86/mm/hap/hap.c                 |  3 +--
 xen/arch/x86/mm/shadow/common.c           |  7 +++---
 xen/arch/x86/platform_hypercall.c         | 14 ++++++++++--
 xen/arch/x86/sysctl.c                     |  9 ++++++--
 xen/common/compat/domain.c                |  9 ++++----
 xen/common/domain.c                       | 37 +++++++++++++++++--------------
 xen/common/domctl.c                       | 19 +++++-----------
 xen/common/kexec.c                        | 20 +++++++++++++----
 xen/common/keyhandler.c                   | 19 ++++++++--------
 xen/common/stop_machine.c                 |  5 +++--
 xen/common/sysctl.c                       | 13 +++++++++--
 xen/common/tasklet.c                      |  6 ++---
 xen/common/trace.c                        |  4 ++--
 xen/drivers/char/console.c                |  4 ++--
 xen/drivers/passthrough/amd/iommu_guest.c |  7 +++---
 xen/drivers/passthrough/amd/iommu_init.c  |  6 ++---
 xen/drivers/passthrough/iommu.c           |  4 ++--
 xen/drivers/passthrough/vtd/iommu.c       |  4 ++--
 xen/include/asm-arm/regs.h                |  2 --
 xen/include/asm-x86/regs.h                |  2 --
 xen/include/asm-x86/shadow.h              |  5 ++---
 xen/include/xen/domain.h                  | 17 +++++++++++---
 xen/include/xen/tasklet.h                 | 10 ++++-----
 28 files changed, 158 insertions(+), 109 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/6] xen/tasklet: Fix return value truncation on arm64

[Andrew Cooper]

The use of return_reg() assumes ARM's 32bit ABI.  Therefore, a failure such as
-EINVAL will appear as a large positive number near 4 billion to a 64bit ARM
guest which happens to use continue_hypercall_on_cpu().

Introduce a new arch_hypercall_tasklet_result() hook which is implemented by
both architectures, and drop the return_reg() macros.  This logic will be
extended in a later change to make continuations out of the tasklet work.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.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>

This was posted on its own previously, but is reset back to v1 now it is in
its series.  This can't be made static inline due to header constraints, but
there is no inherent issue with doing so if the headers become less tangled.

Failing the call with -EINVAL for missing the correct CPU is very rude, and
addressed in a later patch.
---
 xen/arch/arm/traps.c       | 7 +++++++
 xen/arch/x86/hypercall.c   | 7 +++++++
 xen/common/domain.c        | 9 +++++++--
 xen/include/asm-arm/regs.h | 2 --
 xen/include/asm-x86/regs.h | 2 --
 xen/include/xen/domain.h   | 6 ++++++
 6 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d028ec9224..a20474f87c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1485,6 +1485,13 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
         regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
 }
 
+void arch_hypercall_tasklet_result(struct vcpu *v, long res)
+{
+    struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
+
+    HYPERCALL_RESULT_REG(regs) = res;
+}
+
 static bool check_multicall_32bit_clean(struct multicall_entry *multi)
 {
     int i;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 1d42702c6a..7f299d45c6 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -166,6 +166,13 @@ unsigned long hypercall_create_continuation(
 
 #undef NEXT_ARG
 
+void arch_hypercall_tasklet_result(struct vcpu *v, long res)
+{
+    struct cpu_user_regs *regs = &v->arch.user_regs;
+
+    regs->rax = res;
+}
+
 int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
                                 unsigned int mask, ...)
 {
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 611116c7fc..ccf689fcbe 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1665,13 +1665,18 @@ static void continue_hypercall_tasklet_handler(unsigned long _info)
 {
     struct migrate_info *info = (struct migrate_info *)_info;
     struct vcpu *v = info->vcpu;
+    long res = -EINVAL;
 
     /* Wait for vcpu to sleep so that we can access its register state. */
     vcpu_sleep_sync(v);
 
     this_cpu(continue_info) = info;
-    return_reg(v) = (info->cpu == smp_processor_id())
-        ? info->func(info->data) : -EINVAL;
+
+    if ( likely(info->cpu == smp_processor_id()) )
+        res = info->func(info->data);
+
+    arch_hypercall_tasklet_result(v, res);
+
     this_cpu(continue_info) = NULL;
 
     if ( info->nest-- == 0 )
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 0e3e56b452..ec091a28a2 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -57,8 +57,6 @@ static inline bool guest_mode(const struct cpu_user_regs *r)
     return (diff == 0);
 }
 
-#define return_reg(v) ((v)->arch.cpu_info->guest_cpu_user_regs.r0)
-
 register_t get_user_reg(struct cpu_user_regs *regs, int reg);
 void set_user_reg(struct cpu_user_regs *regs, int reg, register_t val);
 
diff --git a/xen/include/asm-x86/regs.h b/xen/include/asm-x86/regs.h
index 725a664e0a..dc00b854e3 100644
--- a/xen/include/asm-x86/regs.h
+++ b/xen/include/asm-x86/regs.h
@@ -15,6 +15,4 @@
     (diff == 0);                                                              \
 })
 
-#define return_reg(v) ((v)->arch.user_regs.rax)
-
 #endif /* __X86_REGS_H__ */
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 769302057b..1cb205d977 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -103,6 +103,12 @@ void domctl_lock_release(void);
 int continue_hypercall_on_cpu(
     unsigned int cpu, long (*func)(void *data), void *data);
 
+/*
+ * Companion to continue_hypercall_on_cpu(), to feed func()'s result back into
+ * vcpu regsiter state.
+ */
+void arch_hypercall_tasklet_result(struct vcpu *v, long res);
+
 extern unsigned int xen_processor_pmbits;
 
 extern bool_t opt_dom0_vcpus_pin;
-- 
2.11.0


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

[Xen-devel] [PATCH 2/6] xen/tasklet: Switch data parameter from unsigned long to void *.

[Andrew Cooper]

Most users pass a vcpu pointer, and only stopmachine_action() takes an integer
parameter.  Switch to using void * to substantially reduce the number of
explicit casts.

No functional change.

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>
---
 xen/arch/x86/hvm/hvm.c                    |  6 ++----
 xen/arch/x86/hvm/vlapic.c                 |  8 +++-----
 xen/arch/x86/mm/shadow/common.c           |  4 ++--
 xen/common/domain.c                       | 15 ++++++---------
 xen/common/keyhandler.c                   | 19 +++++++++----------
 xen/common/stop_machine.c                 |  5 +++--
 xen/common/tasklet.c                      |  6 ++----
 xen/common/trace.c                        |  4 ++--
 xen/drivers/char/console.c                |  4 ++--
 xen/drivers/passthrough/amd/iommu_guest.c |  7 +++----
 xen/drivers/passthrough/amd/iommu_init.c  |  6 +++---
 xen/drivers/passthrough/iommu.c           |  4 ++--
 xen/drivers/passthrough/vtd/iommu.c       |  4 ++--
 xen/include/asm-x86/shadow.h              |  5 ++---
 xen/include/xen/tasklet.h                 | 10 ++++------
 15 files changed, 47 insertions(+), 60 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 47573f71b8..d909fec30d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1525,10 +1525,8 @@ int hvm_vcpu_initialise(struct vcpu *v)
     if ( (rc = hvm_funcs.vcpu_initialise(v)) != 0 ) /* teardown: hvm_funcs.vcpu_destroy */
         goto fail3;
 
-    softirq_tasklet_init(
-        &v->arch.hvm.assert_evtchn_irq_tasklet,
-        (void(*)(unsigned long))hvm_assert_evtchn_irq,
-        (unsigned long)v);
+    softirq_tasklet_init(&v->arch.hvm.assert_evtchn_irq_tasklet,
+                         (void (*)(void *))hvm_assert_evtchn_irq, v);
 
     v->arch.hvm.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
 
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 9b8afb72e8..06235f183e 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -309,9 +309,9 @@ static void vlapic_init_sipi_one(struct vcpu *target, uint32_t icr)
     vcpu_unpause(target);
 }
 
-static void vlapic_init_sipi_action(unsigned long _vcpu)
+static void vlapic_init_sipi_action(void *data)
 {
-    struct vcpu *origin = (struct vcpu *)_vcpu;
+    struct vcpu *origin = data;
     uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
     uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
     uint32_t short_hand = icr & APIC_SHORT_MASK;
@@ -1637,9 +1637,7 @@ int vlapic_init(struct vcpu *v)
 
     spin_lock_init(&vlapic->esr_lock);
 
-    tasklet_init(&vlapic->init_sipi.tasklet,
-                 vlapic_init_sipi_action,
-                 (unsigned long)v);
+    tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
 
     if ( v->vcpu_id == 0 )
         register_mmio_handler(v->domain, &vlapic_mmio_ops);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 314d837602..6212ec2c4a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3471,9 +3471,9 @@ void shadow_audit_tables(struct vcpu *v)
 
 #ifdef CONFIG_PV
 
-void pv_l1tf_tasklet(unsigned long data)
+void pv_l1tf_tasklet(void *data)
 {
-    struct domain *d = (void *)data;
+    struct domain *d = data;
 
     domain_pause(d);
     paging_lock(d);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ccf689fcbe..865a1cb9d7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -150,7 +150,7 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
 
     spin_lock_init(&v->virq_lock);
 
-    tasklet_init(&v->continue_hypercall_tasklet, NULL, 0);
+    tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
 
     grant_table_init_vcpu(v);
 
@@ -1661,9 +1661,9 @@ struct migrate_info {
 
 static DEFINE_PER_CPU(struct migrate_info *, continue_info);
 
-static void continue_hypercall_tasklet_handler(unsigned long _info)
+static void continue_hypercall_tasklet_handler(void *data)
 {
-    struct migrate_info *info = (struct migrate_info *)_info;
+    struct migrate_info *info = data;
     struct vcpu *v = info->vcpu;
     long res = -EINVAL;
 
@@ -1707,12 +1707,9 @@ int continue_hypercall_on_cpu(
         info->vcpu = curr;
         info->nest = 0;
 
-        tasklet_kill(
-            &curr->continue_hypercall_tasklet);
-        tasklet_init(
-            &curr->continue_hypercall_tasklet,
-            continue_hypercall_tasklet_handler,
-            (unsigned long)info);
+        tasklet_kill(&curr->continue_hypercall_tasklet);
+        tasklet_init(&curr->continue_hypercall_tasklet,
+                     continue_hypercall_tasklet_handler, info);
 
         get_knownalive_domain(curr->domain);
         vcpu_pause_nosync(curr);
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index dc6396b225..f50490d0f3 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -71,12 +71,12 @@ static struct keyhandler {
 #undef KEYHANDLER
 };
 
-static void keypress_action(unsigned long unused)
+static void keypress_action(void *unused)
 {
     handle_keypress(keypress_key, NULL);
 }
 
-static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
+static DECLARE_TASKLET(keypress_tasklet, keypress_action, NULL);
 
 void handle_keypress(unsigned char key, struct cpu_user_regs *regs)
 {
@@ -199,11 +199,11 @@ static void dump_registers(unsigned char key, struct cpu_user_regs *regs)
     watchdog_enable();
 }
 
-static DECLARE_TASKLET(dump_hwdom_tasklet, NULL, 0);
+static DECLARE_TASKLET(dump_hwdom_tasklet, NULL, NULL);
 
-static void dump_hwdom_action(unsigned long arg)
+static void dump_hwdom_action(void *data)
 {
-    struct vcpu *v = (void *)arg;
+    struct vcpu *v = data;
 
     for ( ; ; )
     {
@@ -212,7 +212,7 @@ static void dump_hwdom_action(unsigned long arg)
             break;
         if ( softirq_pending(smp_processor_id()) )
         {
-            dump_hwdom_tasklet.data = (unsigned long)v;
+            dump_hwdom_tasklet.data = v;
             tasklet_schedule_on_cpu(&dump_hwdom_tasklet, v->processor);
             break;
         }
@@ -233,8 +233,7 @@ static void dump_hwdom_registers(unsigned char key)
         if ( alt_key_handling && softirq_pending(smp_processor_id()) )
         {
             tasklet_kill(&dump_hwdom_tasklet);
-            tasklet_init(&dump_hwdom_tasklet, dump_hwdom_action,
-                         (unsigned long)v);
+            tasklet_init(&dump_hwdom_tasklet, dump_hwdom_action, v);
             tasklet_schedule_on_cpu(&dump_hwdom_tasklet, v->processor);
             return;
         }
@@ -433,7 +432,7 @@ static void read_clocks(unsigned char key)
            maxdif_cycles, sumdif_cycles/count, count, dif_cycles);
 }
 
-static void run_all_nonirq_keyhandlers(unsigned long unused)
+static void run_all_nonirq_keyhandlers(void *unused)
 {
     /* Fire all the non-IRQ-context diagnostic keyhandlers */
     struct keyhandler *h;
@@ -455,7 +454,7 @@ static void run_all_nonirq_keyhandlers(unsigned long unused)
 }
 
 static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
-                       run_all_nonirq_keyhandlers, 0);
+                       run_all_nonirq_keyhandlers, NULL);
 
 static void run_all_keyhandlers(unsigned char key, struct cpu_user_regs *regs)
 {
diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
index 681b40906d..33d9602217 100644
--- a/xen/common/stop_machine.c
+++ b/xen/common/stop_machine.c
@@ -134,8 +134,9 @@ int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
     return ret;
 }
 
-static void stopmachine_action(unsigned long cpu)
+static void stopmachine_action(void *data)
 {
+    unsigned int cpu = (unsigned long)data;
     enum stopmachine_state state = STOPMACHINE_START;
 
     BUG_ON(cpu != smp_processor_id());
@@ -181,7 +182,7 @@ static int cpu_callback(
 
     if ( action == CPU_UP_PREPARE )
         tasklet_init(&per_cpu(stopmachine_tasklet, cpu),
-                     stopmachine_action, cpu);
+                     stopmachine_action, hcpu);
 
     return NOTIFY_DONE;
 }
diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
index d4fea3151c..ac89511a09 100644
--- a/xen/common/tasklet.c
+++ b/xen/common/tasklet.c
@@ -199,8 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, struct list_head *list)
     spin_unlock_irqrestore(&tasklet_lock, flags);
 }
 
-void tasklet_init(
-    struct tasklet *t, void (*func)(unsigned long), unsigned long data)
+void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
 {
     memset(t, 0, sizeof(*t));
     INIT_LIST_HEAD(&t->list);
@@ -209,8 +208,7 @@ void tasklet_init(
     t->data = data;
 }
 
-void softirq_tasklet_init(
-    struct tasklet *t, void (*func)(unsigned long), unsigned long data)
+void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
 {
     tasklet_init(t, func, data);
     t->is_softirq = 1;
diff --git a/xen/common/trace.c b/xen/common/trace.c
index d1ef81407b..ebfc735b31 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -662,12 +662,12 @@ static inline void insert_lost_records(struct t_buf *buf)
  * Notification is performed in qtasklet to avoid deadlocks with contexts
  * which __trace_var() may be called from (e.g., scheduler critical regions).
  */
-static void trace_notify_dom0(unsigned long unused)
+static void trace_notify_dom0(void *unused)
 {
     send_global_virq(VIRQ_TBUF);
 }
 static DECLARE_SOFTIRQ_TASKLET(trace_notify_dom0_tasklet,
-                               trace_notify_dom0, 0);
+                               trace_notify_dom0, NULL);
 
 /**
  * __trace_var - Enters a trace tuple into the trace buffer for the current CPU.
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 844c5de74e..b31d789a5d 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -505,12 +505,12 @@ static void serial_rx(char c, struct cpu_user_regs *regs)
     __serial_rx(c, regs);
 }
 
-static void notify_dom0_con_ring(unsigned long unused)
+static void notify_dom0_con_ring(void *unused)
 {
     send_global_virq(VIRQ_CON_RING);
 }
 static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
-                               notify_dom0_con_ring, 0);
+                               notify_dom0_con_ring, NULL);
 
 #ifdef CONFIG_X86
 static inline void xen_console_write_debug_port(const char *buf, size_t len)
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 1f2bcfbe15..4ed6519e6e 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -435,11 +435,11 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
     return 0;
 }
 
-static void guest_iommu_process_command(unsigned long _d)
+static void guest_iommu_process_command(void *data)
 {
     unsigned long opcode, tail, head, entries_per_page, cmd_mfn;
     cmd_entry_t *cmd, *cmd_base;
-    struct domain *d = (struct domain *)_d;
+    struct domain *d = data;
     struct guest_iommu *iommu;
 
     iommu = domain_iommu(d);
@@ -837,8 +837,7 @@ int guest_iommu_init(struct domain* d)
     iommu->domain = d;
     hd->arch.g_iommu = iommu;
 
-    tasklet_init(&iommu->cmd_buffer_tasklet,
-                 guest_iommu_process_command, (unsigned long)d);
+    tasklet_init(&iommu->cmd_buffer_tasklet, guest_iommu_process_command, d);
 
     spin_lock_init(&iommu->lock);
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 9509124158..2f26fed4a3 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -33,8 +33,8 @@
 static int __initdata nr_amd_iommus;
 static bool __initdata pci_init;
 
-static void do_amd_iommu_irq(unsigned long data);
-static DECLARE_SOFTIRQ_TASKLET(amd_iommu_irq_tasklet, do_amd_iommu_irq, 0);
+static void do_amd_iommu_irq(void *data);
+static DECLARE_SOFTIRQ_TASKLET(amd_iommu_irq_tasklet, do_amd_iommu_irq, NULL);
 
 unsigned int __read_mostly ivrs_bdf_entries;
 u8 __read_mostly ivhd_type;
@@ -723,7 +723,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
-static void do_amd_iommu_irq(unsigned long data)
+static void do_amd_iommu_irq(void *unused)
 {
     struct amd_iommu *iommu;
 
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 05e740757a..4e19cf56cc 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -369,7 +369,7 @@ int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
     return iommu_call(hd->platform_ops, lookup_page, d, dfn, mfn, flags);
 }
 
-static void iommu_free_pagetables(unsigned long unused)
+static void iommu_free_pagetables(void *unused)
 {
     do {
         struct page_info *pg;
@@ -500,7 +500,7 @@ int __init iommu_setup(void)
                iommu_hwdom_passthrough ? "Passthrough" :
                iommu_hwdom_strict ? "Strict" : "Relaxed");
         printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis");
-        tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, 0);
+        tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, NULL);
     }
 
     return rc;
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 664556aa51..c56df8e58e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -965,7 +965,7 @@ static void __do_iommu_page_fault(struct vtd_iommu *iommu)
     }
 }
 
-static void do_iommu_page_fault(unsigned long data)
+static void do_iommu_page_fault(void *unused)
 {
     struct acpi_drhd_unit *drhd;
 
@@ -2309,7 +2309,7 @@ static int __init vtd_setup(void)
         }
     }
 
-    softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, 0);
+    softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, NULL);
 
     if ( !iommu_qinval && iommu_intremap )
     {
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 8ebb89c027..907c71f497 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -230,7 +230,7 @@ static inline bool pv_l1tf_check_l4e(struct domain *d, l4_pgentry_t l4e)
     return pv_l1tf_check_pte(d, 4, l4e.l4);
 }
 
-void pv_l1tf_tasklet(unsigned long data);
+void pv_l1tf_tasklet(void *data);
 
 static inline void pv_l1tf_domain_init(struct domain *d)
 {
@@ -238,8 +238,7 @@ static inline void pv_l1tf_domain_init(struct domain *d)
                                                   : opt_pv_l1tf_domu;
 
 #ifdef CONFIG_SHADOW_PAGING
-    tasklet_init(&d->arch.paging.shadow.pv_l1tf_tasklet,
-                 pv_l1tf_tasklet, (unsigned long)d);
+    tasklet_init(&d->arch.paging.shadow.pv_l1tf_tasklet, pv_l1tf_tasklet, d);
 #endif
 }
 
diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index bc9ddace6d..ea73171f6e 100644
--- a/xen/include/xen/tasklet.h
+++ b/xen/include/xen/tasklet.h
@@ -21,8 +21,8 @@ struct tasklet
     bool_t is_softirq;
     bool_t is_running;
     bool_t is_dead;
-    void (*func)(unsigned long);
-    unsigned long data;
+    void (*func)(void *);
+    void *data;
 };
 
 #define _DECLARE_TASKLET(name, func, data, softirq)                     \
@@ -59,10 +59,8 @@ void tasklet_schedule_on_cpu(struct tasklet *t, unsigned int cpu);
 void tasklet_schedule(struct tasklet *t);
 void do_tasklet(void);
 void tasklet_kill(struct tasklet *t);
-void tasklet_init(
-    struct tasklet *t, void (*func)(unsigned long), unsigned long data);
-void softirq_tasklet_init(
-    struct tasklet *t, void (*func)(unsigned long), unsigned long data);
+void tasklet_init(struct tasklet *t, void (*func)(void *), void *data);
+void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data);
 void tasklet_subsys_init(void);
 
 #endif /* __XEN_TASKLET_H__ */
-- 
2.11.0


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

[Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Andrew Cooper]

More paths are going start using hypercall continuations.  We could add extra
calls to hypercall_create_continuation() but it is much easier to handle
-ERESTART once at the top level.

One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
compatibility in a security fix, turn a DOMCTL continuation into
__HYPERVISOR_arch_1.  This remains as it was, gaining a comment explaining
what is going on.

With -ERESTART handling in place, the !domctl_lock_acquire() path can use the
normal exit path, instead of opencoding a subset of it locally.

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>
---
 xen/arch/x86/domctl.c           |  5 ++++-
 xen/arch/x86/mm/hap/hap.c       |  3 +--
 xen/arch/x86/mm/shadow/common.c |  3 +--
 xen/common/domctl.c             | 19 +++++--------------
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b461aadbd6..2fa0e7dda5 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -326,9 +326,12 @@ long arch_do_domctl(
 
     switch ( domctl->cmd )
     {
-
     case XEN_DOMCTL_shadow_op:
         ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
+        /*
+         * Continuations from paging_domctl() switch index to arch_1, and
+         * can't use the common domctl continuation path.
+         */
         if ( ret == -ERESTART )
             return hypercall_create_continuation(__HYPERVISOR_arch_1,
                                                  "h", u_domctl);
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..3996e17b7e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -600,8 +600,7 @@ int hap_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
         paging_unlock(d);
         if ( preempted )
             /* Not finished.  Set up to re-run the call. */
-            rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
-                                               u_domctl);
+            rc = -ERESTART;
         else
             /* Finished.  Return the new allocation */
             sc->mb = hap_get_allocation(d);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 6212ec2c4a..17ca21104f 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3400,8 +3400,7 @@ int shadow_domctl(struct domain *d,
         paging_unlock(d);
         if ( preempted )
             /* Not finished.  Set up to re-run the call. */
-            rc = hypercall_create_continuation(
-                __HYPERVISOR_domctl, "h", u_domctl);
+            rc = -ERESTART;
         else
             /* Finished.  Return the new allocation */
             sc->mb = shadow_get_allocation(d);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 03d0226039..cb0295085d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -415,10 +415,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     if ( !domctl_lock_acquire() )
     {
-        if ( d && d != dom_io )
-            rcu_unlock_domain(d);
-        return hypercall_create_continuation(
-            __HYPERVISOR_domctl, "h", u_domctl);
+        ret = -ERESTART;
+        goto domctl_out_unlock_domonly;
     }
 
     switch ( op->cmd )
@@ -438,9 +436,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         if ( guest_handle_is_null(op->u.vcpucontext.ctxt) )
         {
             ret = vcpu_reset(v);
-            if ( ret == -ERESTART )
-                ret = hypercall_create_continuation(
-                          __HYPERVISOR_domctl, "h", u_domctl);
             break;
         }
 
@@ -469,10 +464,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             domain_pause(d);
             ret = arch_set_info_guest(v, c);
             domain_unpause(d);
-
-            if ( ret == -ERESTART )
-                ret = hypercall_create_continuation(
-                          __HYPERVISOR_domctl, "h", u_domctl);
         }
 
         free_vcpu_guest_context(c.nat);
@@ -585,9 +576,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         domain_lock(d);
         ret = domain_kill(d);
         domain_unlock(d);
-        if ( ret == -ERESTART )
-            ret = hypercall_create_continuation(
-                __HYPERVISOR_domctl, "h", u_domctl);
         goto domctl_out_unlock_domonly;
 
     case XEN_DOMCTL_setnodeaffinity:
@@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     if ( copyback && __copy_to_guest(u_domctl, op, 1) )
         ret = -EFAULT;
 
+    if ( ret == -ERESTART )
+        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
+                                            "h", u_domctl);
     return ret;
 }
 
-- 
2.11.0


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

[Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Andrew Cooper]

These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
folding existing contination logic where applicable.

In addition:
 * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
   tight spinlock loop, and make the continuation logic common at the
   epilogue.
 * Contrary to the comment in the code, kexec_exec() does return in the
   KEXEC_REBOOT case, needs to pass ret back to the caller.

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>
---
 xen/arch/x86/platform_hypercall.c | 14 ++++++++++++--
 xen/common/compat/domain.c        |  9 ++++-----
 xen/common/domain.c               |  8 ++++----
 xen/common/kexec.c                | 20 ++++++++++++++++----
 xen/common/sysctl.c               | 13 +++++++++++--
 5 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index b19f6ec4ed..c0c209baac 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -201,9 +201,12 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
      * with this vcpu.
      */
     while ( !spin_trylock(&xenpf_lock) )
+    {
+        cpu_relax();
+
         if ( hypercall_preempt_check() )
-            return hypercall_create_continuation(
-                __HYPERVISOR_platform_op, "h", u_xenpf_op);
+            goto create_continuation;
+    }
 
     switch ( op->cmd )
     {
@@ -816,6 +819,13 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
  out:
     spin_unlock(&xenpf_lock);
 
+    if ( ret == -ERESTART )
+    {
+    create_continuation:
+        ret = hypercall_create_continuation(__HYPERVISOR_platform_op,
+                                            "h", u_xenpf_op);
+    }
+
     return ret;
 }
 
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 11c6afc463..1a14403672 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -79,11 +79,6 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
 
             xfree(ctxt);
         }
-
-        if ( rc == -ERESTART )
-            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
-                                               cmd, vcpuid, arg);
-
         break;
     }
 
@@ -130,6 +125,10 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
         break;
     }
 
+    if ( rc == -ERESTART )
+        rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
+                                           cmd, vcpuid, arg);
+
     return rc;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 865a1cb9d7..ab7e4d09c0 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1422,10 +1422,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EINVAL;
 
         rc = arch_initialise_vcpu(v, arg);
-        if ( rc == -ERESTART )
-            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
-                                               cmd, vcpuid, arg);
-
         break;
 
     case VCPUOP_up:
@@ -1598,6 +1594,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     }
 
+    if ( rc == -ERESTART )
+        rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
+                                           cmd, vcpuid, arg);
+
     return rc;
 }
 
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index a262cc5a18..2fca75cec0 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -842,7 +842,7 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
         break;
     }
 
-    return -EINVAL; /* never reached */
+    return ret;
 }
 
 static int kexec_swap_images(int type, struct kexec_image *new,
@@ -1220,7 +1220,7 @@ static int do_kexec_op_internal(unsigned long op,
         return ret;
 
     if ( test_and_set_bit(KEXEC_FLAG_IN_HYPERCALL, &kexec_flags) )
-        return hypercall_create_continuation(__HYPERVISOR_kexec_op, "lh", op, uarg);
+        return -ERESTART;
 
     switch ( op )
     {
@@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long op,
 
 long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
 {
-    return do_kexec_op_internal(op, uarg, 0);
+    int ret = do_kexec_op_internal(op, uarg, 0);
+
+    if ( ret == -ERESTART )
+        ret = hypercall_create_continuation(__HYPERVISOR_kexec_op,
+                                            "lh", op, uarg);
+
+    return ret;
 }
 
 #ifdef CONFIG_COMPAT
 int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
 {
-    return do_kexec_op_internal(op, uarg, 1);
+    int ret = do_kexec_op_internal(op, uarg, 1);
+
+    if ( ret == -ERESTART )
+        ret = hypercall_create_continuation(__HYPERVISOR_kexec_op,
+                                            "lh", op, uarg);
+
+    return ret;
 }
 #endif
 
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index f88a285e7f..7b55047bb9 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -51,9 +51,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
      * with this vcpu.
      */
     while ( !spin_trylock(&sysctl_lock) )
+    {
+        cpu_relax();
+
         if ( hypercall_preempt_check() )
-            return hypercall_create_continuation(
-                __HYPERVISOR_sysctl, "h", u_sysctl);
+            goto create_continuation;
+    }
 
     switch ( op->cmd )
     {
@@ -516,6 +519,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
          __copy_to_guest(u_sysctl, op, 1) )
         ret = -EFAULT;
 
+    if ( ret == -ERESTART )
+    {
+    create_continuation:
+        ret = hypercall_create_continuation(__HYPERVISOR_sysctl, "h", u_sysctl);
+    }
+
     return ret;
 }
 
-- 
2.11.0


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

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

[Andrew Cooper]

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

[Xen-devel] [PATCH 6/6] x86/smt: Don't use -EBUSY for smt_up_down_helper() continuations

[Andrew Cooper]

Tasklet context is now capable of using handling continuations.  Use this
rather than -EBUSY as it is a more efficient way to restart the hypercall.

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>
---
 xen/arch/x86/sysctl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 4a76f0f47f..06955fdc3e 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -85,6 +85,9 @@ long cpu_up_helper(void *data)
         /* On EBUSY, flush RCU work and have one more go. */
         rcu_barrier();
         ret = cpu_up(cpu);
+
+        if ( ret == -EBUSY )
+            ret = -ERESTART;
     }
 
     if ( !ret && !opt_smt &&
@@ -110,6 +113,9 @@ long cpu_down_helper(void *data)
         /* On EBUSY, flush RCU work and have one more go. */
         rcu_barrier();
         ret = cpu_down(cpu);
+
+        if ( ret == -EBUSY )
+            ret = -ERESTART;
     }
     return ret;
 }
@@ -143,8 +149,7 @@ static long smt_up_down_helper(void *data)
          */
         if ( ret != -EEXIST && general_preempt_check() )
         {
-            /* In tasklet context - can't create a contination. */
-            ret = -EBUSY;
+            ret = -ERESTART;
             break;
         }
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Jan Beulich]

On 05.12.2019 23:30, Andrew Cooper wrote:
> From testing this series, I have re-confirmed the previous reported
> observation that:
> 
>   # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done
> 
> in dom0 eventually causes the serial console to cease working (wedge midway
> through printing a line).
> 
> There are sporadic "Broke affinity for IRQ26, new: ffff" messages, but the
> serial always seems to break shortly after the first "Broke affinity for
> IRQ30, new: ffff".  Both IRQs are non-descript PCI-MSI/-X interrupts bound to
> dom0.

And neither IRQ30 not IRQ26 are the serial ones? And serial does
use an IRQ (i.e. isn't running in polling mode)? I'll see if I
can repro (and then maybe be able to debug).

Jan

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

Re: [Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Andrew Cooper]

On 06/12/2019 09:58, Jan Beulich wrote:
> On 05.12.2019 23:30, Andrew Cooper wrote:
>> From testing this series, I have re-confirmed the previous reported
>> observation that:
>>
>>   # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done
>>
>> in dom0 eventually causes the serial console to cease working (wedge midway
>> through printing a line).
>>
>> There are sporadic "Broke affinity for IRQ26, new: ffff" messages, but the
>> serial always seems to break shortly after the first "Broke affinity for
>> IRQ30, new: ffff".  Both IRQs are non-descript PCI-MSI/-X interrupts bound to
>> dom0.
> And neither IRQ30 not IRQ26 are the serial ones? And serial does
> use an IRQ (i.e. isn't running in polling mode)? I'll see if I
> can repro (and then maybe be able to debug).

Serial uses IRQ0 and never has affinity changes as it is always bound to
CPU0.

Given our recent fun with the PEOI stack, that was going to be my next
port of call.  However everything else in the system looks to be working
fine, including the disks and network, suggesting that we aren't losing
other interrupts.

~Andrew

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

Re: [Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Jan Beulich]

On 06.12.2019 11:14, Andrew Cooper wrote:
> On 06/12/2019 09:58, Jan Beulich wrote:
>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>> From testing this series, I have re-confirmed the previous reported
>>> observation that:
>>>
>>>   # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done
>>>
>>> in dom0 eventually causes the serial console to cease working (wedge midway
>>> through printing a line).
>>>
>>> There are sporadic "Broke affinity for IRQ26, new: ffff" messages, but the
>>> serial always seems to break shortly after the first "Broke affinity for
>>> IRQ30, new: ffff".  Both IRQs are non-descript PCI-MSI/-X interrupts bound to
>>> dom0.
>> And neither IRQ30 not IRQ26 are the serial ones? And serial does
>> use an IRQ (i.e. isn't running in polling mode)? I'll see if I
>> can repro (and then maybe be able to debug).
> 
> Serial uses IRQ0 and never has affinity changes as it is always bound to
> CPU0.

IRQ0? DYM IRQ3 or IRQ4? (In any event the important part is for it
to be in the ISA range, rather than the PCI one.)

> Given our recent fun with the PEOI stack, that was going to be my next
> port of call.  However everything else in the system looks to be working
> fine, including the disks and network, suggesting that we aren't losing
> other interrupts.

The high priority vector we use for serial makes the PEOI stack
unrelated afaict, as mentioned in the discussion there as well.

Jan

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

Re: [Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets

[Andrew Cooper]

On 06/12/2019 10:18, Jan Beulich wrote:
> On 06.12.2019 11:14, Andrew Cooper wrote:
>> On 06/12/2019 09:58, Jan Beulich wrote:
>>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>>> From testing this series, I have re-confirmed the previous reported
>>>> observation that:
>>>>
>>>>   # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done
>>>>
>>>> in dom0 eventually causes the serial console to cease working (wedge midway
>>>> through printing a line).
>>>>
>>>> There are sporadic "Broke affinity for IRQ26, new: ffff" messages, but the
>>>> serial always seems to break shortly after the first "Broke affinity for
>>>> IRQ30, new: ffff".  Both IRQs are non-descript PCI-MSI/-X interrupts bound to
>>>> dom0.
>>> And neither IRQ30 not IRQ26 are the serial ones? And serial does
>>> use an IRQ (i.e. isn't running in polling mode)? I'll see if I
>>> can repro (and then maybe be able to debug).
>> Serial uses IRQ0 and never has affinity changes as it is always bound to
>> CPU0.
> IRQ0? DYM IRQ3 or IRQ4? (In any event the important part is for it
> to be in the ISA range, rather than the PCI one.)

I do mean IRQ4, sorry.  Mixed it up with the timer IRQ.

On SMT disable, it does lose half of its affinity.

(XEN)    IRQ:   4 vec:f1 IO-APIC-edge    status=000 aff:{0-7}/{0,2,4,6}
drivers/char/ns16550.c#ns16550_interrupt()

So this is more likely to be related to a irq-migration than LAPIC acks.

~Andrew

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

Re: [Xen-devel] [PATCH 1/6] xen/tasklet: Fix return value truncation on arm64

[Julien Grall]

Hi Andrew,

On 05/12/2019 22:30, Andrew Cooper wrote:
> The use of return_reg() assumes ARM's 32bit ABI.  Therefore, a failure such as
> -EINVAL will appear as a large positive number near 4 billion to a 64bit ARM
> guest which happens to use continue_hypercall_on_cpu().
> 
> Introduce a new arch_hypercall_tasklet_result() hook which is implemented by
> both architectures, and drop the return_reg() macros.  This logic will be
> extended in a later change to make continuations out of the tasklet work.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/6] xen/tasklet: Switch data parameter from unsigned long to void *.

[Julien Grall]

Hi Andrew,

On 05/12/2019 22:30, Andrew Cooper wrote:
> Most users pass a vcpu pointer, and only stopmachine_action() takes an integer
> parameter.  Switch to using void * to substantially reduce the number of
> explicit casts.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Julien Grall]

Hi,

On 05/12/2019 22:30, Andrew Cooper wrote:
> More paths are going start using hypercall continuations.  We could add extra
> calls to hypercall_create_continuation() but it is much easier to handle
> -ERESTART once at the top level.
> 
> One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
> compatibility in a security fix, turn a DOMCTL continuation into
> __HYPERVISOR_arch_1.  This remains as it was, gaining a comment explaining
> what is going on.
> 
> With -ERESTART handling in place, the !domctl_lock_acquire() path can use the
> normal exit path, instead of opencoding a subset of it locally.
> 
> 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>
> ---
>   xen/arch/x86/domctl.c           |  5 ++++-
>   xen/arch/x86/mm/hap/hap.c       |  3 +--
>   xen/arch/x86/mm/shadow/common.c |  3 +--
>   xen/common/domctl.c             | 19 +++++--------------

You seem to have missed the hypercall_create_continuation() call in 
iommu_do_pci_domctl().

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Julien Grall]

Hi Andrew,

On 05/12/2019 22:30, Andrew Cooper wrote:
> These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
> folding existing contination logic where applicable.
> 
> In addition:
>   * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
>     tight spinlock loop, and make the continuation logic common at the
>     epilogue.
>   * Contrary to the comment in the code, kexec_exec() does return in the
>     KEXEC_REBOOT case, needs to pass ret back to the caller.

It is not entirely trivial to me that KEXEC_REBOOT refers to 
KEXEC_DEFAULT_TYPE. The more that if you look at the kexec_reboot() 
helper, it will not return (see BUG()). What may return is 
continue_hypercall_on_cpu().

So would it make sense to use KEXEC_DEFAULT_TYPE?

[...]

> @@ -816,6 +819,13 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>    out:
>       spin_unlock(&xenpf_lock);
>   
> +    if ( ret == -ERESTART )
> +    {
> +    create_continuation:

Shall we indent create_continuation the same way as out?

[...]

> @@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long op,
>   
>   long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
>   {
> -    return do_kexec_op_internal(op, uarg, 0);
> +    int ret = do_kexec_op_internal(op, uarg, 0);
Shouldn't it be long (or unsigned long) here? Otherwise, the return of 
hypercall_create_continuation() may be truncated.


> +
> +    if ( ret == -ERESTART )
> +        ret = hypercall_create_continuation(__HYPERVISOR_kexec_op,
> +                                            "lh", op, uarg);
> +
> +    return ret;
>   }

[...]

> @@ -516,6 +519,12 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>            __copy_to_guest(u_sysctl, op, 1) )
>           ret = -EFAULT;
>   
> +    if ( ret == -ERESTART )
> +    {
> +    create_continuation:


Similar question as the previous label create_continuation.

> +        ret = hypercall_create_continuation(__HYPERVISOR_sysctl, "h", u_sysctl);
> +    }
> +
>       return ret;
>   }
>   
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 2/6] xen/tasklet: Switch data parameter from unsigned long to void *.

[Jan Beulich]

On 05.12.2019 23:30, Andrew Cooper wrote:
> Most users pass a vcpu pointer, and only stopmachine_action() takes an integer
> parameter.  Switch to using void * to substantially reduce the number of
> explicit casts.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Jan Beulich]

On 05.12.2019 23:30, Andrew Cooper wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -326,9 +326,12 @@ long arch_do_domctl(
>  
>      switch ( domctl->cmd )
>      {
> -
>      case XEN_DOMCTL_shadow_op:
>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
> +        /*
> +         * Continuations from paging_domctl() switch index to arch_1, and
> +         * can't use the common domctl continuation path.
> +         */
>          if ( ret == -ERESTART )
>              return hypercall_create_continuation(__HYPERVISOR_arch_1,
>                                                   "h", u_domctl);

There's also XEN_DOMCTL_getpageframeinfo3 down from here which
now invokes a continuation.

> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>          ret = -EFAULT;
>  
> +    if ( ret == -ERESTART )
> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
> +                                            "h", u_domctl);

You may want to mention in the description the bug you fix here:
Previously the -EFAULT returning visible in context should have
canceled any active continuation.

Jan

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Jan Beulich]

On 05.12.2019 23:30, Andrew Cooper wrote:
> These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
> folding existing contination logic where applicable.
> 
> In addition:
>  * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
>    tight spinlock loop, and make the continuation logic common at the
>    epilogue.

Is this really needed with a hypercall_preempt_check() invocation
already in the bodies of these loops?

Jan

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Jan Beulich]

On 09.12.2019 17:25, Jan Beulich wrote:
> On 05.12.2019 23:30, Andrew Cooper wrote:
>> These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>> folding existing contination logic where applicable.
>>
>> In addition:
>>  * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
>>    tight spinlock loop, and make the continuation logic common at the
>>    epilogue.
> 
> Is this really needed with a hypercall_preempt_check() invocation
> already in the bodies of these loops?

And if it's really to be added, shouldn't it be at the bottom
of the loop bodies rather than at the top?

Jan

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

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

[Jan Beulich]

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

"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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Andrew Cooper]

On 08/12/2019 12:18, Julien Grall wrote:
> Hi,
>
> On 05/12/2019 22:30, Andrew Cooper wrote:
>> More paths are going start using hypercall continuations.  We could
>> add extra
>> calls to hypercall_create_continuation() but it is much easier to handle
>> -ERESTART once at the top level.
>>
>> One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
>> compatibility in a security fix, turn a DOMCTL continuation into
>> __HYPERVISOR_arch_1.  This remains as it was, gaining a comment
>> explaining
>> what is going on.
>>
>> With -ERESTART handling in place, the !domctl_lock_acquire() path can
>> use the
>> normal exit path, instead of opencoding a subset of it locally.
>>
>> 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>
>> ---
>>   xen/arch/x86/domctl.c           |  5 ++++-
>>   xen/arch/x86/mm/hap/hap.c       |  3 +--
>>   xen/arch/x86/mm/shadow/common.c |  3 +--
>>   xen/common/domctl.c             | 19 +++++--------------
>
> You seem to have missed the hypercall_create_continuation() call in
> iommu_do_pci_domctl().

So I have.  Fixed.

~Andrew

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

Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Andrew Cooper]

On 09/12/2019 16:19, Jan Beulich wrote:
> On 05.12.2019 23:30, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -326,9 +326,12 @@ long arch_do_domctl(
>>  
>>      switch ( domctl->cmd )
>>      {
>> -
>>      case XEN_DOMCTL_shadow_op:
>>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
>> +        /*
>> +         * Continuations from paging_domctl() switch index to arch_1, and
>> +         * can't use the common domctl continuation path.
>> +         */
>>          if ( ret == -ERESTART )
>>              return hypercall_create_continuation(__HYPERVISOR_arch_1,
>>                                                   "h", u_domctl);
> There's also XEN_DOMCTL_getpageframeinfo3 down from here which
> now invokes a continuation.

Fixed.

>
>> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>>          ret = -EFAULT;
>>  
>> +    if ( ret == -ERESTART )
>> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
>> +                                            "h", u_domctl);
> You may want to mention in the description the bug you fix here:
> Previously the -EFAULT returning visible in context should have
> canceled any active continuation.

That would be presuming I'd even spotted it...

Having looked though the paths once again, I don't think there was a
path which continued and had copyback set, so this is at most a latent bug.

~Andrew

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Andrew Cooper]

On 08/12/2019 12:57, Julien Grall wrote:
> Hi Andrew,
>
> On 05/12/2019 22:30, Andrew Cooper wrote:
>> These hypercalls each use continue_hypercall_on_cpu(), whose API is
>> about to
>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>> folding existing contination logic where applicable.
>>
>> In addition:
>>   * For platform op and sysctl, insert a cpu_relax() into what is
>> otherwise a
>>     tight spinlock loop, and make the continuation logic common at the
>>     epilogue.
>>   * Contrary to the comment in the code, kexec_exec() does return in the
>>     KEXEC_REBOOT case, needs to pass ret back to the caller.
>
> It is not entirely trivial to me that KEXEC_REBOOT refers to
> KEXEC_DEFAULT_TYPE. The more that if you look at the kexec_reboot()
> helper, it will not return (see BUG()). What may return is
> continue_hypercall_on_cpu().
>
> So would it make sense to use KEXEC_DEFAULT_TYPE?

I'm not sure why I capitalised it, but no - using KEXEC_DEFAULT_TYPE is
worse.  A casual reader is far more likely to understand kexec_reboot()
in this context.

>
> [...]
>
>> @@ -816,6 +819,13 @@ ret_t
>> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>>    out:
>>       spin_unlock(&xenpf_lock);
>>   +    if ( ret == -ERESTART )
>> +    {
>> +    create_continuation:
>
> Shall we indent create_continuation the same way as out?

They have different scopes, and while it may look weird, this is in
accordance with our style.

>
> [...]
>
>> @@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long
>> op,
>>     long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
>> uarg)
>>   {
>> -    return do_kexec_op_internal(op, uarg, 0);
>> +    int ret = do_kexec_op_internal(op, uarg, 0);
> Shouldn't it be long (or unsigned long) here? Otherwise, the return of
> hypercall_create_continuation() may be truncated.

If you're concerned about truncation via this pattern, then there are
other areas of the code to be worred about.

However, there is nothing to truncate.  The return value of
hypercall_create_continuation() is the primary hypercall number, i.e.
__HYPERVISOR_kexec_op in this case.

~Andrew

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Andrew Cooper]

On 09/12/2019 16:29, Jan Beulich wrote:
> On 09.12.2019 17:25, Jan Beulich wrote:
>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>> These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
>>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>>> folding existing contination logic where applicable.
>>>
>>> In addition:
>>>  * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
>>>    tight spinlock loop, and make the continuation logic common at the
>>>    epilogue.
>> Is this really needed with a hypercall_preempt_check() invocation
>> already in the bodies of these loops?

Yes.  The reason you're supposed to pause is to stop having memory
traffic constantly trying to pull the spinlock's cacheline into shared
state.

Racing round a tight loop constantly reading 4 or 5 memory locations is
almost as bad.

> And if it's really to be added, shouldn't it be at the bottom
> of the loop bodies rather than at the top?

It doesn't matter.

~Andrew

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

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

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level

[Jan Beulich]

On 09.12.2019 18:29, Andrew Cooper wrote:
> On 09/12/2019 16:19, Jan Beulich wrote:
>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>>>          ret = -EFAULT;
>>>  
>>> +    if ( ret == -ERESTART )
>>> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
>>> +                                            "h", u_domctl);
>> You may want to mention in the description the bug you fix here:
>> Previously the -EFAULT returning visible in context should have
>> canceled any active continuation.
> 
> That would be presuming I'd even spotted it...
> 
> Having looked though the paths once again, I don't think there was a
> path which continued and had copyback set, so this is at most a latent
> bug.

Ah, good. I actually first meant to check, but then forgot before
sending the earlier reply.

Jan

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Jan Beulich]

On 09.12.2019 18:43, Andrew Cooper wrote:
> On 09/12/2019 16:29, Jan Beulich wrote:
>> On 09.12.2019 17:25, Jan Beulich wrote:
>>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>>> These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
>>>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>>>> folding existing contination logic where applicable.
>>>>
>>>> In addition:
>>>>  * For platform op and sysctl, insert a cpu_relax() into what is otherwise a
>>>>    tight spinlock loop, and make the continuation logic common at the
>>>>    epilogue.
>>> Is this really needed with a hypercall_preempt_check() invocation
>>> already in the bodies of these loops?
> 
> Yes.  The reason you're supposed to pause is to stop having memory
> traffic constantly trying to pull the spinlock's cacheline into shared
> state.
> 
> Racing round a tight loop constantly reading 4 or 5 memory locations is
> almost as bad.
> 
>> And if it's really to be added, shouldn't it be at the bottom
>> of the loop bodies rather than at the top?
> 
> It doesn't matter.

Well, modern documentation indeed gives no hint whatsoever towards
its placement. Recalling the initial guidelines from Intel (from
even before the whitepaper was made available) there was a
suggestion towards placing it close ahead of the problematic
memory access. The last example in the whitepaper looks to support
this without explicitly saying so.

Anyway, preferably with it moved
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

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

[Jan Beulich]

On 09.12.2019 18:49, Andrew Cooper wrote:
> 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).

Well, in-memory arguments can be accessed as long as the mapping is
the right one (which it typically wouldn't be inside a tasklet). Do
existing continue_hypercall_on_cpu() users need this? Looking over
patch 4 again, I didn't think so. (Which isn't to say that removing
the latent issue is not a good thing.)

In-register values can be changed as long as the respective exit
path will suitably pick up the value, which I thought was always
the case.

Hence I'm afraid your single reply sentence didn't really clarify
matters. I'm sorry if this is just because of me being dense.

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

Accepted, but it's still not like "completely broken". I would even
suppose the case wasn't considered when CPU offlining support was
introduced, not when continue_hypercall_on_cpu() came into existence
(which presumably is when the comment was written).

Anyway - yes, I agree this is a fair solution to the issue at hand.

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

Oh, I see I was mislead by continue_hypercall_tasklet_handler() not
updating info->cpu, not paying attention to it actually freeing info.
Plus a crucial aspect looks to be that there are no "chained" uses of
continue_hypercall_on_cpu() anymore (the microcode loading one being
gone now) - afaict any such wouldn't guarantee forward progress with
this new model (without recording somewhere which CPUs had been dealt
with already).

Jan

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

Re: [Xen-devel] [PATCH 6/6] x86/smt: Don't use -EBUSY for smt_up_down_helper() continuations

[Jan Beulich]

On 05.12.2019 23:30, Andrew Cooper wrote:
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -85,6 +85,9 @@ long cpu_up_helper(void *data)
>          /* On EBUSY, flush RCU work and have one more go. */
>          rcu_barrier();
>          ret = cpu_up(cpu);
> +
> +        if ( ret == -EBUSY )
> +            ret = -ERESTART;
>      }
>  
>      if ( !ret && !opt_smt &&
> @@ -110,6 +113,9 @@ long cpu_down_helper(void *data)
>          /* On EBUSY, flush RCU work and have one more go. */
>          rcu_barrier();
>          ret = cpu_down(cpu);
> +
> +        if ( ret == -EBUSY )
> +            ret = -ERESTART;
>      }
>      return ret;
>  }

For both of these - if two successive attempts didn't work, is
there really much point not reporting the fact back to the
caller? You're liable to request continuations indefinitely then.

> @@ -143,8 +149,7 @@ static long smt_up_down_helper(void *data)
>           */
>          if ( ret != -EEXIST && general_preempt_check() )
>          {
> -            /* In tasklet context - can't create a contination. */
> -            ret = -EBUSY;
> +            ret = -ERESTART;
>              break;
>          }
>  

I agree with this change.

Jan

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

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

[Andrew Cooper]

On 10/12/2019 08:55, Jan Beulich wrote:
> On 09.12.2019 18:49, Andrew Cooper wrote:
>> 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).
> Well, in-memory arguments can be accessed as long as the mapping is
> the right one (which it typically wouldn't be inside a tasklet). Do
> existing continue_hypercall_on_cpu() users need this? Looking over
> patch 4 again, I didn't think so. (Which isn't to say that removing
> the latent issue is not a good thing.)
>
> In-register values can be changed as long as the respective exit
> path will suitably pick up the value, which I thought was always
> the case.
>
> Hence I'm afraid your single reply sentence didn't really clarify
> matters. I'm sorry if this is just because of me being dense.

How, physically, would you arrange for continue_hypercall_on_cpu() to
make the requisite state adjustments?

Yes - registers and memory can be accessed, but only the hypercall
(sub?)op handler knows how to mutate them appropriately.

You'd have to copy the mutation logic into continue_hypercall_on_cpu(),
and pass in op/subops and a union of all pointers, *and* whatever
intermediate state the subop handler needs.

Or you can return -ERESTART and let the caller DTRT with the state it
has in context, as it would in other cases requiring a continuation.

>
>>>> 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.
> Accepted, but it's still not like "completely broken".

I didn't mean it like that.  I mean "it has never had the property it
claimed", which is distinct from "the claim used to be true, but was
then accidentally regressed".

> I would even
> suppose the case wasn't considered when CPU offlining support was
> introduced, not when continue_hypercall_on_cpu() came into existence
> (which presumably is when the comment was written).
>
> Anyway - yes, I agree this is a fair solution to the issue at hand.
>
>>>> 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.
> Oh, I see I was mislead by continue_hypercall_tasklet_handler() not
> updating info->cpu, not paying attention to it actually freeing info.
> Plus a crucial aspect looks to be that there are no "chained" uses of
> continue_hypercall_on_cpu() anymore (the microcode loading one being
> gone now) - afaict any such wouldn't guarantee forward progress with
> this new model (without recording somewhere which CPUs had been dealt
> with already).

I'd forgotten that we had that, but I can't say I'm sad to see the back
of it.  I recall at the time saying that it wasn't a clever move.

For now, I suggest that we ignore that case.  If an when a real usecase
appears, we can consider making adjustments.

~Andrew

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

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

[Jan Beulich]

On 10.12.2019 18:55, Andrew Cooper wrote:
> On 10/12/2019 08:55, Jan Beulich wrote:
>> On 09.12.2019 18:49, Andrew Cooper wrote:
>>> 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).
>> Well, in-memory arguments can be accessed as long as the mapping is
>> the right one (which it typically wouldn't be inside a tasklet). Do
>> existing continue_hypercall_on_cpu() users need this? Looking over
>> patch 4 again, I didn't think so. (Which isn't to say that removing
>> the latent issue is not a good thing.)
>>
>> In-register values can be changed as long as the respective exit
>> path will suitably pick up the value, which I thought was always
>> the case.
>>
>> Hence I'm afraid your single reply sentence didn't really clarify
>> matters. I'm sorry if this is just because of me being dense.
> 
> How, physically, would you arrange for continue_hypercall_on_cpu() to
> make the requisite state adjustments?

You can't (at least not without having sufficient further context),
I agree. Yet ...

> Yes - registers and memory can be accessed, but only the hypercall
> (sub?)op handler knows how to mutate them appropriately.
> 
> You'd have to copy the mutation logic into continue_hypercall_on_cpu(),
> and pass in op/subops and a union of all pointers, *and* whatever
> intermediate state the subop handler needs.
> 
> Or you can return -ERESTART and let the caller DTRT with the state it
> has in context, as it would in other cases requiring a continuation.

... it continues to be unclear to me whether you're fixing an actual
bug here, or just a latent one. The existing uses of
continue_hypercall_on_cpu() don't look to require state updates
beyond the hypercall return value (or else, aiui, they wouldn't have
worked in the first place), and that one had a way to get modified.

>>>>> 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.
>> Oh, I see I was mislead by continue_hypercall_tasklet_handler() not
>> updating info->cpu, not paying attention to it actually freeing info.
>> Plus a crucial aspect looks to be that there are no "chained" uses of
>> continue_hypercall_on_cpu() anymore (the microcode loading one being
>> gone now) - afaict any such wouldn't guarantee forward progress with
>> this new model (without recording somewhere which CPUs had been dealt
>> with already).
> 
> I'd forgotten that we had that, but I can't say I'm sad to see the back
> of it.  I recall at the time saying that it wasn't a clever move.
> 
> For now, I suggest that we ignore that case.  If an when a real usecase
> appears, we can consider making adjustments.

Oh, of course - I didn't mean to even remotely suggest anything else.

Jan

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

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

[Andrew Cooper]

On 11/12/2019 07:41, Jan Beulich wrote:
> On 10.12.2019 18:55, Andrew Cooper wrote:
>> On 10/12/2019 08:55, Jan Beulich wrote:
>>> On 09.12.2019 18:49, Andrew Cooper wrote:
>>>> 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).
>>> Well, in-memory arguments can be accessed as long as the mapping is
>>> the right one (which it typically wouldn't be inside a tasklet). Do
>>> existing continue_hypercall_on_cpu() users need this? Looking over
>>> patch 4 again, I didn't think so. (Which isn't to say that removing
>>> the latent issue is not a good thing.)
>>>
>>> In-register values can be changed as long as the respective exit
>>> path will suitably pick up the value, which I thought was always
>>> the case.
>>>
>>> Hence I'm afraid your single reply sentence didn't really clarify
>>> matters. I'm sorry if this is just because of me being dense.
>> How, physically, would you arrange for continue_hypercall_on_cpu() to
>> make the requisite state adjustments?
> You can't (at least not without having sufficient further context),
> I agree. Yet ...
>
>> Yes - registers and memory can be accessed, but only the hypercall
>> (sub?)op handler knows how to mutate them appropriately.
>>
>> You'd have to copy the mutation logic into continue_hypercall_on_cpu(),
>> and pass in op/subops and a union of all pointers, *and* whatever
>> intermediate state the subop handler needs.
>>
>> Or you can return -ERESTART and let the caller DTRT with the state it
>> has in context, as it would in other cases requiring a continuation.
> ... it continues to be unclear to me whether you're fixing an actual
> bug here, or just a latent one.

I'm not fixing any bug.

I am making a fundamental change in behaviour, so tasklet context can
use -ERESTART.

Tasklet context doesn't even know what the primary hypercall index needs
to be to correctly create a continuation.

~Andrew

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

Re: [Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths

[Julien Grall]

Hi,

On 09/12/2019 17:37, Andrew Cooper wrote:
> On 08/12/2019 12:57, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 05/12/2019 22:30, Andrew Cooper wrote:
>>> These hypercalls each use continue_hypercall_on_cpu(), whose API is
>>> about to
>>> switch to use -ERESTART.  Update the soon-to-be affected paths to cope,
>>> folding existing contination logic where applicable.
>>>
>>> In addition:
>>>    * For platform op and sysctl, insert a cpu_relax() into what is
>>> otherwise a
>>>      tight spinlock loop, and make the continuation logic common at the
>>>      epilogue.
>>>    * Contrary to the comment in the code, kexec_exec() does return in the
>>>      KEXEC_REBOOT case, needs to pass ret back to the caller.
>>
>> It is not entirely trivial to me that KEXEC_REBOOT refers to
>> KEXEC_DEFAULT_TYPE. The more that if you look at the kexec_reboot()
>> helper, it will not return (see BUG()). What may return is
>> continue_hypercall_on_cpu().
>>
>> So would it make sense to use KEXEC_DEFAULT_TYPE?
> 
> I'm not sure why I capitalised it, but no - using KEXEC_DEFAULT_TYPE is
> worse.  A casual reader is far more likely to understand kexec_reboot()
> in this context.

But kexec_reboot() cannot return (see BUG()) so a reader may not 
understand why you suggest that it will return. So I think this want to 
be reworded.
>>
>> [...]
>>
>>> @@ -816,6 +819,13 @@ ret_t
>>> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>>>     out:
>>>        spin_unlock(&xenpf_lock);
>>>    +    if ( ret == -ERESTART )
>>> +    {
>>> +    create_continuation:
>>
>> Shall we indent create_continuation the same way as out?
> 
> They have different scopes, and while it may look weird, this is in
> accordance with our style.
> 
>>
>> [...]
>>
>>> @@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long
>>> op,
>>>      long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
>>> uarg)
>>>    {
>>> -    return do_kexec_op_internal(op, uarg, 0);
>>> +    int ret = do_kexec_op_internal(op, uarg, 0);
>> Shouldn't it be long (or unsigned long) here? Otherwise, the return of
>> hypercall_create_continuation() may be truncated.
> 
> If you're concerned about truncation via this pattern, then there are
> other areas of the code to be worred about.

I knew you would mention the other places :).

> 
> However, there is nothing to truncate.  The return value of
> hypercall_create_continuation() is the primary hypercall number, i.e.
> __HYPERVISOR_kexec_op in this case.

Make sense. And I guess the signed bit will be propagated so if 
do_kexec_op() return -EINVAL (32-bit), then it would still be -EINVAL 
(64-bit).

Cheers,

-- 
Julien Grall

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