Re: [Qemu-devel] [RFC V2 03/10] cpu: add device_add foo-x86_64-cpu support

[Igor Mammedov <imammedo@redhat.com>]

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