[02/16] vl: extract accelerator option processing to a separate function
diff mbox series

Message ID 1573655945-14912-3-git-send-email-pbonzini@redhat.com
State New
Headers show
Series
  • Untitled series #142605
Related show

Commit Message

Paolo Bonzini Nov. 13, 2019, 2:38 p.m. UTC
As a first step towards supporting multiple "-accel" options, push -icount
and -accel semantics into a new function, and use qemu_opts_foreach to
retrieve the key/value lists instead of stashing them into globals.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

Comments

Marc-André Lureau Nov. 14, 2019, 7:55 a.m. UTC | #1
Hi

On Wed, Nov 13, 2019 at 6:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> As a first step towards supporting multiple "-accel" options, push -icount
> and -accel semantics into a new function, and use qemu_opts_foreach to
> retrieve the key/value lists instead of stashing them into globals.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 841fdae..5367f23 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2827,6 +2827,33 @@ static void user_register_global_props(void)
>                        global_init_func, NULL, NULL);
>  }
>
> +static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        configure_icount(opts, errp);
> +    } else {
> +        error_setg(errp, "-icount is not allowed with hardware virtualization");
> +    }
> +    return 0;
> +}
> +
> +static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        qemu_tcg_configure(opts, &error_fatal);
> +    }
> +    return 0;
> +}
> +
> +static void configure_accelerators(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("icount"),
> +                      do_configure_icount, NULL, &error_fatal);
> +
> +    qemu_opts_foreach(qemu_find_opts("accel"),
> +                      do_configure_accelerator, NULL, &error_fatal);

It used to call qemu_tcg_configure() when no -accel option given. In
this case, it still sets mttcg_enabled = default_mttcg_enabled(), but
now it misses that. Perhaps it could be set earlier.

> +}
> +
>  int main(int argc, char **argv, char **envp)
>  {
>      int i;
> @@ -4241,18 +4268,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>
>      cpu_ticks_init();
> -    if (icount_opts) {
> -        if (!tcg_enabled()) {
> -            error_report("-icount is not allowed with hardware virtualization");
> -            exit(1);
> -        }
> -        configure_icount(icount_opts, &error_abort);
> -        qemu_opts_del(icount_opts);
> -    }
> -
> -    if (tcg_enabled()) {
> -        qemu_tcg_configure(accel_opts, &error_fatal);
> -    }
> +    configure_accelerators();
>
>      if (default_net) {
>          QemuOptsList *net = qemu_find_opts("net");
> --
> 1.8.3.1
>
>
>
Paolo Bonzini Nov. 14, 2019, 9:29 a.m. UTC | #2
On 14/11/19 08:55, Marc-André Lureau wrote:
>> +
>> +    qemu_opts_foreach(qemu_find_opts("accel"),
>> +                      do_configure_accelerator, NULL, &error_fatal);
> It used to call qemu_tcg_configure() when no -accel option given. In
> this case, it still sets mttcg_enabled = default_mttcg_enabled(), but
> now it misses that. Perhaps it could be set earlier.
> 

Oh, right.  This is fixed later in the series, but I think in v2 I will
move this to the TCG init function.

Paolo
Thomas Huth Nov. 18, 2019, 10:58 a.m. UTC | #3
On 13/11/2019 15.38, Paolo Bonzini wrote:
> As a first step towards supporting multiple "-accel" options, push -icount
> and -accel semantics into a new function, and use qemu_opts_foreach to
> retrieve the key/value lists instead of stashing them into globals.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 841fdae..5367f23 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2827,6 +2827,33 @@ static void user_register_global_props(void)
>                        global_init_func, NULL, NULL);
>  }
>  
> +static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        configure_icount(opts, errp);
> +    } else {
> +        error_setg(errp, "-icount is not allowed with hardware virtualization");
> +    }
> +    return 0;
> +}
> +
> +static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        qemu_tcg_configure(opts, &error_fatal);
> +    }
> +    return 0;
> +}
> +
> +static void configure_accelerators(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("icount"),
> +                      do_configure_icount, NULL, &error_fatal);
> +
> +    qemu_opts_foreach(qemu_find_opts("accel"),
> +                      do_configure_accelerator, NULL, &error_fatal);
> +}

vl.c is already quite overcrowded ... maybe you could add the new code
to accel/accel.c instead? Just my 0.02 €.

 Thomas
Paolo Bonzini Nov. 18, 2019, 11:39 a.m. UTC | #4
On 18/11/19 11:58, Thomas Huth wrote:
>> +static void configure_accelerators(void)
>> +{
>> +    qemu_opts_foreach(qemu_find_opts("icount"),
>> +                      do_configure_icount, NULL, &error_fatal);
>> +
>> +    qemu_opts_foreach(qemu_find_opts("accel"),
>> +                      do_configure_accelerator, NULL, &error_fatal);
>> +}
>
> vl.c is already quite overcrowded ... maybe you could add the new code
> to accel/accel.c instead? Just my 0.02 €.

I liked the idea of keeping all command line parsing in vl.c, especially
because all the ugliness for backwards compatibility can then be
confined there.

Paolo

Patch
diff mbox series

diff --git a/vl.c b/vl.c
index 841fdae..5367f23 100644
--- a/vl.c
+++ b/vl.c
@@ -2827,6 +2827,33 @@  static void user_register_global_props(void)
                       global_init_func, NULL, NULL);
 }
 
+static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
+{
+    if (tcg_enabled()) {
+        configure_icount(opts, errp);
+    } else {
+        error_setg(errp, "-icount is not allowed with hardware virtualization");
+    }
+    return 0;
+}
+
+static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
+{
+    if (tcg_enabled()) {
+        qemu_tcg_configure(opts, &error_fatal);
+    }
+    return 0;
+}
+
+static void configure_accelerators(void)
+{
+    qemu_opts_foreach(qemu_find_opts("icount"),
+                      do_configure_icount, NULL, &error_fatal);
+
+    qemu_opts_foreach(qemu_find_opts("accel"),
+                      do_configure_accelerator, NULL, &error_fatal);
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -4241,18 +4268,7 @@  int main(int argc, char **argv, char **envp)
     qemu_spice_init();
 
     cpu_ticks_init();
-    if (icount_opts) {
-        if (!tcg_enabled()) {
-            error_report("-icount is not allowed with hardware virtualization");
-            exit(1);
-        }
-        configure_icount(icount_opts, &error_abort);
-        qemu_opts_del(icount_opts);
-    }
-
-    if (tcg_enabled()) {
-        qemu_tcg_configure(accel_opts, &error_fatal);
-    }
+    configure_accelerators();
 
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");