On Wed, 14 Jan 2015 15:27:29 +0800 Zhu Guihua <zhugh.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() to avoid duplicate. > Besides, in order to support "device/device_add foo-x86_64-cpu" > which without specified apic id, we assign cpuid_apic_id with a > default broadcast value (0xFFFFFFFF) in initfn, and a new function > get_free_apic_id() to provide a free apid id to cpuid_apic_id if > it still has the default at realize time (e.g. hot add foo-cpu without > a specified apic id) to avoid apic id duplicates. > > Thanks very much for Igor's suggestion. > > Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com> > Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com> > Signed-off-by: Zhu Guihua <zhugh.f...@cn.fujitsu.com> > --- > hw/acpi/cpu_hotplug.c | 7 +++++-- > hw/i386/pc.c | 6 ------ > target-i386/cpu.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--- > target-i386/topology.h | 18 ++++++++++++++++++ > 4 files changed, 67 insertions(+), 11 deletions(-) > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c > index b8ebfad..4047294 100644 > --- a/hw/acpi/cpu_hotplug.c > +++ b/hw/acpi/cpu_hotplug.c > @@ -59,8 +59,11 @@ void acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, > return; > } > > - ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS; > - acpi_update_sci(ar, irq); > + /* Only trigger sci if cpu is hotplugged */ > + if (dev->hotplugged) { > + ar->gpe.sts[0] |= ACPI_CPU_HOTPLUG_STATUS; > + acpi_update_sci(ar, irq); > + } > } separate patch? > > void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index e07f1fa..006f355 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1678,13 +1678,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev, > Error *local_err = NULL; > PCMachineState *pcms = PC_MACHINE(hotplug_dev); > > - if (!dev->hotplugged) { > - goto out; > - } Now, cold-plugged CPUs would be wired by this function too, but what about rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); we are doing in this function later, for cold-plugged CPUs it was done somewhere else, but I don't see you handling it in this patch. > - > if (!pcms->acpi_dev) { > - error_setg(&local_err, > - "cpu hotplug is not enabled: missing acpi device"); Also coldplugged CPU should work and without ACPI, maybe merge with above condition? > goto out; > } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 3406fe8..4347948 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1705,6 +1705,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 " > @@ -1724,6 +1725,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) { If I recall correctly, APIC id also encodes socket and numa node it belongs to, so why it's not checked here? > + 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; > @@ -2178,8 +2192,10 @@ 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) > @@ -2188,6 +2204,7 @@ static void x86_register_cpudef_type(X86CPUDefinition > *def) > TypeInfo ti = { > .name = typename, > .parent = TYPE_X86_CPU, > + .instance_size = sizeof(X86CPU), > .class_init = x86_cpu_cpudef_class_init, > .class_data = def, > }; > @@ -2721,12 +2738,29 @@ static void mce_init(X86CPU *cpu) > } > > #ifndef CONFIG_USER_ONLY > +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); > +} > + > +#define APIC_ID_NOT_SET (~0U) > + > static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > { > - CPUX86State *env = &cpu->env; > DeviceState *dev = DEVICE(cpu); > APICCommonState *apic; > const char *apic_type = "apic"; > + uint32_t apic_id; > > if (kvm_irqchip_in_kernel()) { > apic_type = "kvm-apic"; > @@ -2742,7 +2776,14 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error > **errp) > > object_property_add_child(OBJECT(cpu), "apic", > OBJECT(cpu->apic_state), NULL); > - qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id); > + > + apic_id = object_property_get_int(OBJECT(cpu), "apic-id", NULL); > + if (apic_id == APIC_ID_NOT_SET) { Do we have in QOM a way to check if property was ever set? > + apic_id = get_free_apic_id(); > + object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp); > + } > + > + qdev_prop_set_uint8(cpu->apic_state, "id", apic_id); > /* TODO: convert to link<> */ > apic = APIC_COMMON(cpu->apic_state); > apic->cpu = cpu; > @@ -2931,7 +2972,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 = APIC_ID_NOT_SET; > > x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); > > 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 */