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

Thomas Huth posted 1 patch 110 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1493718721-29774-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
qemu-options.hx |  8 ++++----
vl.c            | 24 ++++++++++--------------
2 files changed, 14 insertions(+), 18 deletions(-)

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

Posted by Thomas Huth 110 weeks ago
Since 'hax' is a possible accelerator nowadays, too, the '-accel'
option should support it and we should mention it in the documentation,
too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qemu-options.hx |  8 ++++----
 vl.c            | 24 ++++++++++--------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 787b9c3..c7b1d2d 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
diff --git a/vl.c b/vl.c
index f46e070..d5e88fb 100644
--- a/vl.c
+++ b/vl.c
@@ -3725,26 +3725,22 @@ 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: {
+                char *mstr;
+
                 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 (is_help_option(optarg)) {
+                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
                     exit(1);
                 }
+                mstr = g_strdup_printf("accel=%s", optarg);
+                olist = qemu_find_opts("machine");
+                qemu_opts_parse_noisily(olist, mstr, false);
+                g_free(mstr);
                 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] Fix the -accel parameter and the documentation for 'hax'

Posted by Paolo Bonzini 110 weeks ago

On 02/05/2017 11:52, Thomas Huth wrote:
> Since 'hax' is a possible accelerator nowadays, too, the '-accel'
> option should support it and we should mention it in the documentation,
> too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qemu-options.hx |  8 ++++----
>  vl.c            | 24 ++++++++++--------------
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 787b9c3..c7b1d2d 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
> diff --git a/vl.c b/vl.c
> index f46e070..d5e88fb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3725,26 +3725,22 @@ 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: {
> +                char *mstr;
> +
>                  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 (is_help_option(optarg)) {
> +                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
>                      exit(1);
>                  }
> +                mstr = g_strdup_printf("accel=%s", optarg);
> +                olist = qemu_find_opts("machine");
> +                qemu_opts_parse_noisily(olist, mstr, false);

Thanks for the report.  I think this has bad side effects when one
specifies an invalid accelerator such as -accel tcg,,usb=on.  Eduardo
has more plans for -accel, I think.

Paolo

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

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

Posted by Eduardo Habkost 110 weeks ago
On Tue, May 02, 2017 at 02:58:35PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/05/2017 11:52, Thomas Huth wrote:
> > Since 'hax' is a possible accelerator nowadays, too, the '-accel'
> > option should support it and we should mention it in the documentation,
> > too.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  qemu-options.hx |  8 ++++----
> >  vl.c            | 24 ++++++++++--------------
> >  2 files changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 787b9c3..c7b1d2d 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
> > diff --git a/vl.c b/vl.c
> > index f46e070..d5e88fb 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3725,26 +3725,22 @@ 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: {
> > +                char *mstr;
> > +
> >                  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 {

Getting rid of this hardcoded list is welcome.

> > -                    if (!is_help_option(optarg)) {
> > -                        error_printf("Unknown accelerator: %s", optarg);
> > -                    }
> > -                    error_printf("Supported accelerators: kvm, xen, tcg\n");
> > +                if (is_help_option(optarg)) {
> > +                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");

This should be replaced by object_class_get_list()- or
object_class_for_each()-based loop, so we don't need to maintain
the list manually. But that's for a follow-up patch.

> >                      exit(1);
> >                  }
> > +                mstr = g_strdup_printf("accel=%s", optarg);
> > +                olist = qemu_find_opts("machine");
> > +                qemu_opts_parse_noisily(olist, mstr, false);
> 
> Thanks for the report.  I think this has bad side effects when one
> specifies an invalid accelerator such as -accel tcg,,usb=on.  Eduardo
> has more plans for -accel, I think.

We can use qemu_opt_set() here to avoid this problem.

I wasn't even aware that we had a new -accel option. Thanks for
CCing me. I believe -accel should be mapped to
"-object ...-accel" eventually, but that's for the future.


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

-- 
Eduardo