[Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'

[Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'

[Thomas Huth]

Since 'hax' is a possible accelerator nowadays, too, the '-accel'
option should support it and we should mention this accelerator
in the documentation, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Use qemu_opt_set() instead of qemu_opts_parse_noisily()
 - Improve the documentation of the -accel option

 qemu-options.hx | 18 +++++++++---------
 vl.c            | 23 +++++++++--------------
 2 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 787b9c3..21f8ff2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -31,7 +31,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "-machine [type=]name[,prop[=value][,...]]\n"
     "                selects emulated machine ('-machine help' for list)\n"
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
-    "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
+    "                supported accelerators are kvm, xen, hax or tcg (default: tcg)\n"
     "                kernel_irqchip=on|off|split controls accelerated irqchip support (default=off)\n"
     "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
     "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
@@ -52,9 +52,9 @@ available machines. 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, or tcg can be available. By default, tcg is used. If there is more
-than one accelerator specified, the next one is used if the previous one fails
-to initialize.
+kvm, xen, hax or tcg can be available. By default, tcg 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
 Controls in-kernel irqchip support for the chosen accelerator when available.
 @item gfx_passthru=on|off
@@ -97,15 +97,15 @@ ETEXI
 
 DEF("accel", HAS_ARG, QEMU_OPTION_accel,
     "-accel [accel=]accelerator[,thread=single|multi]\n"
-    "               select accelerator ('-accel help for list')\n"
-    "               thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL)
+    "                select accelerator (kvm, xen, hax or tcg; use 'help' for a list)\n"
+    "                thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL)
 STEXI
 @item -accel @var{name}[,prop=@var{value}[,...]]
 @findex -accel
 This is used to enable an accelerator. Depending on the target architecture,
-kvm, xen, or tcg can be available. By default, tcg is used. If there is more
-than one accelerator specified, the next one is used if the previous one fails
-to initialize.
+kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
+more than one accelerator specified, the next one is used if the previous one
+fails to initialize.
 @table @option
 @item thread=single|multi
 Controls number of TCG threads. When the TCG is multi-threaded there will be one
diff --git a/vl.c b/vl.c
index f46e070..0a1b931 100644
--- a/vl.c
+++ b/vl.c
@@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
                 qdev_prop_register_global(&kvm_pit_lost_tick_policy);
                 break;
             }
-            case QEMU_OPTION_accel:
+            case QEMU_OPTION_accel: {
+                QemuOpts *accel_opts;
+
                 accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
                                                      optarg, true);
                 optarg = qemu_opt_get(accel_opts, "accel");
-
-                olist = qemu_find_opts("machine");
-                if (strcmp("kvm", optarg) == 0) {
-                    qemu_opts_parse_noisily(olist, "accel=kvm", false);
-                } else if (strcmp("xen", optarg) == 0) {
-                    qemu_opts_parse_noisily(olist, "accel=xen", false);
-                } else if (strcmp("tcg", optarg) == 0) {
-                    qemu_opts_parse_noisily(olist, "accel=tcg", false);
-                } else {
-                    if (!is_help_option(optarg)) {
-                        error_printf("Unknown accelerator: %s", optarg);
-                    }
-                    error_printf("Supported accelerators: kvm, xen, tcg\n");
+                if (!optarg || is_help_option(optarg)) {
+                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
                     exit(1);
                 }
+                accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
+                                              false, &error_abort);
+                qemu_opt_set(accel_opts, "accel", optarg, &error_abort);
                 break;
+            }
             case QEMU_OPTION_usb:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse_noisily(olist, "usb=on", false);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Since 'hax' is a possible accelerator nowadays, too, the '-accel'
> option should support it and we should mention this accelerator
> in the documentation, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Use qemu_opt_set() instead of qemu_opts_parse_noisily()
>  - Improve the documentation of the -accel option
>
>  qemu-options.hx | 18 +++++++++---------
>  vl.c            | 23 +++++++++--------------
>  2 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 787b9c3..21f8ff2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -31,7 +31,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "-machine [type=]name[,prop[=value][,...]]\n"
>      "                selects emulated machine ('-machine help' for list)\n"
>      "                property accel=accel1[:accel2[:...]] selects accelerator\n"
> -    "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
> +    "                supported accelerators are kvm, xen, hax or tcg (default: tcg)\n"

The list of accelerators is approaching a length where sorting may make
sense.  You decide.

>      "                kernel_irqchip=on|off|split controls accelerated irqchip support (default=off)\n"
>      "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
> @@ -52,9 +52,9 @@ available machines. 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, or tcg can be available. By default, tcg is used. If there is more
> -than one accelerator specified, the next one is used if the previous one fails
> -to initialize.
> +kvm, xen, hax or tcg can be available. By default, tcg 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
>  Controls in-kernel irqchip support for the chosen accelerator when available.
>  @item gfx_passthru=on|off
> @@ -97,15 +97,15 @@ ETEXI
>  
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>      "-accel [accel=]accelerator[,thread=single|multi]\n"
> -    "               select accelerator ('-accel help for list')\n"
> -    "               thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL)
> +    "                select accelerator (kvm, xen, hax or tcg; use 'help' for a list)\n"
> +    "                thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL)

Thanks for fixing help indentation while there.

>  STEXI
>  @item -accel @var{name}[,prop=@var{value}[,...]]
>  @findex -accel
>  This is used to enable an accelerator. Depending on the target architecture,
> -kvm, xen, or tcg can be available. By default, tcg is used. If there is more
> -than one accelerator specified, the next one is used if the previous one fails
> -to initialize.
> +kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
> +more than one accelerator specified, the next one is used if the previous one
> +fails to initialize.
>  @table @option
>  @item thread=single|multi
>  Controls number of TCG threads. When the TCG is multi-threaded there will be one
> diff --git a/vl.c b/vl.c
> index f46e070..0a1b931 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
>                  qdev_prop_register_global(&kvm_pit_lost_tick_policy);
>                  break;
>              }
> -            case QEMU_OPTION_accel:
> +            case QEMU_OPTION_accel: {
> +                QemuOpts *accel_opts;

Doesn't this shadow the @accel_opts declared in main()'s outermost
scope?

What's wrong with using @opts for its intended purpose here?

> +
>                  accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
>                                                       optarg, true);
>                  optarg = qemu_opt_get(accel_opts, "accel");

Abusing optarg this way is not nice.  It should be assigned to only once
per iteration, via lookup_opt().  Not your fault, but perhaps you'd like
to clean it up anyway.

Even more pointless abuse in case QEMU_OPTION_rotate.  That one should
be converted to qemu_strtol().  Separate patch, and not required to get
my blessings for this one.

> -
> -                olist = qemu_find_opts("machine");
> -                if (strcmp("kvm", optarg) == 0) {
> -                    qemu_opts_parse_noisily(olist, "accel=kvm", false);
> -                } else if (strcmp("xen", optarg) == 0) {
> -                    qemu_opts_parse_noisily(olist, "accel=xen", false);
> -                } else if (strcmp("tcg", optarg) == 0) {
> -                    qemu_opts_parse_noisily(olist, "accel=tcg", false);
> -                } else {
> -                    if (!is_help_option(optarg)) {
> -                        error_printf("Unknown accelerator: %s", optarg);
> -                    }
> -                    error_printf("Supported accelerators: kvm, xen, tcg\n");
> +                if (!optarg || is_help_option(optarg)) {
> +                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
>                      exit(1);

Not your patch's fault, but trivial to fix now: this should be exit(0),
as asking for help is not an error.

>                  }
> +                accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
> +                                              false, &error_abort);
> +                qemu_opt_set(accel_opts, "accel", optarg, &error_abort);

The switch from qemu_opt_set() to qemu_opts_parse_noisily() makes
desugaring less explicit, but also less repetitive.  Okay.

Hmm, where did the "Unknown accelerator" error go?  Aha:

    $ qemu-system-x86_64 -accel xxx
    "xxx" accelerator not found.
    No accelerator found!

Two error messages instead of one, and I don't care for the overexcited
'!', but that's all beyond this patch's scope.

>                  break;
> +            }
>              case QEMU_OPTION_usb:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse_noisily(olist, "usb=on", false);

Re: [Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'

[Emilio G. Cota]

On Thu, May 04, 2017 at 09:11:50 +0200, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
(snip)
> >  STEXI
> >  @item -accel @var{name}[,prop=@var{value}[,...]]
> >  @findex -accel
> >  This is used to enable an accelerator. Depending on the target architecture,
> > -kvm, xen, or tcg can be available. By default, tcg is used. If there is more
> > -than one accelerator specified, the next one is used if the previous one fails
> > -to initialize.
> > +kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
> > +more than one accelerator specified, the next one is used if the previous one
> > +fails to initialize.
> >  @table @option
> >  @item thread=single|multi
> >  Controls number of TCG threads. When the TCG is multi-threaded there will be one
> > diff --git a/vl.c b/vl.c
> > index f46e070..0a1b931 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
> >                  qdev_prop_register_global(&kvm_pit_lost_tick_policy);
> >                  break;
> >              }
> > -            case QEMU_OPTION_accel:
> > +            case QEMU_OPTION_accel: {
> > +                QemuOpts *accel_opts;
> 
> Doesn't this shadow the @accel_opts declared in main()'s outermost
> scope?

Yes, it does :( Unfortunately Markus' review slipped through the
cracks and this patch ended up upstream (bde4d9205). It causes
a regression that breaks qemu_tcg_configure(accel_opts)
since now accel_opts is always NULL. That is, in `-accel [..],thread=foo'
foo is ignored.

		Emilio

Re: [Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'

[Thomas Huth]

On 07.06.2017 22:14, Emilio G. Cota wrote:
> On Thu, May 04, 2017 at 09:11:50 +0200, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
> (snip)
>>>  STEXI
>>>  @item -accel @var{name}[,prop=@var{value}[,...]]
>>>  @findex -accel
>>>  This is used to enable an accelerator. Depending on the target architecture,
>>> -kvm, xen, or tcg can be available. By default, tcg is used. If there is more
>>> -than one accelerator specified, the next one is used if the previous one fails
>>> -to initialize.
>>> +kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
>>> +more than one accelerator specified, the next one is used if the previous one
>>> +fails to initialize.
>>>  @table @option
>>>  @item thread=single|multi
>>>  Controls number of TCG threads. When the TCG is multi-threaded there will be one
>>> diff --git a/vl.c b/vl.c
>>> index f46e070..0a1b931 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
>>>                  qdev_prop_register_global(&kvm_pit_lost_tick_policy);
>>>                  break;
>>>              }
>>> -            case QEMU_OPTION_accel:
>>> +            case QEMU_OPTION_accel: {
>>> +                QemuOpts *accel_opts;
>>
>> Doesn't this shadow the @accel_opts declared in main()'s outermost
>> scope?
> 
> Yes, it does :( Unfortunately Markus' review slipped through the
> cracks and this patch ended up upstream (bde4d9205). It causes
> a regression that breaks qemu_tcg_configure(accel_opts)
> since now accel_opts is always NULL. That is, in `-accel [..],thread=foo'
> foo is ignored.

Ouch, not sure how I managed to miss that ... big sorry, I'll send a
patch for fixing that mess...

 Thomas