Patch queue, part II

PATCH In review Jan Kiszka 4 participants 39 messages Feb 1, 2011 – Mar 1, 2011

Subsystems (2)

KVM Kbuild

lore

Share & tools

lore

Jan Kiszka

author

2011-02-01 21:17 UTC #0 lore raw

v2 cover Patch queue, part II

Version 2 of part II. Changes:
- Fixed "Unconditionally reenter kernel after IO exits" to take self-INIT into account
- Fixed misplaced hunk in "Fix race between timer signals and vcpu entry under !IOTHREAD" (rebase artifact)
- Factor out block_synchronous_signals (analogue to block_io_signals)
- Additional fix to break out of SMP VCPU loop on pending IO event
- Fork qemu_kvm_init_cpu_signals over CONFIG_IOTHREAD
- Additional cleanup, flattening the main loop

Hope I addressed all review comments (except for passing env to qemu_cpu_kick_self which I think is better as it is).

Thanks,
Jan

CC Alexander Graf <[email protected]>

CC Gleb Natapov <[email protected]>

CC Hidetoshi Seto <[email protected]>

CC Huang Ying <[email protected]>

CC Jin Dongming <[email protected]>

CC Paolo Bonzini <[email protected]>

CC Stefan Hajnoczi <[email protected]>

Jan Kiszka (24):
kvm: x86: Fix build in absence of KVM_CAP_ASYNC_PF Prevent abortion on multiple VCPU kicks
Stop current VCPU on synchronous reset requests
Process vmstop requests in IO thread
Trigger exit from cpu_exec_all on pending IO events Leave inner main_loop faster on pending requests Flatten the main loop
kvm: Report proper error on GET_VCPU_MMAP_SIZE failures
kvm: Drop redundant kvm_enabled from kvm_cpu_thread_fn
kvm: Handle kvm_init_vcpu errors
kvm: Provide sigbus services arch-independently Refactor signal setup functions in cpus.c
kvm: Set up signal mask also for !CONFIG_IOTHREAD
kvm: Refactor qemu_kvm_eat_signals
kvm: Call qemu_kvm_eat_signals also under !CONFIG_IOTHREAD Set up signalfd under !CONFIG_IOTHREAD
kvm: Fix race between timer signals and vcpu entry under !IOTHREAD
kvm: Add MCE signal support for !CONFIG_IOTHREAD Introduce VCPU self-signaling service
kvm: Unconditionally reenter kernel after IO exits
kvm: Remove static return code of kvm_handle_io
kvm: Leave kvm_cpu_exec directly after KVM_EXIT_SHUTDOWN Refactor kvm&tcg function names in cpus.c
Fix a few coding style violations in cpus.c

 Makefile.objs      |    2 +-
 configure          |    6 +
 cpu-defs.h         |    1 +
 cpus.c             |  662 ++++++++++++++++++++++++++++++++--------------------
 cpus.h             |    1 +
 kvm-all.c          |   60 +++--
 kvm-stub.c         |    5 +
 kvm.h              |    7 +-
 qemu-common.h      |    1 +
 target-i386/kvm.c  |   11 +-
 target-ppc/kvm.c   |   10 +
 target-s390x/kvm.c |   10 +
 vl.c               |   40 ++--
 13 files changed, 514 insertions(+), 302 deletions(-)

Jan Kiszka

author

2011-02-01 21:17 UTC #1 lore raw

PATCH v2 1/24 kvm: x86: Fix build in absence of KVM_CAP_ASYNC_PF

From: Jan Kiszka <[email protected]>

Reported by Stefan Hajnoczi.

Signed-off-by Jan Kiszka <[email protected]>

---
 target-i386/kvm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

target-i386/kvm.c +2 -0

index 8e8880a..05010bb 100644
@@ -167,7 +167,9 @@ static int get_para_features(CPUState *env)
             features |= (1 << para_features[i].feature);
         }
     }
+#ifdef KVM_CAP_ASYNC_PF
     has_msr_async_pf_en = features & (1 << KVM_FEATURE_ASYNC_PF);
+#endif
     return features;
 }
 #endif

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #2 lore raw

PATCH v2 2/24 Prevent abortion on multiple VCPU kicks

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

If we call qemu_cpu_kick more than once before the target was able to process the signal, pthread_kill will fail, and qemu will abort. Prevent this by avoiding the redundant signal.

This logic can be found in qemu-kvm as well.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    1 +
 cpus.c     |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

cpu-defs.h +1 -0

index 8d4bf86..db809ed 100644
@@ -205,6 +205,7 @@ typedef struct CPUWatchpoint {
     uint32_t stopped; /* Artificially stopped */                        \
     struct QemuThread *thread;                                          \
     struct QemuCond *halt_cond;                                         \
+    int thread_kicked;                                                  \
     struct qemu_work_item *queued_work_first, *queued_work_last;        \
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \

cpus.c +5 -1

index 4c9928e..ab6e40e 100644
@@ -481,6 +481,7 @@ static void qemu_wait_io_event_common(CPUState *env)
         qemu_cond_signal(&qemu_pause_cond);
     }
     flush_queued_work(env);
+    env->thread_kicked = false;
 }
 
 static void qemu_tcg_wait_io_event(void)

@@ -648,7 +649,10 @@ void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
     qemu_cond_broadcast(env->halt_cond);
-    qemu_thread_signal(env->thread, SIG_IPI);
+    if (!env->thread_kicked) {
+        qemu_thread_signal(env->thread, SIG_IPI);
+        env->thread_kicked = true;
+    }
 }
 
 int qemu_cpu_self(void *_env)

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #3 lore raw

PATCH v2 3/24 Stop current VCPU on synchronous reset requests

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

If some I/O operation ends up calling qemu_system_reset_request in VCPU context, we record this and inform the io-thread, but we do not terminate the VCPU loop. This can lead to fairly unexpected behavior if the triggering reset operation is supposed to work synchronously.

Fix this for TCG (when run in deterministic I/O mode) by setting the VCPU on stop and issuing a cpu_exit. KVM requires some more work on its VCPU loop.

[ ported from qemu-kvm ]

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   13 +++++++++----
 cpus.h |    1 +
 vl.c   |    1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

cpus.c +9 -4

index ab6e40e..ceb3a83 100644
@@ -99,6 +99,14 @@ void cpu_synchronize_all_post_init(void)
     }
 }
 
+void cpu_stop_current(void)
+{
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
+}
+
 int cpu_is_stopped(CPUState *env)
 {
     return !vm_running || env->stopped;

@@ -863,10 +871,7 @@ void vm_stop(int reason)
          * FIXME: should not return to device code in case
          * vm_stop() has been requested.
          */
-        if (cpu_single_env) {
-            cpu_exit(cpu_single_env);
-            cpu_single_env->stop = 1;
-        }
+        cpu_stop_current();
         return;
     }
     do_vm_stop(reason);

cpus.h +1 -0

index bf4d9bb..4cadb64 100644
@@ -6,6 +6,7 @@ int qemu_init_main_loop(void);
 void qemu_main_loop_start(void);
 void resume_all_vcpus(void);
 void pause_all_vcpus(void);
+void cpu_stop_current(void);
 
 /* vl.c */
 extern int smp_cores;

vl.c +1 -0

index 33f844f..db24a05 100644
@@ -1278,6 +1278,7 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    cpu_stop_current();
     qemu_notify_event();
 }

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #4 lore raw

PATCH v2 4/24 Process vmstop requests in IO thread

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

A pending vmstop request is also a reason to leave the inner main loop. So far we ignored it, and pending stop requests issued over VCPU threads were simply ignored.

Signed-off-by Jan Kiszka <[email protected]>

---
 vl.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

vl.c +5 -9

index db24a05..5fad700 100644
@@ -1373,15 +1373,11 @@ void main_loop_wait(int nonblocking)
 
 static int vm_can_run(void)
 {
-    if (powerdown_requested)
-        return 0;
-    if (reset_requested)
-        return 0;
-    if (shutdown_requested)
-        return 0;
-    if (debug_requested)
-        return 0;
-    return 1;
+    return !(powerdown_requested ||
+             reset_requested ||
+             shutdown_requested ||
+             debug_requested ||
+             vmstop_requested);
 }
 
 qemu_irq qemu_system_powerdown;

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #5 lore raw

PATCH v2 5/24 Trigger exit from cpu_exec_all on pending IO events

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Except for timer events, we currently do not leave the loop over all VCPUs if an IO event was filed. That may cause unexpected IO latencies under !CONFIG_IOTHREAD in SMP scenarios. Fix it by setting the global exit_request which breaks the loop.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

cpus.c +1 -0

index ceb3a83..5dfc54e 100644
@@ -315,6 +315,7 @@ void qemu_notify_event(void)
     if (next_cpu && env != next_cpu) {
         cpu_exit(next_cpu);
     }
+    exit_request = 1;
 }
 
 void qemu_mutex_lock_iothread(void) {}

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #6 lore raw

PATCH v2 6/24 Leave inner main_loop faster on pending requests

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

If there is any pending request that requires us to leave the inner loop if main_loop, makes sure we do this as soon as possible by enforcing non-blocking IO processing.

At this change, move variable definitions out of the inner loop to improve readability.

Signed-off-by Jan Kiszka <[email protected]>

---
 vl.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

vl.c +7 -4

index 5fad700..2ebc55b 100644
@@ -1384,18 +1384,21 @@ qemu_irq qemu_system_powerdown;
 
 static void main_loop(void)
 {
+    bool nonblocking = false;
+#ifdef CONFIG_PROFILER
+    int64_t ti;
+#endif
     int r;
 
     qemu_main_loop_start();
 
     for (;;) {
         do {
-            bool nonblocking = false;
-#ifdef CONFIG_PROFILER
-            int64_t ti;
-#endif
 #ifndef CONFIG_IOTHREAD
             nonblocking = cpu_exec_all();
+            if (!vm_can_run()) {
+                nonblocking = true;
+            }
 #endif
 #ifdef CONFIG_PROFILER
             ti = profile_getclock();

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #7 lore raw

PATCH v2 7/24 Flatten the main loop

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

First of all, vm_can_run is a misnomer, it actually means "no request pending". Moreover, there is no need to check all pending requests twice, the first time via the inner loop check and then again when actually processing the requests. We can simply remove the inner loop and do the checks directly.

Signed-off-by Jan Kiszka <[email protected]>

---
 vl.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

vl.c +15 ~~-15~~

index 2ebc55b..f5dec09 100644
@@ -1371,14 +1371,16 @@ void main_loop_wait(int nonblocking)
 
 }
 
-static int vm_can_run(void)
+#ifndef CONFIG_IOTHREAD
+static int vm_request_pending(void)
 {
-    return !(powerdown_requested ||
-             reset_requested ||
-             shutdown_requested ||
-             debug_requested ||
-             vmstop_requested);
+    return powerdown_requested ||
+           reset_requested ||
+           shutdown_requested ||
+           debug_requested ||
+           vmstop_requested;
 }
+#endif
 
 qemu_irq qemu_system_powerdown;

@@ -1393,21 +1395,19 @@ static void main_loop(void)
     qemu_main_loop_start();
 
     for (;;) {
-        do {
 #ifndef CONFIG_IOTHREAD
-            nonblocking = cpu_exec_all();
-            if (!vm_can_run()) {
-                nonblocking = true;
-            }
+        nonblocking = cpu_exec_all();
+        if (vm_request_pending()) {
+            nonblocking = true;
+        }
 #endif
 #ifdef CONFIG_PROFILER
-            ti = profile_getclock();
+        ti = profile_getclock();
 #endif
-            main_loop_wait(nonblocking);
+        main_loop_wait(nonblocking);
 #ifdef CONFIG_PROFILER
-            dev_time += profile_getclock() - ti;
+        dev_time += profile_getclock() - ti;
 #endif
-        } while (vm_can_run());
 
         if ((r = qemu_debug_requested())) {
             vm_stop(r);

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #8 lore raw

PATCH v2 8/24 kvm: Report proper error on GET_VCPU_MMAP_SIZE failures

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 kvm-all.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

kvm-all.c +1 -0

index 9976762..1a55a10 100644
@@ -219,6 +219,7 @@ int kvm_init_vcpu(CPUState *env)
 
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
+        ret = mmap_size;
         DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
         goto err;
     }

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #9 lore raw

PATCH v2 9/24 kvm: Drop redundant kvm_enabled from kvm_cpu_thread_fn

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

cpus.c +2 -2

index 5dfc54e..312c7a2 100644
@@ -607,8 +607,8 @@ static void *kvm_cpu_thread_fn(void *arg)
 
     qemu_mutex_lock(&qemu_global_mutex);
     qemu_thread_self(env->thread);
-    if (kvm_enabled())
-        kvm_init_vcpu(env);
+
+    kvm_init_vcpu(env);
 
     kvm_init_ipi(env);

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #10 lore raw

PATCH v2 10/24 kvm: Handle kvm_init_vcpu errors

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Do not ignore errors of kvm_init_vcpu, they are fatal.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

cpus.c +15 -4

index 312c7a2..8475757 100644
@@ -273,12 +273,18 @@ void qemu_main_loop_start(void)
 void qemu_init_vcpu(void *_env)
 {
     CPUState *env = _env;
+    int r;
 
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
-    if (kvm_enabled())
-        kvm_init_vcpu(env);
-    return;
+
+    if (kvm_enabled()) {
+        r = kvm_init_vcpu(env);
+        if (r < 0) {
+            fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
+            exit(1);
+        }
+    }
 }
 
 int qemu_cpu_self(void *env)

@@ -604,11 +610,16 @@ static int qemu_cpu_exec(CPUState *env);
 static void *kvm_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
+    int r;
 
     qemu_mutex_lock(&qemu_global_mutex);
     qemu_thread_self(env->thread);
 
-    kvm_init_vcpu(env);
+    r = kvm_init_vcpu(env);
+    if (r < 0) {
+        fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
+        exit(1);
+    }
 
     kvm_init_ipi(env);

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #11 lore raw

PATCH v2 11/24 kvm: Provide sigbus services arch-independently

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Provide arch-independent kvm_on_sigbus* stubs to remove the #ifdef'ery from cpus.c. This patch also fixes --disable-kvm build by providing the missing kvm_on_sigbus_vcpu kvm-stub.

Signed-off-by Jan Kiszka <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

Acked-by Alexander Graf <[email protected]>

---
 cpus.c             |   10 ++++------
 kvm-all.c          |   10 ++++++++++
 kvm-stub.c         |    5 +++++
 kvm.h              |    7 +++++--
 target-i386/kvm.c  |    4 ++--
 target-ppc/kvm.c   |   10 ++++++++++
 target-s390x/kvm.c |   10 ++++++++++
 7 files changed, 46 insertions(+), 10 deletions(-)

cpus.c +4 -6

index 8475757..3a32828 100644
@@ -543,10 +543,9 @@ static void sigbus_reraise(void)
 static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
                            void *ctx)
 {
-#if defined(TARGET_I386)
-    if (kvm_on_sigbus(siginfo->ssi_code, (void *)(intptr_t)siginfo->ssi_addr))
-#endif
+    if (kvm_on_sigbus(siginfo->ssi_code, (void *)(intptr_t)siginfo->ssi_addr)) {
         sigbus_reraise();
+    }
 }
 
 static void qemu_kvm_eat_signal(CPUState *env, int timeout)

@@ -579,10 +578,9 @@ static void qemu_kvm_eat_signal(CPUState *env, int timeout)
 
         switch (r) {
         case SIGBUS:
-#ifdef TARGET_I386
-            if (kvm_on_sigbus_vcpu(env, siginfo.si_code, siginfo.si_addr))
-#endif
+            if (kvm_on_sigbus_vcpu(env, siginfo.si_code, siginfo.si_addr)) {
                 sigbus_reraise();
+            }
             break;
         default:
             break;

kvm-all.c +10 -0

index 1a55a10..5bfa8c0 100644
@@ -1362,3 +1362,13 @@ int kvm_set_ioeventfd_pio_word(int fd, uint16_t addr, uint16_t val, bool assign)
     return -ENOSYS;
 #endif
 }
+
+int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
+{
+    return kvm_arch_on_sigbus_vcpu(env, code, addr);
+}
+
+int kvm_on_sigbus(int code, void *addr)
+{
+    return kvm_arch_on_sigbus(code, addr);
+}

kvm-stub.c +5 -0

index 88682f2..d6b6c8e 100644
@@ -147,6 +147,11 @@ int kvm_set_ioeventfd_mmio_long(int fd, uint32_t adr, uint32_t val, bool assign)
     return -ENOSYS;
 }
 
+int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
+{
+    return 1;
+}
+
 int kvm_on_sigbus(int code, void *addr)
 {
     return 1;

kvm.h +5 -2

index ca57517..b2fb5c6 100644
@@ -81,6 +81,9 @@ int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset);
 int kvm_pit_in_kernel(void);
 int kvm_irqchip_in_kernel(void);
 
+int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr);
+int kvm_on_sigbus(int code, void *addr);
+
 /* internal API */
 
 struct KVMState;

@@ -121,8 +124,8 @@ int kvm_arch_init_vcpu(CPUState *env);
 
 void kvm_arch_reset_vcpu(CPUState *env);
 
-int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr);
-int kvm_on_sigbus(int code, void *addr);
+int kvm_arch_on_sigbus_vcpu(CPUState *env, int code, void *addr);
+int kvm_arch_on_sigbus(int code, void *addr);
 
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;

target-i386/kvm.c +2 -2

index 05010bb..9df8ff8 100644
@@ -1839,7 +1839,7 @@ static void kvm_mce_inj_srao_memscrub2(CPUState *env, target_phys_addr_t paddr)
 
 #endif
 
-int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
+int kvm_arch_on_sigbus_vcpu(CPUState *env, int code, void *addr)
 {
 #if defined(KVM_CAP_MCE)
     void *vaddr;

@@ -1889,7 +1889,7 @@ int kvm_on_sigbus_vcpu(CPUState *env, int code, void *addr)
     return 0;
 }
 
-int kvm_on_sigbus(int code, void *addr)
+int kvm_arch_on_sigbus(int code, void *addr)
 {
 #if defined(KVM_CAP_MCE)
     if ((first_cpu->mcg_cap & MCG_SER_P) && addr && code == BUS_MCEERR_AO) {

target-ppc/kvm.c +10 -0

index 710eca1..93ecc57 100644
@@ -404,3 +404,13 @@ bool kvm_arch_stop_on_emulation_error(CPUState *env)
 {
     return true;
 }
+
+int kvm_arch_on_sigbus_vcpu(CPUState *env, int code, void *addr)
+{
+    return 1;
+}
+
+int kvm_arch_on_sigbus(int code, void *addr)
+{
+    return 1;
+}

target-s390x/kvm.c +10 -0

index 38823f5..1702c46 100644
@@ -505,3 +505,13 @@ bool kvm_arch_stop_on_emulation_error(CPUState *env)
 {
     return true;
 }
+
+int kvm_arch_on_sigbus_vcpu(CPUState *env, int code, void *addr)
+{
+    return 1;
+}
+
+int kvm_arch_on_sigbus(int code, void *addr)
+{
+    return 1;
+}

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #12 lore raw

PATCH v2 12/24 Refactor signal setup functions in cpus.c

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Move {tcg,kvm}_init_ipi and block_io_signals to avoid prototypes, rename the former two to clarify that they deal with more than SIG_IPI. No functional changes - except for the tiny fixup of strerror usage.

The forward declaration of sigbus_handler is just temporarily, it will be moved in a succeeding patch. dummy_signal is moved into the !_WIN32 block as we will soon need it also for !CONFIG_IOTHREAD.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |  162 +++++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 83 insertions(+), 79 deletions(-)

cpus.c +83 ~~-79~~

index 3a32828..42717ba 100644
@@ -230,7 +230,15 @@ fail:
     close(fds[1]);
     return err;
 }
-#else
+
+#ifdef CONFIG_IOTHREAD
+static void dummy_signal(int sig)
+{
+}
+#endif
+
+#else /* _WIN32 */
+
 HANDLE qemu_event_handle;
 
 static void dummy_event_handler(void *opaque)

@@ -256,7 +264,7 @@ static void qemu_event_increment(void)
         exit (1);
     }
 }
-#endif
+#endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD
 int qemu_init_main_loop(void)

@@ -352,10 +360,6 @@ static QemuCond qemu_system_cond;
 static QemuCond qemu_pause_cond;
 static QemuCond qemu_work_cond;
 
-static void tcg_init_ipi(void);
-static void kvm_init_ipi(CPUState *env);
-static sigset_t block_io_signals(void);
-
 /* If we have signalfd, we mask out the signals we want to handle and then
  * use signalfd to listen for them.  We rely on whatever the current signal
  * handler is to dispatch the signals when we receive them.

@@ -391,6 +395,77 @@ static void sigfd_handler(void *opaque)
     }
 }
 
+static void cpu_signal(int sig)
+{
+    if (cpu_single_env) {
+        cpu_exit(cpu_single_env);
+    }
+    exit_request = 1;
+}
+
+static void qemu_kvm_init_cpu_signals(CPUState *env)
+{
+    int r;
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = dummy_signal;
+    sigaction(SIG_IPI, &sigact, NULL);
+
+    pthread_sigmask(SIG_BLOCK, NULL, &set);
+    sigdelset(&set, SIG_IPI);
+    sigdelset(&set, SIGBUS);
+    r = kvm_set_signal_mask(env, &set);
+    if (r) {
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        exit(1);
+    }
+}
+
+static void qemu_tcg_init_cpu_signals(void)
+{
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = cpu_signal;
+    sigaction(SIG_IPI, &sigact, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+}
+
+static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
+                           void *ctx);
+
+static sigset_t block_io_signals(void)
+{
+    sigset_t set;
+    struct sigaction action;
+
+    /* SIGUSR2 used by posix-aio-compat.c */
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR2);
+    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGIO);
+    sigaddset(&set, SIGALRM);
+    sigaddset(&set, SIG_IPI);
+    sigaddset(&set, SIGBUS);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+
+    memset(&action, 0, sizeof(action));
+    action.sa_flags = SA_SIGINFO;
+    action.sa_sigaction = (void (*)(int, siginfo_t*, void*))sigbus_handler;
+    sigaction(SIGBUS, &action, NULL);
+    prctl(PR_MCE_KILL, 1, 1, 0, 0);
+
+    return set;
+}
+
 static int qemu_signalfd_init(sigset_t mask)
 {
     int sigfd;

@@ -619,7 +694,7 @@ static void *kvm_cpu_thread_fn(void *arg)
         exit(1);
     }
 
-    kvm_init_ipi(env);
+    qemu_kvm_init_cpu_signals(env);
 
     /* signal CPU creation */
     env->created = 1;

@@ -642,7 +717,7 @@ static void *tcg_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
 
-    tcg_init_ipi();
+    qemu_tcg_init_cpu_signals();
     qemu_thread_self(env->thread);
 
     /* signal CPU creation */

@@ -683,77 +758,6 @@ int qemu_cpu_self(void *_env)
     return qemu_thread_equal(&this, env->thread);
 }
 
-static void cpu_signal(int sig)
-{
-    if (cpu_single_env)
-        cpu_exit(cpu_single_env);
-    exit_request = 1;
-}
-
-static void tcg_init_ipi(void)
-{
-    sigset_t set;
-    struct sigaction sigact;
-
-    memset(&sigact, 0, sizeof(sigact));
-    sigact.sa_handler = cpu_signal;
-    sigaction(SIG_IPI, &sigact, NULL);
-
-    sigemptyset(&set);
-    sigaddset(&set, SIG_IPI);
-    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
-}
-
-static void dummy_signal(int sig)
-{
-}
-
-static void kvm_init_ipi(CPUState *env)
-{
-    int r;
-    sigset_t set;
-    struct sigaction sigact;
-
-    memset(&sigact, 0, sizeof(sigact));
-    sigact.sa_handler = dummy_signal;
-    sigaction(SIG_IPI, &sigact, NULL);
-
-    pthread_sigmask(SIG_BLOCK, NULL, &set);
-    sigdelset(&set, SIG_IPI);
-    sigdelset(&set, SIGBUS);
-    r = kvm_set_signal_mask(env, &set);
-    if (r) {
-        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(r));
-        exit(1);
-    }
-}
-
-static sigset_t block_io_signals(void)
-{
-    sigset_t set;
-    struct sigaction action;
-
-    /* SIGUSR2 used by posix-aio-compat.c */
-    sigemptyset(&set);
-    sigaddset(&set, SIGUSR2);
-    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
-
-    sigemptyset(&set);
-    sigaddset(&set, SIGIO);
-    sigaddset(&set, SIGALRM);
-    sigaddset(&set, SIG_IPI);
-    sigaddset(&set, SIGBUS);
-    pthread_sigmask(SIG_BLOCK, &set, NULL);
-
-    memset(&action, 0, sizeof(action));
-    action.sa_flags = SA_SIGINFO;
-    action.sa_sigaction = (void (*)(int, siginfo_t*, void*))sigbus_handler;
-    sigaction(SIGBUS, &action, NULL);
-    prctl(PR_MCE_KILL, 1, 1, 0, 0);
-
-    return set;
-}
-
 void qemu_mutex_lock_iothread(void)
 {
     if (kvm_enabled()) {

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #13 lore raw

PATCH v2 13/24 kvm: Set up signal mask also for !CONFIG_IOTHREAD

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

cpus.c +27 -2

index 42717ba..a33e470 100644
@@ -231,11 +231,9 @@ fail:
     return err;
 }
 
-#ifdef CONFIG_IOTHREAD
 static void dummy_signal(int sig)
 {
 }
-#endif
 
 #else /* _WIN32 */

@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
 #endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD
+static void qemu_kvm_init_cpu_signals(CPUState *env)
+{
+#ifndef _WIN32
+    int r;
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = dummy_signal;
+    sigaction(SIG_IPI, &sigact, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+
+    pthread_sigmask(SIG_BLOCK, NULL, &set);
+    sigdelset(&set, SIG_IPI);
+    sigdelset(&set, SIGBUS);
+    r = kvm_set_signal_mask(env, &set);
+    if (r) {
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        exit(1);
+    }
+#endif
+}
+
 int qemu_init_main_loop(void)
 {
     cpu_set_debug_excp_handler(cpu_debug_handler);

@@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
             fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
             exit(1);
         }
+        qemu_kvm_init_cpu_signals(env);
     }
 }

Signature · 1 line

1.7.1

Avi Kivity

2011-02-28 15:55 UTC #14 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 8 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.

This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.

I verified that 981085dd465c1 merged with ff48eb5fe79ad works, while 981085dd465c1 merged with ff48eb5fe79ad^ fails.

Quoted 57 lines

cpus.c +27 -2

index 42717ba..a33e470 100644
@@ -231,11 +231,9 @@ fail:
      return err;
  }
 
-#ifdef CONFIG_IOTHREAD
  static void dummy_signal(int sig)
  {
  }
-#endif
 
  #else /* _WIN32 */

@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
  #endif /* _WIN32 */
 
  #ifndef CONFIG_IOTHREAD
+static void qemu_kvm_init_cpu_signals(CPUState *env)
+{
+#ifndef _WIN32
+    int r;
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = dummy_signal;
+    sigaction(SIG_IPI,&sigact, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK,&set, NULL);
+
+    pthread_sigmask(SIG_BLOCK, NULL,&set);
+    sigdelset(&set, SIG_IPI);
+    sigdelset(&set, SIGBUS);
+    r = kvm_set_signal_mask(env,&set);
+    if (r) {
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        exit(1);
+    }
+#endif
+}
+
  int qemu_init_main_loop(void)
  {
      cpu_set_debug_excp_handler(cpu_debug_handler);

@@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
              fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
              exit(1);
          }
+        qemu_kvm_init_cpu_signals(env);
      }
  }

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2011-02-28 16:02 UTC #15 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.

Quoted 4 lines

This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.

Does the same test case work with qemu, iothread on and off? Just to ensure we are not hunting an issue with the patch itself but of the merge.

Will have a look as well.

Jan

Quoted 65 lines

I verified that 981085dd465c1 merged with ff48eb5fe79ad works, while 981085dd465c1 merged with ff48eb5fe79ad^ fails.

Quoted 58 lines

cpus.c +27 -2

index 42717ba..a33e470 100644
@@ -231,11 +231,9 @@ fail:
      return err;
  }
 
-#ifdef CONFIG_IOTHREAD
  static void dummy_signal(int sig)
  {
  }
-#endif
 
  #else /* _WIN32 */

@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
  #endif /* _WIN32 */
 
  #ifndef CONFIG_IOTHREAD
+static void qemu_kvm_init_cpu_signals(CPUState *env)
+{
+#ifndef _WIN32
+    int r;
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = dummy_signal;
+    sigaction(SIG_IPI,&sigact, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK,&set, NULL);
+
+    pthread_sigmask(SIG_BLOCK, NULL,&set);
+    sigdelset(&set, SIG_IPI);
+    sigdelset(&set, SIGBUS);
+    r = kvm_set_signal_mask(env,&set);
+    if (r) {
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        exit(1);
+    }
+#endif
+}
+
  int qemu_init_main_loop(void)
  {
      cpu_set_debug_excp_handler(cpu_debug_handler);

@@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
              fprintf(stderr, "kvm_init_vcpu failed: %s\n",
 strerror(-r));
              exit(1);
          }
+        qemu_kvm_init_cpu_signals(env);
      }
  }

Signature · 2 lines

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

Jan Kiszka

2011-02-28 16:05 UTC #16 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.

Quoted 6 lines

Quoted 4 lines
This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.
Does the same test case work with qemu, iothread on and off? Just to

Err, "iothread on" makes no sense here, of course.

Quoted 4 lines

ensure we are not hunting an issue with the patch itself but of the merge.
Will have a look as well.

Jan

Signature · 2 lines

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

Jan Kiszka

2011-02-28 16:16 UTC #17 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

In reply to Avi Kivity

Quoted 66 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.
This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.
I verified that 981085dd465c1 merged with ff48eb5fe79ad works, while 981085dd465c1 merged with ff48eb5fe79ad^ fails.
Quoted 47 lines
cpus.c +26 -2
index 42717ba..a33e470 100644
@@ -231,11 +231,9 @@ fail:
      return err;
  }
 
-#ifdef CONFIG_IOTHREAD
  static void dummy_signal(int sig)
  {
  }
-#endif
 
  #else /* _WIN32 */
 
@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
  #endif /* _WIN32 */
 
  #ifndef CONFIG_IOTHREAD
+static void qemu_kvm_init_cpu_signals(CPUState *env)
+{
+#ifndef _WIN32
+    int r;
+    sigset_t set;
+    struct sigaction sigact;
+
+    memset(&sigact, 0, sizeof(sigact));
+    sigact.sa_handler = dummy_signal;
+    sigaction(SIG_IPI,&sigact, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK,&set, NULL);
+
+    pthread_sigmask(SIG_BLOCK, NULL,&set);
+    sigdelset(&set, SIG_IPI);
+    sigdelset(&set, SIGBUS);
+    r = kvm_set_signal_mask(env,&set);
+    if (r) {
+        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
+        exit(1);
+    }
+#endif
+}
+
  int qemu_init_main_loop(void)

Quoted 8 lines

cpus.c void qemu_init_vcpu(void *_env) · line 320

Quoted 8 lines

cpus.c +1 -0

  {
      cpu_set_debug_excp_handler(cpu_debug_handler);

@@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
              fprintf(stderr, "kvm_init_vcpu failed: %s\n",
 strerror(-r));
              exit(1);
          }
+        qemu_kvm_init_cpu_signals(env);

Just comment that out as long as qemu-kvm is (mis-)using !IOTHREAD mode. I thought it would run before setup_kernel_sigmask, but it's the other way around, and then the wrong non-iothread signal setup is applied.

Jan

Signature · 2 lines

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

Avi Kivity

2011-02-28 16:46 UTC #18 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 76 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.
Quoted 64 lines
This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.
I verified that 981085dd465c1 merged with ff48eb5fe79ad works, while 981085dd465c1 merged with ff48eb5fe79ad^ fails.
Quoted 55 lines
diff --git a/cpus.c b/cpus.c
index 42717ba..a33e470 100644
--- a/cpus.c
+++ b/cpus.c
@@ -231,11 +231,9 @@ fail:
       return err;
}
-#ifdef CONFIG_IOTHREAD
static void dummy_signal(int sig)
   {
   }
-#endif
   #else /* _WIN32 */
@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
#endif /* _WIN32 */
   #ifndef CONFIG_IOTHREAD
 +static void qemu_kvm_init_cpu_signals(CPUState *env)
 +{
 +#ifndef _WIN32
 +    int r;
 +    sigset_t set;
 +    struct sigaction sigact;
 +
 +    memset(&sigact, 0, sizeof(sigact));
 +    sigact.sa_handler = dummy_signal;
 +    sigaction(SIG_IPI,&sigact, NULL);
 +
 +    sigemptyset(&set);
 +    sigaddset(&set, SIG_IPI);
 +    pthread_sigmask(SIG_BLOCK,&set, NULL);
 +
 +    pthread_sigmask(SIG_BLOCK, NULL,&set);
 +    sigdelset(&set, SIG_IPI);
 +    sigdelset(&set, SIGBUS);
 +    r = kvm_set_signal_mask(env,&set);
 +    if (r) {
 +        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
 +        exit(1);
 +    }
 +#endif
 +}
 +
   int qemu_init_main_loop(void)
   {
       cpu_set_debug_excp_handler(cpu_debug_handler);
 @@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
               fprintf(stderr, "kvm_init_vcpu failed: %s\n",
 strerror(-r));
               exit(1);
           }
+ qemu_kvm_init_cpu_signals(env);

Quoted 3 lines

Just comment that out as long as qemu-kvm is (mis-)using !IOTHREAD mode. I thought it would run before setup_kernel_sigmask, but it's the other way around, and then the wrong non-iothread signal setup is applied.

That's what I tried, and it didn't work?! Maybe I forgot to compile or something.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-02-28 16:48 UTC #19 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 3 lines · Avi Kivity

That's what I tried, and it didn't work?! Maybe I forgot to compile or something.

I misspelled #ifdef.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2011-02-28 16:49 UTC #20 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

In reply to Avi Kivity

Quoted 77 lines · Avi Kivity

Quoted 76 lines · Jan Kiszka
Quoted 74 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
Block SIG_IPI, unblock it during KVM_RUN, just like in io-thread mode. It's unused so far, but this infrastructure will be required for self-IPIs and to process SIGBUS plus, in KVM mode, SIGIO and SIGALRM. As Windows doesn't support signal services, we need to provide a stub for the init function.
This patch breaks qemu-kvm after merging. The symptoms are that Windows XP x64 does not respond when netcat connects to some server in it, via -net user,hostfwd. The vcpu thread loops indefinitely on KVM_EXIT_INTR, which is consistent with signals being messed up.
I verified that 981085dd465c1 merged with ff48eb5fe79ad works, while 981085dd465c1 merged with ff48eb5fe79ad^ fails.
Quoted 55 lines
diff --git a/cpus.c b/cpus.c
index 42717ba..a33e470 100644
--- a/cpus.c
+++ b/cpus.c
@@ -231,11 +231,9 @@ fail:
       return err;
}
-#ifdef CONFIG_IOTHREAD
static void dummy_signal(int sig)
   {
   }
-#endif
   #else /* _WIN32 */
@@ -267,6 +265,32 @@ static void qemu_event_increment(void)
#endif /* _WIN32 */
   #ifndef CONFIG_IOTHREAD
 +static void qemu_kvm_init_cpu_signals(CPUState *env)
 +{
 +#ifndef _WIN32
 +    int r;
 +    sigset_t set;
 +    struct sigaction sigact;
 +
 +    memset(&sigact, 0, sizeof(sigact));
 +    sigact.sa_handler = dummy_signal;
 +    sigaction(SIG_IPI,&sigact, NULL);
 +
 +    sigemptyset(&set);
 +    sigaddset(&set, SIG_IPI);
 +    pthread_sigmask(SIG_BLOCK,&set, NULL);
 +
 +    pthread_sigmask(SIG_BLOCK, NULL,&set);
 +    sigdelset(&set, SIG_IPI);
 +    sigdelset(&set, SIGBUS);
 +    r = kvm_set_signal_mask(env,&set);
 +    if (r) {
 +        fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));
 +        exit(1);
 +    }
 +#endif
 +}
 +
   int qemu_init_main_loop(void)
   {
       cpu_set_debug_excp_handler(cpu_debug_handler);
 @@ -292,6 +316,7 @@ void qemu_init_vcpu(void *_env)
               fprintf(stderr, "kvm_init_vcpu failed: %s\n",
 strerror(-r));
               exit(1);
           }
+ qemu_kvm_init_cpu_signals(env);

Quoted 6 lines

Quoted 3 lines
Just comment that out as long as qemu-kvm is (mis-)using !IOTHREAD mode. I thought it would run before setup_kernel_sigmask, but it's the other way around, and then the wrong non-iothread signal setup is applied.
That's what I tried, and it didn't work?! Maybe I forgot to compile or something.

Well, it maybe failed to build as qemu_kvm_init_cpu_signals became unused and the compiler should have bailed out? Probably it's better to disable it directly in the function.

Jan

Signature · 2 lines

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

Avi Kivity

2011-02-28 16:54 UTC #21 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 8 lines · Jan Kiszka

Quoted 3 lines
That's what I tried, and it didn't work?! Maybe I forgot to compile or something.
Well, it maybe failed to build as qemu_kvm_init_cpu_signals became unused and the compiler should have bailed out? Probably it's better to disable it directly in the function.

That's what I did, with #ifdefs, but brokenly (#ifndef).

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2011-03-01 08:40 UTC #22 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Jan Kiszka
Quoted 2 lines
That's what I tried, and it didn't work?! Maybe I forgot to
compile or

Quoted 10 lines

Quoted 6 lines
Quoted 1 line
something.
Well, it maybe failed to build as qemu_kvm_init_cpu_signals became unused and the compiler should have bailed out? Probably it's better to disable it directly in the function.
That's what I did, with #ifdefs, but brokenly (#ifndef).

Well it fails even with the correct #ifdef. Maybe some later patch adds to the breakage.

This is really strange - the same test (migrate.tcp) works for Fedora and Windows XP x86. Installation and setup of Windows XP x64 work fine. It is only migration.tcp (when using netcat to connect to the guest) that fails.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2011-03-01 08:58 UTC #23 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 12 lines · Avi Kivity

Quoted 11 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 2 lines
That's what I tried, and it didn't work?! Maybe I forgot to
compile or
Quoted 1 line
something.
Well, it maybe failed to build as qemu_kvm_init_cpu_signals became unused and the compiler should have bailed out? Probably it's better to disable it directly in the function.

Quoted 6 lines

Quoted 3 lines
That's what I did, with #ifdefs, but brokenly (#ifndef).
Well it fails even with the correct #ifdef. Maybe some later patch adds to the breakage.

But when ifdef'ed out, this patch should be a nop for qemu-kvm. Indeed strange.

Quoted 6 lines

This is really strange - the same test (migrate.tcp) works for Fedora and Windows XP x86. Installation and setup of Windows XP x64 work fine. It is only migration.tcp (when using netcat to connect to the guest) that fails.

Guess this has to be classically debugged. :-/ Let me know if I can help (though not today).

Jan

Signature · 2 lines

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

Avi Kivity

2011-03-01 09:03 UTC #24 lore raw

Re: [PATCH v2 13/24] kvm: Set up signal mask also for !CONFIG_IOTHREAD

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Avi Kivity
Quoted 14 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 2 lines
That's what I tried, and it didn't work?! Maybe I forgot to
compile or
Quoted 1 line
something.
Well, it maybe failed to build as qemu_kvm_init_cpu_signals became unused and the compiler should have bailed out? Probably it's better to disable it directly in the function.
That's what I did, with #ifdefs, but brokenly (#ifndef).

Quoted 5 lines

Quoted 2 lines
Well it fails even with the correct #ifdef. Maybe some later patch adds to the breakage.
But when ifdef'ed out, this patch should be a nop for qemu-kvm. Indeed strange.

Well, there are two functions in cpus.c named qemu_kvm_init_cpu_signals() (an intriguing coincidence). I #ifdefed the wrong one. With the right #ifdef it works correctly.

Quoted 9 lines

Quoted 6 lines
This is really strange - the same test (migrate.tcp) works for Fedora and Windows XP x86. Installation and setup of Windows XP x64 work fine. It is only migration.tcp (when using netcat to connect to the guest) that fails.
Guess this has to be classically debugged. :-/ Let me know if I can help (though not today).

Still has to be debugged, but at least the tree is alive now.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2011-02-01 21:16 UTC #25 lore raw

PATCH v2 14/24 kvm: Refactor qemu_kvm_eat_signals

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

We do not use the timeout, so drop its logic. As we always poll our signals, we do not need to drop the global lock. Removing those calls allows some further simplifications. Also fix the error processing of sigpending at this chance.

Signed-off-by Jan Kiszka <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
 cpus.c |   23 +++++++----------------
 1 files changed, 7 insertions(+), 16 deletions(-)

cpus.c +7 ~~-16~~

index a33e470..04138ba 100644
@@ -648,31 +648,22 @@ static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
     }
 }
 
-static void qemu_kvm_eat_signal(CPUState *env, int timeout)
+static void qemu_kvm_eat_signals(CPUState *env)
 {
-    struct timespec ts;
-    int r, e;
+    struct timespec ts = { 0, 0 };
     siginfo_t siginfo;
     sigset_t waitset;
     sigset_t chkset;
-
-    ts.tv_sec = timeout / 1000;
-    ts.tv_nsec = (timeout % 1000) * 1000000;
+    int r;
 
     sigemptyset(&waitset);
     sigaddset(&waitset, SIG_IPI);
     sigaddset(&waitset, SIGBUS);
 
     do {
-        qemu_mutex_unlock(&qemu_global_mutex);
-
         r = sigtimedwait(&waitset, &siginfo, &ts);
-        e = errno;
-
-        qemu_mutex_lock(&qemu_global_mutex);
-
-        if (r == -1 && !(e == EAGAIN || e == EINTR)) {
-            fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
+        if (r == -1 && !(errno == EAGAIN || errno == EINTR)) {
+            perror("sigtimedwait");
             exit(1);
         }

@@ -688,7 +679,7 @@ static void qemu_kvm_eat_signal(CPUState *env, int timeout)
 
         r = sigpending(&chkset);
         if (r == -1) {
-            fprintf(stderr, "sigpending: %s\n", strerror(e));
+            perror("sigpending");
             exit(1);
         }
     } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));

@@ -699,7 +690,7 @@ static void qemu_kvm_wait_io_event(CPUState *env)
     while (!cpu_has_work(env))
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
 
-    qemu_kvm_eat_signal(env, 0);
+    qemu_kvm_eat_signals(env);
     qemu_wait_io_event_common(env);
 }

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #26 lore raw

PATCH v2 15/24 kvm: Call qemu_kvm_eat_signals also under !CONFIG_IOTHREAD

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Move qemu_kvm_eat_signals around and call it also when the IO-thread is not used. Do not yet process SIGBUS, will be armed in a separate step.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   90 +++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 52 insertions(+), 38 deletions(-)

cpus.c +52 ~~-38~~

index 04138ba..861e270 100644
@@ -235,6 +235,47 @@ static void dummy_signal(int sig)
 {
 }
 
+static void sigbus_reraise(void);
+
+static void qemu_kvm_eat_signals(CPUState *env)
+{
+    struct timespec ts = { 0, 0 };
+    siginfo_t siginfo;
+    sigset_t waitset;
+    sigset_t chkset;
+    int r;
+
+    sigemptyset(&waitset);
+    sigaddset(&waitset, SIG_IPI);
+    sigaddset(&waitset, SIGBUS);
+
+    do {
+        r = sigtimedwait(&waitset, &siginfo, &ts);
+        if (r == -1 && !(errno == EAGAIN || errno == EINTR)) {
+            perror("sigtimedwait");
+            exit(1);
+        }
+
+        switch (r) {
+#ifdef CONFIG_IOTHREAD
+        case SIGBUS:
+            if (kvm_on_sigbus_vcpu(env, siginfo.si_code, siginfo.si_addr)) {
+                sigbus_reraise();
+            }
+            break;
+#endif
+        default:
+            break;
+        }
+
+        r = sigpending(&chkset);
+        if (r == -1) {
+            perror("sigpending");
+            exit(1);
+        }
+    } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));
+}
+
 #else /* _WIN32 */
 
 HANDLE qemu_event_handle;

@@ -262,6 +303,10 @@ static void qemu_event_increment(void)
         exit (1);
     }
 }
+
+static void qemu_kvm_eat_signals(CPUState *env)
+{
+}
 #endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD

@@ -648,43 +693,6 @@ static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
     }
 }
 
-static void qemu_kvm_eat_signals(CPUState *env)
-{
-    struct timespec ts = { 0, 0 };
-    siginfo_t siginfo;
-    sigset_t waitset;
-    sigset_t chkset;
-    int r;
-
-    sigemptyset(&waitset);
-    sigaddset(&waitset, SIG_IPI);
-    sigaddset(&waitset, SIGBUS);
-
-    do {
-        r = sigtimedwait(&waitset, &siginfo, &ts);
-        if (r == -1 && !(errno == EAGAIN || errno == EINTR)) {
-            perror("sigtimedwait");
-            exit(1);
-        }
-
-        switch (r) {
-        case SIGBUS:
-            if (kvm_on_sigbus_vcpu(env, siginfo.si_code, siginfo.si_addr)) {
-                sigbus_reraise();
-            }
-            break;
-        default:
-            break;
-        }
-
-        r = sigpending(&chkset);
-        if (r == -1) {
-            perror("sigpending");
-            exit(1);
-        }
-    } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));
-}
-
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
     while (!cpu_has_work(env))

@@ -949,6 +957,8 @@ static int qemu_cpu_exec(CPUState *env)
 
 bool cpu_exec_all(void)
 {
+    int r;
+
     if (next_cpu == NULL)
         next_cpu = first_cpu;
     for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {

@@ -960,7 +970,11 @@ bool cpu_exec_all(void)
         if (qemu_alarm_pending())
             break;
         if (cpu_can_run(env)) {
-            if (qemu_cpu_exec(env) == EXCP_DEBUG) {
+            r = qemu_cpu_exec(env);
+            if (kvm_enabled()) {
+                qemu_kvm_eat_signals(env);
+            }
+            if (r == EXCP_DEBUG) {
                 break;
             }
         } else if (env->stop) {

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #27 lore raw

PATCH v2 16/24 Set up signalfd under !CONFIG_IOTHREAD

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Will be required for SIGBUS handling. For obvious reasons, this will remain a nop on Windows hosts.

Signed-off-by Jan Kiszka <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
 Makefile.objs |    2 +-
 cpus.c        |  117 +++++++++++++++++++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 54 deletions(-)

Makefile.objs +1 -1

index c3e52c5..81b9a5b 100644
@@ -141,7 +141,7 @@ common-obj-y += $(addprefix ui/, $(ui-obj-y))
 
 common-obj-y += iov.o acl.o
 common-obj-$(CONFIG_THREAD) += qemu-thread.o
-common-obj-$(CONFIG_IOTHREAD) += compatfd.o
+common-obj-$(CONFIG_POSIX) += compatfd.o
 common-obj-y += notify.o event_notifier.o
 common-obj-y += qemu-timer.o qemu-timer-common.o

cpus.c +64 ~~-53~~

141 lines not on this page · raw message

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #28 lore raw

PATCH v2 17/24 kvm: Fix race between timer signals and vcpu entry under !IOTHREAD

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Found by Stefan Hajnoczi: There is a race in kvm_cpu_exec between checking for exit_request on vcpu entry and timer signals arriving before KVM starts to catch them. Plug it by blocking both timer related signals also on !CONFIG_IOTHREAD and process those via signalfd.

As this fix depends on real signalfd support (otherwise the timer signals only kick the compat helper thread, and the main thread hangs), we need to detect the invalid constellation and abort configure.

Signed-off-by Jan Kiszka <[email protected]>

CC Stefan Hajnoczi <[email protected]>

---
 configure |    6 ++++++
 cpus.c    |   31 ++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletions(-)

configure +6 -0

index 4673bf0..368ca8a 100755
@@ -2056,6 +2056,12 @@ EOF
 
 if compile_prog "" "" ; then
   signalfd=yes
+elif test "$kvm" = "yes" -a "$io_thread" != "yes"; then
+  echo
+  echo "ERROR: Host kernel lacks signalfd() support,"
+  echo "but KVM depends on it when the IO thread is disabled."
+  echo
+  exit 1
 fi
 
 # check if eventfd is supported

cpus.c +30 -1

index 359361f..18caf47 100644
@@ -327,6 +327,12 @@ static void qemu_kvm_eat_signals(CPUState *env)
             exit(1);
         }
     } while (sigismember(&chkset, SIG_IPI) || sigismember(&chkset, SIGBUS));
+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif
 }
 
 #else /* _WIN32 */

@@ -376,11 +382,15 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
 
     sigemptyset(&set);
     sigaddset(&set, SIG_IPI);
+    sigaddset(&set, SIGIO);
+    sigaddset(&set, SIGALRM);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
     r = kvm_set_signal_mask(env, &set);
     if (r) {
         fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));

@@ -389,13 +399,32 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
 #endif
 }
 
+#ifndef _WIN32
+static sigset_t block_synchronous_signals(void)
+{
+    sigset_t set;
+
+    sigemptyset(&set);
+    if (kvm_enabled()) {
+        /*
+         * We need to process timer signals synchronously to avoid a race
+         * between exit_request check and KVM vcpu entry.
+         */
+        sigaddset(&set, SIGIO);
+        sigaddset(&set, SIGALRM);
+    }
+
+    return set;
+}
+#endif
+
 int qemu_init_main_loop(void)
 {
 #ifndef _WIN32
     sigset_t blocked_signals;
     int ret;
 
-    sigemptyset(&blocked_signals);
+    blocked_signals = block_synchronous_signals();
 
     ret = qemu_signalfd_init(blocked_signals);
     if (ret) {

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #29 lore raw

PATCH v2 18/24 kvm: Add MCE signal support for !CONFIG_IOTHREAD

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Currently, we only configure and process MCE-related SIGBUS events if CONFIG_IOTHREAD is enabled. The groundwork is laid, we just need to factor out the required handler registration and system configuration.

Signed-off-by Jan Kiszka <[email protected]>

CC Huang Ying <[email protected]>

CC Hidetoshi Seto <[email protected]>

CC Jin Dongming <[email protected]>

---
 cpus.c |  107 +++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 65 insertions(+), 42 deletions(-)

cpus.c +65 ~~-42~~

index 18caf47..c4c5914 100644
@@ -34,9 +34,6 @@
 
 #include "cpus.h"
 #include "compatfd.h"
-#ifdef CONFIG_LINUX
-#include <sys/prctl.h>
-#endif
 
 #ifdef SIGRTMIN
 #define SIG_IPI (SIGRTMIN+4)

@@ -44,10 +41,24 @@
 #define SIG_IPI SIGUSR1
 #endif
 
+#ifdef CONFIG_LINUX
+
+#include <sys/prctl.h>
+
 #ifndef PR_MCE_KILL
 #define PR_MCE_KILL 33
 #endif
 
+#ifndef PR_MCE_KILL_SET
+#define PR_MCE_KILL_SET 1
+#endif
+
+#ifndef PR_MCE_KILL_EARLY
+#define PR_MCE_KILL_EARLY 1
+#endif
+
+#endif /* CONFIG_LINUX */
+
 static CPUState *next_cpu;
 
 /***********************************************************/

@@ -166,6 +177,52 @@ static void cpu_debug_handler(CPUState *env)
     vm_stop(EXCP_DEBUG);
 }
 
+#ifdef CONFIG_LINUX
+static void sigbus_reraise(void)
+{
+    sigset_t set;
+    struct sigaction action;
+
+    memset(&action, 0, sizeof(action));
+    action.sa_handler = SIG_DFL;
+    if (!sigaction(SIGBUS, &action, NULL)) {
+        raise(SIGBUS);
+        sigemptyset(&set);
+        sigaddset(&set, SIGBUS);
+        sigprocmask(SIG_UNBLOCK, &set, NULL);
+    }
+    perror("Failed to re-raise SIGBUS!\n");
+    abort();
+}
+
+static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
+                           void *ctx)
+{
+    if (kvm_on_sigbus(siginfo->ssi_code,
+                      (void *)(intptr_t)siginfo->ssi_addr)) {
+        sigbus_reraise();
+    }
+}
+
+static void qemu_init_sigbus(void)
+{
+    struct sigaction action;
+
+    memset(&action, 0, sizeof(action));
+    action.sa_flags = SA_SIGINFO;
+    action.sa_sigaction = (void (*)(int, siginfo_t*, void*))sigbus_handler;
+    sigaction(SIGBUS, &action, NULL);
+
+    prctl(PR_MCE_KILL, PR_MCE_KILL_SET, PR_MCE_KILL_EARLY, 0, 0);
+}
+
+#else /* !CONFIG_LINUX */
+
+static void qemu_init_sigbus(void)
+{
+}
+#endif /* !CONFIG_LINUX */
+
 #ifndef _WIN32
 static int io_thread_fd = -1;

@@ -288,8 +345,6 @@ static int qemu_signalfd_init(sigset_t mask)
     return 0;
 }
 
-static void sigbus_reraise(void);
-
 static void qemu_kvm_eat_signals(CPUState *env)
 {
     struct timespec ts = { 0, 0 };

@@ -310,13 +365,11 @@ static void qemu_kvm_eat_signals(CPUState *env)
         }
 
         switch (r) {
-#ifdef CONFIG_IOTHREAD
         case SIGBUS:
             if (kvm_on_sigbus_vcpu(env, siginfo.si_code, siginfo.si_addr)) {
                 sigbus_reraise();
             }
             break;
-#endif
         default:
             break;
         }

@@ -405,6 +458,7 @@ static sigset_t block_synchronous_signals(void)
     sigset_t set;
 
     sigemptyset(&set);
+    sigaddset(&set, SIGBUS);
     if (kvm_enabled()) {
         /*
          * We need to process timer signals synchronously to avoid a race

@@ -433,6 +487,8 @@ int qemu_init_main_loop(void)
 #endif
     cpu_set_debug_excp_handler(cpu_debug_handler);
 
+    qemu_init_sigbus();
+
     return qemu_event_init();
 }

@@ -565,13 +621,9 @@ static void qemu_tcg_init_cpu_signals(void)
     pthread_sigmask(SIG_UNBLOCK, &set, NULL);
 }
 
-static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
-                           void *ctx);
-
 static sigset_t block_io_signals(void)
 {
     sigset_t set;
-    struct sigaction action;
 
     /* SIGUSR2 used by posix-aio-compat.c */
     sigemptyset(&set);

@@ -585,12 +637,6 @@ static sigset_t block_io_signals(void)
     sigaddset(&set, SIGBUS);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 
-    memset(&action, 0, sizeof(action));
-    action.sa_flags = SA_SIGINFO;
-    action.sa_sigaction = (void (*)(int, siginfo_t*, void*))sigbus_handler;
-    sigaction(SIGBUS, &action, NULL);
-    prctl(PR_MCE_KILL, 1, 1, 0, 0);
-
     return set;
 }

@@ -601,6 +647,8 @@ int qemu_init_main_loop(void)
 
     cpu_set_debug_excp_handler(cpu_debug_handler);
 
+    qemu_init_sigbus();
+
     blocked_signals = block_io_signals();
 
     ret = qemu_signalfd_init(blocked_signals);

@@ -708,31 +756,6 @@ static void qemu_tcg_wait_io_event(void)
     }
 }
 
-static void sigbus_reraise(void)
-{
-    sigset_t set;
-    struct sigaction action;
-
-    memset(&action, 0, sizeof(action));
-    action.sa_handler = SIG_DFL;
-    if (!sigaction(SIGBUS, &action, NULL)) {
-        raise(SIGBUS);
-        sigemptyset(&set);
-        sigaddset(&set, SIGBUS);
-        sigprocmask(SIG_UNBLOCK, &set, NULL);
-    }
-    perror("Failed to re-raise SIGBUS!\n");
-    abort();
-}
-
-static void sigbus_handler(int n, struct qemu_signalfd_siginfo *siginfo,
-                           void *ctx)
-{
-    if (kvm_on_sigbus(siginfo->ssi_code, (void *)(intptr_t)siginfo->ssi_addr)) {
-        sigbus_reraise();
-    }
-}
-
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
     while (!cpu_has_work(env))

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #30 lore raw

PATCH v2 19/24 Introduce VCPU self-signaling service

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Introduce qemu_cpu_kick_self to send SIG_IPI to the calling VCPU context. First user will be kvm.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c        |   21 +++++++++++++++++++++
 qemu-common.h |    1 +
 2 files changed, 22 insertions(+), 0 deletions(-)

cpus.c +21 -0

index c4c5914..9c50a34 100644
@@ -537,6 +537,17 @@ void qemu_cpu_kick(void *env)
     return;
 }
 
+void qemu_cpu_kick_self(void)
+{
+#ifndef _WIN32
+    assert(cpu_single_env);
+
+    raise(SIG_IPI);
+#else
+    abort();
+#endif
+}
+
 void qemu_notify_event(void)
 {
     CPUState *env = cpu_single_env;

@@ -835,6 +846,16 @@ void qemu_cpu_kick(void *_env)
     }
 }
 
+void qemu_cpu_kick_self(void)
+{
+    assert(cpu_single_env);
+
+    if (!cpu_single_env->thread_kicked) {
+        qemu_thread_signal(cpu_single_env->thread, SIG_IPI);
+        cpu_single_env->thread_kicked = true;
+    }
+}
+
 int qemu_cpu_self(void *_env)
 {
     CPUState *env = _env;

qemu-common.h +1 -0

index 63d9943..220c8c8 100644
@@ -287,6 +287,7 @@ void qemu_notify_event(void);
 
 /* Unblock cpu */
 void qemu_cpu_kick(void *env);
+void qemu_cpu_kick_self(void);
 int qemu_cpu_self(void *env);
 
 /* work queue */

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:16 UTC #31 lore raw

PATCH v2 20/24 kvm: Unconditionally reenter kernel after IO exits

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

KVM requires to reenter the kernel after IO exits in order to complete instruction emulation. Failing to do so will leave the kernel state inconsistently behind. To ensure that we will get back ASAP, we issue a self-signal that will cause KVM_RUN to return once the pending operations are completed.

We can move kvm_arch_process_irqchip_events out of the inner VCPU loop. The only state that mattered at its old place was a pending INIT request. Catch it in kvm_arch_pre_run and also trigger a self-signal to process the request on next kvm_cpu_exec.

This patch also fixes the missing exit_request check in kvm_cpu_exec in the CONFIG_IOTHREAD case.

Signed-off-by Jan Kiszka <[email protected]>

CC Gleb Natapov <[email protected]>

---
 kvm-all.c         |   31 +++++++++++++++++--------------
 target-i386/kvm.c |    5 +++++
 2 files changed, 22 insertions(+), 14 deletions(-)

kvm-all.c +17 ~~-14~~

index 5bfa8c0..d961697 100644
@@ -199,7 +199,6 @@ int kvm_pit_in_kernel(void)
     return kvm_state->pit_in_kernel;
 }
 
-
 int kvm_init_vcpu(CPUState *env)
 {
     KVMState *s = kvm_state;

@@ -892,29 +891,33 @@ int kvm_cpu_exec(CPUState *env)
 
     DPRINTF("kvm_cpu_exec()\n");
 
-    do {
-#ifndef CONFIG_IOTHREAD
-        if (env->exit_request) {
-            DPRINTF("interrupt exit requested\n");
-            ret = 0;
-            break;
-        }
-#endif
-
-        if (kvm_arch_process_irqchip_events(env)) {
-            ret = 0;
-            break;
-        }
+    if (kvm_arch_process_irqchip_events(env)) {
+        env->exit_request = 0;
+        env->exception_index = EXCP_HLT;
+        return 0;
+    }
 
+    do {
         if (env->kvm_vcpu_dirty) {
             kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
             env->kvm_vcpu_dirty = 0;
         }
 
         kvm_arch_pre_run(env, run);
+        if (env->exit_request) {
+            DPRINTF("interrupt exit requested\n");
+            /*
+             * KVM requires us to reenter the kernel after IO exits to complete
+             * instruction emulation. This self-signal will ensure that we
+             * leave ASAP again.
+             */
+            qemu_cpu_kick_self();
+        }
         cpu_single_env = NULL;
         qemu_mutex_unlock_iothread();
+
         ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
+
         qemu_mutex_lock_iothread();
         cpu_single_env = env;
         kvm_arch_post_run(env, run);

target-i386/kvm.c +5 -0

index 9df8ff8..8a87244 100644
@@ -1426,6 +1426,11 @@ int kvm_arch_get_registers(CPUState *env)
 
 int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
+    /* Force the VCPU out of its inner loop to process the INIT request */
+    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
+        env->exit_request = 1;
+    }
+
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
         env->interrupt_request &= ~CPU_INTERRUPT_NMI;

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #32 lore raw

PATCH v2 21/24 kvm: Remove static return code of kvm_handle_io

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Improve the readability of the exit dispatcher by moving the static return value of kvm_handle_io to its caller.

Signed-off-by Jan Kiszka <[email protected]>

---
 kvm-all.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

kvm-all.c +8 -9

index d961697..cf54256 100644
@@ -770,8 +770,8 @@ err:
     return ret;
 }
 
-static int kvm_handle_io(uint16_t port, void *data, int direction, int size,
-                         uint32_t count)
+static void kvm_handle_io(uint16_t port, void *data, int direction, int size,
+                          uint32_t count)
 {
     int i;
     uint8_t *ptr = data;

@@ -805,8 +805,6 @@ static int kvm_handle_io(uint16_t port, void *data, int direction, int size,
 
         ptr += size;
     }
-
-    return 1;
 }
 
 #ifdef KVM_CAP_INTERNAL_ERROR_DATA

@@ -940,11 +938,12 @@ int kvm_cpu_exec(CPUState *env)
         switch (run->exit_reason) {
         case KVM_EXIT_IO:
             DPRINTF("handle_io\n");
-            ret = kvm_handle_io(run->io.port,
-                                (uint8_t *)run + run->io.data_offset,
-                                run->io.direction,
-                                run->io.size,
-                                run->io.count);
+            kvm_handle_io(run->io.port,
+                          (uint8_t *)run + run->io.data_offset,
+                          run->io.direction,
+                          run->io.size,
+                          run->io.count);
+            ret = 1;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #33 lore raw

PATCH v2 22/24 kvm: Leave kvm_cpu_exec directly after KVM_EXIT_SHUTDOWN

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

The reset we issue on KVM_EXIT_SHUTDOWN implies that we should also leave the VCPU loop. As we now check for exit_request which is set by qemu_system_reset_request, this bug is no longer critical. Still it's an unneeded extra turn.

Signed-off-by Jan Kiszka <[email protected]>

---
 kvm-all.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

kvm-all.c +0 -1

index cf54256..35860df 100644
@@ -959,7 +959,6 @@ int kvm_cpu_exec(CPUState *env)
         case KVM_EXIT_SHUTDOWN:
             DPRINTF("shutdown\n");
             qemu_system_reset_request();
-            ret = 1;
             break;
         case KVM_EXIT_UNKNOWN:
             fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n",

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #34 lore raw

PATCH v2 23/24 Refactor kvm&tcg function names in cpus.c

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Pure interface cosmetics: Ensure that only kvm core services (as declared in kvm.h) start with "kvm_". Prepend "qemu_" to those that violate this rule in cpus.c. Also rename the corresponding tcg functions for the sake of consistency.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

cpus.c +8 -8

index 9c50a34..0d11a20 100644
@@ -778,7 +778,7 @@ static void qemu_kvm_wait_io_event(CPUState *env)
 
 static int qemu_cpu_exec(CPUState *env);
 
-static void *kvm_cpu_thread_fn(void *arg)
+static void *qemu_kvm_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
     int r;

@@ -811,7 +811,7 @@ static void *kvm_cpu_thread_fn(void *arg)
     return NULL;
 }
 
-static void *tcg_cpu_thread_fn(void *arg)
+static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;

@@ -930,7 +930,7 @@ void resume_all_vcpus(void)
     }
 }
 
-static void tcg_init_vcpu(void *_env)
+static void qemu_tcg_init_vcpu(void *_env)
 {
     CPUState *env = _env;
     /* share a single thread for all cpus with TCG */

@@ -938,7 +938,7 @@ static void tcg_init_vcpu(void *_env)
         env->thread = qemu_mallocz(sizeof(QemuThread));
         env->halt_cond = qemu_mallocz(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
-        qemu_thread_create(env->thread, tcg_cpu_thread_fn, env);
+        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
         while (env->created == 0)
             qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
         tcg_cpu_thread = env->thread;

@@ -949,12 +949,12 @@ static void tcg_init_vcpu(void *_env)
     }
 }
 
-static void kvm_start_vcpu(CPUState *env)
+static void qemu_kvm_start_vcpu(CPUState *env)
 {
     env->thread = qemu_mallocz(sizeof(QemuThread));
     env->halt_cond = qemu_mallocz(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
-    qemu_thread_create(env->thread, kvm_cpu_thread_fn, env);
+    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
     while (env->created == 0)
         qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
 }

@@ -966,9 +966,9 @@ void qemu_init_vcpu(void *_env)
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
     if (kvm_enabled())
-        kvm_start_vcpu(env);
+        qemu_kvm_start_vcpu(env);
     else
-        tcg_init_vcpu(env);
+        qemu_tcg_init_vcpu(env);
 }
 
 void qemu_notify_event(void)

Signature · 1 line

1.7.1

Jan Kiszka

author

2011-02-01 21:17 UTC #35 lore raw

PATCH v2 24/24 Fix a few coding style violations in cpus.c

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

No functional changes.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpus.c |   97 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 58 insertions(+), 39 deletions(-)

cpus.c +58 ~~-39~~

index 0d11a20..dd24fe8 100644
@@ -138,25 +138,26 @@ static void do_vm_stop(int reason)
 
 static int cpu_can_run(CPUState *env)
 {
-    if (env->stop)
+    if (env->stop) {
         return 0;
-    if (env->stopped || !vm_running)
+    }
+    if (env->stopped || !vm_running) {
         return 0;
+    }
     return 1;
 }
 
 static int cpu_has_work(CPUState *env)
 {
-    if (env->stop)
+    if (env->stop || env->queued_work_first) {
         return 1;
-    if (env->queued_work_first)
-        return 1;
-    if (env->stopped || !vm_running)
+    }
+    if (env->stopped || !vm_running) {
         return 0;
-    if (!env->halted)
-        return 1;
-    if (qemu_cpu_has_work(env))
+    }
+    if (!env->halted || qemu_cpu_has_work(env)) {
         return 1;
+    }
     return 0;
 }

@@ -164,9 +165,11 @@ static int any_cpu_has_work(void)
 {
     CPUState *env;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (cpu_has_work(env))
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        if (cpu_has_work(env)) {
             return 1;
+        }
+    }
     return 0;
 }

@@ -232,9 +235,9 @@ static void qemu_event_increment(void)
     static const uint64_t val = 1;
     ssize_t ret;
 
-    if (io_thread_fd == -1)
+    if (io_thread_fd == -1) {
         return;
-
+    }
     do {
         ret = write(io_thread_fd, &val, sizeof(val));
     } while (ret < 0 && errno == EINTR);

@@ -265,17 +268,17 @@ static int qemu_event_init(void)
     int fds[2];
 
     err = qemu_eventfd(fds);
-    if (err == -1)
+    if (err == -1) {
         return -errno;
-
+    }
     err = fcntl_setfl(fds[0], O_NONBLOCK);
-    if (err < 0)
+    if (err < 0) {
         goto fail;
-
+    }
     err = fcntl_setfl(fds[1], O_NONBLOCK);
-    if (err < 0)
+    if (err < 0) {
         goto fail;
-
+    }
     qemu_set_fd_handler2(fds[0], NULL, qemu_event_read, NULL,
                          (void *)(unsigned long)fds[0]);

@@ -534,7 +537,6 @@ void pause_all_vcpus(void)
 
 void qemu_cpu_kick(void *env)
 {
-    return;
 }
 
 void qemu_cpu_kick_self(void)

@@ -663,13 +665,15 @@ int qemu_init_main_loop(void)
     blocked_signals = block_io_signals();
 
     ret = qemu_signalfd_init(blocked_signals);
-    if (ret)
+    if (ret) {
         return ret;
+    }
 
     /* Note eventfd must be drained before signalfd handlers run */
     ret = qemu_event_init();
-    if (ret)
+    if (ret) {
         return ret;
+    }
 
     qemu_cond_init(&qemu_pause_cond);
     qemu_cond_init(&qemu_system_cond);

@@ -699,10 +703,11 @@ void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
 
     wi.func = func;
     wi.data = data;
-    if (!env->queued_work_first)
+    if (!env->queued_work_first) {
         env->queued_work_first = &wi;
-    else
+    } else {
         env->queued_work_last->next = &wi;
+    }
     env->queued_work_last = &wi;
     wi.next = NULL;
     wi.done = false;

@@ -720,8 +725,9 @@ static void flush_queued_work(CPUState *env)
 {
     struct qemu_work_item *wi;
 
-    if (!env->queued_work_first)
+    if (!env->queued_work_first) {
         return;
+    }
 
     while ((wi = env->queued_work_first)) {
         env->queued_work_first = wi->next;

@@ -747,8 +753,9 @@ static void qemu_tcg_wait_io_event(void)
 {
     CPUState *env;
 
-    while (!any_cpu_has_work())
+    while (!any_cpu_has_work()) {
         qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_mutex_unlock(&qemu_global_mutex);

@@ -769,9 +776,9 @@ static void qemu_tcg_wait_io_event(void)
 
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
-    while (!cpu_has_work(env))
+    while (!cpu_has_work(env)) {
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
-
+    }
     qemu_kvm_eat_signals(env);
     qemu_wait_io_event_common(env);
 }

@@ -799,12 +806,14 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
+    while (!qemu_system_ready) {
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
-        if (cpu_can_run(env))
+        if (cpu_can_run(env)) {
             qemu_cpu_exec(env);
+        }
         qemu_kvm_wait_io_event(env);
     }

@@ -820,13 +829,15 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     /* signal CPU creation */
     qemu_mutex_lock(&qemu_global_mutex);
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
         env->created = 1;
+    }
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
+    while (!qemu_system_ready) {
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
         cpu_exec_all();

@@ -839,6 +850,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
+
     qemu_cond_broadcast(env->halt_cond);
     if (!env->thread_kicked) {
         qemu_thread_signal(env->thread, SIG_IPI);

@@ -890,8 +902,9 @@ static int all_vcpus_paused(void)
     CPUState *penv = first_cpu;
 
     while (penv) {
-        if (!penv->stopped)
+        if (!penv->stopped) {
             return 0;
+        }
         penv = (CPUState *)penv->next_cpu;
     }

@@ -933,14 +946,16 @@ void resume_all_vcpus(void)
 static void qemu_tcg_init_vcpu(void *_env)
 {
     CPUState *env = _env;
+
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         env->thread = qemu_mallocz(sizeof(QemuThread));
         env->halt_cond = qemu_mallocz(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
         qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
-        while (env->created == 0)
+        while (env->created == 0) {
             qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+        }
         tcg_cpu_thread = env->thread;
         tcg_halt_cond = env->halt_cond;
     } else {

@@ -955,8 +970,9 @@ static void qemu_kvm_start_vcpu(CPUState *env)
     env->halt_cond = qemu_mallocz(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
     qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
-    while (env->created == 0)
+    while (env->created == 0) {
         qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+    }
 }
 
 void qemu_init_vcpu(void *_env)

@@ -965,10 +981,11 @@ void qemu_init_vcpu(void *_env)
 
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
-    if (kvm_enabled())
+    if (kvm_enabled()) {
         qemu_kvm_start_vcpu(env);
-    else
+    } else {
         qemu_tcg_init_vcpu(env);
+    }
 }
 
 void qemu_notify_event(void)

@@ -1043,16 +1060,18 @@ bool cpu_exec_all(void)
 {
     int r;
 
-    if (next_cpu == NULL)
+    if (next_cpu == NULL) {
         next_cpu = first_cpu;
+    }
     for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {
         CPUState *env = next_cpu;
 
         qemu_clock_enable(vm_clock,
                           (env->singlestep_enabled & SSTEP_NOTIMER) == 0);
 
-        if (qemu_alarm_pending())
+        if (qemu_alarm_pending()) {
             break;
+        }
         if (cpu_can_run(env)) {
             r = qemu_cpu_exec(env);
             if (kvm_enabled()) {

Signature · 1 line

1.7.1

Marcelo Tosatti

2011-02-04 14:37 UTC #36 lore raw

In reply to Jan Kiszka

Quoted 10 lines · Jan Kiszka

Version 2 of part II. Changes:
- Fixed "Unconditionally reenter kernel after IO exits" to take self-INIT into account
- Fixed misplaced hunk in "Fix race between timer signals and vcpu entry under !IOTHREAD" (rebase artifact)
- Factor out block_synchronous_signals (analogue to block_io_signals)
- Additional fix to break out of SMP VCPU loop on pending IO event
- Fork qemu_kvm_init_cpu_signals over CONFIG_IOTHREAD
- Additional cleanup, flattening the main loop

Quoted 2 lines

Hope I addressed all review comments (except for passing env to qemu_cpu_kick_self which I think is better as it is).

1 and 24 rejected, applied the remaining ones, thanks.

Jan Kiszka

author

2011-02-04 16:29 UTC #37 lore raw

Quoted 11 lines · Marcelo Tosatti

Quoted 10 lines · Jan Kiszka
Version 2 of part II. Changes:
- Fixed "Unconditionally reenter kernel after IO exits" to take self-INIT into account
- Fixed misplaced hunk in "Fix race between timer signals and vcpu entry under !IOTHREAD" (rebase artifact)
- Factor out block_synchronous_signals (analogue to block_io_signals)
- Additional fix to break out of SMP VCPU loop on pending IO event
- Fork qemu_kvm_init_cpu_signals over CONFIG_IOTHREAD
- Additional cleanup, flattening the main loop

Quoted 4 lines

Quoted 2 lines
Hope I addressed all review comments (except for passing env to qemu_cpu_kick_self which I think is better as it is).
1 and 24 rejected, applied the remaining ones, thanks.

23 is missing as well. You want me to post the last two directly?

Jan

Marcelo Tosatti

2011-02-04 16:50 UTC #38 lore raw

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Jan Kiszka
Version 2 of part II. Changes:
- Fixed "Unconditionally reenter kernel after IO exits" to take self-INIT into account
- Fixed misplaced hunk in "Fix race between timer signals and vcpu entry under !IOTHREAD" (rebase artifact)
- Factor out block_synchronous_signals (analogue to block_io_signals)
- Additional fix to break out of SMP VCPU loop on pending IO event
- Fork qemu_kvm_init_cpu_signals over CONFIG_IOTHREAD
- Additional cleanup, flattening the main loop

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Hope I addressed all review comments (except for passing env to qemu_cpu_kick_self which I think is better as it is).
1 and 24 rejected, applied the remaining ones, thanks.
23 is missing as well. You want me to post the last two directly?
Jan

Better include in the next round to make Anthony's life easier (they're not critical anyway).

BTW, Anthony, patches are being sent twice to the list, once by author and again by pull request. It is that way by your request.