[06/16] vl: configure accelerators from -accel options
diff mbox series

Message ID 1573655945-14912-7-git-send-email-pbonzini@redhat.com
State New
Headers show
Series
  • Complete the implementation of -accel
Related show

Commit Message

Paolo Bonzini Nov. 13, 2019, 2:38 p.m. UTC
Drop the "accel" property from MachineState, and instead desugar
"-machine accel=" to a list of "-accel" options.

This has a semantic change due to removing merge_lists from -accel.
For example:

- "-accel kvm -accel tcg" all but ignored "-accel kvm".  This is a bugfix.

- "-accel kvm -accel thread=single" ignored "thread=single", since it
  applied the option to KVM.  Now it fails due to not specifying the
  accelerator on "-accel thread=single".

- "-accel tcg -accel thread=single" chose single-threaded TCG, while now
  it will fail due to not specifying the accelerator on "-accel
  thread=single".

Also, "-machine accel" and "-accel" become incompatible.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/machine.c   | 21 ------------
 include/hw/boards.h |  1 -
 vl.c                | 93 +++++++++++++++++++++++++++++++----------------------
 3 files changed, 54 insertions(+), 61 deletions(-)

Comments

Wainer dos Santos Moschetta Nov. 18, 2019, 5:59 p.m. UTC | #1
On 11/13/19 12:38 PM, Paolo Bonzini wrote:
> Drop the "accel" property from MachineState, and instead desugar
> "-machine accel=" to a list of "-accel" options.
>
> This has a semantic change due to removing merge_lists from -accel.
> For example:
>
> - "-accel kvm -accel tcg" all but ignored "-accel kvm".  This is a bugfix.
>
> - "-accel kvm -accel thread=single" ignored "thread=single", since it
>    applied the option to KVM.  Now it fails due to not specifying the
>    accelerator on "-accel thread=single".
>
> - "-accel tcg -accel thread=single" chose single-threaded TCG, while now
>    it will fail due to not specifying the accelerator on "-accel
>    thread=single".
>
> Also, "-machine accel" and "-accel" become incompatible.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/core/machine.c   | 21 ------------
>   include/hw/boards.h |  1 -
>   vl.c                | 93 +++++++++++++++++++++++++++++++----------------------
>   3 files changed, 54 insertions(+), 61 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1689ad3..45ddfb6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -173,21 +173,6 @@ GlobalProperty hw_compat_2_1[] = {
>   };
>   const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
>   
> -static char *machine_get_accel(Object *obj, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    return g_strdup(ms->accel);
> -}
> -
> -static void machine_set_accel(Object *obj, const char *value, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    g_free(ms->accel);
> -    ms->accel = g_strdup(value);
> -}
> -
>   static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
>                                          const char *name, void *opaque,
>                                          Error **errp)
> @@ -808,11 +793,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>       mc->numa_mem_align_shift = 23;
>       mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
>   
> -    object_class_property_add_str(oc, "accel",
> -        machine_get_accel, machine_set_accel, &error_abort);
> -    object_class_property_set_description(oc, "accel",
> -        "Accelerator list", &error_abort);
> -
>       object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>           NULL, machine_set_kernel_irqchip,
>           NULL, NULL, &error_abort);
> @@ -971,7 +951,6 @@ static void machine_finalize(Object *obj)
>   {
>       MachineState *ms = MACHINE(obj);
>   
> -    g_free(ms->accel);
>       g_free(ms->kernel_filename);
>       g_free(ms->initrd_filename);
>       g_free(ms->kernel_cmdline);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index de45087..36fcbda 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -275,7 +275,6 @@ struct MachineState {
>   
>       /*< public >*/
>   
> -    char *accel;
>       bool kernel_irqchip_allowed;
>       bool kernel_irqchip_required;
>       bool kernel_irqchip_split;
> diff --git a/vl.c b/vl.c
> index b93217d..dd895db 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -293,7 +293,6 @@ static QemuOptsList qemu_accel_opts = {
>       .name = "accel",
>       .implied_opt_name = "accel",
>       .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
> -    .merge_lists = true,
>       .desc = {
>           {
>               .name = "accel",
> @@ -2651,6 +2650,11 @@ static int machine_set_property(void *opaque,
>           }
>       }
>   
> +    /* Legacy options do not correspond to MachineState properties.  */
> +    if (g_str_equal(qom_name, "accel")) {
> +        return 0;
> +    }
> +
>       r = object_parse_property_opt(opaque, name, value, "type", errp);
>       g_free(qom_name);
>       return r;
> @@ -2843,74 +2847,88 @@ static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
>   
>   static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>   {
> +    bool *p_init_failed = opaque;
> +    const char *acc = qemu_opt_get(opts, "accel");
> +    AccelClass *ac = accel_find(acc);
> +    int ret;
> +
> +    if (!ac) {
> +        return 0;
> +    }
> +    ret = accel_init_machine(ac, current_machine);
> +    if (ret < 0) {
> +        *p_init_failed = true;
> +        error_report("failed to initialize %s: %s",
> +                     acc, strerror(-ret));
> +        return 0;
> +    }
> +
>       if (tcg_enabled()) {
>           qemu_tcg_configure(opts, &error_fatal);
>       }
> -    return 0;
> +    return 1;
>   }
>   
>   static void configure_accelerators(const char *progname)
>   {
>       const char *accel;
>       char **accel_list, **tmp;
> -    int ret;
>       bool accel_initialised = false;
>       bool init_failed = false;
> -    AccelClass *acc = NULL;
>   
>       qemu_opts_foreach(qemu_find_opts("icount"),
>                         do_configure_icount, NULL, &error_fatal);
>   
>       accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> -    if (accel == NULL) {
> -        /* Select the default accelerator */
> -        if (!accel_find("tcg") && !accel_find("kvm")) {
> -            error_report("No accelerator selected and"
> -                         " no default accelerator available");

Perhaps on patch 07 you can also clarify this message, mentioning what's 
the default accelerator.

> -            exit(1);
> -        } else {
> -            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";
> +    if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
> +        if (accel == NULL) {
> +            /* Select the default accelerator */
> +            if (!accel_find("tcg") && !accel_find("kvm")) {
> +                error_report("No accelerator selected and"
> +                             " no default accelerator available");
> +                exit(1);
>               } else {
> -                accel = "tcg:kvm";
> +                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 {
> +                    accel = "tcg:kvm";
> +                }
>               }
>           }
> -    }
>   
> -    accel_list = g_strsplit(accel, ":", 0);
> +        accel_list = g_strsplit(accel, ":", 0);
>   
> -    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> -        acc = accel_find(*tmp);
> -        if (!acc) {
> -            continue;
> +        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +            /*
> +             * Filter invalid accelerators here, to prevent obscenities
> +             * such as "-machine accel=tcg,,thread=single".
> +             */
> +            if (accel_find(*tmp)) {
> +                qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
> +            }
>           }
> -        ret = accel_init_machine(acc, current_machine);
> -        if (ret < 0) {
> -            init_failed = true;
> -            error_report("failed to initialize %s: %s",
> -                         acc->name, strerror(-ret));
> -        } else {
> -            accel_initialised = true;
> +    } else {
> +        if (accel != NULL) {
> +            error_report("The -accel and \"-machine accel=\" options are incompatible");
> +            exit(1);
>           }
>       }
> -    g_strfreev(accel_list);
>   
> -    if (!accel_initialised) {
> +    if (!qemu_opts_foreach(qemu_find_opts("accel"),
> +                           do_configure_accelerator, &init_failed, &error_fatal)) {
>           if (!init_failed) {
> -            error_report("-machine accel=%s: No accelerator found", accel);
> +            error_report("no accelerator found");

Likewise.

Thanks,

Wainer

>           }
>           exit(1);
>       }
>   
>       if (init_failed) {
> -        error_report("Back to %s accelerator", acc->name);
> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> +        error_report("Back to %s accelerator", ac->name);
>       }
>   
> -    qemu_opts_foreach(qemu_find_opts("accel"),
> -                      do_configure_accelerator, NULL, &error_fatal);
> -
>       if (!tcg_enabled() && use_icount) {
>           error_report("-icount is not allowed with hardware virtualization");
>           exit(1);
> @@ -3618,9 +3636,6 @@ int main(int argc, char **argv, char **envp)
>                                    "use -M accel=... for now instead");
>                       exit(1);
>                   }
> -                opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
> -                                        false, &error_abort);
> -                qemu_opt_set(opts, "accel", optarg, &error_abort);
>                   break;
>               case QEMU_OPTION_usb:
>                   olist = qemu_find_opts("machine");

Patch
diff mbox series

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3..45ddfb6 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -173,21 +173,6 @@  GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
-static char *machine_get_accel(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->accel);
-}
-
-static void machine_set_accel(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->accel);
-    ms->accel = g_strdup(value);
-}
-
 static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
                                        const char *name, void *opaque,
                                        Error **errp)
@@ -808,11 +793,6 @@  static void machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_align_shift = 23;
     mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
 
-    object_class_property_add_str(oc, "accel",
-        machine_get_accel, machine_set_accel, &error_abort);
-    object_class_property_set_description(oc, "accel",
-        "Accelerator list", &error_abort);
-
     object_class_property_add(oc, "kernel-irqchip", "on|off|split",
         NULL, machine_set_kernel_irqchip,
         NULL, NULL, &error_abort);
@@ -971,7 +951,6 @@  static void machine_finalize(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
 
-    g_free(ms->accel);
     g_free(ms->kernel_filename);
     g_free(ms->initrd_filename);
     g_free(ms->kernel_cmdline);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087..36fcbda 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -275,7 +275,6 @@  struct MachineState {
 
     /*< public >*/
 
-    char *accel;
     bool kernel_irqchip_allowed;
     bool kernel_irqchip_required;
     bool kernel_irqchip_split;
diff --git a/vl.c b/vl.c
index b93217d..dd895db 100644
--- a/vl.c
+++ b/vl.c
@@ -293,7 +293,6 @@  static QemuOptsList qemu_accel_opts = {
     .name = "accel",
     .implied_opt_name = "accel",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
-    .merge_lists = true,
     .desc = {
         {
             .name = "accel",
@@ -2651,6 +2650,11 @@  static int machine_set_property(void *opaque,
         }
     }
 
+    /* Legacy options do not correspond to MachineState properties.  */
+    if (g_str_equal(qom_name, "accel")) {
+        return 0;
+    }
+
     r = object_parse_property_opt(opaque, name, value, "type", errp);
     g_free(qom_name);
     return r;
@@ -2843,74 +2847,88 @@  static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
 
 static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
 {
+    bool *p_init_failed = opaque;
+    const char *acc = qemu_opt_get(opts, "accel");
+    AccelClass *ac = accel_find(acc);
+    int ret;
+
+    if (!ac) {
+        return 0;
+    }
+    ret = accel_init_machine(ac, current_machine);
+    if (ret < 0) {
+        *p_init_failed = true;
+        error_report("failed to initialize %s: %s",
+                     acc, strerror(-ret));
+        return 0;
+    }
+
     if (tcg_enabled()) {
         qemu_tcg_configure(opts, &error_fatal);
     }
-    return 0;
+    return 1;
 }
 
 static void configure_accelerators(const char *progname)
 {
     const char *accel;
     char **accel_list, **tmp;
-    int ret;
     bool accel_initialised = false;
     bool init_failed = false;
-    AccelClass *acc = NULL;
 
     qemu_opts_foreach(qemu_find_opts("icount"),
                       do_configure_icount, NULL, &error_fatal);
 
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
-    if (accel == NULL) {
-        /* Select the default accelerator */
-        if (!accel_find("tcg") && !accel_find("kvm")) {
-            error_report("No accelerator selected and"
-                         " no default accelerator available");
-            exit(1);
-        } else {
-            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";
+    if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
+        if (accel == NULL) {
+            /* Select the default accelerator */
+            if (!accel_find("tcg") && !accel_find("kvm")) {
+                error_report("No accelerator selected and"
+                             " no default accelerator available");
+                exit(1);
             } else {
-                accel = "tcg:kvm";
+                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 {
+                    accel = "tcg:kvm";
+                }
             }
         }
-    }
 
-    accel_list = g_strsplit(accel, ":", 0);
+        accel_list = g_strsplit(accel, ":", 0);
 
-    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
-        acc = accel_find(*tmp);
-        if (!acc) {
-            continue;
+        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+            /*
+             * Filter invalid accelerators here, to prevent obscenities
+             * such as "-machine accel=tcg,,thread=single".
+             */
+            if (accel_find(*tmp)) {
+                qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
+            }
         }
-        ret = accel_init_machine(acc, current_machine);
-        if (ret < 0) {
-            init_failed = true;
-            error_report("failed to initialize %s: %s",
-                         acc->name, strerror(-ret));
-        } else {
-            accel_initialised = true;
+    } else {
+        if (accel != NULL) {
+            error_report("The -accel and \"-machine accel=\" options are incompatible");
+            exit(1);
         }
     }
-    g_strfreev(accel_list);
 
-    if (!accel_initialised) {
+    if (!qemu_opts_foreach(qemu_find_opts("accel"),
+                           do_configure_accelerator, &init_failed, &error_fatal)) {
         if (!init_failed) {
-            error_report("-machine accel=%s: No accelerator found", accel);
+            error_report("no accelerator found");
         }
         exit(1);
     }
 
     if (init_failed) {
-        error_report("Back to %s accelerator", acc->name);
+        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        error_report("Back to %s accelerator", ac->name);
     }
 
-    qemu_opts_foreach(qemu_find_opts("accel"),
-                      do_configure_accelerator, NULL, &error_fatal);
-
     if (!tcg_enabled() && use_icount) {
         error_report("-icount is not allowed with hardware virtualization");
         exit(1);
@@ -3618,9 +3636,6 @@  int main(int argc, char **argv, char **envp)
                                  "use -M accel=... for now instead");
                     exit(1);
                 }
-                opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
-                                        false, &error_abort);
-                qemu_opt_set(opts, "accel", optarg, &error_abort);
                 break;
             case QEMU_OPTION_usb:
                 olist = qemu_find_opts("machine");