fix migration issues after hotplug a discontinuous cpuid

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);

target-i386/cpu.c +0 -0

 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Pls note that changing vmstate id should be done only for new machine types so not to break old qemu -> new qemu migration.

Quoted 4 lines

target-i386/cpu.c +0 -0

 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

Quoted 3 lines

Pls note that changing vmstate id should be done only for new machine types so not to break old qemu -> new qemu migration.

Yes.

Thanks.
Chen

Quoted 6 lines

Quoted 4 lines

target-i386/cpu.c +0 -0

 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);

Quoted 74 lines · On Wed, 15 Jan 2014 20:24:01 +0800 Chen Fan <[email protected]>…

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Quoted 2 lines

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

yep, that is a gist of it.

There is more to the issue with discontinuous CPUs, a lot of code still use cpu_index and the way it's allocated is not compatible with discontinuous CPUs, so this issue should be fixed as well.

Also you propose to use a raw apic id with CLI/user interface. I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

But nobody took on implementing it yet.

Quoted 93 lines · Igor Mammedov

Quoted 74 lines · On Wed, 15 Jan 2014 20:24:01 +0800 Chen Fan <[email protected]>…

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Quoted 2 lines

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

yep, that is a gist of it.

There is more to the issue with discontinuous CPUs, a lot of code still use cpu_index and the way it's allocated is not compatible with discontinuous CPUs, so this issue should be fixed as well.

Also you propose to use a raw apic id with CLI/user interface. I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

[1] See target-i386/topology.h for references

Quoted 94 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 93 lines · Igor Mammedov

Quoted 74 lines · On Wed, 15 Jan 2014 20:24:01 +0800 Chen Fan <[email protected]>…

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Quoted 2 lines

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

yep, that is a gist of it.

There is more to the issue with discontinuous CPUs, a lot of code still use cpu_index and the way it's allocated is not compatible with discontinuous CPUs, so this issue should be fixed as well.

Also you propose to use a raw apic id with CLI/user interface. I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 1 line

Quoted 3 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

Quoted 3 lines

[1] See target-i386/topology.h for references

Quoted 115 lines · Igor Mammedov

Quoted 94 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 93 lines · Igor Mammedov

Quoted 74 lines · On Wed, 15 Jan 2014 20:24:01 +0800 Chen Fan <[email protected]>…

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Quoted 2 lines

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

yep, that is a gist of it.

There is more to the issue with discontinuous CPUs, a lot of code still use cpu_index and the way it's allocated is not compatible with discontinuous CPUs, so this issue should be fixed as well.

Also you propose to use a raw apic id with CLI/user interface. I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

Thanks,
Chen

Quoted 4 lines

Quoted 3 lines

[1] See target-i386/topology.h for references

Quoted 120 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 119 lines · Igor Mammedov

Quoted 94 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 93 lines · Igor Mammedov

Quoted 74 lines · On Wed, 15 Jan 2014 20:24:01 +0800 Chen Fan <[email protected]>…

Quoted 60 lines · Igor Mammedov

Quoted 57 lines · On Tue, 14 Jan 2014 17:27:20 +0800 Chen Fan <[email protected]>…

the intend of this patch is to register cpu vmstates with apic id instead of cpu index, due to the property setting of apic_id is behind the cpu initialization. so we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to let the set apicid as the parameter.

Signed-off-by Chen Fan <[email protected]>

---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

exec.c +5 -0

index 7e49e8e..9be5855 100644
@@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index)
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+#if !defined(TARGET_I386)
     CPUClass *cc = CPU_GET_CLASS(cpu);
+#endif
     CPUState *some_cpu;
     int cpu_index;
 

@@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env)
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
 #endif
+
+#if !defined(TARGET_I386)
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
     }

@@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+#endif /* !TARGET_I386 */
 }
 
 #if defined(TARGET_HAS_ICE)

target-i386/cpu.c +1 -0

index 967529a..dada6f6 100644
@@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;

@@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_reset(cs);
 

Quoted 12 lines

target-i386/cpu.c static void x86_cpu_realizefn(DeviceState *dev, Error **errp) · line 2626

Quoted 9 lines

target-i386/cpu.c +8 -0

     xcc->parent_realize(dev, &local_err);
+
+    if (qdev_get_vmsd(DEVICE(cs)) == NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs);
+    }
+
+    if (cc->vmsd != NULL) {
+        vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs);
+    }

how about doing it in common CPUclass.realize()
you can use get_arch_id() for getting CPU id, it returns cpu_index by default and apic_id for target-i386.

Quoted 2 lines

Thanks for your kind suggestion, does this mean we can directly move vmstate_register to cpu_common_realizefn()?

yep, that is a gist of it.

There is more to the issue with discontinuous CPUs, a lot of code still use cpu_index and the way it's allocated is not compatible with discontinuous CPUs, so this issue should be fixed as well.

Also you propose to use a raw apic id with CLI/user interface. I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

Quoted 2 lines

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Quoted 12 lines

Thanks,
Chen

Quoted 4 lines

Quoted 3 lines

[1] See target-i386/topology.h for references

Quoted 40 lines

Quoted 30 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 29 lines · Igor Mammedov

Quoted 9 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 8 lines · Igor Mammedov

I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 6 lines

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

Quoted 5 lines

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Thanks, I hope I can take some time to make some patches to implement it.

Am 21.01.2014 10:51, schrieb Chen Fan:

Quoted 41 lines · Igor Mammedov

Quoted 40 lines

Quoted 30 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 29 lines · Igor Mammedov

Quoted 9 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 8 lines · Igor Mammedov

I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 6 lines

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

Quoted 7 lines

Quoted 5 lines

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Thanks, I hope I can take some time to make some patches to implement it.

Please give us a few hours to reply. :)

/machine/node seems too broad a term to me.
You can't prebuild the full tree, you can only prepare the nodes. core[Y]/thread[Z] was previously discussed as syntax.

The important part to decide on will be what is going to be child<> and what link<>. Has anyone played with the Intel Quark platform for instance? (Galileo board or upcoming Edison card) On a regular mainboard, we would have socket[X] as a link<x86_64-cpu>, which might point to a child<cpu> /machine/memory-node[W]/cpu[X]. But if we do so we can't reassign it to another memory node - acceptable? With Quark (or Qseven modules etc.) there would be a container object rather than the /machine itself that has a child<i386-cpu> instead of a link<i386-cpu>. I guess the memory nodes could still be on the /machine though. The other point of discussion between Anthony and me was whether core[Y] should be a link<> or child<>, same for thread. I believe a child<> is better as it enforces that unrealizing the CPU will unrealize all its cores and all its threads in the future.

More issues may pop up when thinking about it longer than a few minutes. But yes, we need to start investigating this, and so far I had other priorities like getting the CPUState mess I created cleaned up.

Regards,
Andreas

Quoted 70 lines · Andreas Färber

Am 21.01.2014 10:51, schrieb Chen Fan:

Quoted 41 lines · Igor Mammedov

Quoted 40 lines

Quoted 30 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 29 lines · Igor Mammedov

Quoted 9 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 8 lines · Igor Mammedov

I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 6 lines

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

Quoted 7 lines

Quoted 5 lines

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Thanks, I hope I can take some time to make some patches to implement it.

Please give us a few hours to reply. :)

/machine/node seems too broad a term to me.
You can't prebuild the full tree, you can only prepare the nodes. core[Y]/thread[Z] was previously discussed as syntax.

The important part to decide on will be what is going to be child<> and what link<>. Has anyone played with the Intel Quark platform for instance? (Galileo board or upcoming Edison card) On a regular mainboard, we would have socket[X] as a link<x86_64-cpu>, which might point to a child<cpu> /machine/memory-node[W]/cpu[X]. But if we do so we can't reassign it to another memory node - acceptable? With Quark (or Qseven modules etc.) there would be a container object rather than the /machine itself that has a child<i386-cpu> instead of a link<i386-cpu>. I guess the memory nodes could still be on the /machine though. The other point of discussion between Anthony and me was whether core[Y] should be a link<> or child<>, same for thread. I believe a child<> is better as it enforces that unrealizing the CPU will unrealize all its cores and all its threads in the future.

Quoted 3 lines

More issues may pop up when thinking about it longer than a few minutes. But yes, we need to start investigating this, and so far I had other priorities like getting the CPUState mess I created cleaned up.

Hi, Igor, Andreas,

In addition, I want to know what way user could use to specify an
arbitrary CPU if using /machine/node/N/socket/X/core/Y/thread/Z idea? -device qemu64,socket=X,core=Y,thread=Z? or add a new optional command line?

Thanks,
Chen

Quoted 4 lines

Regards,
Andreas

Quoted 74 lines · On Thu, 13 Feb 2014 14:14:08 +0800 Chen Fan <[email protected]>…

Quoted 70 lines · Andreas Färber

Am 21.01.2014 10:51, schrieb Chen Fan:

Quoted 41 lines · Igor Mammedov

Quoted 40 lines

Quoted 30 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 29 lines · Igor Mammedov

Quoted 9 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 8 lines · Igor Mammedov

I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 6 lines

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

Quoted 7 lines

Quoted 5 lines

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Thanks, I hope I can take some time to make some patches to implement it.

Please give us a few hours to reply. :)

/machine/node seems too broad a term to me.
You can't prebuild the full tree, you can only prepare the nodes. core[Y]/thread[Z] was previously discussed as syntax.

The important part to decide on will be what is going to be child<> and what link<>. Has anyone played with the Intel Quark platform for instance? (Galileo board or upcoming Edison card) On a regular mainboard, we would have socket[X] as a link<x86_64-cpu>, which might point to a child<cpu> /machine/memory-node[W]/cpu[X]. But if we do so we can't reassign it to another memory node - acceptable? With Quark (or Qseven modules etc.) there would be a container object rather than the /machine itself that has a child<i386-cpu> instead of a link<i386-cpu>. I guess the memory nodes could still be on the /machine though. The other point of discussion between Anthony and me was whether core[Y] should be a link<> or child<>, same for thread. I believe a child<> is better as it enforces that unrealizing the CPU will unrealize all its cores and all its threads in the future.

Quoted 3 lines

More issues may pop up when thinking about it longer than a few minutes. But yes, we need to start investigating this, and so far I had other priorities like getting the CPUState mess I created cleaned up.

Quoted 6 lines

Hi, Igor, Andreas,

In addition, I want to know what way user could use to specify an
arbitrary CPU if using /machine/node/N/socket/X/core/Y/thread/Z idea? -device qemu64,socket=X,core=Y,thread=Z? or add a new optional command line?

Definitely not another CLU option.
I see a couple of options,
1. as you suggest with additional 'numa=N' so that QEMU could know where to attach a new CPU.
2. add 'parent' like option tied to link<cpu> property and specify full QOM path on CLI: -device cpufoo,parent=/machine/node[N]/socket[X]/...

Quoted 10 lines

Thanks,
Chen

Quoted 4 lines

Regards,
Andreas

Quoted 83 lines · Igor Mammedov

Quoted 74 lines · On Thu, 13 Feb 2014 14:14:08 +0800 Chen Fan <[email protected]>…

Quoted 70 lines · Andreas Färber

Am 21.01.2014 10:51, schrieb Chen Fan:

Quoted 41 lines · Igor Mammedov

Quoted 40 lines

Quoted 30 lines · On Tue, 21 Jan 2014 15:12:45 +0800 Chen Fan <[email protected]>…

Quoted 29 lines · Igor Mammedov

Quoted 9 lines · On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost <[email protected]> wrote:

Quoted 8 lines · Igor Mammedov

I recall there were objections to it since APIC ID contains topology information and it's not trivial for user to get it right. The last idea that was discussed to fix it was not expose APIC ID to user but rather introduce QOM hierarchy like:
/machine/node/N/socket/X/core/Y/thread/Z
and use it in user interface as a means to specify an arbitrary CPU and let QEMU calculate APIC ID based on this path.

Quoted 6 lines

Quoted 1 line

But nobody took on implementing it yet.

We're taking so long to get a decent interface implemented, that part of me is considering exposing the APIC ID directly like suggested before, and requiring libvirt to calculate topology-aware APIC IDs[1] to properly implement CPU hotplug (and possibly for other tasks).

If you are speaking about
'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"'
http://patchwork.ozlabs.org/patch/301272/
bug then it's limitation of ACPI implementation,
I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count.

Quoted 4 lines

Another part of me is hoping that the libvirt developers ask us to please not do that, so I can use it as argument against exposing the APIC IDs directly the next time we discuss this. :)

Quoted 6 lines

Quoted 4 lines

why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general.

have there been any plan/model of the idea? Need to add a new option to qemu command?

I suppose we can start with internal default implementation first.

Quoted 7 lines

Quoted 5 lines

one way could be
1. let machine prebuild empty QOM tree /machine/node/N/socket/X/core/Y/thread/Z
2. add node, socket, core, thread properties to CPU and link CPU into respective link created by #1

Thanks, I hope I can take some time to make some patches to implement it.

Please give us a few hours to reply. :)

/machine/node seems too broad a term to me.
You can't prebuild the full tree, you can only prepare the nodes. core[Y]/thread[Z] was previously discussed as syntax.

The important part to decide on will be what is going to be child<> and what link<>. Has anyone played with the Intel Quark platform for instance? (Galileo board or upcoming Edison card) On a regular mainboard, we would have socket[X] as a link<x86_64-cpu>, which might point to a child<cpu> /machine/memory-node[W]/cpu[X]. But if we do so we can't reassign it to another memory node - acceptable? With Quark (or Qseven modules etc.) there would be a container object rather than the /machine itself that has a child<i386-cpu> instead of a link<i386-cpu>. I guess the memory nodes could still be on the /machine though. The other point of discussion between Anthony and me was whether core[Y] should be a link<> or child<>, same for thread. I believe a child<> is better as it enforces that unrealizing the CPU will unrealize all its cores and all its threads in the future.

Quoted 3 lines

More issues may pop up when thinking about it longer than a few minutes. But yes, we need to start investigating this, and so far I had other priorities like getting the CPUState mess I created cleaned up.

Quoted 6 lines

Hi, Igor, Andreas,

In addition, I want to know what way user could use to specify an
arbitrary CPU if using /machine/node/N/socket/X/core/Y/thread/Z idea? -device qemu64,socket=X,core=Y,thread=Z? or add a new optional command line?

Quoted 7 lines

Definitely not another CLU option.
I see a couple of options,
1. as you suggest with additional 'numa=N' so that QEMU could know where to attach a new CPU.
2. add 'parent' like option tied to link<cpu> property and specify full QOM path on CLI: -device cpufoo,parent=/machine/node[N]/socket[X]/...

Hi, Igor,
Currently, we know, after adding an arbitrary CPU then do migration,
on target, there will be not aware that which CPU have been added.
in order to notify target of the cpu topo, can we specify full QOM
path that you mentioned 2th point on target? if we can simply make smp n + 1 work as well at target to be better, but target how to know the cpu topo on source side?

Quoted 14 lines

Quoted 12 lines

Thanks,
Chen

Quoted 4 lines

Regards,
Andreas

Signed-off-by Chen Fan <[email protected]>

---
 include/qom/node.h |  66 +++++++++++++++++++++++
 qom/Makefile.objs  |   2 +-
 qom/node.c         | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 include/qom/node.h
 create mode 100644 qom/node.c

include/qom/node.h new file +0 -0

new file mode 100644
index 0000000..6c948ec

Quoted 9 lines · Chen Fan

Signed-off-by Chen Fan <[email protected]>

---
 include/qom/node.h |  66 +++++++++++++++++++++++
 qom/Makefile.objs  |   2 +-
 qom/node.c         | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 include/qom/node.h
 create mode 100644 qom/node.c

include/qom/node.h line 4

Quoted 10 lines

include/qom/node.h new file +4 -0

new file mode 100644
index 0000000..6c948ec
@@ -0,0 +1,66 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2013 Fujitsu Ltd.

It's 2014 now.

+Object *object_get_thread_from_index(int64_t cpu_index)

qom/node.c new file +5 -0

+{
+    gchar *path;
+    Object *thread;
+    int nodes, cpus_pre_nodes, smp_sockets;
+    unsigned node_id, socket_id, core_id, thread_id;

+ node_id = socket_index / smp_sockets;

qom/node.c new file +10 -0

+    socket_id = socket_index % smp_sockets;
+
+    path = g_strdup_printf("/machine/node[%d]/socket[%d]/core[%d]/thread[%d]",
+                           node_id, socket_id, core_id, thread_id);
+    thread = object_resolve_path(path, NULL);
+    g_free(path);
+
+    return thread;
+}
+

On Tue, Feb 25, 2014 at 05:07:31PM +0800, Chen Fan wrote:
[...]

Quoted 1 line

+Object *object_get_thread_from_index(int64_t cpu_index)

Probably the code that is going to call this function already knows what will be the node/socket/core/thread IDs for the CPU. And if it doesn't, we need to make the code node-/socket-/core-/thread-aware, instead of making the code dependent on CPU-index-based logic.

I believe we should try to make CPU indexes unnecessary, except on places where we are already forced to deal with them for compatibility reasons.

Quoted 5 lines

qom/node.c new file +5 -0

+{
+    gchar *path;
+    Object *thread;
+    int nodes, cpus_pre_nodes, smp_sockets;
+    unsigned node_id, socket_id, core_id, thread_id;

qom/node.c line 49

Quoted 10 lines

qom/node.c new file +10 -0

+    unsigned core_index, socket_index;
+
+    nodes = nb_numa_nodes ? nb_numa_nodes : 1;
+    cpus_pre_nodes = max_cpus / nodes;
+    smp_sockets = cpus_pre_nodes / (smp_cores * smp_threads);
+
+    core_index = cpu_index / smp_threads;
+    thread_id = cpu_index % smp_threads;
+    socket_index = core_index / smp_cores;
+    core_id = core_index % smp_cores;

This is duplicating logic that already exists at topo_ids_from_idx().

Quoted 1 line

+ node_id = socket_index / smp_sockets;

This is not how CPUs are distributed along NUMA nodes. They are distributed depending on the -numa options. Grep for node_cpumask in the code.

Quoted 10 lines

qom/node.c new file +10 -0

+    socket_id = socket_index % smp_sockets;
+
+    path = g_strdup_printf("/machine/node[%d]/socket[%d]/core[%d]/thread[%d]",
+                           node_id, socket_id, core_id, thread_id);
+    thread = object_resolve_path(path, NULL);
+    g_free(path);
+
+    return thread;
+}
+

Signed-off-by Chen Fan <[email protected]>

---
 hw/i386/pc.c           | 13 +++++++++----
 target-i386/cpu.c      | 16 ++++++++++++++++
 target-i386/cpu.h      |  4 ++++
 target-i386/topology.h |  7 +++++++
 4 files changed, 36 insertions(+), 4 deletions(-)

hw/i386/pc.c +0 -0

index 348b15f..a4d539e 100644

hw/i386/pc.c +1 -0

     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {

@@ -956,6 +957,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     DeviceState *icc_bridge;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
+    X86TopoInfo *topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);

hw/i386/pc.c +3 -2

 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)

@@ -996,8 +1000,9 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     current_cpu_model = cpu_model;
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);
+        X86TopoInfo *topo = x86_cpu_topo_from_index(i);
+        cpu = pc_new_cpu(cpu_model, topo, icc_bridge, &error);
+        g_free(topo);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);

target-i386/cpu.c +0 -0

index 0e8812a..1858a66 100644

target-i386/cpu.c +14 -0

+{
+    unsigned pkg_id, core_id, smt_id;
+    X86TopoInfo *x86TopoInfo;
+
+    x86TopoInfo = g_malloc0(sizeof(X86TopoInfo));
+    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
+                          &pkg_id, &core_id, &smt_id);
+    x86TopoInfo->pkg_id = pkg_id;
+    x86TopoInfo->core_id = core_id;
+    x86TopoInfo->smt_id = smt_id;
+    x86TopoInfo->cpu_index = cpu_index;
+
+    return x86TopoInfo;
+}

Quoted 13 lines · Chen Fan

Signed-off-by Chen Fan <[email protected]>

---
 hw/i386/pc.c           | 13 +++++++++----
 target-i386/cpu.c      | 16 ++++++++++++++++
 target-i386/cpu.h      |  4 ++++
 target-i386/topology.h |  7 +++++++
 4 files changed, 36 insertions(+), 4 deletions(-)

hw/i386/pc.c +0 -0

index 348b15f..a4d539e 100644

hw/i386/pc.c void pc_acpi_smi_interrupt(void *opaque, int irq, int level) · line 935

Quoted 10 lines

hw/i386/pc.c +2 -1

@@ -927,11 +927,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
+static X86CPU *pc_new_cpu(const char *cpu_model, X86TopoInfo *topo,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
+    int64_t apic_id = x86_cpu_apic_id_from_index(topo->cpu_index);

You can rewrite apicid_from_topo_ids() to be:

void x86_apicid_from_topo(X86TopoInfo *topo)

then x86_cpu_apic_id_from_index() won't need to exist anymore.

Quoted 12 lines

hw/i386/pc.c +1 -0

     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {

@@ -956,6 +957,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     DeviceState *icc_bridge;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
+    X86TopoInfo *topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);

hw/i386/pc.c void pc_hot_add_cpu(const int64_t id, Error **errp) · line 983

Quoted 7 lines

hw/i386/pc.c +3 -1

@@ -976,7 +978,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    topo = x86_cpu_topo_from_index(id);
+    pc_new_cpu(current_cpu_model, topo, icc_bridge, errp);
+    g_free(topo);

See comment below about simplifying x86_cpu_topo_from_index() to not require malloc/free. The code would be much simpler if it looked like this:

    X86TopoInfo topo;
    x86_cpu_topo_from_index(cpu_index, &topo)
    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp)

Quoted 21 lines

hw/i386/pc.c +3 -2

 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)

@@ -996,8 +1000,9 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     current_cpu_model = cpu_model;
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);
+        X86TopoInfo *topo = x86_cpu_topo_from_index(i);
+        cpu = pc_new_cpu(cpu_model, topo, icc_bridge, &error);
+        g_free(topo);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);

target-i386/cpu.c +0 -0

index 0e8812a..1858a66 100644

target-i386/cpu.c uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) · line 2612

Quoted 4 lines

target-i386/cpu.c +1 -0

@@ -2609,6 +2609,22 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
+X86TopoInfo *x86_cpu_topo_from_index(unsigned int cpu_index)

What about simply making it:

void x86_cpu_topo_from_index(unsigned int cpu_index, X86TopoInfo *topo)

so code can use a stack variable for X86TopoInfo. Much simpler than having to manually allocate/free X86TopoInfo.

What about rewriting x86_topo_ids_from_idx() to get X86TopoInfo* as argument, instead of creating this new function? Both functions have exactly the same purpose.

Quoted 15 lines

target-i386/cpu.c +14 -0

+{
+    unsigned pkg_id, core_id, smt_id;
+    X86TopoInfo *x86TopoInfo;
+
+    x86TopoInfo = g_malloc0(sizeof(X86TopoInfo));
+    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
+                          &pkg_id, &core_id, &smt_id);
+    x86TopoInfo->pkg_id = pkg_id;
+    x86TopoInfo->core_id = core_id;
+    x86TopoInfo->smt_id = smt_id;
+    x86TopoInfo->cpu_index = cpu_index;
+
+    return x86TopoInfo;
+}

[...]

From: "chen.fan.fnst" <[email protected]>

Signed-off-by Chen Fan <[email protected]>

---
 target-i386/topology.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

target-i386/topology.h +0 -0

index 07a6c5f..7843976 100644

From: "chen.fan.fnst" <[email protected]>

Signed-off-by Chen Fan <[email protected]>

---
 hw/i386/pc.c      | 12 ++++++++----
 target-i386/cpu.c |  2 --
 target-i386/cpu.h |  3 +++
 3 files changed, 11 insertions(+), 6 deletions(-)

hw/i386/pc.c +2 -1

index e715a33..c754042 100644
@@ -927,11 +927,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
+static X86CPU *pc_new_cpu(const char *cpu_model, CpuTopoInfo *topo,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
+    uint32_t apic_id = apicid_from_topo_ids(smp_cores, smp_threads, topo);
 
     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {

hw/i386/pc.c +3 -1

+    CpuTopoInfo topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);

@@ -976,7 +978,8 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    x86_topo_ids_from_idx(smp_cores, smp_threads, id, &topo);
+    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp);
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)

hw/i386/pc.c +3 -0

+        CpuTopoInfo topo;
+        x86_topo_ids_from_idx(smp_cores, smp_threads, i, &topo);
+        cpu = pc_new_cpu(cpu_model, &topo, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);

target-i386/cpu.c +0 -2

index 0e8812a..3a5df33 100644
@@ -23,8 +23,6 @@
 
 #include "cpu.h"
 #include "sysemu/kvm.h"
-#include "sysemu/cpus.h"
-#include "topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"

target-i386/cpu.h +3 -0

index 0014acc..8bc7b45 100644
@@ -22,6 +22,9 @@
 #include "config.h"
 #include "qemu-common.h"
 
+#include "sysemu/cpus.h"
+#include "topology.h"
+
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
 #else

Signature · 1 line

1.8.1.4

Quoted 28 lines · chen.fan.fnst

From: "chen.fan.fnst" <[email protected]>

Signed-off-by Chen Fan <[email protected]>

---
 hw/i386/pc.c      | 12 ++++++++----
 target-i386/cpu.c |  2 --
 target-i386/cpu.h |  3 +++
 3 files changed, 11 insertions(+), 6 deletions(-)

hw/i386/pc.c +2 -1

index e715a33..c754042 100644
@@ -927,11 +927,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
+static X86CPU *pc_new_cpu(const char *cpu_model, CpuTopoInfo *topo,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
+    uint32_t apic_id = apicid_from_topo_ids(smp_cores, smp_threads, topo);
 
     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {

hw/i386/pc.c void pc_hot_add_cpu(const int64_t id, Error **errp) · line 959

Quoted 3 lines

hw/i386/pc.c +0 -0

@@ -956,6 +957,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     DeviceState *icc_bridge;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);

With this patch, pc_new_cpu() is CPUTopoInfo-based, but cpu_exists() is apic_id-based. This requires apic_id to be calculated here, but pc_new_cpu() doesn't require it anymore.

We could make this more consistent, and avoid calculating the APIC ID in two different places. We are eveng using two different methods: here we use x86_cpu_apic_id_from_index() and pc_new_cpu() is using apicid_from_topo_ids().

Maybe we could simply move the cpu_exists() check inside pc_new_cpu()?

Quoted 15 lines

hw/i386/pc.c +3 -1

+    CpuTopoInfo topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);

@@ -976,7 +978,8 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    x86_topo_ids_from_idx(smp_cores, smp_threads, id, &topo);
+    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp);
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)

hw/i386/pc.c void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) · line 1000

Quoted 5 lines

hw/i386/pc.c +0 -2

@@ -996,8 +999,9 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     current_cpu_model = cpu_model;
 
     for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);

x86_cpu_apic_id_from_index() has compat code to keep compatibility on older machine-types, and you are removing the compat behavior.

To keep the compat code isolated on pc_cpus_init() and keep pc_new_cpu() interface saner, we could do something like this, on pc_cpus_init() only:

    CPUTopoInfo topo;
    x86_topo_ids_from_idx(cpu_index, &topo);
    if (apic_id_compat) {
        correct_apic_id = apicid_from_topo_ids(&topo);
        apic_id = cpu_index;
        if (apic_id != correct_apic_id) {
            error_report("APIC IDs set in compatibility mode, "
                         "CPU topology won't match the configuration");
        }
        // get_topo_from_apic_id() needs to be written:
        get_topo_from_apic_id(apic_id, &topo);
        // just in case:
        assert(apicid_from_topo_ids(&topo) == apic_id);
    }

And on pc_hot_add_cpu():

    if (apic_id_compat) {
        error_setg(errp, "CPU hotplug is not supported on this machine");
        return;
    }

Quoted 39 lines

hw/i386/pc.c +3 -0

+        CpuTopoInfo topo;
+        x86_topo_ids_from_idx(smp_cores, smp_threads, i, &topo);
+        cpu = pc_new_cpu(cpu_model, &topo, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);

target-i386/cpu.c +0 -2

index 0e8812a..3a5df33 100644
@@ -23,8 +23,6 @@
 
 #include "cpu.h"
 #include "sysemu/kvm.h"
-#include "sysemu/cpus.h"
-#include "topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"

target-i386/cpu.h +3 -0

index 0014acc..8bc7b45 100644
@@ -22,6 +22,9 @@
 #include "config.h"
 #include "qemu-common.h"
 
+#include "sysemu/cpus.h"
+#include "topology.h"
+
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
 #else

Signature · 1 line

1.8.1.4

Signed-off-by Chen Fan <[email protected]>

---
 target-i386/topology.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

target-i386/topology.h +6 -0

index 07a6c5f..9b811c1 100644
@@ -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)

introduce x86_cpu_topo_ids_from_index() to calculate the cpu topology information, and the compat old mode mechanism moved into there. remove unused funciton x86_apicid_from_cpu_idx().

Signed-off-by Chen Fan <[email protected]>

Quoted 5 lines · Chen Fan

introduce x86_cpu_topo_ids_from_index() to calculate the cpu topology information, and the compat old mode mechanism moved into there. remove unused funciton x86_apicid_from_cpu_idx().

Signed-off-by Chen Fan <[email protected]>

[...]

target-i386/topology.h static inline void x86_topo_ids_from_idx(unsigned nr_cores, · line 129

Quoted 7 lines

target-i386/topology.h +4 -3

-static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
-                                                unsigned nr_threads,
-                                                unsigned cpu_index)
+static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
+                                             unsigned nr_threads,
+                                             apic_id_t apic_id,
+                                             X86CPUTopoInfo *topo)

You need to squash this patch and patch 3/4 together, otherwise we get unit test failures in the intermediate tree.

Otherwise, patch looks good.

at present, after hotplug a discontinuous cpu id on source, then done migration, on target, it will fail to add the unoccupied cpu id which was skipped at source, this cause is on target Qemu prebuild CPU with continuous cpu_index. so after migration, the cpu infrastructure bewteen source and target are different.

I suppose we could use apic_id as instance_id which was used at registering vmstate when create cpu. on target, we prebuild the specified cpu topology using comand line:
-device /machine/node[]/socket[]/core[]/cpu[], then migration, we could keep the same
cpu infrastructure on both side.

RFC:
V4: rename CpuTopoInfo to X86CPUTopoInfo. and move cpu_exsit() to pc_new_cpu().

V3: get rid of thread object and tie link<cpu> to <core> directly. and prebuild full
core[] and thread[] as init socket[] according to smp_cores and smp_threads.

TODO:
1. add cpu "path" property which used for specifying the QOM path.
2. add -device cpu-foo.path supported.
3. then we could introduce hot-remove cpu probably.

I don't know wether this way is right or not. pls tell me. :)

Quoted 21 lines · Chen Fan

at present, after hotplug a discontinuous cpu id on source, then done migration, on target, it will fail to add the unoccupied cpu id which was skipped at source, this cause is on target Qemu prebuild CPU with continuous cpu_index. so after migration, the cpu infrastructure bewteen source and target are different.

I suppose we could use apic_id as instance_id which was used at registering vmstate when create cpu. on target, we prebuild the specified cpu topology using comand line:
-device /machine/node[]/socket[]/core[]/cpu[], then migration, we could keep the same
cpu infrastructure on both side.

RFC:
V4: rename CpuTopoInfo to X86CPUTopoInfo. and move cpu_exsit() to pc_new_cpu().

V3: get rid of thread object and tie link<cpu> to <core> directly. and prebuild full
core[] and thread[] as init socket[] according to smp_cores and smp_threads.

TODO:
1. add cpu "path" property which used for specifying the QOM path.
2. add -device cpu-foo.path supported.
3. then we could introduce hot-remove cpu probably.

Quoted 1 line

I don't know wether this way is right or not. pls tell me. :)

When sending a cover letter for a new revision of a patch series, we generally do NOT use In-Reply-To headers, but instead send it as a new thread. Patches are harder to see when they are buried as a reply to another thread.

     if (compat_apic_id_mode) {

target-i386/cpu.c +2 -2

-        if (cpu_index != correct_id && !warned) {
+        if (cpu_index != correct_id && !compat_apic_id_warned) {
             error_report("APIC IDs set in compatibility mode, "
                          "CPU topology won't match the configuration");
-            warned = true;
+            compat_apic_id_warned = true;