Patch queue, part II

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.

Quoted 4 lines · Jan Kiszka

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.

Doesn't fit with the manual page (or with the idea that signals are asynchronous):

NAME
pthread_kill - send a signal to a thread

...

ERRORS
ESRCH No thread with the ID thread could be found.

EINVAL An invalid signal was specified.

Quoted 18 lines · Avi Kivity

Quoted 4 lines · Jan Kiszka

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.

Doesn't fit with the manual page (or with the idea that signals are asynchronous):

NAME
pthread_kill - send a signal to a thread

...

ERRORS
ESRCH No thread with the ID thread could be found.

Quoted 2 lines

EINVAL An invalid signal was specified.

Valid remark, but I was receiving EAGAIN for blocked RT signals. Don't know if this is Linux-specific. A quick glance at the man pages did not reveal if this is allowed or at least gray area.

However, even when selectively ignoring this, it's more efficient to catch the redundant signaling in user space.

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 +4 -0

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 (;;) {

Quoted 28 lines · Jan Kiszka

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 +4 -0

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();
 

vl.c qemu_irq qemu_system_powerdown; · line 1401

Quoted 11 lines

vl.c +3 -4

      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;
+            }

Doesn't this cause vmstop to spin? We'll never execute main_loop_wait(false) if I read the code correctly?

Quoted 41 lines · Avi Kivity

Quoted 39 lines · Jan Kiszka

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(-)

diff --git a/vl.c b/vl.c
index 5fad700..2ebc55b 100644
--- a/vl.c
+++ b/vl.c
@@ -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;
+ }

Quoted 3 lines

Doesn't this cause vmstop to spin? We'll never execute main_loop_wait(false) if I read the code correctly?

The code path is not changed, we just poll instead of wait in main_loop_wait.

Also, I didn't get your error scenario yet. Even if we left the loop here, what magic would main_loop_wait do to vmstop processing? The stop request is handled outside the loop, that's why we should leave ASAP.

Quoted 42 lines · Jan Kiszka

Quoted 41 lines · Avi Kivity

Quoted 39 lines · Jan Kiszka

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(-)

diff --git a/vl.c b/vl.c
index 5fad700..2ebc55b 100644
--- a/vl.c
+++ b/vl.c
@@ -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;
+ }

Quoted 6 lines

Quoted 3 lines

Doesn't this cause vmstop to spin? We'll never execute main_loop_wait(false) if I read the code correctly?

The code path is not changed, we just poll instead of wait in main_loop_wait.

Where do we wait then?

Quoted 3 lines

Also, I didn't get your error scenario yet. Even if we left the loop here, what magic would main_loop_wait do to vmstop processing? The stop request is handled outside the loop, that's why we should leave ASAP.

I'm just missing the alternate place where we sleep. If there's no such place, we spin.

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]>

CC Huang Ying<[email protected]>

CC Alexander Graf<[email protected]>

CC Paolo Bonzini<[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 f89826a..ce40e67 100644
@@ -542,10 +542,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)

@@ -578,10 +577,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 8e8880a..dd2cadc 100644
@@ -1837,7 +1837,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;

@@ -1887,7 +1887,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 +0 -0

index 38823f5..1702c46 100644
@@ -505,3 +505,13 @@ bool kvm_arch_stop_on_emulation_error(CPUState *env)
  {
      return true;
  }

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]>

CC Huang Ying <[email protected]>

CC Alexander Graf <[email protected]>

CC Paolo Bonzini <[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 |   13 +++++++++++--
  1 files changed, 11 insertions(+), 2 deletions(-)

cpus.c +10 -2

index 16bd8fa..03c89b4 100644
@@ -231,7 +231,6 @@ fail:
      return err;
  }
 
-#ifdef CONFIG_IOTHREAD
  static void dummy_signal(int sig)
  {
  }

@@ -246,6 +245,12 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
      sigact.sa_handler = dummy_signal;
      sigaction(SIG_IPI,&sigact, NULL);
 
+#ifndef CONFIG_IOTHREAD
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK,&set, NULL);
+#endif
+
      pthread_sigmask(SIG_BLOCK, NULL,&set);
      sigdelset(&set, SIG_IPI);
      sigdelset(&set, SIGBUS);

@@ -255,7 +260,6 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
          exit(1);
      }
  }
-#endif
 
  #else /* _WIN32 */
 

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

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 |   88 ++++++++++++++++++++++++++++++++++++---------------------------
  1 files changed, 50 insertions(+), 38 deletions(-)

cpus.c +45 -37

index 9071848..558c0d3 100644
@@ -261,6 +261,45 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
      }
  }
 
+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;

@@ -292,6 +331,10 @@ static void qemu_event_increment(void)
  static void qemu_kvm_init_cpu_signals(CPUState *env)
  {
  }
+
+static void qemu_kvm_eat_signals(CPUState *env)
+{
+}
  #endif /* _WIN32 */
 
  #ifndef CONFIG_IOTHREAD

@@ -631,43 +674,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))

@@ -932,6 +938,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) {

@@ -943,7 +951,11 @@ bool cpu_exec_all(void)
          if (qemu_alarm_pending())
              break;

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 |   88 ++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 50 insertions(+), 38 deletions(-)

cpus.c +45 -37

index 9071848..558c0d3 100644
@@ -261,6 +261,45 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     }
 }
 
+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;

@@ -292,6 +331,10 @@ static void qemu_event_increment(void)
 static void qemu_kvm_init_cpu_signals(CPUState *env)
 {
 }
+
+static void qemu_kvm_eat_signals(CPUState *env)
+{
+}
 #endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD

@@ -631,43 +674,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))

@@ -932,6 +938,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) {

@@ -943,7 +951,11 @@ bool cpu_exec_all(void)
         if (qemu_alarm_pending())

Quoted 8 lines

cpus.c +3 -1

@@ -943,7 +951,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);

cpus.c bool cpu_exec_all(void) · line 960

Quoted 4 lines

cpus.c +2 -0

+            }
+            if (r == EXCP_DEBUG) {
                 break;
             }

As mentioned before, signal processing should be independent of cpu_can_run (still want to process SIGALRM if vm is stopped, for example).

Quoted 130 lines · Jan Kiszka

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 |   88 ++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 50 insertions(+), 38 deletions(-)

cpus.c +48 -38

index 9071848..558c0d3 100644
@@ -261,6 +261,45 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     }
 }
 
+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;

@@ -292,6 +331,10 @@ static void qemu_event_increment(void)
 static void qemu_kvm_init_cpu_signals(CPUState *env)
 {
 }
+
+static void qemu_kvm_eat_signals(CPUState *env)
+{
+}
 #endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD

@@ -631,43 +674,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))

@@ -932,6 +938,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) {

@@ -943,7 +951,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);

cpus.c bool cpu_exec_all(void) · line 960

Quoted 4 lines

cpus.c +2 -0

+            }
+            if (r == EXCP_DEBUG) {
                 break;
             }

As mentioned before, signal processing should be independent of cpu_can_run (still want to process SIGALRM if vm is stopped, for example).

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

Signed-off-by Jan Kiszka<[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 -46

index 558c0d3..fc3f222 100644
@@ -261,6 +261,59 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
      }
  }
 
+/* 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.
+ */
+static void sigfd_handler(void *opaque)
+{
+    int fd = (unsigned long) opaque;
+    struct qemu_signalfd_siginfo info;
+    struct sigaction action;
+    ssize_t len;
+
+    while (1) {
+        do {
+            len = read(fd,&info, sizeof(info));
+        } while (len == -1&&  errno == EINTR);
+
+        if (len == -1&&  errno == EAGAIN) {
+            break;
+        }
+
+        if (len != sizeof(info)) {
+            printf("read from sigfd returned %zd: %m\n", len);
+            return;
+        }
+
+        sigaction(info.ssi_signo, NULL,&action);
+        if ((action.sa_flags&  SA_SIGINFO)&&  action.sa_sigaction) {
+            action.sa_sigaction(info.ssi_signo,
+                                (siginfo_t *)&info, NULL);
+        } else if (action.sa_handler) {
+            action.sa_handler(info.ssi_signo);
+        }
+    }
+}
+
+static int qemu_signalfd_init(sigset_t mask)
+{
+    int sigfd;
+
+    sigfd = qemu_signalfd(&mask);
+    if (sigfd == -1) {
+        fprintf(stderr, "failed to create signalfd\n");
+        return -errno;
+    }
+
+    fcntl_setfl(sigfd, O_NONBLOCK);
+
+    qemu_set_fd_handler2(sigfd, NULL, sigfd_handler, NULL,
+                         (void *)(unsigned long) sigfd);
+
+    return 0;
+}
+
  static void qemu_kvm_eat_signals(CPUState *env)
  {
      struct timespec ts = { 0, 0 };

@@ -340,6 +393,17 @@ static void qemu_kvm_eat_signals(CPUState *env)
  #ifndef CONFIG_IOTHREAD
  int qemu_init_main_loop(void)
  {
+#ifndef _WIN32
+    sigset_t blocked_signals;
+    int ret;
+
+    sigemptyset(&blocked_signals);
+
+    ret = qemu_signalfd_init(blocked_signals);
+    if (ret) {
+        return ret;
+    }
+#endif
      cpu_set_debug_excp_handler(cpu_debug_handler);
 
      return qemu_event_init();

@@ -431,41 +495,6 @@ static QemuCond qemu_system_cond;
  static QemuCond qemu_pause_cond;
  static QemuCond qemu_work_cond;
 
-/* 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.
- */
-static void sigfd_handler(void *opaque)
-{
-    int fd = (unsigned long) opaque;
-    struct qemu_signalfd_siginfo info;
-    struct sigaction action;
-    ssize_t len;
-
-    while (1) {
-        do {
-            len = read(fd,&info, sizeof(info));
-        } while (len == -1&&  errno == EINTR);
-
-        if (len == -1&&  errno == EAGAIN) {
-            break;
-        }
-
-        if (len != sizeof(info)) {
-            printf("read from sigfd returned %zd: %m\n", len);
-            return;
-        }
-
-        sigaction(info.ssi_signo, NULL,&action);
-        if ((action.sa_flags&  SA_SIGINFO)&&  action.sa_sigaction) {
-            action.sa_sigaction(info.ssi_signo,
-                                (siginfo_t *)&info, NULL);
-        } else if (action.sa_handler) {
-            action.sa_handler(info.ssi_signo);
-        }
-    }
-}
-
  static void cpu_signal(int sig)
  {
      if (cpu_single_env) {

@@ -517,24 +546,6 @@ static sigset_t block_io_signals(void)
      return set;
  }
 
-static int qemu_signalfd_init(sigset_t mask)
-{
-    int sigfd;
-
-    sigfd = qemu_signalfd(&mask);
-    if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
-        return -errno;
-    }
-
-    fcntl_setfl(sigfd, O_NONBLOCK);

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]>

---

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

Quoted 13 lines · Jan Kiszka

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]>

---

Quoted 3 lines

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

I don't understand the dependency on signalfd. The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

Quoted 14 lines · Avi Kivity

Quoted 13 lines · Jan Kiszka

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]>

---

Quoted 7 lines

Quoted 3 lines

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

I don't understand the dependency on signalfd.  The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

The only alternative is Stefan's original proposal. But that required fiddling with the signal mask twice per KVM_RUN.

Quoted 23 lines · Jan Kiszka

Quoted 14 lines · Avi Kivity

Quoted 13 lines · Jan Kiszka

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]>

---

Quoted 7 lines

Quoted 3 lines

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

I don't understand the dependency on signalfd. The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

Quoted 5 lines

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

The only alternative is Stefan's original proposal. But that required fiddling with the signal mask twice per KVM_RUN.

I think my original patch messed with the sigmask in the wrong place, as you mentioned doing it twice per KVM_RUN isn't a good idea. I wonder if we can enable SIGALRM only in blocking calls and guest code execution but without signalfd. It might be possible, I don't see an immediate problem with doing that, we might have to use pselect(2) or similar in a few places.

Quoted 30 lines · Stefan Hajnoczi

Quoted 23 lines · Jan Kiszka

Quoted 14 lines · Avi Kivity

Quoted 13 lines · Jan Kiszka

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]>

---

Quoted 7 lines

Quoted 3 lines

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

I don't understand the dependency on signalfd.  The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

Quoted 5 lines

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

The only alternative is Stefan's original proposal. But that required fiddling with the signal mask twice per KVM_RUN.

Quoted 6 lines

I think my original patch messed with the sigmask in the wrong place, as you mentioned doing it twice per KVM_RUN isn't a good idea.  I wonder if we can enable SIGALRM only in blocking calls and guest code execution but without signalfd.  It might be possible, I don't see an immediate problem with doing that, we might have to use pselect(2) or similar in a few places.

My main concern about alternative approaches is that IOTHREAD is about to become the default, and hardly anyone (of the few upstream KVM users) will run without it in the foreseeable future. The next step will be the removal of any !CONFIG_IOTHREAD section. So, how much do we want to invest here (provided my proposal has not remaining issues)?

Quoted 18 lines · Avi Kivity

Quoted 16 lines · Jan Kiszka

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]>

---

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

Quoted 3 lines

I don't understand the dependency on signalfd. The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Avi Kivity

Quoted 16 lines · Jan Kiszka

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]>

---

I don't want to invest that much into !IOTHREAD anymore, so let's see if the proposed catch&abort is acceptable.

Quoted 6 lines

Quoted 3 lines

I don't understand the dependency on signalfd. The normal way of doing things, either waiting for the signal in sigtimedwait() or in ioctl(KVM_RUN), works with SIGALRM just fine.

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

If we use signalfd() (either kernel provided or thread+pipe), we kick out of select by select()ing it (though I don't see how it works without an iothread, since an fd can't stop a vcpu unless you enable SIGIO on it, which is silly for signalfd)

If you leave it as a naked signal, then it can break out of either pselect() or vcpu.

Since the goal is to drop !CONFIG_IOTHREAD, the first path seems better, I just don't understand the problem with emulated signalfd().

Quoted 12 lines

Quoted 3 lines

And how would you be kicked out of the select() call if it is waiting with a timeout? We only have a single thread here.

If we use signalfd() (either kernel provided or thread+pipe), we kick out of select by select()ing it (though I don't see how it works without an iothread, since an fd can't stop a vcpu unless you enable SIGIO on it, which is silly for signalfd)

If you leave it as a naked signal, then it can break out of either pselect() or vcpu.

Quoted 3 lines

Since the goal is to drop !CONFIG_IOTHREAD, the first path seems better, I just don't understand the problem with emulated signalfd().

With the emulated signalfd, there won't be any signal for the VCPU while in KVM_RUN.

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c +0 -0

 }
 
 #else /* _WIN32 */

@@ -398,6 +408,14 @@ int qemu_init_main_loop(void)
     int ret;

cpus.c +0 -0

     ret = qemu_signalfd_init(blocked_signals);
     if (ret) {

Signature · 1 line

1.7.1

Quoted 11 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 12 lines

cpus.c +4 -0

index fc3f222..29b1070 100644
@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 5 lines

cpus.c +0 -0

     r = kvm_set_signal_mask(env, &set);
     if (r) {
         fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(-r));

cpus.c static void qemu_kvm_eat_signals(CPUState *env) · line 363

Quoted 9 lines

cpus.c +6 -0

@@ -351,6 +355,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

Why is this necessary?

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

Quoted 7 lines

cpus.c +0 -0

 }
 
 #else /* _WIN32 */

@@ -398,6 +408,14 @@ int qemu_init_main_loop(void)
     int ret;
 

cpus.c int qemu_init_main_loop(void) · line 418

Quoted 9 lines

cpus.c +8 -0

     sigemptyset(&blocked_signals);
+    if (kvm_enabled()) {
+        /*
+         * We need to process timer signals synchronously to avoid a race
+         * between exit_request check and KVM vcpu entry.
+         */
+        sigaddset(&blocked_signals, SIGIO);
+        sigaddset(&blocked_signals, SIGALRM);
+    }

A block_io_signals() function for !IOTHREAD would be nicer.

Quoted 6 lines

cpus.c +0 -0

     ret = qemu_signalfd_init(blocked_signals);
     if (ret) {

Signature · 1 line

1.7.1

Quoted 12 lines · Marcelo Tosatti

Quoted 11 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

Quoted 15 lines

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 12 lines

cpus.c +4 -0

index fc3f222..29b1070 100644
@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Quoted 9 lines

cpus.c static void qemu_kvm_eat_signals(CPUState *env) · line 363

Quoted 6 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

Quoted 2 lines

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Quoted 5 lines

Quoted 5 lines

#else /* _WIN32 */

cpus.c +0 -0

@@ -398,6 +408,14 @@ int qemu_init_main_loop(void)
     int ret;
 

Quoted 11 lines

cpus.c int qemu_init_main_loop(void) · line 418

Quoted 9 lines

cpus.c +8 -0

     sigemptyset(&blocked_signals);
+    if (kvm_enabled()) {
+        /*
+         * We need to process timer signals synchronously to avoid a race
+         * between exit_request check and KVM vcpu entry.
+         */
+        sigaddset(&blocked_signals, SIGIO);
+        sigaddset(&blocked_signals, SIGALRM);
+    }

A block_io_signals() function for !IOTHREAD would be nicer.

Well, we aren't blocking all I/O signals, so I decided against causing confusion to people that try to compare the result against real block_io_signals. If you mean just pushing those lines that set up blocked_signals into a separate function, then I need to find a good name for it.

Quoted 29 lines · Jan Kiszka

Quoted 27 lines · Marcelo Tosatti

Quoted 16 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 7 lines

cpus.c +4 -0

@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 1 line

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

Quoted 13 lines

Quoted 12 lines

cpus.c static void qemu_kvm_eat_signals(CPUState *env) · line 363

Quoted 7 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

Quoted 2 lines

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set).

So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution.

Quoted 18 lines

Quoted 17 lines

Quoted 5 lines

#else /* _WIN32 */

cpus.c +0 -0

@@ -398,6 +408,14 @@ int qemu_init_main_loop(void)
     int ret;

cpus.c int qemu_init_main_loop(void) · line 418

Quoted 10 lines

cpus.c +8 -0

 
     sigemptyset(&blocked_signals);
+    if (kvm_enabled()) {
+        /*
+         * We need to process timer signals synchronously to avoid a race
+         * between exit_request check and KVM vcpu entry.
+         */
+        sigaddset(&blocked_signals, SIGIO);
+        sigaddset(&blocked_signals, SIGALRM);
+    }

A block_io_signals() function for !IOTHREAD would be nicer.

Quoted 5 lines

Well, we aren't blocking all I/O signals, so I decided against causing confusion to people that try to compare the result against real block_io_signals. If you mean just pushing those lines that set up blocked_signals into a separate function, then I need to find a good name for it.

Yes, separate function, similar to CONFIG_IOTHREAD case (feel free to rename function).

Quoted 30 lines · Marcelo Tosatti

Quoted 29 lines · Jan Kiszka

Quoted 27 lines · Marcelo Tosatti

Quoted 16 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 7 lines

cpus.c +4 -0

@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 3 lines

Quoted 1 line

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

Would exchange some #ifdefs against ifndef _WIN32. Haven't measured the delta though.

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

cpus.c static void qemu_kvm_eat_signals(CPUState *env) · line 363

Quoted 7 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

Quoted 5 lines

Quoted 2 lines

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set).

Quoted 2 lines

So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution.

We leave the vcpu loop (thanks to notify_event), process the signal in the event loop and run the timer handler. This pattern is IMO less invasive to the existing code, specifically as it is about to die long-term anyway.

Quoted 60 lines · Jan Kiszka

Quoted 30 lines · Marcelo Tosatti

Quoted 29 lines · Jan Kiszka

Quoted 27 lines · Marcelo Tosatti

Quoted 16 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 7 lines

cpus.c +4 -0

@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 3 lines

Quoted 1 line

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

Would exchange some #ifdefs against ifndef _WIN32. Haven't measured the delta though.

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

cpus.c static void qemu_kvm_eat_signals(CPUState *env) · line 363

Quoted 7 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

Quoted 5 lines

Quoted 2 lines

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set).

So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution.

Quoted 4 lines

We leave the vcpu loop (thanks to notify_event), process the signal in the event loop and run the timer handler. This pattern is IMO less invasive to the existing code, specifically as it is about to die long-term anyway.

You'll probably see poor timer behaviour on smp guests without iothread enabled.

Quoted 66 lines · Marcelo Tosatti

Quoted 64 lines · Jan Kiszka

Quoted 30 lines · Marcelo Tosatti

Quoted 29 lines · Jan Kiszka

Quoted 27 lines · Marcelo Tosatti

Quoted 16 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 7 lines

cpus.c +4 -0

@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 3 lines

Quoted 1 line

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

Would exchange some #ifdefs against ifndef _WIN32. Haven't measured the delta though.

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env)

Quoted 7 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

Quoted 8 lines

Quoted 2 lines

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set).

So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution.

We leave the vcpu loop (thanks to notify_event), process the signal in the event loop and run the timer handler. This pattern is IMO less invasive to the existing code, specifically as it is about to die long-term anyway.

Quoted 3 lines

You'll probably see poor timer behaviour on smp guests without iothread enabled.

Still checking, but that would mean the notification mechanism is broken
anyway: If IO events do not force us to process them quickly, we already suffer from latencies in SMP mode.

Quoted 71 lines · Jan Kiszka

Quoted 69 lines · Marcelo Tosatti

Quoted 57 lines · Jan Kiszka

Quoted 30 lines · Marcelo Tosatti

Quoted 29 lines · Jan Kiszka

Quoted 27 lines · Marcelo Tosatti

Quoted 16 lines · Jan Kiszka

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.

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

CC Stefan Hajnoczi <[email protected]>

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

cpus.c +0 -0

index fc3f222..29b1070 100644

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env) · line 260

Quoted 7 lines

cpus.c +4 -0

@@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
     sigdelset(&set, SIGBUS);
+#ifndef CONFIG_IOTHREAD
+    sigdelset(&set, SIGIO);
+    sigdelset(&set, SIGALRM);
+#endif

I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD section.

Quoted 3 lines

Quoted 1 line

You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

Would exchange some #ifdefs against ifndef _WIN32. Haven't measured the delta though.

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

cpus.c static void qemu_kvm_init_cpu_signals(CPUState *env)

Quoted 7 lines

cpus.c +6 -0

+
+#ifndef CONFIG_IOTHREAD
+    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
+        qemu_notify_event();
+    }
+#endif

Why is this necessary?

Quoted 5 lines

Quoted 2 lines

You should break out of cpu_exec_all if there's a pending alarm (see qemu_alarm_pending()).

qemu_alarm_pending() is not true until the signal is actually taken. The alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside cpu_exec_all loop (so that alarm pending is set).

Quoted 7 lines

Quoted 2 lines

So, if there is a SIGALRM pending, qemu_run_timers has highest priority, not vcpu execution.

We leave the vcpu loop (thanks to notify_event), process the signal in the event loop and run the timer handler. This pattern is IMO less invasive to the existing code, specifically as it is about to die long-term anyway.

You'll probably see poor timer behaviour on smp guests without iothread enabled.

Quoted 3 lines

Still checking, but that would mean the notification mechanism is broken
anyway: If IO events do not force us to process them quickly, we already suffer from latencies in SMP mode.

Maybe a regression caused by the iothread introduction:

we need to break
out of the cpu loop via global exit_request when there is an IO event pending. Fixing this will also heal the above issue.

Sigh, we need to get rid of those two implementations and focus all reviewing and testing on one. I bet there are still more corner cases sleeping somewhere.

Jan