1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [RFC PATCH] vl: only display available accelerators

Philippe Mathieu-Daudé posted 1 patch 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171030181459.1951-1-f4bug@amsat.org
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
vl.c | 39 ++++++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
[Qemu-devel] [RFC PATCH] vl: only display available accelerators
Posted by Philippe Mathieu-Daudé 6 years, 5 months ago 
examples configuring with '--enable-kvm --disable-tcg'

- before

  $ qemu-system-x86_64 -accel help
  Possible accelerators: kvm, xen, hax, tcg

  $ qemu-system-x86_64 -accel tcg
  qemu-system-x86_64: -machine accel=tcg: No accelerator found

  # qemu-system-x86_64 -accel hax
  qemu-system-x86_64: -machine accel=hax: No accelerator found

- after

  $ qemu-system-x86_64 -accel help
  Possible accelerators:
    xen
    kvm

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
since RFC:
  - use much cleaner object_class_get_list(TYPE_ACCEL, false)

 vl.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index ec299099ff..0f13641715 100644
--- a/vl.c
+++ b/vl.c
@@ -2764,6 +2764,39 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
     exit(!name || !is_help_option(name));
 }
 
+static void accel_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    const char *typename = object_class_get_name(oc);
+    int len;
+
+    if (!qtest_driver() && !g_strcmp0(typename, ACCEL_CLASS_NAME("qtest"))) {
+        return; /* used by test cases */
+    }
+
+    len = strlen(typename) - strlen("-" TYPE_ACCEL);
+    if (len > 0) {
+        error_printf("  %.*s\n", len, typename);
+    }
+}
+
+static void accel_parse(const char *name, QemuOpts *accel_opts)
+{
+    const char *optarg = qemu_opt_get(accel_opts, "accel");
+    GSList *list;
+
+    if (!is_help_option(optarg)) {
+        return;
+    }
+
+    list = object_class_get_list(TYPE_ACCEL, false);
+    error_printf("Possible accelerators:\n");
+    g_slist_foreach(list, accel_list_entry, NULL);
+    g_slist_free(list);
+
+    exit(0);
+}
+
 void qemu_add_exit_notifier(Notifier *notify)
 {
     notifier_list_add(&exit_notifiers, notify);
@@ -3881,11 +3914,7 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_accel:
                 accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
                                                      optarg, true);
-                optarg = qemu_opt_get(accel_opts, "accel");
-                if (!optarg || is_help_option(optarg)) {
-                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
-                    exit(0);
-                }
+                accel_parse(optarg, accel_opts);
                 opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
                                         false, &error_abort);
                 qemu_opt_set(opts, "accel", optarg, &error_abort);
-- 
2.15.0.rc2
Re: [Qemu-devel] [RFC PATCH] vl: only display available accelerators
Posted by Philippe Mathieu-Daudé 6 years, 5 months ago 
Oops this isn't RFC anymore, this one is v1 ... sorry!

On Mon, Oct 30, 2017 at 3:14 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
[...]
> ---
> since RFC:
>   - use much cleaner object_class_get_list(TYPE_ACCEL, false)
>
>  vl.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
Re: [Qemu-devel] [RFC PATCH] vl: only display available accelerators
Posted by Eduardo Habkost 6 years, 5 months ago 
On Mon, Oct 30, 2017 at 03:14:59PM -0300, Philippe Mathieu-Daudé wrote:
> examples configuring with '--enable-kvm --disable-tcg'
> 
> - before
> 
>   $ qemu-system-x86_64 -accel help
>   Possible accelerators: kvm, xen, hax, tcg
> 
>   $ qemu-system-x86_64 -accel tcg
>   qemu-system-x86_64: -machine accel=tcg: No accelerator found
> 
>   # qemu-system-x86_64 -accel hax
>   qemu-system-x86_64: -machine accel=hax: No accelerator found
> 
> - after
> 
>   $ qemu-system-x86_64 -accel help
>   Possible accelerators:
>     xen
>     kvm
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> since RFC:
>   - use much cleaner object_class_get_list(TYPE_ACCEL, false)
> 
>  vl.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index ec299099ff..0f13641715 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2764,6 +2764,39 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>      exit(!name || !is_help_option(name));
>  }
>  
> +static void accel_list_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    const char *typename = object_class_get_name(oc);
> +    int len;
> +
> +    if (!qtest_driver() && !g_strcmp0(typename, ACCEL_CLASS_NAME("qtest"))) {
> +        return; /* used by test cases */
> +    }

I would prefer to not hardcode the accel name here.  If you want
qtest to have special behavior, you can add a new field to
AccelClass to implement that.

However, I don't think I agree we should hide qtest from the
user: it is accepted on the command-line, so why should we lie?
Why not add a description field and indicate that qtest is useful
only for testing?

> +
> +    len = strlen(typename) - strlen("-" TYPE_ACCEL);

You can use AccelClass::name here.

> +    if (len > 0) {
> +        error_printf("  %.*s\n", len, typename);
> +    }
> +}
> +
> +static void accel_parse(const char *name, QemuOpts *accel_opts)
> +{
> +    const char *optarg = qemu_opt_get(accel_opts, "accel");
> +    GSList *list;
> +
> +    if (!is_help_option(optarg)) {
> +        return;
> +    }
> +
> +    list = object_class_get_list(TYPE_ACCEL, false);
> +    error_printf("Possible accelerators:\n");
> +    g_slist_foreach(list, accel_list_entry, NULL);
> +    g_slist_free(list);
> +
> +    exit(0);
> +}
> +
>  void qemu_add_exit_notifier(Notifier *notify)
>  {
>      notifier_list_add(&exit_notifiers, notify);
> @@ -3881,11 +3914,7 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_accel:
>                  accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
>                                                       optarg, true);
> -                optarg = qemu_opt_get(accel_opts, "accel");
> -                if (!optarg || is_help_option(optarg)) {
> -                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
> -                    exit(0);
> -                }
> +                accel_parse(optarg, accel_opts);
>                  opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
>                                          false, &error_abort);
>                  qemu_opt_set(opts, "accel", optarg, &error_abort);
> -- 
> 2.15.0.rc2
> 

-- 
Eduardo
Re: [Qemu-devel] [RFC PATCH] vl: only display available accelerators
Posted by Philippe Mathieu-Daudé 6 years, 5 months ago 
On 11/02/2017 09:35 PM, Eduardo Habkost wrote:
> On Mon, Oct 30, 2017 at 03:14:59PM -0300, Philippe Mathieu-Daudé wrote:
>> examples configuring with '--enable-kvm --disable-tcg'
>>
>> - before
>>
>>   $ qemu-system-x86_64 -accel help
>>   Possible accelerators: kvm, xen, hax, tcg
>>
>>   $ qemu-system-x86_64 -accel tcg
>>   qemu-system-x86_64: -machine accel=tcg: No accelerator found
>>
>>   # qemu-system-x86_64 -accel hax
>>   qemu-system-x86_64: -machine accel=hax: No accelerator found
>>
>> - after
>>
>>   $ qemu-system-x86_64 -accel help
>>   Possible accelerators:
>>     xen
>>     kvm
>>
>> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> since RFC:
>>   - use much cleaner object_class_get_list(TYPE_ACCEL, false)
>>
>>  vl.c | 39 ++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index ec299099ff..0f13641715 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2764,6 +2764,39 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>>      exit(!name || !is_help_option(name));
>>  }
>>  
>> +static void accel_list_entry(gpointer data, gpointer user_data)
>> +{
>> +    ObjectClass *oc = data;
>> +    const char *typename = object_class_get_name(oc);
>> +    int len;
>> +
>> +    if (!qtest_driver() && !g_strcmp0(typename, ACCEL_CLASS_NAME("qtest"))) {
>> +        return; /* used by test cases */
>> +    }
> 
> I would prefer to not hardcode the accel name here.  If you want
> qtest to have special behavior, you can add a new field to
> AccelClass to implement that.
> 
> However, I don't think I agree we should hide qtest from the
> user: it is accepted on the command-line, so why should we lie?

Ok.

> Why not add a description field and indicate that qtest is useful
> only for testing?

This is cleaner indeed.

>> +
>> +    len = strlen(typename) - strlen("-" TYPE_ACCEL);
> 
> You can use AccelClass::name here.

Hmm yes but currently this is not what the user should use, since the
accel_find() matches the ACCEL_CLASS_NAME:

$ ppc64-softmmu/qemu-system-ppc64 -accel QTest
qemu-system-ppc64: -machine accel=QTest: No accelerator found

I'll respin but the change will be bigger so I is probably too late for
2.11.

> 
>> +    if (len > 0) {
>> +        error_printf("  %.*s\n", len, typename);
>> +    }
>> +}
>> +
>> +static void accel_parse(const char *name, QemuOpts *accel_opts)
>> +{
>> +    const char *optarg = qemu_opt_get(accel_opts, "accel");
>> +    GSList *list;
>> +
>> +    if (!is_help_option(optarg)) {
>> +        return;
>> +    }
>> +
>> +    list = object_class_get_list(TYPE_ACCEL, false);
>> +    error_printf("Possible accelerators:\n");
>> +    g_slist_foreach(list, accel_list_entry, NULL);
>> +    g_slist_free(list);
>> +
>> +    exit(0);
>> +}
>> +
>>  void qemu_add_exit_notifier(Notifier *notify)
>>  {
>>      notifier_list_add(&exit_notifiers, notify);
>> @@ -3881,11 +3914,7 @@ int main(int argc, char **argv, char **envp)
>>              case QEMU_OPTION_accel:
>>                  accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
>>                                                       optarg, true);
>> -                optarg = qemu_opt_get(accel_opts, "accel");
>> -                if (!optarg || is_help_option(optarg)) {
>> -                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
>> -                    exit(0);
>> -                }
>> +                accel_parse(optarg, accel_opts);
>>                  opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
>>                                          false, &error_abort);
>>                  qemu_opt_set(opts, "accel", optarg, &error_abort);
>> -- 
>> 2.15.0.rc2
>>
>
Re: [Qemu-devel] [RFC PATCH] vl: only display available accelerators
Posted by Eduardo Habkost 6 years, 5 months ago 
On Wed, Nov 08, 2017 at 01:20:49PM -0300, Philippe Mathieu-Daudé wrote:
> On 11/02/2017 09:35 PM, Eduardo Habkost wrote:
> > On Mon, Oct 30, 2017 at 03:14:59PM -0300, Philippe Mathieu-Daudé wrote:
> >> examples configuring with '--enable-kvm --disable-tcg'
> >>
> >> - before
> >>
> >>   $ qemu-system-x86_64 -accel help
> >>   Possible accelerators: kvm, xen, hax, tcg
> >>
> >>   $ qemu-system-x86_64 -accel tcg
> >>   qemu-system-x86_64: -machine accel=tcg: No accelerator found
> >>
> >>   # qemu-system-x86_64 -accel hax
> >>   qemu-system-x86_64: -machine accel=hax: No accelerator found
> >>
> >> - after
> >>
> >>   $ qemu-system-x86_64 -accel help
> >>   Possible accelerators:
> >>     xen
> >>     kvm
> >>
> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >> since RFC:
> >>   - use much cleaner object_class_get_list(TYPE_ACCEL, false)
> >>
> >>  vl.c | 39 ++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/vl.c b/vl.c
> >> index ec299099ff..0f13641715 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -2764,6 +2764,39 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
> >>      exit(!name || !is_help_option(name));
> >>  }
> >>  
> >> +static void accel_list_entry(gpointer data, gpointer user_data)
> >> +{
> >> +    ObjectClass *oc = data;
> >> +    const char *typename = object_class_get_name(oc);
> >> +    int len;
> >> +
> >> +    if (!qtest_driver() && !g_strcmp0(typename, ACCEL_CLASS_NAME("qtest"))) {
> >> +        return; /* used by test cases */
> >> +    }
> > 
> > I would prefer to not hardcode the accel name here.  If you want
> > qtest to have special behavior, you can add a new field to
> > AccelClass to implement that.
> > 
> > However, I don't think I agree we should hide qtest from the
> > user: it is accepted on the command-line, so why should we lie?
> 
> Ok.
> 
> > Why not add a description field and indicate that qtest is useful
> > only for testing?
> 
> This is cleaner indeed.
> 
> >> +
> >> +    len = strlen(typename) - strlen("-" TYPE_ACCEL);
> > 
> > You can use AccelClass::name here.
> 
> Hmm yes but currently this is not what the user should use, since the
> accel_find() matches the ACCEL_CLASS_NAME:
> 
> $ ppc64-softmmu/qemu-system-ppc64 -accel QTest
> qemu-system-ppc64: -machine accel=QTest: No accelerator found

Oops, you're correct.

In this case, I suggest using ACCEL_CLASS_SUFFIX instead of
hardcoding ("-" TYPE_ACCEL).

Maybe a accel_class_get_accel_name() helper similar to
x86_cpu_class_get_model_name() would be even better.  It would
require an extra memory allocation, but the code would be
cleaner.  We might add a QMP command returning the accel list in
the future, so a common helper would be useful.


> 
> I'll respin but the change will be bigger so I is probably too late for
> 2.11.
> 
> > 
> >> +    if (len > 0) {
> >> +        error_printf("  %.*s\n", len, typename);
> >> +    }
> >> +}
> >> +
> >> +static void accel_parse(const char *name, QemuOpts *accel_opts)
> >> +{
> >> +    const char *optarg = qemu_opt_get(accel_opts, "accel");
> >> +    GSList *list;
> >> +
> >> +    if (!is_help_option(optarg)) {
> >> +        return;
> >> +    }
> >> +
> >> +    list = object_class_get_list(TYPE_ACCEL, false);
> >> +    error_printf("Possible accelerators:\n");
> >> +    g_slist_foreach(list, accel_list_entry, NULL);
> >> +    g_slist_free(list);
> >> +
> >> +    exit(0);
> >> +}
> >> +
> >>  void qemu_add_exit_notifier(Notifier *notify)
> >>  {
> >>      notifier_list_add(&exit_notifiers, notify);
> >> @@ -3881,11 +3914,7 @@ int main(int argc, char **argv, char **envp)
> >>              case QEMU_OPTION_accel:
> >>                  accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
> >>                                                       optarg, true);
> >> -                optarg = qemu_opt_get(accel_opts, "accel");
> >> -                if (!optarg || is_help_option(optarg)) {
> >> -                    error_printf("Possible accelerators: kvm, xen, hax, tcg\n");
> >> -                    exit(0);
> >> -                }
> >> +                accel_parse(optarg, accel_opts);
> >>                  opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
> >>                                          false, &error_abort);
> >>                  qemu_opt_set(opts, "accel", optarg, &error_abort);
> >> -- 
> >> 2.15.0.rc2
> >>
> > 

-- 
Eduardo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.