[v3,6/7] cpu: add device_add foo-x86_64-cpu support

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(-)

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 */

On Thu, Jan 29, 2015 at 03:46:03PM +0100, Igor Mammedov wrote:
[...]
> > 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?

APIC ID doesn't encode NUMA node information, just the socket, core, and
thread IDs.

If I understand it coreectly, the check above is to ensure we are within
the limits configured in the command-line. There's a cores-per-socket
and threads-per-core option, but not an explicit limit for the number of
sockets. The limit for the number of sockets is implicit (it depends on
max_cpus), and we are already checking the value against max_cpus above.

This is related to a previous discussion about the semantics of the
"sockets" option. I always assumed the "sockets" option was about
non-hotplug VCPUs (smp_cpus = threads * cores * sockets) but Drew
recently sent some patches assuming the "sockets" option should include
sockets for CPU hotplug as well (max_cpus = threads * cores * sockets),
and it may make more sense. In either case, we need to define and
document those semantics more clearly to avoid confusion.

> 
> > +        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;
[...]
> > +#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?

I don't believe the QOM property model has any abstraction for unset
properties.

> 
> > +        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);

Am 29.01.2015 um 17:01 schrieb Eduardo Habkost:
> On Thu, Jan 29, 2015 at 03:46:03PM +0100, Igor Mammedov wrote:
> [...]
>>> @@ -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?
> 
> I don't believe the QOM property model has any abstraction for unset
> properties.

Correct. The only way I can think of is turning it into a custom
"dynamic" property, which lets you set some flag in the setter. Using a
custom implementation for a static property might also be an option.

But as a general reminder, this series does not seem to address some of
the modeling considerations I had brought up, so I am again prioritizing
work on an RFC for a cross-target QOM topology abstraction (me and
Eduardo each had some early x86 patches to that effect iirc) and am
still considering this v3 more of an RFC destined at testing hot-unplug
on top, which will then be rebased on whatever structure we agree on.

Regards,
Andreas

On 01/30/2015 12:39 AM, Andreas Färber wrote:
> Am 29.01.2015 um 17:01 schrieb Eduardo Habkost:
>> On Thu, Jan 29, 2015 at 03:46:03PM +0100, Igor Mammedov wrote:
>> [...]
>>>> @@ -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?
>> I don't believe the QOM property model has any abstraction for unset
>> properties.
> Correct. The only way I can think of is turning it into a custom
> "dynamic" property, which lets you set some flag in the setter. Using a
> custom implementation for a static property might also be an option.
>
> But as a general reminder, this series does not seem to address some of
> the modeling considerations I had brought up, so I am again prioritizing
> work on an RFC for a cross-target QOM topology abstraction (me and
> Eduardo each had some early x86 patches to that effect iirc) and am
> still considering this v3 more of an RFC destined at testing hot-unplug
> on top, which will then be rebased on whatever structure we agree on.
Hi all,

For migration and arbitrary CPU add/rm, I have an idea to transmit
the source topology to dest for migration. for that we don't need to
care the topology problem on destination. we can add structure to
describe topology when parse smp. the topology show the cpu
details present:

                                  topology
                                         |
    |                                    |                         |
socket0                      socket1              socket2
    |  |                                ...                      ...
    |  core_sibling
core0
    |  |
    |  thread_sibling
thread0
    |
cpu_present

1) each time we create one CPU we update the cpu_present, and
when remove cpu, we clean the present bit.

2) when migration, we pass the cpu_present to destination via a
new vmsd. then from the tree hierarchy we would be aware of the
existed apic-id. so on destination loading the new calculated
apic-id to cpu object. one thing need to do I think is that we must
rebuild the srat table for preboot cpus before resume all cpu
on destination.

Thanks,
Chen


>
> Regards,
> Andreas
>