call kvm_cpu_synchronize_state() on target vcpu

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

Signed-off-by Gleb Natapov <[email protected]>

qemu-kvm.c +16 -8

index 06efd41..9ab0cec 100644
@@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
 }
 #endif
 
-void kvm_cpu_synchronize_state(CPUState *env)
-{
-    if (!env->kvm_cpu_state.regs_modified) {
-        kvm_arch_get_registers(env);
-        env->kvm_cpu_state.regs_modified = 1;
-    }
-}
-
 static int handle_mmio(kvm_vcpu_context_t vcpu)
 {
     unsigned long addr = vcpu->run->mmio.phys_addr;

@@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
+static void do_kvm_cpu_synchronize_state(void *_env)
+{
+    CPUState *env = _env;
+    if (!env->kvm_cpu_state.regs_modified) {
+        kvm_arch_save_regs(env);
+        kvm_arch_load_mpstate(env);
+        env->kvm_cpu_state.regs_modified = 1;
+    }
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+    if (!env->kvm_cpu_state.regs_modified)
+        on_vcpu(env, do_kvm_cpu_synchronize_state, env);
+}
+
 static void inject_interrupt(void *data)
 {
     cpu_interrupt(current_env, (long) data);

qemu-kvm.h +0 -6

index 2c1730b..32f74b3 100644
@@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
     return 0;
 }
 
-static inline void kvm_arch_get_registers(CPUState *env)
-{
-    kvm_save_registers(env);
-    kvm_save_mpstate(env);
-}
-
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);

target-i386/helper.c +1 -2

index 4a16887..57c74a2 100644
@@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
     static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
 
     if (kvm_enabled())
-        kvm_arch_get_registers(env);
+        kvm_cpu_synchronize_state(env);
 
     eflags = env->eflags;
 #ifdef TARGET_X86_64
--

Gleb.

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

We need this upstream too, right? Could you file the corresponding patch?

Thanks,
Jan

Quoted 68 lines

Signed-off-by Gleb Natapov <[email protected]>

qemu-kvm.c +16 -8

index 06efd41..9ab0cec 100644
@@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
 }
 #endif
 
-void kvm_cpu_synchronize_state(CPUState *env)
-{
-    if (!env->kvm_cpu_state.regs_modified) {
-        kvm_arch_get_registers(env);
-        env->kvm_cpu_state.regs_modified = 1;
-    }
-}
-
 static int handle_mmio(kvm_vcpu_context_t vcpu)
 {
     unsigned long addr = vcpu->run->mmio.phys_addr;

@@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
+static void do_kvm_cpu_synchronize_state(void *_env)
+{
+    CPUState *env = _env;
+    if (!env->kvm_cpu_state.regs_modified) {
+        kvm_arch_save_regs(env);
+        kvm_arch_load_mpstate(env);
+        env->kvm_cpu_state.regs_modified = 1;
+    }
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+    if (!env->kvm_cpu_state.regs_modified)
+        on_vcpu(env, do_kvm_cpu_synchronize_state, env);
+}
+
 static void inject_interrupt(void *data)
 {
     cpu_interrupt(current_env, (long) data);

qemu-kvm.h +0 -6

index 2c1730b..32f74b3 100644
@@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
     return 0;
 }
 
-static inline void kvm_arch_get_registers(CPUState *env)
-{
-    kvm_save_registers(env);
-    kvm_save_mpstate(env);
-}
-
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);

target-i386/helper.c +0 -0

index 4a16887..57c74a2 100644
@@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
     static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
 

Quoted 8 lines

target-i386/helper.c +1 -2

     if (kvm_enabled())
-        kvm_arch_get_registers(env);
+        kvm_cpu_synchronize_state(env);
 
     eflags = env->eflags;
 #ifdef TARGET_X86_64
--

Gleb.

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Quoted 8 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

We need this upstream too, right? Could you file the corresponding patch?

Upstream is single threaded. It shouldn't suffer from this bug.

Quoted 71 lines

Thanks,
Jan

Quoted 67 lines

Signed-off-by Gleb Natapov <[email protected]>

qemu-kvm.c +16 -8

index 06efd41..9ab0cec 100644
@@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
 }
 #endif
 
-void kvm_cpu_synchronize_state(CPUState *env)
-{
-    if (!env->kvm_cpu_state.regs_modified) {
-        kvm_arch_get_registers(env);
-        env->kvm_cpu_state.regs_modified = 1;
-    }
-}
-
 static int handle_mmio(kvm_vcpu_context_t vcpu)
 {
     unsigned long addr = vcpu->run->mmio.phys_addr;

@@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
+static void do_kvm_cpu_synchronize_state(void *_env)
+{
+    CPUState *env = _env;
+    if (!env->kvm_cpu_state.regs_modified) {
+        kvm_arch_save_regs(env);
+        kvm_arch_load_mpstate(env);
+        env->kvm_cpu_state.regs_modified = 1;
+    }
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+    if (!env->kvm_cpu_state.regs_modified)
+        on_vcpu(env, do_kvm_cpu_synchronize_state, env);
+}
+
 static void inject_interrupt(void *data)
 {
     cpu_interrupt(current_env, (long) data);

qemu-kvm.h +0 -6

index 2c1730b..32f74b3 100644
@@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
     return 0;
 }
 
-static inline void kvm_arch_get_registers(CPUState *env)
-{
-    kvm_save_registers(env);
-    kvm_save_mpstate(env);
-}
-
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);

target-i386/helper.c +0 -0

index 4a16887..57c74a2 100644
@@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
     static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
 

Quoted 11 lines

Quoted 8 lines

target-i386/helper.c +1 -2

     if (kvm_enabled())
-        kvm_arch_get_registers(env);
+        kvm_cpu_synchronize_state(env);
 
     eflags = env->eflags;
 #ifdef TARGET_X86_64
--

Gleb.

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

--
Gleb.

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

Quoted 3 lines

Quoted 2 lines

We need this upstream too, right? Could you file the corresponding patch?

Upstream is single threaded. It shouldn't suffer from this bug.

Not if you enable iothread support (though I don't remember if that

works now for kvm) + you are also touching shared code here. So qemu-kvm would benefit from keeping the diff small.

Jan

Quoted 83 lines

Quoted 82 lines

Thanks,
Jan

Quoted 75 lines

Signed-off-by Gleb Natapov <[email protected]>

qemu-kvm.c +16 -8

index 06efd41..9ab0cec 100644
@@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
 }
 #endif
 
-void kvm_cpu_synchronize_state(CPUState *env)
-{
-    if (!env->kvm_cpu_state.regs_modified) {
-        kvm_arch_get_registers(env);
-        env->kvm_cpu_state.regs_modified = 1;
-    }
-}
-
 static int handle_mmio(kvm_vcpu_context_t vcpu)
 {
     unsigned long addr = vcpu->run->mmio.phys_addr;

@@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
+static void do_kvm_cpu_synchronize_state(void *_env)
+{
+    CPUState *env = _env;
+    if (!env->kvm_cpu_state.regs_modified) {
+        kvm_arch_save_regs(env);
+        kvm_arch_load_mpstate(env);
+        env->kvm_cpu_state.regs_modified = 1;
+    }
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+    if (!env->kvm_cpu_state.regs_modified)
+        on_vcpu(env, do_kvm_cpu_synchronize_state, env);
+}
+
 static void inject_interrupt(void *data)
 {
     cpu_interrupt(current_env, (long) data);

qemu-kvm.h +0 -6

index 2c1730b..32f74b3 100644
@@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
     return 0;
 }
 
-static inline void kvm_arch_get_registers(CPUState *env)
-{
-    kvm_save_registers(env);
-    kvm_save_mpstate(env);
-}
-
 static inline void kvm_arch_put_registers(CPUState *env)
 {
     kvm_load_registers(env);

target-i386/helper.c +1 -2

index 4a16887..57c74a2 100644
@@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
     static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
 
     if (kvm_enabled())
-        kvm_arch_get_registers(env);
+        kvm_cpu_synchronize_state(env);
 
     eflags = env->eflags;
 #ifdef TARGET_X86_64
--

Gleb.

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Quoted 2 lines

--
Gleb.

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Quoted 8 lines · Jan Kiszka

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

We need this upstream too, right? Could you file the corresponding patch?

Upstream is single threaded. It shouldn't suffer from this bug.

Not if you enable iothread support (though I don't remember if that

It can't work with kvm since all vcpu ioctls are called on the thread that issues them.

Quoted 4 lines

works now for kvm) + you are also touching shared code here. So qemu-kvm would benefit from keeping the diff small.

The patch doesn't touch shared code. (it is almost impossible to tell what code is shared and what's not nowadays)

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

We need this upstream too, right? Could you file the corresponding patch?

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

Upstream is single threaded. It shouldn't suffer from this bug.

Not if you enable iothread support (though I don't remember if that

It can't work with kvm since all vcpu ioctls are called on the thread that issues them.

Yeah, I just recalled all that on_vcpu fuzz and that upstream is still horribly broken /wrt iothread+kvm. But once that is fixed, we also need this fix here.

Quoted 6 lines

Quoted 4 lines

works now for kvm) + you are also touching shared code here. So qemu-kvm would benefit from keeping the diff small.

The patch doesn't touch shared code. (it is almost impossible to tell what code is shared and what's not nowadays)

cpu_dump_state() is definitely shared.

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

regs_modified logic doesn't work if io thread calls kvm_cpu_synchronize_state() since kvm_arch_get_registers() returns only after vcpu thread is back to kernel. Setting regs_modified to 1 at this stage causes loading of wrong vcpu state on the next vcpu_run().

We need this upstream too, right? Could you file the corresponding patch?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

Upstream is single threaded. It shouldn't suffer from this bug.

Not if you enable iothread support (though I don't remember if that

It can't work with kvm since all vcpu ioctls are called on the thread that issues them.

Yeah, I just recalled all that on_vcpu fuzz and that upstream is still horribly broken /wrt iothread+kvm. But once that is fixed, we also need this fix here.

This will be done as part of transition to on_vcpu() for vcpu ioctls.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

works now for kvm) + you are also touching shared code here. So qemu-kvm would benefit from keeping the diff small.

Quoted 4 lines

Quoted 2 lines

The patch doesn't touch shared code. (it is almost impossible to tell what code is shared and what's not nowadays)

cpu_dump_state() is definitely shared.

Ah this one line. Yes it is. But I have not good commit message for this one liner change for upstream :)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The patch doesn't touch shared code. (it is almost impossible to tell what code is shared and what's not nowadays)

cpu_dump_state() is definitely shared.

Ah this one line. Yes it is. But I have not good commit message for this one liner change for upstream :)

That's why I suggested to post the corresponding change also for upstream. Even if it doesn't need it now, it will one day. :)