Re: [Qemu-devel] Re: [PATCH 08/10] MCE: Relay UCR MCE to guest

[Marcelo Tosatti]

On Wed, Oct 20, 2010 at 03:59:29PM -0500, Anthony Liguori wrote:
> On 10/20/2010 02:51 PM, Anthony Liguori wrote:
> >
> >>+}
> >>+
> >>  static void qemu_kvm_eat_signal(CPUState *env, int timeout)
> >>  {
> >>      struct timespec ts;
> >>      int r, e;
> >>      siginfo_t siginfo;
> >>      sigset_t waitset;
> >>+    sigset_t chkset;
> >>
> >>      ts.tv_sec = timeout / 1000;
> >>      ts.tv_nsec = (timeout % 1000) * 1000000;
> >>
> >>      sigemptyset(&waitset);
> >>      sigaddset(&waitset, SIG_IPI);
> >>+    sigaddset(&waitset, SIGBUS);
> >>
> >>-    qemu_mutex_unlock(&qemu_global_mutex);
> >>-    r = sigtimedwait(&waitset,&siginfo,&ts);
> >>-    e = errno;
> >>-    qemu_mutex_lock(&qemu_global_mutex);
> >>+    do {
> >>+        qemu_mutex_unlock(&qemu_global_mutex);
> >>
> >>-    if (r == -1&&  !(e == EAGAIN || e == EINTR)) {
> >>-        fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
> >>-        exit(1);
> >>-    }
> >>+        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));
> >>+            exit(1);
> >>+        }
> >>+
> >>+        switch (r) {
> >>+        case SIGBUS:
> >>+#ifdef TARGET_I386
> >>+            if (kvm_on_sigbus_vcpu(env, siginfo.si_code,
> >>siginfo.si_addr))
> >>+#endif
> >>+                sigbus_reraise();
> >>+            break;
> >>+        default:
> >>+            break;
> >>+        }
> >>+
> >>+        r = sigpending(&chkset);
> >>+        if (r == -1) {
> >>+            fprintf(stderr, "sigpending: %s\n", strerror(e));
> >>+            exit(1);
> >>+        }
> >>+    } while (sigismember(&chkset, SIG_IPI) ||
> >>sigismember(&chkset, SIGBUS));
> >>  }
> >
> >I don't understand why this loop is needed but we specifically
> >wait for a signal to get delivered that's either SIG_IPI or
> >SIGBUS.  We then check whether a SIG_IPI or SIGBUS is pending and
> >loop waiting for signals again.
> >
> >Shouldn't we be looping on just sigismember(SIGBUS)?
> >
> >BTW, we're no longer respecting timeout because we're not
> >adjusting ts after each iteration.
> 
> I think this is important too.  The last time I went through the
> code and played around here, it wasn't possible to set timeout to a
> very, very large value because there are still things that we poll
> for (like whether shutdown has occurred).   If we loop indefinitely
> without reducing ts, we can potentially recreate an infinite timeout
> which means we won't catch any of the events we poll for.  This
> would be a very, very subtle bug to track down.

We should just kill timeout parameter, i don't see any use for it.