Re: [Qemu-devel] [PATCH v3 6/7] cpu: add device_add foo-x86_64-cpu support

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 14 Jan 2015 15:27:29 +0800
Zhu Guihua <zhugh.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() 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.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@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 */