Re: [Qemu-devel] [PATCH v4 09/10] cpu: add device_add foo-x86_64-cpu support

[Eduardo Habkost]

On Fri, Feb 13, 2015 at 06:25:32PM +0800, Zhu Guihua 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 |  6 ++++--
>  hw/i386/pc.c          |  6 ------
>  target-i386/cpu.c     | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index b8ebfad..8e4ed6e 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -59,8 +59,10 @@ 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) {
> +        acpi_send_gpe_event(ar, irq, ACPI_CPU_HOTPLUG_STATUS);
> +    }
>  }
>  
>  void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 500d369..1187e12 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1637,13 +1637,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> -    if (!dev->hotplugged) {
> -        goto out;
> -    }
> -
>      if (!pcms->acpi_dev) {
> -        error_setg(&local_err,
> -                   "cpu hotplug is not enabled: missing acpi device");
>          goto out;
>      }
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 028063c..68a6aa4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1703,6 +1703,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 "
> @@ -1722,6 +1723,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;
> @@ -2166,8 +2180,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)
> @@ -2176,6 +2192,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,
>      };
> @@ -2709,11 +2726,28 @@ 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)

This is inside CONFIG_USER_ONLY...

> +
[...]
> @@ -2920,7 +2962,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;


...but this is not.


    CC    x86_64-linux-user/target-i386/cpu.o
  /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c: In function 
‘x86_cpu_initfn’:
  /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:2964:26: error: 
‘APIC_ID_NOT_SET’ undeclared (first use in this function)
       env->cpuid_apic_id = APIC_ID_NOT_SET;
                            ^
  /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:2964:26: note: each 
undeclared identifier is reported only once for each function it appears in
  /home/ehabkost/rh/proj/virt/qemu/rules.mak:57: recipe for target 
'target-i386/cpu.o' failed
  make[1]: *** [target-i386/cpu.o] Error 1
  Makefile:169: recipe for target 'subdir-x86_64-linux-user' failed
  make: *** [subdir-x86_64-linux-user] Error 2

Also, this will need to be rebased on top of the APIC ID cleanups from
my x86 tree, located at:
  https://github.com/ehabkost/qemu.git x86

>  
>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
>  
> -- 
> 1.9.3
> 
> 

-- 
Eduardo