[Qemu-devel] [PATCH 00/11] pc: allow to use -device/device_add for CPUs

[Qemu-devel] [PATCH 00/11] pc: allow to use -device/device_add for CPUs

[Igor Mammedov]

Series enabling usage of -device/device_add for adding CPUs
as devices. Using -device/device_add in combination with
query-hotpluggable-cpus QMP command allows to hotplug CPUs
at any not used possition and then safely migrate QEMU
instance by specifying hotadded CPUs on target with help
of -device CLI option like with any other device.
Having been able to hotadd arbitrary CPUs and migrate also
opens poosibility to hot-remove arbitrary CPUs, which will
be a separate series on top of this one.

Series depends on following not yet commited patchsets:
 1: [PATCH v2 00/10] ACPI CPU hotplug refactoring to support unplug and more than 255 CPUs
      https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04671.html
 2: [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties
      https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02579.html
   3: [PATCH v2 00/10] globals: Clean up validation and error checking
        https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05662.html

git tree for testing:
  https://github.com/imammedo/qemu.git dev_add_cpu_v1
for viewing:
  https://github.com/imammedo/qemu/commits/dev_add_cpu_v1

Igor Mammedov (11):
  target-i386: cpu: use uint32_t for X86CPU.apic_id
  pc: add x86_topo_ids_from_apicid()
  pc: extract CPU lookup into a separate function
  pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()
  target-i386: cpu: replace custom apic-id setter/getter with static
    property
  target-i386: add socket/core/thread properties to X86CPU
  pc: set APIC ID based on socket/core/thread ids if it's not been set
    yet
  pc: implement query-hotpluggable-cpus callback
  pc: register created initial and hotpluged CPUs in one place
    pc_cpu_plug()
  pc: delay setting number of boot CPUs to machine_done time
  pc: cpu: allow device_add to be used with x86 cpu

 hw/i386/pc.c               | 170 +++++++++++++++++++++++++++++++++++----------
 include/hw/i386/topology.h |  15 ++++
 qmp-commands.hx            |  15 ++++
 target-i386/cpu.c          |  64 +++--------------
 target-i386/cpu.h          |   8 ++-
 5 files changed, 180 insertions(+), 92 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/11] target-i386: cpu: use uint32_t for X86CPU.apic_id

[Igor Mammedov]

redo
 9886e834 target-i386: Require APIC ID to be explicitly set before CPU realize
in another way that doesn't use int64_t to detect
if apic-id property was set.

Use the fact that 0xFFFFFFFF is the broadcast
value that a CPU can't have and set default
uint32_t apic_id to it instead of using int64_t.

Later uint32_t apic_id will be used to drop custom
property setter/getter in favor of static property.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5c69c43..e7319e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2883,7 +2883,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
-    if (cpu->apic_id < 0) {
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
     }
@@ -3154,7 +3154,7 @@ static void x86_cpu_initfn(Object *obj)
 
 #ifndef CONFIG_USER_ONLY
     /* Any code creating new X86CPU objects have to set apic-id explicitly */
-    cpu->apic_id = -1;
+    cpu->apic_id = UNASSIGNED_APIC_ID;
 #endif
 
     for (w = 0; w < FEATURE_WORDS; w++) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f4f65ce..8b09cfa 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -835,6 +835,8 @@ typedef struct {
 
 #define NB_OPMASK_REGS 8
 
+#define UNASSIGNED_APIC_ID UINT32_MAX
+
 typedef union X86LegacyXSaveArea {
     struct {
         uint16_t fcw;
@@ -1163,7 +1165,7 @@ struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
-    int64_t apic_id;
+    uint32_t apic_id;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/11] pc: add x86_topo_ids_from_apicid()

[Igor Mammedov]

it's reverse of apicid_from_topo_ids() and will be used in follow up
patches to fill in data structures for query-hotpluggable-cpus and
for user friendly error reporting

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/topology.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index fc95572..1ebaee0 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -117,6 +117,21 @@ static inline void x86_topo_ids_from_idx(unsigned nr_cores,
     topo->pkg_id = core_index / nr_cores;
 }
 
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on APIC ID
+ */
+static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
+                                            unsigned nr_cores,
+                                            unsigned nr_threads,
+                                            X86CPUTopoInfo *topo)
+{
+    topo->smt_id = apicid &
+                   ~(0xFFFFFFFFUL << apicid_smt_width(nr_cores, nr_threads));
+    topo->core_id = (apicid >> apicid_core_offset(nr_cores, nr_threads)) &
+                   ~(0xFFFFFFFFUL << apicid_core_width(nr_cores, nr_threads));
+    topo->pkg_id = apicid >> apicid_pkg_offset(nr_cores, nr_threads);
+}
+
 /* Make APIC ID for the CPU 'cpu_index'
  *
  * 'cpu_index' is a sequential, contiguous ID for the CPU.
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/11] pc: extract CPU lookup into a separate function

[Igor Mammedov]

it will be reused in the next patch at pre_plug time

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 07a3b82..ec9b14b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1675,11 +1675,25 @@ static int pc_apic_cmp(const void *a, const void *b)
    return apic_a->arch_id - apic_b->arch_id;
 }
 
+static CPUArchId *pc_find_cpu(PCMachineState *pcms, CPUState *cpu, int *idx)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    CPUArchId apic_id, *found_cpu;
+
+    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
+    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
+        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
+        pc_apic_cmp);
+    if (found_cpu && idx) {
+        *idx = found_cpu - pcms->possible_cpus->cpus;
+    }
+    return found_cpu;
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
-    CPUClass *cc = CPU_GET_CLASS(dev);
-    CPUArchId apic_id, *found_cpu;
+    CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
@@ -1703,11 +1717,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
     /* increment the number of CPUs */
     rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
 
-    apic_id.arch_id = cc->get_arch_id(CPU(dev));
-    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
-        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
-        pc_apic_cmp);
-    assert(found_cpu);
+    found_cpu = pc_find_cpu(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
     error_propagate(errp, local_err);
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/11] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Igor Mammedov]

Machine code knows about all possible APIC IDs so use that
instead of hack which does O(n^2) complexity duplicate
checks, interating over global CPUs list.
As result duplicate check is done only once with O(log n) complexity.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ec9b14b..6ba6343 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1071,18 +1071,6 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    if (cpu_exists(apic_id)) {
-        error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", it already exists", id);
-        return;
-    }
-
-    if (id >= max_cpus) {
-        error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", max allowed: %d", id, max_cpus - 1);
-        return;
-    }
-
     if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
                    ", resulting APIC ID (%" PRIi64 ") is too large",
@@ -1766,6 +1754,37 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
+                            DeviceState *dev, Error **errp)
+{
+    int idx;
+    X86CPU *cpu = X86_CPU(dev);
+    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
+
+    if (!cpu_slot) {
+        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
+                   "), valid range 0:%d", cpu->apic_id,
+                   pcms->possible_cpus->len - 1);
+        return;
+    }
+
+    if (cpu_slot->cpu) {
+        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
+                   cpu_slot - pcms->possible_cpus->cpus,
+                   cpu->apic_id);
+        return;
+    }
+}
+
+static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+    }
+}
+
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -2063,6 +2082,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
+    hc->pre_plug = pc_machine_device_pre_plug_cb;
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
     hc->unplug = pc_machine_device_unplug_cb;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7319e3..9511474 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1838,8 +1838,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
 {
     X86CPU *cpu = X86_CPU(obj);
     DeviceState *dev = DEVICE(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
     Error *error = NULL;
     int64_t value;
 
@@ -1854,17 +1852,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
         error_propagate(errp, error);
         return;
     }
-    if (value < min || value > max) {
-        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
-                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
-                   object_get_typename(obj), name, value, min, max);
-        return;
-    }
-
-    if ((value != cpu->apic_id) && cpu_exists(value)) {
-        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
-        return;
-    }
     cpu->apic_id = value;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property

[Igor Mammedov]

custom apic-id setter/getter doesn't do any property specific
checks anymorer, so clean it up and use more compact static
property DEFINE_PROP_UINT32 instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 45 ++++++---------------------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9511474..9294b3d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
     cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
 }
 
-static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    int64_t value = cpu->apic_id;
-
-    visit_type_int(v, name, &value, errp);
-}
-
-static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    DeviceState *dev = DEVICE(obj);
-    Error *error = NULL;
-    int64_t value;
-
-    if (dev->realized) {
-        error_setg(errp, "Attempt to set property '%s' on '%s' after "
-                   "it was realized", name, object_get_typename(obj));
-        return;
-    }
-
-    visit_type_int(v, name, &value, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-    cpu->apic_id = value;
-}
-
 /* Generic getter for "feature-words" and "filtered-features" properties */
 static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
@@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
-    object_property_add(obj, "apic-id", "int",
-                        x86_cpuid_get_apic_id,
-                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
     object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
                         x86_cpu_get_feature_words,
                         NULL, NULL, (void *)env->features, NULL);
@@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
 
-#ifndef CONFIG_USER_ONLY
-    /* Any code creating new X86CPU objects have to set apic-id explicitly */
-    cpu->apic_id = UNASSIGNED_APIC_ID;
-#endif
-
     for (w = 0; w < FEATURE_WORDS; w++) {
         int bitnr;
 
@@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
 }
 
 static Property x86_cpu_properties[] = {
+#ifdef CONFIG_USER_ONLY
+    /* apic_id = 0 by default for *-user, see commit 9886e834 */
+    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
+#else
+    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
+#endif
     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),
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

these properties will be used by as address where to plug
CPU with help -device/device_add commands.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      | 12 ++++++++++++
 target-i386/cpu.c |  3 +++
 target-i386/cpu.h |  4 ++++
 3 files changed, 19 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6ba6343..87352ae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1775,6 +1775,18 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                    cpu->apic_id);
         return;
     }
+
+    /* set 'address' properties socket/core/thread for initial and cpu-add
+     * added CPUs so that query_hotpluggable_cpus would show correct values
+     */
+    if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) {
+        X86CPUTopoInfo topo;
+
+        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+        cpu->thread = topo.smt_id;
+        cpu->core = topo.core_id;
+        cpu->socket = topo.pkg_id;
+    }
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9294b3d..8c651f2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3186,6 +3186,9 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
     DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
     DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
+    DEFINE_PROP_INT32("thread", X86CPU, thread, -1),
+    DEFINE_PROP_INT32("core", X86CPU, core, -1),
+    DEFINE_PROP_INT32("socket", X86CPU, socket, -1),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8b09cfa..a04d334 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1190,6 +1190,10 @@ struct X86CPU {
     Notifier machine_done;
 
     struct kvm_msrs *kvm_msr_buf;
+
+    int32_t socket;
+    int32_t core;
+    int32_t thread;
 };
 
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/11] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

CPU added with device_add help won't have APIC ID set,
so set it according to socket/core/thread ids provided
with device_add command.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 87352ae..63e9bb6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1758,13 +1758,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    CPUArchId *cpu_slot;
+    X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
 
+    if (cpu->apic_id == 0xFFFFFFFF) {
+        /* APIC ID not set, set it based on socket/core/thread properties */
+        X86CPUTopoInfo topo = { cpu->socket, cpu->core, cpu->thread };
+        cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
+    }
+
+    cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
     if (!cpu_slot) {
-        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
-                   "), valid range 0:%d", cpu->apic_id,
+        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+        error_setg(errp, "Invalid CPU[socket: %d, core: %d, thread: %d] with"
+                  " APIC ID (%" PRIu32 "), valid index range 0:%d",
+                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
                    pcms->possible_cpus->len - 1);
         return;
     }
@@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
      * added CPUs so that query_hotpluggable_cpus would show correct values
      */
     if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) {
-        X86CPUTopoInfo topo;
-
         x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
         cpu->thread = topo.smt_id;
         cpu->core = topo.core_id;
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/11] pc: implement query-hotpluggable-cpus callback

[Igor Mammedov]

it returns a list of present/possible to hotplug CPU
objects with a list of properties to use with
device_add.

in PC case returned list would looks like:
-> { "execute": "query-hotpluggable-cpus" }
<- {"return": [
     {
        "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
        "props": {"core": 0, "socket": 1, "thread": 0}
     },
     {
        "qom-path": "/machine/unattached/device[0]",
        "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
        "props": {"core": 0, "socket": 0, "thread": 0}
     }
   ]}

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx | 15 +++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 63e9bb6..3bd52f6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2058,6 +2058,50 @@ static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
     return list;
 }
 
+static HotpluggableCPUList *pc_query_hotpluggable_cpus(MachineState *machine)
+{
+    int i;
+    CPUState *cpu;
+    HotpluggableCPUList *head = NULL;
+    PCMachineState *pcms = PC_MACHINE(machine);
+    const char *cpu_type;
+
+    cpu = pcms->possible_cpus->cpus[0].cpu;
+    assert(cpu); /* BSP is always present */
+    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 = true;
+        cpu_props->socket = topo.pkg_id;
+        cpu_props->has_core = true;
+        cpu_props->core = topo.core_id;
+        cpu_props->has_thread = true;
+        cpu_props->thread = topo.smt_id;
+        cpu_item->props = cpu_props;
+
+        cpu = pcms->possible_cpus->cpus[i].cpu;
+        if (cpu) {
+            cpu_item->has_qom_path = true;
+            cpu_item->qom_path = object_get_canonical_path(OBJECT(cpu));
+        }
+
+        list_item->value = cpu_item;
+        list_item->next = head;
+        head = list_item;
+    }
+    return head;
+}
+
 static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
 {
     /* cpu index isn't used */
@@ -2098,6 +2142,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
+    mc->query_hotpluggable_cpus = pc_query_hotpluggable_cpus;
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b444c20..380e110 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4983,3 +4983,18 @@ Example for pseries machine type started with
      { "props": { "core": 0 }, "type": "POWER8-spapr-cpu-core",
        "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
    ]}'
+
+Example for pc machine type started with
+-smp 1,maxcpus=2:
+    -> { "execute": "query-hotpluggable-cpus" }
+    <- {"return": [
+         {
+            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+            "props": {"core": 0, "socket": 1, "thread": 0}
+         },
+         {
+            "qom-path": "/machine/unattached/device[0]",
+            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+            "props": {"core": 0, "socket": 0, "thread": 0}
+         }
+       ]}
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/11] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Igor Mammedov]

consolidate possible_cpus array management in pc_cpu_plug()
for smp_cpus, coldplugged with -device and hotplugged with
device_add.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3bd52f6..b6b9acc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1145,7 +1145,6 @@ void pc_cpus_init(PCMachineState *pcms)
         if (i < smp_cpus) {
             cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
                              &error_fatal);
-            pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
         }
     }
@@ -1686,25 +1685,19 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
-    if (!dev->hotplugged) {
-        goto out;
-    }
-
-    if (!pcms->acpi_dev) {
-        error_setg(&local_err,
-                   "cpu hotplug is not enabled: missing acpi device");
-        goto out;
+    if (pcms->acpi_dev) {
+        hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+        hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+        if (local_err) {
+            goto out;
+        }
     }
 
-    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
-    hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
-    if (local_err) {
-        goto out;
+    if (dev->hotplugged) {
+        /* increment the number of CPUs */
+        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
     }
 
-    /* increment the number of CPUs */
-    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
-
     found_cpu = pc_find_cpu(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/11] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

currently present CPUs counter in CMOS only contains
smp_cpus (i.e. initial CPUs specified with -smp X) and
doesn't account for CPUs created with -device.
If VM is started with additional CPUs added with
 -device, it will hang in BIOS waiting for condition
   smp_cpus == counted_cpus
forever as counted_cpus will include -device CPUs as well
and be more than smp_cpus.

make present CPUs counter in CMOS to count all CPUs
(initial and coldplugged with -device) by delaying
it to machine done time when it possible to count
CPUs added with -device.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b6b9acc..0b3baf0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -471,9 +471,6 @@ void pc_cmos_init(PCMachineState *pcms,
     rtc_set_memory(s, 0x5c, val >> 8);
     rtc_set_memory(s, 0x5d, val >> 16);
 
-    /* set the number of CPU */
-    rtc_set_memory(s, 0x5f, smp_cpus - 1);
-
     object_property_add_link(OBJECT(pcms), "rtc_state",
                              TYPE_ISA_DEVICE,
                              (Object **)&pcms->rtc,
@@ -1156,10 +1153,19 @@ void pc_cpus_init(PCMachineState *pcms)
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
+    int i, boot_cpus = 0;
     PCMachineState *pcms = container_of(notifier,
                                         PCMachineState, machine_done);
     PCIBus *bus = pcms->bus;
 
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        if (pcms->possible_cpus->cpus[i].cpu) {
+            boot_cpus++;
+        }
+    }
+    /* set the number of CPUs */
+    rtc_set_memory(pcms->rtc, 0x5f, boot_cpus - 1);
+
     if (bus) {
         int extra_hosts = 0;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/11] pc: cpu: allow device_add to be used with x86 cpu

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8c651f2..a67e551 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3236,6 +3236,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->cpu_exec_enter = x86_cpu_exec_enter;
     cc->cpu_exec_exit = x86_cpu_exec_exit;
 
+    dc->cannot_instantiate_with_device_add_yet = false;
     /*
      * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
      * object in cpus -> dangling pointer after final object_unref().
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/11] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 04:54:25PM +0200, Igor Mammedov wrote:
> CPU added with device_add help won't have APIC ID set,
> so set it according to socket/core/thread ids provided
> with device_add command.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 87352ae..63e9bb6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1758,13 +1758,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> +    CPUArchId *cpu_slot;
> +    X86CPUTopoInfo topo;
>      X86CPU *cpu = X86_CPU(dev);
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
>  
> +    if (cpu->apic_id == 0xFFFFFFFF) {
> +        /* APIC ID not set, set it based on socket/core/thread properties */
> +        X86CPUTopoInfo topo = { cpu->socket, cpu->core, cpu->thread };
> +        cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
> +    }

What happens if neither apic-id or socket/core/thread are set?

apicid_from_topo_ids() needs the caller to ensure
core_id < nr_cores && smt_id < nr_threads. Do you do that?

> +
> +    cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
>      if (!cpu_slot) {
> -        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
> -                   "), valid range 0:%d", cpu->apic_id,
> +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
> +        error_setg(errp, "Invalid CPU[socket: %d, core: %d, thread: %d] with"
> +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",
> +                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
>                     pcms->possible_cpus->len - 1);

The error message is a bit confusing: the interface is not based
on CPU index anymore, but it still says "valid index range 0:%d".

>          return;
>      }
> @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>       * added CPUs so that query_hotpluggable_cpus would show correct values
>       */
>      if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) {
> -        X86CPUTopoInfo topo;
> -
>          x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
>          cpu->thread = topo.smt_id;
>          cpu->core = topo.core_id;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/11] pc: extract CPU lookup into a separate function

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 04:54:21PM +0200, Igor Mammedov wrote:
> it will be reused in the next patch at pre_plug time
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 07a3b82..ec9b14b 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1675,11 +1675,25 @@ static int pc_apic_cmp(const void *a, const void *b)
>     return apic_a->arch_id - apic_b->arch_id;
>  }
>  
> +static CPUArchId *pc_find_cpu(PCMachineState *pcms, CPUState *cpu, int *idx)

A short comment explaining what it does (and what does the return
value actually means) would't be bad.

The function name confused me when reviewing patch 07/11, as:
1) returning non-NULL means a CPU "slot"[1] exists, not
necessarily that a CPU exists; 2) returning NULL means that a CPU
doesn't exist _and_ that the ID/slot is not available for
plugging.

I would call it pc_find_cpu_slot() or pc_find_possible_cpu_id().

[1] Or whatever name we choose to call a "possible ID/address for
    a CPU".


> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    CPUArchId apic_id, *found_cpu;
> +
> +    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
> +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> +        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> +        pc_apic_cmp);
> +    if (found_cpu && idx) {
> +        *idx = found_cpu - pcms->possible_cpus->cpus;
> +    }
> +    return found_cpu;
> +}
> +
>  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>                          DeviceState *dev, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(dev);
> -    CPUArchId apic_id, *found_cpu;
> +    CPUArchId *found_cpu;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> @@ -1703,11 +1717,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>      /* increment the number of CPUs */
>      rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
>  
> -    apic_id.arch_id = cc->get_arch_id(CPU(dev));
> -    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> -        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> -        pc_apic_cmp);
> -    assert(found_cpu);
> +    found_cpu = pc_find_cpu(pcms, CPU(dev), NULL);
>      found_cpu->cpu = CPU(dev);
>  out:
>      error_propagate(errp, local_err);
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/11] pc: extract CPU lookup into a separate function

[Igor Mammedov]

On Thu, 23 Jun 2016 14:32:10 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 04:54:21PM +0200, Igor Mammedov wrote:
> > it will be reused in the next patch at pre_plug time
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 07a3b82..ec9b14b 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1675,11 +1675,25 @@ static int pc_apic_cmp(const void *a, const
> > void *b) return apic_a->arch_id - apic_b->arch_id;
> >  }
> >  
> > +static CPUArchId *pc_find_cpu(PCMachineState *pcms, CPUState *cpu,
> > int *idx)
> 
> A short comment explaining what it does (and what does the return
> value actually means) would't be bad.
> 
> The function name confused me when reviewing patch 07/11, as:
> 1) returning non-NULL means a CPU "slot"[1] exists, not
> necessarily that a CPU exists; 2) returning NULL means that a CPU
> doesn't exist _and_ that the ID/slot is not available for
> plugging.
> 
> I would call it pc_find_cpu_slot() or
> pc_find_possible_cpu_id().
Ok, I'll rename it to pc_find_cpu_slot() and add comment

> 
> [1] Or whatever name we choose to call a "possible ID/address for
>     a CPU".
> 
> 
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    CPUArchId apic_id, *found_cpu;
> > +
> > +    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
> > +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > +        pcms->possible_cpus->len,
> > sizeof(*pcms->possible_cpus->cpus),
> > +        pc_apic_cmp);
> > +    if (found_cpu && idx) {
> > +        *idx = found_cpu - pcms->possible_cpus->cpus;
> > +    }
> > +    return found_cpu;
> > +}
> > +
> >  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >                          DeviceState *dev, Error **errp)
> >  {
> > -    CPUClass *cc = CPU_GET_CLASS(dev);
> > -    CPUArchId apic_id, *found_cpu;
> > +    CPUArchId *found_cpu;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > @@ -1703,11 +1717,7 @@ static void pc_cpu_plug(HotplugHandler
> > *hotplug_dev, /* increment the number of CPUs */
> >      rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc,
> > 0x5f) + 1); 
> > -    apic_id.arch_id = cc->get_arch_id(CPU(dev));
> > -    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > -        pcms->possible_cpus->len,
> > sizeof(*pcms->possible_cpus->cpus),
> > -        pc_apic_cmp);
> > -    assert(found_cpu);
> > +    found_cpu = pc_find_cpu(pcms, CPU(dev), NULL);
> >      found_cpu->cpu = CPU(dev);
> >  out:
> >      error_propagate(errp, local_err);
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 04:54:24PM +0200, Igor Mammedov wrote:
> these properties will be used by as address where to plug
> CPU with help -device/device_add commands.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c      | 12 ++++++++++++
>  target-i386/cpu.c |  3 +++
>  target-i386/cpu.h |  4 ++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6ba6343..87352ae 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1775,6 +1775,18 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                     cpu->apic_id);
>          return;
>      }
> +
> +    /* set 'address' properties socket/core/thread for initial and cpu-add
> +     * added CPUs so that query_hotpluggable_cpus would show correct values
> +     */
> +    if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1) {
> +        X86CPUTopoInfo topo;
> +
> +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
> +        cpu->thread = topo.smt_id;
> +        cpu->core = topo.core_id;
> +        cpu->socket = topo.pkg_id;
> +    }

What if both apic-id and socket/core/thread are set?

I suggest validating the properties, and setting them in case
they are not set:

    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);

    if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
        error_setg(errp, "CPU socket ID mismatch: ...");
        return;
    }
    cpu->socket = topo.socket_id;

    if (cpu->core != -1 && cpu->core != topo.core_id) {
        error_setg(errp, "CPU core ID mismatch: ...");
        return;
    }
    cpu->core = topo.core_id;

    if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
        error_setg(errp, "CPU thread ID mismatch: ...");
        return;
    }
    cpu->thread = topo.smt_id;

We could do that inside x86_cpu_realizefn(), so that
socket/core/thread would be always set in all CPUs.

>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9294b3d..8c651f2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3186,6 +3186,9 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
>      DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
> +    DEFINE_PROP_INT32("thread", X86CPU, thread, -1),
> +    DEFINE_PROP_INT32("core", X86CPU, core, -1),
> +    DEFINE_PROP_INT32("socket", X86CPU, socket, -1),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 8b09cfa..a04d334 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1190,6 +1190,10 @@ struct X86CPU {
>      Notifier machine_done;
>  
>      struct kvm_msrs *kvm_msr_buf;
> +
> +    int32_t socket;
> +    int32_t core;
> +    int32_t thread;
>  };
>  
>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/11] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

On Thu, 23 Jun 2016 14:19:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 04:54:25PM +0200, Igor Mammedov wrote:
> > CPU added with device_add help won't have APIC ID set,
> > so set it according to socket/core/thread ids provided
> > with device_add command.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 87352ae..63e9bb6 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1758,13 +1758,23 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev, Error **errp)
> >  {
> >      int idx;
> > +    CPUArchId *cpu_slot;
> > +    X86CPUTopoInfo topo;
> >      X86CPU *cpu = X86_CPU(dev);
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > -    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> >  
> > +    if (cpu->apic_id == 0xFFFFFFFF) {
> > +        /* APIC ID not set, set it based on socket/core/thread
> > properties */
> > +        X86CPUTopoInfo topo = { cpu->socket, cpu->core,
> > cpu->thread };
> > +        cpu->apic_id = apicid_from_topo_ids(smp_cores,
> > smp_threads, &topo);
> > +    }
> 
> What happens if neither apic-id or socket/core/thread are set?
> 
> apicid_from_topo_ids() needs the caller to ensure
> core_id < nr_cores && smt_id < nr_threads. Do you do that?
it will get wrong apic_id and error in "if (!cpu_slot)" will trigger,
I can explicitly check for unset values report error saying that
they must be set if you prefer.

> 
> > +
> > +    cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> >      if (!cpu_slot) {
> > -        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
> > -                   "), valid range 0:%d", cpu->apic_id,
> > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > smp_threads, &topo);
> > +        error_setg(errp, "Invalid CPU[socket: %d, core: %d,
> > thread: %d] with"
> > +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",
> > +                   topo.pkg_id, topo.core_id, topo.smt_id,
> > cpu->apic_id, pcms->possible_cpus->len - 1);
> 
> The error message is a bit confusing: the interface is not based
> on CPU index anymore, but it still says "valid index range 0:%d".
it's still based on CPU index for cpu-add and -numa cpus,
so until we get rid of index in there it still should be printed.

> 
> >          return;
> >      }
> > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev,
> >       * added CPUs so that query_hotpluggable_cpus would show
> > correct values */
> >      if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1)
> > {
> > -        X86CPUTopoInfo topo;
> > -
> >          x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > smp_threads, &topo); cpu->thread = topo.smt_id;
> >          cpu->core = topo.core_id;
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

On Thu, 23 Jun 2016 14:44:53 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 04:54:24PM +0200, Igor Mammedov wrote:
> > these properties will be used by as address where to plug
> > CPU with help -device/device_add commands.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c      | 12 ++++++++++++
> >  target-i386/cpu.c |  3 +++
> >  target-i386/cpu.h |  4 ++++
> >  3 files changed, 19 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6ba6343..87352ae 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1775,6 +1775,18 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev, cpu->apic_id);
> >          return;
> >      }
> > +
> > +    /* set 'address' properties socket/core/thread for initial and
> > cpu-add
> > +     * added CPUs so that query_hotpluggable_cpus would show
> > correct values
> > +     */
> > +    if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1)
> > {
> > +        X86CPUTopoInfo topo;
> > +
> > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > smp_threads, &topo);
> > +        cpu->thread = topo.smt_id;
> > +        cpu->core = topo.core_id;
> > +        cpu->socket = topo.pkg_id;
> > +    }
> 
> What if both apic-id and socket/core/thread are set?
they shouldn't since query_hotpluggable_cpus doesn't have apic_id
attribute so apic_id isn't supposed to be on user provided,
but of cause user could add it manually on device_add command to create
confusion, in that case apic_id would take precedence and
socket/core/thread ignored.

> 
> I suggest validating the properties, and setting them in case
> they are not set:
> 
>     x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads,
> &topo);
> 
>     if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
>         error_setg(errp, "CPU socket ID mismatch: ...");
>         return;
>     }
>     cpu->socket = topo.socket_id;
> 
>     if (cpu->core != -1 && cpu->core != topo.core_id) {
>         error_setg(errp, "CPU core ID mismatch: ...");
>         return;
>     }
>     cpu->core = topo.core_id;
> 
>     if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
>         error_setg(errp, "CPU thread ID mismatch: ...");
>         return;
>     }
>     cpu->thread = topo.smt_id;
> 
> We could do that inside x86_cpu_realizefn(), so that
> socket/core/thread would be always set in all CPUs.
all CPUs pass through pc_cpu_pre_plug() before cpu.relizefn()
so I'd rather do it here at board level responsible for
setting apic_id or socket/core/thread info is not set.


> 
> >  }
> >  
> >  static void pc_machine_device_pre_plug_cb(HotplugHandler
> > *hotplug_dev, diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9294b3d..8c651f2 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3186,6 +3186,9 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
> >      DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
> >      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
> > +    DEFINE_PROP_INT32("thread", X86CPU, thread, -1),
> > +    DEFINE_PROP_INT32("core", X86CPU, core, -1),
> > +    DEFINE_PROP_INT32("socket", X86CPU, socket, -1),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 8b09cfa..a04d334 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1190,6 +1190,10 @@ struct X86CPU {
> >      Notifier machine_done;
> >  
> >      struct kvm_msrs *kvm_msr_buf;
> > +
> > +    int32_t socket;
> > +    int32_t core;
> > +    int32_t thread;
> >  };
> >  
> >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH 07/11] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 07:48:24PM +0200, Igor Mammedov wrote:
> On Thu, 23 Jun 2016 14:19:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 23, 2016 at 04:54:25PM +0200, Igor Mammedov wrote:
> > > CPU added with device_add help won't have APIC ID set,
> > > so set it according to socket/core/thread ids provided
> > > with device_add command.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 87352ae..63e9bb6 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1758,13 +1758,23 @@ static void pc_cpu_pre_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev, Error **errp)
> > >  {
> > >      int idx;
> > > +    CPUArchId *cpu_slot;
> > > +    X86CPUTopoInfo topo;
> > >      X86CPU *cpu = X86_CPU(dev);
> > >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > > -    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> > >  
> > > +    if (cpu->apic_id == 0xFFFFFFFF) {
> > > +        /* APIC ID not set, set it based on socket/core/thread
> > > properties */
> > > +        X86CPUTopoInfo topo = { cpu->socket, cpu->core,
> > > cpu->thread };
> > > +        cpu->apic_id = apicid_from_topo_ids(smp_cores,
> > > smp_threads, &topo);
> > > +    }
> > 
> > What happens if neither apic-id or socket/core/thread are set?
> > 
> > apicid_from_topo_ids() needs the caller to ensure
> > core_id < nr_cores && smt_id < nr_threads. Do you do that?
> it will get wrong apic_id and error in "if (!cpu_slot)" will trigger,
> I can explicitly check for unset values report error saying that
> they must be set if you prefer.

Will it? If smp_cores=2 and smp_threads=2,
socket=0,core=0,thread=3 will generate a valid APIC ID
(corresponding to socket=0,core=1,thread=1).

About unset/invalid values: apicid_from_topo_ids() documentation
explicitly requires core_id < nr_cores && smt_id < nr_threads,
and may generate a valid APIC ID if only some of the
socket/core/thread values are set to -1.

(Also, if you don't check for missing/invalid values explicitly
you will generate a very confusing error message for the user).

> 
> > 
> > > +
> > > +    cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> > >      if (!cpu_slot) {
> > > -        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
> > > -                   "), valid range 0:%d", cpu->apic_id,
> > > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > smp_threads, &topo);
> > > +        error_setg(errp, "Invalid CPU[socket: %d, core: %d,
> > > thread: %d] with"
> > > +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",
> > > +                   topo.pkg_id, topo.core_id, topo.smt_id,
> > > cpu->apic_id, pcms->possible_cpus->len - 1);
> > 
> > The error message is a bit confusing: the interface is not based
> > on CPU index anymore, but it still says "valid index range 0:%d".
> it's still based on CPU index for cpu-add and -numa cpus,
> so until we get rid of index in there it still should be printed.

OK.

> 
> > 
> > >          return;
> > >      }
> > > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler
> > > *hotplug_dev,
> > >       * added CPUs so that query_hotpluggable_cpus would show
> > > correct values */
> > >      if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1)
> > > {
> > > -        X86CPUTopoInfo topo;
> > > -
> > >          x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > smp_threads, &topo); cpu->thread = topo.smt_id;
> > >          cpu->core = topo.core_id;
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 09:18:38PM +0200, Igor Mammedov wrote:
> On Thu, 23 Jun 2016 14:44:53 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 23, 2016 at 04:54:24PM +0200, Igor Mammedov wrote:
> > > these properties will be used by as address where to plug
> > > CPU with help -device/device_add commands.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c      | 12 ++++++++++++
> > >  target-i386/cpu.c |  3 +++
> > >  target-i386/cpu.h |  4 ++++
> > >  3 files changed, 19 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 6ba6343..87352ae 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1775,6 +1775,18 @@ static void pc_cpu_pre_plug(HotplugHandler
> > > *hotplug_dev, cpu->apic_id);
> > >          return;
> > >      }
> > > +
> > > +    /* set 'address' properties socket/core/thread for initial and
> > > cpu-add
> > > +     * added CPUs so that query_hotpluggable_cpus would show
> > > correct values
> > > +     */
> > > +    if (cpu->thread == -1 || cpu->core == -1 || cpu->socket == -1)
> > > {
> > > +        X86CPUTopoInfo topo;
> > > +
> > > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > smp_threads, &topo);
> > > +        cpu->thread = topo.smt_id;
> > > +        cpu->core = topo.core_id;
> > > +        cpu->socket = topo.pkg_id;
> > > +    }
> > 
> > What if both apic-id and socket/core/thread are set?
> they shouldn't since query_hotpluggable_cpus doesn't have apic_id
> attribute so apic_id isn't supposed to be on user provided,
> but of cause user could add it manually on device_add command to create
> confusion, in that case apic_id would take precedence and
> socket/core/thread ignored.

I would like to reject obviously invalid input instead of having
unclear precedence rules.

> 
> > 
> > I suggest validating the properties, and setting them in case
> > they are not set:
> > 
> >     x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads,
> > &topo);
> > 
> >     if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
> >         error_setg(errp, "CPU socket ID mismatch: ...");
> >         return;
> >     }
> >     cpu->socket = topo.socket_id;
> > 
> >     if (cpu->core != -1 && cpu->core != topo.core_id) {
> >         error_setg(errp, "CPU core ID mismatch: ...");
> >         return;
> >     }
> >     cpu->core = topo.core_id;
> > 
> >     if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
> >         error_setg(errp, "CPU thread ID mismatch: ...");
> >         return;
> >     }
> >     cpu->thread = topo.smt_id;
> > 
> > We could do that inside x86_cpu_realizefn(), so that
> > socket/core/thread would be always set in all CPUs.
> all CPUs pass through pc_cpu_pre_plug() before cpu.relizefn()
> so I'd rather do it here at board level responsible for
> setting apic_id or socket/core/thread info is not set.

Then *-user will be inconsistent, and will always have invalid
values in socket/core/thread. If one day we add any logic using
the socket/core/thread properties in cpu.c, it will break on
*-user.

All those properties are X86CPU properties, meaning they are
input to the X86CPU code. I don't see why we should move logic
related to them outside cpu.c unless really necessary.

(The apic-id calculation at patch 07/11, for example, is more
difficult to move to cpu.c, because the PC code needs the APIC ID
before calling realize. We could move it to the apic-id getter,
but I dislike having magic getter/setters and like that you made
it a static property.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

On Thu, 23 Jun 2016 17:18:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 09:18:38PM +0200, Igor Mammedov wrote:
> > On Thu, 23 Jun 2016 14:44:53 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Jun 23, 2016 at 04:54:24PM +0200, Igor Mammedov wrote:
> > > > these properties will be used by as address where to plug
> > > > CPU with help -device/device_add commands.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/i386/pc.c      | 12 ++++++++++++
> > > >  target-i386/cpu.c |  3 +++
> > > >  target-i386/cpu.h |  4 ++++
> > > >  3 files changed, 19 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 6ba6343..87352ae 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1775,6 +1775,18 @@ static void
> > > > pc_cpu_pre_plug(HotplugHandler *hotplug_dev, cpu->apic_id);
> > > >          return;
> > > >      }
> > > > +
> > > > +    /* set 'address' properties socket/core/thread for initial
> > > > and cpu-add
> > > > +     * added CPUs so that query_hotpluggable_cpus would show
> > > > correct values
> > > > +     */
> > > > +    if (cpu->thread == -1 || cpu->core == -1 || cpu->socket ==
> > > > -1) {
> > > > +        X86CPUTopoInfo topo;
> > > > +
> > > > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > > smp_threads, &topo);
> > > > +        cpu->thread = topo.smt_id;
> > > > +        cpu->core = topo.core_id;
> > > > +        cpu->socket = topo.pkg_id;
> > > > +    }
> > > 
> > > What if both apic-id and socket/core/thread are set?
> > they shouldn't since query_hotpluggable_cpus doesn't have apic_id
> > attribute so apic_id isn't supposed to be on user provided,
> > but of cause user could add it manually on device_add command to
> > create confusion, in that case apic_id would take precedence and
> > socket/core/thread ignored.
> 
> I would like to reject obviously invalid input instead of having
> unclear precedence rules.
ok

> > 
> > > 
> > > I suggest validating the properties, and setting them in case
> > > they are not set:
> > > 
> > >     x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads,
> > > &topo);
> > > 
> > >     if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
> > >         error_setg(errp, "CPU socket ID mismatch: ...");
> > >         return;
> > >     }
> > >     cpu->socket = topo.socket_id;
> > > 
> > >     if (cpu->core != -1 && cpu->core != topo.core_id) {
> > >         error_setg(errp, "CPU core ID mismatch: ...");
> > >         return;
> > >     }
> > >     cpu->core = topo.core_id;
> > > 
> > >     if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
> > >         error_setg(errp, "CPU thread ID mismatch: ...");
> > >         return;
> > >     }
> > >     cpu->thread = topo.smt_id;
> > > 
> > > We could do that inside x86_cpu_realizefn(), so that
> > > socket/core/thread would be always set in all CPUs.
> > all CPUs pass through pc_cpu_pre_plug() before cpu.relizefn()
> > so I'd rather do it here at board level responsible for
> > setting apic_id or socket/core/thread info is not set.
> 
> Then *-user will be inconsistent, and will always have invalid
> values in socket/core/thread. If one day we add any logic using
> the socket/core/thread properties in cpu.c, it will break on
> *-user.
> 
> All those properties are X86CPU properties, meaning they are
> input to the X86CPU code. I don't see why we should move logic
> related to them outside cpu.c unless really necessary.
> 
> (The apic-id calculation at patch 07/11, for example, is more
> difficult to move to cpu.c, because the PC code needs the APIC ID
> before calling realize. We could move it to the apic-id getter,
> but I dislike having magic getter/setters and like that you made
> it a static property.)
set of socket/core/thread is a synonym for apic_id and it's board that
manages and knows valid values for them. Putting above snippet into
cpu.relizefn() would make CPU access globals smp_cores, smp_threads
which are essentially machine_state and Drew working on moving them
there and eliminating globals. So suddenly CPU would need to poke into
machine object, and we return to the same state wrt *-user only with
hack in cpu.c.

I'd worry about -smp and *-user when it comes into that target as it
will probably need apic_id and maybe socket/core/thread as well.

Re: [Qemu-devel] [PATCH 07/11] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

On Thu, 23 Jun 2016 16:27:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 07:48:24PM +0200, Igor Mammedov wrote:
> > On Thu, 23 Jun 2016 14:19:16 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Jun 23, 2016 at 04:54:25PM +0200, Igor Mammedov wrote:
> > > > CPU added with device_add help won't have APIC ID set,
> > > > so set it according to socket/core/thread ids provided
> > > > with device_add command.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/i386/pc.c | 18 +++++++++++++-----
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 87352ae..63e9bb6 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1758,13 +1758,23 @@ static void
> > > > pc_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > Error **errp) {
> > > >      int idx;
> > > > +    CPUArchId *cpu_slot;
> > > > +    X86CPUTopoInfo topo;
> > > >      X86CPU *cpu = X86_CPU(dev);
> > > >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > > > -    CPUArchId *cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> > > >  
> > > > +    if (cpu->apic_id == 0xFFFFFFFF) {
> > > > +        /* APIC ID not set, set it based on socket/core/thread
> > > > properties */
> > > > +        X86CPUTopoInfo topo = { cpu->socket, cpu->core,
> > > > cpu->thread };
> > > > +        cpu->apic_id = apicid_from_topo_ids(smp_cores,
> > > > smp_threads, &topo);
> > > > +    }
> > > 
> > > What happens if neither apic-id or socket/core/thread are set?
> > > 
> > > apicid_from_topo_ids() needs the caller to ensure
> > > core_id < nr_cores && smt_id < nr_threads. Do you do that?
> > it will get wrong apic_id and error in "if (!cpu_slot)" will
> > trigger, I can explicitly check for unset values report error
> > saying that they must be set if you prefer.
> 
> Will it? If smp_cores=2 and smp_threads=2,
> socket=0,core=0,thread=3 will generate a valid APIC ID
> (corresponding to socket=0,core=1,thread=1).
> 
> About unset/invalid values: apicid_from_topo_ids() documentation
> explicitly requires core_id < nr_cores && smt_id < nr_threads,
> and may generate a valid APIC ID if only some of the
> socket/core/thread values are set to -1.
> 
> (Also, if you don't check for missing/invalid values explicitly
> you will generate a very confusing error message for the user).
ok, I'll add explicit checks for unset/invalid values here
as you suggested in previous patch.

> 
> > 
> > > 
> > > > +
> > > > +    cpu_slot = pc_find_cpu(pcms, CPU(dev), &idx);
> > > >      if (!cpu_slot) {
> > > > -        error_setg(errp, "Invalid CPU index with APIC ID (%"
> > > > PRIu32
> > > > -                   "), valid range 0:%d", cpu->apic_id,
> > > > +        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > > smp_threads, &topo);
> > > > +        error_setg(errp, "Invalid CPU[socket: %d, core: %d,
> > > > thread: %d] with"
> > > > +                  " APIC ID (%" PRIu32 "), valid index range
> > > > 0:%d",
> > > > +                   topo.pkg_id, topo.core_id, topo.smt_id,
> > > > cpu->apic_id, pcms->possible_cpus->len - 1);
> > > 
> > > The error message is a bit confusing: the interface is not based
> > > on CPU index anymore, but it still says "valid index range 0:%d".
> > it's still based on CPU index for cpu-add and -numa cpus,
> > so until we get rid of index in there it still should be printed.
> 
> OK.
> 
> > 
> > > 
> > > >          return;
> > > >      }
> > > > @@ -1780,8 +1790,6 @@ static void pc_cpu_pre_plug(HotplugHandler
> > > > *hotplug_dev,
> > > >       * added CPUs so that query_hotpluggable_cpus would show
> > > > correct values */
> > > >      if (cpu->thread == -1 || cpu->core == -1 || cpu->socket ==
> > > > -1) {
> > > > -        X86CPUTopoInfo topo;
> > > > -
> > > >          x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > > smp_threads, &topo); cpu->thread = topo.smt_id;
> > > >          cpu->core = topo.core_id;
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 10:46:36PM +0200, Igor Mammedov wrote:
> On Thu, 23 Jun 2016 17:18:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > 
> > > > 
> > > > I suggest validating the properties, and setting them in case
> > > > they are not set:
> > > > 
> > > >     x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads,
> > > > &topo);
> > > > 
> > > >     if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
> > > >         error_setg(errp, "CPU socket ID mismatch: ...");
> > > >         return;
> > > >     }
> > > >     cpu->socket = topo.socket_id;
> > > > 
> > > >     if (cpu->core != -1 && cpu->core != topo.core_id) {
> > > >         error_setg(errp, "CPU core ID mismatch: ...");
> > > >         return;
> > > >     }
> > > >     cpu->core = topo.core_id;
> > > > 
> > > >     if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
> > > >         error_setg(errp, "CPU thread ID mismatch: ...");
> > > >         return;
> > > >     }
> > > >     cpu->thread = topo.smt_id;
> > > > 
> > > > We could do that inside x86_cpu_realizefn(), so that
> > > > socket/core/thread would be always set in all CPUs.
> > > all CPUs pass through pc_cpu_pre_plug() before cpu.relizefn()
> > > so I'd rather do it here at board level responsible for
> > > setting apic_id or socket/core/thread info is not set.
> > 
> > Then *-user will be inconsistent, and will always have invalid
> > values in socket/core/thread. If one day we add any logic using
> > the socket/core/thread properties in cpu.c, it will break on
> > *-user.
> > 
> > All those properties are X86CPU properties, meaning they are
> > input to the X86CPU code. I don't see why we should move logic
> > related to them outside cpu.c unless really necessary.
> > 
> > (The apic-id calculation at patch 07/11, for example, is more
> > difficult to move to cpu.c, because the PC code needs the APIC ID
> > before calling realize. We could move it to the apic-id getter,
> > but I dislike having magic getter/setters and like that you made
> > it a static property.)
> set of socket/core/thread is a synonym for apic_id and it's board that
> manages and knows valid values for them.

The CPU already knows exactly what the bits inside APIC ID mean,
because it has to report topology information through CPUID.

> Putting above snippet into
> cpu.relizefn() would make CPU access globals smp_cores, smp_threads
> which are essentially machine_state and Drew working on moving them
> there and eliminating globals. So suddenly CPU would need to poke into
> machine object, and we return to the same state wrt *-user only with
> hack in cpu.c.

After Drew's code is included, we can simply use
CPUState::nr_cores and CPUState::nr_threads.

> 
> I'd worry about -smp and *-user when it comes into that target as it
> will probably need apic_id and maybe socket/core/thread as well.

The point is to not even have to worry about *-user later, by
keeping both softmmu and *-user consistent. People reviewing
patches a few years from now probably wouldn't even notice that
code using the socket/core/thread fields will break in *-user.

I wouldn't mind about having the code in pc.c at all, if it
didn't make *-user inconsistent, or if the CPU object didn't had
all the required information yet. But I don't think it is
reasonable to intentionally leave X86CPU fields inconsistent in
*-user if we can easily fix it by moving initialization to
realizefn.

But if you are really strongly against that, I can propose that
as a follow-up later (after Drew's series is included).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 06/11] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

On Thu, 23 Jun 2016 18:43:53 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 10:46:36PM +0200, Igor Mammedov wrote:
> > On Thu, 23 Jun 2016 17:18:46 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> > > > 
> > > > > 
> > > > > I suggest validating the properties, and setting them in case
> > > > > they are not set:
> > > > > 
> > > > >     x86_topo_ids_from_apicid(cpu->apic_id, smp_cores,
> > > > > smp_threads, &topo);
> > > > > 
> > > > >     if (cpu->socket != -1 && cpu->socket != topo.socket_id) {
> > > > >         error_setg(errp, "CPU socket ID mismatch: ...");
> > > > >         return;
> > > > >     }
> > > > >     cpu->socket = topo.socket_id;
> > > > > 
> > > > >     if (cpu->core != -1 && cpu->core != topo.core_id) {
> > > > >         error_setg(errp, "CPU core ID mismatch: ...");
> > > > >         return;
> > > > >     }
> > > > >     cpu->core = topo.core_id;
> > > > > 
> > > > >     if (cpu->thread != -1 && cpu->thread != topo.smt_id) {
> > > > >         error_setg(errp, "CPU thread ID mismatch: ...");
> > > > >         return;
> > > > >     }
> > > > >     cpu->thread = topo.smt_id;
> > > > > 
> > > > > We could do that inside x86_cpu_realizefn(), so that
> > > > > socket/core/thread would be always set in all CPUs.
> > > > all CPUs pass through pc_cpu_pre_plug() before cpu.relizefn()
> > > > so I'd rather do it here at board level responsible for
> > > > setting apic_id or socket/core/thread info is not set.
> > > 
> > > Then *-user will be inconsistent, and will always have invalid
> > > values in socket/core/thread. If one day we add any logic using
> > > the socket/core/thread properties in cpu.c, it will break on
> > > *-user.
> > > 
> > > All those properties are X86CPU properties, meaning they are
> > > input to the X86CPU code. I don't see why we should move logic
> > > related to them outside cpu.c unless really necessary.
> > > 
> > > (The apic-id calculation at patch 07/11, for example, is more
> > > difficult to move to cpu.c, because the PC code needs the APIC ID
> > > before calling realize. We could move it to the apic-id getter,
> > > but I dislike having magic getter/setters and like that you made
> > > it a static property.)
> > set of socket/core/thread is a synonym for apic_id and it's board
> > that manages and knows valid values for them.
> 
> The CPU already knows exactly what the bits inside APIC ID mean,
> because it has to report topology information through CPUID.
> 
> > Putting above snippet into
> > cpu.relizefn() would make CPU access globals smp_cores, smp_threads
> > which are essentially machine_state and Drew working on moving them
> > there and eliminating globals. So suddenly CPU would need to poke
> > into machine object, and we return to the same state wrt *-user
> > only with hack in cpu.c.
> 
> After Drew's code is included, we can simply use
> CPUState::nr_cores and CPUState::nr_threads.
> 
> > 
> > I'd worry about -smp and *-user when it comes into that target as it
> > will probably need apic_id and maybe socket/core/thread as well.
> 
> The point is to not even have to worry about *-user later, by
> keeping both softmmu and *-user consistent. People reviewing
> patches a few years from now probably wouldn't even notice that
> code using the socket/core/thread fields will break in *-user.
> 
> I wouldn't mind about having the code in pc.c at all, if it
> didn't make *-user inconsistent, or if the CPU object didn't had
> all the required information yet. But I don't think it is
> reasonable to intentionally leave X86CPU fields inconsistent in
> *-user if we can easily fix it by moving initialization to
> realizefn.
> 
> But if you are really strongly against that, I can propose that
> as a follow-up later (after Drew's series is included).
Lets do it as follow-up after Drew's -smp refactoring is in.

Re: [Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote:
> custom apic-id setter/getter doesn't do any property specific
> checks anymorer, so clean it up and use more compact static
> property DEFINE_PROP_UINT32 instead.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 45 ++++++---------------------------------------
>  1 file changed, 6 insertions(+), 39 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9511474..9294b3d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
>  }
>  
> -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -    int64_t value = cpu->apic_id;
> -
> -    visit_type_int(v, name, &value, errp);
> -}
> -
> -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -    DeviceState *dev = DEVICE(obj);
> -    Error *error = NULL;
> -    int64_t value;
> -
> -    if (dev->realized) {
> -        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> -                   "it was realized", name, object_get_typename(obj));
> -        return;
> -    }
> -
> -    visit_type_int(v, name, &value, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -    cpu->apic_id = value;
> -}
> -
>  /* Generic getter for "feature-words" and "filtered-features" properties */
>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
>                                        const char *name, void *opaque,
> @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> -    object_property_add(obj, "apic-id", "int",
> -                        x86_cpuid_get_apic_id,
> -                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
>      object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
>                          x86_cpu_get_feature_words,
>                          NULL, NULL, (void *)env->features, NULL);
> @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
>  
>      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
>  
> -#ifndef CONFIG_USER_ONLY
> -    /* Any code creating new X86CPU objects have to set apic-id explicitly */
> -    cpu->apic_id = UNASSIGNED_APIC_ID;
> -#endif
> -
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          int bitnr;
>  
> @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
>  }
>  
>  static Property x86_cpu_properties[] = {
> +#ifdef CONFIG_USER_ONLY
> +    /* apic_id = 0 by default for *-user, see commit 9886e834 */
> +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> +#else
> +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> +#endif

Suggestion for a follow-up patch: setting the default to
UNASSIGNED_APIC_ID unconditionally, and just adding this to
x86_cpu_realizefn().

  #ifdef CONFIG_USER_ONLY
      if (cpu->apic_id == UNASSIGNED_APIC_ID) {
          cpu->apic_id = 0;
      }
  #endif

-- 
Eduardo

Re: [Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property

[Igor Mammedov]

On Mon, 27 Jun 2016 14:55:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote:
> > custom apic-id setter/getter doesn't do any property specific
> > checks anymorer, so clean it up and use more compact static
> > property DEFINE_PROP_UINT32 instead.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 45 ++++++---------------------------------------
> >  1 file changed, 6 insertions(+), 39 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9511474..9294b3d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> >      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> >  }
> >  
> > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name,
> > -                                  void *opaque, Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -    int64_t value = cpu->apic_id;
> > -
> > -    visit_type_int(v, name, &value, errp);
> > -}
> > -
> > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
> > -                                  void *opaque, Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -    DeviceState *dev = DEVICE(obj);
> > -    Error *error = NULL;
> > -    int64_t value;
> > -
> > -    if (dev->realized) {
> > -        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> > -                   "it was realized", name, object_get_typename(obj));
> > -        return;
> > -    }
> > -
> > -    visit_type_int(v, name, &value, &error);
> > -    if (error) {
> > -        error_propagate(errp, error);
> > -        return;
> > -    }
> > -    cpu->apic_id = value;
> > -}
> > -
> >  /* Generic getter for "feature-words" and "filtered-features" properties */
> >  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> >                                        const char *name, void *opaque,
> > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
> >      object_property_add(obj, "tsc-frequency", "int",
> >                          x86_cpuid_get_tsc_freq,
> >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > -    object_property_add(obj, "apic-id", "int",
> > -                        x86_cpuid_get_apic_id,
> > -                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
> >      object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
> >                          x86_cpu_get_feature_words,
> >                          NULL, NULL, (void *)env->features, NULL);
> > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
> >  
> >      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
> >  
> > -#ifndef CONFIG_USER_ONLY
> > -    /* Any code creating new X86CPU objects have to set apic-id explicitly */
> > -    cpu->apic_id = UNASSIGNED_APIC_ID;
> > -#endif
> > -
> >      for (w = 0; w < FEATURE_WORDS; w++) {
> >          int bitnr;
> >  
> > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
> >  }
> >  
> >  static Property x86_cpu_properties[] = {
> > +#ifdef CONFIG_USER_ONLY
> > +    /* apic_id = 0 by default for *-user, see commit 9886e834 */
> > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> > +#else
> > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> > +#endif  
> 
> Suggestion for a follow-up patch: setting the default to
> UNASSIGNED_APIC_ID unconditionally, and just adding this to
> x86_cpu_realizefn().
> 
>   #ifdef CONFIG_USER_ONLY
>       if (cpu->apic_id == UNASSIGNED_APIC_ID) {
>           cpu->apic_id = 0;
>       }
>   #endif
Putting default along with property definition seemed cleaner to me,
considering that we want do similar thing for socket/core/thread-ids
it still seems a better way.

BTW:
there is a v2 on list already, in case you missed it.

 [PATCH v2 00/18] pc: add CPU hot-add/hot-remove with device_add/device_del

Re: [Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property

[Eduardo Habkost]

On Tue, Jun 28, 2016 at 08:43:59AM +0200, Igor Mammedov wrote:
> On Mon, 27 Jun 2016 14:55:24 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote:
> > > custom apic-id setter/getter doesn't do any property specific
> > > checks anymorer, so clean it up and use more compact static
> > > property DEFINE_PROP_UINT32 instead.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 45 ++++++---------------------------------------
> > >  1 file changed, 6 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 9511474..9294b3d 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> > >      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > >  }
> > >  
> > > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name,
> > > -                                  void *opaque, Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    int64_t value = cpu->apic_id;
> > > -
> > > -    visit_type_int(v, name, &value, errp);
> > > -}
> > > -
> > > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
> > > -                                  void *opaque, Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    DeviceState *dev = DEVICE(obj);
> > > -    Error *error = NULL;
> > > -    int64_t value;
> > > -
> > > -    if (dev->realized) {
> > > -        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> > > -                   "it was realized", name, object_get_typename(obj));
> > > -        return;
> > > -    }
> > > -
> > > -    visit_type_int(v, name, &value, &error);
> > > -    if (error) {
> > > -        error_propagate(errp, error);
> > > -        return;
> > > -    }
> > > -    cpu->apic_id = value;
> > > -}
> > > -
> > >  /* Generic getter for "feature-words" and "filtered-features" properties */
> > >  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> > >                                        const char *name, void *opaque,
> > > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
> > >      object_property_add(obj, "tsc-frequency", "int",
> > >                          x86_cpuid_get_tsc_freq,
> > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > -    object_property_add(obj, "apic-id", "int",
> > > -                        x86_cpuid_get_apic_id,
> > > -                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
> > >      object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
> > >                          x86_cpu_get_feature_words,
> > >                          NULL, NULL, (void *)env->features, NULL);
> > > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
> > >  
> > >      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
> > >  
> > > -#ifndef CONFIG_USER_ONLY
> > > -    /* Any code creating new X86CPU objects have to set apic-id explicitly */
> > > -    cpu->apic_id = UNASSIGNED_APIC_ID;
> > > -#endif
> > > -
> > >      for (w = 0; w < FEATURE_WORDS; w++) {
> > >          int bitnr;
> > >  
> > > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
> > >  }
> > >  
> > >  static Property x86_cpu_properties[] = {
> > > +#ifdef CONFIG_USER_ONLY
> > > +    /* apic_id = 0 by default for *-user, see commit 9886e834 */
> > > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> > > +#else
> > > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> > > +#endif  
> > 
> > Suggestion for a follow-up patch: setting the default to
> > UNASSIGNED_APIC_ID unconditionally, and just adding this to
> > x86_cpu_realizefn().
> > 
> >   #ifdef CONFIG_USER_ONLY
> >       if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> >           cpu->apic_id = 0;
> >       }
> >   #endif
> Putting default along with property definition seemed cleaner to me,
> considering that we want do similar thing for socket/core/thread-ids
> it still seems a better way.

Agreed that consistency is more important.

It bothers me that we don't have a better mechanism to declare
different defaults depending on CONFIG_USER_ONLY, but that's
something to be discussed later.

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

-- 
Eduardo