[Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Igor Mammedov]

Series introduces a new CLI option to allow mapping cpus to numa
nodes using public properties [socket|core|thread]-ids instead of
internal cpu-index and moving cpu<->node mapping from global bitmaps
to PCMachineState struct.

It focuses on PC in RFC version to see if approach is suitable in general,
later we can move generic parts to common code or do a bigger non RFC
version of this series that would take care of all concerned targets
(arm-virt/spapr/pc).
Thing which I've kept PC specific but could be generalized is:
  * moving array possible_cpus to generic MachineState where
    we already have possible_cpu_arch_ids(). I plan to reuse it
    in virt-arm for CPU hotplug/-device cpu support and as this series
    shows it also could be used for numa mapping.
    Perhaps it could be used in simmilar manner for core based SPAPR
    but I haven't looked it yet. So any opions on if we should move it there.

So far it only provides CLI interface to do mapping, there is no QMP
one as I haven't found a suitable place/way to update/set mapping
after machine_done for QEMU started with -S (stoppod mode) so that
mgmt could query hopluggable-cpus first, then map them to numa nodes
in runtime before actually allowing guest to run.
Any suggestions on how to make it work are welcome.

PS:
skipped travis-ci build as currently it seems to be broken.

CC: Dou Liyang <douly.fnst@cn.fujitsu.com>
CC: fanc.fnst@cn.fujitsu.com
CC: caoj.fnst@cn.fujitsu.com
CC: stefanha@redhat.com
CC: izumi.taku@jp.fujitsu.com
CC: vilanova@ac.upc.edu
CC: ehabkost@redhat.com
CC: peter.maydell@linaro.org
CC: Andrew Jones <drjones@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Thomas Huth <thuth@redhat.com>

Igor Mammedov (13):
  numa: access CPU's node id via property in hmp_info_numa()
  pc: cleanup: move smbios_set_cpuid() into pc_build_smbios()
  pc: don't return cpu pointer from pc_new_cpu() as it's not needed
    anymore
  make possible_cpu_arch_ids() return const pointer
  pc: move pcms->possible_cpus init out of pc_cpus_init()
  pc: calculate topology only once when possible_cpus is initialised
  pc: pass apic_id to pc_find_cpu_slot() directly so lookup could be
    done without CPU object
  pc: add writeonly 'cpu' property to PCMachine
  numa: introduce '-numa cpu' cpu option
  numa: replace cpu_index_to_socket_id() with
    cpu_index_to_instance_props()
  numa: use new machine.cpu property with -numa cpus=... CLI
  pc: drop usage of legacy numa_get_node_for_cpu()
  pc: cpu: make sure that cpu.node-id matches -numa mapping

 include/hw/acpi/acpi_dev_interface.h |   2 +-
 include/hw/boards.h                  |  14 ++-
 include/hw/i386/pc.h                 |   2 +-
 include/qom/cpu.h                    |   2 -
 include/sysemu/numa.h                |   3 +-
 target/arm/cpu.h                     |   2 +
 target/i386/cpu.h                    |   1 +
 target/ppc/cpu.h                     |   2 +
 hw/acpi/cpu.c                        |  13 +--
 hw/acpi/cpu_hotplug.c                |   4 +-
 hw/arm/virt.c                        |  12 +-
 hw/i386/acpi-build.c                 |  25 ++---
 hw/i386/pc.c                         | 210 +++++++++++++++++++++++------------
 hw/ppc/spapr.c                       |  15 ++-
 hw/ppc/spapr_cpu_core.c              |   2 +-
 monitor.c                            |   7 +-
 numa.c                               |  89 +++++++++++----
 qapi-schema.json                     |   3 +-
 stubs/pc_madt_cpu_entry.c            |   2 +-
 target/arm/cpu.c                     |   1 +
 target/i386/cpu.c                    |   1 +
 target/ppc/translate_init.c          |   1 +
 vl.c                                 |   4 +-
 23 files changed, 269 insertions(+), 148 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC 01/13] numa: access CPU's node id via property in hmp_info_numa()

[Igor Mammedov]

Move vcpu's assocciated numa_node field out of generic CPUState
into inherited classes that actually care about cpu<->numa mapping
and make monitor 'info numa' get vcpu's assocciated node id via
node-id property.
It allows to drop implicit node id intialization in
numa_post_machine_init() and would allow switching to mapping
defined by target's CpuInstanceProperties instead of global
numa_info[i].node_cpu bitmaps.

As side effect it fixes 'info numa' displaying wrong mapping
for CPUs specified with -device/device_add.
Before patch following CLI would produce:
QEMU -smp 1,sockets=3,maxcpus=3 \
       -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
       -numa node,nodeid=0,cpus=0 \
       -numa node,nodeid=1,cpus=1 \
       -numa node,nodeid=2,cpus=2
(qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
(qemu) info numa
3 nodes
node 0 cpus: 0 1 2
node 0 size: 40 MB
node 1 cpus:
node 1 size: 40 MB
node 2 cpus:
node 2 size: 48 MB

after patch all CPUs are on nodes they are supposed to be:
(qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
(qemu) info numa
3 nodes
node 0 cpus: 0
node 0 size: 40 MB
node 1 cpus: 1
node 1 size: 40 MB
node 2 cpus: 2
node 2 size: 48 MB

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Dou Liyang <douly.fnst@cn.fujitsu.com>
CC: fanc.fnst@cn.fujitsu.com
CC: caoj.fnst@cn.fujitsu.com
CC: stefanha@redhat.com
CC: izumi.taku@jp.fujitsu.com
CC: vilanova@ac.upc.edu
CC: ehabkost@redhat.com
CC: peter.maydell@linaro.org
CC: Andrew Jones <drjones@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Thomas Huth <thuth@redhat.com>
---
 include/qom/cpu.h           |  2 --
 include/sysemu/numa.h       |  1 -
 target/arm/cpu.h            |  2 ++
 target/i386/cpu.h           |  1 +
 target/ppc/cpu.h            |  2 ++
 hw/arm/virt.c               | 12 ++++++++----
 hw/i386/pc.c                |  5 +++++
 hw/ppc/spapr.c              |  2 +-
 hw/ppc/spapr_cpu_core.c     |  2 +-
 monitor.c                   |  7 +++++--
 numa.c                      | 15 ---------------
 target/arm/cpu.c            |  1 +
 target/i386/cpu.c           |  1 +
 target/ppc/translate_init.c |  1 +
 vl.c                        |  2 --
 15 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3f79a8e..ae637a9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -257,7 +257,6 @@ struct qemu_work_item;
  * @cpu_index: CPU index (informative).
  * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU.
- * @numa_node: NUMA node this CPU is belonging to.
  * @host_tid: Host thread ID.
  * @running: #true if CPU is currently running (lockless).
  * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
@@ -306,7 +305,6 @@ struct CPUState {
 
     int nr_cores;
     int nr_threads;
-    int numa_node;
 
     struct QemuThread *thread;
 #ifdef _WIN32
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 8f09dcf..b8015a5 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -25,7 +25,6 @@ typedef struct node_info {
 
 extern NodeInfo numa_info[MAX_NODES];
 void parse_numa_opts(MachineClass *mc);
-void numa_post_machine_init(void);
 void query_numa_node_mem(uint64_t node_mem[]);
 extern QemuOptsList qemu_numa_opts;
 void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7bd16ee..ef263f1 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -662,6 +662,8 @@ struct ARMCPU {
 
     ARMELChangeHook *el_change_hook;
     void *el_change_hook_opaque;
+
+    int32_t numa_nid;
 };
 
 static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6c1902b..e43dcc2 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1264,6 +1264,7 @@ struct X86CPU {
     int32_t socket_id;
     int32_t core_id;
     int32_t thread_id;
+    int32_t numa_nid;
 };
 
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2a50c43..2d12ad5 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1154,6 +1154,7 @@ do {                                            \
  * @cpu_dt_id: CPU index used in the device tree. KVM uses this index too
  * @max_compat: Maximal supported logical PVR from the command line
  * @cpu_version: Current logical PVR, zero if in "raw" mode
+ * @numa_nid: Numa node id the CPU belongs to
  *
  * A PowerPC CPU.
  */
@@ -1166,6 +1167,7 @@ struct PowerPCCPU {
     int cpu_dt_id;
     uint32_t max_compat;
     uint32_t cpu_version;
+    int32_t numa_nid;
 
     /* Fields related to migration compatibility hacks */
     bool pre_2_8_migration;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7a03f84..b86b5fd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -329,7 +329,6 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
 {
     int cpu;
     int addr_cells = 1;
-    unsigned int i;
 
     /*
      * From Documentation/devicetree/bindings/arm/cpus.txt
@@ -379,9 +378,9 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                                   armcpu->mp_affinity);
         }
 
-        i = numa_get_node_for_cpu(cpu);
-        if (i < nb_numa_nodes) {
-            qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id", i);
+        if (armcpu->numa_nid < nb_numa_nodes) {
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id",
+                                  armcpu->numa_nid);
         }
 
         g_free(nodename);
@@ -1333,6 +1332,11 @@ static void machvirt_init(MachineState *machine)
                                      "secure-memory", &error_abort);
         }
 
+        if (nb_numa_nodes) {
+            object_property_set_int(cpuobj, numa_get_node_for_cpu(n),
+                                    "node-id", NULL);
+        }
+
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
     fdt_add_timer_nodes(vms);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f721fde..9d2b265 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 
     cs = CPU(cpu);
     cs->cpu_index = idx;
+
+    idx = numa_get_node_for_cpu(cs->cpu_index);
+    if (idx < nb_numa_nodes) {
+        cpu->numa_nid = idx;
+    }
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 208ef7b..efcd925 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -182,7 +182,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs)
                                 cpu_to_be32(0x0),
                                 cpu_to_be32(0x0),
                                 cpu_to_be32(0x0),
-                                cpu_to_be32(cs->numa_node),
+                                cpu_to_be32(cpu->numa_nid),
                                 cpu_to_be32(index)};
 
     /* Advertise NUMA via ibm,associativity */
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c18632b..7f6661b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -71,7 +71,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
     /* Set NUMA node for the added CPUs  */
     i = numa_get_node_for_cpu(cs->cpu_index);
     if (i < nb_numa_nodes) {
-            cs->numa_node = i;
+        cpu->numa_nid = i;
     }
 
     xics_cpu_setup(spapr->xics, cpu);
diff --git a/monitor.c b/monitor.c
index 0841d43..8856d5b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1544,9 +1544,12 @@ static void hmp_info_numa(Monitor *mon, const QDict *qdict)
     for (i = 0; i < nb_numa_nodes; i++) {
         monitor_printf(mon, "node %d cpus:", i);
         CPU_FOREACH(cpu) {
-            if (cpu->numa_node == i) {
-                monitor_printf(mon, " %d", cpu->cpu_index);
+            Error *err = NULL;
+            int64_t nid = object_property_get_int(OBJECT(cpu), "node-id", &err);
+            if (nid == i && !err) {
+                    monitor_printf(mon, " %d", cpu->cpu_index);
             }
+            error_free(err);
         }
         monitor_printf(mon, "\n");
         monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
diff --git a/numa.c b/numa.c
index 379bc8a..5f68497 100644
--- a/numa.c
+++ b/numa.c
@@ -394,21 +394,6 @@ void parse_numa_opts(MachineClass *mc)
     }
 }
 
-void numa_post_machine_init(void)
-{
-    CPUState *cpu;
-    int i;
-
-    CPU_FOREACH(cpu) {
-        for (i = 0; i < nb_numa_nodes; i++) {
-            assert(cpu->cpu_index < max_cpus);
-            if (test_bit(cpu->cpu_index, numa_info[i].node_cpu)) {
-                cpu->numa_node = i;
-            }
-        }
-    }
-}
-
 static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
                                            const char *name,
                                            uint64_t ram_size)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9104611..8caf853 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1515,6 +1515,7 @@ static Property arm_cpu_properties[] = {
     DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
     DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
                         mp_affinity, ARM64_AFFINITY_INVALID),
+    DEFINE_PROP_INT32("node-id", ARMCPU, numa_nid, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index aba11ae..85c52f1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3649,6 +3649,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
     DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
 #endif
+    DEFINE_PROP_INT32("node-id", X86CPU, numa_nid, 0),
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
     DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index e6a835c..64bd7be 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -10520,6 +10520,7 @@ static gchar *ppc_gdb_arch_name(CPUState *cs)
 
 static Property ppc_cpu_properties[] = {
     DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, false),
+    DEFINE_PROP_INT32("node-id", PowerPCCPU, numa_nid, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/vl.c b/vl.c
index c643d3f..afe40ce 100644
--- a/vl.c
+++ b/vl.c
@@ -4549,8 +4549,6 @@ int main(int argc, char **argv, char **envp)
 
     cpu_synchronize_all_post_init();
 
-    numa_post_machine_init();
-
     if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
                           parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
         exit(1);
-- 
2.7.4

[Qemu-devel] [RFC 02/13] pc: cleanup: move smbios_set_cpuid() into pc_build_smbios()

[Igor Mammedov]

move smbios_set_cpuid() close to the rest of smbios init code
where it belongs to instead of calling it from pc_cpus_init().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
   it would allow to cleanup pc_new_cpu() in followup patch
---
 hw/i386/pc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9d2b265..2461d84 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -701,16 +701,20 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-static void pc_build_smbios(FWCfgState *fw_cfg)
+static void pc_build_smbios(PCMachineState *pcms)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
     struct smbios_phys_mem_area *mem_array;
     unsigned i, array_count;
+    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
+
+    /* tell smbios about cpuid version and features */
+    smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 
     smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
     if (smbios_tables) {
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
+        fw_cfg_add_bytes(pcms->fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_tables, smbios_tables_len);
     }
 
@@ -731,9 +735,9 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
     g_free(mem_array);
 
     if (smbios_anchor) {
-        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
+        fw_cfg_add_file(pcms->fw_cfg, "etc/smbios/smbios-tables",
                         smbios_tables, smbios_tables_len);
-        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-anchor",
+        fw_cfg_add_file(pcms->fw_cfg, "etc/smbios/smbios-anchor",
                         smbios_anchor, smbios_anchor_len);
     }
 }
@@ -1191,9 +1195,6 @@ void pc_cpus_init(PCMachineState *pcms)
             object_unref(OBJECT(cpu));
         }
     }
-
-    /* tell smbios about cpuid version and features */
-    smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
 
 static void pc_build_feature_control_file(PCMachineState *pcms)
@@ -1266,7 +1267,7 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
-        pc_build_smbios(pcms->fw_cfg);
+        pc_build_smbios(pcms);
         pc_build_feature_control_file(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
-- 
2.7.4

[Qemu-devel] [RFC 03/13] pc: don't return cpu pointer from pc_new_cpu() as it's not needed anymore

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2461d84..c0eba8a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1092,28 +1092,24 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
-                          Error **errp)
+static void pc_new_cpu(const char *typename, int64_t apic_id, Error **errp)
 {
-    X86CPU *cpu = NULL;
+    Object *cpu = NULL;
     Error *local_err = NULL;
 
-    cpu = X86_CPU(object_new(typename));
+    cpu = object_new(typename);
 
-    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
-    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+    object_property_set_int(cpu, apic_id, "apic-id", &local_err);
+    object_property_set_bool(cpu, true, "realized", &local_err);
 
+    object_unref(cpu);
     if (local_err) {
         error_propagate(errp, local_err);
-        object_unref(OBJECT(cpu));
-        cpu = NULL;
     }
-    return cpu;
 }
 
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
-    X86CPU *cpu;
     ObjectClass *oc;
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
@@ -1133,12 +1129,11 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */
     oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
-    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
+    pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
-    object_unref(OBJECT(cpu));
 }
 
 void pc_cpus_init(PCMachineState *pcms)
@@ -1148,7 +1143,6 @@ void pc_cpus_init(PCMachineState *pcms)
     ObjectClass *oc;
     const char *typename;
     gchar **model_pieces;
-    X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
 
     /* init CPUs */
@@ -1190,9 +1184,7 @@ void pc_cpus_init(PCMachineState *pcms)
         pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
         pcms->possible_cpus->len++;
         if (i < smp_cpus) {
-            cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
-                             &error_fatal);
-            object_unref(OBJECT(cpu));
+            pc_new_cpu(typename, x86_cpu_apic_id_from_index(i), &error_fatal);
         }
     }
 }
-- 
2.7.4

[Qemu-devel] [RFC 04/13] make possible_cpu_arch_ids() return const pointer

[Igor Mammedov]

make sure that external callers won't try to modify
possible_cpus and owner of possible_cpus can access
it directly when it modifies it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/acpi_dev_interface.h |  2 +-
 include/hw/boards.h                  |  2 +-
 include/hw/i386/pc.h                 |  2 +-
 hw/acpi/cpu.c                        |  6 ++----
 hw/acpi/cpu_hotplug.c                |  4 +---
 hw/i386/acpi-build.c                 |  8 +++-----
 hw/i386/pc.c                         | 10 +++-------
 stubs/pc_madt_cpu_entry.c            |  2 +-
 8 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 901a4ae..71d3c48 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -57,6 +57,6 @@ typedef struct AcpiDeviceIfClass {
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
     void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
-                     CPUArchIdList *apic_ids, GArray *entry);
+                     const CPUArchIdList *apic_ids, GArray *entry);
 } AcpiDeviceIfClass;
 #endif
diff --git a/include/hw/boards.h b/include/hw/boards.h
index a51da9c..ac891a8 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -135,7 +135,7 @@ struct MachineClass {
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
-    CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
+    const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
     HotpluggableCPUList *(*query_hotpluggable_cpus)(MachineState *machine);
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b22e699..f4a5941 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -361,7 +361,7 @@ uint16_t pvpanic_port(void);
 
 /* acpi-build.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       CPUArchIdList *apic_ids, GArray *entry);
+                       const CPUArchIdList *apic_ids, GArray *entry);
 
 /* e820 types */
 #define E820_RAM        1
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ac89fe..6017ca0 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -190,7 +190,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    CPUArchIdList *id_list;
+    const CPUArchIdList *id_list;
     int i;
 
     assert(mc->possible_cpu_arch_ids);
@@ -201,7 +201,6 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
         state->devs[i].cpu =  id_list->cpus[i].cpu;
         state->devs[i].arch_id = id_list->cpus[i].arch_id;
     }
-    g_free(id_list);
     memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, state,
                           "acpi-mem-hotplug", ACPI_CPU_HOTPLUG_REG_LEN);
     memory_region_add_subregion(as, base_addr, &state->ctrl_reg);
@@ -325,7 +324,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     Aml *one = aml_int(1);
     Aml *sb_scope = aml_scope("_SB");
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    CPUArchIdList *arch_ids = mc->possible_cpu_arch_ids(machine);
+    const CPUArchIdList *arch_ids = mc->possible_cpu_arch_ids(machine);
     char *cphp_res_path = g_strdup_printf("%s." CPUHP_RES_DEVICE, res_root);
     Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(obj);
@@ -574,5 +573,4 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     aml_append(table, method);
 
     g_free(cphp_res_path);
-    g_free(arch_ids);
 }
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index f15a240..5243918 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -128,7 +128,7 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
     Aml *zero = aml_int(0);
     Aml *one = aml_int(1);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
     PCMachineState *pcms = PC_MACHINE(machine);
 
     /*
@@ -329,8 +329,6 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
         apic_idx = apic_id + 1;
     }
     aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
-    g_free(apic_ids);
-
     aml_append(ctx, sb_scope);
 
     method = aml_method("\\_GPE._E02", 0, AML_NOTSERIALIZED);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0c8912f..0c91043 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -332,7 +332,7 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
 }
 
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       CPUArchIdList *apic_ids, GArray *entry)
+                       const CPUArchIdList *apic_ids, GArray *entry)
 {
     uint32_t apic_id = apic_ids->cpus[uid].arch_id;
 
@@ -373,7 +373,7 @@ static void
 build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
-    CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
     int madt_start = table_data->len;
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
     AcpiDeviceIf *adev = ACPI_DEVICE_IF(pcms->acpi_dev);
@@ -394,7 +394,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
             x2apic_mode = true;
         }
     }
-    g_free(apic_ids);
 
     io_apic = acpi_data_push(table_data, sizeof *io_apic);
     io_apic->type = ACPI_APIC_IO;
@@ -2294,7 +2293,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
     PCMachineState *pcms = PC_MACHINE(machine);
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
@@ -2393,7 +2392,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
                  (void *)(table_data->data + srat_start),
                  "SRAT",
                  table_data->len - srat_start, 1, NULL, NULL);
-    g_free(apic_ids);
 }
 
 static void
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0eba8a..a2ab7fb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2245,15 +2245,11 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
     return topo.pkg_id;
 }
 
-static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
+static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    int len = sizeof(CPUArchIdList) +
-              sizeof(CPUArchId) * (pcms->possible_cpus->len);
-    CPUArchIdList *list = g_malloc(len);
-
-    memcpy(list, pcms->possible_cpus, len);
-    return list;
+    assert(pcms->possible_cpus);
+    return pcms->possible_cpus;
 }
 
 static HotpluggableCPUList *pc_query_hotpluggable_cpus(MachineState *machine)
diff --git a/stubs/pc_madt_cpu_entry.c b/stubs/pc_madt_cpu_entry.c
index 427e772..f88d6a0 100644
--- a/stubs/pc_madt_cpu_entry.c
+++ b/stubs/pc_madt_cpu_entry.c
@@ -2,6 +2,6 @@
 #include "hw/i386/pc.h"
 
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       CPUArchIdList *apic_ids, GArray *entry)
+                       const CPUArchIdList *apic_ids, GArray *entry)
 {
 }
-- 
2.7.4

[Qemu-devel] [RFC 05/13] pc: move pcms->possible_cpus init out of pc_cpus_init()

[Igor Mammedov]

possible_cpus could be initialized earlier then cpu objects,
i.e. when -smp is parsed so move init code to possible_cpu_arch_ids()
interface func and do initialization on the first call.

it should help later with making -numa cpu/-smp parsing a machine state
properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a2ab7fb..7ec5304 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1143,7 +1143,9 @@ void pc_cpus_init(PCMachineState *pcms)
     ObjectClass *oc;
     const char *typename;
     gchar **model_pieces;
+    const CPUArchIdList *possible_cpus;
     MachineState *machine = MACHINE(pcms);
+    MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
     /* init CPUs */
     if (machine->cpu_model == NULL) {
@@ -1178,14 +1180,9 @@ void pc_cpus_init(PCMachineState *pcms)
      * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
      */
     pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
-    pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
-                                    sizeof(CPUArchId) * max_cpus);
-    for (i = 0; i < max_cpus; i++) {
-        pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
-        pcms->possible_cpus->len++;
-        if (i < smp_cpus) {
-            pc_new_cpu(typename, x86_cpu_apic_id_from_index(i), &error_fatal);
-        }
+    possible_cpus = mc->possible_cpu_arch_ids(machine);
+    for (i = 0; i < smp_cpus; i++) {
+        pc_new_cpu(typename, possible_cpus->cpus[i].arch_id, &error_fatal);
     }
 }
 
@@ -2248,7 +2245,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
 static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
-    assert(pcms->possible_cpus);
+    if (!pcms->possible_cpus) {
+        int i;
+        CPUArchIdList *possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
+                                                 sizeof(CPUArchId) * max_cpus);
+        possible_cpus->len = max_cpus;
+        for (i = 0; i < max_cpus; i++) {
+            possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
+        }
+        pcms->possible_cpus = possible_cpus;
+    }
+    /*
+     * make sure that max_cpus hasn't changed since the first use, i.e.
+     * -smp hasn't been parsed after it
+     */
+    assert(pcms->possible_cpus->len == max_cpus);
     return pcms->possible_cpus;
 }
 
-- 
2.7.4

[Qemu-devel] [RFC 06/13] pc: calculate topology only once when possible_cpus is initialised

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h |  1 +
 hw/i386/pc.c        | 25 +++++++++++++------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index ac891a8..6364617 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -49,6 +49,7 @@ void machine_register_compat_props(MachineState *machine);
  */
 typedef struct {
     uint64_t arch_id;
+    CpuInstanceProperties props;
     struct CPUState *cpu;
 } CPUArchId;
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7ec5304..ed41046 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2251,7 +2251,18 @@ static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
                                                  sizeof(CPUArchId) * max_cpus);
         possible_cpus->len = max_cpus;
         for (i = 0; i < max_cpus; i++) {
+            X86CPUTopoInfo topo;
+            CpuInstanceProperties *cpu_props = &possible_cpus->cpus[i].props;
+
             possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
+            x86_topo_ids_from_apicid(possible_cpus->cpus[i].arch_id,
+                                     smp_cores, smp_threads, &topo);
+            cpu_props->has_socket_id = true;
+            cpu_props->socket_id = topo.pkg_id;
+            cpu_props->has_core_id = true;
+            cpu_props->core_id = topo.core_id;
+            cpu_props->has_thread_id = true;
+            cpu_props->thread_id = topo.smt_id;
         }
         pcms->possible_cpus = possible_cpus;
     }
@@ -2276,23 +2287,13 @@ static HotpluggableCPUList *pc_query_hotpluggable_cpus(MachineState *machine)
     cpu_type = object_class_get_name(OBJECT_CLASS(CPU_GET_CLASS(cpu)));
 
     for (i = 0; i < pcms->possible_cpus->len; i++) {
-        X86CPUTopoInfo topo;
         HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
         HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
-        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
-        const uint32_t apic_id = pcms->possible_cpus->cpus[i].arch_id;
-
-        x86_topo_ids_from_apicid(apic_id, smp_cores, smp_threads, &topo);
 
         cpu_item->type = g_strdup(cpu_type);
         cpu_item->vcpus_count = 1;
-        cpu_props->has_socket_id = true;
-        cpu_props->socket_id = topo.pkg_id;
-        cpu_props->has_core_id = true;
-        cpu_props->core_id = topo.core_id;
-        cpu_props->has_thread_id = true;
-        cpu_props->thread_id = topo.smt_id;
-        cpu_item->props = cpu_props;
+        cpu_item->props = g_memdup(&pcms->possible_cpus->cpus[i].props,
+                                   sizeof(*cpu_item->props));
 
         cpu = pcms->possible_cpus->cpus[i].cpu;
         if (cpu) {
-- 
2.7.4

[Qemu-devel] [RFC 07/13] pc: pass apic_id to pc_find_cpu_slot() directly so lookup could be done without CPU object

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ed41046..f8ea635 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1776,13 +1776,12 @@ static int pc_apic_cmp(const void *a, const void *b)
  * in pcms->possible_cpus->cpus, if pcms->possible_cpus->cpus has no
  * entry correponding to CPU's apic_id returns NULL.
  */
-static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, CPUState *cpu,
+static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, uint32_t id,
                                    int *idx)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUArchId apic_id, *found_cpu;
 
-    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
+    apic_id.arch_id = id;
     found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
         pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
         pc_apic_cmp);
@@ -1798,6 +1797,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
     CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
+    X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
     if (pcms->acpi_dev) {
@@ -1817,7 +1817,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }
 
-    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
+    found_cpu = pc_find_cpu_slot(pcms, cpu->apic_id, NULL);
     found_cpu->cpu = CPU(dev);
 out:
     error_propagate(errp, local_err);
@@ -1828,9 +1828,10 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
     int idx = -1;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
+    X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
-    pc_find_cpu_slot(pcms, CPU(dev), &idx);
+    pc_find_cpu_slot(pcms, cpu->apic_id, &idx);
     assert(idx != -1);
     if (idx == 0) {
         error_setg(&local_err, "Boot CPU is unpluggable");
@@ -1855,6 +1856,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
+    X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
@@ -1864,7 +1866,7 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
+    found_cpu = pc_find_cpu_slot(pcms, cpu->apic_id, NULL);
     found_cpu->cpu = NULL;
     object_unparent(OBJECT(dev));
 
@@ -1922,7 +1924,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
     }
 
-    cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
+    cpu_slot = pc_find_cpu_slot(pcms, cpu->apic_id, &idx);
     if (!cpu_slot) {
         x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
         error_setg(errp, "Invalid CPU [socket: %u, core: %u, thread: %u] with"
-- 
2.7.4

[Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Igor Mammedov]

it will allow generic numa code to set cpu to numa node mapping
in target independent manner in the next patch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f8ea635..1d33a5e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
     pcms->pit = value;
 }
 
+static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    uint32_t apic_id;
+    X86CPUTopoInfo topo;
+    CPUArchId *cpu_slot;
+    Error *local_err = NULL;
+    CpuInstanceProperties *cpu_props = NULL;
+    PCMachineState *pcms = PC_MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+
+    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    if (!cpu_props->has_node_id) {
+        error_setg(&local_err, "node-id property is not specified");
+        goto out;
+    }
+
+    /*
+     * make sure that possible_cpus is initialized
+     * as property setter might be called before machine init is called
+     */
+    mc->possible_cpu_arch_ids(MACHINE(obj));
+
+    topo.pkg_id = cpu_props->socket_id;
+    topo.core_id = cpu_props->core_id;
+    topo.smt_id = cpu_props->thread_id;
+    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
+    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);
+    if (!cpu_slot) {
+        error_setg(&local_err, "unable to find CPU");
+        goto out;
+    }
+
+    if (cpu_slot->props.has_node_id) {
+        error_setg(&local_err, "CPU has already been assigned to node: %"PRId64,
+                   cpu_slot->props.node_id);
+        goto out;
+    }
+    cpu_slot->props.has_node_id = true;
+    cpu_slot->props.node_id = cpu_props->node_id;
+
+ out:
+    error_propagate(errp, local_err);
+    qapi_free_CpuInstanceProperties(cpu_props);
+}
+
 static void pc_machine_initfn(Object *obj)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -2395,6 +2445,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
 
     object_class_property_add_bool(oc, PC_MACHINE_PIT,
         pc_machine_get_pit, pc_machine_set_pit, &error_abort);
+
+    object_class_property_add(oc, "cpu", "CpuInstanceProperties",
+        NULL, pc_machine_set_cpu,
+        NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "cpu",
+        "Possible cpu placement", &error_abort);
 }
 
 static const TypeInfo pc_machine_info = {
-- 
2.7.4

[Qemu-devel] [RFC 09/13] numa: introduce '-numa cpu' cpu option

[Igor Mammedov]

It allows to specify mapping of a CPU to NUMA node on CLI.
Option should be repeated for each present/possible CPU.
Example for PC machine:
  -numa node,nodeid=0 -numa node,nodeid=1 \
  -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
  -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/numa.h |  2 +-
 numa.c                | 30 +++++++++++++++++++++++++++---
 qapi-schema.json      |  3 ++-
 vl.c                  |  2 +-
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index b8015a5..d4b139e 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -24,7 +24,7 @@ typedef struct node_info {
 } NodeInfo;
 
 extern NodeInfo numa_info[MAX_NODES];
-void parse_numa_opts(MachineClass *mc);
+void parse_numa_opts(MachineState *ms);
 void query_numa_node_mem(uint64_t node_mem[]);
 extern QemuOptsList qemu_numa_opts;
 void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
diff --git a/numa.c b/numa.c
index 5f68497..0164cd7 100644
--- a/numa.c
+++ b/numa.c
@@ -37,6 +37,8 @@
 #include "hw/mem/pc-dimm.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/qobject-input-visitor.h"
 
 QemuOptsList qemu_numa_opts = {
     .name = "numa",
@@ -213,11 +215,13 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
 
 static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
 {
+    Visitor *v;
     NumaOptions *object = NULL;
+    MachineState *ms = MACHINE(opaque);
     Error *err = NULL;
 
     {
-        Visitor *v = opts_visitor_new(opts);
+        v = opts_visitor_new(opts);
         visit_type_NumaOptions(v, NULL, &object, &err);
         visit_free(v);
     }
@@ -234,6 +238,25 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
         }
         nb_numa_nodes++;
         break;
+    case NUMA_OPTIONS_KIND_CPU: {
+        QObject *qobj;
+
+        v = qobject_output_visitor_new(&qobj);
+        visit_type_CpuInstanceProperties(v, "cpu", &object->u.cpu.data, &err);
+        visit_complete(v, &qobj);
+        visit_free(v);
+        if (err) {
+            goto end;
+        }
+        v = qobject_input_visitor_new(qobj, true);
+        object_property_set(OBJECT(ms), v, "cpu", &err);
+        visit_free(v);
+        qobject_decref(qobj);
+        if (err) {
+            goto end;
+        }
+        break;
+    }
     default:
         abort();
     }
@@ -293,15 +316,16 @@ static void validate_numa_cpus(void)
     g_free(seen_cpus);
 }
 
-void parse_numa_opts(MachineClass *mc)
+void parse_numa_opts(MachineState *ms)
 {
     int i;
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
 
     for (i = 0; i < MAX_NODES; i++) {
         numa_info[i].node_cpu = bitmap_new(max_cpus);
     }
 
-    if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, NULL, NULL)) {
+    if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
         exit(1);
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index ddc8783..69c059b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5543,7 +5543,8 @@
 ##
 { 'union': 'NumaOptions',
   'data': {
-    'node': 'NumaNodeOptions' }}
+    'node': 'NumaNodeOptions',
+    'cpu': 'CpuInstanceProperties' }}
 
 ##
 # @NumaNodeOptions:
diff --git a/vl.c b/vl.c
index afe40ce..d724523 100644
--- a/vl.c
+++ b/vl.c
@@ -4485,7 +4485,7 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
-    parse_numa_opts(machine_class);
+    parse_numa_opts(current_machine);
 
     if (qemu_opts_foreach(qemu_find_opts("mon"),
                           mon_init_func, NULL, NULL)) {
-- 
2.7.4

[Qemu-devel] [RFC 10/13] numa: replace cpu_index_to_socket_id() with cpu_index_to_instance_props()

[Igor Mammedov]

new cpu_index_to_instance_props() returns topo info which
includes socket_id so it could be used as drop in replacement
in the only user parse_numa_opts().

In follow up path cpu_index_to_instance_props() will be used in
compat code to set legacy index based cpu<->node mapping using
new machine.cpu property.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h | 11 ++++++-----
 hw/i386/pc.c        | 14 ++++++++------
 hw/ppc/spapr.c      | 13 ++++++++++---
 numa.c              |  7 +++++--
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 6364617..daceee5 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -70,10 +70,10 @@ typedef struct {
  *    of HotplugHandler object, which handles hotplug operation
  *    for a given @dev. It may return NULL if @dev doesn't require
  *    any actions to be performed by hotplug handler.
- * @cpu_index_to_socket_id:
- *    used to provide @cpu_index to socket number mapping, allowing
- *    a machine to group CPU threads belonging to the same socket/package
- *    Returns: socket number given cpu_index belongs to.
+ * @cpu_index_to_instance_props:
+ *    used to provide @cpu_index to socket/core/thread number mapping, allowing
+ *    legacy code to perform maping from cpu_index to topology properties
+ *    Returns: tuple of socket/core/thread ids given cpu_index belongs to.
  * @hw_version:
  *    Value of QEMU_VERSION when the machine was added to QEMU.
  *    Set only by old machines because they need to keep
@@ -135,7 +135,8 @@ struct MachineClass {
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
-    unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
+    CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
+                                                         unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
     HotpluggableCPUList *(*query_hotpluggable_cpus)(MachineState *machine);
 };
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1d33a5e..5066339 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2286,12 +2286,14 @@ static void pc_machine_reset(void)
     }
 }
 
-static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
+static CpuInstanceProperties
+pc_cpu_index_to_instance_props(MachineState *ms, unsigned cpu_index)
 {
-    X86CPUTopoInfo topo;
-    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
-                          &topo);
-    return topo.pkg_id;
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(ms);
+
+    assert(cpu_index < possible_cpus->len);
+    return possible_cpus->cpus[cpu_index].props;
 }
 
 static const CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
@@ -2398,7 +2400,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->acpi_data_size = 0x20000 + 0x8000;
     pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
-    mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
+    mc->cpu_index_to_instance_props = pc_cpu_index_to_instance_props;
     mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
     mc->query_hotpluggable_cpus = pc_query_hotpluggable_cpus;
     mc->default_boot_order = "cad";
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index efcd925..17eec87 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2571,11 +2571,18 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
-static unsigned spapr_cpu_index_to_socket_id(unsigned cpu_index)
+static CpuInstanceProperties
+spapr_cpu_index_to_instance_props(MachineState *ms, unsigned cpu_index)
 {
     /* Allocate to NUMA nodes on a "socket" basis (not that concept of
      * socket means much for the paravirtualized PAPR platform) */
-    return cpu_index / smp_threads / smp_cores;
+    CpuInstanceProperties topo = {
+        .socket_id = cpu_index / smp_threads / smp_cores,
+        .has_socket_id = true,
+        .has_core_id = false,
+        .has_thread_id = false,
+    };
+    return topo;
 }
 
 static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
@@ -2688,7 +2695,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     hc->pre_plug = spapr_machine_device_pre_plug;
     hc->plug = spapr_machine_device_plug;
     hc->unplug = spapr_machine_device_unplug;
-    mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
+    mc->cpu_index_to_instance_props = spapr_cpu_index_to_instance_props;
     hc->unplug_request = spapr_machine_device_unplug_request;
 
     smc->dr_lmb_enabled = true;
diff --git a/numa.c b/numa.c
index 0164cd7..887ee58 100644
--- a/numa.c
+++ b/numa.c
@@ -404,8 +404,11 @@ void parse_numa_opts(MachineState *ms)
         if (i == nb_numa_nodes) {
             for (i = 0; i < max_cpus; i++) {
                 unsigned node_id = i % nb_numa_nodes;
-                if (mc->cpu_index_to_socket_id) {
-                    node_id = mc->cpu_index_to_socket_id(i) % nb_numa_nodes;
+                if (mc->cpu_index_to_instance_props) {
+                    CpuInstanceProperties topo;
+                    topo = mc->cpu_index_to_instance_props(ms, i);
+                    assert(topo.has_socket_id);
+                    node_id = topo.socket_id % nb_numa_nodes;
                 }
 
                 set_bit(i, numa_info[node_id].node_cpu);
-- 
2.7.4

[Qemu-devel] [RFC 11/13] numa: use new machine.cpu property with -numa cpus=... CLI

[Igor Mammedov]

add compat layer to legacy cpu_index based '-numa cpus=...'
CLI option, which will use new machine.cpu property to set
numa mapping if that propety is available.

This way machine that supports machine.cpu property will
not break legacy CLI but will be able to move away from
using numa_info[node_id].node_cpu bitmaps and use only
possible_cpus instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 numa.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 15 deletions(-)

diff --git a/numa.c b/numa.c
index 887ee58..0c34130 100644
--- a/numa.c
+++ b/numa.c
@@ -141,10 +141,35 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp)
     return -1;
 }
 
-static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
+static void numa_cpu_set_nid(MachineState *ms, CpuInstanceProperties *props,
+                             Error **errp)
+{
+    Visitor *v;
+    QObject *qobj;
+    Error *local_error = NULL;
+
+    v = qobject_output_visitor_new(&qobj);
+    visit_type_CpuInstanceProperties(v, "cpu", &props, &local_error);
+    visit_complete(v, &qobj);
+    visit_free(v);
+    if (local_error) {
+        goto end;
+    }
+    v = qobject_input_visitor_new(qobj, true);
+    object_property_set(OBJECT(ms), v, "cpu", &local_error);
+    visit_free(v);
+    qobject_decref(qobj);
+end:
+    error_propagate(errp, local_error);
+}
+
+static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
+                            QemuOpts *opts, Error **errp)
 {
     uint16_t nodenr;
     uint16List *cpus = NULL;
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    bool has_cpu_prop = object_property_find(OBJECT(ms), "cpu", NULL);
 
     if (node->has_nodeid) {
         nodenr = node->nodeid;
@@ -172,6 +197,19 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
             return;
         }
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
+        if (mc->cpu_index_to_instance_props && has_cpu_prop) {
+            CpuInstanceProperties props;
+            Error *local_error = NULL;
+
+            props = mc->cpu_index_to_instance_props(ms, cpus->value);
+            props.has_node_id = true;
+            props.node_id = nodenr;
+            numa_cpu_set_nid(ms, &props, &local_error);
+            if (local_error) {
+                error_propagate(errp, local_error);
+                return;
+            }
+        }
     }
 
     if (node->has_mem && node->has_memdev) {
@@ -232,26 +270,14 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
 
     switch (object->type) {
     case NUMA_OPTIONS_KIND_NODE:
-        numa_node_parse(object->u.node.data, opts, &err);
+        numa_node_parse(ms, object->u.node.data, opts, &err);
         if (err) {
             goto end;
         }
         nb_numa_nodes++;
         break;
     case NUMA_OPTIONS_KIND_CPU: {
-        QObject *qobj;
-
-        v = qobject_output_visitor_new(&qobj);
-        visit_type_CpuInstanceProperties(v, "cpu", &object->u.cpu.data, &err);
-        visit_complete(v, &qobj);
-        visit_free(v);
-        if (err) {
-            goto end;
-        }
-        v = qobject_input_visitor_new(qobj, true);
-        object_property_set(OBJECT(ms), v, "cpu", &err);
-        visit_free(v);
-        qobject_decref(qobj);
+        numa_cpu_set_nid(ms, object->u.cpu.data, &err);
         if (err) {
             goto end;
         }
@@ -402,6 +428,8 @@ void parse_numa_opts(MachineState *ms)
          * would be on the same node.
          */
         if (i == nb_numa_nodes) {
+            bool has_cpu_prop = object_property_find(OBJECT(ms), "cpu", NULL);
+
             for (i = 0; i < max_cpus; i++) {
                 unsigned node_id = i % nb_numa_nodes;
                 if (mc->cpu_index_to_instance_props) {
@@ -409,6 +437,11 @@ void parse_numa_opts(MachineState *ms)
                     topo = mc->cpu_index_to_instance_props(ms, i);
                     assert(topo.has_socket_id);
                     node_id = topo.socket_id % nb_numa_nodes;
+                    if (has_cpu_prop) {
+                        topo.has_node_id = true;
+                        topo.node_id = node_id;
+                        numa_cpu_set_nid(ms, &topo, &error_fatal);
+                    }
                 }
 
                 set_bit(i, numa_info[node_id].node_cpu);
-- 
2.7.4

[Qemu-devel] [RFC 12/13] pc: drop usage of legacy numa_get_node_for_cpu()

[Igor Mammedov]

Replace usage of legacy cpu_index based node_cpu bitmaps
with PCMachineState.possible_cpus which have node-ids for
all possible CPUs.

As result:
 * -1 usage of global max_cpus
 * considering not set CPUArchId.props.node_id is 0
   drop not needed (numa_get_node_for_cpu(i) < nb_numa_nodes) checks,
   for the case where not all CPUs have been mapped to a numa,
   as it will have the same effect
 * -5 usage of global nb_numa_nodes

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/cpu.c        |  7 +++----
 hw/i386/acpi-build.c | 17 ++++++-----------
 hw/i386/pc.c         | 19 ++++++-------------
 3 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 6017ca0..ac63cb9 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -503,7 +503,6 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
 
         /* build Processor object for each processor */
         for (i = 0; i < arch_ids->len; i++) {
-            int j;
             Aml *dev;
             Aml *uid = aml_int(i);
             GArray *madt_buf = g_array_new(0, 1, 1);
@@ -557,9 +556,9 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
              * as a result _PXM is required for all CPUs which might
              * be hot-plugged. For simplicity, add it for all CPUs.
              */
-            j = numa_get_node_for_cpu(i);
-            if (j < nb_numa_nodes) {
-                aml_append(dev, aml_name_decl("_PXM", aml_int(j)));
+            if (arch_ids->cpus[i].props.has_node_id) {
+                aml_append(dev, aml_name_decl("_PXM",
+                    aml_int(arch_ids->cpus[i].props.node_id)));
             }
 
             aml_append(cpus_dev, dev);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0c91043..d0dc8d9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2305,19 +2305,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
     srat->reserved1 = cpu_to_le32(1);
 
     for (i = 0; i < apic_ids->len; i++) {
-        int j = numa_get_node_for_cpu(i);
-        uint32_t apic_id = apic_ids->cpus[i].arch_id;
+        const CPUArchId *cpu = &apic_ids->cpus[i];
 
-        if (apic_id < 255) {
+        if (cpu->arch_id < 255) {
             AcpiSratProcessorAffinity *core;
 
             core = acpi_data_push(table_data, sizeof *core);
             core->type = ACPI_SRAT_PROCESSOR_APIC;
             core->length = sizeof(*core);
-            core->local_apic_id = apic_id;
-            if (j < nb_numa_nodes) {
-                core->proximity_lo = j;
-            }
+            core->local_apic_id = cpu->arch_id;
+            core->proximity_lo = cpu->props.node_id;
             memset(core->proximity_hi, 0, 3);
             core->local_sapic_eid = 0;
             core->flags = cpu_to_le32(1);
@@ -2327,10 +2324,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
             core = acpi_data_push(table_data, sizeof *core);
             core->type = ACPI_SRAT_PROCESSOR_x2APIC;
             core->length = sizeof(*core);
-            core->x2apic_id = cpu_to_le32(apic_id);
-            if (j < nb_numa_nodes) {
-                core->proximity_domain = cpu_to_le32(j);
-            }
+            core->x2apic_id = cpu_to_le32(cpu->arch_id);
+            core->proximity_domain = cpu_to_le32(cpu->props.node_id);
             core->flags = cpu_to_le32(1);
         }
     }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5066339..e2833e0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -746,7 +746,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
-    int i, j;
+    int i;
 
     fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as);
     fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
@@ -781,13 +781,10 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
      */
     numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
-    for (i = 0; i < max_cpus; i++) {
-        unsigned int apic_id = x86_cpu_apic_id_from_index(i);
-        assert(apic_id < pcms->apic_id_limit);
-        j = numa_get_node_for_cpu(i);
-        if (j < nb_numa_nodes) {
-            numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
-        }
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        const CPUArchId *cpu = &pcms->possible_cpus->cpus[i];
+        assert(cpu->arch_id < pcms->apic_id_limit);
+        numa_fw_cfg[cpu->arch_id + 1] = cpu_to_le64(cpu->props.node_id);
     }
     for (i = 0; i < nb_numa_nodes; i++) {
         numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
@@ -1970,11 +1967,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 
     cs = CPU(cpu);
     cs->cpu_index = idx;
-
-    idx = numa_get_node_for_cpu(cs->cpu_index);
-    if (idx < nb_numa_nodes) {
-        cpu->numa_nid = idx;
-    }
+    cpu->numa_nid = cpu_slot->props.node_id;
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
-- 
2.7.4

[Qemu-devel] [RFC 13/13] pc: cpu: make sure that cpu.node-id matches -numa mapping

[Igor Mammedov]

for PC CPU's node mapping defined statically at startup
and not possible to changle at hotplug time.
So check that user set cpu.node-id matches whatever values
were configured with -numa options (ex|im)plicitly.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      | 11 ++++++++++-
 target/i386/cpu.c |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e2833e0..d828a1a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1967,7 +1967,16 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 
     cs = CPU(cpu);
     cs->cpu_index = idx;
-    cpu->numa_nid = cpu_slot->props.node_id;
+
+    if (cpu->numa_nid == -1) {
+        cpu->numa_nid = cpu_slot->props.node_id;
+    } else if (cpu->numa_nid != cpu_slot->props.node_id) {
+        error_setg(errp, "property node-id: %u doesn't match with -numa"
+            " explictly specified value(s) or implicitly set defaults."
+            " Expected value: %" PRIi64,
+            cpu->numa_nid, cpu_slot->props.node_id);
+        return;
+    }
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 85c52f1..6375a37 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3649,7 +3649,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
     DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
 #endif
-    DEFINE_PROP_INT32("node-id", X86CPU, numa_nid, 0),
+    DEFINE_PROP_INT32("node-id", X86CPU, numa_nid, -1),
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
     DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
-- 
2.7.4

Re: [Qemu-devel] [RFC 01/13] numa: access CPU's node id via property in hmp_info_numa()

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:17PM +0100, Igor Mammedov wrote:
> Move vcpu's assocciated numa_node field out of generic CPUState
> into inherited classes that actually care about cpu<->numa mapping
> and make monitor 'info numa' get vcpu's assocciated node id via
> node-id property.
> It allows to drop implicit node id intialization in
> numa_post_machine_init() and would allow switching to mapping
> defined by target's CpuInstanceProperties instead of global
> numa_info[i].node_cpu bitmaps.
> 
> As side effect it fixes 'info numa' displaying wrong mapping
> for CPUs specified with -device/device_add.
> Before patch following CLI would produce:
> QEMU -smp 1,sockets=3,maxcpus=3 \
>        -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
>        -numa node,nodeid=0,cpus=0 \
>        -numa node,nodeid=1,cpus=1 \
>        -numa node,nodeid=2,cpus=2
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0 1 2
> node 0 size: 40 MB
> node 1 cpus:
> node 1 size: 40 MB
> node 2 cpus:
> node 2 size: 48 MB
> 
> after patch all CPUs are on nodes they are supposed to be:
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0
> node 0 size: 40 MB
> node 1 cpus: 1
> node 1 size: 40 MB
> node 2 cpus: 2
> node 2 size: 48 MB
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

So, in addition to the other comments I had, it looks like this
patch is doing 3 things at the same time:

1) Adding a "node-id" property to CPU objects.
2) Moving CPUState::numa_node to arch-specific CPU structs.
3) Fixing the bug where the NUMA node info isn't initialized
   for PC on CPUs created by -device/device_add.

All of them are independent from each other. For example, all you
need to fix the bug you describe above is (3), which is contained
in a single hunk from this patch, that is:

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f721fde..9d2b265 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
> +
> +    idx = numa_get_node_for_cpu(cs->cpu_index);
> +    if (idx < nb_numa_nodes) {
> +        cpu->numa_nid = idx;
> +    }
>  }

All the rest seems irrelevant to fix the bug. (1) and (2) may be
useful, but they need separate justification.

-- 
Eduardo

Re: [Qemu-devel] [RFC 09/13] numa: introduce '-numa cpu' cpu option

[Eric Blake]

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

On 01/18/2017 11:13 AM, Igor Mammedov wrote:
> It allows to specify mapping of a CPU to NUMA node on CLI.
> Option should be repeated for each present/possible CPU.
> Example for PC machine:
>   -numa node,nodeid=0 -numa node,nodeid=1 \
>   -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
>   -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -5543,7 +5543,8 @@
>  ##
>  { 'union': 'NumaOptions',
>    'data': {
> -    'node': 'NumaNodeOptions' }}
> +    'node': 'NumaNodeOptions',
> +    'cpu': 'CpuInstanceProperties' }}

Missing a documentation blurb that includes a '(since 2.9)' tag.


-- 
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] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> it will allow generic numa code to set cpu to numa node mapping
> in target independent manner in the next patch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This looks like a creative way to abuse the QOM property system.
What's the problem with using a simple C function like:
  void (*set_cpu_affinity)(MachineState *m, CpuInstanceProperties *props, Error **errp)
?


> ---
>  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8ea635..1d33a5e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
>      pcms->pit = value;
>  }
>  
> +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    uint32_t apic_id;
> +    X86CPUTopoInfo topo;
> +    CPUArchId *cpu_slot;
> +    Error *local_err = NULL;
> +    CpuInstanceProperties *cpu_props = NULL;
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (!cpu_props->has_node_id) {
> +        error_setg(&local_err, "node-id property is not specified");
> +        goto out;
> +    }
> +
> +    /*
> +     * make sure that possible_cpus is initialized
> +     * as property setter might be called before machine init is called
> +     */
> +    mc->possible_cpu_arch_ids(MACHINE(obj));
> +
> +    topo.pkg_id = cpu_props->socket_id;
> +    topo.core_id = cpu_props->core_id;
> +    topo.smt_id = cpu_props->thread_id;
> +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);
> +    if (!cpu_slot) {
> +        error_setg(&local_err, "unable to find CPU");
> +        goto out;
> +    }
> +
> +    if (cpu_slot->props.has_node_id) {
> +        error_setg(&local_err, "CPU has already been assigned to node: %"PRId64,
> +                   cpu_slot->props.node_id);
> +        goto out;
> +    }
> +    cpu_slot->props.has_node_id = true;
> +    cpu_slot->props.node_id = cpu_props->node_id;
> +
> + out:
> +    error_propagate(errp, local_err);
> +    qapi_free_CpuInstanceProperties(cpu_props);
> +}
> +
>  static void pc_machine_initfn(Object *obj)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2395,6 +2445,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>  
>      object_class_property_add_bool(oc, PC_MACHINE_PIT,
>          pc_machine_get_pit, pc_machine_set_pit, &error_abort);
> +
> +    object_class_property_add(oc, "cpu", "CpuInstanceProperties",
> +        NULL, pc_machine_set_cpu,
> +        NULL, NULL, &error_abort);
> +    object_class_property_set_description(oc, "cpu",
> +        "Possible cpu placement", &error_abort);
>  }
>  
>  static const TypeInfo pc_machine_info = {
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 11/13] numa: use new machine.cpu property with -numa cpus=... CLI

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:27PM +0100, Igor Mammedov wrote:
> add compat layer to legacy cpu_index based '-numa cpus=...'
> CLI option, which will use new machine.cpu property to set
> numa mapping if that propety is available.
> 
> This way machine that supports machine.cpu property will
> not break legacy CLI but will be able to move away from
> using numa_info[node_id].node_cpu bitmaps and use only
> possible_cpus instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  numa.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index 887ee58..0c34130 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -141,10 +141,35 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp)
>      return -1;
>  }
>  
> -static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
> +static void numa_cpu_set_nid(MachineState *ms, CpuInstanceProperties *props,
> +                             Error **errp)
> +{
> +    Visitor *v;
> +    QObject *qobj;
> +    Error *local_error = NULL;
> +
> +    v = qobject_output_visitor_new(&qobj);
> +    visit_type_CpuInstanceProperties(v, "cpu", &props, &local_error);
> +    visit_complete(v, &qobj);
> +    visit_free(v);
> +    if (local_error) {
> +        goto end;
> +    }
> +    v = qobject_input_visitor_new(qobj, true);
> +    object_property_set(OBJECT(ms), v, "cpu", &local_error);
> +    visit_free(v);
> +    qobject_decref(qobj);
> +end:
> +    error_propagate(errp, local_error);
> +}
> +
> +static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> +                            QemuOpts *opts, Error **errp)
>  {
>      uint16_t nodenr;
>      uint16List *cpus = NULL;
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    bool has_cpu_prop = object_property_find(OBJECT(ms), "cpu", NULL);
>  
>      if (node->has_nodeid) {
>          nodenr = node->nodeid;
> @@ -172,6 +197,19 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> +        if (mc->cpu_index_to_instance_props && has_cpu_prop) {
> +            CpuInstanceProperties props;
> +            Error *local_error = NULL;
> +
> +            props = mc->cpu_index_to_instance_props(ms, cpus->value);
> +            props.has_node_id = true;
> +            props.node_id = nodenr;
> +            numa_cpu_set_nid(ms, &props, &local_error);
> +            if (local_error) {
> +                error_propagate(errp, local_error);
> +                return;
> +            }
> +        }
>      }

With this, now we have two completely different ways to store the
results of "-numa node,cpus...", and both are likely to stay
forever if we take too long to update the other machines.

Do you plan to make ppc/spapr and arm/virt use the new
machine.cpu system so we could remove numa_info[].node_cpu
completely in the same series?

-- 
Eduardo

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> it will allow generic numa code to set cpu to numa node mapping
> in target independent manner in the next patch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8ea635..1d33a5e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
>      pcms->pit = value;
>  }
>  
> +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    uint32_t apic_id;
> +    X86CPUTopoInfo topo;
> +    CPUArchId *cpu_slot;
> +    Error *local_err = NULL;
> +    CpuInstanceProperties *cpu_props = NULL;
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +
> +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (!cpu_props->has_node_id) {
> +        error_setg(&local_err, "node-id property is not specified");
> +        goto out;
> +    }
> +
> +    /*
> +     * make sure that possible_cpus is initialized
> +     * as property setter might be called before machine init is called
> +     */
> +    mc->possible_cpu_arch_ids(MACHINE(obj));
> +
> +    topo.pkg_id = cpu_props->socket_id;
> +    topo.core_id = cpu_props->core_id;
> +    topo.smt_id = cpu_props->thread_id;
> +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);

If we make TYPE_MACHINE provide an API to query CPU slots, e.g.:
  CPUArchId *machine_find_cpu_slot(MachineState *m, CpuInstanceProperties *props)
  CPUArchId *machine_slot_for_cpu(MachineState *m, CPUState *cpu)

(Which can probably be implemented using
MachineClass::possible_cpu_arch_ids(), already)

Then this function could be implemented in a generic way, and all
existing calls of:
  numa_get_node_for_cpu(cpu->cpu_index)
could be easily replaced by:
  machine_slot_for_cpu(cpu)->props.node_id

This should make it easier to get rid numa_info[].node_cpu.

> +    if (!cpu_slot) {
> +        error_setg(&local_err, "unable to find CPU");
> +        goto out;
> +    }
> +
> +    if (cpu_slot->props.has_node_id) {
> +        error_setg(&local_err, "CPU has already been assigned to node: %"PRId64,
> +                   cpu_slot->props.node_id);
> +        goto out;
> +    }
> +    cpu_slot->props.has_node_id = true;
> +    cpu_slot->props.node_id = cpu_props->node_id;
> +
> + out:
> +    error_propagate(errp, local_err);
> +    qapi_free_CpuInstanceProperties(cpu_props);
> +}
> +
>  static void pc_machine_initfn(Object *obj)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2395,6 +2445,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>  
>      object_class_property_add_bool(oc, PC_MACHINE_PIT,
>          pc_machine_get_pit, pc_machine_set_pit, &error_abort);
> +
> +    object_class_property_add(oc, "cpu", "CpuInstanceProperties",
> +        NULL, pc_machine_set_cpu,
> +        NULL, NULL, &error_abort);
> +    object_class_property_set_description(oc, "cpu",
> +        "Possible cpu placement", &error_abort);
>  }
>  
>  static const TypeInfo pc_machine_info = {
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Daniel P. Berrange]

On Wed, Jan 18, 2017 at 06:13:16PM +0100, Igor Mammedov wrote:
> 
> Series introduces a new CLI option to allow mapping cpus to numa
> nodes using public properties [socket|core|thread]-ids instead of
> internal cpu-index and moving cpu<->node mapping from global bitmaps
> to PCMachineState struct.

What is the benefit of this change to apps ? Obviously libvirt uses
the current syntax, but I'm not aware of what problems that has - why
would libvirt want to use this new syntax instead ?


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Eduardo Habkost]

On Thu, Jan 19, 2017 at 09:45:11AM +0000, Daniel P. Berrange wrote:
> On Wed, Jan 18, 2017 at 06:13:16PM +0100, Igor Mammedov wrote:
> > 
> > Series introduces a new CLI option to allow mapping cpus to numa
> > nodes using public properties [socket|core|thread]-ids instead of
> > internal cpu-index and moving cpu<->node mapping from global bitmaps
> > to PCMachineState struct.
> 
> What is the benefit of this change to apps ? Obviously libvirt uses
> the current syntax, but I'm not aware of what problems that has - why
> would libvirt want to use this new syntax instead ?

The current interface is based on a "CPU index", and the mapping
from CPU index to a specific CPU object can be non-trivial,
especially if "-device *-cpu", CPU hotplug, or more complex CPU
topologies are used.

-- 
Eduardo

Re: [Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Igor Mammedov]

On Thu, 19 Jan 2017 09:45:11 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 06:13:16PM +0100, Igor Mammedov wrote:
> > 
> > Series introduces a new CLI option to allow mapping cpus to numa
> > nodes using public properties [socket|core|thread]-ids instead of
> > internal cpu-index and moving cpu<->node mapping from global bitmaps
> > to PCMachineState struct.  
> 
> What is the benefit of this change to apps ? Obviously libvirt uses
> the current syntax, but I'm not aware of what problems that has - why
> would libvirt want to use this new syntax instead ?
current syntax -numa cpus=1,2,3... depends on cpu-index which is
internal to QEMU. External users wouldn't actually know which cpu
is associated with which cpu-index without re-implementing cpu-index
assignment which is qemu-version/target/machine/topology dependent.

New '-numa cpu' provides mapping of cpus to numa nodes in
CPU terms that are used with device_add/-device commands.
For management there is query-hotpluggble-cpus command that allows
to get a list of possible cpus with their socket-id/core-id/thread-id
property values.
for example without numa mapping CLI could look like:
  $QEMU -M pc -smp 1,sockets=3,maxcpus=3 \
      -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0

(qemu) info hotpluggable-cpus 
Hotpluggable CPUs:
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  CPUInstance Properties:
    socket-id: "2"
    core-id: "0"
    thread-id: "0"
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  qom_path: "/machine/peripheral-anon/device[0]"
  CPUInstance Properties:
    socket-id: "1"
    core-id: "0"
    thread-id: "0"
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  qom_path: "/machine/unattached/device[0]"
  CPUInstance Properties:
    socket-id: "0"
    core-id: "0"
    thread-id: "0"


based on that list the one could extend CLI with numa mapping:

  $QEMU -M pc -smp 1,sockets=3,maxcpus=3 \
      -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
      -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
      -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1 \
      -numa cpu,socket-id=2,core-id=0,thread-id=0,node-id=2 

(qemu) info hotpluggable-cpus 
Hotpluggable CPUs:
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  CPUInstance Properties:
    node-id: "2"
    socket-id: "2"
    core-id: "0"
    thread-id: "0"
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  qom_path: "/machine/peripheral-anon/device[0]"
  CPUInstance Properties:
    node-id: "1"
    socket-id: "1"
    core-id: "0"
    thread-id: "0"
  type: "qemu64-x86_64-cpu"
  vcpus_count: "1"
  qom_path: "/machine/unattached/device[0]"
  CPUInstance Properties:
    node-id: "0"
    socket-id: "0"
    core-id: "0"
    thread-id: "0"

As it's been discussed previous times, there is chicken/egg
issue where management has to get query-hotpluggble-cpus result
for a specific combination of machine type and -smp option.
It would need to be done at most one time when creating
a configuration and then CLI numa mapping could be used
for starting VM without doing query-hotpluggable-cpus.
'-numa cpu' CLI option is also needed for setting known
mapping on target side of migration.

Previous consensus has been that the only way to avoid 2 stage
discovery/configuration is starting QEMU in paused mode and
than do query + mapping at runtime via QMP before allowing
guest run with 'continue' command.
But this part hasn't been implemented in this series yet.

> 
> 
> Regards,
> Daniel

Re: [Qemu-devel] [RFC 11/13] numa: use new machine.cpu property with -numa cpus=... CLI

[Igor Mammedov]

On Wed, 18 Jan 2017 16:46:37 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 06:13:27PM +0100, Igor Mammedov wrote:
> > add compat layer to legacy cpu_index based '-numa cpus=...'
> > CLI option, which will use new machine.cpu property to set
> > numa mapping if that propety is available.
> > 
> > This way machine that supports machine.cpu property will
> > not break legacy CLI but will be able to move away from
> > using numa_info[node_id].node_cpu bitmaps and use only
> > possible_cpus instead.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  numa.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 48 insertions(+), 15 deletions(-)
> > 
> > diff --git a/numa.c b/numa.c
> > index 887ee58..0c34130 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -141,10 +141,35 @@ uint32_t numa_get_node(ram_addr_t addr, Error **errp)
> >      return -1;
> >  }
> >  
> > -static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
> > +static void numa_cpu_set_nid(MachineState *ms, CpuInstanceProperties *props,
> > +                             Error **errp)
> > +{
> > +    Visitor *v;
> > +    QObject *qobj;
> > +    Error *local_error = NULL;
> > +
> > +    v = qobject_output_visitor_new(&qobj);
> > +    visit_type_CpuInstanceProperties(v, "cpu", &props, &local_error);
> > +    visit_complete(v, &qobj);
> > +    visit_free(v);
> > +    if (local_error) {
> > +        goto end;
> > +    }
> > +    v = qobject_input_visitor_new(qobj, true);
> > +    object_property_set(OBJECT(ms), v, "cpu", &local_error);
> > +    visit_free(v);
> > +    qobject_decref(qobj);
> > +end:
> > +    error_propagate(errp, local_error);
> > +}
> > +
> > +static void numa_node_parse(MachineState *ms, NumaNodeOptions *node,
> > +                            QemuOpts *opts, Error **errp)
> >  {
> >      uint16_t nodenr;
> >      uint16List *cpus = NULL;
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +    bool has_cpu_prop = object_property_find(OBJECT(ms), "cpu", NULL);
> >  
> >      if (node->has_nodeid) {
> >          nodenr = node->nodeid;
> > @@ -172,6 +197,19 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
> >              return;
> >          }
> >          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
> > +        if (mc->cpu_index_to_instance_props && has_cpu_prop) {
> > +            CpuInstanceProperties props;
> > +            Error *local_error = NULL;
> > +
> > +            props = mc->cpu_index_to_instance_props(ms, cpus->value);
> > +            props.has_node_id = true;
> > +            props.node_id = nodenr;
> > +            numa_cpu_set_nid(ms, &props, &local_error);
> > +            if (local_error) {
> > +                error_propagate(errp, local_error);
> > +                return;
> > +            }
> > +        }
> >      }  
> 
> With this, now we have two completely different ways to store the
> results of "-numa node,cpus...", and both are likely to stay
> forever if we take too long to update the other machines.
> 
> Do you plan to make ppc/spapr and arm/virt use the new
> machine.cpu system so we could remove numa_info[].node_cpu
> completely in the same series?
this RFC is just to demonstrate an approach.

it would be better to do conversion within the same merge
window (however note ARM is not even close to -device based cpus)
so I would prefer do it incrementally per target
(under promise of not abandoning project midways)
above said doing conversion for x86 and spapr within one merge
windows seems feasible.

Re: [Qemu-devel] [RFC 01/13] numa: access CPU's node id via property in hmp_info_numa()

[Igor Mammedov]

On Wed, 18 Jan 2017 16:18:20 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 06:13:17PM +0100, Igor Mammedov wrote:
> > Move vcpu's assocciated numa_node field out of generic CPUState
> > into inherited classes that actually care about cpu<->numa mapping
> > and make monitor 'info numa' get vcpu's assocciated node id via
> > node-id property.
> > It allows to drop implicit node id intialization in
> > numa_post_machine_init() and would allow switching to mapping
> > defined by target's CpuInstanceProperties instead of global
> > numa_info[i].node_cpu bitmaps.
> > 
> > As side effect it fixes 'info numa' displaying wrong mapping
> > for CPUs specified with -device/device_add.
> > Before patch following CLI would produce:
> > QEMU -smp 1,sockets=3,maxcpus=3 \
> >        -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
> >        -numa node,nodeid=0,cpus=0 \
> >        -numa node,nodeid=1,cpus=1 \
> >        -numa node,nodeid=2,cpus=2
> > (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> > (qemu) info numa
> > 3 nodes
> > node 0 cpus: 0 1 2
> > node 0 size: 40 MB
> > node 1 cpus:
> > node 1 size: 40 MB
> > node 2 cpus:
> > node 2 size: 48 MB
> > 
> > after patch all CPUs are on nodes they are supposed to be:
> > (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> > (qemu) info numa
> > 3 nodes
> > node 0 cpus: 0
> > node 0 size: 40 MB
> > node 1 cpus: 1
> > node 1 size: 40 MB
> > node 2 cpus: 2
> > node 2 size: 48 MB
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> So, in addition to the other comments I had, it looks like this
> patch is doing 3 things at the same time:
> 
> 1) Adding a "node-id" property to CPU objects.
> 2) Moving CPUState::numa_node to arch-specific CPU structs.
> 3) Fixing the bug where the NUMA node info isn't initialized
>    for PC on CPUs created by -device/device_add.
ok, I'll split this in 3 patches

> 
> All of them are independent from each other. For example, all you
> need to fix the bug you describe above is (3), which is contained
> in a single hunk from this patch, that is:
> 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f721fde..9d2b265 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >  
> >      cs = CPU(cpu);
> >      cs->cpu_index = idx;
> > +
> > +    idx = numa_get_node_for_cpu(cs->cpu_index);
> > +    if (idx < nb_numa_nodes) {
> > +        cpu->numa_nid = idx;
> > +    }
> >  }  
> 
> All the rest seems irrelevant to fix the bug. (1) and (2) may be
> useful, but they need separate justification.
rationale for (1) and (2) is the first 2 paragraphs of this commit message

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Igor Mammedov]

On Wed, 18 Jan 2017 16:27:21 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> > it will allow generic numa code to set cpu to numa node mapping
> > in target independent manner in the next patch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> This looks like a creative way to abuse the QOM property system.
> What's the problem with using a simple C function like:
>   void (*set_cpu_affinity)(MachineState *m, CpuInstanceProperties *props, Error **errp)
Agreed, it should simplify parsing code as well.

> ?
> 
> 
> > ---
> >  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8ea635..1d33a5e 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
> >      pcms->pit = value;
> >  }
> >  
> > +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> > +                               void *opaque, Error **errp)
> > +{
> > +    uint32_t apic_id;
> > +    X86CPUTopoInfo topo;
> > +    CPUArchId *cpu_slot;
> > +    Error *local_err = NULL;
> > +    CpuInstanceProperties *cpu_props = NULL;
> > +    PCMachineState *pcms = PC_MACHINE(obj);
> > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> > +
> > +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (!cpu_props->has_node_id) {
> > +        error_setg(&local_err, "node-id property is not specified");
> > +        goto out;
> > +    }
> > +
> > +    /*
> > +     * make sure that possible_cpus is initialized
> > +     * as property setter might be called before machine init is called
> > +     */
> > +    mc->possible_cpu_arch_ids(MACHINE(obj));
> > +
> > +    topo.pkg_id = cpu_props->socket_id;
> > +    topo.core_id = cpu_props->core_id;
> > +    topo.smt_id = cpu_props->thread_id;
> > +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> > +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);
> > +    if (!cpu_slot) {
> > +        error_setg(&local_err, "unable to find CPU");
> > +        goto out;
> > +    }
> > +
> > +    if (cpu_slot->props.has_node_id) {
> > +        error_setg(&local_err, "CPU has already been assigned to node: %"PRId64,
> > +                   cpu_slot->props.node_id);
> > +        goto out;
> > +    }
> > +    cpu_slot->props.has_node_id = true;
> > +    cpu_slot->props.node_id = cpu_props->node_id;
> > +
> > + out:
> > +    error_propagate(errp, local_err);
> > +    qapi_free_CpuInstanceProperties(cpu_props);
> > +}
> > +
> >  static void pc_machine_initfn(Object *obj)
> >  {
> >      PCMachineState *pcms = PC_MACHINE(obj);
> > @@ -2395,6 +2445,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >  
> >      object_class_property_add_bool(oc, PC_MACHINE_PIT,
> >          pc_machine_get_pit, pc_machine_set_pit, &error_abort);
> > +
> > +    object_class_property_add(oc, "cpu", "CpuInstanceProperties",
> > +        NULL, pc_machine_set_cpu,
> > +        NULL, NULL, &error_abort);
> > +    object_class_property_set_description(oc, "cpu",
> > +        "Possible cpu placement", &error_abort);
> >  }
> >  
> >  static const TypeInfo pc_machine_info = {
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Igor Mammedov]

On Wed, 18 Jan 2017 16:57:13 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> > it will allow generic numa code to set cpu to numa node mapping
> > in target independent manner in the next patch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8ea635..1d33a5e 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
> >      pcms->pit = value;
> >  }
> >  
> > +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> > +                               void *opaque, Error **errp)
> > +{
> > +    uint32_t apic_id;
> > +    X86CPUTopoInfo topo;
> > +    CPUArchId *cpu_slot;
> > +    Error *local_err = NULL;
> > +    CpuInstanceProperties *cpu_props = NULL;
> > +    PCMachineState *pcms = PC_MACHINE(obj);
> > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> > +
> > +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (!cpu_props->has_node_id) {
> > +        error_setg(&local_err, "node-id property is not specified");
> > +        goto out;
> > +    }
> > +
> > +    /*
> > +     * make sure that possible_cpus is initialized
> > +     * as property setter might be called before machine init is called
> > +     */
> > +    mc->possible_cpu_arch_ids(MACHINE(obj));
> > +
> > +    topo.pkg_id = cpu_props->socket_id;
> > +    topo.core_id = cpu_props->core_id;
> > +    topo.smt_id = cpu_props->thread_id;
> > +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> > +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);  
> 
> If we make TYPE_MACHINE provide an API to query CPU slots, e.g.:
>   CPUArchId *machine_find_cpu_slot(MachineState *m, CpuInstanceProperties *props)
so if there is no objections, 
I'll move possible_cpus to MachineState
and add to MachineClass above callback so target machine
would be able to provide arch specific lookup function.
it should work for both x86 and ARM.



>   CPUArchId *machine_slot_for_cpu(MachineState *m, CPUState *cpu)
probably won't work for SPAPR where they have cores,
machine_find_cpu_slot() alone might be sufficient.


> (Which can probably be implemented using
> MachineClass::possible_cpu_arch_ids(), already)
> 
> Then this function could be implemented in a generic way, and all
> existing calls of:
>   numa_get_node_for_cpu(cpu->cpu_index)
> could be easily replaced by:
>   machine_slot_for_cpu(cpu)->props.node_id
most of such places could be replaced directly by plain
cpu->node_id

> 
> This should make it easier to get rid numa_info[].node_cpu.

PS:
Adding Bharata to CC so SPAPR side could voice their opinion.


> > +    if (!cpu_slot) {
> > +        error_setg(&local_err, "unable to find CPU");
> > +        goto out;
> > +    }
> > +
> > +    if (cpu_slot->props.has_node_id) {
> > +        error_setg(&local_err, "CPU has already been assigned to node: %"PRId64,
> > +                   cpu_slot->props.node_id);
> > +        goto out;
> > +    }
> > +    cpu_slot->props.has_node_id = true;
> > +    cpu_slot->props.node_id = cpu_props->node_id;
> > +
> > + out:
> > +    error_propagate(errp, local_err);
> > +    qapi_free_CpuInstanceProperties(cpu_props);
> > +}
> > +
> >  static void pc_machine_initfn(Object *obj)
> >  {
> >      PCMachineState *pcms = PC_MACHINE(obj);
> > @@ -2395,6 +2445,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >  
> >      object_class_property_add_bool(oc, PC_MACHINE_PIT,
> >          pc_machine_get_pit, pc_machine_set_pit, &error_abort);
> > +
> > +    object_class_property_add(oc, "cpu", "CpuInstanceProperties",
> > +        NULL, pc_machine_set_cpu,
> > +        NULL, NULL, &error_abort);
> > +    object_class_property_set_description(oc, "cpu",
> > +        "Possible cpu placement", &error_abort);
> >  }
> >  
> >  static const TypeInfo pc_machine_info = {
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [RFC 09/13] numa: introduce '-numa cpu' cpu option

[Igor Mammedov]

On Wed, 18 Jan 2017 12:22:01 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 01/18/2017 11:13 AM, Igor Mammedov wrote:
> > It allows to specify mapping of a CPU to NUMA node on CLI.
> > Option should be repeated for each present/possible CPU.
> > Example for PC machine:
> >   -numa node,nodeid=0 -numa node,nodeid=1 \
> >   -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
> >   -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---  
> 
> > +++ b/qapi-schema.json
> > @@ -5543,7 +5543,8 @@
> >  ##
> >  { 'union': 'NumaOptions',
> >    'data': {
> > -    'node': 'NumaNodeOptions' }}
> > +    'node': 'NumaNodeOptions',
> > +    'cpu': 'CpuInstanceProperties' }}  
> 
> Missing a documentation blurb that includes a '(since 2.9)' tag.
> 
I'll fix it in the next version

Re: [Qemu-devel] [RFC 05/13] pc: move pcms->possible_cpus init out of pc_cpus_init()

[Dou Liyang]

At 01/19/2017 01:13 AM, Igor Mammedov wrote:
> possible_cpus could be initialized earlier then cpu objects,

s/then/than/

> i.e. when -smp is parsed so move init code to possible_cpu_arch_ids()

[...]

Re: [Qemu-devel] [RFC 09/13] numa: introduce '-numa cpu' cpu option

[Paolo Bonzini]

On 18/01/2017 18:13, Igor Mammedov wrote:
> It allows to specify mapping of a CPU to NUMA node on CLI.
> Option should be repeated for each present/possible CPU.
> Example for PC machine:
>   -numa node,nodeid=0 -numa node,nodeid=1 \
>   -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
>   -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

What about making core-id/thread-id optional, like

-numa cpu,socket-id=0,node-id=0

?

Also perhaps change it like this to make it less verbose and match
"-numa node":

  -numa node,nodeid=0 -numa cpus,nodeid=0,socket=0 \
  -numa node,nodeid=1 -numa cpus,nodeid=1,socket=1

(Yeah, I know, bikeshedding).

Paolo

Re: [Qemu-devel] [RFC 09/13] numa: introduce '-numa cpu' cpu option

[Igor Mammedov]

On Fri, 20 Jan 2017 14:40:34 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 18/01/2017 18:13, Igor Mammedov wrote:
> > It allows to specify mapping of a CPU to NUMA node on CLI.
> > Option should be repeated for each present/possible CPU.
> > Example for PC machine:
> >   -numa node,nodeid=0 -numa node,nodeid=1 \
> >   -numa cpu,socket-id=0,core-id=0,thread-id=0,node-id=0 \
> >   -numa cpu,socket-id=1,core-id=0,thread-id=0,node-id=1
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> What about making core-id/thread-id optional, like
> 
> -numa cpu,socket-id=0,node-id=0
> 
> ?
> 
> Also perhaps change it like this to make it less verbose and match
> "-numa node":
> 
>   -numa node,nodeid=0 -numa cpus,nodeid=0,socket=0 \
>   -numa node,nodeid=1 -numa cpus,nodeid=1,socket=1

the reason, I've made it strict/mandatory is that it's becomes
trivial property setting and matches mandatory options for -device x86-foo-cpu,...

we can always make it less strict afterwards but not other way around,
but I can try to make it less strict on respin.

> (Yeah, I know, bikeshedding).
> 
> Paolo

Re: [Qemu-devel] [RFC 05/13] pc: move pcms->possible_cpus init out of pc_cpus_init()

[Igor Mammedov]

On Fri, 20 Jan 2017 11:31:56 +0800
Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> At 01/19/2017 01:13 AM, Igor Mammedov wrote:
> > possible_cpus could be initialized earlier then cpu objects,  
> 
> s/then/than/
ok,
I'll fix it on respin

> 
> > i.e. when -smp is parsed so move init code to possible_cpu_arch_ids()  
> 
> [...]
> 
> 
> 

Re: [Qemu-devel] [RFC 00/13] numa: add '-numa cpu' option

[Igor Mammedov]

On Wed, 18 Jan 2017 18:13:16 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

[...]
>   pc: cleanup: move smbios_set_cpuid() into pc_build_smbios()
>   pc: don't return cpu pointer from pc_new_cpu() as it's not needed
>     anymore
>   make possible_cpu_arch_ids() return const pointer
Eduardo,

Could you review/merge 2-4/13 as generic cleanups to PC code
so I won't spam list with them on respin?

>   pc: move pcms->possible_cpus init out of pc_cpus_init()
>   pc: calculate topology only once when possible_cpus is initialised
>   pc: pass apic_id to pc_find_cpu_slot() directly so lookup could be
>     done without CPU object
>   pc: add writeonly 'cpu' property to PCMachine
>   numa: introduce '-numa cpu' cpu option
>   numa: replace cpu_index_to_socket_id() with
>     cpu_index_to_instance_props()
>   numa: use new machine.cpu property with -numa cpus=... CLI
>   pc: drop usage of legacy numa_get_node_for_cpu()
>   pc: cpu: make sure that cpu.node-id matches -numa mapping
> 
>  include/hw/acpi/acpi_dev_interface.h |   2 +-
>  include/hw/boards.h                  |  14 ++-
>  include/hw/i386/pc.h                 |   2 +-
>  include/qom/cpu.h                    |   2 -
>  include/sysemu/numa.h                |   3 +-
>  target/arm/cpu.h                     |   2 +
>  target/i386/cpu.h                    |   1 +
>  target/ppc/cpu.h                     |   2 +
>  hw/acpi/cpu.c                        |  13 +--
>  hw/acpi/cpu_hotplug.c                |   4 +-
>  hw/arm/virt.c                        |  12 +-
>  hw/i386/acpi-build.c                 |  25 ++---
>  hw/i386/pc.c                         | 210 +++++++++++++++++++++++------------
>  hw/ppc/spapr.c                       |  15 ++-
>  hw/ppc/spapr_cpu_core.c              |   2 +-
>  monitor.c                            |   7 +-
>  numa.c                               |  89 +++++++++++----
>  qapi-schema.json                     |   3 +-
>  stubs/pc_madt_cpu_entry.c            |   2 +-
>  target/arm/cpu.c                     |   1 +
>  target/i386/cpu.c                    |   1 +
>  target/ppc/translate_init.c          |   1 +
>  vl.c                                 |   4 +-
>  23 files changed, 269 insertions(+), 148 deletions(-)
> 

Re: [Qemu-devel] [RFC 02/13] pc: cleanup: move smbios_set_cpuid() into pc_build_smbios()

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:18PM +0100, Igor Mammedov wrote:
> move smbios_set_cpuid() close to the rest of smbios init code
> where it belongs to instead of calling it from pc_cpus_init().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applying to machine-next. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [RFC 03/13] pc: don't return cpu pointer from pc_new_cpu() as it's not needed anymore

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:19PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applying to machine-next. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Bharata B Rao]

On Thu, Jan 19, 2017 at 04:04:23PM +0100, Igor Mammedov wrote:
> On Wed, 18 Jan 2017 16:57:13 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> > > it will allow generic numa code to set cpu to numa node mapping
> > > in target independent manner in the next patch.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index f8ea635..1d33a5e 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
> > >      pcms->pit = value;
> > >  }
> > >  
> > > +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> > > +                               void *opaque, Error **errp)
> > > +{
> > > +    uint32_t apic_id;
> > > +    X86CPUTopoInfo topo;
> > > +    CPUArchId *cpu_slot;
> > > +    Error *local_err = NULL;
> > > +    CpuInstanceProperties *cpu_props = NULL;
> > > +    PCMachineState *pcms = PC_MACHINE(obj);
> > > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> > > +
> > > +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (!cpu_props->has_node_id) {
> > > +        error_setg(&local_err, "node-id property is not specified");
> > > +        goto out;
> > > +    }
> > > +
> > > +    /*
> > > +     * make sure that possible_cpus is initialized
> > > +     * as property setter might be called before machine init is called
> > > +     */
> > > +    mc->possible_cpu_arch_ids(MACHINE(obj));
> > > +
> > > +    topo.pkg_id = cpu_props->socket_id;
> > > +    topo.core_id = cpu_props->core_id;
> > > +    topo.smt_id = cpu_props->thread_id;
> > > +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> > > +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);  
> > 
> > If we make TYPE_MACHINE provide an API to query CPU slots, e.g.:
> >   CPUArchId *machine_find_cpu_slot(MachineState *m, CpuInstanceProperties *props)
> so if there is no objections, 
> I'll move possible_cpus to MachineState
> and add to MachineClass above callback so target machine
> would be able to provide arch specific lookup function.
> it should work for both x86 and ARM.

The need for possible_cpus in MachineState for sPAPR isn't immediately
apparent to me. In the context of this new numa "cpu" property, PC target
seems to use possible_cpus to store and later lookup the numa node id for
a given CPU. Wondering if that could be achieved w/o needing possible_cpus
in MachineState ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Eduardo Habkost]

On Mon, Jan 23, 2017 at 12:20:33PM +0530, Bharata B Rao wrote:
> On Thu, Jan 19, 2017 at 04:04:23PM +0100, Igor Mammedov wrote:
> > On Wed, 18 Jan 2017 16:57:13 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:
> > > > it will allow generic numa code to set cpu to numa node mapping
> > > > in target independent manner in the next patch.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index f8ea635..1d33a5e 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
> > > >      pcms->pit = value;
> > > >  }
> > > >  
> > > > +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> > > > +                               void *opaque, Error **errp)
> > > > +{
> > > > +    uint32_t apic_id;
> > > > +    X86CPUTopoInfo topo;
> > > > +    CPUArchId *cpu_slot;
> > > > +    Error *local_err = NULL;
> > > > +    CpuInstanceProperties *cpu_props = NULL;
> > > > +    PCMachineState *pcms = PC_MACHINE(obj);
> > > > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> > > > +
> > > > +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> > > > +    if (local_err) {
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (!cpu_props->has_node_id) {
> > > > +        error_setg(&local_err, "node-id property is not specified");
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * make sure that possible_cpus is initialized
> > > > +     * as property setter might be called before machine init is called
> > > > +     */
> > > > +    mc->possible_cpu_arch_ids(MACHINE(obj));
> > > > +
> > > > +    topo.pkg_id = cpu_props->socket_id;
> > > > +    topo.core_id = cpu_props->core_id;
> > > > +    topo.smt_id = cpu_props->thread_id;
> > > > +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> > > > +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);  
> > > 
> > > If we make TYPE_MACHINE provide an API to query CPU slots, e.g.:
> > >   CPUArchId *machine_find_cpu_slot(MachineState *m, CpuInstanceProperties *props)
> > so if there is no objections, 
> > I'll move possible_cpus to MachineState
> > and add to MachineClass above callback so target machine
> > would be able to provide arch specific lookup function.
> > it should work for both x86 and ARM.
> 
> The need for possible_cpus in MachineState for sPAPR isn't immediately
> apparent to me. In the context of this new numa "cpu" property, PC target
> seems to use possible_cpus to store and later lookup the numa node id for
> a given CPU. Wondering if that could be achieved w/o needing possible_cpus
> in MachineState ?

We need to save the node ID for not-yet-plugged CPUs somewhere,
and the existing numa_info[].node_cpu field is cpu_index-based so
it needs to be replaced. A MachineState field would allow us to
do that in a generic way.

-- 
Eduardo

Re: [Qemu-devel] [RFC 08/13] pc: add writeonly 'cpu' property to PCMachine

[Igor Mammedov]

On Mon, 23 Jan 2017 13:03:50 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jan 23, 2017 at 12:20:33PM +0530, Bharata B Rao wrote:
> > On Thu, Jan 19, 2017 at 04:04:23PM +0100, Igor Mammedov wrote:  
> > > On Wed, 18 Jan 2017 16:57:13 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Jan 18, 2017 at 06:13:24PM +0100, Igor Mammedov wrote:  
> > > > > it will allow generic numa code to set cpu to numa node mapping
> > > > > in target independent manner in the next patch.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  hw/i386/pc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 56 insertions(+)
> > > > > 
> > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > index f8ea635..1d33a5e 100644
> > > > > --- a/hw/i386/pc.c
> > > > > +++ b/hw/i386/pc.c
> > > > > @@ -2201,6 +2201,56 @@ static void pc_machine_set_pit(Object *obj, bool value, Error **errp)
> > > > >      pcms->pit = value;
> > > > >  }
> > > > >  
> > > > > +static void pc_machine_set_cpu(Object *obj, Visitor *v, const char *name,
> > > > > +                               void *opaque, Error **errp)
> > > > > +{
> > > > > +    uint32_t apic_id;
> > > > > +    X86CPUTopoInfo topo;
> > > > > +    CPUArchId *cpu_slot;
> > > > > +    Error *local_err = NULL;
> > > > > +    CpuInstanceProperties *cpu_props = NULL;
> > > > > +    PCMachineState *pcms = PC_MACHINE(obj);
> > > > > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> > > > > +
> > > > > +    visit_type_CpuInstanceProperties(v, name, &cpu_props, &local_err);
> > > > > +    if (local_err) {
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    if (!cpu_props->has_node_id) {
> > > > > +        error_setg(&local_err, "node-id property is not specified");
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    /*
> > > > > +     * make sure that possible_cpus is initialized
> > > > > +     * as property setter might be called before machine init is called
> > > > > +     */
> > > > > +    mc->possible_cpu_arch_ids(MACHINE(obj));
> > > > > +
> > > > > +    topo.pkg_id = cpu_props->socket_id;
> > > > > +    topo.core_id = cpu_props->core_id;
> > > > > +    topo.smt_id = cpu_props->thread_id;
> > > > > +    apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> > > > > +    cpu_slot = pc_find_cpu_slot(pcms, apic_id, NULL);    
> > > > 
> > > > If we make TYPE_MACHINE provide an API to query CPU slots, e.g.:
> > > >   CPUArchId *machine_find_cpu_slot(MachineState *m, CpuInstanceProperties *props)  
> > > so if there is no objections, 
> > > I'll move possible_cpus to MachineState
> > > and add to MachineClass above callback so target machine
> > > would be able to provide arch specific lookup function.
> > > it should work for both x86 and ARM.  
> > 
> > The need for possible_cpus in MachineState for sPAPR isn't immediately
> > apparent to me. In the context of this new numa "cpu" property, PC target
> > seems to use possible_cpus to store and later lookup the numa node id for
> > a given CPU. Wondering if that could be achieved w/o needing possible_cpus
> > in MachineState ?  
> 
> We need to save the node ID for not-yet-plugged CPUs somewhere,
> and the existing numa_info[].node_cpu field is cpu_index-based so
> it needs to be replaced. A MachineState field would allow us to
> do that in a generic way.
I'm trying to quickly hack SPAPR code to use possible_cpus
to show how it would be used. But using the same possible_cpus
across targets have as minimum a benefit of uniform approach
and possibly more code sharing.