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

[Gu Zheng <guz.fnst@cn.fujitsu.com>]

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