[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu suppo
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu support |
Date: |
Thu, 29 Jan 2015 15:46:03 +0100 |
On Wed, 14 Jan 2015 15:27:29 +0800
Zhu Guihua <address@hidden> wrote:
> From: Chen Fan <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Gu Zheng <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
> 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 */
- [Qemu-devel] [PATCH v3 3/7] qom/cpu: move apic vmstate register into x86_cpu_apic_realize, (continued)
- [Qemu-devel] [PATCH v3 3/7] qom/cpu: move apic vmstate register into x86_cpu_apic_realize, Zhu Guihua, 2015/01/14
- [Qemu-devel] [PATCH v3 5/7] acpi:cpu hotplug: set pcmachine as icc bus' hotplug handler, Zhu Guihua, 2015/01/14
- [Qemu-devel] [PATCH v3 4/7] monitor: use cc->get_arch_id as the cpu index, Zhu Guihua, 2015/01/14
- [Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn, Zhu Guihua, 2015/01/14
- [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu support, Zhu Guihua, 2015/01/14
- Re: [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu support,
Igor Mammedov <=
- [Qemu-devel] [PATCH v3 7/7] i386/cpu: add instance finalize callback, Zhu Guihua, 2015/01/14