Re: [Qemu-devel] [PATCH v14 17/21] numa: convert to use QObjectInputVisitor for -numa

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> Switch away from using OptsVisitor to parse the -numa
> argument processing. This enables use of the modern
> list syntax for specifying CPUs. e.g. the old syntax
>
>   -numa node,nodeid=0,cpus=0-3,cpus=8-11,mem=107
>
> is equivalent to
>
>   -numa node,nodeid=0,cpus.0=0,cpus.1=1,cpus.2=2,cpus.3=3,\
>         cpus.4=8,cpus.5=9,cpus.6=10,cpus.7=11,mem=107
>
> Furthermore, the cli arg can now follow the QAPI schema
> nesting, so the above is equivalent to the canonical
> syntax:
>
>   -numa type=node,data.nodeid=0,data.cpus.0=0,data.cpus.1=1,\
>         data.cpus.2=2,data.cpus.3=3,data.cpus.4=8,data.cpus.5=9,\
>       data.cpus.6=10,data.cpus.7=11,data.mem=107
>
> A test case is added to cover argument parsing to validate
> that both the old and new syntax is correctly handled.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/sysemu/numa_int.h |  11 +++++
>  numa.c                    |  36 +++++++++-----
>  stubs/Makefile.objs       |   5 ++
>  stubs/exec.c              |   6 +++
>  stubs/hostmem.c           |  14 ++++++
>  stubs/memory.c            |  41 ++++++++++++++++
>  stubs/qdev.c              |   8 ++++
>  stubs/vl.c                |   8 ++++
>  stubs/vmstate.c           |   4 ++
>  tests/Makefile.include    |   2 +
>  tests/test-numa.c         | 116 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  11 files changed, 240 insertions(+), 11 deletions(-)
>  create mode 100644 include/sysemu/numa_int.h
>  create mode 100644 stubs/exec.c
>  create mode 100644 stubs/hostmem.c
>  create mode 100644 stubs/memory.c
>  create mode 100644 stubs/qdev.c
>  create mode 100644 stubs/vl.c
>  create mode 100644 tests/test-numa.c
>
> diff --git a/include/sysemu/numa_int.h b/include/sysemu/numa_int.h
> new file mode 100644
> index 0000000..93160da
> --- /dev/null
> +++ b/include/sysemu/numa_int.h
> @@ -0,0 +1,11 @@
> +#ifndef SYSEMU_NUMA_INT_H
> +#define SYSEMU_NUMA_INT_H
> +
> +#include "sysemu/numa.h"
> +
> +extern int have_memdevs;
> +extern int max_numa_nodeid;
> +
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> +
> +#endif
> diff --git a/numa.c b/numa.c
> index 6289f46..653ebf1 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -23,14 +23,14 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "sysemu/numa.h"
> +#include "sysemu/numa_int.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/bitmap.h"
>  #include "qom/cpu.h"
>  #include "qemu/error-report.h"
>  #include "include/exec/cpu-common.h" /* for RAM_ADDR_FMT */
>  #include "qapi-visit.h"
> -#include "qapi/opts-visitor.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "hw/boards.h"
>  #include "sysemu/hostmem.h"
>  #include "qmp-commands.h"
> @@ -45,10 +45,10 @@ QemuOptsList qemu_numa_opts = {
>      .desc = { { 0 } } /* validated with OptsVisitor */
>  };
>  
> -static int have_memdevs = -1;
> -static int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> -                             * For all nodes, nodeid < max_numa_nodeid
> -                             */
> +int have_memdevs = -1;
> +int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> +                      * For all nodes, nodeid < max_numa_nodeid
> +                      */
>  int nb_numa_nodes;
>  NodeInfo numa_info[MAX_NODES];
>  
> @@ -189,6 +189,9 @@ static void numa_node_parse(NumaNodeOptions *node, 
> QemuOpts *opts, Error **errp)
>      if (node->has_mem) {
>          uint64_t mem_size = node->mem;
>          const char *mem_str = qemu_opt_get(opts, "mem");
> +        if (!mem_str) {
> +            mem_str = qemu_opt_get(opts, "data.mem");
> +        }
>          /* Fix up legacy suffix-less format */
>          if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
>              mem_size <<= 20;
> @@ -211,16 +214,27 @@ static void numa_node_parse(NumaNodeOptions *node, 
> QemuOpts *opts, Error **errp)
>      max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
>  }
>  
> -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      NumaOptions *object = NULL;
>      Error *err = NULL;
> +    Visitor *v;
>  
> -    {
> -        Visitor *v = opts_visitor_new(opts);
> -        visit_type_NumaOptions(v, NULL, &object, &err);
> -        visit_free(v);
> +    /*
> +     * Needs autocreate_list=true, permit_int_ranges=true and
> +     * permit_repeated_opts=true in order to support existing
> +     * syntax for 'cpus' parameter which is an int list.
> +     *
> +     * Needs autocreate_struct_levels=1, because existing syntax
> +     * used a nested struct in the QMP schema with a flat namespace
> +     * in the CLI args.

Which QAPI definition(s) do you allude to here?

If it's just union NumaOptions, that's not ABI.  I'd simply flatten it.
Sketch appended.

> +     */
> +    v = qobject_input_visitor_new_opts(opts, true, 1, true, true, &err);
> +    if (err) {
> +        goto end;
>      }
> +    visit_type_NumaOptions(v, NULL, &object, &err);
> +    visit_free(v);
>  
>      if (err) {
>          goto end;
[Skipping test updates for now...]



diff --git a/numa.c b/numa.c
index 9c09e45..792fa2a 100644
--- a/numa.c
+++ b/numa.c
@@ -227,8 +227,8 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error 
**errp)
     }
 
     switch (object->type) {
-    case NUMA_OPTIONS_KIND_NODE:
-        numa_node_parse(object->u.node.data, opts, &err);
+    case NUMA_OPTIONS_TYPE_NODE:
+        numa_node_parse(&object->u.node, opts, &err);
         if (err) {
             goto end;
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index ded1179..4838258 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4255,7 +4255,10 @@
 #
 # Since 2.1
 ##
+{ 'enum': 'NumaOptionsType', 'data': ['node'] }
 { 'union': 'NumaOptions',
+  'base': { 'type': 'NumaOptionsType' },
+  'discriminator': 'type',
   'data': {
     'node': 'NumaNodeOptions' }}