[Qemu-devel] Re: [PATCH] qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back to non-accelerated mode

[Dustin Kirkland <kirkland@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]

On Fri, Sep 4, 2009 at 11:36 AM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> On Fri, Sep 04, 2009 at 11:06:38AM -0500, Dustin Kirkland wrote:
>> On Fri, 2009-09-04 at 08:22 +0100, Mark McLoughlin wrote:
>> > On Thu, 2009-09-03 at 19:18 -0300, Glauber Costa wrote:
>> > > On Thu, Sep 3, 2009 at 6:48 PM, Dustin Kirkland<kirkland@canonical.com> wrote:
>> > > > On Thu, Sep 3, 2009 at 3:01 PM, Mark McLoughlin<markmc@redhat.com> wrote:
>> > > >> On Thu, 2009-09-03 at 12:31 -0500, Dustin Kirkland wrote:
>> > > >>> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back
>> > > >>> to non-accelerated mode
>> > > >>>
>> > > >>> We're seeing segfaults on systems without access to /dev/kvm.  It
>> > > >>> looks like the global kvm_allowed is being set just a little too late
>> > > >>> in vl.c.  This patch moves the kvm initialization a bit higher in the
>> > > >>> vl.c main, just after options processing, and solves the segfaults.
>> > > >>> We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>> > > >>> upstream, or advise if and why this might not be the optimal solution.
>> > > >>
>> > > >> Ah discussion about an alternative fix for this fizzled out recently:
>> > > >>
>> > > >>  http://www.mail-archive.com/kvm@vger.kernel.org/msg19890.html
>> > > >
>> > > > Ah, thanks Mark.  In that thread, I found Daniel's suggestion the most
>> > > > reasonable, and user-friendly:
>> > > >
>> > > > On Mon, Jul 27, 2009 at 1:44 PM, Daniel P. Berrange<berrange@redhat.com> wrote:
>> > > >> Well, we could go for logic like:
>> > > >>
>> > > >>  * No arg given          => try kvm, try kqemu, try tcg
>> > > >>  * --accelmode arg given => try $arg, and fail if unavailable
>> > > >>
>> > > >> then libvirt would simply always supply --accelmode for all VMs,
>> > > >> while people running qemu manually would get best available
>> > > I sent some patches to do that, but they were incomplete, and I was
>> > > preempted by something else.
>> > > If you want, you can wait for my cycles to come back, or pick from where I left
>>
>> Thanks for the pointer, Glauber.  My cycles a bit constrained too, but
>> I'll have a look when I get a chance.
>>
>> > In the meantime, can we commit to stable-0.11 either Dustin's fix or
>> > this:
>> >
>> >   http://git.et.redhat.com/?p=qemu-fedora.git;a=commitdiff;h=aa1620047b
>>
>> +1.  We're looking for something agreeable in stable-0.11, that solves
>> the segfault and proceeds without VT acceleration.
>
> Dustin,
>
> Can you please resend the patch with the suggestion i made earlier, for
> stable-0.11?

Sure, Marcelo.  It's attached.

I tested it, and it still does avoid the segfault.

Luiz, could you re-test this patch on your side too?

:-Dustin

[-- Attachment #2: fix-no-dev-kvm-segfault.patch --]
[-- Type: text/x-diff, Size: 1833 bytes --]

qemu-kvm: fix segfault when running kvm without /dev/kvm

qemu-kvm segfaults on systems without access to /dev/kvm.
The global kvm_allowed is being set too late in vl.c.

This patch moves the kvm initialization a bit higher in the
vl.c main, just after the daemonize fork.

This fix is intended to be a short term solution, solving the
segfaults.

In the longer term, the suggested approach requires a bit more
development and testing:
 * If no arg given          => try kvm, try kqemu, try tcg
 * If --accelmode arg given => try $arg, and fail if unavailable

Signed-off-by: Dustin Kirkland <kirkland@canonical.com>

diff --git a/vl.c b/vl.c
index db75470..26bced8 100644
--- a/vl.c
+++ b/vl.c
@@ -5831,6 +5831,20 @@ int main(int argc, char **argv, char **envp)
     }
 #endif
 
+    if (kvm_enabled()) {
+        int ret;
+
+        ret = kvm_init(smp_cpus);
+        if (ret < 0) {
+#if defined(KVM_UPSTREAM) || defined(NO_CPU_EMULATION)
+            fprintf(stderr, "failed to initialize KVM\n");
+            exit(1);
+#endif
+            fprintf(stderr, "Could not initialize KVM, will disable KVM support\n");
+            kvm_allowed = 0;
+        }
+    }
+
 #ifdef CONFIG_KQEMU
     if (smp_cpus > 1)
         kqemu_allowed = 0;
@@ -6002,20 +6016,6 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    if (kvm_enabled()) {
-        int ret;
-
-        ret = kvm_init(smp_cpus);
-        if (ret < 0) {
-#if defined(KVM_UPSTREAM) || defined(NO_CPU_EMULATION)
-            fprintf(stderr, "failed to initialize KVM\n");
-            exit(1);
-#endif
-            fprintf(stderr, "Could not initialize KVM, will disable KVM support\n");
-	     kvm_allowed = 0;
-        }
-    }
-
     if (monitor_device) {
         monitor_hd = qemu_chr_open("monitor", monitor_device, NULL);
         if (!monitor_hd) {