Re: [PATCH 1/11] QEMU/KVM: Fix deadlocks in monitor and debugger

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Some monitor commands as well as the vm_stop() issued by the gdbstub on
>> external interruption so far deadlock on vcpu locks in the kernel. Patch
>> below resolves the issue by temporarily or permanently stopping all vcpu
>> threads before issuing the related KVM IOCTLs. It enables, e.g., to
>> break into guest code spinning in the vcpu and to use things like "info
>> cpus" in the monitor.
>>   
> 
> I implemented the alternative on_vcpu() approach for this (similar to
> smp_call_function_single in the kernel) which solves the livelock
> without resorting to stopping the VM.

I assume this pattern should then be applied to kvm_guest_debug (and
later on kvm_set_guest_debug) as well? You missed to fix that bug. 

Here is a quick patch to complete the work, will post rebased versions
of my remaining patches later today:
-----------

Use on_vcpu to call into kvm_guest_debug. Fix on_vpuc usage in
kvm_load_registers.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu/qemu-kvm.c |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

Index: b/qemu/qemu-kvm.c
===================================================================
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -187,7 +187,7 @@ static void kvm_do_load_registers(void *
 void kvm_load_registers(CPUState *env)
 {
     if (kvm_enabled())
-        on_vcpu(env->cpu_index, kvm_do_load_registers, env);
+        on_vcpu(env, kvm_do_load_registers, env);
 }
 
 static void kvm_do_save_registers(void *_env)
@@ -816,23 +816,37 @@ int kvm_qemu_init_env(CPUState *cenv)
     return kvm_arch_qemu_init_env(cenv);
 }
 
+struct kvm_guest_debug_data {
+    struct kvm_debug_guest dbg;
+    int err;
+};
+
+void kvm_invoke_guest_debug(void *data)
+{
+    struct kvm_guest_debug_data *dbg_data = data;
+
+    dbg_data->err = kvm_guest_debug(kvm_context, cpu_single_env->cpu_index,
+                                    &dbg_data->dbg);
+}
+
 int kvm_update_debugger(CPUState *env)
 {
-    struct kvm_debug_guest dbg;
+    struct kvm_guest_debug_data data;
     int i;
 
-    memset(dbg.breakpoints, 0, sizeof(dbg.breakpoints));
+    memset(data.dbg.breakpoints, 0, sizeof(data.dbg.breakpoints));
 
-    dbg.enabled = 0;
+    data.dbg.enabled = 0;
     if (env->nb_breakpoints || env->singlestep_enabled) {
-	dbg.enabled = 1;
+	data.dbg.enabled = 1;
 	for (i = 0; i < 4 && i < env->nb_breakpoints; ++i) {
-	    dbg.breakpoints[i].enabled = 1;
-	    dbg.breakpoints[i].address = env->breakpoints[i];
+	    data.dbg.breakpoints[i].enabled = 1;
+	    data.dbg.breakpoints[i].address = env->breakpoints[i];
 	}
-	dbg.singlestep = env->singlestep_enabled;
+	data.dbg.singlestep = env->singlestep_enabled;
     }
-    return kvm_guest_debug(kvm_context, env->cpu_index, &dbg);
+    on_vcpu(env, kvm_invoke_guest_debug, &data);
+    return data.err;
 }