[PATCH 0/5] CPU: Detect put cpu register errors for migrations

[PATCH 0/5] CPU: Detect put cpu register errors for migrations

[Peter Xu]

rfc->v1:
- Rebase to master, drop RFC tag.

This series teaches QEMU to detect errors when e.g. putting registers from
QEMU to KVM, and fail migrations properly.

For the rational of this series and why it was posted, please refer to the
bug report here:

https://lore.kernel.org/all/YppVupW+IWsm7Osr@xz-m1.local/

But I'd rather not go into that if the reviewer doesn't have that context,
because we don't really need that complexity..  It can be simple as we
should fail migration early when we see issues happening already, so:

  1) We fail explicitly, rather than afterward with some weird guest
     errors.  In my bug report, it was a guest double fault.  There's
     another bug report that Sean mentioned in the thread from Mike Tancsa
     that can have other sympotons rather than double fault, but anyway
     they'll be hard to diagnose since the processor state can be corrupted
     (please refer to kvm_arch_put_registers() where we stop putting more
     registers to KVM when we see any error).

  2) For precopy, with this early failure the VM won't crash itself since
     we still have a chance to keep running it on src host, while if
     without this patch we will fail later, and it can crash the VM.

In this specific case, when KVM_SET_XSAVE ioctl failed on dest host before
start running the VM there, we should fail the migration already.

After the patchset applied, the above "double fault" issue will become
migration failures, and...

For precopy, we can see some error dumped for precopy on dest, then the VM
will be kept running on src host:

2022-06-07T22:48:48.804234Z qemu-system-x86_64: kvm_arch_put_registers() failed with retval=-22
2022-06-07T22:48:48.804588Z qemu-system-x86_64: load of migration failed: Invalid argument

For postcopy, currently we'll pause the VM immediately for admin to decide
what to do:

2022-06-07T22:47:49.448192Z qemu-system-x86_64: kvm_arch_put_registers() failed with retval=-22
13072@1654642069.518993:runstate_set current_run_state 1 (inmigrate) new_state 4 (paused)

If something like this series is welcomed, we could do better in the future
by telling the src host about this issue and keep running, because
put-register happens right at the switch-over, so we actually have this
chance (no dirty page on dest host yet).

Comments welcomed.  Thanks,

Peter Xu (5):
  cpus-common: Introduce run_on_cpu_func2 which allows error returns
  cpus-common: Add run_on_cpu2()
  accel: Allow synchronize_post_init() to take an Error**
  cpu: Allow cpu_synchronize_all_post_init() to take an errp
  KVM: Hook kvm_arch_put_registers() errors to the caller

 accel/hvf/hvf-accel-ops.c     |  2 +-
 accel/kvm/kvm-all.c           | 15 +++++++---
 accel/kvm/kvm-cpus.h          |  2 +-
 cpus-common.c                 | 55 +++++++++++++++++++++++++++++++++--
 hw/core/machine.c             |  2 +-
 include/hw/core/cpu.h         | 28 ++++++++++++++++++
 include/sysemu/accel-ops.h    |  2 +-
 include/sysemu/cpus.h         |  2 +-
 include/sysemu/hw_accel.h     |  1 +
 migration/savevm.c            | 20 +++++++++++--
 softmmu/cpus.c                | 23 ++++++++++++---
 stubs/cpu-synchronize-state.c |  3 ++
 target/i386/hax/hax-all.c     |  2 +-
 target/i386/nvmm/nvmm-all.c   |  2 +-
 target/i386/whpx/whpx-all.c   |  2 +-
 15 files changed, 139 insertions(+), 22 deletions(-)

-- 
2.32.0

[PATCH 1/5] cpus-common: Introduce run_on_cpu_func2 which allows error returns

[Peter Xu]

run_on_cpu API does not yet support any way to pass over an error message
to above.  Add a new run_on_cpu_func2 hook to grant possibility of that.

Note that this only changes the cpus-common core, no API is yet introduced
for v2 of the run_on_cpu_func function.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus-common.c         | 28 +++++++++++++++++++++++++---
 include/hw/core/cpu.h |  2 ++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index db459b41ce..1db7bbbb88 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -116,9 +116,20 @@ __thread CPUState *current_cpu;
 
 struct qemu_work_item {
     QSIMPLEQ_ENTRY(qemu_work_item) node;
-    run_on_cpu_func func;
+    union {
+        run_on_cpu_func func;     /* When has_errp==false */
+        run_on_cpu_func2 func2;   /* When has_errp==true  */
+    };
     run_on_cpu_data data;
     bool free, exclusive, done;
+
+    /*
+     * Below are only used by v2 of work item, where we allow to return
+     * errors for cpu work items.  When has_errp==true, then: (1) we call
+     * func2 rather than func, and (2) we pass in errp into func2() call.
+     */
+    bool has_errp;
+    Error **errp;
 };
 
 static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
@@ -314,6 +325,17 @@ void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func func,
     queue_work_on_cpu(cpu, wi);
 }
 
+static void process_one_work_item(struct qemu_work_item *wi, CPUState *cpu)
+{
+    if (wi->has_errp) {
+        /* V2 of work item, allows errors */
+        wi->func2(cpu, wi->data, wi->errp);
+    } else {
+        /* Old version of work item, no error returned */
+        wi->func(cpu, wi->data);
+    }
+}
+
 void process_queued_cpu_work(CPUState *cpu)
 {
     struct qemu_work_item *wi;
@@ -336,11 +358,11 @@ void process_queued_cpu_work(CPUState *cpu)
              */
             qemu_mutex_unlock_iothread();
             start_exclusive();
-            wi->func(cpu, wi->data);
+            process_one_work_item(wi, cpu);
             end_exclusive();
             qemu_mutex_lock_iothread();
         } else {
-            wi->func(cpu, wi->data);
+            process_one_work_item(wi, cpu);
         }
         qemu_mutex_lock(&cpu->work_mutex);
         if (wi->free) {
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 996f94059f..7a303576d0 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -252,6 +252,8 @@ typedef union {
 #define RUN_ON_CPU_NULL           RUN_ON_CPU_HOST_PTR(NULL)
 
 typedef void (*run_on_cpu_func)(CPUState *cpu, run_on_cpu_data data);
+/* Same as run_on_cpu_func but allows to return an error */
+typedef void (*run_on_cpu_func2)(CPUState *cpu, run_on_cpu_data data, Error **errp);
 
 struct qemu_work_item;
 
-- 
2.32.0

[PATCH 2/5] cpus-common: Add run_on_cpu2()

[Peter Xu]

This version of run_on_cpu() allows to take an Error** to detect errors.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus-common.c         | 27 +++++++++++++++++++++++++++
 include/hw/core/cpu.h | 26 ++++++++++++++++++++++++++
 softmmu/cpus.c        |  6 ++++++
 3 files changed, 59 insertions(+)

diff --git a/cpus-common.c b/cpus-common.c
index 1db7bbbb88..1d67c0c655 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -167,6 +167,33 @@ void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
     }
 }
 
+void do_run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
+                    QemuMutex *mutex, Error **errp)
+{
+    struct qemu_work_item wi;
+
+    if (qemu_cpu_is_self(cpu)) {
+        func2(cpu, data, errp);
+        return;
+    }
+
+    wi.func2 = func2;
+    wi.data = data;
+    wi.done = false;
+    wi.free = false;
+    wi.exclusive = false;
+    wi.has_errp = true;
+    wi.errp = errp;
+
+    queue_work_on_cpu(cpu, &wi);
+    while (!qatomic_mb_read(&wi.done)) {
+        CPUState *self_cpu = current_cpu;
+
+        qemu_cond_wait(&qemu_work_cond, mutex);
+        current_cpu = self_cpu;
+    }
+}
+
 void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
     struct qemu_work_item *wi;
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 7a303576d0..4bb40a03cf 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -709,6 +709,19 @@ bool cpu_is_stopped(CPUState *cpu);
 void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
                    QemuMutex *mutex);
 
+/**
+ * do_run_on_cpu2:
+ * @cpu: The vCPU to run on.
+ * @func2: The function to be executed.
+ * @data: Data to pass to the function.
+ * @mutex: Mutex to release while waiting for @func2 to run.
+ * @errp: The Error** pointer to be passed into @func2.
+ *
+ * Used internally in the implementation of run_on_cpu2.
+ */
+void do_run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
+                    QemuMutex *mutex, Error **errp);
+
 /**
  * run_on_cpu:
  * @cpu: The vCPU to run on.
@@ -719,6 +732,19 @@ void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
  */
 void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data);
 
+/**
+ * run_on_cpu2:
+ * @cpu: The vCPU to run on.
+ * @func: The function to be executed.
+ * @data: Data to pass to the function.
+ * @errp: The Error** pointer to be passed into @func2.
+ *
+ * Schedules the function @func2 for execution on the vCPU @cpu, capture
+ * any error and put it into *@errp when provided.
+ */
+void run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
+                 Error **errp);
+
 /**
  * async_run_on_cpu:
  * @cpu: The vCPU to run on.
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 23b30484b2..898363a1d0 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -391,6 +391,12 @@ void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
     do_run_on_cpu(cpu, func, data, &qemu_global_mutex);
 }
 
+void run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
+                 Error **errp)
+{
+    do_run_on_cpu2(cpu, func2, data, &qemu_global_mutex, errp);
+}
+
 static void qemu_cpu_stop(CPUState *cpu, bool exit)
 {
     g_assert(qemu_cpu_is_self(cpu));
-- 
2.32.0

[PATCH 3/5] accel: Allow synchronize_post_init() to take an Error**

[Peter Xu]

It allows accel->synchronize_post_init() hook to return an error upwards.
Add a new cpu_synchronize_post_init_full() for it, then let the existing
cpu_synchronize_post_init() to call it with errp==NULL.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/hvf/hvf-accel-ops.c     |  2 +-
 accel/kvm/kvm-all.c           |  2 +-
 include/sysemu/accel-ops.h    |  2 +-
 include/sysemu/hw_accel.h     |  1 +
 softmmu/cpus.c                | 10 ++++++++--
 stubs/cpu-synchronize-state.c |  3 +++
 target/i386/hax/hax-all.c     |  2 +-
 target/i386/nvmm/nvmm-all.c   |  2 +-
 target/i386/whpx/whpx-all.c   |  2 +-
 9 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index 24913ca9c4..dec4446264 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -228,7 +228,7 @@ static void hvf_cpu_synchronize_post_reset(CPUState *cpu)
     run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
 }
 
-static void hvf_cpu_synchronize_post_init(CPUState *cpu)
+static void hvf_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
     run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
 }
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ba3210b1c1..df4f7c98f3 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2777,7 +2777,7 @@ static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
     cpu->vcpu_dirty = false;
 }
 
-void kvm_cpu_synchronize_post_init(CPUState *cpu)
+void kvm_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
     run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index a0572ea87a..7e526d3c65 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -35,7 +35,7 @@ struct AccelOpsClass {
     bool (*cpu_thread_is_idle)(CPUState *cpu);
 
     void (*synchronize_post_reset)(CPUState *cpu);
-    void (*synchronize_post_init)(CPUState *cpu);
+    void (*synchronize_post_init)(CPUState *cpu, Error **errp);
     void (*synchronize_state)(CPUState *cpu);
     void (*synchronize_pre_loadvm)(CPUState *cpu);
     void (*synchronize_pre_resume)(bool step_pending);
diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
index 22903a55f7..3ee3508411 100644
--- a/include/sysemu/hw_accel.h
+++ b/include/sysemu/hw_accel.h
@@ -21,6 +21,7 @@
 void cpu_synchronize_state(CPUState *cpu);
 void cpu_synchronize_post_reset(CPUState *cpu);
 void cpu_synchronize_post_init(CPUState *cpu);
+void cpu_synchronize_post_init_full(CPUState *cpu, Error **errp);
 void cpu_synchronize_pre_loadvm(CPUState *cpu);
 
 #endif /* QEMU_HW_ACCEL_H */
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 898363a1d0..464c06201c 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -178,13 +178,19 @@ void cpu_synchronize_post_reset(CPUState *cpu)
     }
 }
 
-void cpu_synchronize_post_init(CPUState *cpu)
+void cpu_synchronize_post_init_full(CPUState *cpu, Error **errp)
 {
     if (cpus_accel->synchronize_post_init) {
-        cpus_accel->synchronize_post_init(cpu);
+        cpus_accel->synchronize_post_init(cpu, errp);
     }
 }
 
+void cpu_synchronize_post_init(CPUState *cpu)
+{
+    /* errp=NULL means we won't capture any error */
+    cpu_synchronize_post_init_full(cpu, NULL);
+}
+
 void cpu_synchronize_pre_loadvm(CPUState *cpu)
 {
     if (cpus_accel->synchronize_pre_loadvm) {
diff --git a/stubs/cpu-synchronize-state.c b/stubs/cpu-synchronize-state.c
index d9211da66c..6d2c9f509a 100644
--- a/stubs/cpu-synchronize-state.c
+++ b/stubs/cpu-synchronize-state.c
@@ -7,3 +7,6 @@ void cpu_synchronize_state(CPUState *cpu)
 void cpu_synchronize_post_init(CPUState *cpu)
 {
 }
+void cpu_synchronize_post_init_full(CPUState *cpu, Error **errp)
+{
+}
diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
index b185ee8de4..782d83b531 100644
--- a/target/i386/hax/hax-all.c
+++ b/target/i386/hax/hax-all.c
@@ -651,7 +651,7 @@ static void do_hax_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
     cpu->vcpu_dirty = false;
 }
 
-void hax_cpu_synchronize_post_init(CPUState *cpu)
+void hax_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
     run_on_cpu(cpu, do_hax_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index b75738ee9c..f429e940af 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -869,7 +869,7 @@ void nvmm_cpu_synchronize_post_reset(CPUState *cpu)
     run_on_cpu(cpu, do_nvmm_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
 }
 
-void nvmm_cpu_synchronize_post_init(CPUState *cpu)
+void nvmm_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
     run_on_cpu(cpu, do_nvmm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index b22a3314b4..09bf5681ce 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -2123,7 +2123,7 @@ void whpx_cpu_synchronize_post_reset(CPUState *cpu)
     run_on_cpu(cpu, do_whpx_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
 }
 
-void whpx_cpu_synchronize_post_init(CPUState *cpu)
+void whpx_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
     run_on_cpu(cpu, do_whpx_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
-- 
2.32.0

[PATCH 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp

[Peter Xu]

Allow cpu_synchronize_all_post_init() to fail with an errp when it's set.
Modify both precopy and postcopy to try to detect such error.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c     |  2 +-
 include/sysemu/cpus.h |  2 +-
 migration/savevm.c    | 20 +++++++++++++++++---
 softmmu/cpus.c        |  2 +-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a673302cce..e1a072080a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1419,7 +1419,7 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify)
 
 void qdev_machine_creation_done(void)
 {
-    cpu_synchronize_all_post_init();
+    cpu_synchronize_all_post_init(NULL);
 
     if (current_machine->boot_config.has_once) {
         qemu_boot_set(current_machine->boot_config.once, &error_fatal);
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index b5c87d48b3..a51ee46441 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -45,7 +45,7 @@ bool cpus_are_resettable(void);
 
 void cpu_synchronize_all_states(void);
 void cpu_synchronize_all_post_reset(void);
-void cpu_synchronize_all_post_init(void);
+void cpu_synchronize_all_post_init(Error **errp);
 void cpu_synchronize_all_pre_loadvm(void);
 
 #ifndef CONFIG_USER_ONLY
diff --git a/migration/savevm.c b/migration/savevm.c
index d9076897b8..1175ddefd4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
     /* TODO we should move all of this lot into postcopy_ram.c or a shared code
      * in migration.c
      */
-    cpu_synchronize_all_post_init();
+    cpu_synchronize_all_post_init(&local_err);
+    if (local_err) {
+        /*
+         * TODO: a better way to do this is to tell the src that we cannot
+         * run the VM here so hopefully we can keep the VM running on src
+         * and immediately halt the switch-over.  But that needs work.
+         */
+        error_report_err(local_err);
+        local_err = NULL;
+        autostart = false;
+    }
 
     trace_loadvm_postcopy_handle_run_bh("after cpu sync");
 
@@ -2772,7 +2782,11 @@ int qemu_loadvm_state(QEMUFile *f)
     }
 
     qemu_loadvm_state_cleanup();
-    cpu_synchronize_all_post_init();
+    cpu_synchronize_all_post_init(&local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
 
     return ret;
 }
@@ -2789,7 +2803,7 @@ int qemu_load_device_state(QEMUFile *f)
         return ret;
     }
 
-    cpu_synchronize_all_post_init();
+    cpu_synchronize_all_post_init(NULL);
     return 0;
 }
 
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 464c06201c..59c70fd496 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -146,7 +146,7 @@ void cpu_synchronize_all_post_reset(void)
     }
 }
 
-void cpu_synchronize_all_post_init(void)
+void cpu_synchronize_all_post_init(Error **errp)
 {
     CPUState *cpu;
 
-- 
2.32.0

[PATCH 5/5] KVM: Hook kvm_arch_put_registers() errors to the caller

[Peter Xu]

Leverage the new mechanism to pass over errors to upper stack for
kvm_arch_put_registers() when called for the post_init() accel hook.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/kvm/kvm-all.c  | 13 ++++++++++---
 accel/kvm/kvm-cpus.h |  2 +-
 softmmu/cpus.c       |  5 ++++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index df4f7c98f3..03e29ab1ed 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2771,15 +2771,22 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
     run_on_cpu(cpu, do_kvm_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
 }
 
-static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
+static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg,
+                                             Error **errp)
 {
-    kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
+    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
+
+    if (ret) {
+        error_setg(errp, "kvm_arch_put_registers() failed with retval=%d", ret);
+        return;
+    }
+
     cpu->vcpu_dirty = false;
 }
 
 void kvm_cpu_synchronize_post_init(CPUState *cpu, Error **errp)
 {
-    run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
+    run_on_cpu2(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL, errp);
 }
 
 static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index bf0bd1bee4..c9b8262704 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -16,7 +16,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp);
 int kvm_cpu_exec(CPUState *cpu);
 void kvm_destroy_vcpu(CPUState *cpu);
 void kvm_cpu_synchronize_post_reset(CPUState *cpu);
-void kvm_cpu_synchronize_post_init(CPUState *cpu);
+void kvm_cpu_synchronize_post_init(CPUState *cpu, Error **errp);
 void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
 
 #endif /* KVM_CPUS_H */
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 59c70fd496..6c0b5b87f0 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -151,7 +151,10 @@ void cpu_synchronize_all_post_init(Error **errp)
     CPUState *cpu;
 
     CPU_FOREACH(cpu) {
-        cpu_synchronize_post_init(cpu);
+        cpu_synchronize_post_init_full(cpu, errp);
+        if (errp && *errp) {
+            break;
+        }
     }
 }
 
-- 
2.32.0

Re: [PATCH 2/5] cpus-common: Add run_on_cpu2()

[Peter Maydell]

On Fri, 17 Jun 2022 at 15:57, Peter Xu <peterx@redhat.com> wrote:
>
> This version of run_on_cpu() allows to take an Error** to detect errors.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  cpus-common.c         | 27 +++++++++++++++++++++++++++
>  include/hw/core/cpu.h | 26 ++++++++++++++++++++++++++
>  softmmu/cpus.c        |  6 ++++++
>  3 files changed, 59 insertions(+)

> +/**
> + * run_on_cpu2:
> + * @cpu: The vCPU to run on.
> + * @func: The function to be executed.
> + * @data: Data to pass to the function.
> + * @errp: The Error** pointer to be passed into @func2.
> + *
> + * Schedules the function @func2 for execution on the vCPU @cpu, capture
> + * any error and put it into *@errp when provided.
> + */
> +void run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
> +                 Error **errp);

Can you give this a more descriptive name, please, unless the plan
is to convert all the users of the existing run_on_cpu() and then
rename this function to that ?

thanks
-- PMM

Re: [PATCH 5/5] KVM: Hook kvm_arch_put_registers() errors to the caller

[Peter Maydell]

On Fri, 17 Jun 2022 at 15:53, Peter Xu <peterx@redhat.com> wrote:
>
> Leverage the new mechanism to pass over errors to upper stack for
> kvm_arch_put_registers() when called for the post_init() accel hook.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  accel/kvm/kvm-all.c  | 13 ++++++++++---
>  accel/kvm/kvm-cpus.h |  2 +-
>  softmmu/cpus.c       |  5 ++++-
>  3 files changed, 15 insertions(+), 5 deletions(-)

Checking for errors definitely does seem like the right thing to do.
That said:

(1) Why do we want to check for errors only on the call
for post_init synchronize, and not any of the other places
where we call kvm_arch_put_registers()?

(2) I suspect this will uncover some situations where we've
been happening-to-work because we ignore an error, and now
will start to actively fail. But I guess there's not much
we can do about that except say "we'll fix them as we encounter
bug reports about them". (I know of at least one: on the
Mac M1 running Linux, if the host doesn't have this kernel fix:
https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
then the first put_registers will fail (mostly harmlessly).
I think that's the post_init sync but it might be the post_reset
one.)

thanks
-- PMM

Re: [PATCH 2/5] cpus-common: Add run_on_cpu2()

[Peter Xu]

On Thu, Jun 23, 2022 at 02:04:03PM +0100, Peter Maydell wrote:
> On Fri, 17 Jun 2022 at 15:57, Peter Xu <peterx@redhat.com> wrote:
> >
> > This version of run_on_cpu() allows to take an Error** to detect errors.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  cpus-common.c         | 27 +++++++++++++++++++++++++++
> >  include/hw/core/cpu.h | 26 ++++++++++++++++++++++++++
> >  softmmu/cpus.c        |  6 ++++++
> >  3 files changed, 59 insertions(+)
> 
> > +/**
> > + * run_on_cpu2:
> > + * @cpu: The vCPU to run on.
> > + * @func: The function to be executed.
> > + * @data: Data to pass to the function.
> > + * @errp: The Error** pointer to be passed into @func2.
> > + *
> > + * Schedules the function @func2 for execution on the vCPU @cpu, capture
> > + * any error and put it into *@errp when provided.
> > + */
> > +void run_on_cpu2(CPUState *cpu, run_on_cpu_func2 func2, run_on_cpu_data data,
> > +                 Error **errp);
> 
> Can you give this a more descriptive name, please,

Ack on the rename, though do you have a suggestion?  I did thought about
things like run_on_cpu_with_error_captured but that's awfully long..

The use of "suffix 2" seem to be an option in this case and there're users
of it even on published KVM interfaces (to name some, KVM_[SET|GET]_PIT2,
KVM_[SET|GET]_CPUID2, KVM_[SET|GET]_SREGS2, KVM_GET_XSAVE2..), while this
is qemu helper so we can even rename when we want.

> unless the plan is to convert all the users of the existing run_on_cpu()
> and then rename this function to that ?

No plan for that, since I don't see a strong need and maybe many callers do
not care about retval.

What I plan to do is to fix the immediate migration issue only so we at
least don't hit hard-to-debug bugs even when migration completed succeeded
(it seems) on QEMU level.  That's also why I used a separate helper just to
keep the rest untouched.  We could move to the new one in other codes when
proper, but that's not part of the goal of this series.

Thanks,

-- 
Peter Xu

Re: [PATCH 5/5] KVM: Hook kvm_arch_put_registers() errors to the caller

[Peter Xu]

On Thu, Jun 23, 2022 at 02:09:43PM +0100, Peter Maydell wrote:
> On Fri, 17 Jun 2022 at 15:53, Peter Xu <peterx@redhat.com> wrote:
> >
> > Leverage the new mechanism to pass over errors to upper stack for
> > kvm_arch_put_registers() when called for the post_init() accel hook.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  accel/kvm/kvm-all.c  | 13 ++++++++++---
> >  accel/kvm/kvm-cpus.h |  2 +-
> >  softmmu/cpus.c       |  5 ++++-
> >  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> Checking for errors definitely does seem like the right thing to do.
> That said:
> 
> (1) Why do we want to check for errors only on the call
> for post_init synchronize, and not any of the other places
> where we call kvm_arch_put_registers()?

Because I only know that's what we need to keep live migration honest on
being successful, and I didn't want to spread the fire elsewhere, neither
from knowledge nor time..  So I wanted to keep the series simple but
useful.

If we have reasons to cover some of the rest, I can still try.

> 
> (2) I suspect this will uncover some situations where we've
> been happening-to-work because we ignore an error, and now
> will start to actively fail. But I guess there's not much
> we can do about that except say "we'll fix them as we encounter
> bug reports about them". (I know of at least one: on the
> Mac M1 running Linux, if the host doesn't have this kernel fix:
> https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
> then the first put_registers will fail (mostly harmlessly).
> I think that's the post_init sync but it might be the post_reset
> one.)

.. from what I read from the commit message in the link, hopefully that was
only about the reset process since that sounds like a mismatched regs
before/after gic created.  When migration completes, I guess we're always
fetching from the after-gic-created case?  But it'll be great if we double
check.  Luckily it seems only for m1.

What my series wanted to achieve is not affect anything else but migration
(so if they fail elsewhere they keep the benign failing).  What I worried
is exactly when we have benign failures on put regs on live migration use
cases, but hopefully not.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/5] CPU: Detect put cpu register errors for migrations

[Peter Xu]

On Fri, Jun 17, 2022 at 10:48:52AM -0400, Peter Xu wrote:
> rfc->v1:
> - Rebase to master, drop RFC tag.
> 
> This series teaches QEMU to detect errors when e.g. putting registers from
> QEMU to KVM, and fail migrations properly.
> 
> For the rational of this series and why it was posted, please refer to the
> bug report here:
> 
> https://lore.kernel.org/all/YppVupW+IWsm7Osr@xz-m1.local/
> 
> But I'd rather not go into that if the reviewer doesn't have that context,
> because we don't really need that complexity..  It can be simple as we
> should fail migration early when we see issues happening already, so:
> 
>   1) We fail explicitly, rather than afterward with some weird guest
>      errors.  In my bug report, it was a guest double fault.  There's
>      another bug report that Sean mentioned in the thread from Mike Tancsa
>      that can have other sympotons rather than double fault, but anyway
>      they'll be hard to diagnose since the processor state can be corrupted
>      (please refer to kvm_arch_put_registers() where we stop putting more
>      registers to KVM when we see any error).
> 
>   2) For precopy, with this early failure the VM won't crash itself since
>      we still have a chance to keep running it on src host, while if
>      without this patch we will fail later, and it can crash the VM.
> 
> In this specific case, when KVM_SET_XSAVE ioctl failed on dest host before
> start running the VM there, we should fail the migration already.
> 
> After the patchset applied, the above "double fault" issue will become
> migration failures, and...
> 
> For precopy, we can see some error dumped for precopy on dest, then the VM
> will be kept running on src host:
> 
> 2022-06-07T22:48:48.804234Z qemu-system-x86_64: kvm_arch_put_registers() failed with retval=-22
> 2022-06-07T22:48:48.804588Z qemu-system-x86_64: load of migration failed: Invalid argument
> 
> For postcopy, currently we'll pause the VM immediately for admin to decide
> what to do:
> 
> 2022-06-07T22:47:49.448192Z qemu-system-x86_64: kvm_arch_put_registers() failed with retval=-22
> 13072@1654642069.518993:runstate_set current_run_state 1 (inmigrate) new_state 4 (paused)
> 
> If something like this series is welcomed, we could do better in the future
> by telling the src host about this issue and keep running, because
> put-register happens right at the switch-over, so we actually have this
> chance (no dirty page on dest host yet).
> 
> Comments welcomed.  Thanks,

Ping,

We seem to still hit FPU-put issues and it always takes time to debug on
unsynchronized cpu registers because it can cause all kinds of errors. The
real evil should be that not only we ignored errors on e.g. put xsave, but
also we will stop putting all the rest cpu regs after any of such failure,
then qemu continues to run with synced + fail-synced + unsynced registers.

It'll be great if something like this series could still be considered.

Thanks,

-- 
Peter Xu