[PATCH 0/4] [uq/master] VCPU writeback rework and related bits

[PATCH 0/4] [uq/master] VCPU writeback rework and related bits

[Jan Kiszka]

This is the corresponding uq/master series to [1]. Should allow smooth
re-sync with upstream once merged.

Pull URL is

	git://git.kiszka.org/qemu-kvm.git queues/kvm-upstream

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/47706

Jan Kiszka (4):
  KVM: Rework of guest debug state writing
  KVM: Rework VCPU state writeback API
  KVM: x86: Restrict writeback of VCPU state
  x86: Extend validity of bsp_to_cpu

 exec.c                |   17 -----------
 hw/apic.c             |    2 -
 hw/pc.c               |    3 +-
 hw/ppc_newworld.c     |    3 --
 hw/ppc_oldworld.c     |    3 --
 hw/s390-virtio.c      |    1 -
 kvm-all.c             |   43 ++++++++++++++++++---------
 kvm.h                 |   26 ++++++++++++++++-
 savevm.c              |    4 ++
 sysemu.h              |    4 ++
 target-i386/kvm.c     |   77 +++++++++++++++++++++++++++++++++++++++---------
 target-i386/machine.c |   11 -------
 target-ppc/kvm.c      |    2 +-
 target-ppc/machine.c  |    4 --
 target-s390x/kvm.c    |    3 +-
 vl.c                  |   29 ++++++++++++++++++
 16 files changed, 157 insertions(+), 75 deletions(-)

[Qemu-devel] [PATCH 0/4] [uq/master] VCPU writeback rework and related bits

[Jan Kiszka]

This is the corresponding uq/master series to [1]. Should allow smooth
re-sync with upstream once merged.

Pull URL is

	git://git.kiszka.org/qemu-kvm.git queues/kvm-upstream

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/47706

Jan Kiszka (4):
  KVM: Rework of guest debug state writing
  KVM: Rework VCPU state writeback API
  KVM: x86: Restrict writeback of VCPU state
  x86: Extend validity of bsp_to_cpu

 exec.c                |   17 -----------
 hw/apic.c             |    2 -
 hw/pc.c               |    3 +-
 hw/ppc_newworld.c     |    3 --
 hw/ppc_oldworld.c     |    3 --
 hw/s390-virtio.c      |    1 -
 kvm-all.c             |   43 ++++++++++++++++++---------
 kvm.h                 |   26 ++++++++++++++++-
 savevm.c              |    4 ++
 sysemu.h              |    4 ++
 target-i386/kvm.c     |   77 +++++++++++++++++++++++++++++++++++++++---------
 target-i386/machine.c |   11 -------
 target-ppc/kvm.c      |    2 +-
 target-ppc/machine.c  |    4 --
 target-s390x/kvm.c    |    3 +-
 vl.c                  |   29 ++++++++++++++++++
 16 files changed, 157 insertions(+), 75 deletions(-)

[PATCH 1/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

So far we synchronized any dirty VCPU state back into the kernel before
updating the guest debug state. This was a tribute to a deficite in x86
kernels before 2.6.33. But as this is an arch-dependent issue, it is
better handle in the x86 part of KVM and remove the writeback point for
generic code. This also avoids overwriting the flushed state later on if
user space decides to change some more registers before resuming the
guest.

We furthermore need to reinject guest exceptions via the appropriate
mechanism. That is KVM_SET_GUEST_DEBUG for older kernels and
KVM_SET_VCPU_EVENTS for recent ones. Using both mechanisms at the same
time will cause state corruptions.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c         |   24 ++++++++++++++++--------
 kvm.h             |    1 +
 target-i386/kvm.c |   47 +++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 1a02076..2f7e33a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -65,6 +65,7 @@ struct KVMState
     int broken_set_mem_region;
     int migration_log;
     int vcpu_events;
+    int robust_singlestep;
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
@@ -659,6 +660,12 @@ int kvm_init(int smp_cpus)
     s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
 #endif
 
+    s->robust_singlestep = 0;
+#ifdef KVM_CAP_X86_ROBUST_SINGLESTEP
+    s->robust_singlestep =
+        kvm_check_extension(s, KVM_CAP_X86_ROBUST_SINGLESTEP);
+#endif
+
     ret = kvm_arch_init(s, smp_cpus);
     if (ret < 0)
         goto err;
@@ -917,6 +924,11 @@ int kvm_has_vcpu_events(void)
     return kvm_state->vcpu_events;
 }
 
+int kvm_has_robust_singlestep(void)
+{
+    return kvm_state->robust_singlestep;
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
@@ -974,10 +986,6 @@ static void kvm_invoke_set_guest_debug(void *data)
     struct kvm_set_guest_debug_data *dbg_data = data;
     CPUState *env = dbg_data->env;
 
-    if (env->kvm_vcpu_dirty) {
-        kvm_arch_put_registers(env);
-        env->kvm_vcpu_dirty = 0;
-    }
     dbg_data->err = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
 }
 
@@ -985,12 +993,12 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
 {
     struct kvm_set_guest_debug_data data;
 
-    data.dbg.control = 0;
-    if (env->singlestep_enabled)
-        data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    data.dbg.control = reinject_trap;
 
+    if (env->singlestep_enabled) {
+        data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     kvm_arch_update_guest_debug(env, &data.dbg);
-    data.dbg.control |= reinject_trap;
     data.env = env;
 
     on_vcpu(env, kvm_invoke_set_guest_debug, &data);
diff --git a/kvm.h b/kvm.h
index a74dfcb..a602e45 100644
--- a/kvm.h
+++ b/kvm.h
@@ -40,6 +40,7 @@ int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size);
 
 int kvm_has_sync_mmu(void);
 int kvm_has_vcpu_events(void);
+int kvm_has_robust_singlestep(void);
 
 void kvm_setup_guest_memory(void *start, size_t size);
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6b741ba..2365ca3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -851,6 +851,37 @@ static int kvm_get_vcpu_events(CPUState *env)
     return 0;
 }
 
+static int kvm_guest_debug_workarounds(CPUState *env)
+{
+    int ret = 0;
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+    unsigned long reinject_trap = 0;
+
+    if (!kvm_has_vcpu_events()) {
+        if (env->exception_injected == 1) {
+            reinject_trap = KVM_GUESTDBG_INJECT_DB;
+        } else if (env->exception_injected == 3) {
+            reinject_trap = KVM_GUESTDBG_INJECT_BP;
+        }
+        env->exception_injected = -1;
+    }
+
+    /*
+     * Kernels before KVM_CAP_X86_ROBUST_SINGLESTEP overwrote flags.TF
+     * injected via SET_GUEST_DEBUG while updating GP regs. Work around this
+     * by updating the debug state once again if single-stepping is on.
+     * Another reason to call kvm_update_guest_debug here is a pending debug
+     * trap raise by the guest. On kernels without SET_VCPU_EVENTS we have to
+     * reinject them via SET_GUEST_DEBUG.
+     */
+    if (reinject_trap ||
+        (!kvm_has_robust_singlestep() && env->singlestep_enabled)) {
+        ret = kvm_update_guest_debug(env, reinject_trap);
+    }
+#endif /* KVM_CAP_SET_GUEST_DEBUG */
+    return ret;
+}
+
 int kvm_arch_put_registers(CPUState *env)
 {
     int ret;
@@ -879,6 +910,11 @@ int kvm_arch_put_registers(CPUState *env)
     if (ret < 0)
         return ret;
 
+    /* must be last */
+    ret = kvm_guest_debug_workarounds(env);
+    if (ret < 0)
+        return ret;
+
     return 0;
 }
 
@@ -1122,10 +1158,13 @@ int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info)
     } else if (kvm_find_sw_breakpoint(cpu_single_env, arch_info->pc))
         handle = 1;
 
-    if (!handle)
-        kvm_update_guest_debug(cpu_single_env,
-                        (arch_info->exception == 1) ?
-                        KVM_GUESTDBG_INJECT_DB : KVM_GUESTDBG_INJECT_BP);
+    if (!handle) {
+        cpu_synchronize_state(cpu_single_env);
+        assert(cpu_single_env->exception_injected == -1);
+
+        cpu_single_env->exception_injected = arch_info->exception;
+        cpu_single_env->has_error_code = 0;
+    }
 
     return handle;
 }
-- 
1.6.0.2

[Qemu-devel] [PATCH 1/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

So far we synchronized any dirty VCPU state back into the kernel before
updating the guest debug state. This was a tribute to a deficite in x86
kernels before 2.6.33. But as this is an arch-dependent issue, it is
better handle in the x86 part of KVM and remove the writeback point for
generic code. This also avoids overwriting the flushed state later on if
user space decides to change some more registers before resuming the
guest.

We furthermore need to reinject guest exceptions via the appropriate
mechanism. That is KVM_SET_GUEST_DEBUG for older kernels and
KVM_SET_VCPU_EVENTS for recent ones. Using both mechanisms at the same
time will cause state corruptions.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c         |   24 ++++++++++++++++--------
 kvm.h             |    1 +
 target-i386/kvm.c |   47 +++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 1a02076..2f7e33a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -65,6 +65,7 @@ struct KVMState
     int broken_set_mem_region;
     int migration_log;
     int vcpu_events;
+    int robust_singlestep;
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
@@ -659,6 +660,12 @@ int kvm_init(int smp_cpus)
     s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
 #endif
 
+    s->robust_singlestep = 0;
+#ifdef KVM_CAP_X86_ROBUST_SINGLESTEP
+    s->robust_singlestep =
+        kvm_check_extension(s, KVM_CAP_X86_ROBUST_SINGLESTEP);
+#endif
+
     ret = kvm_arch_init(s, smp_cpus);
     if (ret < 0)
         goto err;
@@ -917,6 +924,11 @@ int kvm_has_vcpu_events(void)
     return kvm_state->vcpu_events;
 }
 
+int kvm_has_robust_singlestep(void)
+{
+    return kvm_state->robust_singlestep;
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
@@ -974,10 +986,6 @@ static void kvm_invoke_set_guest_debug(void *data)
     struct kvm_set_guest_debug_data *dbg_data = data;
     CPUState *env = dbg_data->env;
 
-    if (env->kvm_vcpu_dirty) {
-        kvm_arch_put_registers(env);
-        env->kvm_vcpu_dirty = 0;
-    }
     dbg_data->err = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
 }
 
@@ -985,12 +993,12 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
 {
     struct kvm_set_guest_debug_data data;
 
-    data.dbg.control = 0;
-    if (env->singlestep_enabled)
-        data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    data.dbg.control = reinject_trap;
 
+    if (env->singlestep_enabled) {
+        data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     kvm_arch_update_guest_debug(env, &data.dbg);
-    data.dbg.control |= reinject_trap;
     data.env = env;
 
     on_vcpu(env, kvm_invoke_set_guest_debug, &data);
diff --git a/kvm.h b/kvm.h
index a74dfcb..a602e45 100644
--- a/kvm.h
+++ b/kvm.h
@@ -40,6 +40,7 @@ int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size);
 
 int kvm_has_sync_mmu(void);
 int kvm_has_vcpu_events(void);
+int kvm_has_robust_singlestep(void);
 
 void kvm_setup_guest_memory(void *start, size_t size);
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6b741ba..2365ca3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -851,6 +851,37 @@ static int kvm_get_vcpu_events(CPUState *env)
     return 0;
 }
 
+static int kvm_guest_debug_workarounds(CPUState *env)
+{
+    int ret = 0;
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+    unsigned long reinject_trap = 0;
+
+    if (!kvm_has_vcpu_events()) {
+        if (env->exception_injected == 1) {
+            reinject_trap = KVM_GUESTDBG_INJECT_DB;
+        } else if (env->exception_injected == 3) {
+            reinject_trap = KVM_GUESTDBG_INJECT_BP;
+        }
+        env->exception_injected = -1;
+    }
+
+    /*
+     * Kernels before KVM_CAP_X86_ROBUST_SINGLESTEP overwrote flags.TF
+     * injected via SET_GUEST_DEBUG while updating GP regs. Work around this
+     * by updating the debug state once again if single-stepping is on.
+     * Another reason to call kvm_update_guest_debug here is a pending debug
+     * trap raise by the guest. On kernels without SET_VCPU_EVENTS we have to
+     * reinject them via SET_GUEST_DEBUG.
+     */
+    if (reinject_trap ||
+        (!kvm_has_robust_singlestep() && env->singlestep_enabled)) {
+        ret = kvm_update_guest_debug(env, reinject_trap);
+    }
+#endif /* KVM_CAP_SET_GUEST_DEBUG */
+    return ret;
+}
+
 int kvm_arch_put_registers(CPUState *env)
 {
     int ret;
@@ -879,6 +910,11 @@ int kvm_arch_put_registers(CPUState *env)
     if (ret < 0)
         return ret;
 
+    /* must be last */
+    ret = kvm_guest_debug_workarounds(env);
+    if (ret < 0)
+        return ret;
+
     return 0;
 }
 
@@ -1122,10 +1158,13 @@ int kvm_arch_debug(struct kvm_debug_exit_arch *arch_info)
     } else if (kvm_find_sw_breakpoint(cpu_single_env, arch_info->pc))
         handle = 1;
 
-    if (!handle)
-        kvm_update_guest_debug(cpu_single_env,
-                        (arch_info->exception == 1) ?
-                        KVM_GUESTDBG_INJECT_DB : KVM_GUESTDBG_INJECT_BP);
+    if (!handle) {
+        cpu_synchronize_state(cpu_single_env);
+        assert(cpu_single_env->exception_injected == -1);
+
+        cpu_single_env->exception_injected = arch_info->exception;
+        cpu_single_env->has_error_code = 0;
+    }
 
     return handle;
 }
-- 
1.6.0.2

[PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

This grand cleanup drops all reset and vmsave/load related
synchronization points in favor of four(!) generic hooks:

- cpu_synchronize_all_states in qemu_savevm_state_complete
  (initial sync from kernel before vmsave)
- cpu_synchronize_all_post_init in qemu_loadvm_state
  (writeback after vmload)
- cpu_synchronize_all_post_init in main after machine init
- cpu_synchronize_all_post_reset in qemu_system_reset
  (writeback after system reset)

These writeback points + the existing one of VCPU exec after
cpu_synchronize_state map on three levels of writeback:

- KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
- KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
- KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)

This level is passed to the arch-specific VCPU state writing function
that will decide which concrete substates need to be written. That way,
no writer of load, save or reset functions that interact with in-kernel
KVM states will ever have to worry about synchronization again. That
also means that a lot of reasons for races, segfaults and deadlocks are
eliminated.

cpu_synchronize_state remains untouched, just as Anthony suggested. We
continue to need it before reading or writing of VCPU states that are
also tracked by in-kernel KVM subsystems.

Consequently, this patch removes many cpu_synchronize_state calls that
are now redundant, just like remaining explicit register syncs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                |   17 -----------------
 hw/apic.c             |    2 --
 hw/ppc_newworld.c     |    3 ---
 hw/ppc_oldworld.c     |    3 ---
 hw/s390-virtio.c      |    1 -
 kvm-all.c             |   19 +++++++++++++------
 kvm.h                 |   25 ++++++++++++++++++++++++-
 savevm.c              |    4 ++++
 sysemu.h              |    4 ++++
 target-i386/kvm.c     |    2 +-
 target-i386/machine.c |   11 -----------
 target-ppc/kvm.c      |    2 +-
 target-ppc/machine.c  |    4 ----
 target-s390x/kvm.c    |    3 +--
 vl.c                  |   29 +++++++++++++++++++++++++++++
 15 files changed, 77 insertions(+), 52 deletions(-)

diff --git a/exec.c b/exec.c
index 8616ff9..50a2e46 100644
--- a/exec.c
+++ b/exec.c
@@ -518,21 +518,6 @@ void cpu_exec_init_all(unsigned long tb_size)
 
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
 
-static void cpu_common_pre_save(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-}
-
-static int cpu_common_pre_load(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-    return 0;
-}
-
 static int cpu_common_post_load(void *opaque, int version_id)
 {
     CPUState *env = opaque;
@@ -550,8 +535,6 @@ static const VMStateDescription vmstate_cpu_common = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = cpu_common_pre_save,
-    .pre_load = cpu_common_pre_load,
     .post_load = cpu_common_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINT32(halted, CPUState),
diff --git a/hw/apic.c b/hw/apic.c
index 87e7dc0..3c90f4c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -938,8 +938,6 @@ static void apic_reset(void *opaque)
     APICState *s = opaque;
     int bsp;
 
-    cpu_synchronize_state(s->cpu_env);
-
     bsp = cpu_is_bsp(s->cpu_env);
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index bc86c85..d4f9013 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -167,9 +167,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
         envs[i] = env;
     }
 
-    /* Make sure all register sets take effect */
-    cpu_synchronize_state(env);
-
     /* allocate RAM */
     ram_offset = qemu_ram_alloc(ram_size);
     cpu_register_physical_memory(0, ram_size, ram_offset);
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index 04a7835..93c95ba 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -165,9 +165,6 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
         envs[i] = env;
     }
 
-    /* Make sure all register sets take effect */
-    cpu_synchronize_state(env);
-
     /* allocate RAM */
     if (ram_size > (2047 << 20)) {
         fprintf(stderr,
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 3582728..ad3386f 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -185,7 +185,6 @@ static void s390_init(ram_addr_t ram_size,
             exit(1);
         }
 
-        cpu_synchronize_state(env);
         env->psw.addr = KERN_IMAGE_START;
         env->psw.mask = 0x0000000180000000ULL;
     }
diff --git a/kvm-all.c b/kvm-all.c
index 2f7e33a..534ead0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -156,10 +156,6 @@ static void kvm_reset_vcpu(void *opaque)
     CPUState *env = opaque;
 
     kvm_arch_reset_vcpu(env);
-    if (kvm_arch_put_registers(env)) {
-        fprintf(stderr, "Fatal: kvm vcpu reset failed\n");
-        abort();
-    }
 }
 
 int kvm_irqchip_in_kernel(void)
@@ -214,7 +210,6 @@ int kvm_init_vcpu(CPUState *env)
     if (ret == 0) {
         qemu_register_reset(kvm_reset_vcpu, env);
         kvm_arch_reset_vcpu(env);
-        ret = kvm_arch_put_registers(env);
     }
 err:
     return ret;
@@ -753,6 +748,18 @@ void kvm_cpu_synchronize_state(CPUState *env)
     }
 }
 
+void kvm_cpu_synchronize_post_reset(CPUState *env)
+{
+    kvm_arch_put_registers(env, KVM_PUT_RESET_STATE);
+    env->kvm_vcpu_dirty = 0;
+}
+
+void kvm_cpu_synchronize_post_init(CPUState *env)
+{
+    kvm_arch_put_registers(env, KVM_PUT_FULL_STATE);
+    env->kvm_vcpu_dirty = 0;
+}
+
 int kvm_cpu_exec(CPUState *env)
 {
     struct kvm_run *run = env->kvm_run;
@@ -770,7 +777,7 @@ int kvm_cpu_exec(CPUState *env)
 #endif
 
         if (env->kvm_vcpu_dirty) {
-            kvm_arch_put_registers(env);
+            kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
             env->kvm_vcpu_dirty = 0;
         }
 
diff --git a/kvm.h b/kvm.h
index a602e45..b2937b9 100644
--- a/kvm.h
+++ b/kvm.h
@@ -82,7 +82,14 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run);
 
 int kvm_arch_get_registers(CPUState *env);
 
-int kvm_arch_put_registers(CPUState *env);
+/* state subset only touched by the VCPU itself during runtime */
+#define KVM_PUT_RUNTIME_STATE   1
+/* state subset modified during VCPU reset */
+#define KVM_PUT_RESET_STATE     2
+/* full state set, modified during initialization or on vmload */
+#define KVM_PUT_FULL_STATE      3
+
+int kvm_arch_put_registers(CPUState *env, int level);
 
 int kvm_arch_init(KVMState *s, int smp_cpus);
 
@@ -126,6 +133,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
                                       int reg);
 void kvm_cpu_synchronize_state(CPUState *env);
+void kvm_cpu_synchronize_post_reset(CPUState *env);
+void kvm_cpu_synchronize_post_init(CPUState *env);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -136,4 +145,18 @@ static inline void cpu_synchronize_state(CPUState *env)
     }
 }
 
+static inline void cpu_synchronize_post_reset(CPUState *env)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_synchronize_post_reset(env);
+    }
+}
+
+static inline void cpu_synchronize_post_init(CPUState *env)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_synchronize_post_init(env);
+    }
+}
+
 #endif
diff --git a/savevm.c b/savevm.c
index 4b58663..a6e774b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1345,6 +1345,8 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
 {
     SaveStateEntry *se;
 
+    cpu_synchronize_all_states();
+
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->save_live_state == NULL)
             continue;
@@ -1545,6 +1547,8 @@ int qemu_loadvm_state(QEMUFile *f)
         }
     }
 
+    cpu_synchronize_all_post_init();
+
     ret = 0;
 
 out:
diff --git a/sysemu.h b/sysemu.h
index 8ba618e..d77344c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -58,6 +58,10 @@ int load_vmstate(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const QDict *qdict);
 void do_info_snapshots(Monitor *mon);
 
+void cpu_synchronize_all_states(void);
+void cpu_synchronize_all_post_reset(void);
+void cpu_synchronize_all_post_init(void);
+
 void qemu_announce_self(void);
 
 void main_loop_wait(int timeout);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2365ca3..a4767b2 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -882,7 +882,7 @@ static int kvm_guest_debug_workarounds(CPUState *env)
     return ret;
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     int ret;
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 8770491..b547e2a 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -321,8 +321,6 @@ static void cpu_pre_save(void *opaque)
     CPUState *env = opaque;
     int i;
 
-    cpu_synchronize_state(env);
-
     /* FPU */
     env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
     env->fptag_vmstate = 0;
@@ -337,14 +335,6 @@ static void cpu_pre_save(void *opaque)
 #endif
 }
 
-static int cpu_pre_load(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-    return 0;
-}
-
 static int cpu_post_load(void *opaque, int version_id)
 {
     CPUState *env = opaque;
@@ -373,7 +363,6 @@ static const VMStateDescription vmstate_cpu = {
     .minimum_version_id = 3,
     .minimum_version_id_old = 3,
     .pre_save = cpu_pre_save,
-    .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINTTL_ARRAY(regs, CPUState, CPU_NB_REGS),
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 8ad0037..aa3d432 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -73,7 +73,7 @@ void kvm_arch_reset_vcpu(CPUState *env)
 {
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     struct kvm_regs regs;
     int ret;
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 4897c8a..67de951 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -7,8 +7,6 @@ void cpu_save(QEMUFile *f, void *opaque)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
-    cpu_synchronize_state(env);
-
     for (i = 0; i < 32; i++)
         qemu_put_betls(f, &env->gpr[i]);
 #if !defined(TARGET_PPC64)
@@ -96,8 +94,6 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
-    cpu_synchronize_state(env);
-
     for (i = 0; i < 32; i++)
         qemu_get_betls(f, &env->gpr[i]);
 #if !defined(TARGET_PPC64)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 0199a65..72e77b0 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -91,7 +91,7 @@ void kvm_arch_reset_vcpu(CPUState *env)
     /* FIXME: add code to reset vcpu. */
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     struct kvm_regs regs;
     int ret;
@@ -296,7 +296,6 @@ static int handle_hypercall(CPUState *env, struct kvm_run *run)
 
     cpu_synchronize_state(env);
     r = s390_virtio_hypercall(env);
-    kvm_arch_put_registers(env);
 
     return r;
 }
diff --git a/vl.c b/vl.c
index 66e477a..67143a7 100644
--- a/vl.c
+++ b/vl.c
@@ -3002,6 +3002,33 @@ static void nographic_update(void *opaque)
     qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
 }
 
+void cpu_synchronize_all_states(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_state(cpu);
+    }
+}
+
+void cpu_synchronize_all_post_reset(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_post_reset(cpu);
+    }
+}
+
+void cpu_synchronize_all_post_init(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_post_init(cpu);
+    }
+}
+
 struct vm_change_state_entry {
     VMChangeStateHandler *cb;
     void *opaque;
@@ -3143,6 +3170,7 @@ void qemu_system_reset(void)
     QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
         re->func(re->opaque);
     }
+    cpu_synchronize_all_post_reset();
 }
 
 void qemu_system_reset_request(void)
@@ -5927,6 +5955,7 @@ int main(int argc, char **argv, char **envp)
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
+    cpu_synchronize_all_post_init();
 
 #ifndef _WIN32
     /* must be after terminal init, SDL library changes signal handlers */
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

This grand cleanup drops all reset and vmsave/load related
synchronization points in favor of four(!) generic hooks:

- cpu_synchronize_all_states in qemu_savevm_state_complete
  (initial sync from kernel before vmsave)
- cpu_synchronize_all_post_init in qemu_loadvm_state
  (writeback after vmload)
- cpu_synchronize_all_post_init in main after machine init
- cpu_synchronize_all_post_reset in qemu_system_reset
  (writeback after system reset)

These writeback points + the existing one of VCPU exec after
cpu_synchronize_state map on three levels of writeback:

- KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
- KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
- KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)

This level is passed to the arch-specific VCPU state writing function
that will decide which concrete substates need to be written. That way,
no writer of load, save or reset functions that interact with in-kernel
KVM states will ever have to worry about synchronization again. That
also means that a lot of reasons for races, segfaults and deadlocks are
eliminated.

cpu_synchronize_state remains untouched, just as Anthony suggested. We
continue to need it before reading or writing of VCPU states that are
also tracked by in-kernel KVM subsystems.

Consequently, this patch removes many cpu_synchronize_state calls that
are now redundant, just like remaining explicit register syncs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c                |   17 -----------------
 hw/apic.c             |    2 --
 hw/ppc_newworld.c     |    3 ---
 hw/ppc_oldworld.c     |    3 ---
 hw/s390-virtio.c      |    1 -
 kvm-all.c             |   19 +++++++++++++------
 kvm.h                 |   25 ++++++++++++++++++++++++-
 savevm.c              |    4 ++++
 sysemu.h              |    4 ++++
 target-i386/kvm.c     |    2 +-
 target-i386/machine.c |   11 -----------
 target-ppc/kvm.c      |    2 +-
 target-ppc/machine.c  |    4 ----
 target-s390x/kvm.c    |    3 +--
 vl.c                  |   29 +++++++++++++++++++++++++++++
 15 files changed, 77 insertions(+), 52 deletions(-)

diff --git a/exec.c b/exec.c
index 8616ff9..50a2e46 100644
--- a/exec.c
+++ b/exec.c
@@ -518,21 +518,6 @@ void cpu_exec_init_all(unsigned long tb_size)
 
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
 
-static void cpu_common_pre_save(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-}
-
-static int cpu_common_pre_load(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-    return 0;
-}
-
 static int cpu_common_post_load(void *opaque, int version_id)
 {
     CPUState *env = opaque;
@@ -550,8 +535,6 @@ static const VMStateDescription vmstate_cpu_common = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = cpu_common_pre_save,
-    .pre_load = cpu_common_pre_load,
     .post_load = cpu_common_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINT32(halted, CPUState),
diff --git a/hw/apic.c b/hw/apic.c
index 87e7dc0..3c90f4c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -938,8 +938,6 @@ static void apic_reset(void *opaque)
     APICState *s = opaque;
     int bsp;
 
-    cpu_synchronize_state(s->cpu_env);
-
     bsp = cpu_is_bsp(s->cpu_env);
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index bc86c85..d4f9013 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -167,9 +167,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
         envs[i] = env;
     }
 
-    /* Make sure all register sets take effect */
-    cpu_synchronize_state(env);
-
     /* allocate RAM */
     ram_offset = qemu_ram_alloc(ram_size);
     cpu_register_physical_memory(0, ram_size, ram_offset);
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index 04a7835..93c95ba 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -165,9 +165,6 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
         envs[i] = env;
     }
 
-    /* Make sure all register sets take effect */
-    cpu_synchronize_state(env);
-
     /* allocate RAM */
     if (ram_size > (2047 << 20)) {
         fprintf(stderr,
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 3582728..ad3386f 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -185,7 +185,6 @@ static void s390_init(ram_addr_t ram_size,
             exit(1);
         }
 
-        cpu_synchronize_state(env);
         env->psw.addr = KERN_IMAGE_START;
         env->psw.mask = 0x0000000180000000ULL;
     }
diff --git a/kvm-all.c b/kvm-all.c
index 2f7e33a..534ead0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -156,10 +156,6 @@ static void kvm_reset_vcpu(void *opaque)
     CPUState *env = opaque;
 
     kvm_arch_reset_vcpu(env);
-    if (kvm_arch_put_registers(env)) {
-        fprintf(stderr, "Fatal: kvm vcpu reset failed\n");
-        abort();
-    }
 }
 
 int kvm_irqchip_in_kernel(void)
@@ -214,7 +210,6 @@ int kvm_init_vcpu(CPUState *env)
     if (ret == 0) {
         qemu_register_reset(kvm_reset_vcpu, env);
         kvm_arch_reset_vcpu(env);
-        ret = kvm_arch_put_registers(env);
     }
 err:
     return ret;
@@ -753,6 +748,18 @@ void kvm_cpu_synchronize_state(CPUState *env)
     }
 }
 
+void kvm_cpu_synchronize_post_reset(CPUState *env)
+{
+    kvm_arch_put_registers(env, KVM_PUT_RESET_STATE);
+    env->kvm_vcpu_dirty = 0;
+}
+
+void kvm_cpu_synchronize_post_init(CPUState *env)
+{
+    kvm_arch_put_registers(env, KVM_PUT_FULL_STATE);
+    env->kvm_vcpu_dirty = 0;
+}
+
 int kvm_cpu_exec(CPUState *env)
 {
     struct kvm_run *run = env->kvm_run;
@@ -770,7 +777,7 @@ int kvm_cpu_exec(CPUState *env)
 #endif
 
         if (env->kvm_vcpu_dirty) {
-            kvm_arch_put_registers(env);
+            kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
             env->kvm_vcpu_dirty = 0;
         }
 
diff --git a/kvm.h b/kvm.h
index a602e45..b2937b9 100644
--- a/kvm.h
+++ b/kvm.h
@@ -82,7 +82,14 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run);
 
 int kvm_arch_get_registers(CPUState *env);
 
-int kvm_arch_put_registers(CPUState *env);
+/* state subset only touched by the VCPU itself during runtime */
+#define KVM_PUT_RUNTIME_STATE   1
+/* state subset modified during VCPU reset */
+#define KVM_PUT_RESET_STATE     2
+/* full state set, modified during initialization or on vmload */
+#define KVM_PUT_FULL_STATE      3
+
+int kvm_arch_put_registers(CPUState *env, int level);
 
 int kvm_arch_init(KVMState *s, int smp_cpus);
 
@@ -126,6 +133,8 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
                                       int reg);
 void kvm_cpu_synchronize_state(CPUState *env);
+void kvm_cpu_synchronize_post_reset(CPUState *env);
+void kvm_cpu_synchronize_post_init(CPUState *env);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
@@ -136,4 +145,18 @@ static inline void cpu_synchronize_state(CPUState *env)
     }
 }
 
+static inline void cpu_synchronize_post_reset(CPUState *env)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_synchronize_post_reset(env);
+    }
+}
+
+static inline void cpu_synchronize_post_init(CPUState *env)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_synchronize_post_init(env);
+    }
+}
+
 #endif
diff --git a/savevm.c b/savevm.c
index 4b58663..a6e774b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1345,6 +1345,8 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
 {
     SaveStateEntry *se;
 
+    cpu_synchronize_all_states();
+
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (se->save_live_state == NULL)
             continue;
@@ -1545,6 +1547,8 @@ int qemu_loadvm_state(QEMUFile *f)
         }
     }
 
+    cpu_synchronize_all_post_init();
+
     ret = 0;
 
 out:
diff --git a/sysemu.h b/sysemu.h
index 8ba618e..d77344c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -58,6 +58,10 @@ int load_vmstate(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const QDict *qdict);
 void do_info_snapshots(Monitor *mon);
 
+void cpu_synchronize_all_states(void);
+void cpu_synchronize_all_post_reset(void);
+void cpu_synchronize_all_post_init(void);
+
 void qemu_announce_self(void);
 
 void main_loop_wait(int timeout);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2365ca3..a4767b2 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -882,7 +882,7 @@ static int kvm_guest_debug_workarounds(CPUState *env)
     return ret;
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     int ret;
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 8770491..b547e2a 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -321,8 +321,6 @@ static void cpu_pre_save(void *opaque)
     CPUState *env = opaque;
     int i;
 
-    cpu_synchronize_state(env);
-
     /* FPU */
     env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
     env->fptag_vmstate = 0;
@@ -337,14 +335,6 @@ static void cpu_pre_save(void *opaque)
 #endif
 }
 
-static int cpu_pre_load(void *opaque)
-{
-    CPUState *env = opaque;
-
-    cpu_synchronize_state(env);
-    return 0;
-}
-
 static int cpu_post_load(void *opaque, int version_id)
 {
     CPUState *env = opaque;
@@ -373,7 +363,6 @@ static const VMStateDescription vmstate_cpu = {
     .minimum_version_id = 3,
     .minimum_version_id_old = 3,
     .pre_save = cpu_pre_save,
-    .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINTTL_ARRAY(regs, CPUState, CPU_NB_REGS),
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 8ad0037..aa3d432 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -73,7 +73,7 @@ void kvm_arch_reset_vcpu(CPUState *env)
 {
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     struct kvm_regs regs;
     int ret;
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 4897c8a..67de951 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -7,8 +7,6 @@ void cpu_save(QEMUFile *f, void *opaque)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
-    cpu_synchronize_state(env);
-
     for (i = 0; i < 32; i++)
         qemu_put_betls(f, &env->gpr[i]);
 #if !defined(TARGET_PPC64)
@@ -96,8 +94,6 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
-    cpu_synchronize_state(env);
-
     for (i = 0; i < 32; i++)
         qemu_get_betls(f, &env->gpr[i]);
 #if !defined(TARGET_PPC64)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 0199a65..72e77b0 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -91,7 +91,7 @@ void kvm_arch_reset_vcpu(CPUState *env)
     /* FIXME: add code to reset vcpu. */
 }
 
-int kvm_arch_put_registers(CPUState *env)
+int kvm_arch_put_registers(CPUState *env, int level)
 {
     struct kvm_regs regs;
     int ret;
@@ -296,7 +296,6 @@ static int handle_hypercall(CPUState *env, struct kvm_run *run)
 
     cpu_synchronize_state(env);
     r = s390_virtio_hypercall(env);
-    kvm_arch_put_registers(env);
 
     return r;
 }
diff --git a/vl.c b/vl.c
index 66e477a..67143a7 100644
--- a/vl.c
+++ b/vl.c
@@ -3002,6 +3002,33 @@ static void nographic_update(void *opaque)
     qemu_mod_timer(nographic_timer, interval + qemu_get_clock(rt_clock));
 }
 
+void cpu_synchronize_all_states(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_state(cpu);
+    }
+}
+
+void cpu_synchronize_all_post_reset(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_post_reset(cpu);
+    }
+}
+
+void cpu_synchronize_all_post_init(void)
+{
+    CPUState *cpu;
+
+    for (cpu = first_cpu; cpu; cpu = cpu->next_cpu) {
+        cpu_synchronize_post_init(cpu);
+    }
+}
+
 struct vm_change_state_entry {
     VMChangeStateHandler *cb;
     void *opaque;
@@ -3143,6 +3170,7 @@ void qemu_system_reset(void)
     QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
         re->func(re->opaque);
     }
+    cpu_synchronize_all_post_reset();
 }
 
 void qemu_system_reset_request(void)
@@ -5927,6 +5955,7 @@ int main(int argc, char **argv, char **envp)
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
+    cpu_synchronize_all_post_init();
 
 #ifndef _WIN32
     /* must be after terminal init, SDL library changes signal handlers */
-- 
1.6.0.2

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> This grand cleanup drops all reset and vmsave/load related
> synchronization points in favor of four(!) generic hooks:
> 
> - cpu_synchronize_all_states in qemu_savevm_state_complete
>   (initial sync from kernel before vmsave)
> - cpu_synchronize_all_post_init in qemu_loadvm_state
>   (writeback after vmload)
> - cpu_synchronize_all_post_init in main after machine init
> - cpu_synchronize_all_post_reset in qemu_system_reset
>   (writeback after system reset)
> 
> These writeback points + the existing one of VCPU exec after
> cpu_synchronize_state map on three levels of writeback:
> 
> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> 
> This level is passed to the arch-specific VCPU state writing function
> that will decide which concrete substates need to be written. That way,
> no writer of load, save or reset functions that interact with in-kernel
> KVM states will ever have to worry about synchronization again. That
> also means that a lot of reasons for races, segfaults and deadlocks are
> eliminated.
> 
> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> continue to need it before reading or writing of VCPU states that are
> also tracked by in-kernel KVM subsystems.
> 
> Consequently, this patch removes many cpu_synchronize_state calls that
> are now redundant, just like remaining explicit register syncs.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan,

This patch breaks system reset of WinXP.32 install (more easily
reproducible without iothread enabled).

Screenshot attached.


[-- Attachment #2: uqmaster-failure.png --]
[-- Type: image/png, Size: 4451 bytes --]

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> This grand cleanup drops all reset and vmsave/load related
> synchronization points in favor of four(!) generic hooks:
> 
> - cpu_synchronize_all_states in qemu_savevm_state_complete
>   (initial sync from kernel before vmsave)
> - cpu_synchronize_all_post_init in qemu_loadvm_state
>   (writeback after vmload)
> - cpu_synchronize_all_post_init in main after machine init
> - cpu_synchronize_all_post_reset in qemu_system_reset
>   (writeback after system reset)
> 
> These writeback points + the existing one of VCPU exec after
> cpu_synchronize_state map on three levels of writeback:
> 
> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> 
> This level is passed to the arch-specific VCPU state writing function
> that will decide which concrete substates need to be written. That way,
> no writer of load, save or reset functions that interact with in-kernel
> KVM states will ever have to worry about synchronization again. That
> also means that a lot of reasons for races, segfaults and deadlocks are
> eliminated.
> 
> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> continue to need it before reading or writing of VCPU states that are
> also tracked by in-kernel KVM subsystems.
> 
> Consequently, this patch removes many cpu_synchronize_state calls that
> are now redundant, just like remaining explicit register syncs.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan,

This patch breaks system reset of WinXP.32 install (more easily
reproducible without iothread enabled).

Screenshot attached.


[-- Attachment #2: uqmaster-failure.png --]
[-- Type: image/png, Size: 4451 bytes --]

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

Marcelo Tosatti wrote:
> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>> This grand cleanup drops all reset and vmsave/load related
>> synchronization points in favor of four(!) generic hooks:
>>
>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>   (initial sync from kernel before vmsave)
>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>   (writeback after vmload)
>> - cpu_synchronize_all_post_init in main after machine init
>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>   (writeback after system reset)
>>
>> These writeback points + the existing one of VCPU exec after
>> cpu_synchronize_state map on three levels of writeback:
>>
>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>
>> This level is passed to the arch-specific VCPU state writing function
>> that will decide which concrete substates need to be written. That way,
>> no writer of load, save or reset functions that interact with in-kernel
>> KVM states will ever have to worry about synchronization again. That
>> also means that a lot of reasons for races, segfaults and deadlocks are
>> eliminated.
>>
>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>> continue to need it before reading or writing of VCPU states that are
>> also tracked by in-kernel KVM subsystems.
>>
>> Consequently, this patch removes many cpu_synchronize_state calls that
>> are now redundant, just like remaining explicit register syncs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Jan,
> 
> This patch breaks system reset of WinXP.32 install (more easily
> reproducible without iothread enabled).
> 
> Screenshot attached.
> 

Strange - no issues with qemu-kvm? Any special command line switch? /me
goes scrounging for some installation XP32 CD in the meantime...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

Marcelo Tosatti wrote:
> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>> This grand cleanup drops all reset and vmsave/load related
>> synchronization points in favor of four(!) generic hooks:
>>
>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>   (initial sync from kernel before vmsave)
>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>   (writeback after vmload)
>> - cpu_synchronize_all_post_init in main after machine init
>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>   (writeback after system reset)
>>
>> These writeback points + the existing one of VCPU exec after
>> cpu_synchronize_state map on three levels of writeback:
>>
>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>
>> This level is passed to the arch-specific VCPU state writing function
>> that will decide which concrete substates need to be written. That way,
>> no writer of load, save or reset functions that interact with in-kernel
>> KVM states will ever have to worry about synchronization again. That
>> also means that a lot of reasons for races, segfaults and deadlocks are
>> eliminated.
>>
>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>> continue to need it before reading or writing of VCPU states that are
>> also tracked by in-kernel KVM subsystems.
>>
>> Consequently, this patch removes many cpu_synchronize_state calls that
>> are now redundant, just like remaining explicit register syncs.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Jan,
> 
> This patch breaks system reset of WinXP.32 install (more easily
> reproducible without iothread enabled).
> 
> Screenshot attached.
> 

Strange - no issues with qemu-kvm? Any special command line switch? /me
goes scrounging for some installation XP32 CD in the meantime...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >> This grand cleanup drops all reset and vmsave/load related
> >> synchronization points in favor of four(!) generic hooks:
> >>
> >> - cpu_synchronize_all_states in qemu_savevm_state_complete
> >>   (initial sync from kernel before vmsave)
> >> - cpu_synchronize_all_post_init in qemu_loadvm_state
> >>   (writeback after vmload)
> >> - cpu_synchronize_all_post_init in main after machine init
> >> - cpu_synchronize_all_post_reset in qemu_system_reset
> >>   (writeback after system reset)
> >>
> >> These writeback points + the existing one of VCPU exec after
> >> cpu_synchronize_state map on three levels of writeback:
> >>
> >> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>
> >> This level is passed to the arch-specific VCPU state writing function
> >> that will decide which concrete substates need to be written. That way,
> >> no writer of load, save or reset functions that interact with in-kernel
> >> KVM states will ever have to worry about synchronization again. That
> >> also means that a lot of reasons for races, segfaults and deadlocks are
> >> eliminated.
> >>
> >> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >> continue to need it before reading or writing of VCPU states that are
> >> also tracked by in-kernel KVM subsystems.
> >>
> >> Consequently, this patch removes many cpu_synchronize_state calls that
> >> are now redundant, just like remaining explicit register syncs.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Jan,
> > 
> > This patch breaks system reset of WinXP.32 install (more easily
> > reproducible without iothread enabled).
> > 
> > Screenshot attached.
> > 
> 
> Strange - no issues with qemu-kvm? Any special command line switch? /me
> goes scrounging for some installation XP32 CD in the meantime...

No issues with qemu-kvm. Could not spot anything obvious.

> 
> Jan
> 

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >> This grand cleanup drops all reset and vmsave/load related
> >> synchronization points in favor of four(!) generic hooks:
> >>
> >> - cpu_synchronize_all_states in qemu_savevm_state_complete
> >>   (initial sync from kernel before vmsave)
> >> - cpu_synchronize_all_post_init in qemu_loadvm_state
> >>   (writeback after vmload)
> >> - cpu_synchronize_all_post_init in main after machine init
> >> - cpu_synchronize_all_post_reset in qemu_system_reset
> >>   (writeback after system reset)
> >>
> >> These writeback points + the existing one of VCPU exec after
> >> cpu_synchronize_state map on three levels of writeback:
> >>
> >> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>
> >> This level is passed to the arch-specific VCPU state writing function
> >> that will decide which concrete substates need to be written. That way,
> >> no writer of load, save or reset functions that interact with in-kernel
> >> KVM states will ever have to worry about synchronization again. That
> >> also means that a lot of reasons for races, segfaults and deadlocks are
> >> eliminated.
> >>
> >> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >> continue to need it before reading or writing of VCPU states that are
> >> also tracked by in-kernel KVM subsystems.
> >>
> >> Consequently, this patch removes many cpu_synchronize_state calls that
> >> are now redundant, just like remaining explicit register syncs.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Jan,
> > 
> > This patch breaks system reset of WinXP.32 install (more easily
> > reproducible without iothread enabled).
> > 
> > Screenshot attached.
> > 
> 
> Strange - no issues with qemu-kvm? Any special command line switch? /me
> goes scrounging for some installation XP32 CD in the meantime...

No issues with qemu-kvm. Could not spot anything obvious.

> 
> Jan
> 

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>>>> This grand cleanup drops all reset and vmsave/load related
>>>> synchronization points in favor of four(!) generic hooks:
>>>>
>>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>>>   (initial sync from kernel before vmsave)
>>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>>>   (writeback after vmload)
>>>> - cpu_synchronize_all_post_init in main after machine init
>>>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>>>   (writeback after system reset)
>>>>
>>>> These writeback points + the existing one of VCPU exec after
>>>> cpu_synchronize_state map on three levels of writeback:
>>>>
>>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>>>
>>>> This level is passed to the arch-specific VCPU state writing function
>>>> that will decide which concrete substates need to be written. That way,
>>>> no writer of load, save or reset functions that interact with in-kernel
>>>> KVM states will ever have to worry about synchronization again. That
>>>> also means that a lot of reasons for races, segfaults and deadlocks are
>>>> eliminated.
>>>>
>>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>>>> continue to need it before reading or writing of VCPU states that are
>>>> also tracked by in-kernel KVM subsystems.
>>>>
>>>> Consequently, this patch removes many cpu_synchronize_state calls that
>>>> are now redundant, just like remaining explicit register syncs.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Jan,
>>>
>>> This patch breaks system reset of WinXP.32 install (more easily
>>> reproducible without iothread enabled).
>>>
>>> Screenshot attached.
>>>
>> Strange - no issues with qemu-kvm? Any special command line switch? /me
>> goes scrounging for some installation XP32 CD in the meantime...
> 
> No issues with qemu-kvm. Could not spot anything obvious.
> 

And, of course, my WinXP installation did not trigger any reset issue,
even in non-iothreaded mode. :(

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>>>> This grand cleanup drops all reset and vmsave/load related
>>>> synchronization points in favor of four(!) generic hooks:
>>>>
>>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>>>   (initial sync from kernel before vmsave)
>>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>>>   (writeback after vmload)
>>>> - cpu_synchronize_all_post_init in main after machine init
>>>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>>>   (writeback after system reset)
>>>>
>>>> These writeback points + the existing one of VCPU exec after
>>>> cpu_synchronize_state map on three levels of writeback:
>>>>
>>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>>>
>>>> This level is passed to the arch-specific VCPU state writing function
>>>> that will decide which concrete substates need to be written. That way,
>>>> no writer of load, save or reset functions that interact with in-kernel
>>>> KVM states will ever have to worry about synchronization again. That
>>>> also means that a lot of reasons for races, segfaults and deadlocks are
>>>> eliminated.
>>>>
>>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>>>> continue to need it before reading or writing of VCPU states that are
>>>> also tracked by in-kernel KVM subsystems.
>>>>
>>>> Consequently, this patch removes many cpu_synchronize_state calls that
>>>> are now redundant, just like remaining explicit register syncs.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Jan,
>>>
>>> This patch breaks system reset of WinXP.32 install (more easily
>>> reproducible without iothread enabled).
>>>
>>> Screenshot attached.
>>>
>> Strange - no issues with qemu-kvm? Any special command line switch? /me
>> goes scrounging for some installation XP32 CD in the meantime...
> 
> No issues with qemu-kvm. Could not spot anything obvious.
> 

And, of course, my WinXP installation did not trigger any reset issue,
even in non-iothreaded mode. :(

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >>>> This grand cleanup drops all reset and vmsave/load related
> >>>> synchronization points in favor of four(!) generic hooks:
> >>>>
> >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> >>>>   (initial sync from kernel before vmsave)
> >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> >>>>   (writeback after vmload)
> >>>> - cpu_synchronize_all_post_init in main after machine init
> >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> >>>>   (writeback after system reset)
> >>>>
> >>>> These writeback points + the existing one of VCPU exec after
> >>>> cpu_synchronize_state map on three levels of writeback:
> >>>>
> >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>>>
> >>>> This level is passed to the arch-specific VCPU state writing function
> >>>> that will decide which concrete substates need to be written. That way,
> >>>> no writer of load, save or reset functions that interact with in-kernel
> >>>> KVM states will ever have to worry about synchronization again. That
> >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> >>>> eliminated.
> >>>>
> >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >>>> continue to need it before reading or writing of VCPU states that are
> >>>> also tracked by in-kernel KVM subsystems.
> >>>>
> >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> >>>> are now redundant, just like remaining explicit register syncs.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Jan,
> >>>
> >>> This patch breaks system reset of WinXP.32 install (more easily
> >>> reproducible without iothread enabled).
> >>>
> >>> Screenshot attached.
> >>>
> >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> >> goes scrounging for some installation XP32 CD in the meantime...
> > 
> > No issues with qemu-kvm. Could not spot anything obvious.
> > 
> 
> And, of course, my WinXP installation did not trigger any reset issue,
> even in non-iothreaded mode. :(

Try kvm-autotest. I could not reproduce easily in hand run either.

This is not it, but still looks wrong:

In real mode emulation, kvm_get_sregs returns TR segment values of VMX
fields, while KVM caches them in vmx_vcpu->rmode.

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 14873b9..898173a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1822,13 +1822,23 @@ static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 static void vmx_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
 	u32 ar;
 
+	if (vmx->rmode.vm86_active && seg == VCPU_SREG_TR) {
+		var->base = vmx->rmode.tr.base;
+		var->limit = vmx->rmode.tr.limit;
+		var->selector = vmx->rmode.tr.selector;
+		ar = vmx->rmode.tr.ar;
+		goto ar;
+	}
+
 	var->base = vmcs_readl(sf->base);
 	var->limit = vmcs_read32(sf->limit);
 	var->selector = vmcs_read16(sf->selector);
 	ar = vmcs_read32(sf->ar_bytes);
+ar:
 	if ((ar & AR_UNUSABLE_MASK) && !emulate_invalid_guest_state)
 		ar = 0;
 	var->type = ar & 15;

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >>>> This grand cleanup drops all reset and vmsave/load related
> >>>> synchronization points in favor of four(!) generic hooks:
> >>>>
> >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> >>>>   (initial sync from kernel before vmsave)
> >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> >>>>   (writeback after vmload)
> >>>> - cpu_synchronize_all_post_init in main after machine init
> >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> >>>>   (writeback after system reset)
> >>>>
> >>>> These writeback points + the existing one of VCPU exec after
> >>>> cpu_synchronize_state map on three levels of writeback:
> >>>>
> >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>>>
> >>>> This level is passed to the arch-specific VCPU state writing function
> >>>> that will decide which concrete substates need to be written. That way,
> >>>> no writer of load, save or reset functions that interact with in-kernel
> >>>> KVM states will ever have to worry about synchronization again. That
> >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> >>>> eliminated.
> >>>>
> >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >>>> continue to need it before reading or writing of VCPU states that are
> >>>> also tracked by in-kernel KVM subsystems.
> >>>>
> >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> >>>> are now redundant, just like remaining explicit register syncs.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> Jan,
> >>>
> >>> This patch breaks system reset of WinXP.32 install (more easily
> >>> reproducible without iothread enabled).
> >>>
> >>> Screenshot attached.
> >>>
> >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> >> goes scrounging for some installation XP32 CD in the meantime...
> > 
> > No issues with qemu-kvm. Could not spot anything obvious.
> > 
> 
> And, of course, my WinXP installation did not trigger any reset issue,
> even in non-iothreaded mode. :(

Try kvm-autotest. I could not reproduce easily in hand run either.

This is not it, but still looks wrong:

In real mode emulation, kvm_get_sregs returns TR segment values of VMX
fields, while KVM caches them in vmx_vcpu->rmode.

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 14873b9..898173a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1822,13 +1822,23 @@ static u64 vmx_get_segment_base(struct kvm_vcpu *vcpu, int seg)
 static void vmx_get_segment(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg)
 {
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
 	u32 ar;
 
+	if (vmx->rmode.vm86_active && seg == VCPU_SREG_TR) {
+		var->base = vmx->rmode.tr.base;
+		var->limit = vmx->rmode.tr.limit;
+		var->selector = vmx->rmode.tr.selector;
+		ar = vmx->rmode.tr.ar;
+		goto ar;
+	}
+
 	var->base = vmcs_readl(sf->base);
 	var->limit = vmcs_read32(sf->limit);
 	var->selector = vmcs_read16(sf->selector);
 	ar = vmcs_read32(sf->ar_bytes);
+ar:
 	if ((ar & AR_UNUSABLE_MASK) && !emulate_invalid_guest_state)
 		ar = 0;
 	var->type = ar & 15;

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Tue, Mar 02, 2010 at 11:29:10PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> > > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> > >> Marcelo Tosatti wrote:
> > >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> > >>>> This grand cleanup drops all reset and vmsave/load related
> > >>>> synchronization points in favor of four(!) generic hooks:
> > >>>>
> > >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> > >>>>   (initial sync from kernel before vmsave)
> > >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> > >>>>   (writeback after vmload)
> > >>>> - cpu_synchronize_all_post_init in main after machine init
> > >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> > >>>>   (writeback after system reset)
> > >>>>
> > >>>> These writeback points + the existing one of VCPU exec after
> > >>>> cpu_synchronize_state map on three levels of writeback:
> > >>>>
> > >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> > >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> > >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> > >>>>
> > >>>> This level is passed to the arch-specific VCPU state writing function
> > >>>> that will decide which concrete substates need to be written. That way,
> > >>>> no writer of load, save or reset functions that interact with in-kernel
> > >>>> KVM states will ever have to worry about synchronization again. That
> > >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> > >>>> eliminated.
> > >>>>
> > >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> > >>>> continue to need it before reading or writing of VCPU states that are
> > >>>> also tracked by in-kernel KVM subsystems.
> > >>>>
> > >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> > >>>> are now redundant, just like remaining explicit register syncs.
> > >>>>
> > >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Jan,
> > >>>
> > >>> This patch breaks system reset of WinXP.32 install (more easily
> > >>> reproducible without iothread enabled).
> > >>>
> > >>> Screenshot attached.
> > >>>
> > >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> > >> goes scrounging for some installation XP32 CD in the meantime...
> > > 
> > > No issues with qemu-kvm. Could not spot anything obvious.
> > > 
> > 
> > And, of course, my WinXP installation did not trigger any reset issue,
> > even in non-iothreaded mode. :(

The regression seems to be caused by seabios commit d7e998f. Kevin, the
failure can be seen on the attached screenshot, which happens on the
first reboot of WinXP 32 installation (after copying files etc).


[-- Attachment #2: uqmaster-failure.png --]
[-- Type: image/png, Size: 4451 bytes --]

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Tue, Mar 02, 2010 at 11:29:10PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 02, 2010 at 05:31:09PM +0100, Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> > > On Tue, Mar 02, 2010 at 09:00:04AM +0100, Jan Kiszka wrote:
> > >> Marcelo Tosatti wrote:
> > >>> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> > >>>> This grand cleanup drops all reset and vmsave/load related
> > >>>> synchronization points in favor of four(!) generic hooks:
> > >>>>
> > >>>> - cpu_synchronize_all_states in qemu_savevm_state_complete
> > >>>>   (initial sync from kernel before vmsave)
> > >>>> - cpu_synchronize_all_post_init in qemu_loadvm_state
> > >>>>   (writeback after vmload)
> > >>>> - cpu_synchronize_all_post_init in main after machine init
> > >>>> - cpu_synchronize_all_post_reset in qemu_system_reset
> > >>>>   (writeback after system reset)
> > >>>>
> > >>>> These writeback points + the existing one of VCPU exec after
> > >>>> cpu_synchronize_state map on three levels of writeback:
> > >>>>
> > >>>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> > >>>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> > >>>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> > >>>>
> > >>>> This level is passed to the arch-specific VCPU state writing function
> > >>>> that will decide which concrete substates need to be written. That way,
> > >>>> no writer of load, save or reset functions that interact with in-kernel
> > >>>> KVM states will ever have to worry about synchronization again. That
> > >>>> also means that a lot of reasons for races, segfaults and deadlocks are
> > >>>> eliminated.
> > >>>>
> > >>>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
> > >>>> continue to need it before reading or writing of VCPU states that are
> > >>>> also tracked by in-kernel KVM subsystems.
> > >>>>
> > >>>> Consequently, this patch removes many cpu_synchronize_state calls that
> > >>>> are now redundant, just like remaining explicit register syncs.
> > >>>>
> > >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Jan,
> > >>>
> > >>> This patch breaks system reset of WinXP.32 install (more easily
> > >>> reproducible without iothread enabled).
> > >>>
> > >>> Screenshot attached.
> > >>>
> > >> Strange - no issues with qemu-kvm? Any special command line switch? /me
> > >> goes scrounging for some installation XP32 CD in the meantime...
> > > 
> > > No issues with qemu-kvm. Could not spot anything obvious.
> > > 
> > 
> > And, of course, my WinXP installation did not trigger any reset issue,
> > even in non-iothreaded mode. :(

The regression seems to be caused by seabios commit d7e998f. Kevin, the
failure can be seen on the attached screenshot, which happens on the
first reboot of WinXP 32 installation (after copying files etc).


[-- Attachment #2: uqmaster-failure.png --]
[-- Type: image/png, Size: 4451 bytes --]

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Kevin O'Connor]

On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> The regression seems to be caused by seabios commit d7e998f. Kevin, the
> failure can be seen on the attached screenshot, which happens on the
> first reboot of WinXP 32 installation (after copying files etc).

Sorry - I also noticed a bug in that commit recently.  I pushed the
fix I had in my local tree.

-Kevin

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Kevin O'Connor]

On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> The regression seems to be caused by seabios commit d7e998f. Kevin, the
> failure can be seen on the attached screenshot, which happens on the
> first reboot of WinXP 32 installation (after copying files etc).

Sorry - I also noticed a bug in that commit recently.  I pushed the
fix I had in my local tree.

-Kevin

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > failure can be seen on the attached screenshot, which happens on the
> > first reboot of WinXP 32 installation (after copying files etc).
> 
> Sorry - I also noticed a bug in that commit recently.  I pushed the
> fix I had in my local tree.

Thanks, it does fix the issue here. Anthony can you please update
seabios?

TIA

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > failure can be seen on the attached screenshot, which happens on the
> > first reboot of WinXP 32 installation (after copying files etc).
> 
> Sorry - I also noticed a bug in that commit recently.  I pushed the
> fix I had in my local tree.

Thanks, it does fix the issue here. Anthony can you please update
seabios?

TIA

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Kevin O'Connor]

On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > failure can be seen on the attached screenshot, which happens on the
> > > first reboot of WinXP 32 installation (after copying files etc).
> > 
> > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > fix I had in my local tree.
> 
> Thanks, it does fix the issue here. Anthony can you please update
> seabios?

Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
branch.  Is qemu ready to pull in bigger changes now?

-Kevin

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Kevin O'Connor]

On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > failure can be seen on the attached screenshot, which happens on the
> > > first reboot of WinXP 32 installation (after copying files etc).
> > 
> > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > fix I had in my local tree.
> 
> Thanks, it does fix the issue here. Anthony can you please update
> seabios?

Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
branch.  Is qemu ready to pull in bigger changes now?

-Kevin

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> > On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > > failure can be seen on the attached screenshot, which happens on the
> > > > first reboot of WinXP 32 installation (after copying files etc).
> > > 
> > > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > > fix I had in my local tree.
> > 
> > Thanks, it does fix the issue here. Anthony can you please update
> > seabios?
> 
> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
> branch.  Is qemu ready to pull in bigger changes now?

Anthony pulls in seabios master into qemu.git master periodically.

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
> > On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
> > > On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
> > > > The regression seems to be caused by seabios commit d7e998f. Kevin, the
> > > > failure can be seen on the attached screenshot, which happens on the
> > > > first reboot of WinXP 32 installation (after copying files etc).
> > > 
> > > Sorry - I also noticed a bug in that commit recently.  I pushed the
> > > fix I had in my local tree.
> > 
> > Thanks, it does fix the issue here. Anthony can you please update
> > seabios?
> 
> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
> branch.  Is qemu ready to pull in bigger changes now?

Anthony pulls in seabios master into qemu.git master periodically.

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Anthony Liguori]

On 03/08/2010 02:33 PM, Marcelo Tosatti wrote:
> On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
>    
>> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
>>      
>>> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
>>>        
>>>> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
>>>>          
>>>>> The regression seems to be caused by seabios commit d7e998f. Kevin, the
>>>>> failure can be seen on the attached screenshot, which happens on the
>>>>> first reboot of WinXP 32 installation (after copying files etc).
>>>>>            
>>>> Sorry - I also noticed a bug in that commit recently.  I pushed the
>>>> fix I had in my local tree.
>>>>          
>>> Thanks, it does fix the issue here. Anthony can you please update
>>> seabios?
>>>        
>> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
>> branch.  Is qemu ready to pull in bigger changes now?
>>      
> Anthony pulls in seabios master into qemu.git master periodically.
>    

We should be up to date now FWIW.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Anthony Liguori]

On 03/08/2010 02:33 PM, Marcelo Tosatti wrote:
> On Fri, Mar 05, 2010 at 09:37:26PM -0500, Kevin O'Connor wrote:
>    
>> On Thu, Mar 04, 2010 at 03:35:52PM -0300, Marcelo Tosatti wrote:
>>      
>>> On Thu, Mar 04, 2010 at 12:58:58AM -0500, Kevin O'Connor wrote:
>>>        
>>>> On Thu, Mar 04, 2010 at 01:21:12AM -0300, Marcelo Tosatti wrote:
>>>>          
>>>>> The regression seems to be caused by seabios commit d7e998f. Kevin, the
>>>>> failure can be seen on the attached screenshot, which happens on the
>>>>> first reboot of WinXP 32 installation (after copying files etc).
>>>>>            
>>>> Sorry - I also noticed a bug in that commit recently.  I pushed the
>>>> fix I had in my local tree.
>>>>          
>>> Thanks, it does fix the issue here. Anthony can you please update
>>> seabios?
>>>        
>> Neither commit d7e998f nor the fix 8f469b96 are on the SeaBIOS stable
>> branch.  Is qemu ready to pull in bigger changes now?
>>      
> Anthony pulls in seabios master into qemu.git master periodically.
>    

We should be up to date now FWIW.

Regards,

Anthony Liguori

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Avi Kivity]

On 03/02/2010 02:14 AM, Marcelo Tosatti wrote:
> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>    
>> This grand cleanup drops all reset and vmsave/load related
>> synchronization points in favor of four(!) generic hooks:
>>
>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>    (initial sync from kernel before vmsave)
>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>    (writeback after vmload)
>> - cpu_synchronize_all_post_init in main after machine init
>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>    (writeback after system reset)
>>
>> These writeback points + the existing one of VCPU exec after
>> cpu_synchronize_state map on three levels of writeback:
>>
>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>
>> This level is passed to the arch-specific VCPU state writing function
>> that will decide which concrete substates need to be written. That way,
>> no writer of load, save or reset functions that interact with in-kernel
>> KVM states will ever have to worry about synchronization again. That
>> also means that a lot of reasons for races, segfaults and deadlocks are
>> eliminated.
>>
>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>> continue to need it before reading or writing of VCPU states that are
>> also tracked by in-kernel KVM subsystems.
>>
>> Consequently, this patch removes many cpu_synchronize_state calls that
>> are now redundant, just like remaining explicit register syncs.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>      
> Jan,
>
> This patch breaks system reset of WinXP.32 install (more easily
> reproducible without iothread enabled).
>
>    

What's the conclusion here?  The patch is innocent of the regression?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Avi Kivity]

On 03/02/2010 02:14 AM, Marcelo Tosatti wrote:
> On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
>    
>> This grand cleanup drops all reset and vmsave/load related
>> synchronization points in favor of four(!) generic hooks:
>>
>> - cpu_synchronize_all_states in qemu_savevm_state_complete
>>    (initial sync from kernel before vmsave)
>> - cpu_synchronize_all_post_init in qemu_loadvm_state
>>    (writeback after vmload)
>> - cpu_synchronize_all_post_init in main after machine init
>> - cpu_synchronize_all_post_reset in qemu_system_reset
>>    (writeback after system reset)
>>
>> These writeback points + the existing one of VCPU exec after
>> cpu_synchronize_state map on three levels of writeback:
>>
>> - KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
>> - KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
>> - KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
>>
>> This level is passed to the arch-specific VCPU state writing function
>> that will decide which concrete substates need to be written. That way,
>> no writer of load, save or reset functions that interact with in-kernel
>> KVM states will ever have to worry about synchronization again. That
>> also means that a lot of reasons for races, segfaults and deadlocks are
>> eliminated.
>>
>> cpu_synchronize_state remains untouched, just as Anthony suggested. We
>> continue to need it before reading or writing of VCPU states that are
>> also tracked by in-kernel KVM subsystems.
>>
>> Consequently, this patch removes many cpu_synchronize_state calls that
>> are now redundant, just like remaining explicit register syncs.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>      
> Jan,
>
> This patch breaks system reset of WinXP.32 install (more easily
> reproducible without iothread enabled).
>
>    

What's the conclusion here?  The patch is innocent of the regression?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Thu, Mar 11, 2010 at 10:32:50AM +0200, Avi Kivity wrote:
> On 03/02/2010 02:14 AM, Marcelo Tosatti wrote:
> >On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >>This grand cleanup drops all reset and vmsave/load related
> >>synchronization points in favor of four(!) generic hooks:
> >>
> >>- cpu_synchronize_all_states in qemu_savevm_state_complete
> >>   (initial sync from kernel before vmsave)
> >>- cpu_synchronize_all_post_init in qemu_loadvm_state
> >>   (writeback after vmload)
> >>- cpu_synchronize_all_post_init in main after machine init
> >>- cpu_synchronize_all_post_reset in qemu_system_reset
> >>   (writeback after system reset)
> >>
> >>These writeback points + the existing one of VCPU exec after
> >>cpu_synchronize_state map on three levels of writeback:
> >>
> >>- KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >>- KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >>- KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>
> >>This level is passed to the arch-specific VCPU state writing function
> >>that will decide which concrete substates need to be written. That way,
> >>no writer of load, save or reset functions that interact with in-kernel
> >>KVM states will ever have to worry about synchronization again. That
> >>also means that a lot of reasons for races, segfaults and deadlocks are
> >>eliminated.
> >>
> >>cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >>continue to need it before reading or writing of VCPU states that are
> >>also tracked by in-kernel KVM subsystems.
> >>
> >>Consequently, this patch removes many cpu_synchronize_state calls that
> >>are now redundant, just like remaining explicit register syncs.
> >>
> >>Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >Jan,
> >
> >This patch breaks system reset of WinXP.32 install (more easily
> >reproducible without iothread enabled).
> >
> 
> What's the conclusion here?  The patch is innocent of the regression?

Yes, it is. The problem was caused by a recent seabios change, now
fixed.

[Qemu-devel] Re: [PATCH 2/4] KVM: Rework VCPU state writeback API

[Marcelo Tosatti]

On Thu, Mar 11, 2010 at 10:32:50AM +0200, Avi Kivity wrote:
> On 03/02/2010 02:14 AM, Marcelo Tosatti wrote:
> >On Mon, Mar 01, 2010 at 07:10:30PM +0100, Jan Kiszka wrote:
> >>This grand cleanup drops all reset and vmsave/load related
> >>synchronization points in favor of four(!) generic hooks:
> >>
> >>- cpu_synchronize_all_states in qemu_savevm_state_complete
> >>   (initial sync from kernel before vmsave)
> >>- cpu_synchronize_all_post_init in qemu_loadvm_state
> >>   (writeback after vmload)
> >>- cpu_synchronize_all_post_init in main after machine init
> >>- cpu_synchronize_all_post_reset in qemu_system_reset
> >>   (writeback after system reset)
> >>
> >>These writeback points + the existing one of VCPU exec after
> >>cpu_synchronize_state map on three levels of writeback:
> >>
> >>- KVM_PUT_RUNTIME_STATE (during runtime, other VCPUs continue to run)
> >>- KVM_PUT_RESET_STATE   (on synchronous system reset, all VCPUs stopped)
> >>- KVM_PUT_FULL_STATE    (on init or vmload, all VCPUs stopped as well)
> >>
> >>This level is passed to the arch-specific VCPU state writing function
> >>that will decide which concrete substates need to be written. That way,
> >>no writer of load, save or reset functions that interact with in-kernel
> >>KVM states will ever have to worry about synchronization again. That
> >>also means that a lot of reasons for races, segfaults and deadlocks are
> >>eliminated.
> >>
> >>cpu_synchronize_state remains untouched, just as Anthony suggested. We
> >>continue to need it before reading or writing of VCPU states that are
> >>also tracked by in-kernel KVM subsystems.
> >>
> >>Consequently, this patch removes many cpu_synchronize_state calls that
> >>are now redundant, just like remaining explicit register syncs.
> >>
> >>Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >Jan,
> >
> >This patch breaks system reset of WinXP.32 install (more easily
> >reproducible without iothread enabled).
> >
> 
> What's the conclusion here?  The patch is innocent of the regression?

Yes, it is. The problem was caused by a recent seabios change, now
fixed.

[PATCH 3/4] KVM: x86: Restrict writeback of VCPU state

[Jan Kiszka]

Do not write nmi_pending, sipi_vector, and mpstate unless we at least go
through a reset. And TSC as well as KVM wallclocks should only be
written on full sync, otherwise we risk to drop some time on state
read-modify-write.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/kvm.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index a4767b2..0ac4391 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -545,7 +545,7 @@ static void kvm_msr_entry_set(struct kvm_msr_entry *entry,
     entry->data = value;
 }
 
-static int kvm_put_msrs(CPUState *env)
+static int kvm_put_msrs(CPUState *env, int level)
 {
     struct {
         struct kvm_msrs info;
@@ -559,7 +559,6 @@ static int kvm_put_msrs(CPUState *env)
     kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip);
     if (kvm_has_msr_star(env))
 	kvm_msr_entry_set(&msrs[n++], MSR_STAR, env->star);
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
 #ifdef TARGET_X86_64
     /* FIXME if lm capable */
     kvm_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar);
@@ -567,8 +566,12 @@ static int kvm_put_msrs(CPUState *env)
     kvm_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask);
     kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
 #endif
-    kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,  env->system_time_msr);
-    kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK,  env->wall_clock_msr);
+    if (level == KVM_PUT_FULL_STATE) {
+        kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
+        kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,
+                          env->system_time_msr);
+        kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+    }
 
     msr_data.info.nmsrs = n;
 
@@ -781,7 +784,7 @@ static int kvm_get_mp_state(CPUState *env)
     return 0;
 }
 
-static int kvm_put_vcpu_events(CPUState *env)
+static int kvm_put_vcpu_events(CPUState *env, int level)
 {
 #ifdef KVM_CAP_VCPU_EVENTS
     struct kvm_vcpu_events events;
@@ -805,8 +808,11 @@ static int kvm_put_vcpu_events(CPUState *env)
 
     events.sipi_vector = env->sipi_vector;
 
-    events.flags =
-        KVM_VCPUEVENT_VALID_NMI_PENDING | KVM_VCPUEVENT_VALID_SIPI_VECTOR;
+    events.flags = 0;
+    if (level >= KVM_PUT_RESET_STATE) {
+        events.flags |=
+            KVM_VCPUEVENT_VALID_NMI_PENDING | KVM_VCPUEVENT_VALID_SIPI_VECTOR;
+    }
 
     return kvm_vcpu_ioctl(env, KVM_SET_VCPU_EVENTS, &events);
 #else
@@ -898,15 +904,17 @@ int kvm_arch_put_registers(CPUState *env, int level)
     if (ret < 0)
         return ret;
 
-    ret = kvm_put_msrs(env);
+    ret = kvm_put_msrs(env, level);
     if (ret < 0)
         return ret;
 
-    ret = kvm_put_mp_state(env);
-    if (ret < 0)
-        return ret;
+    if (level >= KVM_PUT_RESET_STATE) {
+        ret = kvm_put_mp_state(env);
+        if (ret < 0)
+            return ret;
+    }
 
-    ret = kvm_put_vcpu_events(env);
+    ret = kvm_put_vcpu_events(env, level);
     if (ret < 0)
         return ret;
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/4] KVM: x86: Restrict writeback of VCPU state

[Jan Kiszka]

Do not write nmi_pending, sipi_vector, and mpstate unless we at least go
through a reset. And TSC as well as KVM wallclocks should only be
written on full sync, otherwise we risk to drop some time on state
read-modify-write.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/kvm.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index a4767b2..0ac4391 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -545,7 +545,7 @@ static void kvm_msr_entry_set(struct kvm_msr_entry *entry,
     entry->data = value;
 }
 
-static int kvm_put_msrs(CPUState *env)
+static int kvm_put_msrs(CPUState *env, int level)
 {
     struct {
         struct kvm_msrs info;
@@ -559,7 +559,6 @@ static int kvm_put_msrs(CPUState *env)
     kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip);
     if (kvm_has_msr_star(env))
 	kvm_msr_entry_set(&msrs[n++], MSR_STAR, env->star);
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
 #ifdef TARGET_X86_64
     /* FIXME if lm capable */
     kvm_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar);
@@ -567,8 +566,12 @@ static int kvm_put_msrs(CPUState *env)
     kvm_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask);
     kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
 #endif
-    kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,  env->system_time_msr);
-    kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK,  env->wall_clock_msr);
+    if (level == KVM_PUT_FULL_STATE) {
+        kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
+        kvm_msr_entry_set(&msrs[n++], MSR_KVM_SYSTEM_TIME,
+                          env->system_time_msr);
+        kvm_msr_entry_set(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+    }
 
     msr_data.info.nmsrs = n;
 
@@ -781,7 +784,7 @@ static int kvm_get_mp_state(CPUState *env)
     return 0;
 }
 
-static int kvm_put_vcpu_events(CPUState *env)
+static int kvm_put_vcpu_events(CPUState *env, int level)
 {
 #ifdef KVM_CAP_VCPU_EVENTS
     struct kvm_vcpu_events events;
@@ -805,8 +808,11 @@ static int kvm_put_vcpu_events(CPUState *env)
 
     events.sipi_vector = env->sipi_vector;
 
-    events.flags =
-        KVM_VCPUEVENT_VALID_NMI_PENDING | KVM_VCPUEVENT_VALID_SIPI_VECTOR;
+    events.flags = 0;
+    if (level >= KVM_PUT_RESET_STATE) {
+        events.flags |=
+            KVM_VCPUEVENT_VALID_NMI_PENDING | KVM_VCPUEVENT_VALID_SIPI_VECTOR;
+    }
 
     return kvm_vcpu_ioctl(env, KVM_SET_VCPU_EVENTS, &events);
 #else
@@ -898,15 +904,17 @@ int kvm_arch_put_registers(CPUState *env, int level)
     if (ret < 0)
         return ret;
 
-    ret = kvm_put_msrs(env);
+    ret = kvm_put_msrs(env, level);
     if (ret < 0)
         return ret;
 
-    ret = kvm_put_mp_state(env);
-    if (ret < 0)
-        return ret;
+    if (level >= KVM_PUT_RESET_STATE) {
+        ret = kvm_put_mp_state(env);
+        if (ret < 0)
+            return ret;
+    }
 
-    ret = kvm_put_vcpu_events(env);
+    ret = kvm_put_vcpu_events(env, level);
     if (ret < 0)
         return ret;
 
-- 
1.6.0.2

[PATCH 4/4] x86: Extend validity of bsp_to_cpu

[Jan Kiszka]

As we hard-wire the BSP to CPU 0 anyway and cpuid_apic_id equals
cpu_index, bsp_to_cpu can also be based on the latter directly. This
will help an early user of it: KVM while initializing mp_state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index bdc297f..e50a488 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -760,7 +760,8 @@ static void pc_init_ne2k_isa(NICInfo *nd)
 
 int cpu_is_bsp(CPUState *env)
 {
-    return env->cpuid_apic_id == 0;
+    /* We hard-wire the BSP to the first CPU. */
+    return env->cpu_index == 0;
 }
 
 static CPUState *pc_new_cpu(const char *cpu_model)
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/4] x86: Extend validity of bsp_to_cpu

[Jan Kiszka]

As we hard-wire the BSP to CPU 0 anyway and cpuid_apic_id equals
cpu_index, bsp_to_cpu can also be based on the latter directly. This
will help an early user of it: KVM while initializing mp_state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index bdc297f..e50a488 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -760,7 +760,8 @@ static void pc_init_ne2k_isa(NICInfo *nd)
 
 int cpu_is_bsp(CPUState *env)
 {
-    return env->cpuid_apic_id == 0;
+    /* We hard-wire the BSP to the first CPU. */
+    return env->cpu_index == 0;
 }
 
 static CPUState *pc_new_cpu(const char *cpu_model)
-- 
1.6.0.2