Introduce machine_set_cpu_numa_node() helper that stores
node mapping for CPU in MachineState::possible_cpus.
CPU and node it belongs to is specified by 'props' argument.

Patch doesn't remove old way of storing mapping in
numa_info[X].node_cpu as removing it at the same time
makes patch rather big. Instead it just mirrors mapping
in possible_cpus and follow up per target patches will
switch to possible_cpus and numa_info[X].node_cpu will
be removed once there isn't any users left.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/hw/boards.h |  2 ++
 hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 numa.c              |  8 +++++++
 3 files changed, 78 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5d6af21..1f518a1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
 void machine_register_compat_props(MachineState *machine);
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
+void machine_set_cpu_numa_node(MachineState *machine,
+                               CpuInstanceProperties *props, Error **errp);
 
 /**
  * CPUArchId:
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ada9eea..a63f17b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
     return head;
 }
 
+void machine_set_cpu_numa_node(MachineState *machine,
+                               CpuInstanceProperties *props, Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    bool match = false;
+    int i;
+
+    if (!mc->possible_cpu_arch_ids) {
+        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
+        return;
+    }
+
+    /* force board to initialize possible_cpus if it hasn't been done yet */
+    mc->possible_cpu_arch_ids(machine);
+
+    for (i = 0; i < machine->possible_cpus->len; i++) {
+        CPUArchId *slot = &machine->possible_cpus->cpus[i];
+
+        /* reject unsupported by board properties */
+        if (props->has_thread_id && !slot->props.has_thread_id) {
+            error_setg(errp, "thread-id is not supported");
+            return;
+        }
+
+        if (props->has_core_id && !slot->props.has_core_id) {
+            error_setg(errp, "core-id is not supported");
+            return;
+        }
+
+        if (props->has_socket_id && !slot->props.has_socket_id) {
+            error_setg(errp, "socket-id is not supported");
+            return;
+        }
+
+        /* skip slots with explicit mismatch */
+        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
+                continue;
+        }
+
+        if (props->has_core_id && props->core_id != slot->props.core_id) {
+                continue;
+        }
+
+        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
+                continue;
+        }
+
+        /* reject assignment if slot is already assigned, for compatibility
+         * of legacy cpu_index mapping with SPAPR core based mapping do not
+         * error out if cpu thread and matched core have the same node-id */
+        if (slot->props.has_node_id &&
+            slot->props.node_id != props->node_id) {
+            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
+                       slot->props.node_id);
+            return;
+        }
+
+        /* assign slot to node as it's matched '-numa cpu' key */
+        match = true;
+        slot->props.node_id = props->node_id;
+        slot->props.has_node_id = props->has_node_id;
+    }
+
+    if (!match) {
+        error_setg(errp, "no match found");
+    }
+}
+
 static void machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
diff --git a/numa.c b/numa.c
index b517870..5ff1212 100644
--- a/numa.c
+++ b/numa.c
@@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
         exit(1);
     }
     for (cpus = node->cpus; cpus; cpus = cpus->next) {
+        CpuInstanceProperties props;
         if (cpus->value >= max_cpus) {
             error_setg(errp,
                        "CPU index (%" PRIu16 ")"
@@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
             return;
         }
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
+        props = mc->cpu_index_to_instance_props(ms, cpus->value);
+        props.node_id = nodenr;
+        props.has_node_id = true;
+        machine_set_cpu_numa_node(ms, &props, &error_fatal);
     }
 
     if (node->has_mem && node->has_memdev) {
@@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
         if (i == nb_numa_nodes) {
             for (i = 0; i < max_cpus; i++) {
                 CpuInstanceProperties props;
+                /* fetch default mapping from board and enable it */
                 props = mc->cpu_index_to_instance_props(ms, i);
+                props.has_node_id = true;
 
                 set_bit(i, numa_info[props.node_id].node_cpu);
+                machine_set_cpu_numa_node(ms, &props, &error_fatal);
             }
         }
 
-- 
2.7.4

On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> Introduce machine_set_cpu_numa_node() helper that stores
> node mapping for CPU in MachineState::possible_cpus.
> CPU and node it belongs to is specified by 'props' argument.
> 
> Patch doesn't remove old way of storing mapping in
> numa_info[X].node_cpu as removing it at the same time
> makes patch rather big. Instead it just mirrors mapping
> in possible_cpus and follow up per target patches will
> switch to possible_cpus and numa_info[X].node_cpu will
> be removed once there isn't any users left.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  include/hw/boards.h |  2 ++
>  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  numa.c              |  8 +++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5d6af21..1f518a1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
>  bool machine_mem_merge(MachineState *machine);
>  void machine_register_compat_props(MachineState *machine);
>  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> +void machine_set_cpu_numa_node(MachineState *machine,
> +                               CpuInstanceProperties *props, Error **errp);
>  
>  /**
>   * CPUArchId:
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ada9eea..a63f17b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
>      return head;
>  }
>  
> +void machine_set_cpu_numa_node(MachineState *machine,
> +                               CpuInstanceProperties *props, Error **errp)

As the semantics of this function aren't trivial, it would be
nice to have a comment explaining what exactly this function do.

e.g.:
* make it clear that it could affect multiple CPU slots;
* make it clear what does it mean to have props->has_node_id=false as
  argument (is it really valid?);
* make it clear that it will refuse to change an existing mapping.


> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    bool match = false;
> +    int i;
> +
> +    if (!mc->possible_cpu_arch_ids) {
> +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> +        return;
> +    }
> +
> +    /* force board to initialize possible_cpus if it hasn't been done yet */
> +    mc->possible_cpu_arch_ids(machine);
> +
> +    for (i = 0; i < machine->possible_cpus->len; i++) {
> +        CPUArchId *slot = &machine->possible_cpus->cpus[i];
> +
> +        /* reject unsupported by board properties */
> +        if (props->has_thread_id && !slot->props.has_thread_id) {
> +            error_setg(errp, "thread-id is not supported");
> +            return;
> +        }
> +
> +        if (props->has_core_id && !slot->props.has_core_id) {
> +            error_setg(errp, "core-id is not supported");
> +            return;
> +        }
> +
> +        if (props->has_socket_id && !slot->props.has_socket_id) {
> +            error_setg(errp, "socket-id is not supported");
> +            return;
> +        }
> +
> +        /* skip slots with explicit mismatch */
> +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> +                continue;
> +        }
> +
> +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> +                continue;
> +        }
> +
> +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> +                continue;
> +        }
> +
> +        /* reject assignment if slot is already assigned, for compatibility
> +         * of legacy cpu_index mapping with SPAPR core based mapping do not
> +         * error out if cpu thread and matched core have the same node-id */
> +        if (slot->props.has_node_id &&
> +            slot->props.node_id != props->node_id) {
> +            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
> +                       slot->props.node_id);
> +            return;
> +        }
> +
> +        /* assign slot to node as it's matched '-numa cpu' key */
> +        match = true;
> +        slot->props.node_id = props->node_id;
> +        slot->props.has_node_id = props->has_node_id;

Is it really valid to have has_node_id=false? Maybe an
assert(props->has_node_id) at the beginning of the function would
be useful.

> +    }
> +
> +    if (!match) {
> +        error_setg(errp, "no match found");
> +    }
> +}
> +
>  static void machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> diff --git a/numa.c b/numa.c
> index b517870..5ff1212 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
>          exit(1);
>      }
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> +        CpuInstanceProperties props;
>          if (cpus->value >= max_cpus) {
>              error_setg(errp,
>                         "CPU index (%" PRIu16 ")"
> @@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> +        props.node_id = nodenr;
> +        props.has_node_id = true;
> +        machine_set_cpu_numa_node(ms, &props, &error_fatal);
>      }
>  
>      if (node->has_mem && node->has_memdev) {
> @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
>          if (i == nb_numa_nodes) {
>              for (i = 0; i < max_cpus; i++) {
>                  CpuInstanceProperties props;
> +                /* fetch default mapping from board and enable it */
>                  props = mc->cpu_index_to_instance_props(ms, i);
> +                props.has_node_id = true;
>  
>                  set_bit(i, numa_info[props.node_id].node_cpu);
> +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
>              }
>          }
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

On Wed, 3 May 2017 12:20:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> > Introduce machine_set_cpu_numa_node() helper that stores
> > node mapping for CPU in MachineState::possible_cpus.
> > CPU and node it belongs to is specified by 'props' argument.
> > 
> > Patch doesn't remove old way of storing mapping in
> > numa_info[X].node_cpu as removing it at the same time
> > makes patch rather big. Instead it just mirrors mapping
> > in possible_cpus and follow up per target patches will
> > switch to possible_cpus and numa_info[X].node_cpu will
> > be removed once there isn't any users left.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  include/hw/boards.h |  2 ++
> >  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  numa.c              |  8 +++++++
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 5d6af21..1f518a1 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> >  bool machine_mem_merge(MachineState *machine);
> >  void machine_register_compat_props(MachineState *machine);
> >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp);
> >  
> >  /**
> >   * CPUArchId:
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ada9eea..a63f17b 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> >      return head;
> >  }
> >  
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp)  
> 
> As the semantics of this function aren't trivial, it would be
> nice to have a comment explaining what exactly this function do.
> 
> e.g.:
> * make it clear that it could affect multiple CPU slots;
> * make it clear what does it mean to have props->has_node_id=false as
>   argument (is it really valid?);
> * make it clear that it will refuse to change an existing mapping.
sure, I'll add comment.

> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    bool match = false;
> > +    int i;
> > +
> > +    if (!mc->possible_cpu_arch_ids) {
> > +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> > +        return;
> > +    }
> > +
> > +    /* force board to initialize possible_cpus if it hasn't been done yet */
> > +    mc->possible_cpu_arch_ids(machine);
> > +
> > +    for (i = 0; i < machine->possible_cpus->len; i++) {
> > +        CPUArchId *slot = &machine->possible_cpus->cpus[i];
> > +
> > +        /* reject unsupported by board properties */
> > +        if (props->has_thread_id && !slot->props.has_thread_id) {
> > +            error_setg(errp, "thread-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_core_id && !slot->props.has_core_id) {
> > +            error_setg(errp, "core-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_socket_id && !slot->props.has_socket_id) {
> > +            error_setg(errp, "socket-id is not supported");
> > +            return;
> > +        }
> > +
> > +        /* skip slots with explicit mismatch */
> > +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> > +                continue;
> > +        }
> > +
> > +        /* reject assignment if slot is already assigned, for compatibility
> > +         * of legacy cpu_index mapping with SPAPR core based mapping do not
> > +         * error out if cpu thread and matched core have the same node-id */
> > +        if (slot->props.has_node_id &&
> > +            slot->props.node_id != props->node_id) {
> > +            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
> > +                       slot->props.node_id);
> > +            return;
> > +        }
> > +
> > +        /* assign slot to node as it's matched '-numa cpu' key */
> > +        match = true;
> > +        slot->props.node_id = props->node_id;
> > +        slot->props.has_node_id = props->has_node_id;  
> 
> Is it really valid to have has_node_id=false? Maybe an
> assert(props->has_node_id) at the beginning of the function would
> be useful.
yep, it shouldn't be false for props->has_node_id
I'll add assert

> 
> > +    }
> > +
> > +    if (!match) {
> > +        error_setg(errp, "no match found");
> > +    }
> > +}
> > +
> >  static void machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > diff --git a/numa.c b/numa.c
> > index b517870..5ff1212 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >          exit(1);
> >      }
> >      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> > +        CpuInstanceProperties props;
> >          if (cpus->value >= max_cpus) {
> >              error_setg(errp,
> >                         "CPU index (%" PRIu16 ")"
> > @@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >              return;
> >          }
> >          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> > +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> > +        props.node_id = nodenr;
> > +        props.has_node_id = true;
> > +        machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >      }
> >  
> >      if (node->has_mem && node->has_memdev) {
> > @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
> >          if (i == nb_numa_nodes) {
> >              for (i = 0; i < max_cpus; i++) {
> >                  CpuInstanceProperties props;
> > +                /* fetch default mapping from board and enable it */
> >                  props = mc->cpu_index_to_instance_props(ms, i);
> > +                props.has_node_id = true;
> >  
> >                  set_bit(i, numa_info[props.node_id].node_cpu);
> > +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >              }
> >          }
> >  
> > -- 
> > 2.7.4
> >   
> 

On Wed, 3 May 2017 12:20:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> > Introduce machine_set_cpu_numa_node() helper that stores
> > node mapping for CPU in MachineState::possible_cpus.
> > CPU and node it belongs to is specified by 'props' argument.
> > 
> > Patch doesn't remove old way of storing mapping in
> > numa_info[X].node_cpu as removing it at the same time
> > makes patch rather big. Instead it just mirrors mapping
> > in possible_cpus and follow up per target patches will
> > switch to possible_cpus and numa_info[X].node_cpu will
> > be removed once there isn't any users left.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  include/hw/boards.h |  2 ++
> >  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  numa.c              |  8 +++++++
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 5d6af21..1f518a1 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> >  bool machine_mem_merge(MachineState *machine);
> >  void machine_register_compat_props(MachineState *machine);
> >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp);
> >  
> >  /**
> >   * CPUArchId:
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ada9eea..a63f17b 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> >      return head;
> >  }
> >  
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp)  
> 
> As the semantics of this function aren't trivial, it would be
> nice to have a comment explaining what exactly this function do.
> 
> e.g.:
> * make it clear that it could affect multiple CPU slots;
> * make it clear what does it mean to have props->has_node_id=false as
>   argument (is it really valid?);
> * make it clear that it will refuse to change an existing mapping.
Will be following comment sufficient?

+/**
+ * machine_set_cpu_numa_node:
+ * @machine: machine object to modify
+ * @props: specifies which cpu objects to assign to
+ *         numa node specified by @props.node_id
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Associate NUMA node specified by @props.node_id with cpu slots that
+ * match socket/core/thread-ids specified by @props. It's recommended to use
+ * query-hotpluggable-cpus.props values to specify affected cpu slots,
+ * which would lead to exact 1:1 mapping of cpu slots to NUMA node.
+ *
+ * However for CLI convenience it's possible to pass in subset of properties,
+ * which would affect all cpu slots that match it.
+ * Ex for pc machine:
+ *    -smp 4,cores=2,sockets=2 -numa node,nodeid=0 -numa node,nodeid=1 \
+ *    -numa cpu,node-id=0,socket_id=0 \
+ *    -numa cpu,node-id=1,socket_id=1
+ * will assign all child cores of socket 0 to node 0 and
+ * of socket 1 to node 1.
+ *
+ * Empty subset is disallowed and function will return with error in this case.
+ */
 void machine_set_cpu_numa_node(MachineState *machine,
                                CpuInstanceProperties *props, Error **errp)
 {
@@ -400,6 +423,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
         error_setg(errp, "mapping of CPUs to NUMA node is not supported");
         return;
     }
+    /* disabling numa mapping is not supported, forbid it */
+    assert(props->has_node_id);

> 
> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    bool match = false;
> > +    int i;
> > +
> > +    if (!mc->possible_cpu_arch_ids) {
> > +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> > +        return;
> > +    }
> > +
> > +    /* force board to initialize possible_cpus if it hasn't been done yet */
> > +    mc->possible_cpu_arch_ids(machine);
> > +
> > +    for (i = 0; i < machine->possible_cpus->len; i++) {
> > +        CPUArchId *slot = &machine->possible_cpus->cpus[i];
> > +
> > +        /* reject unsupported by board properties */
> > +        if (props->has_thread_id && !slot->props.has_thread_id) {
> > +            error_setg(errp, "thread-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_core_id && !slot->props.has_core_id) {
> > +            error_setg(errp, "core-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_socket_id && !slot->props.has_socket_id) {
> > +            error_setg(errp, "socket-id is not supported");
> > +            return;
> > +        }
> > +
> > +        /* skip slots with explicit mismatch */
> > +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> > +                continue;
> > +        }
> > +
> > +        /* reject assignment if slot is already assigned, for compatibility
> > +         * of legacy cpu_index mapping with SPAPR core based mapping do not
> > +         * error out if cpu thread and matched core have the same node-id */
> > +        if (slot->props.has_node_id &&
> > +            slot->props.node_id != props->node_id) {
> > +            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
> > +                       slot->props.node_id);
> > +            return;
> > +        }
> > +
> > +        /* assign slot to node as it's matched '-numa cpu' key */
> > +        match = true;
> > +        slot->props.node_id = props->node_id;
> > +        slot->props.has_node_id = props->has_node_id;  
> 
> Is it really valid to have has_node_id=false? Maybe an
> assert(props->has_node_id) at the beginning of the function would
> be useful.
> 
> > +    }
> > +
> > +    if (!match) {
> > +        error_setg(errp, "no match found");
> > +    }
> > +}
> > +
> >  static void machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > diff --git a/numa.c b/numa.c
> > index b517870..5ff1212 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >          exit(1);
> >      }
> >      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> > +        CpuInstanceProperties props;
> >          if (cpus->value >= max_cpus) {
> >              error_setg(errp,
> >                         "CPU index (%" PRIu16 ")"
> > @@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >              return;
> >          }
> >          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> > +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> > +        props.node_id = nodenr;
> > +        props.has_node_id = true;
> > +        machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >      }
> >  
> >      if (node->has_mem && node->has_memdev) {
> > @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
> >          if (i == nb_numa_nodes) {
> >              for (i = 0; i < max_cpus; i++) {
> >                  CpuInstanceProperties props;
> > +                /* fetch default mapping from board and enable it */
> >                  props = mc->cpu_index_to_instance_props(ms, i);
> > +                props.has_node_id = true;
> >  
> >                  set_bit(i, numa_info[props.node_id].node_cpu);
> > +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >              }
> >          }
> >  
> > -- 
> > 2.7.4
> >   
> 

On Fri, May 05, 2017 at 02:16:48PM +0200, Igor Mammedov wrote:
> On Wed, 3 May 2017 12:20:22 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> > > Introduce machine_set_cpu_numa_node() helper that stores
> > > node mapping for CPU in MachineState::possible_cpus.
> > > CPU and node it belongs to is specified by 'props' argument.
> > > 
> > > Patch doesn't remove old way of storing mapping in
> > > numa_info[X].node_cpu as removing it at the same time
> > > makes patch rather big. Instead it just mirrors mapping
> > > in possible_cpus and follow up per target patches will
> > > switch to possible_cpus and numa_info[X].node_cpu will
> > > be removed once there isn't any users left.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  include/hw/boards.h |  2 ++
> > >  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  numa.c              |  8 +++++++
> > >  3 files changed, 78 insertions(+)
> > > 
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 5d6af21..1f518a1 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> > >  bool machine_mem_merge(MachineState *machine);
> > >  void machine_register_compat_props(MachineState *machine);
> > >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> > > +void machine_set_cpu_numa_node(MachineState *machine,
> > > +                               CpuInstanceProperties *props, Error **errp);
> > >  
> > >  /**
> > >   * CPUArchId:
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index ada9eea..a63f17b 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> > >      return head;
> > >  }
> > >  
> > > +void machine_set_cpu_numa_node(MachineState *machine,
> > > +                               CpuInstanceProperties *props, Error **errp)  
> > 
> > As the semantics of this function aren't trivial, it would be
> > nice to have a comment explaining what exactly this function do.
> > 
> > e.g.:
> > * make it clear that it could affect multiple CPU slots;
> > * make it clear what does it mean to have props->has_node_id=false as
> >   argument (is it really valid?);
> > * make it clear that it will refuse to change an existing mapping.
> Will be following comment sufficient?
> 
> +/**
> + * machine_set_cpu_numa_node:
> + * @machine: machine object to modify
> + * @props: specifies which cpu objects to assign to
> + *         numa node specified by @props.node_id
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Associate NUMA node specified by @props.node_id with cpu slots that
> + * match socket/core/thread-ids specified by @props. It's recommended to use
> + * query-hotpluggable-cpus.props values to specify affected cpu slots,
> + * which would lead to exact 1:1 mapping of cpu slots to NUMA node.
> + *
> + * However for CLI convenience it's possible to pass in subset of properties,
> + * which would affect all cpu slots that match it.
> + * Ex for pc machine:
> + *    -smp 4,cores=2,sockets=2 -numa node,nodeid=0 -numa node,nodeid=1 \
> + *    -numa cpu,node-id=0,socket_id=0 \
> + *    -numa cpu,node-id=1,socket_id=1
> + * will assign all child cores of socket 0 to node 0 and
> + * of socket 1 to node 1.
> + *
> + * Empty subset is disallowed and function will return with error in this case.
> + */
>  void machine_set_cpu_numa_node(MachineState *machine,
>                                 CpuInstanceProperties *props, Error **errp)

Sounds good to me.

While at it, we could make 'props' const, as it's not going to be
touched by the function.

-- 
Eduardo

On Fri, 5 May 2017 14:04:15 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, May 05, 2017 at 02:16:48PM +0200, Igor Mammedov wrote:
> > On Wed, 3 May 2017 12:20:22 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> > > > Introduce machine_set_cpu_numa_node() helper that stores
> > > > node mapping for CPU in MachineState::possible_cpus.
> > > > CPU and node it belongs to is specified by 'props' argument.
> > > > 
> > > > Patch doesn't remove old way of storing mapping in
> > > > numa_info[X].node_cpu as removing it at the same time
> > > > makes patch rather big. Instead it just mirrors mapping
> > > > in possible_cpus and follow up per target patches will
> > > > switch to possible_cpus and numa_info[X].node_cpu will
> > > > be removed once there isn't any users left.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  include/hw/boards.h |  2 ++
> > > >  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  numa.c              |  8 +++++++
> > > >  3 files changed, 78 insertions(+)
> > > > 
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index 5d6af21..1f518a1 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> > > >  bool machine_mem_merge(MachineState *machine);
> > > >  void machine_register_compat_props(MachineState *machine);
> > > >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> > > > +void machine_set_cpu_numa_node(MachineState *machine,
> > > > +                               CpuInstanceProperties *props, Error **errp);
> > > >  
> > > >  /**
> > > >   * CPUArchId:
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index ada9eea..a63f17b 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> > > >      return head;
> > > >  }
> > > >  
> > > > +void machine_set_cpu_numa_node(MachineState *machine,
> > > > +                               CpuInstanceProperties *props, Error **errp)  
> > > 
> > > As the semantics of this function aren't trivial, it would be
> > > nice to have a comment explaining what exactly this function do.
> > > 
> > > e.g.:
> > > * make it clear that it could affect multiple CPU slots;
> > > * make it clear what does it mean to have props->has_node_id=false as
> > >   argument (is it really valid?);
> > > * make it clear that it will refuse to change an existing mapping.
> > Will be following comment sufficient?
> > 
> > +/**
> > + * machine_set_cpu_numa_node:
> > + * @machine: machine object to modify
> > + * @props: specifies which cpu objects to assign to
> > + *         numa node specified by @props.node_id
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Associate NUMA node specified by @props.node_id with cpu slots that
> > + * match socket/core/thread-ids specified by @props. It's recommended to use
> > + * query-hotpluggable-cpus.props values to specify affected cpu slots,
> > + * which would lead to exact 1:1 mapping of cpu slots to NUMA node.
> > + *
> > + * However for CLI convenience it's possible to pass in subset of properties,
> > + * which would affect all cpu slots that match it.
> > + * Ex for pc machine:
> > + *    -smp 4,cores=2,sockets=2 -numa node,nodeid=0 -numa node,nodeid=1 \
> > + *    -numa cpu,node-id=0,socket_id=0 \
> > + *    -numa cpu,node-id=1,socket_id=1
> > + * will assign all child cores of socket 0 to node 0 and
> > + * of socket 1 to node 1.
> > + *
> > + * Empty subset is disallowed and function will return with error in this case.
> > + */
> >  void machine_set_cpu_numa_node(MachineState *machine,
> >                                 CpuInstanceProperties *props, Error **errp)
> 
> Sounds good to me.
> 
> While at it, we could make 'props' const, as it's not going to be
> touched by the function.
sure

On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> Introduce machine_set_cpu_numa_node() helper that stores
> node mapping for CPU in MachineState::possible_cpus.
> CPU and node it belongs to is specified by 'props' argument.
> 
> Patch doesn't remove old way of storing mapping in
> numa_info[X].node_cpu as removing it at the same time
> makes patch rather big. Instead it just mirrors mapping
> in possible_cpus and follow up per target patches will
> switch to possible_cpus and numa_info[X].node_cpu will
> be removed once there isn't any users left.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  include/hw/boards.h |  2 ++
>  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  numa.c              |  8 +++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 5d6af21..1f518a1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
>  bool machine_mem_merge(MachineState *machine);
>  void machine_register_compat_props(MachineState *machine);
>  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> +void machine_set_cpu_numa_node(MachineState *machine,
> +                               CpuInstanceProperties *props, Error **errp);
>  
>  /**
>   * CPUArchId:
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ada9eea..a63f17b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
>      return head;
>  }
>  
> +void machine_set_cpu_numa_node(MachineState *machine,
> +                               CpuInstanceProperties *props, Error **errp)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    bool match = false;
> +    int i;
> +
> +    if (!mc->possible_cpu_arch_ids) {
> +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> +        return;
> +    }
> +
> +    /* force board to initialize possible_cpus if it hasn't been done yet */
> +    mc->possible_cpu_arch_ids(machine);
> +
> +    for (i = 0; i < machine->possible_cpus->len; i++) {
> +        CPUArchId *slot = &machine->possible_cpus->cpus[i];
> +
> +        /* reject unsupported by board properties */
> +        if (props->has_thread_id && !slot->props.has_thread_id) {
> +            error_setg(errp, "thread-id is not supported");
> +            return;
> +        }
> +
> +        if (props->has_core_id && !slot->props.has_core_id) {
> +            error_setg(errp, "core-id is not supported");
> +            return;
> +        }
> +
> +        if (props->has_socket_id && !slot->props.has_socket_id) {
> +            error_setg(errp, "socket-id is not supported");
> +            return;
> +        }
> +
> +        /* skip slots with explicit mismatch */
> +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> +                continue;
> +        }
> +
> +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> +                continue;
> +        }
> +
> +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> +                continue;
> +        }

nit: above 3 if-conditions could be condensed into 1 compound condition

> +
> +        /* reject assignment if slot is already assigned, for compatibility
> +         * of legacy cpu_index mapping with SPAPR core based mapping do not
> +         * error out if cpu thread and matched core have the same node-id */
> +        if (slot->props.has_node_id &&
> +            slot->props.node_id != props->node_id) {
> +            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
> +                       slot->props.node_id);
> +            return;
> +        }
> +
> +        /* assign slot to node as it's matched '-numa cpu' key */
> +        match = true;
> +        slot->props.node_id = props->node_id;
> +        slot->props.has_node_id = props->has_node_id;
> +    }
> +
> +    if (!match) {
> +        error_setg(errp, "no match found");
> +    }
> +}
> +
>  static void machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> diff --git a/numa.c b/numa.c
> index b517870..5ff1212 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
>          exit(1);
>      }
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> +        CpuInstanceProperties props;
>          if (cpus->value >= max_cpus) {
>              error_setg(errp,
>                         "CPU index (%" PRIu16 ")"
> @@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> +        props.node_id = nodenr;
> +        props.has_node_id = true;
> +        machine_set_cpu_numa_node(ms, &props, &error_fatal);
>      }
>  
>      if (node->has_mem && node->has_memdev) {
> @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
>          if (i == nb_numa_nodes) {
>              for (i = 0; i < max_cpus; i++) {
>                  CpuInstanceProperties props;
> +                /* fetch default mapping from board and enable it */
>                  props = mc->cpu_index_to_instance_props(ms, i);
> +                props.has_node_id = true;
>  
>                  set_bit(i, numa_info[props.node_id].node_cpu);
> +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
>              }
>          }
>  
> -- 
> 2.7.4
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

On Thu, 4 May 2017 13:40:18 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Wed, May 03, 2017 at 02:57:04PM +0200, Igor Mammedov wrote:
> > Introduce machine_set_cpu_numa_node() helper that stores
> > node mapping for CPU in MachineState::possible_cpus.
> > CPU and node it belongs to is specified by 'props' argument.
> > 
> > Patch doesn't remove old way of storing mapping in
> > numa_info[X].node_cpu as removing it at the same time
> > makes patch rather big. Instead it just mirrors mapping
> > in possible_cpus and follow up per target patches will
> > switch to possible_cpus and numa_info[X].node_cpu will
> > be removed once there isn't any users left.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  include/hw/boards.h |  2 ++
> >  hw/core/machine.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  numa.c              |  8 +++++++
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 5d6af21..1f518a1 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -42,6 +42,8 @@ bool machine_dump_guest_core(MachineState *machine);
> >  bool machine_mem_merge(MachineState *machine);
> >  void machine_register_compat_props(MachineState *machine);
> >  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp);
> >  
> >  /**
> >   * CPUArchId:
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ada9eea..a63f17b 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -388,6 +388,74 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
> >      return head;
> >  }
> >  
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp)
> > +{
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    bool match = false;
> > +    int i;
> > +
> > +    if (!mc->possible_cpu_arch_ids) {
> > +        error_setg(errp, "mapping of CPUs to NUMA node is not supported");
> > +        return;
> > +    }
> > +
> > +    /* force board to initialize possible_cpus if it hasn't been done yet */
> > +    mc->possible_cpu_arch_ids(machine);
> > +
> > +    for (i = 0; i < machine->possible_cpus->len; i++) {
> > +        CPUArchId *slot = &machine->possible_cpus->cpus[i];
> > +
> > +        /* reject unsupported by board properties */
> > +        if (props->has_thread_id && !slot->props.has_thread_id) {
> > +            error_setg(errp, "thread-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_core_id && !slot->props.has_core_id) {
> > +            error_setg(errp, "core-id is not supported");
> > +            return;
> > +        }
> > +
> > +        if (props->has_socket_id && !slot->props.has_socket_id) {
> > +            error_setg(errp, "socket-id is not supported");
> > +            return;
> > +        }
> > +
> > +        /* skip slots with explicit mismatch */
> > +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> > +                continue;
> > +        }  
> 
> nit: above 3 if-conditions could be condensed into 1 compound condition
I'll merge them on the respin

> 
> > +
> > +        /* reject assignment if slot is already assigned, for compatibility
> > +         * of legacy cpu_index mapping with SPAPR core based mapping do not
> > +         * error out if cpu thread and matched core have the same node-id */
> > +        if (slot->props.has_node_id &&
> > +            slot->props.node_id != props->node_id) {
> > +            error_setg(errp, "CPU is already assigned to node-id: %" PRId64,
> > +                       slot->props.node_id);
> > +            return;
> > +        }
> > +
> > +        /* assign slot to node as it's matched '-numa cpu' key */
> > +        match = true;
> > +        slot->props.node_id = props->node_id;
> > +        slot->props.has_node_id = props->has_node_id;
> > +    }
> > +
> > +    if (!match) {
> > +        error_setg(errp, "no match found");
> > +    }
> > +}
> > +
> >  static void machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > diff --git a/numa.c b/numa.c
> > index b517870..5ff1212 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -169,6 +169,7 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >          exit(1);
> >      }
> >      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> > +        CpuInstanceProperties props;
> >          if (cpus->value >= max_cpus) {
> >              error_setg(errp,
> >                         "CPU index (%" PRIu16 ")"
> > @@ -177,6 +178,10 @@ static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> >              return;
> >          }
> >          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> > +        props = mc->cpu_index_to_instance_props(ms, cpus->value);
> > +        props.node_id = nodenr;
> > +        props.has_node_id = true;
> > +        machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >      }
> >  
> >      if (node->has_mem && node->has_memdev) {
> > @@ -393,9 +398,12 @@ void parse_numa_opts(MachineState *ms)
> >          if (i == nb_numa_nodes) {
> >              for (i = 0; i < max_cpus; i++) {
> >                  CpuInstanceProperties props;
> > +                /* fetch default mapping from board and enable it */
> >                  props = mc->cpu_index_to_instance_props(ms, i);
> > +                props.has_node_id = true;
> >  
> >                  set_bit(i, numa_info[props.node_id].node_cpu);
> > +                machine_set_cpu_numa_node(ms, &props, &error_fatal);
> >              }
> >          }
> >  
> > -- 
> > 2.7.4
> >  
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com> 

On Thu, 4 May 2017 13:40:18 +0200
Andrew Jones <drjones@redhat.com> wrote:

[...]
> > +void machine_set_cpu_numa_node(MachineState *machine,
> > +                               CpuInstanceProperties *props, Error **errp)
> > +{
[...]

> > +        }
> > +
> > +        /* skip slots with explicit mismatch */
> > +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> > +                continue;
> > +        }
> > +
> > +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> > +                continue;
> > +        }  
> 
> nit: above 3 if-conditions could be condensed into 1 compound condition
this reduces number of lines but result is a bit ugly due to 80chr/line limit:

        /* skip slots with explicit mismatch */                                  
        if ((props->has_thread_id &&                                             
             props->thread_id != slot->props.thread_id) ||                       
            (props->has_core_id && props->core_id != slot->props.core_id) ||     
            (props->has_socket_id && props->socket_id != slot->props.socket_id)  
        ) {                                                                      
                continue;                                                        
        }

do you still want the change?

On Fri, May 05, 2017 at 01:28:26PM +0200, Igor Mammedov wrote:
> On Thu, 4 May 2017 13:40:18 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> [...]
> > > +void machine_set_cpu_numa_node(MachineState *machine,
> > > +                               CpuInstanceProperties *props, Error **errp)
> > > +{
> [...]
> 
> > > +        }
> > > +
> > > +        /* skip slots with explicit mismatch */
> > > +        if (props->has_thread_id && props->thread_id != slot->props.thread_id) {
> > > +                continue;
> > > +        }
> > > +
> > > +        if (props->has_core_id && props->core_id != slot->props.core_id) {
> > > +                continue;
> > > +        }
> > > +
> > > +        if (props->has_socket_id && props->socket_id != slot->props.socket_id) {
> > > +                continue;
> > > +        }  
> > 
> > nit: above 3 if-conditions could be condensed into 1 compound condition
> this reduces number of lines but result is a bit ugly due to 80chr/line limit:
> 
>         /* skip slots with explicit mismatch */                                  
>         if ((props->has_thread_id &&                                             
>              props->thread_id != slot->props.thread_id) ||                       
>             (props->has_core_id && props->core_id != slot->props.core_id) ||     
>             (props->has_socket_id && props->socket_id != slot->props.socket_id)  
>         ) {                                                                      
>                 continue;                                                        
>         }
> 
> do you still want the change?

If only we could kill the 80 char limit! I don't feel strongly about any
of this stuff, and you've already got my R-b either way.

Thanks,
drew

> 
> 
> 
>