Patchwork [2/9] qemu-kvm: Clean up register access API

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 19, 2010, 6:22 p.m.
Message ID <97ef7408c86f7984db721c38cd736b98b81efad9.1266603744.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/45875/
State New
Headers show

Comments

Jan Kiszka - Feb. 19, 2010, 6:22 p.m.
qemu-kvm's functions for accessing the VCPU registers are
kvm_arch_load/save_regs. Use them directly instead of going through
various wrappers. Specifically, we do not need on_vcpu wrapping as all
users either already run in the related thread or call while the vm is
stopped. This is now also validated during runtime via asserts.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-kvm-x86.c        |    4 ++++
 qemu-kvm.c            |   46 ++++++++--------------------------------------
 qemu-kvm.h            |   14 +++-----------
 target-ia64/machine.c |    4 ++--
 4 files changed, 17 insertions(+), 51 deletions(-)
Avi Kivity - Feb. 21, 2010, 9:25 a.m.
On 02/19/2010 08:22 PM, Jan Kiszka wrote:
> qemu-kvm's functions for accessing the VCPU registers are
> kvm_arch_load/save_regs. Use them directly instead of going through
> various wrappers. Specifically, we do not need on_vcpu wrapping as all
> users either already run in the related thread or call while the vm is
> stopped. This is now also validated during runtime via asserts.
>
>
> +    assert(kvm_cpu_is_stopped(env) || env->thread_id == kvm_get_thread_id());
>    

The kvm_cpu_is_stopped() part is wrong, for two reasons.  First, a 
future ABI revolution might switch to syscalls and thus make the 
association between vcpu and thread implicit.  This will allow us to 
drop vcpu->mutex, eventually.  Second, kvm_cpu_is_stopped() will be racy 
in a threaded future version of qemu.  All vcpu ioctls should be made 
from the vcpu thread.

Documentation/kvm/api.txt has this to say about the matter:

>  - vcpu ioctls: These query and set attributes that control the operation
>    of a single virtual cpu.
>
>    Only run vcpu ioctls from the same thread that was used to create the
>    vcpu.

So it isn't just me.

Patch

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 7f820a4..4cb1cb3 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -911,6 +911,8 @@  void kvm_arch_load_regs(CPUState *env)
     struct kvm_msr_entry msrs[100];
     int rc, n, i;
 
+    assert(kvm_cpu_is_stopped(env) || env->thread_id == kvm_get_thread_id());
+
     regs.rax = env->regs[R_EAX];
     regs.rbx = env->regs[R_EBX];
     regs.rcx = env->regs[R_ECX];
@@ -1072,6 +1074,8 @@  void kvm_arch_save_regs(CPUState *env)
     uint32_t hflags;
     uint32_t i, n, rc, bit;
 
+    assert(kvm_cpu_is_stopped(env) || env->thread_id == kvm_get_thread_id());
+
     kvm_get_regs(env, &regs);
 
     env->regs[R_EAX] = regs.rax;
diff --git a/qemu-kvm.c b/qemu-kvm.c
index b534d4e..555d959 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -871,7 +871,7 @@  int pre_kvm_run(kvm_context_t kvm, CPUState *env)
     kvm_arch_pre_run(env, env->kvm_run);
 
     if (env->kvm_vcpu_dirty) {
-        kvm_arch_put_registers(env);
+        kvm_arch_load_regs(env);
         env->kvm_vcpu_dirty = 0;
     }
 
@@ -1438,7 +1438,7 @@  int kvm_irqfd(kvm_context_t kvm, int gsi, int flags)
 }
 
 #endif                          /* KVM_CAP_IRQFD */
-static inline unsigned long kvm_get_thread_id(void)
+unsigned long kvm_get_thread_id(void)
 {
     return syscall(SYS_gettid);
 }
@@ -1541,16 +1541,12 @@  static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
-void kvm_arch_get_registers(CPUState *env)
-{
-	kvm_arch_save_regs(env);
-}
-
 static void do_kvm_cpu_synchronize_state(void *_env)
 {
     CPUState *env = _env;
+
     if (!env->kvm_vcpu_dirty) {
-        kvm_arch_get_registers(env);
+        kvm_arch_save_regs(env);
         env->kvm_vcpu_dirty = 1;
     }
 }
@@ -1593,32 +1589,6 @@  void kvm_update_interrupt_request(CPUState *env)
     }
 }
 
-static void kvm_do_load_registers(void *_env)
-{
-    CPUState *env = _env;
-
-    kvm_arch_load_regs(env);
-}
-
-void kvm_load_registers(CPUState *env)
-{
-    if (kvm_enabled() && qemu_system_ready)
-        on_vcpu(env, kvm_do_load_registers, env);
-}
-
-static void kvm_do_save_registers(void *_env)
-{
-    CPUState *env = _env;
-
-    kvm_arch_save_regs(env);
-}
-
-void kvm_save_registers(CPUState *env)
-{
-    if (kvm_enabled())
-        on_vcpu(env, kvm_do_save_registers, env);
-}
-
 static void kvm_do_load_mpstate(void *_env)
 {
     CPUState *env = _env;
@@ -1662,7 +1632,7 @@  int kvm_cpu_exec(CPUState *env)
     return 0;
 }
 
-static int is_cpu_stopped(CPUState *env)
+int kvm_cpu_is_stopped(CPUState *env)
 {
     return !vm_running || env->stopped;
 }
@@ -1889,7 +1859,7 @@  static void process_irqchip_events(CPUState *env)
 static int kvm_main_loop_cpu(CPUState *env)
 {
     while (1) {
-        int run_cpu = !is_cpu_stopped(env);
+        int run_cpu = !kvm_cpu_is_stopped(env);
         if (run_cpu && !kvm_irqchip_in_kernel()) {
             process_irqchip_events(env);
             run_cpu = !env->halted;
@@ -2372,8 +2342,8 @@  static void kvm_invoke_set_guest_debug(void *data)
     struct kvm_set_guest_debug_data *dbg_data = data;
 
     if (cpu_single_env->kvm_vcpu_dirty) {
-        kvm_arch_put_registers(cpu_single_env);
-        cpu_single_env->kvm_vcpu_dirty = 1;
+        kvm_arch_save_regs(cpu_single_env);
+        cpu_single_env->kvm_vcpu_dirty = 0;
     }
     dbg_data->err =
         kvm_set_guest_debug(cpu_single_env,
diff --git a/qemu-kvm.h b/qemu-kvm.h
index c1675d4..9c132da 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -902,8 +902,6 @@  int kvm_main_loop(void);
 int kvm_init_ap(void);
 #ifndef QEMU_KVM_NO_CPU
 int kvm_vcpu_inited(CPUState *env);
-void kvm_load_registers(CPUState *env);
-void kvm_save_registers(CPUState *env);
 void kvm_load_mpstate(CPUState *env);
 void kvm_save_mpstate(CPUState *env);
 int kvm_cpu_exec(CPUState *env);
@@ -1069,8 +1067,6 @@  void kvm_load_tsc(CPUState *env);
 #ifdef TARGET_I386
 #define qemu_kvm_has_pit_state2() (0)
 #endif
-#define kvm_load_registers(env) do {} while(0)
-#define kvm_save_registers(env) do {} while(0)
 #define kvm_save_mpstate(env)   do {} while(0)
 #define qemu_kvm_cpu_stop(env) do {} while(0)
 static inline void kvm_init_vcpu(CPUState *env)
@@ -1099,13 +1095,6 @@  static inline int kvm_sync_vcpus(void)
 }
 
 #ifndef QEMU_KVM_NO_CPU
-void kvm_arch_get_registers(CPUState *env);
-
-static inline void kvm_arch_put_registers(CPUState *env)
-{
-    kvm_load_registers(env);
-}
-
 void kvm_cpu_synchronize_state(CPUState *env);
 
 static inline void cpu_synchronize_state(CPUState *env)
@@ -1168,6 +1157,9 @@  int kvm_check_extension(KVMState *s, unsigned int ext);
 
 int kvm_tpr_enable_vapic(CPUState *env);
 
+unsigned long kvm_get_thread_id(void);
+int kvm_cpu_is_stopped(CPUState *env);
+
 #endif
 
 #endif
diff --git a/target-ia64/machine.c b/target-ia64/machine.c
index 70ef379..7d29575 100644
--- a/target-ia64/machine.c
+++ b/target-ia64/machine.c
@@ -9,7 +9,7 @@  void cpu_save(QEMUFile *f, void *opaque)
     CPUState *env = opaque;
 
     if (kvm_enabled()) {
-        kvm_save_registers(env);
+        kvm_arch_save_regs(env);
         kvm_arch_save_mpstate(env);
     }
 }
@@ -19,7 +19,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     CPUState *env = opaque;
 
     if (kvm_enabled()) {
-        kvm_load_registers(env);
+        kvm_arch_load_regs(env);
         kvm_arch_load_mpstate(env);
     }
     return 0;