diff mbox

[RFC,V2,03/10] cpu: add device_add foo-x86_64-cpu support

Message ID 1409197002-9498-4-git-send-email-guz.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Gu Zheng Aug. 28, 2014, 3:36 a.m. UTC
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

Add support to device_add foo-x86_64-cpu, and additional checks of
apic id are added into x86_cpuid_set_apic_id() and x86_cpu_apic_create()
for duplicate. Besides, in order to support "device/device_add foo-x86_64-cpu"
which without specified apic id, we add a new function get_free_apic_id() to
provide the first free apid id each time to avoid apic id duplicate.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 include/qom/cpu.h      |    1 +
 qdev-monitor.c         |    1 +
 target-i386/cpu.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++-
 target-i386/topology.h |   18 ++++++++++++++
 4 files changed, 80 insertions(+), 1 deletions(-)

Comments

Igor Mammedov Sept. 9, 2014, 12:44 p.m. UTC | #1
On Thu, 28 Aug 2014 11:36:35 +0800
Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:

> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> 
> Add support to device_add foo-x86_64-cpu, and additional checks of
> apic id are added into x86_cpuid_set_apic_id() and x86_cpu_apic_create()
> for duplicate. Besides, in order to support "device/device_add foo-x86_64-cpu"
> which without specified apic id, we add a new function get_free_apic_id() to
> provide the first free apid id each time to avoid apic id duplicate.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  include/qom/cpu.h      |    1 +
>  qdev-monitor.c         |    1 +
>  target-i386/cpu.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++-
>  target-i386/topology.h |   18 ++++++++++++++
>  4 files changed, 80 insertions(+), 1 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index bc32c9a..2fc00ef 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -292,6 +292,7 @@ struct CPUState {
>  QTAILQ_HEAD(CPUTailQ, CPUState);
>  extern struct CPUTailQ cpus;
>  #define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
> +#define CPU_REMOVE(cpu) QTAILQ_REMOVE(&cpus, cpu, node)
>  #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
>  #define CPU_FOREACH_SAFE(cpu, next_cpu) \
>      QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index fb9ee24..1aa446d 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -24,6 +24,7 @@
>  #include "qmp-commands.h"
>  #include "sysemu/arch_init.h"
>  #include "qemu/config-file.h"
> +#include "qom/object_interfaces.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 2aa2b31..5255ddb 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -49,6 +49,7 @@
>  #include "hw/i386/apic_internal.h"
>  #endif
>  
> +#include "qom/object_interfaces.h"
This probably belongs to another patch.

>  
>  /* Cache topology CPUID constants: */
>  
> @@ -1634,6 +1635,7 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = UINT32_MAX;
>      Error *error = NULL;
>      int64_t value;
> +    X86CPUTopoInfo topo;
>  
>      if (dev->realized) {
>          error_setg(errp, "Attempt to set property '%s' on '%s' after "
> @@ -1653,6 +1655,19 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>  
> +    if (value > x86_cpu_apic_id_from_index(max_cpus - 1)) {
> +        error_setg(errp, "CPU with APIC ID %" PRIi64
> +                   " is more than MAX APIC ID limits", value);
> +        return;
> +    }
> +
> +    x86_topo_ids_from_apic_id(smp_cores, smp_threads, value, &topo);
> +    if (topo.smt_id >= smp_threads || topo.core_id >= smp_cores) {
> +        error_setg(errp, "CPU with APIC ID %" PRIi64 " does not match "
> +                   "topology configuration.", value);
> +        return;
> +    }
> +
>      if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
>          error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
>          return;
> @@ -2096,12 +2111,21 @@ out:
>      return cpu;
>  }
>  
> +static void x86_cpu_cpudef_instance_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    dev->hotplugged = true;
> +}
looks unnecessary, see device_initfn() which already does above.


> +
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUDefinition *cpudef = data;
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      xcc->cpu_def = cpudef;
> +    dc->cannot_instantiate_with_device_add_yet = false;
>  }
>  
>  static void x86_register_cpudef_type(X86CPUDefinition *def)
> @@ -2110,6 +2134,8 @@ static void x86_register_cpudef_type(X86CPUDefinition *def)
>      TypeInfo ti = {
>          .name = typename,
>          .parent = TYPE_X86_CPU,
> +        .instance_size = sizeof(X86CPU),
> +        .instance_init = x86_cpu_cpudef_instance_init,
this hunk is not needed, providing x86_cpu_cpudef_instance_init() is not necessary.

>          .class_init = x86_cpu_cpudef_class_init,
>          .class_data = def,
>      };
> @@ -2652,6 +2678,14 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>          return;
>      }
>  
> +    if (env->cpuid_apic_id > x86_cpu_apic_id_from_index(max_cpus - 1)) {
> +        error_setg(errp, "CPU with APIC ID %" PRIi32
> +                    " is more than MAX APIC ID:%" PRIi32,
> +                    env->cpuid_apic_id,
> +                    x86_cpu_apic_id_from_index(max_cpus - 1));
> +        return;
> +    }
use property apic-id here that has checks instead of duplicating them.

> +
>      object_property_add_child(OBJECT(cpu), "apic",
>                                OBJECT(cpu->apic_state), NULL);
>      qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
> @@ -2777,6 +2811,21 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
>      }
>  }
>  
> +static uint32_t get_free_apic_id(void)
> +{
> +    int i;
> +
> +    for (i = 0; i < max_cpus; i++) {
> +        uint32_t id = x86_cpu_apic_id_from_index(i);
> +
> +        if (!cpu_exists(id)) {
> +            return id;
> +        }
> +    }
> +
> +    return x86_cpu_apic_id_from_index(max_cpus);
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -2784,7 +2833,9 @@ static void x86_cpu_initfn(Object *obj)
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>      CPUX86State *env = &cpu->env;
>      static int inited;
> +    uint32_t value;
s/value/apic_id/ ???

>  
> +    value = get_free_apic_id();
What if there isn't any free APIC ID it the time it's called?

Could you just assign default broadcast value here 0xFFFFFFFF
and do actual APIC ID allocation at realize time if it still has
default value.

>      cs->env_ptr = env;
>      cpu_exec_init(env);
>  
> @@ -2823,7 +2874,7 @@ static void x86_cpu_initfn(Object *obj)
>                          NULL, NULL, (void *)cpu->filtered_features, NULL);
>  
>      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
> -    env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> +    env->cpuid_apic_id = value;
>  
>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
>  
> @@ -2837,6 +2888,13 @@ static void x86_cpu_initfn(Object *obj)
>      }
>  }
>  
> +static void x86_cpu_finalizefn(Object *obj)
> +{
> +    CPUState *cs = CPU(obj);
> +
> +    CPU_REMOVE(cs);
> +}
It might be better to split off device_del support into a separate patch.

>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> @@ -2937,6 +2995,7 @@ static const TypeInfo x86_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(X86CPU),
>      .instance_init = x86_cpu_initfn,
> +    .instance_finalize = x86_cpu_finalizefn,
>      .abstract = true,
>      .class_size = sizeof(X86CPUClass),
>      .class_init = x86_cpu_common_class_init,
> diff --git a/target-i386/topology.h b/target-i386/topology.h
> index e9ff89c..dcb4988 100644
> --- a/target-i386/topology.h
> +++ b/target-i386/topology.h
> @@ -132,4 +132,22 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
>      return apicid_from_topo_ids(nr_cores, nr_threads, &topo);
>  }
>  
> +/* Calculate CPU topology based on CPU APIC ID.
> + */
> +static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
> +                                             unsigned nr_threads,
> +                                             apic_id_t apic_id,
> +                                             X86CPUTopoInfo *topo)
> +{
> +    unsigned offset_mask;
> +    topo->pkg_id = apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
> +
> +    offset_mask = (1L << apicid_pkg_offset(nr_cores, nr_threads)) - 1;
> +    topo->core_id = (apic_id & offset_mask)
> +                     >> apicid_core_offset(nr_cores, nr_threads);
> +
> +    offset_mask = (1L << apicid_core_offset(nr_cores, nr_threads)) - 1;
> +    topo->smt_id = apic_id & offset_mask;
> +}
> +
>  #endif /* TARGET_I386_TOPOLOGY_H */
Gu Zheng Sept. 10, 2014, 3:37 a.m. UTC | #2
Hi Igor,
On 09/09/2014 08:44 PM, Igor Mammedov wrote:

> On Thu, 28 Aug 2014 11:36:35 +0800
> Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> 
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> Add support to device_add foo-x86_64-cpu, and additional checks of
>> apic id are added into x86_cpuid_set_apic_id() and x86_cpu_apic_create()
>> for duplicate. Besides, in order to support "device/device_add foo-x86_64-cpu"
>> which without specified apic id, we add a new function get_free_apic_id() to
>> provide the first free apid id each time to avoid apic id duplicate.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  include/qom/cpu.h      |    1 +
>>  qdev-monitor.c         |    1 +
>>  target-i386/cpu.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  target-i386/topology.h |   18 ++++++++++++++
>>  4 files changed, 80 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index bc32c9a..2fc00ef 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -292,6 +292,7 @@ struct CPUState {
>>  QTAILQ_HEAD(CPUTailQ, CPUState);
>>  extern struct CPUTailQ cpus;
>>  #define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
>> +#define CPU_REMOVE(cpu) QTAILQ_REMOVE(&cpus, cpu, node)
>>  #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
>>  #define CPU_FOREACH_SAFE(cpu, next_cpu) \
>>      QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index fb9ee24..1aa446d 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -24,6 +24,7 @@
>>  #include "qmp-commands.h"
>>  #include "sysemu/arch_init.h"
>>  #include "qemu/config-file.h"
>> +#include "qom/object_interfaces.h"
>>  
>>  /*
>>   * Aliases were a bad idea from the start.  Let's keep them
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 2aa2b31..5255ddb 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -49,6 +49,7 @@
>>  #include "hw/i386/apic_internal.h"
>>  #endif
>>  
>> +#include "qom/object_interfaces.h"
> This probably belongs to another patch.

Yes, It's a typo here.

> 
>>  
>>  /* Cache topology CPUID constants: */
>>  
>> @@ -1634,6 +1635,7 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>>      const int64_t max = UINT32_MAX;
>>      Error *error = NULL;
>>      int64_t value;
>> +    X86CPUTopoInfo topo;
>>  
>>      if (dev->realized) {
>>          error_setg(errp, "Attempt to set property '%s' on '%s' after "
>> @@ -1653,6 +1655,19 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>>          return;
>>      }
>>  
>> +    if (value > x86_cpu_apic_id_from_index(max_cpus - 1)) {
>> +        error_setg(errp, "CPU with APIC ID %" PRIi64
>> +                   " is more than MAX APIC ID limits", value);
>> +        return;
>> +    }
>> +
>> +    x86_topo_ids_from_apic_id(smp_cores, smp_threads, value, &topo);
>> +    if (topo.smt_id >= smp_threads || topo.core_id >= smp_cores) {
>> +        error_setg(errp, "CPU with APIC ID %" PRIi64 " does not match "
>> +                   "topology configuration.", value);
>> +        return;
>> +    }
>> +
>>      if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
>>          error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
>>          return;
>> @@ -2096,12 +2111,21 @@ out:
>>      return cpu;
>>  }
>>  
>> +static void x86_cpu_cpudef_instance_init(Object *obj)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +
>> +    dev->hotplugged = true;
>> +}
> looks unnecessary, see device_initfn() which already does above.

You are right, we do not need the x86_cpu_cpudef_instance_init, device_initfn
will do the job for us.

> 
> 
>> +
>>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>>  {
>>      X86CPUDefinition *cpudef = data;
>>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>  
>>      xcc->cpu_def = cpudef;
>> +    dc->cannot_instantiate_with_device_add_yet = false;
>>  }
>>  
>>  static void x86_register_cpudef_type(X86CPUDefinition *def)
>> @@ -2110,6 +2134,8 @@ static void x86_register_cpudef_type(X86CPUDefinition *def)
>>      TypeInfo ti = {
>>          .name = typename,
>>          .parent = TYPE_X86_CPU,
>> +        .instance_size = sizeof(X86CPU),
>> +        .instance_init = x86_cpu_cpudef_instance_init,
> this hunk is not needed, providing x86_cpu_cpudef_instance_init() is not necessary.
> 
>>          .class_init = x86_cpu_cpudef_class_init,
>>          .class_data = def,
>>      };
>> @@ -2652,6 +2678,14 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>>          return;
>>      }
>>  
>> +    if (env->cpuid_apic_id > x86_cpu_apic_id_from_index(max_cpus - 1)) {
>> +        error_setg(errp, "CPU with APIC ID %" PRIi32
>> +                    " is more than MAX APIC ID:%" PRIi32,
>> +                    env->cpuid_apic_id,
>> +                    x86_cpu_apic_id_from_index(max_cpus - 1));
>> +        return;
>> +    }
> use property apic-id here that has checks instead of duplicating them.

Reasonable, I will fix it.

> 
>> +
>>      object_property_add_child(OBJECT(cpu), "apic",
>>                                OBJECT(cpu->apic_state), NULL);
>>      qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
>> @@ -2777,6 +2811,21 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
>>      }
>>  }
>>  
>> +static uint32_t get_free_apic_id(void)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < max_cpus; i++) {
>> +        uint32_t id = x86_cpu_apic_id_from_index(i);
>> +
>> +        if (!cpu_exists(id)) {
>> +            return id;
>> +        }
>> +    }
>> +
>> +    return x86_cpu_apic_id_from_index(max_cpus);
>> +}
>> +
>>  static void x86_cpu_initfn(Object *obj)
>>  {
>>      CPUState *cs = CPU(obj);
>> @@ -2784,7 +2833,9 @@ static void x86_cpu_initfn(Object *obj)
>>      X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>>      CPUX86State *env = &cpu->env;
>>      static int inited;
>> +    uint32_t value;
> s/value/apic_id/ ???
> 
>>  
>> +    value = get_free_apic_id();
> What if there isn't any free APIC ID it the time it's called?

It will return a invalid id (x86_cpu_apic_id_from_index(max_cpus)),
and let the following check rejects it.

> 
> Could you just assign default broadcast value here 0xFFFFFFFF
> and do actual APIC ID allocation at realize time if it still has
> default value.

Sounds reasonable, will try this way.

> 
>>      cs->env_ptr = env;
>>      cpu_exec_init(env);
>>  
>> @@ -2823,7 +2874,7 @@ static void x86_cpu_initfn(Object *obj)
>>                          NULL, NULL, (void *)cpu->filtered_features, NULL);
>>  
>>      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
>> -    env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>> +    env->cpuid_apic_id = value;
>>  
>>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
>>  
>> @@ -2837,6 +2888,13 @@ static void x86_cpu_initfn(Object *obj)
>>      }
>>  }
>>  
>> +static void x86_cpu_finalizefn(Object *obj)
>> +{
>> +    CPUState *cs = CPU(obj);
>> +
>> +    CPU_REMOVE(cs);
>> +}
> It might be better to split off device_del support into a separate patch.

Yes, It should be moved to "device_del" part.

Thanks,
Gu

> 
>>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
>>  {
>>      X86CPU *cpu = X86_CPU(cs);
>> @@ -2937,6 +2995,7 @@ static const TypeInfo x86_cpu_type_info = {
>>      .parent = TYPE_CPU,
>>      .instance_size = sizeof(X86CPU),
>>      .instance_init = x86_cpu_initfn,
>> +    .instance_finalize = x86_cpu_finalizefn,
>>      .abstract = true,
>>      .class_size = sizeof(X86CPUClass),
>>      .class_init = x86_cpu_common_class_init,
>> diff --git a/target-i386/topology.h b/target-i386/topology.h
>> index e9ff89c..dcb4988 100644
>> --- a/target-i386/topology.h
>> +++ b/target-i386/topology.h
>> @@ -132,4 +132,22 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
>>      return apicid_from_topo_ids(nr_cores, nr_threads, &topo);
>>  }
>>  
>> +/* Calculate CPU topology based on CPU APIC ID.
>> + */
>> +static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
>> +                                             unsigned nr_threads,
>> +                                             apic_id_t apic_id,
>> +                                             X86CPUTopoInfo *topo)
>> +{
>> +    unsigned offset_mask;
>> +    topo->pkg_id = apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
>> +
>> +    offset_mask = (1L << apicid_pkg_offset(nr_cores, nr_threads)) - 1;
>> +    topo->core_id = (apic_id & offset_mask)
>> +                     >> apicid_core_offset(nr_cores, nr_threads);
>> +
>> +    offset_mask = (1L << apicid_core_offset(nr_cores, nr_threads)) - 1;
>> +    topo->smt_id = apic_id & offset_mask;
>> +}
>> +
>>  #endif /* TARGET_I386_TOPOLOGY_H */
> 
> .
>
diff mbox

Patch

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index bc32c9a..2fc00ef 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -292,6 +292,7 @@  struct CPUState {
 QTAILQ_HEAD(CPUTailQ, CPUState);
 extern struct CPUTailQ cpus;
 #define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
+#define CPU_REMOVE(cpu) QTAILQ_REMOVE(&cpus, cpu, node)
 #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
     QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index fb9ee24..1aa446d 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -24,6 +24,7 @@ 
 #include "qmp-commands.h"
 #include "sysemu/arch_init.h"
 #include "qemu/config-file.h"
+#include "qom/object_interfaces.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2aa2b31..5255ddb 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -49,6 +49,7 @@ 
 #include "hw/i386/apic_internal.h"
 #endif
 
+#include "qom/object_interfaces.h"
 
 /* Cache topology CPUID constants: */
 
@@ -1634,6 +1635,7 @@  static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
     const int64_t max = UINT32_MAX;
     Error *error = NULL;
     int64_t value;
+    X86CPUTopoInfo topo;
 
     if (dev->realized) {
         error_setg(errp, "Attempt to set property '%s' on '%s' after "
@@ -1653,6 +1655,19 @@  static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
+    if (value > x86_cpu_apic_id_from_index(max_cpus - 1)) {
+        error_setg(errp, "CPU with APIC ID %" PRIi64
+                   " is more than MAX APIC ID limits", value);
+        return;
+    }
+
+    x86_topo_ids_from_apic_id(smp_cores, smp_threads, value, &topo);
+    if (topo.smt_id >= smp_threads || topo.core_id >= smp_cores) {
+        error_setg(errp, "CPU with APIC ID %" PRIi64 " does not match "
+                   "topology configuration.", value);
+        return;
+    }
+
     if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
         error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
         return;
@@ -2096,12 +2111,21 @@  out:
     return cpu;
 }
 
+static void x86_cpu_cpudef_instance_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    dev->hotplugged = true;
+}
+
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
 {
     X86CPUDefinition *cpudef = data;
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     xcc->cpu_def = cpudef;
+    dc->cannot_instantiate_with_device_add_yet = false;
 }
 
 static void x86_register_cpudef_type(X86CPUDefinition *def)
@@ -2110,6 +2134,8 @@  static void x86_register_cpudef_type(X86CPUDefinition *def)
     TypeInfo ti = {
         .name = typename,
         .parent = TYPE_X86_CPU,
+        .instance_size = sizeof(X86CPU),
+        .instance_init = x86_cpu_cpudef_instance_init,
         .class_init = x86_cpu_cpudef_class_init,
         .class_data = def,
     };
@@ -2652,6 +2678,14 @@  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         return;
     }
 
+    if (env->cpuid_apic_id > x86_cpu_apic_id_from_index(max_cpus - 1)) {
+        error_setg(errp, "CPU with APIC ID %" PRIi32
+                    " is more than MAX APIC ID:%" PRIi32,
+                    env->cpuid_apic_id,
+                    x86_cpu_apic_id_from_index(max_cpus - 1));
+        return;
+    }
+
     object_property_add_child(OBJECT(cpu), "apic",
                               OBJECT(cpu->apic_state), NULL);
     qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
@@ -2777,6 +2811,21 @@  uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
+static uint32_t get_free_apic_id(void)
+{
+    int i;
+
+    for (i = 0; i < max_cpus; i++) {
+        uint32_t id = x86_cpu_apic_id_from_index(i);
+
+        if (!cpu_exists(id)) {
+            return id;
+        }
+    }
+
+    return x86_cpu_apic_id_from_index(max_cpus);
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -2784,7 +2833,9 @@  static void x86_cpu_initfn(Object *obj)
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
     static int inited;
+    uint32_t value;
 
+    value = get_free_apic_id();
     cs->env_ptr = env;
     cpu_exec_init(env);
 
@@ -2823,7 +2874,7 @@  static void x86_cpu_initfn(Object *obj)
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-    env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
+    env->cpuid_apic_id = value;
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
@@ -2837,6 +2888,13 @@  static void x86_cpu_initfn(Object *obj)
     }
 }
 
+static void x86_cpu_finalizefn(Object *obj)
+{
+    CPUState *cs = CPU(obj);
+
+    CPU_REMOVE(cs);
+}
+
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
@@ -2937,6 +2995,7 @@  static const TypeInfo x86_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
     .instance_init = x86_cpu_initfn,
+    .instance_finalize = x86_cpu_finalizefn,
     .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
diff --git a/target-i386/topology.h b/target-i386/topology.h
index e9ff89c..dcb4988 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -132,4 +132,22 @@  static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
     return apicid_from_topo_ids(nr_cores, nr_threads, &topo);
 }
 
+/* Calculate CPU topology based on CPU APIC ID.
+ */
+static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
+                                             unsigned nr_threads,
+                                             apic_id_t apic_id,
+                                             X86CPUTopoInfo *topo)
+{
+    unsigned offset_mask;
+    topo->pkg_id = apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
+
+    offset_mask = (1L << apicid_pkg_offset(nr_cores, nr_threads)) - 1;
+    topo->core_id = (apic_id & offset_mask)
+                     >> apicid_core_offset(nr_cores, nr_threads);
+
+    offset_mask = (1L << apicid_core_offset(nr_cores, nr_threads)) - 1;
+    topo->smt_id = apic_id & offset_mask;
+}
+
 #endif /* TARGET_I386_TOPOLOGY_H */