[Qemu-devel] [PATCH 0/3] Validate NUMA node IDs (reject duplicate/missing node IDs properly)

[Qemu-devel] [PATCH 0/3] Validate NUMA node IDs (reject duplicate/missing node IDs properly)

[Eduardo Habkost]

This is just a cleanup to make sure QEMU validate the NUMA node IDs on the
command-line. After that, we may eventually change the code to accept sparse
node IDs under some circumstances (but I am not sure that would be material for
QEMU 2.1).

Cc: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Cc: qemu-devel@nongnu.org
Cc: Anton Blanchard <anton@samba.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Igor Mammedov <imammedo@redhat.com>

Eduardo Habkost (3):
  numa: Keep track of NUMA nodes present on the command-line
  numa: Reject duplicate node IDs
  numa: Reject configuration if not all node IDs are present

 include/sysemu/sysemu.h |  7 ++++++-
 numa.c                  | 24 +++++++++++++++++++++++-
 vl.c                    |  3 +++
 3 files changed, 32 insertions(+), 2 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] numa: Keep track of NUMA nodes present on the command-line

[Eduardo Habkost]

Based on "enable sparse node numbering" patch from Nishanth Aravamudan,
but without the code to actually support sparse node IDs. This just adds
the code to keep track of present/non-present nodes on the command-line,
without changing any behavior.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
[Rename max_numa_node to max_numa_nodeid -Eduardo]
[Initialize max_numa_nodeid to 0 -Eduardo]
[Use MAX() macro when setting max_numa_nodeid -Eduardo]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/sysemu/sysemu.h | 7 ++++++-
 numa.c                  | 2 ++
 vl.c                    | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 285c45b..d8539fd 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -146,11 +146,16 @@ extern int mem_prealloc;
  */
 #define MAX_CPUMASK_BITS 255
 
-extern int nb_numa_nodes;
+extern int nb_numa_nodes;   /* Number of NUMA nodes */
+extern int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
+                             * For all nodes, nodeid < max_numa_nodeid
+                             */
+
 typedef struct node_info {
     uint64_t node_mem;
     DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
     struct HostMemoryBackend *node_memdev;
+    bool present;
 } NodeInfo;
 extern NodeInfo numa_info[MAX_NODES];
 void set_numa_nodes(void);
diff --git a/numa.c b/numa.c
index e471afe..3de9116 100644
--- a/numa.c
+++ b/numa.c
@@ -106,6 +106,8 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
         numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
         numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
     }
+    numa_info[nodenr].present = true;
+    max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
 }
 
 int numa_init_func(QemuOpts *opts, void *opaque)
diff --git a/vl.c b/vl.c
index a1686ef..41ddcd2 100644
--- a/vl.c
+++ b/vl.c
@@ -196,6 +196,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
     QTAILQ_HEAD_INITIALIZER(fw_boot_order);
 
 int nb_numa_nodes;
+int max_numa_nodeid;
 NodeInfo numa_info[MAX_NODES];
 
 uint8_t qemu_uuid[16];
@@ -2984,10 +2985,12 @@ int main(int argc, char **argv, char **envp)
 
     for (i = 0; i < MAX_NODES; i++) {
         numa_info[i].node_mem = 0;
+        numa_info[i].present = false;
         bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
 
     nb_numa_nodes = 0;
+    max_numa_nodeid = 0;
     nb_nics = 0;
 
     bdrv_init_with_whitelist();
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/3] numa: Keep track of NUMA nodes present on the command-line

[Hu Tao]

On Thu, Jun 26, 2014 at 06:33:18PM -0300, Eduardo Habkost wrote:
> Based on "enable sparse node numbering" patch from Nishanth Aravamudan,
> but without the code to actually support sparse node IDs. This just adds
> the code to keep track of present/non-present nodes on the command-line,
> without changing any behavior.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> [Rename max_numa_node to max_numa_nodeid -Eduardo]
> [Initialize max_numa_nodeid to 0 -Eduardo]
> [Use MAX() macro when setting max_numa_nodeid -Eduardo]
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Hu Tao <hutao@cn.fujitsu.com>

> ---
>  include/sysemu/sysemu.h | 7 ++++++-
>  numa.c                  | 2 ++
>  vl.c                    | 3 +++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 285c45b..d8539fd 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -146,11 +146,16 @@ extern int mem_prealloc;
>   */
>  #define MAX_CPUMASK_BITS 255
>  
> -extern int nb_numa_nodes;
> +extern int nb_numa_nodes;   /* Number of NUMA nodes */
> +extern int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> +                             * For all nodes, nodeid < max_numa_nodeid
> +                             */
> +
>  typedef struct node_info {
>      uint64_t node_mem;
>      DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
>      struct HostMemoryBackend *node_memdev;
> +    bool present;
>  } NodeInfo;
>  extern NodeInfo numa_info[MAX_NODES];
>  void set_numa_nodes(void);
> diff --git a/numa.c b/numa.c
> index e471afe..3de9116 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -106,6 +106,8 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>          numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
>          numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
>      }
> +    numa_info[nodenr].present = true;
> +    max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
>  }
>  
>  int numa_init_func(QemuOpts *opts, void *opaque)
> diff --git a/vl.c b/vl.c
> index a1686ef..41ddcd2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -196,6 +196,7 @@ static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
>      QTAILQ_HEAD_INITIALIZER(fw_boot_order);
>  
>  int nb_numa_nodes;
> +int max_numa_nodeid;
>  NodeInfo numa_info[MAX_NODES];
>  
>  uint8_t qemu_uuid[16];
> @@ -2984,10 +2985,12 @@ int main(int argc, char **argv, char **envp)
>  
>      for (i = 0; i < MAX_NODES; i++) {
>          numa_info[i].node_mem = 0;
> +        numa_info[i].present = false;
>          bitmap_zero(numa_info[i].node_cpu, MAX_CPUMASK_BITS);
>      }
>  
>      nb_numa_nodes = 0;
> +    max_numa_nodeid = 0;
>      nb_nics = 0;
>  
>      bdrv_init_with_whitelist();
> -- 
> 1.9.3

[Qemu-devel] [PATCH 2/3] numa: Reject duplicate node IDs

[Eduardo Habkost]

The same nodeid shouldn't appear multiple times in the command-line.

In addition to detecting command-line mistakes, this will fix a bug
where nb_numa_nodes may become larger than MAX_NODES (and cause
out-of-bounds access on the numa_info array).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 numa.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/numa.c b/numa.c
index 3de9116..e93407a 100644
--- a/numa.c
+++ b/numa.c
@@ -62,6 +62,11 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
         return;
     }
 
+    if (numa_info[nodenr].present) {
+        error_setg(errp, "Duplicate NUMA nodeid: %" PRIu16, nodenr);
+        return;
+    }
+
     for (cpus = node->cpus; cpus; cpus = cpus->next) {
         if (cpus->value > MAX_CPUMASK_BITS) {
             error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/3] numa: Reject duplicate node IDs

[Hu Tao]

On Thu, Jun 26, 2014 at 06:33:19PM -0300, Eduardo Habkost wrote:
> The same nodeid shouldn't appear multiple times in the command-line.
> 
> In addition to detecting command-line mistakes, this will fix a bug
> where nb_numa_nodes may become larger than MAX_NODES (and cause
> out-of-bounds access on the numa_info array).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Hu Tao <hutao@cn.fujitsu.com>

This patch makes me remembering the same problem for cpus, like -numa
..cpus=1,.. -numa ..cpus=1,.., in the case one node has cpu, the other
doesn't. Maybe we should eject it too.

> ---
>  numa.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 3de9116..e93407a 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -62,6 +62,11 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>          return;
>      }
>  
> +    if (numa_info[nodenr].present) {
> +        error_setg(errp, "Duplicate NUMA nodeid: %" PRIu16, nodenr);
> +        return;
> +    }
> +
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
>          if (cpus->value > MAX_CPUMASK_BITS) {
>              error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
> -- 
> 1.9.3

[Qemu-devel] [PATCH 3/3] numa: Reject configuration if not all node IDs are present

[Eduardo Habkost]

We don't support sparse NUMA node IDs yet, so this changes QEMU to
reject configs where not all nodes are present.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 numa.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/numa.c b/numa.c
index e93407a..96fe5dd 100644
--- a/numa.c
+++ b/numa.c
@@ -160,9 +160,24 @@ error:
 
 void set_numa_nodes(void)
 {
+    int i;
+
+    assert(max_numa_nodeid <= MAX_NODES);
+
+    /* No support for sparse NUMA node IDs yet: */
+    for (i = max_numa_nodeid - 1; i >= 0; i--) {
+        /* Report large node IDs first, to make mistakes easier to spot */
+        if (!numa_info[i].present) {
+            error_report("numa: Node ID missing: %d", i);
+            exit(1);
+        }
+    }
+
+    /* This must be always true if all nodes are present: */
+    assert(nb_numa_nodes == max_numa_nodeid);
+
     if (nb_numa_nodes > 0) {
         uint64_t numa_total;
-        int i;
 
         if (nb_numa_nodes > MAX_NODES) {
             nb_numa_nodes = MAX_NODES;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 3/3] numa: Reject configuration if not all node IDs are present

[Hu Tao]

On Thu, Jun 26, 2014 at 06:33:20PM -0300, Eduardo Habkost wrote:
> We don't support sparse NUMA node IDs yet, so this changes QEMU to
> reject configs where not all nodes are present.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  numa.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/numa.c b/numa.c
> index e93407a..96fe5dd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -160,9 +160,24 @@ error:
>  
>  void set_numa_nodes(void)
>  {
> +    int i;
> +
> +    assert(max_numa_nodeid <= MAX_NODES);
> +
> +    /* No support for sparse NUMA node IDs yet: */
> +    for (i = max_numa_nodeid - 1; i >= 0; i--) {
> +        /* Report large node IDs first, to make mistakes easier to spot */
> +        if (!numa_info[i].present) {
> +            error_report("numa: Node ID missing: %d", i);
> +            exit(1);

This reports only one missing node id. Dont' we report all missing ids?

> +        }
> +    }
> +
> +    /* This must be always true if all nodes are present: */
> +    assert(nb_numa_nodes == max_numa_nodeid);
> +
>      if (nb_numa_nodes > 0) {
>          uint64_t numa_total;
> -        int i;
>  
>          if (nb_numa_nodes > MAX_NODES) {
>              nb_numa_nodes = MAX_NODES;
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH 3/3] numa: Reject configuration if not all node IDs are present

[Eduardo Habkost]

On Fri, Jun 27, 2014 at 11:33:59AM +0800, Hu Tao wrote:
> On Thu, Jun 26, 2014 at 06:33:20PM -0300, Eduardo Habkost wrote:
> > We don't support sparse NUMA node IDs yet, so this changes QEMU to
> > reject configs where not all nodes are present.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  numa.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/numa.c b/numa.c
> > index e93407a..96fe5dd 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -160,9 +160,24 @@ error:
> >  
> >  void set_numa_nodes(void)
> >  {
> > +    int i;
> > +
> > +    assert(max_numa_nodeid <= MAX_NODES);
> > +
> > +    /* No support for sparse NUMA node IDs yet: */
> > +    for (i = max_numa_nodeid - 1; i >= 0; i--) {
> > +        /* Report large node IDs first, to make mistakes easier to spot */
> > +        if (!numa_info[i].present) {
> > +            error_report("numa: Node ID missing: %d", i);
> > +            exit(1);
> 
> This reports only one missing node id. Dont' we report all missing ids?

I considered that, but then a simple user mistake could make QEMU spit
more than 100 lines of error messages.

I expect this error code to go away soon, anyway (as soon as we
implement sparse node ID support).

-- 
Eduardo

Re: [Qemu-devel] [PATCH for 2.1 0/3] Validate NUMA node IDs (reject duplicate/missing node IDs properly)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On 06/26/2014 03:33 PM, Eduardo Habkost wrote:
> This is just a cleanup to make sure QEMU validate the NUMA node IDs on the

Yay - this counts as a bug fix, so good for inclusion in 2.1.

> command-line. After that, we may eventually change the code to accept sparse
> node IDs under some circumstances (but I am not sure that would be material for
> QEMU 2.1).

I _am_ sure; accepting sparse nodes is NOT 2.1 material (it's a new
feature, and we're in soft freeze).

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Validate NUMA node IDs (reject duplicate/missing node IDs properly)

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 06:33:17PM -0300, Eduardo Habkost wrote:
> This is just a cleanup to make sure QEMU validate the NUMA node IDs on the
> command-line. After that, we may eventually change the code to accept sparse
> node IDs under some circumstances (but I am not sure that would be material for
> QEMU 2.1).
> 
> Cc: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Hu Tao <hutao@cn.fujitsu.com>
> Cc: qemu-devel@nongnu.org
> Cc: Anton Blanchard <anton@samba.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Igor Mammedov <imammedo@redhat.com>


Applied, thanks!

> Eduardo Habkost (3):
>   numa: Keep track of NUMA nodes present on the command-line
>   numa: Reject duplicate node IDs
>   numa: Reject configuration if not all node IDs are present
> 
>  include/sysemu/sysemu.h |  7 ++++++-
>  numa.c                  | 24 +++++++++++++++++++++++-
>  vl.c                    |  3 +++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.3