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

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

[Igor Mammedov]

Changelog:
  since v1:
    * s/pc_find_cpu/pc_find_cpu_slot/ + add comment to it
    * add more sanity checks for socket-id/core-id/thread-id and 'apic'
      properties
    * include device_del cpu patches and related fixes to x86 CPU/apic

Series enabling usage of -device/device_add for adding CPUs as devices
and device_del for removing them. 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 this series enables by permitting using
device_del with x86 CPUs.

Series depends on following not yet commited patchsets:
 1: [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties
      https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02579.html
    2: [PATCH v2 00/10] globals: Clean up validation and error checking
         https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05662.html
 3: [PATCH v2 2/2] qapi: keep names in 'CpuInstanceProperties' in sync with struct CPUCore
     https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06972.html


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

Tested with RHEL7.2 guest including ping/pong migration with adding/removing
CPUs in between.
CPU removal only slightly tested (~ for a week) so please give it more attention
when testing.
v1 for referrence:
  https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06810.html

Igor Mammedov (18):
  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: delay setting number of boot CPUs to machine_done time
  pc: register created initial and hotpluged CPUs in one place
    pc_cpu_plug()
  pc: cpu: allow device_add to be used with x86 cpu
  apic: move MAX_APICS check to 'apic' class
  apic: drop APICCommonState.idx and use APIC ID as index in
    local_apics[]
  (kvm)apic: add unrealize callbacks
  target-i386: do not ignore error and fix apic parent
  target-i386: fix apic object leak when CPU is deleted
  target-i386: add x86_cpu_unrealizefn()
  pc: make device_del CPU work for x86 CPUs

 hw/i386/kvm/apic.c              |   5 +
 hw/i386/pc.c                    | 233 ++++++++++++++++++++++++++++++++--------
 hw/intc/apic.c                  |  26 ++++-
 hw/intc/apic_common.c           |  21 ++--
 include/hw/i386/apic_internal.h |   4 +-
 include/hw/i386/topology.h      |  15 +++
 qmp-commands.hx                 |  15 +++
 target-i386/cpu.c               |  88 ++++++---------
 target-i386/cpu.h               |   8 +-
 9 files changed, 300 insertions(+), 115 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 01/18] 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

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

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:05:49PM +0200, Igor Mammedov wrote:
> redo
>  9886e834 target-i386: Require APIC ID to be explicitly set before CPU realize

Proper format:
    Redo 9886e834 ("target-i386: Require APIC ID to be explicitly set before
    CPU realize") in another ...

> 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
>  

Let's add a comment here too. And I think 0xFFFFFFFF
would be cleaner.

> +#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 v2 02/18] 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 v2 03/18] 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>
---
v2:
  - rename pc_find_cpu() into pc_find_cpu_slot() and add comment to it
     Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 07a3b82..e94524a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1675,11 +1675,30 @@ static int pc_apic_cmp(const void *a, const void *b)
    return apic_a->arch_id - apic_b->arch_id;
 }
 
+/* returns pointer to CPUArchId descriptor that matches CPU's apic_id
+ * 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,
+                                   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 +1722,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_slot(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
     error_propagate(errp, local_err);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 04/18] 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 e94524a..db792e6 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",
@@ -1771,6 +1759,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_slot(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)
 {
@@ -2068,6 +2087,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 v2 05/18] 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

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

[Eric Blake]

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

On 06/24/2016 10:05 AM, 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

s/anymorer/anymore/

> 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(-)
> 


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


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

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

[Igor Mammedov]

On Fri, 24 Jun 2016 10:33:10 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 06/24/2016 10:05 AM, 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  
> 
> s/anymorer/anymore/
Thanks,
I'll fix it on respin.

> 
> > 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(-)
> >   
> 
> 

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

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:05:53PM +0200, Igor Mammedov wrote:
> custom apic-id setter/getter doesn't do any property specific
> checks anymorer,

anymore

> 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 v2 06/18] 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>
---
v2:
  - rename socket/core/thread properties to socket-id/core-id/thread-id
  - add mismatch checks for apic_id and socket-id/core-id/thread-id
    in case both are set
---
 hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
 target-i386/cpu.c |  6 ++++++
 target-i386/cpu.h |  4 ++++
 3 files changed, 39 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index db792e6..eaa4b59 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
@@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                    cpu->apic_id);
         return;
     }
+
+    /* if 'address' properties socket-id/core-id/thread-id are not set, set them
+     * so that query_hotpluggable_cpus would show correct values
+     */
+    /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
+     * once -smp refactoring is complete and there will be CPU private
+     * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
+    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
+        error_setg(errp, "CPU socket-id: %d mismatch with apic-id: %u",
+                   cpu->socket_id, cpu->apic_id);
+        return;
+    }
+    cpu->socket_id = topo.pkg_id;
+
+    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
+        error_setg(errp, "CPU core-id: %d mismatch with apic-id: %u",
+                   cpu->core_id, cpu->apic_id);
+        return;
+    }
+    cpu->core_id = topo.core_id;
+
+    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
+        error_setg(errp, "CPU thread-id: %d mismatch with apic-id: %u",
+                   cpu->thread_id, cpu->apic_id);
+        return;
+    }
+    cpu->thread_id = topo.smt_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..1ec40a0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3164,8 +3164,14 @@ 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),
+    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
+    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
+    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
 #else
     DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
+    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
+    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
+    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
 #endif
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8b09cfa..e110633 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_id;
+    int32_t core_id;
+    int32_t thread_id;
 };
 
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:05:54PM +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>
> ---
> v2:
>   - rename socket/core/thread properties to socket-id/core-id/thread-id
>   - add mismatch checks for apic_id and socket-id/core-id/thread-id
>     in case both are set
> ---
>  hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
>  target-i386/cpu.c |  6 ++++++
>  target-i386/cpu.h |  4 ++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index db792e6..eaa4b59 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> +    X86CPUTopoInfo topo;
>      X86CPU *cpu = X86_CPU(dev);
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
> @@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                     cpu->apic_id);
>          return;
>      }
> +
> +    /* if 'address' properties socket-id/core-id/thread-id are not set, set them
> +     * so that query_hotpluggable_cpus would show correct values
> +     */
> +    /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
> +     * once -smp refactoring is complete and there will be CPU private
> +     * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
> +    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
> +    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
> +        error_setg(errp, "CPU socket-id: %d mismatch with apic-id: %u",
> +                   cpu->socket_id, cpu->apic_id);

why not list the pkg_id too?
Same below.

> +        return;
> +    }
> +    cpu->socket_id = topo.pkg_id;
> +
> +    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
> +        error_setg(errp, "CPU core-id: %d mismatch with apic-id: %u",
> +                   cpu->core_id, cpu->apic_id);
> +        return;
> +    }
> +    cpu->core_id = topo.core_id;
> +
> +    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
> +        error_setg(errp, "CPU thread-id: %d mismatch with apic-id: %u",
> +                   cpu->thread_id, cpu->apic_id);
> +        return;
> +    }
> +    cpu->thread_id = topo.smt_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..1ec40a0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3164,8 +3164,14 @@ 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),
> +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
> +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
> +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
>  #else
>      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
> +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
>  #endif
>      DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
>      { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 8b09cfa..e110633 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_id;
> +    int32_t core_id;
> +    int32_t thread_id;
>  };
>  
>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> -- 
> 1.8.3.1

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

[Igor Mammedov]

On Mon, 4 Jul 2016 17:12:23 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 06:05:54PM +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>
> > ---
> > v2:
> >   - rename socket/core/thread properties to
> > socket-id/core-id/thread-id
> >   - add mismatch checks for apic_id and socket-id/core-id/thread-id
> >     in case both are set
> > ---
> >  hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
> >  target-i386/cpu.c |  6 ++++++
> >  target-i386/cpu.h |  4 ++++
> >  3 files changed, 39 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index db792e6..eaa4b59 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev, DeviceState *dev, Error **errp)
> >  {
> >      int idx;
> > +    X86CPUTopoInfo topo;
> >      X86CPU *cpu = X86_CPU(dev);
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >      CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
> > @@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler
> > *hotplug_dev, cpu->apic_id);
> >          return;
> >      }
> > +
> > +    /* if 'address' properties socket-id/core-id/thread-id are not
> > set, set them
> > +     * so that query_hotpluggable_cpus would show correct values
> > +     */
> > +    /* TODO: move socket_id/core_id/thread_id checks into
> > x86_cpu_realizefn()
> > +     * once -smp refactoring is complete and there will be CPU
> > private
> > +     * CPUState::nr_cores and CPUState::nr_threads fields instead
> > of globals */
> > +    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads,
> > &topo);
> > +    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
> > +        error_setg(errp, "CPU socket-id: %d mismatch with apic-id:
> > %u",
> > +                   cpu->socket_id, cpu->apic_id);
> 
> why not list the pkg_id too?
> Same below.
Ok, will do on respin.

> 
> > +        return;
> > +    }
> > +    cpu->socket_id = topo.pkg_id;
> > +
> > +    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
> > +        error_setg(errp, "CPU core-id: %d mismatch with apic-id:
> > %u",
> > +                   cpu->core_id, cpu->apic_id);
> > +        return;
> > +    }
> > +    cpu->core_id = topo.core_id;
> > +
> > +    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
> > +        error_setg(errp, "CPU thread-id: %d mismatch with apic-id:
> > %u",
> > +                   cpu->thread_id, cpu->apic_id);
> > +        return;
> > +    }
> > +    cpu->thread_id = topo.smt_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..1ec40a0 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3164,8 +3164,14 @@ 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),
> > +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
> > +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
> > +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
> >  #else
> >      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id,
> > UNASSIGNED_APIC_ID),
> > +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
> > +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> > +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
> >  #endif
> >      DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
> >      { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 8b09cfa..e110633 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_id;
> > +    int32_t core_id;
> > +    int32_t thread_id;
> >  };
> >  
> >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> > -- 
> > 1.8.3.1

[Qemu-devel] [PATCH v2 07/18] 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>
---
v2:
 - add validity checks for socket-id/core-id/thread-id values
---
 hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index eaa4b59..0a65caf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1763,14 +1763,52 @@ 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_slot(pcms, CPU(dev), &idx);
 
+    /* if APIC ID is not set, set it based on socket/core/thread properties */
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;
+
+        if (cpu->socket_id < 0) {
+            error_setg(errp, "CPU socket-id is not set");
+            return;
+        } else if (cpu->socket_id > max_socket) {
+            error_setg(errp, "Invalid CPU socket-id: %d must be in range 0:%d",
+                       cpu->socket_id, max_socket);
+            return;
+        }
+        if (cpu->core_id < 0) {
+            error_setg(errp, "CPU core-id is not set");
+            return;
+        } else if (cpu->core_id > (smp_cores - 1)) {
+            error_setg(errp, "Invalid CPU core-id: %d must be in range 0:%d",
+                       cpu->core_id, smp_cores - 1);
+            return;
+        }
+        if (cpu->thread_id < 0) {
+            error_setg(errp, "CPU thread-id is not set");
+            return;
+        } else if (cpu->thread_id > (smp_threads - 1)) {
+            error_setg(errp, "Invalid CPU thread-id: %d must be in range 0:%d",
+                       cpu->thread_id, smp_threads - 1);
+            return;
+        }
+
+        topo.pkg_id = cpu->socket_id;
+        topo.core_id = cpu->core_id;
+        topo.smt_id = cpu->thread_id;
+        cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
+    }
+
+    cpu_slot = pc_find_cpu_slot(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;
     }
-- 
1.8.3.1

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

[Eduardo Habkost]

On Fri, Jun 24, 2016 at 06:05:55PM +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>
> ---
> v2:
>  - add validity checks for socket-id/core-id/thread-id values
> ---
>  hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index eaa4b59..0a65caf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,14 +1763,52 @@ 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_slot(pcms, CPU(dev), &idx);
>  
> +    /* if APIC ID is not set, set it based on socket/core/thread properties */
> +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> +        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;

The calculation looks right, but I would love to move this to
common code that takes care of SMP configuration and topology,
instead of an inline calculation. We have been bitten by bugs
related to ad hoc topology calculations before (see commit
7dfddd7).

I think we can do that as a follow-up patch after Drew's series is included.

> +
[...]
> +    cpu_slot = pc_find_cpu_slot(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"

topo.{pkg,core,smt}_id are unsigned. Treating them as signed can generate
confusing error messages:

 (qemu) device_add qemu64-x86_64-cpu,apic-id=0xfffffffe
 Invalid CPU[socket: -2, core: 0, thread: 0] with APIC ID (4294967294), valid index range 0:3

Also, I wonder if showing APIC ID in hex wouldn't be more useful.


> +                  " 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;
>      }
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Mon, 4 Jul 2016 16:17:09 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 06:05:55PM +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>
> > ---
> > v2:
> >  - add validity checks for socket-id/core-id/thread-id values
> > ---
> >  hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 41 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index eaa4b59..0a65caf 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1763,14 +1763,52 @@ 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_slot(pcms, CPU(dev), &idx);
> >  
> > +    /* if APIC ID is not set, set it based on socket/core/thread
> > properties */
> > +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > +        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;
> 
> The calculation looks right, but I would love to move this to
> common code that takes care of SMP configuration and topology,
> instead of an inline calculation. We have been bitten by bugs
> related to ad hoc topology calculations before (see commit
> 7dfddd7).
> 
> I think we can do that as a follow-up patch after Drew's series is
> included.
Ok, I can do it on top of Drew's series as follow-up

> > +
> [...]
> > +    cpu_slot = pc_find_cpu_slot(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"
> 
> topo.{pkg,core,smt}_id are unsigned. Treating them as signed can
> generate confusing error messages:
> 
>  (qemu) device_add qemu64-x86_64-cpu,apic-id=0xfffffffe
>  Invalid CPU[socket: -2, core: 0, thread: 0] with APIC ID
> (4294967294), valid index range 0:3
> 
> Also, I wonder if showing APIC ID in hex wouldn't be more useful.
it would be easier to read, I'll change it to hex.

> 
> 
> > +                  " 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;
> >      }
> > -- 
> > 1.8.3.1
> > 
> > 
> 

[Qemu-devel] [PATCH v2 08/18] 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-id": 0, "socket-id": 1, "thread-id": 0}
     },
     {
        "qom-path": "/machine/unattached/device[0]",
        "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
        "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
     }
   ]}

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 - add -id suffix to socket/core/thread properties to match fixed schema
---
 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 0a65caf..b4902f4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2110,6 +2110,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_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 = 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 */
@@ -2150,6 +2194,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..c19b0a2 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-id": 0, "socket-id": 1, "thread-id": 0}
+         },
+         {
+            "qom-path": "/machine/unattached/device[0]",
+            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+            "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
+         }
+       ]}
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 09/18] 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 b4902f4..06c9de1 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,
@@ -1157,10 +1154,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

Re: [Qemu-devel] [PATCH v2 09/18] pc: delay setting number of boot CPUs to machine_done time

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:05:57PM +0200, Igor Mammedov wrote:
> 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.

what if device_add from HMP is invoked after machine done
but just before bios init?

> 
> 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 b4902f4..06c9de1 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,
> @@ -1157,10 +1154,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

Re: [Qemu-devel] [PATCH v2 09/18] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Mon, 4 Jul 2016 17:17:51 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 06:05:57PM +0200, Igor Mammedov wrote:
> > 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.
> 
> what if device_add from HMP is invoked after machine done
> but just before bios init?

pc_cpu_plug():
  rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);

so BIOS will see and count CPU, the same as with cpu-add HMP command.

> 
> > 
> > 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 b4902f4..06c9de1 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,
> > @@ -1157,10 +1154,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

Re: [Qemu-devel] [PATCH v2 09/18] pc: delay setting number of boot CPUs to machine_done time

[Michael S. Tsirkin]

On Mon, Jul 04, 2016 at 06:15:42PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jul 2016 17:17:51 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jun 24, 2016 at 06:05:57PM +0200, Igor Mammedov wrote:
> > > 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.
> > 
> > what if device_add from HMP is invoked after machine done
> > but just before bios init?
> 
> pc_cpu_plug():
>   rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> 
> so BIOS will see and count CPU, the same as with cpu-add HMP command.

It's all rather fragile.
Can't we call above line for both hotpluggable and non-hotpluggable
CPUs?

> > 
> > > 
> > > 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 b4902f4..06c9de1 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,
> > > @@ -1157,10 +1154,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

Re: [Qemu-devel] [PATCH v2 09/18] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Mon, 4 Jul 2016 19:42:07 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 04, 2016 at 06:15:42PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jul 2016 17:17:51 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jun 24, 2016 at 06:05:57PM +0200, Igor Mammedov wrote:
> > > > 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.
> > > 
> > > what if device_add from HMP is invoked after machine done
> > > but just before bios init?
> > 
> > pc_cpu_plug():
> >   rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) +
> > 1);
> > 
> > so BIOS will see and count CPU, the same as with cpu-add HMP
> > command.
> 
> It's all rather fragile.
> Can't we call above line for both hotpluggable and non-hotpluggable
> CPUs?
I've looked at it before and answer was: not unless we rewrite half of
machine as rtc is initialized quite a bit after initial CPUs and it has
a lot of deps. That's why I've kept current split of initial vs
hotplugged cpus wrt rtc_set_memory()

> 
> > > 
> > > > 
> > > > 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 b4902f4..06c9de1 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,
> > > > @@ -1157,10 +1154,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 v2 10/18] 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 06c9de1..f5124d5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1142,7 +1142,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));
         }
     }
@@ -1697,25 +1696,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_slot(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 11/18] 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 1ec40a0..ebf4140 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3239,6 +3239,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

[Qemu-devel] [PATCH v2 12/18] apic: move MAX_APICS check to 'apic' class

[Igor Mammedov]

MAX_APICS is only used by child 'apic' class and not
by its parent TYPE_APIC_COMMON or any other derived
class.
Move check into end user 'apic' class so it won't
get in the way of other APIC implementations
if they support more then MAX_APICS.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           |  8 --------
 include/hw/i386/apic_internal.h |  4 +---
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index e1ab935..b0d237b 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -28,7 +28,9 @@
 #include "trace.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic-msidef.h"
+#include "qapi/error.h"
 
+#define MAX_APICS 255
 #define MAX_APIC_WORDS 8
 
 #define SYNC_FROM_VAPIC                 0x1
@@ -869,6 +871,14 @@ static const MemoryRegionOps apic_io_ops = {
 static void apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
+    static int apic_no;
+
+    if (apic_no >= MAX_APICS) {
+        error_setg(errp, "%s initialization failed.",
+                   object_get_typename(OBJECT(dev)));
+        return;
+    }
+    s->idx = apic_no++;
 
     memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
                           APIC_SPACE_SIZE);
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index e6eb694..fd425d1 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -299,14 +299,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
-    static int apic_no;
-
-    if (apic_no >= MAX_APICS) {
-        error_setg(errp, "%s initialization failed.",
-                   object_get_typename(OBJECT(dev)));
-        return;
-    }
-    s->idx = apic_no++;
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 74fe935..5d3be9a 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -120,8 +120,6 @@
 #define VAPIC_ENABLE_BIT                0
 #define VAPIC_ENABLE_MASK               (1 << VAPIC_ENABLE_BIT)
 
-#define MAX_APICS 255
-
 typedef struct APICCommonState APICCommonState;
 
 #define TYPE_APIC_COMMON "apic-common"
@@ -175,7 +173,7 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx;
+    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 13/18] apic: drop APICCommonState.idx and use APIC ID as index in local_apics[]

[Igor Mammedov]

local_apics[] is sized to contain all APIC ID supported in xAPIC mode,
so use APIC ID as index in it instead of constantly increasing counter idx.

Fixes error "apic initialization failed" when a CPU hotplugged and
unplugged more times than there are free slots in local_apics[].

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/intc/apic.c                  | 16 +++++++---------
 include/hw/i386/apic_internal.h |  1 -
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index b0d237b..f473572 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -421,7 +421,7 @@ static int apic_find_dest(uint8_t dest)
     int i;
 
     if (apic && apic->id == dest)
-        return dest;  /* shortcut in case apic->id == apic->idx */
+        return dest;  /* shortcut in case apic->id == local_apics[dest]->id */
 
     for (i = 0; i < MAX_APICS; i++) {
         apic = local_apics[i];
@@ -504,14 +504,14 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-        apic_set_bit(deliver_bitmask, s->idx);
+        apic_set_bit(deliver_bitmask, s->id);
         break;
     case 2:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
         break;
     case 3:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
-        apic_reset_bit(deliver_bitmask, s->idx);
+        apic_reset_bit(deliver_bitmask, s->id);
         break;
     }
 
@@ -871,20 +871,18 @@ static const MemoryRegionOps apic_io_ops = {
 static void apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
-    static int apic_no;
 
-    if (apic_no >= MAX_APICS) {
-        error_setg(errp, "%s initialization failed.",
-                   object_get_typename(OBJECT(dev)));
+    if (s->id >= MAX_APICS) {
+        error_setg(errp, "%s initialization failed. APIC ID %d is invalid",
+                   object_get_typename(OBJECT(dev)), s->id);
         return;
     }
-    s->idx = apic_no++;
 
     memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
                           APIC_SPACE_SIZE);
 
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
-    local_apics[s->idx] = s;
+    local_apics[s->id] = s;
 
     msi_nonbroken = true;
 }
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 5d3be9a..e5d1550 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -173,7 +173,6 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 14/18] (kvm)apic: add unrealize callbacks

[Igor Mammedov]

callbacks will do necessary cleanups before APIC device is deleted

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/kvm/apic.c              |  5 +++++
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           | 13 +++++++++++++
 include/hw/i386/apic_internal.h |  1 +
 4 files changed, 29 insertions(+)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c5983c7..6cde5f1 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -192,11 +192,16 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static void kvm_apic_unrealize(DeviceState *dev, Error **errp)
+{
+}
+
 static void kvm_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
     k->realize = kvm_apic_realize;
+    k->unrealize = kvm_apic_unrealize;
     k->reset = kvm_apic_reset;
     k->set_base = kvm_apic_set_base;
     k->set_tpr = kvm_apic_set_tpr;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index f473572..45887d9 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -887,11 +887,21 @@ static void apic_realize(DeviceState *dev, Error **errp)
     msi_nonbroken = true;
 }
 
+static void apic_unrealize(DeviceState *dev, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(dev);
+
+    timer_del(s->timer);
+    timer_free(s->timer);
+    local_apics[s->id] = NULL;
+}
+
 static void apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
     k->realize = apic_realize;
+    k->unrealize = apic_unrealize;
     k->set_base = apic_set_base;
     k->set_tpr = apic_set_tpr;
     k->get_tpr = apic_get_tpr;
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index fd425d1..0bb9ad5 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -315,6 +315,18 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
 }
 
+static void apic_common_unrealize(DeviceState *dev, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(dev);
+    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
+
+    info->unrealize(dev, errp);
+
+    if (apic_report_tpr_access && info->enable_tpr_reporting) {
+        info->enable_tpr_reporting(s, false);
+    }
+}
+
 static int apic_pre_load(void *opaque)
 {
     APICCommonState *s = APIC_COMMON(opaque);
@@ -421,6 +433,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
     dc->realize = apic_common_realize;
+    dc->unrealize = apic_common_unrealize;
     /*
      * Reason: APIC and CPU need to be wired up by
      * x86_cpu_apic_create()
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index e5d1550..52ca45d 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -135,6 +135,7 @@ typedef struct APICCommonClass
     DeviceClass parent_class;
 
     DeviceRealize realize;
+    DeviceUnrealize unrealize;
     void (*set_base)(APICCommonState *s, uint64_t val);
     void (*set_tpr)(APICCommonState *s, uint8_t val);
     uint8_t (*get_tpr)(APICCommonState *s);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 15/18] target-i386: do not ignore error and fix apic parent

[Igor Mammedov]

object_property_add_child() silently fails with error that it can't
create duplicate propery 'apic' as we already have 'apic' property
registered for AIPC ID. As result generic device_realize puts
apic as into unattached container.

As it's programming error, abort on it and fix property name for
apic_state to 'lapic', this way apic is a child of cpu instance.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ebf4140..04c0b79 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2763,8 +2763,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     cpu->apic_state = DEVICE(object_new(apic_type));
 
-    object_property_add_child(OBJECT(cpu), "apic",
-                              OBJECT(cpu->apic_state), NULL);
+    object_property_add_child(OBJECT(cpu), "lapic",
+                              OBJECT(cpu->apic_state), &error_abort);
+
     qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 15/18] target-i386: do not ignore error and fix apic parent

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:06:03PM +0200, Igor Mammedov wrote:
> object_property_add_child() silently fails with error that it can't
> create duplicate propery 'apic' as we already have 'apic' property

I thought that one was 'apic-id'?

> registered for AIPC ID.

APIC

> As result generic device_realize puts
> apic as into unattached container.
> 
> As it's programming error, abort on it and fix property name for
> apic_state to 'lapic', this way apic is a child of cpu instance.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ebf4140..04c0b79 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2763,8 +2763,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  
>      cpu->apic_state = DEVICE(object_new(apic_type));
>  
> -    object_property_add_child(OBJECT(cpu), "apic",
> -                              OBJECT(cpu->apic_state), NULL);
> +    object_property_add_child(OBJECT(cpu), "lapic",
> +                              OBJECT(cpu->apic_state), &error_abort);
> +
>      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(cpu->apic_state);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2 15/18] target-i386: do not ignore error and fix apic parent

[Igor Mammedov]

On Mon, 4 Jul 2016 17:20:59 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 06:06:03PM +0200, Igor Mammedov wrote:
> > object_property_add_child() silently fails with error that it can't
> > create duplicate propery 'apic' as we already have 'apic' property
> 
> I thought that one was 'apic-id'?
indeed, but with error_abort it prints:
  qemu-system-x86_64: attempt to add duplicate property 'apic' to
  object (type 'qemu64-x86_64-cpu')

I'll try to trace where it comes from.

> 
> > registered for AIPC ID.
> 
> APIC
> 
> > As result generic device_realize puts
> > apic as into unattached container.
> > 
> > As it's programming error, abort on it and fix property name for
> > apic_state to 'lapic', this way apic is a child of cpu instance.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ebf4140..04c0b79 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2763,8 +2763,9 @@ static void x86_cpu_apic_create(X86CPU *cpu,
> > Error **errp) 
> >      cpu->apic_state = DEVICE(object_new(apic_type));
> >  
> > -    object_property_add_child(OBJECT(cpu), "apic",
> > -                              OBJECT(cpu->apic_state), NULL);
> > +    object_property_add_child(OBJECT(cpu), "lapic",
> > +                              OBJECT(cpu->apic_state),
> > &error_abort); +
> >      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
> >      /* TODO: convert to link<> */
> >      apic = APIC_COMMON(cpu->apic_state);
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH v2 15/18] target-i386: do not ignore error and fix apic parent

[Igor Mammedov]

On Mon, 4 Jul 2016 18:39:21 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 4 Jul 2016 17:20:59 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jun 24, 2016 at 06:06:03PM +0200, Igor Mammedov wrote:
> > > object_property_add_child() silently fails with error that it
> > > can't create duplicate propery 'apic' as we already have 'apic'
> > > property
> > 
> > I thought that one was 'apic-id'?
> indeed, but with error_abort it prints:
>   qemu-system-x86_64: attempt to add duplicate property 'apic' to
>   object (type 'qemu64-x86_64-cpu')
> 
> I'll try to trace where it comes from.

We have cpu feature 'apic', so patch is correct, I'll fix commit
message that refers mistakingly to APIC ID
 
> 
> > 
> > > registered for AIPC ID.
> > 
> > APIC
> > 
> > > As result generic device_realize puts
> > > apic as into unattached container.
> > > 
> > > As it's programming error, abort on it and fix property name for
> > > apic_state to 'lapic', this way apic is a child of cpu instance.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index ebf4140..04c0b79 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -2763,8 +2763,9 @@ static void x86_cpu_apic_create(X86CPU *cpu,
> > > Error **errp) 
> > >      cpu->apic_state = DEVICE(object_new(apic_type));
> > >  
> > > -    object_property_add_child(OBJECT(cpu), "apic",
> > > -                              OBJECT(cpu->apic_state), NULL);
> > > +    object_property_add_child(OBJECT(cpu), "lapic",
> > > +                              OBJECT(cpu->apic_state),
> > > &error_abort); +
> > >      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
> > >      /* TODO: convert to link<> */
> > >      apic = APIC_COMMON(cpu->apic_state);
> > > -- 
> > > 1.8.3.1
> 
> 

[Qemu-devel] [PATCH v2 16/18] target-i386: fix apic object leak when CPU is deleted

[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 04c0b79..2fa445d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2765,6 +2765,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state), &error_abort);
+    object_unref(OBJECT(cpu->apic_state));
 
     qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 16/18] target-i386: fix apic object leak when CPU is deleted

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 06:06:04PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Can this bugfix be extracted from series?

> ---
>  target-i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 04c0b79..2fa445d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2765,6 +2765,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  
>      object_property_add_child(OBJECT(cpu), "lapic",
>                                OBJECT(cpu->apic_state), &error_abort);
> +    object_unref(OBJECT(cpu->apic_state));
>  
>      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
>      /* TODO: convert to link<> */
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2 16/18] target-i386: fix apic object leak when CPU is deleted

[Igor Mammedov]

On Mon, 4 Jul 2016 17:19:33 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 24, 2016 at 06:06:04PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Can this bugfix be extracted from series?
It's possible but there isn't much point as leak happens on CPU removal
and its harmful only with cpu-hotunplug which this series enables.
 
> > ---
> >  target-i386/cpu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 04c0b79..2fa445d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2765,6 +2765,7 @@ static void x86_cpu_apic_create(X86CPU *cpu,
> > Error **errp) 
> >      object_property_add_child(OBJECT(cpu), "lapic",
> >                                OBJECT(cpu->apic_state),
> > &error_abort);
> > +    object_unref(OBJECT(cpu->apic_state));
> >  
> >      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
> >      /* TODO: convert to link<> */
> > -- 
> > 1.8.3.1

[Qemu-devel] [PATCH v2 17/18] target-i386: add x86_cpu_unrealizefn()

[Igor Mammedov]

first remove VCPU from exec loop and only then remove lapic.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2fa445d..f86dae0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2963,6 +2963,20 @@ out:
     }
 }
 
+static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(dev);
+
+#ifndef CONFIG_USER_ONLY
+    cpu_remove_sync(CPU(dev));
+    qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
+#endif
+
+    if (cpu->apic_state) {
+        object_unparent(OBJECT(cpu->apic_state));
+    }
+}
+
 typedef struct BitProperty {
     uint32_t *ptr;
     uint32_t mask;
@@ -3205,6 +3219,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_realize = dc->realize;
     dc->realize = x86_cpu_realizefn;
+    dc->unrealize = x86_cpu_unrealizefn;
     dc->props = x86_cpu_properties;
 
     xcc->parent_reset = cc->reset;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 18/18] pc: make device_del CPU work for x86 CPUs

[Igor Mammedov]

ACPI subsystem already has all logic in place the only
thing left to eject CPU is destroy it and ammend
present CPUs counter in CMOS, do so.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f5124d5..296eebe 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1736,6 +1736,7 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
                              DeviceState *dev, Error **errp)
 {
+    CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
@@ -1747,13 +1748,11 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    /*
-     * TODO: enable unplug once generic CPU remove bits land
-     * for now guest will be able to eject CPU ACPI wise but
-     * it will come back again on machine reset.
-     */
-    /*  object_unparent(OBJECT(dev)); */
+    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
+    found_cpu->cpu = NULL;
+    object_unparent(OBJECT(dev));
 
+    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
  out:
     error_propagate(errp, local_err);
 }
-- 
1.8.3.1