[Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Thomas Huth]

When compiling with "--disable-tcg", we currently still use "tcg"
as default accelerator. "kvm" should be used in this case instead.
Also, some downstream distros provide QEMU binaries which have "kvm"
in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
KVM by default - and some users might want to do something similar
with upstream binaries, too. Accomodate them by using "kvm:tcg" as
default when we detect such a binary name.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 accel/accel.c          | 18 +++++++++++++++---
 include/sysemu/accel.h |  2 +-
 vl.c                   |  2 +-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 966b2d8..9be195c 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -68,7 +68,7 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
     return ret;
 }
 
-void configure_accelerator(MachineState *ms)
+void configure_accelerator(MachineState *ms, const char *progname)
 {
     const char *accel;
     char **accel_list, **tmp;
@@ -79,8 +79,20 @@ void configure_accelerator(MachineState *ms)
 
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
     if (accel == NULL) {
-        /* Use the default "accelerator", tcg */
-        accel = "tcg";
+        /* Select the default accelerator */
+        int pnlen = strlen(progname);
+        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
+            /* If the program name ends with "kvm", we prefer KVM */
+            accel = "kvm:tcg";
+        } else {
+#if defined(CONFIG_TCG)
+            accel = "tcg";
+#elif defined(CONFIG_KVM)
+            accel = "kvm";
+#else
+#error "No default accelerator available"
+#endif
+        }
     }
 
     accel_list = g_strsplit(accel, ":", 0);
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 637358f..285899e 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -66,7 +66,7 @@ typedef struct AccelClass {
 
 extern unsigned long tcg_tb_size;
 
-void configure_accelerator(MachineState *ms);
+void configure_accelerator(MachineState *ms, const char *progname);
 /* Register accelerator specific global properties */
 void accel_register_compat_props(AccelState *accel);
 /* Called just before os_setup_post (ie just before drop OS privs) */
diff --git a/vl.c b/vl.c
index 0388852..757246a 100644
--- a/vl.c
+++ b/vl.c
@@ -4220,7 +4220,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    configure_accelerator(current_machine);
+    configure_accelerator(current_machine, argv[0]);
 
     if (!qtest_enabled() && machine_class->deprecation_reason) {
         error_report("Machine type '%s' is deprecated: %s",
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Peter Maydell]

On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
> When compiling with "--disable-tcg", we currently still use "tcg"
> as default accelerator. "kvm" should be used in this case instead.

This part is non-controversial and makes good sense.

> Also, some downstream distros provide QEMU binaries which have "kvm"
> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
> KVM by default - and some users might want to do something similar
> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
> default when we detect such a binary name.

This part is much riskier and less clearly a good plan --
do we really want our behaviour to vary based on the name
of the executable? Distros who want that sort of qemu-kvm
wrapper generally are providing it already (the Ubuntu one
is a 2-line shell script).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Cornelia Huck]

On Fri,  5 Oct 2018 16:13:12 +0200
Thomas Huth <thuth@redhat.com> wrote:

> When compiling with "--disable-tcg", we currently still use "tcg"
> as default accelerator. "kvm" should be used in this case instead.
> Also, some downstream distros provide QEMU binaries which have "kvm"
> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
> KVM by default - and some users might want to do something similar
> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
> default when we detect such a binary name.

Heh, we haven't had that discussion for some time ;)

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  accel/accel.c          | 18 +++++++++++++++---
>  include/sysemu/accel.h |  2 +-
>  vl.c                   |  2 +-
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 966b2d8..9be195c 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -68,7 +68,7 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>      return ret;
>  }
>  
> -void configure_accelerator(MachineState *ms)
> +void configure_accelerator(MachineState *ms, const char *progname)
>  {
>      const char *accel;
>      char **accel_list, **tmp;
> @@ -79,8 +79,20 @@ void configure_accelerator(MachineState *ms)
>  
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (accel == NULL) {
> -        /* Use the default "accelerator", tcg */
> -        accel = "tcg";
> +        /* Select the default accelerator */
> +        int pnlen = strlen(progname);
> +        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +            /* If the program name ends with "kvm", we prefer KVM */
> +            accel = "kvm:tcg";

Ends, or also starts?

(In general, that approach probably makes more sense than the current
default.)

> +        } else {
> +#if defined(CONFIG_TCG)
> +            accel = "tcg";
> +#elif defined(CONFIG_KVM)
> +            accel = "kvm";
> +#else
> +#error "No default accelerator available"

So, we can't configure a qemu with neither tcg nor kvm, but for example
with hax or xen? (Is that possible today?)

> +#endif
> +        }
>      }
>  
>      accel_list = g_strsplit(accel, ":", 0);

ISTR that we had a suggestion last time that we should provide
qemu-kvm, qemu-tcg, etc. binaries...

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Philippe Mathieu-Daudé]

On 05/10/2018 16:13, Thomas Huth wrote:
> When compiling with "--disable-tcg", we currently still use "tcg"
> as default accelerator. "kvm" should be used in this case instead.
> Also, some downstream distros provide QEMU binaries which have "kvm"
> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
> KVM by default - and some users might want to do something similar
> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
> default when we detect such a binary name.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  accel/accel.c          | 18 +++++++++++++++---
>  include/sysemu/accel.h |  2 +-
>  vl.c                   |  2 +-
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 966b2d8..9be195c 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -68,7 +68,7 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>      return ret;
>  }
>  
> -void configure_accelerator(MachineState *ms)
> +void configure_accelerator(MachineState *ms, const char *progname)
>  {
>      const char *accel;
>      char **accel_list, **tmp;
> @@ -79,8 +79,20 @@ void configure_accelerator(MachineState *ms)
>  
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (accel == NULL) {
> -        /* Use the default "accelerator", tcg */
> -        accel = "tcg";
> +        /* Select the default accelerator */
> +        int pnlen = strlen(progname);
> +        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +            /* If the program name ends with "kvm", we prefer KVM */
> +            accel = "kvm:tcg";
> +        } else {
> +#if defined(CONFIG_TCG)
> +            accel = "tcg";

OK until here.

> +#elif defined(CONFIG_KVM)
> +            accel = "kvm";

I'm not sure "kvm" here is necessary.

> +#else
> +#error "No default accelerator available"
> +#endif
> +        }
>      }
>  
>      accel_list = g_strsplit(accel, ":", 0);
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 637358f..285899e 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -66,7 +66,7 @@ typedef struct AccelClass {
>  
>  extern unsigned long tcg_tb_size;
>  
> -void configure_accelerator(MachineState *ms);
> +void configure_accelerator(MachineState *ms, const char *progname);
>  /* Register accelerator specific global properties */
>  void accel_register_compat_props(AccelState *accel);
>  /* Called just before os_setup_post (ie just before drop OS privs) */
> diff --git a/vl.c b/vl.c
> index 0388852..757246a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4220,7 +4220,7 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> -    configure_accelerator(current_machine);
> +    configure_accelerator(current_machine, argv[0]);
>  
>      if (!qtest_enabled() && machine_class->deprecation_reason) {
>          error_report("Machine type '%s' is deprecated: %s",
> 

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Peter Maydell]

On 5 October 2018 at 15:30, Cornelia Huck <cohuck@redhat.com> wrote:
> ISTR that we had a suggestion last time that we should provide
> qemu-kvm, qemu-tcg, etc. binaries...

That should shave off at least 0.1% from the length of the typical
QEMU command line :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Paolo Bonzini]

On 05/10/2018 16:22, Peter Maydell wrote:
> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
>> When compiling with "--disable-tcg", we currently still use "tcg"
>> as default accelerator. "kvm" should be used in this case instead.
> 
> This part is non-controversial and makes good sense.

Though it probably should be extended to "whpx" and "hvf" (probably
"xen" too if !CONFIG_KVM).

>> Also, some downstream distros provide QEMU binaries which have "kvm"
>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
>> KVM by default - and some users might want to do something similar
>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
>> default when we detect such a binary name.
> 
> This part is much riskier and less clearly a good plan --
> do we really want our behaviour to vary based on the name
> of the executable? Distros who want that sort of qemu-kvm
> wrapper generally are providing it already (the Ubuntu one
> is a 2-line shell script).

I think it makes sense.  At least RHEL has qemu-kvm but no
qemu-system-x86_64, so it has a non-upstream patch to change the
accelerator; for other distros there are two benefits:

1) now: they could switch to a symlink

2) later: right now, "-accel kvm:tcg" works but we should instead switch
that to "-accel kvm -accel tcg", and deprecate "-M accel=kvm:tcg" (which
doesn't let you specify accelerator options).

I don't really like second guessing based on argv[0], because argv[0]
can actually be spoofed by the exec-ing program.  One idea could be to
pick the "best" KVM-enabled emulator that we can build, and also install
it as qemu-kvm with some magic to flip the default to kvm:tcg.  But that
can be done later; something like Thomas's patch is nice to have, and
good enough.

Paolo

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Paolo Bonzini]

On 05/10/2018 16:30, Cornelia Huck wrote:
>> +        }
>>      }
>>  
>>      accel_list = g_strsplit(accel, ":", 0);
> ISTR that we had a suggestion last time that we should provide
> qemu-kvm, qemu-tcg, etc. binaries...

Not qemu-tcg, as that would be qemu-system-*, but yes that was my
suggestion.  Compared to it, Thomas's patch provides almost the same
benefit and has the big advantage of existing.

Paolo

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
>> When compiling with "--disable-tcg", we currently still use "tcg"
>> as default accelerator. "kvm" should be used in this case instead.
>
> This part is non-controversial and makes good sense.

Agreed.

>> Also, some downstream distros provide QEMU binaries which have "kvm"
>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
>> KVM by default - and some users might want to do something similar
>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
>> default when we detect such a binary name.
>
> This part is much riskier and less clearly a good plan --
> do we really want our behaviour to vary based on the name
> of the executable? Distros who want that sort of qemu-kvm
> wrapper generally are providing it already (the Ubuntu one
> is a 2-line shell script).

I hate it when argv[0] affects behavior[*].  I hate shell wrappers less.

If a system provides just one qemu executable, and its default
accelerator should be something other than tcg:kvm, then there's a use
for making it compile-time configurable.  Reading the default from /etc/
would also work.  Not sure such a system exists.



[*] Go document the behavior with proper precision, and you might come
to share the feeling.

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
>>> When compiling with "--disable-tcg", we currently still use "tcg"
>>> as default accelerator. "kvm" should be used in this case instead.
>>
>> This part is non-controversial and makes good sense.
>
> Agreed.
>
>>> Also, some downstream distros provide QEMU binaries which have "kvm"
>>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
>>> KVM by default - and some users might want to do something similar
>>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
>>> default when we detect such a binary name.
>>
>> This part is much riskier and less clearly a good plan --
>> do we really want our behaviour to vary based on the name
>> of the executable? Distros who want that sort of qemu-kvm
>> wrapper generally are providing it already (the Ubuntu one
>> is a 2-line shell script).
>
> I hate it when argv[0] affects behavior[*].  I hate shell wrappers less.
>
> If a system provides just one qemu executable, and its default
> accelerator should be something other than tcg:kvm, then there's a use

Correction: "other than tcg".  See configure_accelerator().

Remind me, why is "tcg" a good default?

> for making it compile-time configurable.  Reading the default from /etc/
> would also work.  Not sure such a system exists.
>
>
>
> [*] Go document the behavior with proper precision, and you might come
> to share the feeling.

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Thomas Huth]

On 2018-10-09 15:14, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
>>>> When compiling with "--disable-tcg", we currently still use "tcg"
>>>> as default accelerator. "kvm" should be used in this case instead.
>>>
>>> This part is non-controversial and makes good sense.
>>
>> Agreed.
>>
>>>> Also, some downstream distros provide QEMU binaries which have "kvm"
>>>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
>>>> KVM by default - and some users might want to do something similar
>>>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
>>>> default when we detect such a binary name.
>>>
>>> This part is much riskier and less clearly a good plan --
>>> do we really want our behaviour to vary based on the name
>>> of the executable? Distros who want that sort of qemu-kvm
>>> wrapper generally are providing it already (the Ubuntu one
>>> is a 2-line shell script).
>>
>> I hate it when argv[0] affects behavior[*].  I hate shell wrappers less.
>>
>> If a system provides just one qemu executable, and its default
>> accelerator should be something other than tcg:kvm, then there's a use
> 
> Correction: "other than tcg".  See configure_accelerator().
> 
> Remind me, why is "tcg" a good default?

"It's been always like this and we're keen on backward compatibility" ?

 Thomas

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Daniel P. Berrangé]

On Tue, Oct 09, 2018 at 03:14:59PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> >> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
> >>> When compiling with "--disable-tcg", we currently still use "tcg"
> >>> as default accelerator. "kvm" should be used in this case instead.
> >>
> >> This part is non-controversial and makes good sense.
> >
> > Agreed.
> >
> >>> Also, some downstream distros provide QEMU binaries which have "kvm"
> >>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
> >>> KVM by default - and some users might want to do something similar
> >>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
> >>> default when we detect such a binary name.
> >>
> >> This part is much riskier and less clearly a good plan --
> >> do we really want our behaviour to vary based on the name
> >> of the executable? Distros who want that sort of qemu-kvm
> >> wrapper generally are providing it already (the Ubuntu one
> >> is a 2-line shell script).
> >
> > I hate it when argv[0] affects behavior[*].  I hate shell wrappers less.
> >
> > If a system provides just one qemu executable, and its default
> > accelerator should be something other than tcg:kvm, then there's a use
> 
> Correction: "other than tcg".  See configure_accelerator().
> 
> Remind me, why is "tcg" a good default?

Note that when libvirt is driving QEMU, the default is not used. Libvirt
will always explicitly ask for either KVM or TCG. Libvirt reports whether
each host supports TCG or KVM or both, and management applications will
look at this and usually pick KVM, only use TCG if KVM isn't there.

IOW, most users of libvirt have effectively had the behaviour of
'-accel kvm:tcg' for years. Only direct users of QEMU have suffered
from '-accel tcg:kvm'.

The only time we've had trouble with this is with buggy nested virt
implementations, which expose /dev/kvm but then crash & burn when it
is used. I don't think that's an argument against prioritizing
KVM though. Its an argument for not exposing broken nested virt to VMs.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Cornelia Huck]

On Tue, 09 Oct 2018 15:14:59 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >  
> >> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:  
> >>> When compiling with "--disable-tcg", we currently still use "tcg"
> >>> as default accelerator. "kvm" should be used in this case instead.  
> >>
> >> This part is non-controversial and makes good sense.  
> >
> > Agreed.
> >  
> >>> Also, some downstream distros provide QEMU binaries which have "kvm"
> >>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
> >>> KVM by default - and some users might want to do something similar
> >>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
> >>> default when we detect such a binary name.  
> >>
> >> This part is much riskier and less clearly a good plan --
> >> do we really want our behaviour to vary based on the name
> >> of the executable? Distros who want that sort of qemu-kvm
> >> wrapper generally are providing it already (the Ubuntu one
> >> is a 2-line shell script).  
> >
> > I hate it when argv[0] affects behavior[*].  I hate shell wrappers less.
> >
> > If a system provides just one qemu executable, and its default
> > accelerator should be something other than tcg:kvm, then there's a use  
> 
> Correction: "other than tcg".  See configure_accelerator().
> 
> Remind me, why is "tcg" a good default?

I'm not sure why a single accelerator (any of them) would be a good
default. A list (tcg:kvm:<whatever>) sounds much saner, as it would
continue to work even if some accelerators have been disabled (right?)

(And I'd prefer kvm to be first in that list; anything that relies on
tcg being used should specify it explicitly... a normal user will
likely always want the fast variant.)

> 
> > for making it compile-time configurable.  Reading the default from /etc/
> > would also work.  Not sure such a system exists.

Or making it overrideable like that.

> >
> >
> >
> > [*] Go document the behavior with proper precision, and you might come
> > to share the feeling.  
> 

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Peter Maydell]

On 9 October 2018 at 14:43, Cornelia Huck <cohuck@redhat.com> wrote:
> I'm not sure why a single accelerator (any of them) would be a good
> default. A list (tcg:kvm:<whatever>) sounds much saner, as it would
> continue to work even if some accelerators have been disabled (right?)
>
> (And I'd prefer kvm to be first in that list; anything that relies on
> tcg being used should specify it explicitly... a normal user will
> likely always want the fast variant.)

tcg should be the default for binaries without KVM compiled in,
of course... But as Thomas points out, the reason for our current
default is the usual "because we tend not to change things that
would break existing working command lines".

thanks
-- PMM

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Daniel P. Berrangé]

On Tue, Oct 09, 2018 at 02:58:41PM +0100, Peter Maydell wrote:
> On 9 October 2018 at 14:43, Cornelia Huck <cohuck@redhat.com> wrote:
> > I'm not sure why a single accelerator (any of them) would be a good
> > default. A list (tcg:kvm:<whatever>) sounds much saner, as it would
> > continue to work even if some accelerators have been disabled (right?)
> >
> > (And I'd prefer kvm to be first in that list; anything that relies on
> > tcg being used should specify it explicitly... a normal user will
> > likely always want the fast variant.)
> 
> tcg should be the default for binaries without KVM compiled in,
> of course... But as Thomas points out, the reason for our current
> default is the usual "because we tend not to change things that
> would break existing working command lines".

Putting KVM first shouldn't break existing working command lines
in general.

If user doesn't have KVM it won't have any impact as it'll still
fallback to TCG. If user does have KVM, their VMs will be faster. 

It could conceivably break if

  - TCG had some different behaviour to KVM that
    the guest relied upon (we should fix such differences)
  - KVM kmod was buggy (eg broken nested virt, again should be
    something we fix)
  - If the guest OS relied on things being slow (that's already
    fragile)

IMHO we should assume that TCG & KVM are functionally equivalent
and non-broken, and thus changing default accelerator should not
be considered an incompatible change.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Peter Maydell]

On 9 October 2018 at 15:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Tue, Oct 09, 2018 at 02:58:41PM +0100, Peter Maydell wrote:
>> On 9 October 2018 at 14:43, Cornelia Huck <cohuck@redhat.com> wrote:
>> > I'm not sure why a single accelerator (any of them) would be a good
>> > default. A list (tcg:kvm:<whatever>) sounds much saner, as it would
>> > continue to work even if some accelerators have been disabled (right?)
>> >
>> > (And I'd prefer kvm to be first in that list; anything that relies on
>> > tcg being used should specify it explicitly... a normal user will
>> > likely always want the fast variant.)
>>
>> tcg should be the default for binaries without KVM compiled in,
>> of course... But as Thomas points out, the reason for our current
>> default is the usual "because we tend not to change things that
>> would break existing working command lines".
>
> Putting KVM first shouldn't break existing working command lines
> in general.

There are ARM QEMU command lines which will fail with KVM
and work with TCG (eg ones which use -cpu something-other-than-host
or which ask for a GICv3 when the host has only a GICv2).
These are basically cases where KVM can't provide features
that TCG can. I imagine other archs like power have similar.


thanks
-- PMM

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Cornelia Huck]

On Tue, 9 Oct 2018 15:34:22 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 9 October 2018 at 15:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Tue, Oct 09, 2018 at 02:58:41PM +0100, Peter Maydell wrote:  
> >> On 9 October 2018 at 14:43, Cornelia Huck <cohuck@redhat.com> wrote:  
> >> > I'm not sure why a single accelerator (any of them) would be a good
> >> > default. A list (tcg:kvm:<whatever>) sounds much saner, as it would
> >> > continue to work even if some accelerators have been disabled (right?)
> >> >
> >> > (And I'd prefer kvm to be first in that list; anything that relies on
> >> > tcg being used should specify it explicitly... a normal user will
> >> > likely always want the fast variant.)  
> >>
> >> tcg should be the default for binaries without KVM compiled in,
> >> of course... But as Thomas points out, the reason for our current
> >> default is the usual "because we tend not to change things that
> >> would break existing working command lines".  
> >
> > Putting KVM first shouldn't break existing working command lines
> > in general.  
> 
> There are ARM QEMU command lines which will fail with KVM
> and work with TCG (eg ones which use -cpu something-other-than-host
> or which ask for a GICv3 when the host has only a GICv2).
> These are basically cases where KVM can't provide features
> that TCG can. I imagine other archs like power have similar.

Well, on s390x there are (newer) cpu models that we don't support with
tcg but that work fine with kvm on a new-enough host... so I'm not sure
whether we can assume that any given accelerator supports the same set
of cpu models.

It feels like we're doomed whatever we decide to do :/

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Paolo Bonzini]

On 09/10/2018 16:23, Daniel P. Berrangé wrote:
>>>
>>> (And I'd prefer kvm to be first in that list; anything that relies on
>>> tcg being used should specify it explicitly... a normal user will
>>> likely always want the fast variant.)
>> tcg should be the default for binaries without KVM compiled in,
>> of course... But as Thomas points out, the reason for our current
>> default is the usual "because we tend not to change things that
>> would break existing working command lines".

... and because we rely on people that want KVM using almost always the
distro qemu-kvm.

> Putting KVM first shouldn't break existing working command lines
> in general.
> 
> If user doesn't have KVM it won't have any impact as it'll still
> fallback to TCG. If user does have KVM, their VMs will be faster. 
> 
> It could conceivably break if
> 
>   - TCG had some different behaviour to KVM that
>     the guest relied upon (we should fix such differences)

TCG can emulate stuff that doesn't exist in the host processor (e.g.
MPX, PKRU, and on older processors SMEP/SMAP though those should be
ubiquitous).  There are (or were) CI systems for the kernel that were
using it that way.

Paolo

>   - KVM kmod was buggy (eg broken nested virt, again should be
>     something we fix)
>   - If the guest OS relied on things being slow (that's already
>     fragile)
> 
> IMHO we should assume that TCG & KVM are functionally equivalent
> and non-broken, and thus changing default accelerator should not
> be considered an incompatible change.

Re: [Qemu-devel] [PATCH] accel: Improve selection of the default accelerator

[Thomas Huth]

On 2018-10-05 23:12, Paolo Bonzini wrote:
> On 05/10/2018 16:22, Peter Maydell wrote:
>> On 5 October 2018 at 15:13, Thomas Huth <thuth@redhat.com> wrote:
>>> When compiling with "--disable-tcg", we currently still use "tcg"
>>> as default accelerator. "kvm" should be used in this case instead.
>>
>> This part is non-controversial and makes good sense.
> 
> Though it probably should be extended to "whpx" and "hvf" (probably
> "xen" too if !CONFIG_KVM).

Sure, I was just unsure whether anybody has ever tried to compile with
--disable-tcg and --disable-kvm and use one of those accelerators
instead? Does it work?

>>> Also, some downstream distros provide QEMU binaries which have "kvm"
>>> in their names (e.g. "qemu-kvm" on RHEL or "kvm" on Ubuntu) that use
>>> KVM by default - and some users might want to do something similar
>>> with upstream binaries, too. Accomodate them by using "kvm:tcg" as
>>> default when we detect such a binary name.
>>
>> This part is much riskier and less clearly a good plan --
>> do we really want our behaviour to vary based on the name
>> of the executable? Distros who want that sort of qemu-kvm
>> wrapper generally are providing it already (the Ubuntu one
>> is a 2-line shell script).
> 
> I think it makes sense.  At least RHEL has qemu-kvm but no
> qemu-system-x86_64, so it has a non-upstream patch to change the
> accelerator; for other distros there are two benefits:
> 
> 1) now: they could switch to a symlink

Right, I think it's a good idea to avoid wrapper scripts - there is less
chance to get things wrong in this case.

 Thomas