| Message ID | 1248131416-11272-4-git-send-email-glommer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Glauber Costa wrote: > The goal is to get rid of the call to kvm_init. But those things > are subtle, and often break. So do it in a separate patch, to help > finding potential issues in future bisections. Found such an issued: This patch triggers a segfault if no kvm modules are loaded and you start qemu without -no-kvm. Please have a look. Jan > > Signed-off-by: Glauber Costa <glommer@redhat.com> > --- > vl.c | 18 +++++++++--------- > 1 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/vl.c b/vl.c > index f4e4d0f..86a6d70 100644 > --- a/vl.c > +++ b/vl.c > @@ -5748,15 +5748,6 @@ int main(int argc, char **argv, char **envp) > signal(SIGTTIN, SIG_IGN); > } > > -#ifdef CONFIG_KVM > - if (kvm_enabled()) { > - if (kvm_init(smp_cpus) < 0) { > - fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); > - exit(1); > - } > - } > -#endif > - > if (pid_file && qemu_create_pidfile(pid_file) != 0) { > if (daemonize) { > uint8_t status = 1; > @@ -5956,6 +5947,15 @@ int main(int argc, char **argv, char **envp) > } > #endif > > +#ifdef CONFIG_KVM > + if (kvm_enabled()) { > + if (kvm_init(smp_cpus) < 0) { > + fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); > + exit(1); > + } > + } > +#endif > + > if (monitor_device) { > monitor_hd = qemu_chr_open("monitor", monitor_device, NULL); > if (!monitor_hd) {
On Sun, Jul 26, 2009 at 08:59:44PM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > The goal is to get rid of the call to kvm_init. But those things > > are subtle, and often break. So do it in a separate patch, to help > > finding potential issues in future bisections. > > Found such an issued: This patch triggers a segfault if no kvm modules > are loaded and you start qemu without -no-kvm. Please have a look. > > Jan ok, the culprit seems to be a if (kvm_enabled()) return; in the beginning of code_gen_alloc. It is 7f3d0cbe, by Avi, and according to changelog, suggested by anthony. I however, fail to realise the purpose of this optimization. For one thing, it totally dictates that kvm has absolutely to be enabled or disabled prior to this point. No mind changing later. Also, the real deal is to be able to compile out tcg entirely. The strategy of just disabling the code gen alloc is a minor nitpick that just papers over this. So I'd say let's revert it. But I'm open to suggestions. > > > > > Signed-off-by: Glauber Costa <glommer@redhat.com> > > --- > > vl.c | 18 +++++++++--------- > > 1 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index f4e4d0f..86a6d70 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -5748,15 +5748,6 @@ int main(int argc, char **argv, char **envp) > > signal(SIGTTIN, SIG_IGN); > > } > > > > -#ifdef CONFIG_KVM > > - if (kvm_enabled()) { > > - if (kvm_init(smp_cpus) < 0) { > > - fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); > > - exit(1); > > - } > > - } > > -#endif > > - > > if (pid_file && qemu_create_pidfile(pid_file) != 0) { > > if (daemonize) { > > uint8_t status = 1; > > @@ -5956,6 +5947,15 @@ int main(int argc, char **argv, char **envp) > > } > > #endif > > > > +#ifdef CONFIG_KVM > > + if (kvm_enabled()) { > > + if (kvm_init(smp_cpus) < 0) { > > + fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); > > + exit(1); > > + } > > + } > > +#endif > > + > > if (monitor_device) { > > monitor_hd = qemu_chr_open("monitor", monitor_device, NULL); > > if (!monitor_hd) { > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Glauber Costa wrote: > On Sun, Jul 26, 2009 at 08:59:44PM +0200, Jan Kiszka wrote: > >> Glauber Costa wrote: >> >>> The goal is to get rid of the call to kvm_init. But those things >>> are subtle, and often break. So do it in a separate patch, to help >>> finding potential issues in future bisections. >>> >> Found such an issued: This patch triggers a segfault if no kvm modules >> are loaded and you start qemu without -no-kvm. Please have a look. >> >> Jan >> > > ok, the culprit seems to be a > > if (kvm_enabled()) > return; > > in the beginning of code_gen_alloc. > > It is 7f3d0cbe, by Avi, and according to changelog, suggested by anthony. > I however, fail to realise the purpose of this optimization. For one thing, > it totally dictates that kvm has absolutely to be enabled or disabled prior > to this point. No mind changing later. Also, the real deal is to be able > to compile out tcg entirely. The strategy of just disabling the code gen > alloc is a minor nitpick that just papers over this. > I agree with you in principle but I think reverting this papers over an issue. Why are we touching code_gen_ptr when using KVM? Can someone post the full back trace?
On Mon, Jul 27, 2009 at 12:49:06PM -0500, Anthony Liguori wrote: > Glauber Costa wrote: >> On Sun, Jul 26, 2009 at 08:59:44PM +0200, Jan Kiszka wrote: >> >>> Glauber Costa wrote: >>> >>>> The goal is to get rid of the call to kvm_init. But those things >>>> are subtle, and often break. So do it in a separate patch, to help >>>> finding potential issues in future bisections. >>>> >>> Found such an issued: This patch triggers a segfault if no kvm modules >>> are loaded and you start qemu without -no-kvm. Please have a look. >>> >>> Jan >>> >> >> ok, the culprit seems to be a >> >> if (kvm_enabled()) >> return; >> >> in the beginning of code_gen_alloc. >> >> It is 7f3d0cbe, by Avi, and according to changelog, suggested by anthony. >> I however, fail to realise the purpose of this optimization. For one thing, >> it totally dictates that kvm has absolutely to be enabled or disabled prior >> to this point. No mind changing later. Also, the real deal is to be able >> to compile out tcg entirely. The strategy of just disabling the code gen >> alloc is a minor nitpick that just papers over this. >> > > I agree with you in principle but I think reverting this papers over an > issue. > > Why are we touching code_gen_ptr when using KVM? Can someone post the > full back trace? we're not. The issue happens exactly when the kvm modules are not loaded, then we're failing to initialize kvm. However, in the patch that raised this issue, I'm moving KVM initialization to after this code path. And in qemu-kvm.git, kvm is enabled-by-default. So tcg code would think kvm is enabled and skip initialization, while kvm code will fail to really initialize itself later. Result? Mayhem. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Glauber Costa wrote: > On Mon, Jul 27, 2009 at 12:49:06PM -0500, Anthony Liguori wrote: >> Glauber Costa wrote: >>> On Sun, Jul 26, 2009 at 08:59:44PM +0200, Jan Kiszka wrote: >>> >>>> Glauber Costa wrote: >>>> >>>>> The goal is to get rid of the call to kvm_init. But those things >>>>> are subtle, and often break. So do it in a separate patch, to help >>>>> finding potential issues in future bisections. >>>>> >>>> Found such an issued: This patch triggers a segfault if no kvm modules >>>> are loaded and you start qemu without -no-kvm. Please have a look. >>>> >>>> Jan >>>> >>> ok, the culprit seems to be a >>> >>> if (kvm_enabled()) >>> return; >>> >>> in the beginning of code_gen_alloc. >>> >>> It is 7f3d0cbe, by Avi, and according to changelog, suggested by anthony. >>> I however, fail to realise the purpose of this optimization. For one thing, >>> it totally dictates that kvm has absolutely to be enabled or disabled prior >>> to this point. No mind changing later. Also, the real deal is to be able >>> to compile out tcg entirely. The strategy of just disabling the code gen >>> alloc is a minor nitpick that just papers over this. >>> >> I agree with you in principle but I think reverting this papers over an >> issue. >> >> Why are we touching code_gen_ptr when using KVM? Can someone post the >> full back trace? > we're not. > > The issue happens exactly when the kvm modules are not loaded, then we're failing > to initialize kvm. However, in the patch that raised this issue, I'm moving > KVM initialization to after this code path. And in qemu-kvm.git, kvm is > enabled-by-default. So tcg code would think kvm is enabled and skip initialization, > while kvm code will fail to really initialize itself later. > > Result? Mayhem. > I think we should simply resolves this the way upstream does: Do not start if modules are missing and -no-kvm is omitted - or even switch over to -enable-kvm as I think you already suggested in some other thread. Then we can either fail or succeed, but not fall back more or less silently. This falling back of qemu-kvm to tcg is a constant source of confusion anyway. Jan
On Mon, Jul 27, 2009 at 08:10:24PM +0200, Jan Kiszka wrote: > Glauber Costa wrote: > > On Mon, Jul 27, 2009 at 12:49:06PM -0500, Anthony Liguori wrote: > >> Glauber Costa wrote: > >>> On Sun, Jul 26, 2009 at 08:59:44PM +0200, Jan Kiszka wrote: > >>> > >>>> Glauber Costa wrote: > >>>> > >>>>> The goal is to get rid of the call to kvm_init. But those things > >>>>> are subtle, and often break. So do it in a separate patch, to help > >>>>> finding potential issues in future bisections. > >>>>> > >>>> Found such an issued: This patch triggers a segfault if no kvm modules > >>>> are loaded and you start qemu without -no-kvm. Please have a look. > >>>> > >>>> Jan > >>>> > >>> ok, the culprit seems to be a > >>> > >>> if (kvm_enabled()) > >>> return; > >>> > >>> in the beginning of code_gen_alloc. > >>> > >>> It is 7f3d0cbe, by Avi, and according to changelog, suggested by anthony. > >>> I however, fail to realise the purpose of this optimization. For one thing, > >>> it totally dictates that kvm has absolutely to be enabled or disabled prior > >>> to this point. No mind changing later. Also, the real deal is to be able > >>> to compile out tcg entirely. The strategy of just disabling the code gen > >>> alloc is a minor nitpick that just papers over this. > >>> > >> I agree with you in principle but I think reverting this papers over an > >> issue. > >> > >> Why are we touching code_gen_ptr when using KVM? Can someone post the > >> full back trace? > > we're not. > > > > The issue happens exactly when the kvm modules are not loaded, then we're failing > > to initialize kvm. However, in the patch that raised this issue, I'm moving > > KVM initialization to after this code path. And in qemu-kvm.git, kvm is > > enabled-by-default. So tcg code would think kvm is enabled and skip initialization, > > while kvm code will fail to really initialize itself later. > > > > Result? Mayhem. > > > > I think we should simply resolves this the way upstream does: Do not > start if modules are missing and -no-kvm is omitted - or even switch > over to -enable-kvm as I think you already suggested in some other > thread. Then we can either fail or succeed, but not fall back more or > less silently. This falling back of qemu-kvm to tcg is a constant source > of confusion anyway. switching to --enable-kvm would be my preferred solution, but guys from mgmt tools may not like it. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 27, 2009 at 03:20:08PM -0300, Glauber Costa wrote: > On Mon, Jul 27, 2009 at 08:10:24PM +0200, Jan Kiszka wrote: > > > > I think we should simply resolves this the way upstream does: Do not > > start if modules are missing and -no-kvm is omitted - or even switch > > over to -enable-kvm as I think you already suggested in some other > > thread. Then we can either fail or succeed, but not fall back more or > > less silently. This falling back of qemu-kvm to tcg is a constant source > > of confusion anyway. > > switching to --enable-kvm would be my preferred solution, but guys from > mgmt tools may not like it. Totally agree that it should never ever fallback to a different mode than the one requested, since falling back from KVM to QEMU simply means the user doesn't discover the problem till their VM install has wasted an hour of their time. Personally I would vote for --accelmode qemu|kvm|kqemu since it is more future proof, but I'm not too bothered if people prefer to have --enable-kvm on the grounds that kqemu is being killed off. libvirt just needs a reliable way to request one of qemu, kvm, or kqemu, and either get an error message, or have the requested mode work. Regards, Daniel
Glauber Costa wrote: > we're not. > > The issue happens exactly when the kvm modules are not loaded, then we're failing > to initialize kvm. However, in the patch that raised this issue, I'm moving > KVM initialization to after this code path. And in qemu-kvm.git, kvm is > enabled-by-default. So tcg code would think kvm is enabled and skip initialization, > while kvm code will fail to really initialize itself later. > That's pure evilness. Good argument to never fall back to TCG.
On Mon, Jul 27, 2009 at 07:28:17PM +0100, Daniel P. Berrange wrote: > On Mon, Jul 27, 2009 at 03:20:08PM -0300, Glauber Costa wrote: > > On Mon, Jul 27, 2009 at 08:10:24PM +0200, Jan Kiszka wrote: > > > > > > I think we should simply resolves this the way upstream does: Do not > > > start if modules are missing and -no-kvm is omitted - or even switch > > > over to -enable-kvm as I think you already suggested in some other > > > thread. Then we can either fail or succeed, but not fall back more or > > > less silently. This falling back of qemu-kvm to tcg is a constant source > > > of confusion anyway. > > > > switching to --enable-kvm would be my preferred solution, but guys from > > mgmt tools may not like it. > > Totally agree that it should never ever fallback to a different mode than > the one requested, since falling back from KVM to QEMU simply means the > user doesn't discover the problem till their VM install has wasted an > hour of their time. Personally I would vote for --accelmode qemu|kvm|kqemu > since it is more future proof, but I'm not too bothered if people prefer > to have --enable-kvm on the grounds that kqemu is being killed off. > libvirt just needs a reliable way to request one of qemu, kvm, or kqemu, > and either get an error message, or have the requested mode work. The big problem here is that in qemu-kvm.git, kvm happens without any user request. That would be the advantage of --enable-kvm or --accelmode, or whatever. Simply changing the default to kill the VM if we fail to initialize KVM is cumbersome, because it would mean that users of pure tcg would have to add an option for a basic VM to work. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 27, 2009 at 03:38:57PM -0300, Glauber Costa wrote: > On Mon, Jul 27, 2009 at 07:28:17PM +0100, Daniel P. Berrange wrote: > > On Mon, Jul 27, 2009 at 03:20:08PM -0300, Glauber Costa wrote: > > > On Mon, Jul 27, 2009 at 08:10:24PM +0200, Jan Kiszka wrote: > > > > > > > > I think we should simply resolves this the way upstream does: Do not > > > > start if modules are missing and -no-kvm is omitted - or even switch > > > > over to -enable-kvm as I think you already suggested in some other > > > > thread. Then we can either fail or succeed, but not fall back more or > > > > less silently. This falling back of qemu-kvm to tcg is a constant source > > > > of confusion anyway. > > > > > > switching to --enable-kvm would be my preferred solution, but guys from > > > mgmt tools may not like it. > > > > Totally agree that it should never ever fallback to a different mode than > > the one requested, since falling back from KVM to QEMU simply means the > > user doesn't discover the problem till their VM install has wasted an > > hour of their time. Personally I would vote for --accelmode qemu|kvm|kqemu > > since it is more future proof, but I'm not too bothered if people prefer > > to have --enable-kvm on the grounds that kqemu is being killed off. > > libvirt just needs a reliable way to request one of qemu, kvm, or kqemu, > > and either get an error message, or have the requested mode work. > The big problem here is that in qemu-kvm.git, kvm happens without any user request. > That would be the advantage of --enable-kvm or --accelmode, or whatever. > Simply changing the default to kill the VM if we fail to initialize KVM is cumbersome, > because it would mean that users of pure tcg would have to add an option for a > basic VM to work. 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 Daniel
On Mon, Jul 27, 2009 at 07:44:27PM +0100, Daniel P. Berrange wrote: > On Mon, Jul 27, 2009 at 03:38:57PM -0300, Glauber Costa wrote: > > On Mon, Jul 27, 2009 at 07:28:17PM +0100, Daniel P. Berrange wrote: > > > On Mon, Jul 27, 2009 at 03:20:08PM -0300, Glauber Costa wrote: > > > > On Mon, Jul 27, 2009 at 08:10:24PM +0200, Jan Kiszka wrote: > > > > > > > > > > I think we should simply resolves this the way upstream does: Do not > > > > > start if modules are missing and -no-kvm is omitted - or even switch > > > > > over to -enable-kvm as I think you already suggested in some other > > > > > thread. Then we can either fail or succeed, but not fall back more or > > > > > less silently. This falling back of qemu-kvm to tcg is a constant source > > > > > of confusion anyway. > > > > > > > > switching to --enable-kvm would be my preferred solution, but guys from > > > > mgmt tools may not like it. > > > > > > Totally agree that it should never ever fallback to a different mode than > > > the one requested, since falling back from KVM to QEMU simply means the > > > user doesn't discover the problem till their VM install has wasted an > > > hour of their time. Personally I would vote for --accelmode qemu|kvm|kqemu > > > since it is more future proof, but I'm not too bothered if people prefer > > > to have --enable-kvm on the grounds that kqemu is being killed off. > > > libvirt just needs a reliable way to request one of qemu, kvm, or kqemu, > > > and either get an error message, or have the requested mode work. > > The big problem here is that in qemu-kvm.git, kvm happens without any user request. > > That would be the advantage of --enable-kvm or --accelmode, or whatever. > > Simply changing the default to kill the VM if we fail to initialize KVM is cumbersome, > > because it would mean that users of pure tcg would have to add an option for a > > basic VM to work. > > 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 were best available can mean a crash. If we're getting this sort of things now, that we're in fall back mode, and thus testing it once in a while, imagine when most of us don't even bother. The only sane semantics to me is: * No arg given => tcg. * some arg given: try it, and if we fail, exit. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/vl.c b/vl.c index f4e4d0f..86a6d70 100644 --- a/vl.c +++ b/vl.c @@ -5748,15 +5748,6 @@ int main(int argc, char **argv, char **envp) signal(SIGTTIN, SIG_IGN); } -#ifdef CONFIG_KVM - if (kvm_enabled()) { - if (kvm_init(smp_cpus) < 0) { - fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); - exit(1); - } - } -#endif - if (pid_file && qemu_create_pidfile(pid_file) != 0) { if (daemonize) { uint8_t status = 1; @@ -5956,6 +5947,15 @@ int main(int argc, char **argv, char **envp) } #endif +#ifdef CONFIG_KVM + if (kvm_enabled()) { + if (kvm_init(smp_cpus) < 0) { + fprintf(stderr, "Could not initialize KVM, will disable KVM support\n"); + exit(1); + } + } +#endif + if (monitor_device) { monitor_hd = qemu_chr_open("monitor", monitor_device, NULL); if (!monitor_hd) {
The goal is to get rid of the call to kvm_init. But those things are subtle, and often break. So do it in a separate patch, to help finding potential issues in future bisections. Signed-off-by: Glauber Costa <glommer@redhat.com> --- vl.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)