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

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

[Zhu Guihua]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2662 bytes --]

This series is based on the previous patchset from Chen Fan:
https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html

We try to make cpu hotplug with device_add, and make
"-device foo-x86_64-cpu" available，also we can set apic-id
property with command line, if without setting apic-id property,
we offer the first unoccupied apic id as the default new apic id.
When hotplug cpu with device_add, additional check of APIC ID will be
done after cpu object initialization which was different from
'cpu_add' command that check 'ids' at the beginning.

The is the first half of the previous series:
[RFC V2 00/10] cpu: add device_add foo-x86_64-cpu and i386 cpu hot remove support
https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg04779.html

---
Changelog since v2:
 -rebase on latest upstream.
 -add cpu instance finalize.

Changelog since v1:
 -rebased on latest upstream.
 -introduce a help function to hide the access to icc_bus.
 -use a macro ACPI_ID_NOT_SET to replace the magic number(0xFFFFFFFF).

Changelog since RFC:
 -split out APIC vmstate/QMP-monitor changes into separate patches.
 -add the handle of the startup cpus(-device foo).
 -remove duplicated checking about env->cpuid_apic_id.
 -do actual APIC ID allocation at realize time if it is not set before.
 -remove the unneeded x86_cpu_cpudef_instance_init().
 -split off device_del support out here.
---

Chen Fan (2):
  cpu: introduce CpuTopoInfo structure for argument simplification
  cpu: add device_add foo-x86_64-cpu support

Gu Zheng (5):
  qom/cpu: move register_vmstate to common CPUClass.realizefn
  qom/cpu: move apic vmstate register into x86_cpu_apic_realize
  monitor: use cc->get_arch_id as the cpu index
  acpi:cpu hotplug: set pcmachine as icc bus' hotplug handler
  i386/cpu: add instance finalize callback

 cpus.c                          |  4 ++-
 exec.c                          | 32 ++++++++++++---------
 hw/acpi/cpu_hotplug.c           |  7 +++--
 hw/cpu/icc_bus.c                |  5 ++++
 hw/i386/pc.c                    |  6 ----
 hw/i386/pc_piix.c               |  5 ++++
 hw/i386/pc_q35.c                |  5 ++++
 hw/intc/apic_common.c           |  3 +-
 include/hw/cpu/icc_bus.h        |  2 ++
 include/hw/i386/apic_internal.h |  3 ++
 include/qom/cpu.h               |  3 ++
 monitor.c                       |  4 ++-
 qom/cpu.c                       |  2 ++
 target-i386/cpu.c               | 63 ++++++++++++++++++++++++++++++++++++++---
 target-i386/topology.h          | 51 ++++++++++++++++++++++-----------
 15 files changed, 150 insertions(+), 45 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/7] cpu: introduce CpuTopoInfo structure for argument simplification

[Zhu Guihua]

From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
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>
---
 target-i386/topology.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..e9ff89c 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -47,6 +47,12 @@
  */
 typedef uint32_t apic_id_t;
 
+typedef struct X86CPUTopoInfo {
+    unsigned pkg_id;
+    unsigned core_id;
+    unsigned smt_id;
+} X86CPUTopoInfo;
+
 /* Return the bit width needed for 'count' IDs
  */
 static unsigned apicid_bitwidth_for_count(unsigned count)
@@ -92,13 +98,11 @@ static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
  */
 static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
                                              unsigned nr_threads,
-                                             unsigned pkg_id,
-                                             unsigned core_id,
-                                             unsigned smt_id)
+                                             const X86CPUTopoInfo *topo)
 {
-    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
-           (core_id << apicid_core_offset(nr_cores, nr_threads)) |
-           smt_id;
+    return (topo->pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
+           (topo->core_id << apicid_core_offset(nr_cores, nr_threads)) |
+           topo->smt_id;
 }
 
 /* Calculate thread/core/package IDs for a specific topology,
@@ -107,14 +111,12 @@ static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
 static inline void x86_topo_ids_from_idx(unsigned nr_cores,
                                          unsigned nr_threads,
                                          unsigned cpu_index,
-                                         unsigned *pkg_id,
-                                         unsigned *core_id,
-                                         unsigned *smt_id)
+                                         X86CPUTopoInfo *topo)
 {
     unsigned core_index = cpu_index / nr_threads;
-    *smt_id = cpu_index % nr_threads;
-    *core_id = core_index % nr_cores;
-    *pkg_id = core_index / nr_cores;
+    topo->smt_id = cpu_index % nr_threads;
+    topo->core_id = core_index % nr_cores;
+    topo->pkg_id = core_index / nr_cores;
 }
 
 /* Make APIC ID for the CPU 'cpu_index'
@@ -125,10 +127,9 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
                                                 unsigned nr_threads,
                                                 unsigned cpu_index)
 {
-    unsigned pkg_id, core_id, smt_id;
-    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
-                          &pkg_id, &core_id, &smt_id);
-    return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+    X86CPUTopoInfo topo;
+    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index, &topo);
+    return apicid_from_topo_ids(nr_cores, nr_threads, &topo);
 }
 
 #endif /* TARGET_I386_TOPOLOGY_H */
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn

[Zhu Guihua]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

Move cpu vmstate register from cpu_exec_init into cpu_common_realizefn,
and use cc->get_arch_id as the instance id that suggested by Igor to
fix the migration issue.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 exec.c            | 32 +++++++++++++++++++-------------
 include/qom/cpu.h |  2 ++
 qom/cpu.c         |  2 ++
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index 081818e..c9ffda6 100644
--- a/exec.c
+++ b/exec.c
@@ -513,10 +513,28 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
+void cpu_vmstate_register(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    int cpu_index = cc->get_arch_id(cpu);
+
+    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
+    }
+#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
+    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
+                    cpu_save, cpu_load, cpu->env_ptr);
+    assert(cc->vmsd == NULL);
+    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
+#endif
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
+    }
+}
+
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUState *some_cpu;
     int cpu_index;
 
@@ -539,18 +557,6 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
-    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
-    }
-#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
-    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
-                    cpu_save, cpu_load, env);
-    assert(cc->vmsd == NULL);
-    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
-#endif
-    if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
-    }
 }
 
 #if defined(TARGET_HAS_ICE)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 2098f1c..936afcd 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -562,6 +562,8 @@ void cpu_interrupt(CPUState *cpu, int mask);
 
 #endif /* USER_ONLY */
 
+void cpu_vmstate_register(CPUState *cpu);
+
 #ifdef CONFIG_SOFTMMU
 static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
                                          bool is_write, bool is_exec,
diff --git a/qom/cpu.c b/qom/cpu.c
index 9c68fa4..a639822 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -302,6 +302,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
 
+    cpu_vmstate_register(cpu);
+
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
         cpu_resume(cpu);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn

[Igor Mammedov]

On Wed, 14 Jan 2015 15:27:25 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> Move cpu vmstate register from cpu_exec_init into cpu_common_realizefn,
> and use cc->get_arch_id as the instance id that suggested by Igor to
> fix the migration issue.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  exec.c            | 32 +++++++++++++++++++-------------
>  include/qom/cpu.h |  2 ++
>  qom/cpu.c         |  2 ++
>  3 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 081818e..c9ffda6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -513,10 +513,28 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>  }
>  #endif
>  
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int cpu_index = cc->get_arch_id(cpu);
that probable would be source migration problems:
because cc->get_arch_id(cpu) depending on topology might
be not sequential, for example: sockets=4,core=3
that would create sparse APIC numbering.

as result migration from old qemu to one with this change
would be rejected due to vmsd id mismatch mismatch.

we need a better way to handle cross version migration
between old/new scheme.

> +
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
> +    }
> +#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
that ifdef block affects only sparc/mips/cris and builds target specific code
while you are trying to call it from target independent qom/cpu.c

I'd suggest leave it where it was or better move into respective
targets realize_fns

> +    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
> +                    cpu_save, cpu_load, cpu->env_ptr);
> +    assert(cc->vmsd == NULL);
> +    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
> +#endif
> +    if (cc->vmsd != NULL) {
> +        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
> +    }
> +}
> +
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUState *some_cpu;
>      int cpu_index;
>  
> @@ -539,18 +557,6 @@ void cpu_exec_init(CPUArchState *env)
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
>  #endif
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
> -    }
> -#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> -    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
> -                    cpu_save, cpu_load, env);
> -    assert(cc->vmsd == NULL);
> -    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
> -#endif
> -    if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
> -    }
And in general do CONFIG_USER_ONLY targets actually need/use
migration code?

>  }
>  
>  #if defined(TARGET_HAS_ICE)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 2098f1c..936afcd 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -562,6 +562,8 @@ void cpu_interrupt(CPUState *cpu, int mask);
>  
>  #endif /* USER_ONLY */
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +
>  #ifdef CONFIG_SOFTMMU
>  static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
>                                           bool is_write, bool is_exec,
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 9c68fa4..a639822 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -302,6 +302,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
> +
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);

Re: [Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn

[Chen Fan]

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]


On 01/29/2015 10:04 PM, Igor Mammedov wrote:
> On Wed, 14 Jan 2015 15:27:25 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>
>> Move cpu vmstate register from cpu_exec_init into cpu_common_realizefn,
>> and use cc->get_arch_id as the instance id that suggested by Igor to
>> fix the migration issue.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> ---
>>   exec.c            | 32 +++++++++++++++++++-------------
>>   include/qom/cpu.h |  2 ++
>>   qom/cpu.c         |  2 ++
>>   3 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 081818e..c9ffda6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -513,10 +513,28 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>>   }
>>   #endif
>>   
>> +void cpu_vmstate_register(CPUState *cpu)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +    int cpu_index = cc->get_arch_id(cpu);
> that probable would be source migration problems:
> because cc->get_arch_id(cpu) depending on topology might
> be not sequential, for example: sockets=4,core=3
> that would create sparse APIC numbering.
>
> as result migration from old qemu to one with this change
> would be rejected due to vmsd id mismatch mismatch.
>
> we need a better way to handle cross version migration
> between old/new scheme.
Hi Igor,

     I think to handler cross version migration issue, we only
need to do is converting new 'apic-id' to old 'cpu_index' to match
vmsd id  between old to new scheme.
we can save old cpu_index in alias id. and in order to keep
instance_id differ from alias id. we can use apic_id + maxcpus
as the instance_id. so during migration we can find the corresponding
cpu with instance_id regardless new/old scheme.

I has made a patch and test migrating from old version to new version.
it seems work fine. pls have a look at the attach file. ;)

Thanks,
Chen

>> +
>> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>> +        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>> +    }
>> +#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> that ifdef block affects only sparc/mips/cris and builds target specific code
> while you are trying to call it from target independent qom/cpu.c
>
> I'd suggest leave it where it was or better move into respective
> targets realize_fns
>
>> +    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>> +                    cpu_save, cpu_load, cpu->env_ptr);
>> +    assert(cc->vmsd == NULL);
>> +    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
>> +#endif
>> +    if (cc->vmsd != NULL) {
>> +        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>> +    }
>> +}
>> +
>>   void cpu_exec_init(CPUArchState *env)
>>   {
>>       CPUState *cpu = ENV_GET_CPU(env);
>> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>>       CPUState *some_cpu;
>>       int cpu_index;
>>   
>> @@ -539,18 +557,6 @@ void cpu_exec_init(CPUArchState *env)
>>   #if defined(CONFIG_USER_ONLY)
>>       cpu_list_unlock();
>>   #endif
>> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>> -        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>> -    }
>> -#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>> -    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>> -                    cpu_save, cpu_load, env);
>> -    assert(cc->vmsd == NULL);
>> -    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
>> -#endif
>> -    if (cc->vmsd != NULL) {
>> -        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>> -    }
> And in general do CONFIG_USER_ONLY targets actually need/use
> migration code?
>
>>   }
>>   
>>   #if defined(TARGET_HAS_ICE)
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 2098f1c..936afcd 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -562,6 +562,8 @@ void cpu_interrupt(CPUState *cpu, int mask);
>>   
>>   #endif /* USER_ONLY */
>>   
>> +void cpu_vmstate_register(CPUState *cpu);
>> +
>>   #ifdef CONFIG_SOFTMMU
>>   static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
>>                                            bool is_write, bool is_exec,
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 9c68fa4..a639822 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -302,6 +302,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>>   {
>>       CPUState *cpu = CPU(dev);
>>   
>> +    cpu_vmstate_register(cpu);
>> +
>>       if (dev->hotplugged) {
>>           cpu_synchronize_post_init(cpu);
>>           cpu_resume(cpu);
> .
>


[-- Attachment #2: fix-cross-version-impact.diff --]
[-- Type: text/x-patch, Size: 5032 bytes --]

diff --git a/exec.c b/exec.c
index bda96c6..6f3a90d 100644
--- a/exec.c
+++ b/exec.c
@@ -516,10 +516,12 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 void cpu_vmstate_register(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-    int cpu_index = cc->get_arch_id(cpu);
+    int cpu_index = cc->get_arch_id(cpu) + max_cpus;
+    int compat_index = cc->get_compat_id(cpu);
 
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register_with_alias_id(NULL, cpu_index, &vmstate_cpu_common,
+                                       cpu, compat_index, 3);
     }
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
@@ -528,7 +530,8 @@ void cpu_vmstate_register(CPUState *cpu)
     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
 #endif
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
+        vmstate_register_with_alias_id(NULL, cpu_index, cc->vmsd,
+                                       cpu, compat_index, 3);
     }
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0592b4d..3d0bcbf 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -130,6 +130,7 @@ typedef struct CPUClass {
     void (*dump_statistics)(CPUState *cpu, FILE *f,
                             fprintf_function cpu_fprintf, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
+    int64_t (*get_compat_id)(CPUState *cpu);
     bool (*get_paging_enabled)(const CPUState *cpu);
     void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
                                Error **errp);
diff --git a/qom/cpu.c b/qom/cpu.c
index a639822..48dee41 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -323,6 +323,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static int64_t cpu_common_get_compat_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -332,6 +337,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->parse_features = cpu_common_parse_features;
     k->reset = cpu_common_reset;
     k->get_arch_id = cpu_common_get_arch_id;
+    k->get_compat_id = cpu_common_get_compat_id;
     k->has_work = cpu_common_has_work;
     k->get_paging_enabled = cpu_common_get_paging_enabled;
     k->get_memory_mapping = cpu_common_get_memory_mapping;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e25789a..8c024b9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2794,13 +2794,15 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
     DeviceState *apic_state = cpu->apic_state;
     CPUClass *cc = CPU_GET_CLASS(CPU(cpu));
+    int cpu_index = cc->get_arch_id(CPU(cpu)) + max_cpus;
+    int compat_index = cc->get_compat_id(CPU(cpu));
 
     if (apic_state == NULL) {
         return;
     }
 
-    vmstate_register(0, cc->get_arch_id(CPU(cpu)),
-                     &vmstate_apic_common, apic_state);
+    vmstate_register_with_alias_id(NULL, cpu_index, &vmstate_apic_common,
+                                   apic_state, compat_index, 3);
 
     if (qdev_init(cpu->apic_state)) {
         error_setg(errp, "APIC device '%s' could not be initialized",
@@ -3042,6 +3044,15 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs)
     return env->cpuid_apic_id;
 }
 
+static int64_t x86_cpu_get_compat_id(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    return x86_compat_index_from_apic_id(smp_cores, smp_threads,
+                                         env->cpuid_apic_id);
+}
+
 static bool x86_cpu_get_paging_enabled(const CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
@@ -3122,6 +3133,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->gdb_read_register = x86_cpu_gdb_read_register;
     cc->gdb_write_register = x86_cpu_gdb_write_register;
     cc->get_arch_id = x86_cpu_get_arch_id;
+    cc->get_compat_id = x86_cpu_get_compat_id;
     cc->get_paging_enabled = x86_cpu_get_paging_enabled;
 #ifdef CONFIG_USER_ONLY
     cc->handle_mmu_fault = x86_cpu_handle_mmu_fault;
diff --git a/target-i386/topology.h b/target-i386/topology.h
index dcb4988..42d1d07 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -150,4 +150,20 @@ static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
     topo->smt_id = apic_id & offset_mask;
 }
 
+static inline unsigned x86_compat_index_from_apic_id(unsigned nr_cores,
+                                                     unsigned nr_threads,
+                                                     apic_id_t apic_id)
+{
+    X86CPUTopoInfo topo;
+
+    x86_topo_ids_from_apic_id(nr_cores, nr_threads, apic_id, &topo);
+
+
+    return topo.pkg_id * nr_cores * nr_threads +
+           topo.core_id * nr_threads +
+           topo.smt_id;
+}
+
+
+
 #endif /* TARGET_I386_TOPOLOGY_H */

[Qemu-devel] [PATCH v3 3/7] qom/cpu: move apic vmstate register into x86_cpu_apic_realize

[Zhu Guihua]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

move apic vmstate register into x86_cpu_apic_realize, and use
cc->get_arch_id as the instance id to avoid using the auto-id which will
break the migration if we add device not in order.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/intc/apic_common.c           | 3 +--
 include/hw/i386/apic_internal.h | 3 +++
 target-i386/cpu.c               | 8 +++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d9bb188..719f74d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -380,7 +380,7 @@ static const VMStateDescription vmstate_apic_common_sipi = {
     }
 };
 
-static const VMStateDescription vmstate_apic_common = {
+const VMStateDescription vmstate_apic_common = {
     .name = "apic",
     .version_id = 3,
     .minimum_version_id = 3,
@@ -434,7 +434,6 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
     ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &vmstate_apic_common;
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
     idc->realize = apic_common_realize;
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index dc7a89d..00d29e2 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -23,6 +23,7 @@
 #include "exec/memory.h"
 #include "hw/cpu/icc_bus.h"
 #include "qemu/timer.h"
+#include "migration/vmstate.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER                  0
@@ -138,6 +139,8 @@ typedef struct VAPICState {
 
 extern bool apic_report_tpr_access;
 
+extern const VMStateDescription vmstate_apic_common;
+
 void apic_report_irq_delivered(int delivered);
 bool apic_next_timer(APICCommonState *s, int64_t current_time);
 void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b81ac5c..3406fe8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2750,10 +2750,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
-    if (cpu->apic_state == NULL) {
+    DeviceState *apic_state = cpu->apic_state;
+    CPUClass *cc = CPU_GET_CLASS(CPU(cpu));
+
+    if (apic_state == NULL) {
         return;
     }
 
+    vmstate_register(0, cc->get_arch_id(CPU(cpu)),
+                     &vmstate_apic_common, apic_state);
+
     if (qdev_init(cpu->apic_state)) {
         error_setg(errp, "APIC device '%s' could not be initialized",
                    object_get_typename(OBJECT(cpu->apic_state)));
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 3/7] qom/cpu: move apic vmstate register into x86_cpu_apic_realize

[Igor Mammedov]

On Wed, 14 Jan 2015 15:27:26 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> move apic vmstate register into x86_cpu_apic_realize, and use
> cc->get_arch_id as the instance id to avoid using the auto-id which will
> break the migration if we add device not in order.
the same migration issue as in previous patch,

but it's easier to workaround it with machine compat switch,
just do the old way for old machine types and new way for new machine type.

> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/intc/apic_common.c           | 3 +--
>  include/hw/i386/apic_internal.h | 3 +++
>  target-i386/cpu.c               | 8 +++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index d9bb188..719f74d 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -380,7 +380,7 @@ static const VMStateDescription vmstate_apic_common_sipi = {
>      }
>  };
>  
> -static const VMStateDescription vmstate_apic_common = {
> +const VMStateDescription vmstate_apic_common = {
>      .name = "apic",
>      .version_id = 3,
>      .minimum_version_id = 3,
> @@ -434,7 +434,6 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
>      ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_apic_common;
>      dc->reset = apic_reset_common;
>      dc->props = apic_properties_common;
>      idc->realize = apic_common_realize;
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index dc7a89d..00d29e2 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -23,6 +23,7 @@
>  #include "exec/memory.h"
>  #include "hw/cpu/icc_bus.h"
>  #include "qemu/timer.h"
> +#include "migration/vmstate.h"
>  
>  /* APIC Local Vector Table */
>  #define APIC_LVT_TIMER                  0
> @@ -138,6 +139,8 @@ typedef struct VAPICState {
>  
>  extern bool apic_report_tpr_access;
>  
> +extern const VMStateDescription vmstate_apic_common;
> +
>  void apic_report_irq_delivered(int delivered);
>  bool apic_next_timer(APICCommonState *s, int64_t current_time);
>  void apic_enable_tpr_access_reporting(DeviceState *d, bool enable);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b81ac5c..3406fe8 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2750,10 +2750,16 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>  {
> -    if (cpu->apic_state == NULL) {
> +    DeviceState *apic_state = cpu->apic_state;
> +    CPUClass *cc = CPU_GET_CLASS(CPU(cpu));
> +
> +    if (apic_state == NULL) {
>          return;
>      }
>  
> +    vmstate_register(0, cc->get_arch_id(CPU(cpu)),
> +                     &vmstate_apic_common, apic_state);
> +
>      if (qdev_init(cpu->apic_state)) {
>          error_setg(errp, "APIC device '%s' could not be initialized",
>                     object_get_typename(OBJECT(cpu->apic_state)));

[Qemu-devel] [PATCH v3 4/7] monitor: use cc->get_arch_id as the cpu index

[Zhu Guihua]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

Use cc->get_arch_id as the cpu index to avoid the cpu index duplicated
issue in the QMP/HMP command output.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 cpus.c    | 4 +++-
 monitor.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 2edb5cd..d5e35c0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1420,6 +1420,7 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 
     CPU_FOREACH(cpu) {
         CpuInfoList *info;
+        CPUClass *cc;
 #if defined(TARGET_I386)
         X86CPU *x86_cpu = X86_CPU(cpu);
         CPUX86State *env = &x86_cpu->env;
@@ -1437,11 +1438,12 @@ CpuInfoList *qmp_query_cpus(Error **errp)
         CPUTriCoreState *env = &tricore_cpu->env;
 #endif
 
+        cc = CPU_GET_CLASS(cpu);
         cpu_synchronize_state(cpu);
 
         info = g_malloc0(sizeof(*info));
         info->value = g_malloc0(sizeof(*info->value));
-        info->value->CPU = cpu->cpu_index;
+        info->value->CPU = cc->get_arch_id(cpu);
         info->value->current = (cpu == first_cpu);
         info->value->halted = cpu->halted;
         info->value->thread_id = cpu->thread_id;
diff --git a/monitor.c b/monitor.c
index 1808e41..2283461 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1024,7 +1024,9 @@ static CPUArchState *mon_get_cpu(void)
 int monitor_get_cpu_index(void)
 {
     CPUState *cpu = ENV_GET_CPU(mon_get_cpu());
-    return cpu->cpu_index;
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->get_arch_id(cpu);
 }
 
 static void do_info_registers(Monitor *mon, const QDict *qdict)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 4/7] monitor: use cc->get_arch_id as the cpu index

[Igor Mammedov]

On Wed, 14 Jan 2015 15:27:27 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> Use cc->get_arch_id as the cpu index to avoid the cpu index duplicated
> issue in the QMP/HMP command output.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  cpus.c    | 4 +++-
>  monitor.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 2edb5cd..d5e35c0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1420,6 +1420,7 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  
>      CPU_FOREACH(cpu) {
>          CpuInfoList *info;
> +        CPUClass *cc;
>  #if defined(TARGET_I386)
>          X86CPU *x86_cpu = X86_CPU(cpu);
>          CPUX86State *env = &x86_cpu->env;
> @@ -1437,11 +1438,12 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>          CPUTriCoreState *env = &tricore_cpu->env;
>  #endif
>  
> +        cc = CPU_GET_CLASS(cpu);
>          cpu_synchronize_state(cpu);
>  
>          info = g_malloc0(sizeof(*info));
>          info->value = g_malloc0(sizeof(*info->value));
> -        info->value->CPU = cpu->cpu_index;
> +        info->value->CPU = cc->get_arch_id(cpu);
I'm not sure what impact sparse ID numbering would be on libvirt
CCing libvirt folks so they could check it.

>          info->value->current = (cpu == first_cpu);
>          info->value->halted = cpu->halted;
>          info->value->thread_id = cpu->thread_id;
> diff --git a/monitor.c b/monitor.c
> index 1808e41..2283461 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1024,7 +1024,9 @@ static CPUArchState *mon_get_cpu(void)
>  int monitor_get_cpu_index(void)
>  {
>      CPUState *cpu = ENV_GET_CPU(mon_get_cpu());
> -    return cpu->cpu_index;
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    return cc->get_arch_id(cpu);
>  }
>  
>  static void do_info_registers(Monitor *mon, const QDict *qdict)

Re: [Qemu-devel] [PATCH v3 4/7] monitor: use cc->get_arch_id as the cpu index

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

On Thu, Jan 29, 2015 at 15:12:49 +0100, Igor Mammedov wrote:
> On Wed, 14 Jan 2015 15:27:27 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> 
> > From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> > 
> > Use cc->get_arch_id as the cpu index to avoid the cpu index duplicated
> > issue in the QMP/HMP command output.
> > 
> > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> > Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> > ---
> >  cpus.c    | 4 +++-
> >  monitor.c | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 2edb5cd..d5e35c0 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1420,6 +1420,7 @@ CpuInfoList *qmp_query_cpus(Error **errp)
> >  
> >      CPU_FOREACH(cpu) {
> >          CpuInfoList *info;
> > +        CPUClass *cc;
> >  #if defined(TARGET_I386)
> >          X86CPU *x86_cpu = X86_CPU(cpu);
> >          CPUX86State *env = &x86_cpu->env;
> > @@ -1437,11 +1438,12 @@ CpuInfoList *qmp_query_cpus(Error **errp)
> >          CPUTriCoreState *env = &tricore_cpu->env;
> >  #endif
> >  
> > +        cc = CPU_GET_CLASS(cpu);
> >          cpu_synchronize_state(cpu);
> >  
> >          info = g_malloc0(sizeof(*info));
> >          info->value = g_malloc0(sizeof(*info->value));
> > -        info->value->CPU = cpu->cpu_index;
> > +        info->value->CPU = cc->get_arch_id(cpu);
> I'm not sure what impact sparse ID numbering would be on libvirt
> CCing libvirt folks so they could check it.

Libvirt is currently using only the "thread_id" field of the data
returned by the query-cpus QMP command.

One place where we currently treat the cpu ID space as continuous is in
the CPU hotplug and unplug API, but the unplug part is currently
obsolete as it uses only the very old HMP command.

At any rate, changing the "CPU" field shouldn't change anything that
libvirt relies on.

Peter



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v3 5/7] acpi:cpu hotplug: set pcmachine as icc bus' hotplug handler

[Zhu Guihua]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

As the pre-check in the qdev_device_add():
    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
        return NULL;
    }
if device has parent bus, the bus must have valid hotplug_handler,
otherwise can not hot plug.
Currently cpu hotplug is based on the PCMachine's hotplug handler,
so when hot add cpu, the hotpluggable check of icc bus will be
rejected.
So we set pcmachine as icc bus' hotplug handler to avoid the rejetion.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/cpu/icc_bus.c         | 5 +++++
 hw/i386/pc_piix.c        | 5 +++++
 hw/i386/pc_q35.c         | 5 +++++
 include/hw/cpu/icc_bus.h | 2 ++
 4 files changed, 17 insertions(+)

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 6646ea2..f455a20 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -92,6 +92,11 @@ static void icc_bridge_init(Object *obj)
     s->icc_bus.apic_address_space = &s->apic_container;
 }
 
+ICCBus *get_icc_bus(DeviceState *dev)
+{
+    return &ICC_BRIDGE(dev)->icc_bus;
+}
+
 static void icc_bridge_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f0a3201..363f796 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -281,6 +281,7 @@ static void pc_init1(MachineState *machine,
     if (pci_enabled && acpi_enabled) {
         DeviceState *piix4_pm;
         I2CBus *smbus;
+        ICCBus *iccbus;
 
         smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
         /* TODO: Populate SPD eeprom data.  */
@@ -296,6 +297,10 @@ static void pc_init1(MachineState *machine,
                                  OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
         object_property_set_link(OBJECT(machine), OBJECT(piix4_pm),
                                  PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
+
+        iccbus = get_icc_bus(icc_bridge);
+        object_property_set_link(OBJECT(iccbus), OBJECT(pc_machine),
+                                 QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
     }
 
     if (pci_enabled) {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a432944..2ce8b70 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
+    ICCBus *iccbus;
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -212,6 +213,10 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(machine), OBJECT(lpc),
                              PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
 
+    iccbus = get_icc_bus(icc_bridge);
+    object_property_set_link(OBJECT(iccbus), OBJECT(pc_machine),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
+
     ich9_lpc = ICH9_LPC_DEVICE(lpc);
     ich9_lpc->pic = gsi;
     ich9_lpc->ioapic = gsi_state->ioapic_irq;
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
index 98a979f..7e53afb 100644
--- a/include/hw/cpu/icc_bus.h
+++ b/include/hw/cpu/icc_bus.h
@@ -44,6 +44,8 @@ typedef struct ICCBus {
 
 #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
 
+ICCBus *get_icc_bus(DeviceState *dev);
+
 /**
  * ICCDevice:
  *
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 5/7] acpi:cpu hotplug: set pcmachine as icc bus' hotplug handler

[Igor Mammedov]

On Wed, 14 Jan 2015 15:27:28 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> As the pre-check in the qdev_device_add():
>     if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>         qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>         return NULL;
>     }
> if device has parent bus, the bus must have valid hotplug_handler,
> otherwise can not hot plug.
> Currently cpu hotplug is based on the PCMachine's hotplug handler,
> so when hot add cpu, the hotpluggable check of icc bus will be
> rejected.
> So we set pcmachine as icc bus' hotplug handler to avoid the rejetion.
ICC bus was invented only to provide hotplug capability to
CPU and APIC because at the time being hotplug was available only for
BUS attached devices.

Could we drop ICC bus impl. completely and switch to bus-less
CPU+APIC hotplug, handling them in the same manner as pc-dimm?

> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/cpu/icc_bus.c         | 5 +++++
>  hw/i386/pc_piix.c        | 5 +++++
>  hw/i386/pc_q35.c         | 5 +++++
>  include/hw/cpu/icc_bus.h | 2 ++
>  4 files changed, 17 insertions(+)
> 
> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> index 6646ea2..f455a20 100644
> --- a/hw/cpu/icc_bus.c
> +++ b/hw/cpu/icc_bus.c
> @@ -92,6 +92,11 @@ static void icc_bridge_init(Object *obj)
>      s->icc_bus.apic_address_space = &s->apic_container;
>  }
>  
> +ICCBus *get_icc_bus(DeviceState *dev)
> +{
> +    return &ICC_BRIDGE(dev)->icc_bus;
> +}
> +
>  static void icc_bridge_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f0a3201..363f796 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -281,6 +281,7 @@ static void pc_init1(MachineState *machine,
>      if (pci_enabled && acpi_enabled) {
>          DeviceState *piix4_pm;
>          I2CBus *smbus;
> +        ICCBus *iccbus;
>  
>          smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
>          /* TODO: Populate SPD eeprom data.  */
> @@ -296,6 +297,10 @@ static void pc_init1(MachineState *machine,
>                                   OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
>          object_property_set_link(OBJECT(machine), OBJECT(piix4_pm),
>                                   PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
> +
> +        iccbus = get_icc_bus(icc_bridge);
> +        object_property_set_link(OBJECT(iccbus), OBJECT(pc_machine),
> +                                 QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
>      }
>  
>      if (pci_enabled) {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a432944..2ce8b70 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -88,6 +88,7 @@ static void pc_q35_init(MachineState *machine)
>      PcGuestInfo *guest_info;
>      ram_addr_t lowmem;
>      DriveInfo *hd[MAX_SATA_PORTS];
> +    ICCBus *iccbus;
>  
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
> @@ -212,6 +213,10 @@ static void pc_q35_init(MachineState *machine)
>      object_property_set_link(OBJECT(machine), OBJECT(lpc),
>                               PC_MACHINE_ACPI_DEVICE_PROP, &error_abort);
>  
> +    iccbus = get_icc_bus(icc_bridge);
> +    object_property_set_link(OBJECT(iccbus), OBJECT(pc_machine),
> +                             QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
> +
>      ich9_lpc = ICH9_LPC_DEVICE(lpc);
>      ich9_lpc->pic = gsi;
>      ich9_lpc->ioapic = gsi_state->ioapic_irq;
> diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
> index 98a979f..7e53afb 100644
> --- a/include/hw/cpu/icc_bus.h
> +++ b/include/hw/cpu/icc_bus.h
> @@ -44,6 +44,8 @@ typedef struct ICCBus {
>  
>  #define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
>  
> +ICCBus *get_icc_bus(DeviceState *dev);
> +
>  /**
>   * ICCDevice:
>   *

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

[Zhu Guihua]

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);
+    }
 }
 
 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;
-    }
-
     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 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) {
+        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) {
+        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 */
-- 
1.9.3

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

[Igor Mammedov]

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

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

[Eduardo Habkost]

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

-- 
Eduardo

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

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Chen Fan]

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
>

[Qemu-devel] [PATCH v3 7/7] i386/cpu: add instance finalize callback

[Zhu Guihua]

From: Gu Zheng <guz.fnst@cn.fujitsu.com>

Add a func to finalize a cpu's instance. When cpu's device_add failed,
and cpu's device_del executed, this func would be invoked.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 include/qom/cpu.h | 1 +
 target-i386/cpu.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 936afcd..f663199 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -305,6 +305,7 @@ struct CPUState {
 QTAILQ_HEAD(CPUTailQ, CPUState);
 extern struct CPUTailQ cpus;
 #define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
+#define CPU_REMOVE(cpu) QTAILQ_REMOVE(&cpus, cpu, node)
 #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
     QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4347948..4746814 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2983,6 +2983,13 @@ static void x86_cpu_initfn(Object *obj)
     }
 }
 
+static void x86_cpu_finalizefn(Object *obj)
+{
+    CPUState *cs = CPU(obj);
+
+    CPU_REMOVE(cs);
+}
+
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
@@ -3095,6 +3102,7 @@ static const TypeInfo x86_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
     .instance_init = x86_cpu_initfn,
+    .instance_finalize = x86_cpu_finalizefn,
     .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 7/7] i386/cpu: add instance finalize callback

[Igor Mammedov]

On Wed, 14 Jan 2015 15:27:30 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> Add a func to finalize a cpu's instance. When cpu's device_add failed,
> and cpu's device_del executed, this func would be invoked.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  include/qom/cpu.h | 1 +
>  target-i386/cpu.c | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 936afcd..f663199 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -305,6 +305,7 @@ struct CPUState {
>  QTAILQ_HEAD(CPUTailQ, CPUState);
>  extern struct CPUTailQ cpus;
>  #define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
> +#define CPU_REMOVE(cpu) QTAILQ_REMOVE(&cpus, cpu, node)
>  #define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
>  #define CPU_FOREACH_SAFE(cpu, next_cpu) \
>      QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4347948..4746814 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2983,6 +2983,13 @@ static void x86_cpu_initfn(Object *obj)
>      }
>  }
>  
> +static void x86_cpu_finalizefn(Object *obj)
> +{
> +    CPUState *cs = CPU(obj);
> +
> +    CPU_REMOVE(cs);
> +}


issue with reliance on cpu_index is not solved completely,
There is still a lot of places that use cpu_index and when
we start ignore it in x86 CPU, the rest of the places probably
would have a broken logic. Please look at that call sites,
and if they are fine with it, especially common files shared
with other targets.

> +
>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> @@ -3095,6 +3102,7 @@ static const TypeInfo x86_cpu_type_info = {
>      .parent = TYPE_CPU,
>      .instance_size = sizeof(X86CPU),
>      .instance_init = x86_cpu_initfn,
> +    .instance_finalize = x86_cpu_finalizefn,
>      .abstract = true,
>      .class_size = sizeof(X86CPUClass),
>      .class_init = x86_cpu_common_class_init,

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On Wed, Jan 14, 2015 at 03:27:23PM +0800, Zhu Guihua wrote:
> This series is based on the previous patchset from Chen Fan:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html

This email has an invalid charset:
Content-Type: text/plain; charset="y"

I guess you entered "y" when asked how the message was encoded.

Please don't do that, it means we can only guess at the charset.

Thanks,
Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

[adding git list to cc]

On 02/05/2015 04:49 AM, Stefan Hajnoczi wrote:
> On Wed, Jan 14, 2015 at 03:27:23PM +0800, Zhu Guihua wrote:
>> This series is based on the previous patchset from Chen Fan:
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html
> 
> This email has an invalid charset:
> Content-Type: text/plain; charset="y"
> 
> I guess you entered "y" when asked how the message was encoded.
> 
> Please don't do that, it means we can only guess at the charset.

In the past, people made a similar problem when 'git send-email' was
asking if a message was in-reply-to something else (the number of
messages incorrectly threaded to a message-id of 'y' or 'n' was evidence
of the poor quality of the question).  git.git commit 51bbccfd1b4a
corrected that problem.  Sounds like charset encoding is another case
where the interactive parser should be taught to balk at nonsense
encoding answers?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Junio C Hamano]

Eric Blake <eblake@redhat.com> writes:

> On 02/05/2015 04:49 AM, Stefan Hajnoczi wrote:
>> On Wed, Jan 14, 2015 at 03:27:23PM +0800, Zhu Guihua wrote:
>>> This series is based on the previous patchset from Chen Fan:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html
>> 
>> This email has an invalid charset:
>> Content-Type: text/plain; charset="y"
>> 
>> I guess you entered "y" when asked how the message was encoded.
>> 
>> Please don't do that, it means we can only guess at the charset.
>
> In the past, people made a similar problem when 'git send-email' was
> asking if a message was in-reply-to something else (the number of
> messages incorrectly threaded to a message-id of 'y' or 'n' was evidence
> of the poor quality of the question).  git.git commit 51bbccfd1b4a
> corrected that problem.  Sounds like charset encoding is another case
> where the interactive parser should be taught to balk at nonsense
> encoding answers?

I think I answered this in $gmane/263354; care to come up with a
plausible valid_re?  It is inpractical to attempt to cover all valid
charset names, so whatever you do I'd imagine you would want to pass
the confirm_only parameter set to true.

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

[Jeff King]

On Thu, Feb 05, 2015 at 11:29:07AM -0800, Junio C Hamano wrote:

> Eric Blake <eblake@redhat.com> writes:
> 
> > On 02/05/2015 04:49 AM, Stefan Hajnoczi wrote:
> >> On Wed, Jan 14, 2015 at 03:27:23PM +0800, Zhu Guihua wrote:
> >>> This series is based on the previous patchset from Chen Fan:
> >>> https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html
> >> 
> >> This email has an invalid charset:
> >> Content-Type: text/plain; charset="y"
> >> 
> >> I guess you entered "y" when asked how the message was encoded.
> >> 
> >> Please don't do that, it means we can only guess at the charset.
> >
> > In the past, people made a similar problem when 'git send-email' was
> > asking if a message was in-reply-to something else (the number of
> > messages incorrectly threaded to a message-id of 'y' or 'n' was evidence
> > of the poor quality of the question).  git.git commit 51bbccfd1b4a
> > corrected that problem.  Sounds like charset encoding is another case
> > where the interactive parser should be taught to balk at nonsense
> > encoding answers?
> 
> I think I answered this in $gmane/263354; care to come up with a
> plausible valid_re?  It is inpractical to attempt to cover all valid
> charset names, so whatever you do I'd imagine you would want to pass
> the confirm_only parameter set to true.

Would "length() > 1" be enough[1]? Or are people really typing "yes" and
not just "y"?

I cannot imagine a charset name that is smaller than two characters. It
may be that there are none smaller than 4, and we could cut it off
there. Googling around for some lists of common charsets, it seems like
that might be plausible (but not any larger; "big5" is 4 characters, and
people may spell "utf8" without the hyphen).

-Peff

[1] Of course, to match the existing regex code, we may want to spell
    this as "/../" or "/..../".

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 05, 2015 at 11:29:07AM -0800, Junio C Hamano wrote:
>
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 02/05/2015 04:49 AM, Stefan Hajnoczi wrote:
>> >> On Wed, Jan 14, 2015 at 03:27:23PM +0800, Zhu Guihua wrote:
>> >>> This series is based on the previous patchset from Chen Fan:
>> >>> https://lists.nongnu.org/archive/html/qemu-devel/2014-05/msg02360.html
>> >> 
>> >> This email has an invalid charset:
>> >> Content-Type: text/plain; charset="y"
>> >> 
>> >> I guess you entered "y" when asked how the message was encoded.
>> >> 
>> >> Please don't do that, it means we can only guess at the charset.
>> >
>> > In the past, people made a similar problem when 'git send-email' was
>> > asking if a message was in-reply-to something else (the number of
>> > messages incorrectly threaded to a message-id of 'y' or 'n' was evidence
>> > of the poor quality of the question).  git.git commit 51bbccfd1b4a
>> > corrected that problem.  Sounds like charset encoding is another case
>> > where the interactive parser should be taught to balk at nonsense
>> > encoding answers?
>> 
>> I think I answered this in $gmane/263354; care to come up with a
>> plausible valid_re?  It is inpractical to attempt to cover all valid
>> charset names, so whatever you do I'd imagine you would want to pass
>> the confirm_only parameter set to true.
>
> Would "length() > 1" be enough[1]? Or are people really typing "yes" and
> not just "y"?
>
> I cannot imagine a charset name that is smaller than two characters. It
> may be that there are none smaller than 4, and we could cut it off
> there. Googling around for some lists of common charsets, it seems like
> that might be plausible (but not any larger; "big5" is 4 characters, and
> people may spell "utf8" without the hyphen).
>
> -Peff
>
> [1] Of course, to match the existing regex code, we may want to spell
>     this as "/../" or "/..../".

Perhaps. Just in case there were shorter ones, something like this
with confirm_only to allow them to say "Yes, I do mean 'xx'"?

 git-send-email.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3092ab3..848f176 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -752,6 +752,7 @@ sub file_declares_8bit_cte {
 		print "    $f\n";
 	}
 	$auto_8bit_encoding = ask("Which 8bit encoding should I declare [UTF-8]? ",
+				  valid_re => qr/.{4}/, confirm_only => 1,
 				  default => "UTF-8");
 }
 

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

[Jeff King]

On Thu, Feb 05, 2015 at 12:17:15PM -0800, Junio C Hamano wrote:

> > Would "length() > 1" be enough[1]? Or are people really typing "yes" and
> > not just "y"?
> >
> > I cannot imagine a charset name that is smaller than two characters. It
> > may be that there are none smaller than 4, and we could cut it off
> > there. Googling around for some lists of common charsets, it seems like
> > that might be plausible (but not any larger; "big5" is 4 characters, and
> > people may spell "utf8" without the hyphen).
> >
> > -Peff
> >
> > [1] Of course, to match the existing regex code, we may want to spell
> >     this as "/../" or "/..../".
> 
> Perhaps. Just in case there were shorter ones, something like this
> with confirm_only to allow them to say "Yes, I do mean 'xx'"?
> 
>  git-send-email.perl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 3092ab3..848f176 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -752,6 +752,7 @@ sub file_declares_8bit_cte {
>  		print "    $f\n";
>  	}
>  	$auto_8bit_encoding = ask("Which 8bit encoding should I declare [UTF-8]? ",
> +				  valid_re => qr/.{4}/, confirm_only => 1,
>  				  default => "UTF-8");
>  }

Yes, I think leaving an escape hatch is a good idea, just in case.

-Peff

[Qemu-devel] [PATCH] send-email: ask confirmation if given encoding name is very short

[Junio C Hamano]

Sometimes people respond "y<ENTER>" (or "yes<ENTER>") when asked
this question:

    Which 8bit encoding should I declare [UTF-8]?

We already have a mechanism to avoid accepting a mistyped e-mail
address (we ask to confirm when the given address lacks "@" in it);
reuse it to trigger the same confirmation when given a very short
answer.  As a typical charset name is probably at least 4 chars or
longer (e.g. "UTF8" spelled without the dash, or "Big5"), this would
prevent such a mistake.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Will mark to be merged to 'next'.

 git-send-email.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index fdb0029..eb32371 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -733,6 +733,7 @@ sub file_declares_8bit_cte {
 		print "    $f\n";
 	}
 	$auto_8bit_encoding = ask("Which 8bit encoding should I declare [UTF-8]? ",
+				  valid_re => qr/.{4}/, confirm_only => 1,
 				  default => "UTF-8");
 }
 
-- 
2.3.0-282-gf18c841

Re: [Qemu-devel] [PATCH] send-email: ask confirmation if given encoding name is very short

[Jeff King]

On Mon, Feb 16, 2015 at 02:34:14PM -0800, Junio C Hamano wrote:

> Sometimes people respond "y<ENTER>" (or "yes<ENTER>") when asked
> this question:
> 
>     Which 8bit encoding should I declare [UTF-8]?
> 
> We already have a mechanism to avoid accepting a mistyped e-mail
> address (we ask to confirm when the given address lacks "@" in it);
> reuse it to trigger the same confirmation when given a very short
> answer.  As a typical charset name is probably at least 4 chars or
> longer (e.g. "UTF8" spelled without the dash, or "Big5"), this would
> prevent such a mistake.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Will mark to be merged to 'next'.

Probably belated review, but this looks good to me.

-Peff