Hi Igor,
On 09/09/2014 08:44 PM, Igor Mammedov wrote:
> On Thu, 28 Aug 2014 11:36:35 +0800
> Gu Zheng <guz.f...@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.f...@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.f...@cn.fujitsu.com>
>> Signed-off-by: Gu Zheng <guz.f...@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 */
>
> .
>
