Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/numa.h |  1 +
 hw/arm/virt.c         | 16 ++--------------
 hw/i386/pc.c          | 17 +----------------
 hw/ppc/spapr.c        | 17 +----------------
 numa.c                | 22 ++++++++++++++++++++++
 5 files changed, 27 insertions(+), 46 deletions(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 7ffde5b..610eece 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
                                   int nb_nodes, ram_addr_t size);
+void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c7c8159..ce676df 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
     for (n = 0; n < possible_cpus->len; n++) {
         Object *cpuobj;
         CPUState *cs;
-        int node_id;
 
         if (n >= smp_cpus) {
             break;
@@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
         cs = CPU(cpuobj);
         cs->cpu_index = n;
 
-        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
-        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
-            /* by default CPUState::numa_node was 0 if it's not set via CLI
-             * keep it this way for now but in future we probably should
-             * refuse to start up with incomplete numa mapping */
-             node_id = 0;
-        }
-        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
-            cs->numa_node = node_id;
-        } else {
-            /* CPU isn't device_add compatible yet, this shouldn't happen */
-            error_setg(&error_abort, "user set node-id not implemented");
-        }
+        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
+                          &error_fatal);
 
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e36a375..d83c158 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
-    int node_id;
     CPUState *cs;
     CPUArchId *cpu_slot;
     X86CPUTopoInfo topo;
@@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     cs = CPU(cpu);
     cs->cpu_index = idx;
 
-    node_id = cpu_slot->props.node_id;
-    if (!cpu_slot->props.has_node_id) {
-        /* by default CPUState::numa_node was 0 if it's not set via CLI
-         * keep it this way for now but in future we probably should
-         * refuse to start up with incomplete numa mapping */
-        node_id = 0;
-    }
-    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
-        cs->numa_node = node_id;
-    } else if (cs->numa_node != node_id) {
-            error_setg(errp, "node-id %d must match numa node specified"
-                "with -numa option for cpu-index %d",
-                cs->numa_node, cs->cpu_index);
-            return;
-    }
+    numa_cpu_pre_plug(cpu_slot, dev, errp);
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0980d73..c7fee8b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
-    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
     char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
     const char *type = object_get_typename(OBJECT(dev));
     CPUArchId *core_slot;
-    int node_id;
     int index;
 
     if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
@@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    node_id = core_slot->props.node_id;
-    if (!core_slot->props.has_node_id) {
-        /* by default CPUState::numa_node was 0 if it's not set via CLI
-         * keep it this way for now but in future we probably should
-         * refuse to start up with incomplete numa mapping */
-        node_id = 0;
-    }
-    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
-        sc->node_id = node_id;
-    } else if (sc->node_id != node_id) {
-        error_setg(&local_err, "node-id %d must match numa node specified"
-            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
-        goto out;
-    }
+    numa_cpu_pre_plug(core_slot, dev, &local_err);
 
 out:
     g_free(base_core_type);
diff --git a/numa.c b/numa.c
index ca73145..0115bfd 100644
--- a/numa.c
+++ b/numa.c
@@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
     }
 }
 
+void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
+{
+    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);
+
+    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
+        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
+         * TODO: make it error when incomplete numa mapping support is removed
+         */
+        node_id = 0;
+
+        /* due to bug in libvirt, it doesn't pass node-id from props on
+         * device_add as expected, so we have to fix it up here */
+        if (slot->props.has_node_id) {
+            node_id = slot->props.node_id;
+        }
+        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
+    } else if (node_id != slot->props.node_id) {
+        error_setg(errp, "node-id=%d must match numa node specified "
+                   "with -numa option", node_id);
+    }
+}
+
 static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
                                            const char *name,
                                            uint64_t ram_size)
-- 
2.7.4

On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/sysemu/numa.h |  1 +
>  hw/arm/virt.c         | 16 ++--------------
>  hw/i386/pc.c          | 17 +----------------
>  hw/ppc/spapr.c        | 17 +----------------
>  numa.c                | 22 ++++++++++++++++++++++
>  5 files changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 7ffde5b..610eece 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);

I understand an explicitly call to numa_cpu_pre_plug() is needed
on spapr_core_pre_plug() because it is not handling a TYPE_CPU
object. But why not adding a numa_cpu_pre_plug() call to
cpu_common_realizefn(), so the explicit calls in machvirt_init()
and pc_cpu_pre_plug() are not necessary?

Adding the code to cpu_common_realizefn() would also ensure
CPUState::node_id is set consistently, even if hotplug was not
done at thread level.


>  #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c7c8159..ce676df 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
>      for (n = 0; n < possible_cpus->len; n++) {
>          Object *cpuobj;
>          CPUState *cs;
> -        int node_id;
>  
>          if (n >= smp_cpus) {
>              break;
> @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
>          cs = CPU(cpuobj);
>          cs->cpu_index = n;
>  
> -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> -             * keep it this way for now but in future we probably should
> -             * refuse to start up with incomplete numa mapping */
> -             node_id = 0;
> -        }
> -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -            cs->numa_node = node_id;
> -        } else {
> -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> -            error_setg(&error_abort, "user set node-id not implemented");
> -        }
> +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> +                          &error_fatal);
>  
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e36a375..d83c158 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> -    int node_id;
>      CPUState *cs;
>      CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
> @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
>  
> -    node_id = cpu_slot->props.node_id;
> -    if (!cpu_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -        cs->numa_node = node_id;
> -    } else if (cs->numa_node != node_id) {
> -            error_setg(errp, "node-id %d must match numa node specified"
> -                "with -numa option for cpu-index %d",
> -                cs->numa_node, cs->cpu_index);
> -            return;
> -    }
> +    numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..c7fee8b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
> -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
>      CPUArchId *core_slot;
> -    int node_id;
>      int index;
>  
>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> -    node_id = core_slot->props.node_id;
> -    if (!core_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> -        sc->node_id = node_id;
> -    } else if (sc->node_id != node_id) {
> -        error_setg(&local_err, "node-id %d must match numa node specified"
> -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> -        goto out;
> -    }
> +    numa_cpu_pre_plug(core_slot, dev, &local_err);
>  
>  out:
>      g_free(base_core_type);
> diff --git a/numa.c b/numa.c
> index ca73145..0115bfd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
>      }
>  }
>  
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> +{
> +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);

You don't check for errors here. If they will never happen,
should we use &error_abort instead?

> +
> +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> +         * TODO: make it error when incomplete numa mapping support is removed
> +         */
> +        node_id = 0;
> +
> +        /* due to bug in libvirt, it doesn't pass node-id from props on
> +         * device_add as expected, so we have to fix it up here */
> +        if (slot->props.has_node_id) {
> +            node_id = slot->props.node_id;
> +        }
> +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> +    } else if (node_id != slot->props.node_id) {
> +        error_setg(errp, "node-id=%d must match numa node specified "
> +                   "with -numa option", node_id);

There's less detail on the error message, now. Probably harmless,
but I would like to understand when exactly this can be
triggered: is device_add the only way to trigger this error
message?

> +    }
> +}
> +
>  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                             const char *name,
>                                             uint64_t ram_size)
> -- 
> 2.7.4
> 

-- 
Eduardo

On Thu, 18 May 2017 15:19:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/sysemu/numa.h |  1 +
> >  hw/arm/virt.c         | 16 ++--------------
> >  hw/i386/pc.c          | 17 +----------------
> >  hw/ppc/spapr.c        | 17 +----------------
> >  numa.c                | 22 ++++++++++++++++++++++
> >  5 files changed, 27 insertions(+), 46 deletions(-)
> > 
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 7ffde5b..610eece 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >                                   int nb_nodes, ram_addr_t size);
> >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >                                    int nb_nodes, ram_addr_t size);
> > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);  
> 
> I understand an explicitly call to numa_cpu_pre_plug() is needed
> on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> object. But why not adding a numa_cpu_pre_plug() call to
> cpu_common_realizefn(), so the explicit calls in machvirt_init()
> and pc_cpu_pre_plug() are not necessary?
1. of the reasons is not to pollute all cpus with numa code

> Adding the code to cpu_common_realizefn() would also ensure
> CPUState::node_id is set consistently, even if hotplug was not
> done at thread level.
2. not all CPUs have node-id property
3. call site of thread_realize() in encapsulating object (spapr_core)
   might be somewhere in the middle of parent's realize and likely
   failure would need proper parent state rollback/cleanup.
4. and finely it's not cpu's responsibility to assign/check
   node-id property. It's machine's job that owns/manages topology
   layout. It' the same like with socket/core/thread properties.
   So for the sake of small optimization, I'm not really willing
   to violate that.
 
> >  #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c7c8159..ce676df 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
> >      for (n = 0; n < possible_cpus->len; n++) {
> >          Object *cpuobj;
> >          CPUState *cs;
> > -        int node_id;
> >  
> >          if (n >= smp_cpus) {
> >              break;
> > @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
> >          cs = CPU(cpuobj);
> >          cs->cpu_index = n;
> >  
> > -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> > -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> > -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -             * keep it this way for now but in future we probably should
> > -             * refuse to start up with incomplete numa mapping */
> > -             node_id = 0;
> > -        }
> > -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > -            cs->numa_node = node_id;
> > -        } else {
> > -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> > -            error_setg(&error_abort, "user set node-id not implemented");
> > -        }
> > +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> > +                          &error_fatal);
> >  
> >          if (!vms->secure) {
> >              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e36a375..d83c158 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >                              DeviceState *dev, Error **errp)
> >  {
> >      int idx;
> > -    int node_id;
> >      CPUState *cs;
> >      CPUArchId *cpu_slot;
> >      X86CPUTopoInfo topo;
> > @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >      cs = CPU(cpu);
> >      cs->cpu_index = idx;
> >  
> > -    node_id = cpu_slot->props.node_id;
> > -    if (!cpu_slot->props.has_node_id) {
> > -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -         * keep it this way for now but in future we probably should
> > -         * refuse to start up with incomplete numa mapping */
> > -        node_id = 0;
> > -    }
> > -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > -        cs->numa_node = node_id;
> > -    } else if (cs->numa_node != node_id) {
> > -            error_setg(errp, "node-id %d must match numa node specified"
> > -                "with -numa option for cpu-index %d",
> > -                cs->numa_node, cs->cpu_index);
> > -            return;
> > -    }
> > +    numa_cpu_pre_plug(cpu_slot, dev, errp);
> >  }
> >  
> >  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0980d73..c7fee8b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> >      Error *local_err = NULL;
> >      CPUCore *cc = CPU_CORE(dev);
> > -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> >      const char *type = object_get_typename(OBJECT(dev));
> >      CPUArchId *core_slot;
> > -    int node_id;
> >      int index;
> >  
> >      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> > @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          goto out;
> >      }
> >  
> > -    node_id = core_slot->props.node_id;
> > -    if (!core_slot->props.has_node_id) {
> > -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -         * keep it this way for now but in future we probably should
> > -         * refuse to start up with incomplete numa mapping */
> > -        node_id = 0;
> > -    }
> > -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> > -        sc->node_id = node_id;
> > -    } else if (sc->node_id != node_id) {
> > -        error_setg(&local_err, "node-id %d must match numa node specified"
> > -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> > -        goto out;
> > -    }
> > +    numa_cpu_pre_plug(core_slot, dev, &local_err);
> >  
> >  out:
> >      g_free(base_core_type);
> > diff --git a/numa.c b/numa.c
> > index ca73145..0115bfd 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
> >      }
> >  }
> >  
> > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> > +{
> > +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);  
> 
> You don't check for errors here. If they will never happen,
> should we use &error_abort instead?
sure, I'll use &error_abort in v2

> > +
> > +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> > +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> > +         * TODO: make it error when incomplete numa mapping support is removed
> > +         */
> > +        node_id = 0;
> > +
> > +        /* due to bug in libvirt, it doesn't pass node-id from props on
> > +         * device_add as expected, so we have to fix it up here */
> > +        if (slot->props.has_node_id) {
> > +            node_id = slot->props.node_id;
> > +        }
> > +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> > +    } else if (node_id != slot->props.node_id) {
> > +        error_setg(errp, "node-id=%d must match numa node specified "
> > +                   "with -numa option", node_id);  
> 
> There's less detail on the error message, now. Probably harmless,
> but I would like to understand when exactly this can be
> triggered: is device_add the only way to trigger this error
> message?
error is triggered only during -device/device_add so there were no
need in more verbose error as device_add will report its arguments
(affected cpu in this case)


> 
> > +    }
> > +}
> > +
> >  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> >                                             const char *name,
> >                                             uint64_t ram_size)
> > -- 
> > 2.7.4
> >   
> 

On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:19:13 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  include/sysemu/numa.h |  1 +
> > >  hw/arm/virt.c         | 16 ++--------------
> > >  hw/i386/pc.c          | 17 +----------------
> > >  hw/ppc/spapr.c        | 17 +----------------
> > >  numa.c                | 22 ++++++++++++++++++++++
> > >  5 files changed, 27 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > index 7ffde5b..610eece 100644
> > > --- a/include/sysemu/numa.h
> > > +++ b/include/sysemu/numa.h
> > > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > >                                   int nb_nodes, ram_addr_t size);
> > >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > >                                    int nb_nodes, ram_addr_t size);
> > > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);  
> > 
> > I understand an explicitly call to numa_cpu_pre_plug() is needed
> > on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> > object. But why not adding a numa_cpu_pre_plug() call to
> > cpu_common_realizefn(), so the explicit calls in machvirt_init()
> > and pc_cpu_pre_plug() are not necessary?
> 1. of the reasons is not to pollute all cpus with numa code

I understand this goal...

> 
> > Adding the code to cpu_common_realizefn() would also ensure
> > CPUState::node_id is set consistently, even if hotplug was not
> > done at thread level.
> 2. not all CPUs have node-id property

...and this. But: we already have the CPUState::numa_node field.
If we don't handle it in common code, we risk leaving the field
uninitialized, which is a problem if other code tries to use the
field for something.

Maybe that's an argument for removing the CPUState::numa_node
field too.


> 3. call site of thread_realize() in encapsulating object (spapr_core)
>    might be somewhere in the middle of parent's realize and likely
>    failure would need proper parent state rollback/cleanup.

I don't see why this could be a problem, if the code setting
realized=true is already supposed to handle errors on the realize
method.

> 4. and finely it's not cpu's responsibility to assign/check
>    node-id property. It's machine's job that owns/manages topology
>    layout. It' the same like with socket/core/thread properties.
>    So for the sake of small optimization, I'm not really willing
>    to violate that.

I don't disagree with that, but in that case I would like to
remove the CPUState::numa_node field soon, if possible.

[...]
> > > +    } else if (node_id != slot->props.node_id) {
> > > +        error_setg(errp, "node-id=%d must match numa node specified "
> > > +                   "with -numa option", node_id);  
> > 
> > There's less detail on the error message, now. Probably harmless,
> > but I would like to understand when exactly this can be
> > triggered: is device_add the only way to trigger this error
> > message?
> error is triggered only during -device/device_add so there were no
> need in more verbose error as device_add will report its arguments
> (affected cpu in this case)

OK.

-- 
Eduardo

On Mon, 22 May 2017 09:58:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> > On Thu, 18 May 2017 15:19:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:  
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  include/sysemu/numa.h |  1 +
> > > >  hw/arm/virt.c         | 16 ++--------------
> > > >  hw/i386/pc.c          | 17 +----------------
> > > >  hw/ppc/spapr.c        | 17 +----------------
> > > >  numa.c                | 22 ++++++++++++++++++++++
> > > >  5 files changed, 27 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > index 7ffde5b..610eece 100644
> > > > --- a/include/sysemu/numa.h
> > > > +++ b/include/sysemu/numa.h
> > > > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > > >                                   int nb_nodes, ram_addr_t size);
> > > >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > > >                                    int nb_nodes, ram_addr_t size);
> > > > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);    
> > > 
> > > I understand an explicitly call to numa_cpu_pre_plug() is needed
> > > on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> > > object. But why not adding a numa_cpu_pre_plug() call to
> > > cpu_common_realizefn(), so the explicit calls in machvirt_init()
> > > and pc_cpu_pre_plug() are not necessary?  
> > 1. of the reasons is not to pollute all cpus with numa code  
> 
> I understand this goal...
> 
> >   
> > > Adding the code to cpu_common_realizefn() would also ensure
> > > CPUState::node_id is set consistently, even if hotplug was not
> > > done at thread level.  
> > 2. not all CPUs have node-id property  
> 
> ...and this. But: we already have the CPUState::numa_node field.
> If we don't handle it in common code, we risk leaving the field
> uninitialized, which is a problem if other code tries to use the
> field for something.
> 
> Maybe that's an argument for removing the CPUState::numa_node
> field too.
> 
> 
> > 3. call site of thread_realize() in encapsulating object (spapr_core)
> >    might be somewhere in the middle of parent's realize and likely
> >    failure would need proper parent state rollback/cleanup.  
> 
> I don't see why this could be a problem, if the code setting
> realized=true is already supposed to handle errors on the realize
> method.
> 
> > 4. and finely it's not cpu's responsibility to assign/check
> >    node-id property. It's machine's job that owns/manages topology
> >    layout. It' the same like with socket/core/thread properties.
> >    So for the sake of small optimization, I'm not really willing
> >    to violate that.  
> 
> I don't disagree with that, but in that case I would like to
> remove the CPUState::numa_node field soon, if possible.
ok, I'll try add a patch on respin to do it (I think that I've
tried this at some RFC time)

On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  include/sysemu/numa.h |  1 +
>  hw/arm/virt.c         | 16 ++--------------
>  hw/i386/pc.c          | 17 +----------------
>  hw/ppc/spapr.c        | 17 +----------------
>  numa.c                | 22 ++++++++++++++++++++++
>  5 files changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 7ffde5b..610eece 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
>  #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c7c8159..ce676df 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
>      for (n = 0; n < possible_cpus->len; n++) {
>          Object *cpuobj;
>          CPUState *cs;
> -        int node_id;
>  
>          if (n >= smp_cpus) {
>              break;
> @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
>          cs = CPU(cpuobj);
>          cs->cpu_index = n;
>  
> -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> -             * keep it this way for now but in future we probably should
> -             * refuse to start up with incomplete numa mapping */
> -             node_id = 0;
> -        }
> -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -            cs->numa_node = node_id;
> -        } else {
> -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> -            error_setg(&error_abort, "user set node-id not implemented");
> -        }
> +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> +                          &error_fatal);
>  
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e36a375..d83c158 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> -    int node_id;
>      CPUState *cs;
>      CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
> @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
>  
> -    node_id = cpu_slot->props.node_id;
> -    if (!cpu_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -        cs->numa_node = node_id;
> -    } else if (cs->numa_node != node_id) {
> -            error_setg(errp, "node-id %d must match numa node specified"
> -                "with -numa option for cpu-index %d",
> -                cs->numa_node, cs->cpu_index);
> -            return;
> -    }
> +    numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..c7fee8b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
> -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
>      CPUArchId *core_slot;
> -    int node_id;
>      int index;
>  
>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> -    node_id = core_slot->props.node_id;
> -    if (!core_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> -        sc->node_id = node_id;
> -    } else if (sc->node_id != node_id) {
> -        error_setg(&local_err, "node-id %d must match numa node specified"
> -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> -        goto out;
> -    }
> +    numa_cpu_pre_plug(core_slot, dev, &local_err);
>  
>  out:
>      g_free(base_core_type);
> diff --git a/numa.c b/numa.c
> index ca73145..0115bfd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
>      }
>  }
>  
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> +{
> +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);
> +
> +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> +         * TODO: make it error when incomplete numa mapping support is removed
> +         */
> +        node_id = 0;
> +
> +        /* due to bug in libvirt, it doesn't pass node-id from props on
> +         * device_add as expected, so we have to fix it up here */
> +        if (slot->props.has_node_id) {
> +            node_id = slot->props.node_id;
> +        }
> +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> +    } else if (node_id != slot->props.node_id) {
> +        error_setg(errp, "node-id=%d must match numa node specified "
> +                   "with -numa option", node_id);
> +    }
> +}
> +
>  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                             const char *name,
>                                             uint64_t ram_size)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson