Re: [PATCH v2 3/3] cpu, qdict, vl: Enable printing options for CPU type

[Markus Armbruster <armbru@redhat.com>]

This is really, really, *really* for maintainers of the code parsing
-cpu to review.  Code parsing -cpu:

* parse_cpu_option() in cpu.c

  Eduardo Habkost <eduardo@habkost.net> (supporter:Machine core)
  Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:Machine core)
  "Philippe Mathieu-Daudé" <philmd@linaro.org> (reviewer:Machine core)
  Yanan Wang <wangyanan55@huawei.com> (reviewer:Machine core)

* cpu_common_parse_features() in hw/core/cpu-common.c

  No maintainers *boggle*

* x86_cpu_parse_featurestr() in qemu/target/i386/cpu.c

  No maintainers *BOGGLE*

* sparc_cpu_parse_features() in target/sparc/cpu.c

  Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> (maintainer:SPARC TCG CPUs)
  Artyom Tarasenko <atar4qemu@gmail.com> (maintainer:SPARC TCG CPUs)

Paolo, Richard, Eduardo, care to get these covered in MAINTAINERS?

Since the patch has been waiting for review for so long, I'll give it a
try, even though I'm only passingly familiar with -cpu parsing.

Paolo, I have a question for you further down.

Dinah Baum <dinahbaum123@gmail.com> writes:

> Change parsing of -cpu argument to allow -cpu cpu,help
> to print options for the CPU type similar to
> how the '-device' option works.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1480
>
> Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>
> ---
>  cpu.c                     | 41 +++++++++++++++++++++++++++++++++++++++
>  include/exec/cpu-common.h |  2 ++
>  include/qapi/qmp/qdict.h  |  2 ++
>  qemu-options.hx           |  7 ++++---
>  qobject/qdict.c           |  5 +++++
>  softmmu/vl.c              | 36 ++++++++++++++++++++++++++++++++--
>  6 files changed, 88 insertions(+), 5 deletions(-)
>
> diff --git a/cpu.c b/cpu.c
> index daf4e1ff0d..5f8a72e51f 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -23,7 +23,9 @@
>  #include "exec/target_page.h"
>  #include "hw/qdev-core.h"
>  #include "hw/qdev-properties.h"
> +#include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> +#include "qemu/qemu-print.h"
>  #include "migration/vmstate.h"
>  #ifdef CONFIG_USER_ONLY
>  #include "qemu.h"
> @@ -43,6 +45,8 @@
>  #include "trace/trace-root.h"
>  #include "qemu/accel.h"
>  #include "qemu/plugin.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qobject.h"
>  
>  uintptr_t qemu_host_page_size;
>  intptr_t qemu_host_page_mask;
> @@ -312,6 +316,43 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      return get_cpu_model_expansion_info(type, model, errp);
>  }
>  
> +void list_cpu_model_expansion(CpuModelExpansionType type,
> +                              CpuModelInfo *model,
> +                              Error **errp)
> +{
> +    CpuModelExpansionInfo *expansion_info;
> +    QDict *qdict;
> +    QDictEntry *qdict_entry;
> +    const char *key;
> +    QObject *obj;
> +    QType q_type;
> +    GPtrArray *array;
> +    int i;
> +    const char *type_name;
> +
> +    expansion_info = get_cpu_model_expansion_info(type, model, errp);
> +    if (expansion_info) {

Avoid nesting:

       if (!expansion_info) {
           return;
       }

       ... work with expansion_info ...

> +        qdict = qobject_to(QDict, expansion_info->model->props);
> +        if (qdict) {

Likewise.

> +            qemu_printf("%s features:\n", model->name);
> +            array = g_ptr_array_new();

Name it @props, please.

> +            for (qdict_entry = (QDictEntry *)qdict_first(qdict); qdict_entry;
> +                 qdict_entry = (QDictEntry *)qdict_next(qdict, qdict_entry)) {
> +                g_ptr_array_add(array, qdict_entry);
> +            }

@qdict can change while we're using it here (if it could, your code
would be wrong).  So, no need for a flexible array.  Create a dynamic
one with g_new(QDictEntry, qdict_size(qdict), fill it, then sort with
qsort().

> +            g_ptr_array_sort(array, (GCompareFunc)dict_key_compare);

Casting function pointers is iffy.  The clean way is to define the
function so it is a GCompareFunc exactly, and have it cast its arguments
if necessary.

> +            for (i = 0; i < array->len; i++) {
> +                qdict_entry = array->pdata[i];
> +                key = qdict_entry_key(qdict_entry);
> +                obj = qdict_get(qdict, key);
> +                q_type = qobject_type(obj);
> +                type_name = QType_str(q_type);
> +                qemu_printf("  %s=<%s>\n", key, type_name);

Contract to

                   qemu_printf("  %s=<%s>\n",
                               key, QType_str(qobject_type(obj)));

Actually, don't use QType_str(), because the type comes out as "qnum",
"qstring", "qbool" (bad), or as "qdict", "qlist" (worse), or as "qnull"
(still worse, but impossible, I think).

Is CpuModelInfo the appropriate source?  Could we get properties
straight from QOM instead, like we do for "-device TYPE,help" and
"-object TYPE,help"?  I guess this question is for Paolo.

> +            }
> +        }
> +    }
> +}
> +
>  #if defined(CONFIG_USER_ONLY)
>  void tb_invalidate_phys_addr(target_ulong addr)
>  {
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index ec6024dfde..8fc05307ad 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -174,5 +174,7 @@ typedef void (*cpu_model_expansion_func)(CpuModelExpansionType type,
>  CpuModelExpansionInfo *get_cpu_model_expansion_info(CpuModelExpansionType type,
>                                                      CpuModelInfo *model,
>                                                      Error **errp);
> +void list_cpu_model_expansion(CpuModelExpansionType type,
> +                              CpuModelInfo *model, Error **errp);
>  
>  #endif /* CPU_COMMON_H */
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 82e90fc072..1ff9523a13 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -68,4 +68,6 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key);
>  
>  QDict *qdict_clone_shallow(const QDict *src);
>  
> +int dict_key_compare(QDictEntry **entry1, QDictEntry **entry2);
> +
>  #endif /* QDICT_H */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 59bdf67a2c..10601626b7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -169,11 +169,12 @@ SRST
>  ERST
>  
>  DEF("cpu", HAS_ARG, QEMU_OPTION_cpu,
> -    "-cpu cpu        select CPU ('-cpu help' for list)\n", QEMU_ARCH_ALL)
> +    "-cpu cpu        select CPU ('-cpu help' for list)\n"
> +    "                use '-cpu cpu,help' to print possible properties\n", QEMU_ARCH_ALL)
>  SRST
>  ``-cpu model``
> -    Select CPU model (``-cpu help`` for list and additional feature
> -    selection)
> +    Select CPU model (``-cpu help`` and ``-cpu cpu,help``) for list and additional feature
> +    selection
>  ERST
>  
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 8faff230d3..31407e62f6 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -447,3 +447,8 @@ void qdict_unref(QDict *q)
>  {
>      qobject_unref(q);
>  }
> +
> +int dict_key_compare(QDictEntry **entry1, QDictEntry **entry2)
> +{
> +    return g_strcmp0(qdict_entry_key(*entry1), qdict_entry_key(*entry2));
> +}

This file's external functions start with qdict_, not dict_.

There is just one caller.  Let's put the function next to it, and make
it static.

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ea20b23e4c..af6753a7e3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -500,6 +500,15 @@ static QemuOptsList qemu_action_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_cpu_opts = {
> +    .name = "cpu",
> +    .implied_opt_name = "cpu",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head),
> +    .desc = {
> +        { /* end of list */ }
> +    },
> +};
> +
>  const char *qemu_get_vm_name(void)
>  {
>      return qemu_name;
> @@ -1147,6 +1156,26 @@ static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> +static int cpu_help_func(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    CpuModelInfo *model;
> +
> +    if (cpu_option && is_help_option(cpu_option)) {
> +        list_cpus(cpu_option);
> +        return 1;
> +    }
> +
> +    if (!cpu_option || !qemu_opt_has_help_opt(opts)) {
> +        return 0;
> +    }
> +
> +    model = g_new0(CpuModelInfo, 1);
> +    model->name = (char *)qemu_opt_get(opts, "cpu");
> +    /* TODO: handle other expansion cases */
> +    list_cpu_model_expansion(CPU_MODEL_EXPANSION_TYPE_FULL, model, errp);
> +    return 1;
> +}
> +
>  static int chardev_init_func(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      Error *local_err = NULL;
> @@ -2431,8 +2460,9 @@ static void qemu_process_help_options(void)
>       * type and the user did not specify one, so that the user doesn't need
>       * to say '-cpu help -machine something'.
>       */
> -    if (cpu_option && is_help_option(cpu_option)) {
> -        list_cpus(cpu_option);
> +    Error *errp = NULL;
> +    if (qemu_opts_foreach(qemu_find_opts("cpu"),
> +                          cpu_help_func, NULL, &errp)) {
>          exit(0);
>      }
>  
> @@ -2673,6 +2703,7 @@ void qemu_init(int argc, char **argv)
>      qemu_add_opts(&qemu_semihosting_config_opts);
>      qemu_add_opts(&qemu_fw_cfg_opts);
>      qemu_add_opts(&qemu_action_opts);
> +    qemu_add_opts(&qemu_cpu_opts);
>      module_call_init(MODULE_INIT_OPTS);
>  
>      error_init(argv[0]);
> @@ -2724,6 +2755,7 @@ void qemu_init(int argc, char **argv)
>              switch(popt->index) {
>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */
> +                qemu_opts_parse_noisily(qemu_find_opts("cpu"), optarg, true);

No :)

We have bespoke parsers for the argument of -cpu: parse_cpu_option()
together with CPUClass methods parse_features().  The syntax they parse
is superficially similar to QemuOpts (parts separated with comma), but
it's not the same.  If it was, we'd use QemuOpts and ditch the bespoke
parsers.

If qemu_opts_parse_noisily() rejects @optarg here, it reports an error,
and we continue anyway.

If parse_cpu_option() also rejects @optarg later on, the error is
reported twice.  Bad.

If it doesn't, the error qemu_opts_parse_noisily() reported is bogus.

If both succeed, they may well yield different parse results.  I can try
to dig up examples if necessary.

As far as I can see, you use the result of qemu_opts_parse_noisily()
only with cpu_help_func().  Can we slot the help feature into the
bespoke parser instead?  Let's have a look.

When the argument of -cpu is "help", qemu_process_help_options() shows
help and exits before we call parse_cpu_option().

parse_cpu_option() splits the argument of -cpu at the first comma into
CPU class name and features.

If you factor the splitting out of parse_cpu_option(), you can call it
from qemu_process_help_options(), then check whether the features are
"help".

>                  cpu_option = optarg;
>                  break;
>              case QEMU_OPTION_hda: