[Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator

Cornelia Huck posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170906094927.22376-1-cohuck@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
accel/accel.c              | 22 ++++++++++++++++++++--
arch_init.c                | 17 +++++++++++++++++
include/sysemu/arch_init.h |  2 ++
qemu-options.hx            |  6 ++++--
4 files changed, 43 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Cornelia Huck 6 years, 7 months ago
configure_accelerator() falls back to tcg if no accelerator has
been specified. Formerly, we could be sure that tcg is always
available; however, with --disable-tcg, this is not longer true,
and you are not able to start qemu without explicitly specifying
another accelerator on those builds.

Instead, choose an accelerator in the order tcg->kvm->xen->hax.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

RFC mainly because this breaks iotest 186 in a different way on a
tcg-less x86_64 build: Before, it fails with "-machine accel=tcg: No
accelerator found"; afterwards, there seems to be a difference in
output due to different autogenerated devices. Not sure how to handle
that.

cc:ing some hopefully interested folks (-ENOMAINTAINER again).

---
 accel/accel.c              | 22 ++++++++++++++++++++--
 arch_init.c                | 17 +++++++++++++++++
 include/sysemu/arch_init.h |  2 ++
 qemu-options.hx            |  6 ++++--
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 8ae40e1e13..26a3f32627 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -68,6 +68,25 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
     return ret;
 }
 
+static const char *default_accelerator(void)
+{
+    if (tcg_available()) {
+        return "tcg";
+    }
+    if (kvm_available()) {
+        return "kvm";
+    }
+    if (xen_available()) {
+        return "xen";
+    }
+    if (hax_available()) {
+        return "hax";
+    }
+    /* configure makes sure we have at least one accelerator */
+    g_assert(false);
+    return "";
+}
+
 void configure_accelerator(MachineState *ms)
 {
     const char *accel, *p;
@@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
 
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
     if (accel == NULL) {
-        /* Use the default "accelerator", tcg */
-        accel = "tcg";
+        accel = default_accelerator();
     }
 
     p = accel;
diff --git a/arch_init.c b/arch_init.c
index a0b8ed6167..1d84eca14d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -103,6 +103,23 @@ int xen_available(void)
 #endif
 }
 
+int tcg_available(void)
+{
+#ifdef CONFIG_TCG
+    return 1;
+#else
+    return 0;
+#endif
+}
+
+int hax_available(void)
+{
+#ifdef CONFIG_HAX
+    return 1;
+#else
+    return 0;
+#endif
+}
 
 TargetInfo *qmp_query_target(Error **errp)
 {
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 8751c468ed..43e515c233 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -30,6 +30,8 @@ extern const uint32_t arch_type;
 
 int kvm_available(void);
 int xen_available(void);
+int tcg_available(void);
+int hax_available(void);
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
 CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType type,
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2adfff..386e6e945d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -66,7 +66,8 @@ Supported machine properties are:
 @table @option
 @item accel=@var{accels1}[:@var{accels2}[:...]]
 This is used to enable an accelerator. Depending on the target architecture,
-kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
+kvm, xen, hax or tcg can be available. By default, the first one available
+out of tcg, kvm, xen, hax (in that order) is used. If there is
 more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
 @item kernel_irqchip=on|off
@@ -126,7 +127,8 @@ STEXI
 @item -accel @var{name}[,prop=@var{value}[,...]]
 @findex -accel
 This is used to enable an accelerator. Depending on the target architecture,
-kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
+kvm, xen, hax or tcg can be available. By default, the first one available
+out of tcg, kvm, xen, hax (in that order) is used. If there is
 more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
 @table @option
-- 
2.13.5


Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Cornelia Huck 6 years, 7 months ago
On Wed,  6 Sep 2017 11:49:27 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> configure_accelerator() falls back to tcg if no accelerator has
> been specified. Formerly, we could be sure that tcg is always
> available; however, with --disable-tcg, this is not longer true,
> and you are not able to start qemu without explicitly specifying
> another accelerator on those builds.
> 
> Instead, choose an accelerator in the order tcg->kvm->xen->hax.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> RFC mainly because this breaks iotest 186 in a different way on a
> tcg-less x86_64 build: Before, it fails with "-machine accel=tcg: No
> accelerator found"; afterwards, there seems to be a difference in
> output due to different autogenerated devices. Not sure how to handle
> that.
> 
> cc:ing some hopefully interested folks (-ENOMAINTAINER again).
> 
> ---
>  accel/accel.c              | 22 ++++++++++++++++++++--
>  arch_init.c                | 17 +++++++++++++++++
>  include/sysemu/arch_init.h |  2 ++
>  qemu-options.hx            |  6 ++++--
>  4 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 8ae40e1e13..26a3f32627 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -68,6 +68,25 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>      return ret;
>  }
>  
> +static const char *default_accelerator(void)
> +{
> +    if (tcg_available()) {
> +        return "tcg";
> +    }
> +    if (kvm_available()) {
> +        return "kvm";
> +    }
> +    if (xen_available()) {
> +        return "xen";
> +    }
> +    if (hax_available()) {
> +        return "hax";
> +    }
> +    /* configure makes sure we have at least one accelerator */
> +    g_assert(false);
> +    return "";
> +}
> +
>  void configure_accelerator(MachineState *ms)
>  {
>      const char *accel, *p;
> @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
>  
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (accel == NULL) {
> -        /* Use the default "accelerator", tcg */
> -        accel = "tcg";
> +        accel = default_accelerator();

It actually may be easier to just switch the default to
"tcg:kvm:xen:hax". Haven't tested that, though.

>      }
>  
>      p = accel;

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2adfff..386e6e945d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -66,7 +66,8 @@ Supported machine properties are:
>  @table @option
>  @item accel=@var{accels1}[:@var{accels2}[:...]]
>  This is used to enable an accelerator. Depending on the target architecture,
> -kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
> +kvm, xen, hax or tcg can be available. By default, the first one available
> +out of tcg, kvm, xen, hax (in that order) is used. If there is
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @item kernel_irqchip=on|off
> @@ -126,7 +127,8 @@ STEXI
>  @item -accel @var{name}[,prop=@var{value}[,...]]
>  @findex -accel
>  This is used to enable an accelerator. Depending on the target architecture,
> -kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
> +kvm, xen, hax or tcg can be available. By default, the first one available
> +out of tcg, kvm, xen, hax (in that order) is used. If there is
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @table @option

These changes would still apply, as would the question about what to do
with the iotest.

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Peter Maydell 6 years, 7 months ago
On 6 September 2017 at 12:29, Cornelia Huck <cohuck@redhat.com> wrote:
> On Wed,  6 Sep 2017 11:49:27 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>> @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
>>
>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>>      if (accel == NULL) {
>> -        /* Use the default "accelerator", tcg */
>> -        accel = "tcg";
>> +        accel = default_accelerator();
>
> It actually may be easier to just switch the default to
> "tcg:kvm:xen:hax". Haven't tested that, though.

Does it make sense to include Xen in the default list?
I don't know much about Xen but I was under the impression
that it's a special purpose thing that you can only use
as part of a Xen setup, whereas tcg, kvm, hax are all
more-or-less interchangeable ways to run a VM under a
Linux/etc host. Do I have the wrong end of the Xen stick?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Cornelia Huck 6 years, 7 months ago
On Wed, 6 Sep 2017 15:35:16 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 6 September 2017 at 12:29, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Wed,  6 Sep 2017 11:49:27 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:  
> >> @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
> >>
> >>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> >>      if (accel == NULL) {
> >> -        /* Use the default "accelerator", tcg */
> >> -        accel = "tcg";
> >> +        accel = default_accelerator();  
> >
> > It actually may be easier to just switch the default to
> > "tcg:kvm:xen:hax". Haven't tested that, though.  
> 
> Does it make sense to include Xen in the default list?
> I don't know much about Xen but I was under the impression
> that it's a special purpose thing that you can only use
> as part of a Xen setup, whereas tcg, kvm, hax are all
> more-or-less interchangeable ways to run a VM under a
> Linux/etc host. Do I have the wrong end of the Xen stick?

I'm unfortunately not familiar with xen either.

I was going with what configure considers as an available accelerator
(in supported_target()). FWIW, I can build x86_64 with xen as the only
available accelerator, but have not been able to get it to work (in all
of the 5 minutes I tried). I can't get it to build with hax as the only
accelerator (although configure does not complain; I might be missing
something).

Switching the default from "tcg" to "tcg:kvm" would already fix the
problem for s390x ;), but maybe someone else has a better idea?

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Paolo Bonzini 6 years, 7 months ago
On 06/09/2017 16:35, Peter Maydell wrote:
>>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>>>      if (accel == NULL) {
>>> -        /* Use the default "accelerator", tcg */
>>> -        accel = "tcg";
>>> +        accel = default_accelerator();
>> It actually may be easier to just switch the default to
>> "tcg:kvm:xen:hax". Haven't tested that, though.
> Does it make sense to include Xen in the default list?
> I don't know much about Xen but I was under the impression
> that it's a special purpose thing that you can only use
> as part of a Xen setup, whereas tcg, kvm, hax are all
> more-or-less interchangeable ways to run a VM under a
> Linux/etc host. Do I have the wrong end of the Xen stick?

Yes, that is correct (in fact, -xen-domid is required too).

Paolo

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Cornelia Huck 6 years, 7 months ago
On Mon, 11 Sep 2017 13:48:46 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 06/09/2017 16:35, Peter Maydell wrote:
> >>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> >>>      if (accel == NULL) {
> >>> -        /* Use the default "accelerator", tcg */
> >>> -        accel = "tcg";
> >>> +        accel = default_accelerator();  
> >> It actually may be easier to just switch the default to
> >> "tcg:kvm:xen:hax". Haven't tested that, though.  
> > Does it make sense to include Xen in the default list?
> > I don't know much about Xen but I was under the impression
> > that it's a special purpose thing that you can only use
> > as part of a Xen setup, whereas tcg, kvm, hax are all
> > more-or-less interchangeable ways to run a VM under a
> > Linux/etc host. Do I have the wrong end of the Xen stick?  
> 
> Yes, that is correct (in fact, -xen-domid is required too).

OK, so we should use "tcg:kvm:hax"?

(Not sure how useful the hax statement is, I'm not familiar with that
one.)

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Paolo Bonzini 6 years, 7 months ago
On 11/09/2017 13:51, Cornelia Huck wrote:
> On Mon, 11 Sep 2017 13:48:46 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 06/09/2017 16:35, Peter Maydell wrote:
>>>>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>>>>>      if (accel == NULL) {
>>>>> -        /* Use the default "accelerator", tcg */
>>>>> -        accel = "tcg";
>>>>> +        accel = default_accelerator();  
>>>> It actually may be easier to just switch the default to
>>>> "tcg:kvm:xen:hax". Haven't tested that, though.  
>>> Does it make sense to include Xen in the default list?
>>> I don't know much about Xen but I was under the impression
>>> that it's a special purpose thing that you can only use
>>> as part of a Xen setup, whereas tcg, kvm, hax are all
>>> more-or-less interchangeable ways to run a VM under a
>>> Linux/etc host. Do I have the wrong end of the Xen stick?  
>>
>> Yes, that is correct (in fact, -xen-domid is required too).
> 
> OK, so we should use "tcg:kvm:hax"?
> 
> (Not sure how useful the hax statement is, I'm not familiar with that
> one.)

Yes.  When we move KVM to the front, however, HAX and the upcoming HVF
accelerator probably should stay in the back because they are less
tested than TCG (e.g. HAX doesn't support -cpu, HVF will not support
live migration in the first iteration, etc.).

Paolo

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Kevin Wolf 6 years, 7 months ago
Am 06.09.2017 um 13:29 hat Cornelia Huck geschrieben:
> On Wed,  6 Sep 2017 11:49:27 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > configure_accelerator() falls back to tcg if no accelerator has
> > been specified. Formerly, we could be sure that tcg is always
> > available; however, with --disable-tcg, this is not longer true,
> > and you are not able to start qemu without explicitly specifying
> > another accelerator on those builds.
> > 
> > Instead, choose an accelerator in the order tcg->kvm->xen->hax.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > RFC mainly because this breaks iotest 186 in a different way on a
> > tcg-less x86_64 build: Before, it fails with "-machine accel=tcg: No
> > accelerator found"; afterwards, there seems to be a difference in
> > output due to different autogenerated devices. Not sure how to handle
> > that.
> > 
> > cc:ing some hopefully interested folks (-ENOMAINTAINER again).
> > 
> > ---
> >  accel/accel.c              | 22 ++++++++++++++++++++--
> >  arch_init.c                | 17 +++++++++++++++++
> >  include/sysemu/arch_init.h |  2 ++
> >  qemu-options.hx            |  6 ++++--
> >  4 files changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/accel/accel.c b/accel/accel.c
> > index 8ae40e1e13..26a3f32627 100644
> > --- a/accel/accel.c
> > +++ b/accel/accel.c
> > @@ -68,6 +68,25 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
> >      return ret;
> >  }
> >  
> > +static const char *default_accelerator(void)
> > +{
> > +    if (tcg_available()) {
> > +        return "tcg";
> > +    }
> > +    if (kvm_available()) {
> > +        return "kvm";
> > +    }
> > +    if (xen_available()) {
> > +        return "xen";
> > +    }
> > +    if (hax_available()) {
> > +        return "hax";
> > +    }
> > +    /* configure makes sure we have at least one accelerator */
> > +    g_assert(false);
> > +    return "";
> > +}
> > +
> >  void configure_accelerator(MachineState *ms)
> >  {
> >      const char *accel, *p;
> > @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
> >  
> >      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> >      if (accel == NULL) {
> > -        /* Use the default "accelerator", tcg */
> > -        accel = "tcg";
> > +        accel = default_accelerator();
> 
> It actually may be easier to just switch the default to
> "tcg:kvm:xen:hax". Haven't tested that, though.

This would have been my first thought and looks a bit simpler, so if
it works, I'd go for it.

But the real reason why I'm replying: Should we add changing the default
to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
that intentionally uses TCG occasionally, but I think the majority of
users wants to use KVM (or whatever the fastest option is on their
system) whenever it is available.

Kevin

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Thomas Huth 6 years, 7 months ago
On 07.09.2017 10:11, Kevin Wolf wrote:
[...]
> But the real reason why I'm replying: Should we add changing the default
> to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
> that intentionally uses TCG occasionally, but I think the majority of
> users wants to use KVM (or whatever the fastest option is on their
> system) whenever it is available.

If you consider how often people are getting this wrong (they want to
use KVM but end up with TCG in the first try), I think that's a good idea.

Maybe we should start a Wiki page where we collect ideas for QEMU 3.0?

 Thomas

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Cornelia Huck 6 years, 7 months ago
On Thu, 7 Sep 2017 10:14:27 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 07.09.2017 10:11, Kevin Wolf wrote:
> [...]
> > But the real reason why I'm replying: Should we add changing the default
> > to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
> > that intentionally uses TCG occasionally, but I think the majority of
> > users wants to use KVM (or whatever the fastest option is on their
> > system) whenever it is available.  
> 
> If you consider how often people are getting this wrong (they want to
> use KVM but end up with TCG in the first try), I think that's a good idea.

Agreed.

I'm wondering what that means for our tests, though. Some of them work
slightly different under tcg or kvm (cf. iotest 186, as referenced in
the original mail), and sometimes you'll probably explicitly want to
exercise tcg. That does not speak against the change, but we probably
need to look at what we want in more detail.

> Maybe we should start a Wiki page where we collect ideas for QEMU 3.0?

Also a good idea.

[Do we already have any idea about the timeframe for 3.0?]

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Kevin Wolf 6 years, 7 months ago
Am 07.09.2017 um 10:25 hat Cornelia Huck geschrieben:
> On Thu, 7 Sep 2017 10:14:27 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 07.09.2017 10:11, Kevin Wolf wrote:
> > [...]
> > > But the real reason why I'm replying: Should we add changing the default
> > > to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
> > > that intentionally uses TCG occasionally, but I think the majority of
> > > users wants to use KVM (or whatever the fastest option is on their
> > > system) whenever it is available.  
> > 
> > If you consider how often people are getting this wrong (they want to
> > use KVM but end up with TCG in the first try), I think that's a good idea.
> 
> Agreed.
> 
> I'm wondering what that means for our tests, though. Some of them work
> slightly different under tcg or kvm (cf. iotest 186, as referenced in
> the original mail), and sometimes you'll probably explicitly want to
> exercise tcg. That does not speak against the change, but we probably
> need to look at what we want in more detail.

This is a bug in test 186, and probably 172, too. Normally, we use the
options from ./common:

    export QEMU_OPTIONS="-nodefaults -machine accel=qtest"

However, these two test cases overwrite QEMU_OPTIONS and neglect to add
a '-machine accel=qtest' option manually.

Kevin

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Paolo Bonzini 6 years, 7 months ago
On 07/09/2017 10:11, Kevin Wolf wrote:
> Am 06.09.2017 um 13:29 hat Cornelia Huck geschrieben:
>> On Wed,  6 Sep 2017 11:49:27 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> configure_accelerator() falls back to tcg if no accelerator has
>>> been specified. Formerly, we could be sure that tcg is always
>>> available; however, with --disable-tcg, this is not longer true,
>>> and you are not able to start qemu without explicitly specifying
>>> another accelerator on those builds.
>>>
>>> Instead, choose an accelerator in the order tcg->kvm->xen->hax.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> RFC mainly because this breaks iotest 186 in a different way on a
>>> tcg-less x86_64 build: Before, it fails with "-machine accel=tcg: No
>>> accelerator found"; afterwards, there seems to be a difference in
>>> output due to different autogenerated devices. Not sure how to handle
>>> that.
>>>
>>> cc:ing some hopefully interested folks (-ENOMAINTAINER again).
>>>
>>> ---
>>>  accel/accel.c              | 22 ++++++++++++++++++++--
>>>  arch_init.c                | 17 +++++++++++++++++
>>>  include/sysemu/arch_init.h |  2 ++
>>>  qemu-options.hx            |  6 ++++--
>>>  4 files changed, 43 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/accel/accel.c b/accel/accel.c
>>> index 8ae40e1e13..26a3f32627 100644
>>> --- a/accel/accel.c
>>> +++ b/accel/accel.c
>>> @@ -68,6 +68,25 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>>>      return ret;
>>>  }
>>>  
>>> +static const char *default_accelerator(void)
>>> +{
>>> +    if (tcg_available()) {
>>> +        return "tcg";
>>> +    }
>>> +    if (kvm_available()) {
>>> +        return "kvm";
>>> +    }
>>> +    if (xen_available()) {
>>> +        return "xen";
>>> +    }
>>> +    if (hax_available()) {
>>> +        return "hax";
>>> +    }
>>> +    /* configure makes sure we have at least one accelerator */
>>> +    g_assert(false);
>>> +    return "";
>>> +}
>>> +
>>>  void configure_accelerator(MachineState *ms)
>>>  {
>>>      const char *accel, *p;
>>> @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
>>>  
>>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>>>      if (accel == NULL) {
>>> -        /* Use the default "accelerator", tcg */
>>> -        accel = "tcg";
>>> +        accel = default_accelerator();
>>
>> It actually may be easier to just switch the default to
>> "tcg:kvm:xen:hax". Haven't tested that, though.
> 
> This would have been my first thought and looks a bit simpler, so if
> it works, I'd go for it.
> 
> But the real reason why I'm replying: Should we add changing the default
> to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
> that intentionally uses TCG occasionally, but I think the majority of
> users wants to use KVM (or whatever the fastest option is on their
> system) whenever it is available.

Yes, the only thing to be clarified is the default family/model/stepping
for "-accel kvm".  "-cpu qemu64" with KVM uses an AMD f/m/s and Intel as
the vendor, and some software (IIRC the GMP testsuite or something like
that) doesn't like it.  We should probably change qemu64 to a core2
f/m/s or something like that when running under KVM.  Eduardo?

Paolo


Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Eduardo Habkost 6 years, 7 months ago
On Mon, Sep 11, 2017 at 01:51:54PM +0200, Paolo Bonzini wrote:
> On 07/09/2017 10:11, Kevin Wolf wrote:
> > Am 06.09.2017 um 13:29 hat Cornelia Huck geschrieben:
> >> On Wed,  6 Sep 2017 11:49:27 +0200
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >>> configure_accelerator() falls back to tcg if no accelerator has
> >>> been specified. Formerly, we could be sure that tcg is always
> >>> available; however, with --disable-tcg, this is not longer true,
> >>> and you are not able to start qemu without explicitly specifying
> >>> another accelerator on those builds.
> >>>
> >>> Instead, choose an accelerator in the order tcg->kvm->xen->hax.
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>
> >>> RFC mainly because this breaks iotest 186 in a different way on a
> >>> tcg-less x86_64 build: Before, it fails with "-machine accel=tcg: No
> >>> accelerator found"; afterwards, there seems to be a difference in
> >>> output due to different autogenerated devices. Not sure how to handle
> >>> that.
> >>>
> >>> cc:ing some hopefully interested folks (-ENOMAINTAINER again).
> >>>
> >>> ---
> >>>  accel/accel.c              | 22 ++++++++++++++++++++--
> >>>  arch_init.c                | 17 +++++++++++++++++
> >>>  include/sysemu/arch_init.h |  2 ++
> >>>  qemu-options.hx            |  6 ++++--
> >>>  4 files changed, 43 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/accel/accel.c b/accel/accel.c
> >>> index 8ae40e1e13..26a3f32627 100644
> >>> --- a/accel/accel.c
> >>> +++ b/accel/accel.c
> >>> @@ -68,6 +68,25 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
> >>>      return ret;
> >>>  }
> >>>  
> >>> +static const char *default_accelerator(void)
> >>> +{
> >>> +    if (tcg_available()) {
> >>> +        return "tcg";
> >>> +    }
> >>> +    if (kvm_available()) {
> >>> +        return "kvm";
> >>> +    }
> >>> +    if (xen_available()) {
> >>> +        return "xen";
> >>> +    }
> >>> +    if (hax_available()) {
> >>> +        return "hax";
> >>> +    }
> >>> +    /* configure makes sure we have at least one accelerator */
> >>> +    g_assert(false);
> >>> +    return "";
> >>> +}
> >>> +
> >>>  void configure_accelerator(MachineState *ms)
> >>>  {
> >>>      const char *accel, *p;
> >>> @@ -79,8 +98,7 @@ void configure_accelerator(MachineState *ms)
> >>>  
> >>>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> >>>      if (accel == NULL) {
> >>> -        /* Use the default "accelerator", tcg */
> >>> -        accel = "tcg";
> >>> +        accel = default_accelerator();
> >>
> >> It actually may be easier to just switch the default to
> >> "tcg:kvm:xen:hax". Haven't tested that, though.
> > 
> > This would have been my first thought and looks a bit simpler, so if
> > it works, I'd go for it.
> > 
> > But the real reason why I'm replying: Should we add changing the default
> > to "kvm:tcg" to the list of planned 3.0 changes? I am part of the group
> > that intentionally uses TCG occasionally, but I think the majority of
> > users wants to use KVM (or whatever the fastest option is on their
> > system) whenever it is available.
> 
> Yes, the only thing to be clarified is the default family/model/stepping
> for "-accel kvm".  "-cpu qemu64" with KVM uses an AMD f/m/s and Intel as
> the vendor, and some software (IIRC the GMP testsuite or something like
> that) doesn't like it.  We should probably change qemu64 to a core2
> f/m/s or something like that when running under KVM.  Eduardo?

The current f/m/s was supposed to make sense for both AMD and
Intel CPUs, to avoid requiring per-cpu-vendor defaults.  If we
find a more recent f/m/s combination that still makes some sense
for both vendors, changing it will be very simple.

Long term, however, we should probably add per-cpu-vendor
defaults to make this more flexible.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC] accel: default to an actually available accelerator
Posted by Richard Henderson 6 years, 7 months ago
On 09/06/2017 02:49 AM, Cornelia Huck wrote:
> +    /* configure makes sure we have at least one accelerator */
> +    g_assert(false);
> +    return "";

g_assert_not_reached();

Though I do like the follow-up idea of "t:k:x:h".


r~