Re: [PATCH 06/16] vl: configure accelerators from -accel options

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

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");