[Qemu-devel] [RFC 0/3] fix migration issues after hotplug a discontinuous cpuid

[Qemu-devel] [RFC 0/3] fix migration issues after hotplug a discontinuous cpuid

[Chen Fan]

At present, after hotplug a discontinuous cpu id on source side, then done
migration, hotplug again will fail on destination side. for example:
on source side:
   1) boot with -smp 1,maxcpus=4    
   2) cpu-add id=2
   3) live-migration
on destination side:
   1) boot with -smp 2,maxcpus=4
   1) cpu-add id=1

the root cause is the generated apicid is sequential from 0 to smp_cpus when
initialize cpus at booting time on destination side, there apicid will be 0 
and 1, but on source side the existed apicid after hotplug are 0 and 2.
so if add cpu with id=1, this will show error with this cpu alreay exists.

this patches added -smp X,apics=0x option to specify apic map. follow above
example:
on destination side:
   1) boot with -smp 2,maxcpus=4,apics=0xA
this apics value is a hex number as existed apicid bitmap, 0xA is 1010B for
apicid are 0 and 2.

this patches will be helpful for arbitrary CPU hot-remove as well.

Chen Fan (3):
  target-i386: moving registers of vmstate from cpu_exec_init() to
    x86_cpu_realizefn()
  target-i386: add -smp X,apics=0x option
  target-i386: add qmp command 'query-cpus' to display apic_id

 cpus.c                  |  1 +
 exec.c                  |  5 ++++
 hw/i386/pc.c            |  9 +++++--
 include/sysemu/sysemu.h |  4 ++++
 qapi-schema.json        |  4 +++-
 qemu-options.hx         | 15 +++++++++---
 target-i386/cpu.c       |  9 +++++++
 vl.c                    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++-
 8 files changed, 102 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn()

[Chen Fan]

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 <chen.fan.fnst@cn.fujitsu.com>
---
 exec.c            | 5 +++++
 target-i386/cpu.c | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/exec.c b/exec.c
index 7e49e8e..9be5855 100644
--- a/exec.c
+++ b/exec.c
@@ -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)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 967529a..dada6f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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);
 
     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);
+    }
 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-- 
1.8.1.4

Re: [Qemu-devel] [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn()

[Igor Mammedov]

On Tue, 14 Jan 2014 17:27:20 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> 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 <chen.fan.fnst@cn.fujitsu.com>
> ---
>  exec.c            | 5 +++++
>  target-i386/cpu.c | 9 +++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 7e49e8e..9be5855 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -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)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 967529a..dada6f6 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -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);
>  
>      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.

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

Re: [Qemu-devel] [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn()

[Chen Fan]

On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> On Tue, 14 Jan 2014 17:27:20 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> 
> > 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 <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  exec.c            | 5 +++++
> >  target-i386/cpu.c | 9 +++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 7e49e8e..9be5855 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -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)
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 967529a..dada6f6 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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);
> >  
> >      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()? 

> 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

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

Re: [Qemu-devel] [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn()

[Igor Mammedov]

On Wed, 15 Jan 2014 20:24:01 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > On Tue, 14 Jan 2014 17:27:20 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > 
> > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > ---
> > >  exec.c            | 5 +++++
> > >  target-i386/cpu.c | 9 +++++++++
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 7e49e8e..9be5855 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -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)
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 967529a..dada6f6 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -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);
> > >  
> > >      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()? 
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.

CCing Eduardo as hi might be interested in this as well.

> 
> > 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
> 
> > 
> > >  out:
> > >      if (local_err != NULL) {
> > >          error_propagate(errp, local_err);
> > 
> > 
> 
> 


-- 
Regards,
  Igor

[Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Eduardo Habkost]

On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> On Wed, 15 Jan 2014 20:24:01 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > > On Tue, 14 Jan 2014 17:27:20 +0800
> > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > 
> > > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > > ---
> > > >  exec.c            | 5 +++++
> > > >  target-i386/cpu.c | 9 +++++++++
> > > >  2 files changed, 14 insertions(+)
> > > > 
> > > > diff --git a/exec.c b/exec.c
> > > > index 7e49e8e..9be5855 100644
> > > > --- a/exec.c
> > > > +++ b/exec.c
> > > > @@ -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)
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 967529a..dada6f6 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -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);
> > > >  
> > > >      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()? 
> 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.

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

-- 
Eduardo

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Igor Mammedov]

On Fri, 17 Jan 2014 17:13:55 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > On Wed, 15 Jan 2014 20:24:01 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > > > On Tue, 14 Jan 2014 17:27:20 +0800
> > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > 
> > > > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > > > ---
> > > > >  exec.c            | 5 +++++
> > > > >  target-i386/cpu.c | 9 +++++++++
> > > > >  2 files changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index 7e49e8e..9be5855 100644
> > > > > --- a/exec.c
> > > > > +++ b/exec.c
> > > > > @@ -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)
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 967529a..dada6f6 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -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);
> > > > >  
> > > > >      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()? 
> > 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.
> 
> 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.

> 
> 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.


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

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Chen Fan]

On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> On Fri, 17 Jan 2014 17:13:55 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > > On Wed, 15 Jan 2014 20:24:01 +0800
> > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > > > > On Tue, 14 Jan 2014 17:27:20 +0800
> > > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > > 
> > > > > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > > > > ---
> > > > > >  exec.c            | 5 +++++
> > > > > >  target-i386/cpu.c | 9 +++++++++
> > > > > >  2 files changed, 14 insertions(+)
> > > > > > 
> > > > > > diff --git a/exec.c b/exec.c
> > > > > > index 7e49e8e..9be5855 100644
> > > > > > --- a/exec.c
> > > > > > +++ b/exec.c
> > > > > > @@ -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)
> > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > index 967529a..dada6f6 100644
> > > > > > --- a/target-i386/cpu.c
> > > > > > +++ b/target-i386/cpu.c
> > > > > > @@ -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);
> > > > > >  
> > > > > >      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()? 
> > > 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.
> > 
> > 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.
> 
> > 
> > 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.
> 
have there been any plan/model of the idea? Need to add a new option to
qemu command?

Thanks,
Chen

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

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Igor Mammedov]

On Tue, 21 Jan 2014 15:12:45 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> > On Fri, 17 Jan 2014 17:13:55 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > > > On Wed, 15 Jan 2014 20:24:01 +0800
> > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > > > > > On Tue, 14 Jan 2014 17:27:20 +0800
> > > > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > > > 
> > > > > > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > > > > > ---
> > > > > > >  exec.c            | 5 +++++
> > > > > > >  target-i386/cpu.c | 9 +++++++++
> > > > > > >  2 files changed, 14 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/exec.c b/exec.c
> > > > > > > index 7e49e8e..9be5855 100644
> > > > > > > --- a/exec.c
> > > > > > > +++ b/exec.c
> > > > > > > @@ -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)
> > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > index 967529a..dada6f6 100644
> > > > > > > --- a/target-i386/cpu.c
> > > > > > > +++ b/target-i386/cpu.c
> > > > > > > @@ -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);
> > > > > > >  
> > > > > > >      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()? 
> > > > 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.
> > > 
> > > 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.
> > 
> > > 
> > > 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.
> > 
> 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
 

> 
> Thanks,
> Chen
> 
> > 
> > > [1] See target-i386/topology.h for references
> > > 
> > 
> 
> 
> 

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Chen Fan]

On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> On Tue, 21 Jan 2014 15:12:45 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> 
> > On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> > > On Fri, 17 Jan 2014 17:13:55 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > > > > On Wed, 15 Jan 2014 20:24:01 +0800
> > > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > > > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote:
> > > > > > > On Tue, 14 Jan 2014 17:27:20 +0800
> > > > > > > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > > > > 
> > > > > > > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > > > > > > ---
> > > > > > > >  exec.c            | 5 +++++
> > > > > > > >  target-i386/cpu.c | 9 +++++++++
> > > > > > > >  2 files changed, 14 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/exec.c b/exec.c
> > > > > > > > index 7e49e8e..9be5855 100644
> > > > > > > > --- a/exec.c
> > > > > > > > +++ b/exec.c
> > > > > > > > @@ -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)
> > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > > index 967529a..dada6f6 100644
> > > > > > > > --- a/target-i386/cpu.c
> > > > > > > > +++ b/target-i386/cpu.c
> > > > > > > > @@ -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);
> > > > > > > >  
> > > > > > > >      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()? 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > > 
> > > > 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.
> > > 
> > 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
>  
Thanks, I hope I can take some time to make some patches to implement
it.

> 
> > 
> > Thanks,
> > Chen
> > 
> > > 
> > > > [1] See target-i386/topology.h for references
> > > > 
> > > 
> > 
> > 
> > 
> 

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Andreas Färber]

Am 21.01.2014 10:51, schrieb Chen Fan:
> On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
>> On Tue, 21 Jan 2014 15:12:45 +0800
>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
>>>> On Fri, 17 Jan 2014 17:13:55 -0200
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
>>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>>>
>>>>> 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.
>>>>
>>> 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
>>  
> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Chen Fan]

On Tue, 2014-01-21 at 11:10 +0100, Andreas Färber wrote:
> Am 21.01.2014 10:51, schrieb Chen Fan:
> > On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> >> On Tue, 21 Jan 2014 15:12:45 +0800
> >> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> >>>> On Fri, 17 Jan 2014 17:13:55 -0200
> >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> >>>>>> 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.
> >>>>>
> >>>>> 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.
> >>>>
> >>>>>
> >>>>> 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.
> >>>>
> >>> 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
> >>  
> > 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.
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

> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Igor Mammedov]

On Thu, 13 Feb 2014 14:14:08 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> On Tue, 2014-01-21 at 11:10 +0100, Andreas Färber wrote:
> > Am 21.01.2014 10:51, schrieb Chen Fan:
> > > On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> > >> On Tue, 21 Jan 2014 15:12:45 +0800
> > >> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > >>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> > >>>> On Fri, 17 Jan 2014 17:13:55 -0200
> > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > >>>>>> 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.
> > >>>>>
> > >>>>> 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.
> > >>>>
> > >>>>>
> > >>>>> 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.
> > >>>>
> > >>> 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
> > >>  
> > > 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.
> 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]/...


> Thanks,
> Chen
> 
> > 
> > Regards,
> > Andreas
> > 
> 
> 

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Chen Fan]

On Thu, 2014-02-13 at 10:44 +0100, Igor Mammedov wrote:
> On Thu, 13 Feb 2014 14:14:08 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> 
> > On Tue, 2014-01-21 at 11:10 +0100, Andreas Färber wrote:
> > > Am 21.01.2014 10:51, schrieb Chen Fan:
> > > > On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> > > >> On Tue, 21 Jan 2014 15:12:45 +0800
> > > >> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > >>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> > > >>>> On Fri, 17 Jan 2014 17:13:55 -0200
> > > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > > >>>>>> 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.
> > > >>>>>
> > > >>>>> 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.
> > > >>>>
> > > >>>>>
> > > >>>>> 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.
> > > >>>>
> > > >>> 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
> > > >>  
> > > > 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.
> > 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]/...
> 
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?


> 
> > Thanks,
> > Chen
> > 
> > > 
> > > Regards,
> > > Andreas
> > > 
> > 
> > 
> 
> 

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Igor Mammedov]

On Mon, 17 Feb 2014 18:24:09 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> On Thu, 2014-02-13 at 10:44 +0100, Igor Mammedov wrote:
> > On Thu, 13 Feb 2014 14:14:08 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > 
> > > On Tue, 2014-01-21 at 11:10 +0100, Andreas Färber wrote:
> > > > Am 21.01.2014 10:51, schrieb Chen Fan:
> > > > > On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> > > > >> On Tue, 21 Jan 2014 15:12:45 +0800
> > > > >> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > > > >>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> > > > >>>> On Fri, 17 Jan 2014 17:13:55 -0200
> > > > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> > > > >>>>>> 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.
> > > > >>>>>
> > > > >>>>> 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.
> > > > >>>>
> > > > >>>>>
> > > > >>>>> 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.
> > > > >>>>
> > > > >>> 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
> > > > >>  
> > > > > 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.
> > > 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]/...
> > 
> 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?
Regarding topology, I suppose -smp & -numa options should match on both,
that should give the same topology on target as on source.

As for specifying present CPUs on target, there is work in progress
to make CPUs work with -device/device_add (x86 cpu properties, x86 CPU
subclasses)

> 
> 
> > 
> > > Thanks,
> > > Chen
> > > 
> > > > 
> > > > Regards,
> > > > Andreas
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 0/2][RFC] prebuild cpu QOM tree /machine/node/socket/core/thread/..

[Chen Fan]

Hi,
    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 using QOM comand line:
  /machine/node/socket/core/thread, then migration, we could keep the same cpu
  infrastructure on both side.

  at first, I introduce a empty QOM tree /machine/node/socket/core/thread/, then when
  create cpu, link each created cpu into the tree. which idea was suggested by Igor. thanks.
  
  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. :)

Thanks,
Chen

Chen Fan (2):
  qom: introduce cpu QOM hierarchy tree
    /machine/node/socket/core/thread/cpu.
  cpu: link each new cpu to QOM tree
    /machine/node/socket/core/thread/cpu respectively.

 hw/i386/pc.c       |  40 ++++++++++++--
 include/qom/node.h |  66 +++++++++++++++++++++++
 qom/Makefile.objs  |   2 +-
 qom/node.c         | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 259 insertions(+), 5 deletions(-)
 create mode 100644 include/qom/node.h
 create mode 100644 qom/node.c

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2][RFC] qom: introduce cpu QOM hierarchy tree /machine/node/socket/core/thread/cpu.

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 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

diff --git a/include/qom/node.h b/include/qom/node.h
new file mode 100644
index 0000000..6c948ec
--- /dev/null
+++ b/include/qom/node.h
@@ -0,0 +1,66 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2013 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/qdev-core.h"
+
+#define TYPE_NODE   "node"
+#define TYPE_SOCKET "socket"
+#define TYPE_CORE   "core"
+#define TYPE_THREAD "thread"
+
+#define NODE(obj) OBJECT_CHECK(NodeState, (obj), TYPE_NODE)
+
+#define SOCKET(obj) OBJECT_CHECK(SocketState, (obj), TYPE_SOCKET)
+
+#define CORE(obj) OBJECT_CHECK(CoreState, (obj), TYPE_CORE)
+
+#define THREAD(obj) OBJECT_CHECK(ThreadState, (obj), TYPE_THREAD)
+
+Object *object_get_thread_from_index(int64_t cpu_index);
+
+typedef struct ThreadState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    CPUState **cpu;
+} ThreadState;
+
+typedef struct CoreState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} CoreState;
+
+typedef struct SocketState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} SocketState;
+
+typedef struct NodeState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} NodeState;
diff --git a/qom/Makefile.objs b/qom/Makefile.objs
index 985003b..5863919 100644
--- a/qom/Makefile.objs
+++ b/qom/Makefile.objs
@@ -1,3 +1,3 @@
 common-obj-y = object.o container.o qom-qobject.o
-common-obj-y += cpu.o
+common-obj-y += cpu.o node.o
 common-obj-y += object_interfaces.o
diff --git a/qom/node.c b/qom/node.c
new file mode 100644
index 0000000..a335992
--- /dev/null
+++ b/qom/node.c
@@ -0,0 +1,156 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2013 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "hw/hw.h"
+#include "sysemu/cpus.h"
+#include "sysemu/sysemu.h"
+#include "qom/cpu.h"
+#include "qom/node.h"
+
+Object *object_get_thread_from_index(int64_t cpu_index)
+{
+    gchar *path;
+    Object *thread;
+    int nodes, cpus_pre_nodes, smp_sockets;
+    unsigned node_id, socket_id, core_id, thread_id;
+    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;
+    node_id = socket_index / smp_sockets;
+    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;
+}
+
+static void thread_initfn(Object *obj)
+{
+    ThreadState *ts = THREAD(obj);
+
+    ts->cpu = g_malloc0(sizeof(CPUState *));
+
+    object_ref(OBJECT(ts->cpu));
+    object_property_add_link(obj, "cpu", TYPE_CPU, (Object **)ts->cpu, NULL);
+}
+
+static const TypeInfo thread_type_info = {
+    .name = TYPE_THREAD,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(ThreadState),
+    .instance_init = thread_initfn,
+};
+
+static void core_initfn(Object *obj)
+{
+    int i;
+    Object *thread_obj;
+    gchar *thread_name;
+
+    for (i = 0; i < smp_threads; i++) {
+        thread_obj = object_new(TYPE_THREAD);
+        thread_name = g_strdup_printf("thread[%" PRIi32 "]", i);
+        object_property_add_child(obj, thread_name, thread_obj, NULL);
+        g_free(thread_name);
+    }
+}
+
+static const TypeInfo core_type_info = {
+    .name = TYPE_CORE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(CoreState),
+    .instance_init = core_initfn,
+};
+
+static void socket_initfn(Object *obj)
+{
+    int i;
+    Object *core_obj;
+    gchar *core_name;
+
+    for (i = 0; i < smp_cores; i++) {
+        core_obj = object_new(TYPE_CORE);
+        core_name = g_strdup_printf("core[%" PRIi32 "]", i);
+        object_property_add_child(obj, core_name, core_obj, NULL);
+        g_free(core_name);
+    }
+}
+
+static const TypeInfo socket_type_info = {
+    .name = TYPE_SOCKET,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SocketState),
+    .instance_init = socket_initfn,
+};
+
+static void node_initfn(Object *obj)
+{
+    int smp_sockets, sockets_pre_node, nodes;
+    int i;
+    Object *socket_obj;
+    gchar *socket_name;
+
+    smp_sockets = max_cpus / (smp_cores * smp_threads);
+
+    nodes = nb_numa_nodes ? nb_numa_nodes : 1;
+
+    sockets_pre_node = smp_sockets / nodes;
+
+    for (i = 0; i < sockets_pre_node; i++) {
+        socket_obj = object_new(TYPE_SOCKET);
+        socket_name = g_strdup_printf("socket[%" PRIi32 "]", i);
+        object_property_add_child(obj, socket_name, socket_obj, NULL);
+        g_free(socket_name);
+    }
+}
+
+static const TypeInfo node_type_info = {
+    .name = TYPE_NODE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(NodeState),
+    .instance_init = node_initfn,
+};
+
+static void node_register_types(void)
+{
+    type_register_static(&node_type_info);
+    type_register_static(&socket_type_info);
+    type_register_static(&core_type_info);
+    type_register_static(&thread_type_info);
+}
+
+type_init(node_register_types)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2][RFC] qom: introduce cpu QOM hierarchy tree /machine/node/socket/core/thread/cpu.

[Eric Blake]

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

On 02/25/2014 02:07 AM, Chen Fan wrote:
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  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
> 
> diff --git a/include/qom/node.h b/include/qom/node.h
> new file mode 100644
> index 0000000..6c948ec
> --- /dev/null
> +++ b/include/qom/node.h
> @@ -0,0 +1,66 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2013 Fujitsu Ltd.

It's 2014 now.

-- 
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 1/2][RFC] qom: introduce cpu QOM hierarchy tree /machine/node/socket/core/thread/cpu.

[Chen Fan]

On Tue, 2014-02-25 at 06:35 -0700, Eric Blake wrote:
> On 02/25/2014 02:07 AM, Chen Fan wrote:
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  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
> > 
> > diff --git a/include/qom/node.h b/include/qom/node.h
> > new file mode 100644
> > index 0000000..6c948ec
> > --- /dev/null
> > +++ b/include/qom/node.h
> > @@ -0,0 +1,66 @@
> > +/*
> > + * QEMU System Emulator
> > + *
> > + * Copyright (c) 2013 Fujitsu Ltd.
> 
> It's 2014 now.
> 
oh, thanks.

Re: [Qemu-devel] [PATCH 1/2][RFC] qom: introduce cpu QOM hierarchy tree /machine/node/socket/core/thread/cpu.

[Eduardo Habkost]

On Tue, Feb 25, 2014 at 05:07:31PM +0800, Chen Fan wrote:
[...]
> +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.

> +{
> +    gchar *path;
> +    Object *thread;
> +    int nodes, cpus_pre_nodes, smp_sockets;
> +    unsigned node_id, socket_id, core_id, thread_id;
> +    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().

> +    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.


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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2][RFC] qom: introduce cpu QOM hierarchy tree /machine/node/socket/core/thread/cpu.

[Chen Fan]

On Wed, 2014-02-26 at 15:52 -0300, Eduardo Habkost wrote:
> On Tue, Feb 25, 2014 at 05:07:31PM +0800, Chen Fan wrote:
> [...]
> > +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.
> 
firstly, thanks for you important feedback.

I suppose that if we try to make CPU indexes unnecessary we should add
"-device cpufoo" support, and we should also need to add a "path" in
command line to let function pc_new_cpu() know the IDs which can be
calculated from the "path". then which one should the commend line be 
"-device cpufoo,sockets=X,cores=Y,threads=Z" or "-device cpufoo,paths=/
node-/socket-/core-/thread-/", or any other better?


> > +{
> > +    gchar *path;
> > +    Object *thread;
> > +    int nodes, cpus_pre_nodes, smp_sockets;
> > +    unsigned node_id, socket_id, core_id, thread_id;
> > +    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().
> 
> > +    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.
> 
> 
> > +    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;
> > +}
> > +
> 

[Qemu-devel] [RFC v2 0/2] prebuild cpu QOM tree /machine/node/socket/core/thread/..

[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 using QOM comand line:
/machine/node/socket/core/thread, then migration, we could keep the same cpu
infrastructure on both side.

at first, I introduce a empty QOM tree /machine/node/socket/core/thread/, then when
create cpu, link each created cpu into the tree. which idea was suggested by Igor. thanks.


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. :)


Chen Fan (2):
  i386: introduce "struct X86TopoInfo" for saving cpu topology
    information
  i386: introduce cpu QOM hierarchy tree

 hw/i386/pc.c               |  19 ++++--
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-qom.h      |   1 +
 target-i386/cpu-topology.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++++++
 target-i386/cpu.c          |  45 +++++++++++++
 target-i386/cpu.h          |   6 ++
 target-i386/topology.h     |   7 ++
 8 files changed, 308 insertions(+), 6 deletions(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

-- 
1.8.1.4

[Qemu-devel] [RFC v2 1/2] i386: introduce "struct X86TopoInfo" for saving cpu topology information

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 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(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 348b15f..a4d539e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);
 
     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);
@@ -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);
 }
 
 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);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..1858a66 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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)
+{
+    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;
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1b94f0f..1647394 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1283,6 +1283,10 @@ const char *get_register_name_32(unsigned int reg);
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
+#include "topology.h"
+
+X86TopoInfo *x86_cpu_topo_from_index(unsigned int cpu_index);
+
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..6f4886d 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -131,4 +131,11 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
     return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
 }
 
+typedef struct X86TopoInfo {
+    unsigned smt_id;
+    unsigned core_id;
+    unsigned pkg_id;
+    unsigned cpu_index;
+} X86TopoInfo;
+
 #endif /* TARGET_I386_TOPOLOGY_H */
-- 
1.8.1.4

Re: [Qemu-devel] [RFC v2 1/2] i386: introduce "struct X86TopoInfo" for saving cpu topology information

[Eduardo Habkost]

On Tue, Mar 04, 2014 at 06:50:24PM +0800, Chen Fan wrote:
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  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(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 348b15f..a4d539e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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.

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


>  }
>  
>  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);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0e8812a..1858a66 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -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.

> +{
> +    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;
> +}
[...]

-- 
Eduardo

Re: [Qemu-devel] [RFC v2 1/2] i386: introduce "struct X86TopoInfo" for saving cpu topology information

[Chen Fan]

On Tue, 2014-03-04 at 16:35 -0300, Eduardo Habkost wrote:
> On Tue, Mar 04, 2014 at 06:50:24PM +0800, Chen Fan wrote:
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  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(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 348b15f..a4d539e 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -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.
> 
> >  
> >      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);
> > @@ -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)
> 
Ok, that will be better. thanks.

> 
> >  }
> >  
> >  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);
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0e8812a..1858a66 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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.
I will have a try, thanks for you advice.


Thanks,
Chen

> 
> > +{
> > +    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;
> > +}
> [...]

[Qemu-devel] [RFC v3 0/3] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu

[chen.fan.fnst]

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

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.

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. :)

chen.fan.fnst (3):
  cpu: introduce CpuTopoInfo structure for argument simplification
  i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()
  i386: introduce cpu QOM hierarchy tree

 hw/i386/pc.c               |  15 +++-
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 198 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  15 +++-
 target-i386/cpu.h          |   6 ++
 target-i386/topology.h     |  33 ++++----
 7 files changed, 317 insertions(+), 23 deletions(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

-- 
1.8.1.4

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

[chen.fan.fnst]

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

Signed-off-by: Chen Fan <chen.fan.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..7843976 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -47,6 +47,12 @@
  */
 typedef uint32_t apic_id_t;
 
+typedef struct CpuTopoInfo {
+    unsigned pkg_id;
+    unsigned core_id;
+    unsigned smt_id;
+} CpuTopoInfo;
+
 /* 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)
+                                             CpuTopoInfo *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)
+                                         CpuTopoInfo *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);
+    CpuTopoInfo 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.8.1.4

Re: [Qemu-devel] [RFC v3 1/3] cpu: introduce CpuTopoInfo structure for argument simplification

[Eduardo Habkost]

On Tue, Mar 11, 2014 at 06:58:52PM +0800, chen.fan.fnst wrote:
> From: "chen.fan.fnst" <chen.fan.fnst@cn.fujitsu.com>
> 
> Signed-off-by: Chen Fan <chen.fan.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..7843976 100644
> --- a/target-i386/topology.h
> +++ b/target-i386/topology.h
> @@ -47,6 +47,12 @@
>   */
>  typedef uint32_t apic_id_t;
>  
> +typedef struct CpuTopoInfo {
> +    unsigned pkg_id;
> +    unsigned core_id;
> +    unsigned smt_id;
> +} CpuTopoInfo;

I suggest naming it "CPUTopoInfo". Maybe "X86CPUTopoInfo", to indicate
it is x86-specific.

The rest of the patch looks good.

-- 
Eduardo

[Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[chen.fan.fnst]

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

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c      | 12 ++++++++----
 target-i386/cpu.c |  2 --
 target-i386/cpu.h |  3 +++
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..c754042 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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) {
@@ -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);
+    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)
@@ -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);
+        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);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..3a5df33 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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"
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0014acc..8bc7b45 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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
-- 
1.8.1.4

Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Eduardo Habkost]

On Tue, Mar 11, 2014 at 06:58:53PM +0800, chen.fan.fnst wrote:
> From: "chen.fan.fnst" <chen.fan.fnst@cn.fujitsu.com>
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c      | 12 ++++++++----
>  target-i386/cpu.c |  2 --
>  target-i386/cpu.h |  3 +++
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e715a33..c754042 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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) {
> @@ -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()?


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



> +        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);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0e8812a..3a5df33 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -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"
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 0014acc..8bc7b45 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -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
> -- 
> 1.8.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Chen Fan]

On Tue, 2014-03-11 at 15:00 -0300, Eduardo Habkost wrote:
> On Tue, Mar 11, 2014 at 06:58:53PM +0800, chen.fan.fnst wrote:
> > From: "chen.fan.fnst" <chen.fan.fnst@cn.fujitsu.com>
> > 
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  hw/i386/pc.c      | 12 ++++++++----
> >  target-i386/cpu.c |  2 --
> >  target-i386/cpu.h |  3 +++
> >  3 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e715a33..c754042 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -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) {
> > @@ -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()?
> 
> 
> > +    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)
> > @@ -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);
>     }
I think we can write new x86_cpu_topo_ids_from_index() to do this
 compat work. I think I can make it soon.

> 
> And on pc_hot_add_cpu():
> 
>     if (apic_id_compat) {
>         error_setg(errp, "CPU hotplug is not supported on this machine");
>         return;
>     }
cpu hotplug supported since pc 1.5, and apic_id_compat is below pc 1.3,
so I think this must be unnecessary.

Thanks,
Chen


> 
> 
> 
> > +        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);
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0e8812a..3a5df33 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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"
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 0014acc..8bc7b45 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -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
> > -- 
> > 1.8.1.4
> > 
> 

[Qemu-devel] [RFC v4 0/3] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu

[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.

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. :)


Chen Fan (3):
  i386: introduce cpu QOM hierarchy tree
  cpu: introduce X86CPUTopoInfo structure for argument simplification
  i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

 hw/i386/pc.c               |  28 ++++---
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 199 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  41 ++++++++--
 target-i386/cpu.h          |   8 ++
 target-i386/topology.h     |  51 ++++++++----
 7 files changed, 367 insertions(+), 33 deletions(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

-- 
1.8.1.4

[Qemu-devel] [RFC v4 1/3] cpu: introduce CpuTopoInfo structure for argument simplification

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.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..9b811c1 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)
+                                             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.8.1.4

Re: [Qemu-devel] [RFC v4 1/3] cpu: introduce CpuTopoInfo structure for argument simplification

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 03:51:34PM +0800, Chen Fan wrote:
> Signed-off-by: Chen Fan <chen.fan.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..9b811c1 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)
> +                                             X86CPUTopoInfo *topo)

topo could be const X86CPUTopoInfo*, but not a big deal.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v1 0/4] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu

[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.

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

Chen Fan (4):
  cpu: introduce CpuTopoInfo structure for argument simplification
  i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()
  topo unit-test: update Unit tests to test-x86-cpuid.c
  i386: introduce cpu QOM hierarchy tree

 hw/i386/pc.c               |  25 +++---
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 199 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  55 +++++++++----
 target-i386/cpu.h          |   8 ++
 target-i386/topology.h     |  53 +++++++-----
 tests/test-x86-cpuid.c     | 165 +++++++++++++++++++++++++++++--------
 8 files changed, 494 insertions(+), 84 deletions(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

-- 
1.8.1.4

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

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.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.8.1.4

[Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[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 <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c           | 22 ++++++++++++----------
 target-i386/cpu.c      | 33 ++++++++++++++++++---------------
 target-i386/cpu.h      |  5 +++++
 target-i386/topology.h | 26 +++++++++++++++++---------
 4 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..84a017e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -927,7 +927,7 @@ 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, X86CPUTopoInfo *topo,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
@@ -939,7 +939,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
         return NULL;
     }
 
-    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
+    object_property_set_int(OBJECT(cpu), topo->apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
     if (local_err) {
@@ -955,28 +955,29 @@ static const char *current_cpu_model;
 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);
+    X86CPUTopoInfo topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);
         return;
     }
 
-    if (cpu_exists(apic_id)) {
+    if (id >= max_cpus) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", it already exists", id);
+                   ", max allowed: %d", id, max_cpus - 1);
         return;
     }
 
-    if (id >= max_cpus) {
+    x86_cpu_topo_ids_from_index(id, &topo);
+    if (cpu_exists(topo.apic_id)) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", max allowed: %d", id, max_cpus - 1);
+                   ", it already exists", id);
         return;
     }
 
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp);
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
@@ -996,8 +997,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);
+        X86CPUTopoInfo topo;
+        x86_cpu_topo_ids_from_index(i, &topo);
+        cpu = pc_new_cpu(cpu_model, &topo, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..6f2ba1c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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"
@@ -2584,6 +2582,21 @@ void enable_compat_apic_id_mode(void)
     compat_apic_id_mode = true;
 }
 
+void x86_cpu_topo_ids_from_index(unsigned int cpu_index, X86CPUTopoInfo *topo)
+{
+    static bool warned;
+
+    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index, topo);
+    if (compat_apic_id_mode) {
+        if (cpu_index != topo->apic_id && !warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            warned = true;
+        }
+        x86_topo_ids_from_apic_id(smp_cores, smp_threads, cpu_index, topo);
+    }
+}
+
 /* Calculates initial APIC ID for a specific CPU index
  *
  * Currently we need to be able to calculate the APIC ID from the CPU index
@@ -2593,20 +2606,10 @@ void enable_compat_apic_id_mode(void)
  */
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
 {
-    uint32_t correct_id;
-    static bool warned;
+    X86CPUTopoInfo topo;
 
-    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
-    if (compat_apic_id_mode) {
-        if (cpu_index != correct_id && !warned) {
-            error_report("APIC IDs set in compatibility mode, "
-                         "CPU topology won't match the configuration");
-            warned = true;
-        }
-        return cpu_index;
-    } else {
-        return correct_id;
-    }
+    x86_cpu_topo_ids_from_index(cpu_index, &topo);
+    return topo.apic_id;
 }
 
 static void x86_cpu_initfn(Object *obj)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0014acc..a410b16 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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
@@ -1286,6 +1289,8 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
 const char *get_register_name_32(unsigned int reg);
 
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
+void x86_cpu_topo_ids_from_index(unsigned int cpu_index,
+                                 X86CPUTopoInfo *topo);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
diff --git a/target-i386/topology.h b/target-i386/topology.h
index e9ff89c..e89e0cc 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -51,6 +51,7 @@ typedef struct X86CPUTopoInfo {
     unsigned pkg_id;
     unsigned core_id;
     unsigned smt_id;
+    apic_id_t apic_id;
 } X86CPUTopoInfo;
 
 /* Return the bit width needed for 'count' IDs
@@ -117,19 +118,26 @@ static inline void x86_topo_ids_from_idx(unsigned nr_cores,
     topo->smt_id = cpu_index % nr_threads;
     topo->core_id = core_index % nr_cores;
     topo->pkg_id = core_index / nr_cores;
+    topo->apic_id = apicid_from_topo_ids(nr_cores, nr_threads, topo);
 }
 
-/* Make APIC ID for the CPU 'cpu_index'
- *
- * 'cpu_index' is a sequential, contiguous ID for the CPU.
+/* Calculate CPU topology based on CPU APIC ID.
  */
-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)
 {
-    X86CPUTopoInfo topo;
-    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index, &topo);
-    return apicid_from_topo_ids(nr_cores, nr_threads, &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;
+    topo->apic_id = apic_id;
 }
 
 #endif /* TARGET_I386_TOPOLOGY_H */
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Eduardo Habkost]

On Wed, Mar 19, 2014 at 04:53:41PM +0800, Chen Fan wrote:
> 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 <chen.fan.fnst@cn.fujitsu.com>
[...]
> -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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Chen Fan]

On Wed, 2014-03-19 at 16:27 -0300, Eduardo Habkost wrote:
> On Wed, Mar 19, 2014 at 04:53:41PM +0800, Chen Fan wrote:
> > 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 <chen.fan.fnst@cn.fujitsu.com>
> [...]
> > -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.
Ok, this patches will come soon.

Thanks

> 
> Otherwise, patch looks good.
> 

[Qemu-devel] [PATCH v1 3/4] topo unit-test: update Unit tests to test-x86-cpuid.c

[Chen Fan]

remove redundant x86_apicid_from_cpu_idx() tests.
add tests to check x86_topo_ids_from_apic_id() and
x86_topo_ids_from_apic_id() output.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 tests/test-x86-cpuid.c | 165 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 129 insertions(+), 36 deletions(-)

diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 8d9f96a..4f33ecb 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -32,11 +32,36 @@ static void test_topo_bits(void)
     g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
     g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
 
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 0), ==, 0);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 1), ==, 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 2), ==, 2);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 3), ==, 3);
-
+    X86CPUTopoInfo topo = {0};
+
+    x86_topo_ids_from_idx(1, 1, 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 0);
+    x86_topo_ids_from_idx(1, 1, 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 1);
+    x86_topo_ids_from_idx(1, 1, 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 2);
+    x86_topo_ids_from_idx(1, 1, 3, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 3);
+
+    x86_topo_ids_from_apic_id(1, 1, 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+
+    x86_topo_ids_from_apic_id(1, 1, 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 1);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+
+    x86_topo_ids_from_apic_id(1, 1, 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 2);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+
+    x86_topo_ids_from_apic_id(1, 1, 3, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 3);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
 
     /* Test field width calculation for multiple values
      */
@@ -65,37 +90,105 @@ static void test_topo_bits(void)
     g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
     g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
 
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 0), ==, 0);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1), ==, 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2), ==, 2);
-
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 0), ==,
-                     (1 << 2) | 0);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 1), ==,
-                     (1 << 2) | 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 2), ==,
-                     (1 << 2) | 2);
-
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 0), ==,
-                     (2 << 2) | 0);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 1), ==,
-                     (2 << 2) | 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 2), ==,
-                     (2 << 2) | 2);
-
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 0), ==,
-                     (5 << 2) | 0);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 1), ==,
-                     (5 << 2) | 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 2), ==,
-                     (5 << 2) | 2);
-
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
-                     (1 << 5));
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
-                     (1 << 5) | (1 << 2) | 1);
-    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
-                     (3 << 5) | (5 << 2) | 2);
+    x86_topo_ids_from_idx(6, 3, 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 0);
+    x86_topo_ids_from_idx(6, 3, 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 1);
+    x86_topo_ids_from_idx(6, 3, 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, 2);
+
+    x86_topo_ids_from_idx(6, 3, 1 * 3 + 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (1 << 2) | 0);
+    x86_topo_ids_from_idx(6, 3, 1 * 3 + 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (1 << 2) | 1);
+    x86_topo_ids_from_idx(6, 3, 1 * 3 + 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (1 << 2) | 2);
+
+    x86_topo_ids_from_idx(6, 3, 2 * 3 + 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (2 << 2) | 0);
+    x86_topo_ids_from_idx(6, 3, 2 * 3 + 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (2 << 2) | 1);
+    x86_topo_ids_from_idx(6, 3, 2 * 3 + 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (2 << 2) | 2);
+
+    x86_topo_ids_from_idx(6, 3, 5 * 3 + 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (5 << 2) | 0);
+    x86_topo_ids_from_idx(6, 3, 5 * 3 + 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (5 << 2) | 1);
+    x86_topo_ids_from_idx(6, 3, 5 * 3 + 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (5 << 2) | 2);
+
+    x86_topo_ids_from_idx(6, 3, 1 * 6 * 3 + 0 * 3 + 0, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (1 << 5) | 0);
+    x86_topo_ids_from_idx(6, 3, 1 * 6 * 3 + 1 * 3 + 1, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (1 << 5) | (1 << 2) | 1);
+    x86_topo_ids_from_idx(6, 3, 3 * 6 * 3 + 5 * 3 + 2, &topo);
+    g_assert_cmpuint(topo.apic_id, ==, (3 << 5) | (5 << 2) | 2);
+
+    x86_topo_ids_from_apic_id(6, 3, 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+    x86_topo_ids_from_apic_id(6, 3, 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 1);
+    x86_topo_ids_from_apic_id(6, 3, 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 2);
+
+    x86_topo_ids_from_apic_id(6, 3, (1 << 2) | 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 1);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+    x86_topo_ids_from_apic_id(6, 3, (1 << 2) | 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 1);
+    g_assert_cmpuint(topo.smt_id,  ==, 1);
+    x86_topo_ids_from_apic_id(6, 3, (1 << 2) | 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 1);
+    g_assert_cmpuint(topo.smt_id,  ==, 2);
+
+    x86_topo_ids_from_apic_id(6, 3, (2 << 2) | 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 2);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+    x86_topo_ids_from_apic_id(6, 3, (2 << 2) | 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 2);
+    g_assert_cmpuint(topo.smt_id,  ==, 1);
+    x86_topo_ids_from_apic_id(6, 3, (2 << 2) | 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 2);
+    g_assert_cmpuint(topo.smt_id,  ==, 2);
+
+    x86_topo_ids_from_apic_id(6, 3, (5 << 2) | 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 5);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+    x86_topo_ids_from_apic_id(6, 3, (5 << 2) | 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 5);
+    g_assert_cmpuint(topo.smt_id,  ==, 1);
+    x86_topo_ids_from_apic_id(6, 3, (5 << 2) | 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 0);
+    g_assert_cmpuint(topo.core_id, ==, 5);
+    g_assert_cmpuint(topo.smt_id,  ==, 2);
+
+    x86_topo_ids_from_apic_id(6, 3, (1 << 5) | 0, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 1);
+    g_assert_cmpuint(topo.core_id, ==, 0);
+    g_assert_cmpuint(topo.smt_id,  ==, 0);
+    x86_topo_ids_from_apic_id(6, 3, (1 << 5) | (1 << 2) | 1, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 1);
+    g_assert_cmpuint(topo.core_id, ==, 1);
+    g_assert_cmpuint(topo.smt_id,  ==, 1);
+    x86_topo_ids_from_apic_id(6, 3, (1 << 5) | (1 << 2) | 2, &topo);
+    g_assert_cmpuint(topo.pkg_id,  ==, 1);
+    g_assert_cmpuint(topo.core_id, ==, 1);
+    g_assert_cmpuint(topo.smt_id,  ==, 2);
 }
 
 int main(int argc, char **argv)
-- 
1.8.1.4

[Qemu-devel] [PATCH v1 4/4] i386: introduce cpu QOM hierarchy tree

[Chen Fan]

add cpu-topology.h cpu-topology.c files for prebuilding cpu qom tree
"/machine/node[X]/socket[Y]/core[Z]->link cpu"

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c               |   3 +
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 199 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  22 +++++
 target-i386/cpu.h          |   3 +
 6 files changed, 299 insertions(+), 1 deletion(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 84a017e..8a489f5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -941,6 +941,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, X86CPUTopoInfo *topo,
 
     object_property_set_int(OBJECT(cpu), topo->apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+    x86_topo_cpu_set_link(OBJECT(cpu), topo, &local_err);
 
     if (local_err) {
         error_propagate(errp, local_err);
@@ -996,6 +997,8 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    cpu_topo_init();
+
     for (i = 0; i < smp_cpus; i++) {
         X86CPUTopoInfo topo;
         x86_cpu_topo_ids_from_index(i, &topo);
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index 027b94e..239474d 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += translate.o helper.o cpu.o
+obj-y += translate.o helper.o cpu.o cpu-topology.o
 obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-y += gdbstub.o
diff --git a/target-i386/cpu-topology.c b/target-i386/cpu-topology.c
new file mode 100644
index 0000000..e611bae
--- /dev/null
+++ b/target-i386/cpu-topology.c
@@ -0,0 +1,199 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "hw/hw.h"
+#include "sysemu/cpus.h"
+#include "sysemu/sysemu.h"
+#include "cpu-topology.h"
+
+static QTAILQ_HEAD(, SocketState) sockets = QTAILQ_HEAD_INITIALIZER(sockets);
+static QTAILQ_HEAD(, NodeState) nodes = QTAILQ_HEAD_INITIALIZER(nodes);
+
+static NodeState *node_get(int node_id)
+{
+    NodeState *node;
+
+    QTAILQ_FOREACH(node, &nodes, next) {
+        if (node->node_id == node_id) {
+            return node;
+        }
+    }
+    return NULL;
+}
+
+static SocketState *cpu_socket_find(int pkg_id)
+{
+    SocketState *ss;
+
+    QTAILQ_FOREACH(ss, &sockets, next) {
+        if (ss->socket_id == pkg_id) {
+            return ss;
+        }
+    }
+    return NULL;
+}
+
+void cpu_topo_init(void)
+{
+    unsigned long *node_mask;
+    int i;
+
+    node_mask = bitmap_new(MAX_NODES);
+
+    for (i = 0; i < max_cpus; i++) {
+        NodeState *node;
+        SocketState *ss;
+        gchar *name;
+        int node_id = 0, socket_id;
+        int j;
+
+        for (j = 0; j < nb_numa_nodes; j++) {
+            if (test_bit(i, node_cpumask[j])) {
+                node_id = j;
+                break;
+            }
+        }
+
+        if (test_bit(node_id, node_mask)) {
+            node = node_get(node_id);
+        } else {
+            node = NODE(object_new(TYPE_NODE));
+            node->node_id = node_id;
+            name = g_strdup_printf("node[%" PRIu32 "]", node_id);
+            object_property_add_child(qdev_get_machine(), name,
+                                      OBJECT(node), NULL);
+            set_bit(node_id, node_mask);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&nodes, node, next);
+        }
+
+        socket_id = i / (smp_cores * smp_threads);
+        ss = cpu_socket_find(socket_id);
+        if (!ss) {
+            ss = SOCKET(object_new(TYPE_SOCKET));
+            ss->socket_id = socket_id;
+            name = g_strdup_printf("socket[%" PRIu32 "]", socket_id);
+            object_property_add_child(OBJECT(node), name,
+                                      OBJECT(ss), NULL);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&sockets, ss, next);
+        }
+    }
+
+    g_free(node_mask);
+}
+
+CoreState *cpu_topo_object_core_find(X86CPUTopoInfo *topo)
+{
+    SocketState *ss;
+    CoreState *core;
+
+    ss = cpu_socket_find(topo->pkg_id);
+    if (!ss) {
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(core, &ss->children, sibling) {
+        if (core->core_id == topo->core_id) {
+            return core;
+        }
+    }
+    return NULL;
+}
+
+static void socket_initfn(Object *obj)
+{
+    SocketState *ss = SOCKET(obj);
+    int i;
+
+    QTAILQ_INIT(&ss->children);
+
+    for (i = 0; i < smp_cores; i++) {
+        gchar *name;
+        CoreState *core;
+
+        core = CORE(object_new(TYPE_CORE));
+        core->core_id = i;
+        QTAILQ_INSERT_TAIL(&ss->children, core, sibling);
+
+        name = g_strdup_printf("core[%" PRIu32 "]", i);
+        object_property_add_child(obj, name, OBJECT(core), NULL);
+        g_free(name);
+    }
+}
+
+static void core_initfn(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+    gchar *name;
+    int i;
+
+    cs->link_cpu = g_malloc0(sizeof(CPUState *) * smp_threads);
+    for (i = 0; i < smp_threads; i++) {
+        name = g_strdup_printf("cpu[%" PRIu32 "]", i);
+        object_property_add_link(obj, name, TYPE_CPU,
+                                 (Object **)&cs->link_cpu[i], NULL);
+        g_free(name);
+    }
+}
+
+static void core_fini(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+
+    g_free(cs->link_cpu);
+}
+
+static const TypeInfo core_type_info = {
+    .name = TYPE_CORE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(CoreState),
+    .instance_init = core_initfn,
+    .instance_finalize = core_fini,
+};
+
+static const TypeInfo socket_type_info = {
+    .name = TYPE_SOCKET,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SocketState),
+    .instance_init = socket_initfn,
+};
+
+static const TypeInfo node_type_info = {
+    .name = TYPE_NODE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(NodeState),
+};
+
+static void node_register_types(void)
+{
+    type_register_static(&node_type_info);
+    type_register_static(&socket_type_info);
+    type_register_static(&core_type_info);
+}
+
+type_init(node_register_types)
diff --git a/target-i386/cpu-topology.h b/target-i386/cpu-topology.h
new file mode 100644
index 0000000..6465d94
--- /dev/null
+++ b/target-i386/cpu-topology.h
@@ -0,0 +1,71 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_CPU_TOPOLOGY_H
+#define TARGET_I386_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "cpu.h"
+#include "qom/cpu.h"
+
+#define TYPE_NODE   "node"
+#define TYPE_SOCKET "socket"
+#define TYPE_CORE   "core"
+
+#define NODE(obj) OBJECT_CHECK(NodeState, (obj), TYPE_NODE)
+
+#define SOCKET(obj) OBJECT_CHECK(SocketState, (obj), TYPE_SOCKET)
+
+#define CORE(obj) OBJECT_CHECK(CoreState, (obj), TYPE_CORE)
+
+typedef struct CoreState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int core_id;
+    QTAILQ_ENTRY(CoreState) sibling;
+    CPUState **link_cpu;
+} CoreState;
+
+typedef struct SocketState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    unsigned socket_id;
+    QTAILQ_HEAD(ChildHread, CoreState) children;
+    QTAILQ_ENTRY(SocketState) next;
+} SocketState;
+
+typedef struct NodeState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int node_id;
+    QTAILQ_ENTRY(NodeState) next;
+} NodeState;
+
+CoreState *cpu_topo_object_core_find(X86CPUTopoInfo *topo);
+void cpu_topo_init(void);
+
+#endif /* TARGET_I386_CPU_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6f2ba1c..9754ac2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2698,6 +2698,28 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
+void x86_topo_cpu_set_link(Object *obj, X86CPUTopoInfo *topo, Error **errp)
+{
+    gchar *name;
+    CoreState *core;
+    Error *local_err = NULL;
+
+    core = cpu_topo_object_core_find(topo);
+    if (!core) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, TYPE_CORE);
+        goto out;
+    }
+
+    name = g_strdup_printf("cpu[%" PRIu32 "]", topo->smt_id);
+    object_property_set_link(OBJECT(core), obj, name, &local_err);
+    g_free(name);
+
+out:
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+    }
+}
+
 static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a410b16..42db0fd 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -24,6 +24,7 @@
 
 #include "sysemu/cpus.h"
 #include "topology.h"
+#include "cpu-topology.h"
 
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
@@ -1293,6 +1294,8 @@ void x86_cpu_topo_ids_from_index(unsigned int cpu_index,
                                  X86CPUTopoInfo *topo);
 void enable_compat_apic_id_mode(void);
 
+void x86_topo_cpu_set_link(Object *obj, X86CPUTopoInfo *topo, Error **errp);
+
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v1 0/4] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu

[Eric Blake]

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

On 03/19/2014 02:53 AM, Chen Fan wrote:
> 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. :)

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.

-- 
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 v1 0/4] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu

[Chen Fan]

On Wed, 2014-03-19 at 06:00 -0600, Eric Blake wrote:
> On 03/19/2014 02:53 AM, Chen Fan wrote:
> > 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. :)
> 
> 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.
> 
I see, Thanks.

Chen

[Qemu-devel] [RFC v4 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c           | 25 ++++++++++++++-----------
 target-i386/cpu.c      | 28 +++++++++++++++++++++++-----
 target-i386/cpu.h      |  5 +++++
 target-i386/topology.h | 18 ++++++++++++++++++
 4 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..765b634 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -927,11 +927,18 @@ 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, X86CPUTopoInfo *topo,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
+    int64_t apic_id = apicid_from_topo_ids(smp_cores, smp_threads, topo);
+
+    if (cpu_exists(apic_id)) {
+        error_setg(errp, "Unable to add CPU with APIC ID: %" PRIi64
+                   ", it already exists", apic_id);
+        return NULL;
+    }
 
     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {
@@ -955,19 +962,13 @@ static const char *current_cpu_model;
 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);
+    X86CPUTopoInfo topo;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);
         return;
     }
 
-    if (cpu_exists(apic_id)) {
-        error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", it already exists", id);
-        return;
-    }
-
     if (id >= max_cpus) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
                    ", max allowed: %d", id, max_cpus - 1);
@@ -976,7 +977,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_cpu_topo_ids_from_index(id, &topo);
+    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp);
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
@@ -996,8 +998,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);
+        X86CPUTopoInfo topo;
+        x86_cpu_topo_ids_from_index(i, &topo);
+        cpu = pc_new_cpu(cpu_model, &topo, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..d8ad484 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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"
@@ -2584,6 +2582,8 @@ void enable_compat_apic_id_mode(void)
     compat_apic_id_mode = true;
 }
 
+static bool compat_apic_id_warned;
+
 /* Calculates initial APIC ID for a specific CPU index
  *
  * Currently we need to be able to calculate the APIC ID from the CPU index
@@ -2594,14 +2594,13 @@ void enable_compat_apic_id_mode(void)
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
 {
     uint32_t correct_id;
-    static bool warned;
 
     correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
     if (compat_apic_id_mode) {
-        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;
         }
         return cpu_index;
     } else {
@@ -2609,6 +2608,25 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
+void x86_cpu_topo_ids_from_index(unsigned int cpu_index, X86CPUTopoInfo *topo)
+{
+    int64_t correct_apic_id;
+
+    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index, topo);
+    if (compat_apic_id_mode) {
+        correct_apic_id = apicid_from_topo_ids(smp_cores,
+                                               smp_threads,
+                                               topo);
+        if (cpu_index != correct_apic_id && !compat_apic_id_warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            compat_apic_id_warned = true;
+        }
+        x86_topo_ids_from_apic_id(smp_cores, smp_threads, cpu_index, topo);
+        assert(apicid_from_topo_ids(smp_cores, smp_threads, topo) == cpu_index);
+    }
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0014acc..a410b16 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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
@@ -1286,6 +1289,8 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
 const char *get_register_name_32(unsigned int reg);
 
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
+void x86_cpu_topo_ids_from_index(unsigned int cpu_index,
+                                 X86CPUTopoInfo *topo);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
diff --git a/target-i386/topology.h b/target-i386/topology.h
index 9b811c1..6f7eebb 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.8.1.4

Re: [Qemu-devel] [RFC v4 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Eduardo Habkost]

On Wed, Mar 12, 2014 at 03:51:35PM +0800, Chen Fan wrote:
[...]
>      if (compat_apic_id_mode) {
> -        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;
[...]
> +    if (compat_apic_id_mode) {
> +        correct_apic_id = apicid_from_topo_ids(smp_cores,
> +                                               smp_threads,
> +                                               topo);
> +        if (cpu_index != correct_apic_id && !compat_apic_id_warned) {
> +            error_report("APIC IDs set in compatibility mode, "
> +                         "CPU topology won't match the configuration");
> +            compat_apic_id_warned = true;

The new logic makes sense, but please don't duplicate code.

-- 
Eduardo

[Qemu-devel] [RFC v4 3/3] i386: introduce cpu QOM hierarchy tree

[Chen Fan]

add cpu-topology.h cpu-topology.c files for prebuilding cpu qom tree
 "/machine/node[X]/socket[Y]/core[Z]->link cpu"

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c               |   3 +
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 199 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  13 +++
 target-i386/cpu.h          |   3 +
 6 files changed, 290 insertions(+), 1 deletion(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 765b634..22e81be 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, X86CPUTopoInfo *topo,
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+    x86_topo_cpu_set_link(OBJECT(cpu), topo);
 
     if (local_err) {
         error_propagate(errp, local_err);
@@ -997,6 +998,8 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    cpu_topo_init();
+
     for (i = 0; i < smp_cpus; i++) {
         X86CPUTopoInfo topo;
         x86_cpu_topo_ids_from_index(i, &topo);
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index 027b94e..239474d 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += translate.o helper.o cpu.o
+obj-y += translate.o helper.o cpu.o cpu-topology.o
 obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-y += gdbstub.o
diff --git a/target-i386/cpu-topology.c b/target-i386/cpu-topology.c
new file mode 100644
index 0000000..707f080
--- /dev/null
+++ b/target-i386/cpu-topology.c
@@ -0,0 +1,199 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "hw/hw.h"
+#include "sysemu/cpus.h"
+#include "sysemu/sysemu.h"
+#include "cpu-topology.h"
+
+static QTAILQ_HEAD(, SocketState) sockets = QTAILQ_HEAD_INITIALIZER(sockets);
+static QTAILQ_HEAD(, NodeState) nodes = QTAILQ_HEAD_INITIALIZER(nodes);
+
+static NodeState *node_get(int node_id)
+{
+    NodeState *node;
+
+    QTAILQ_FOREACH(node, &nodes, next) {
+        if (node->node_id == node_id) {
+            return node;
+        }
+    }
+    return NULL;
+}
+
+static SocketState *cpu_socket_find(int pkg_id)
+{
+    SocketState *ss;
+
+    QTAILQ_FOREACH(ss, &sockets, next) {
+        if (ss->socket_id == pkg_id) {
+            return ss;
+        }
+    }
+    return NULL;
+}
+
+void cpu_topo_init(void)
+{
+    unsigned long *node_mask;
+    int i;
+
+    node_mask = bitmap_new(MAX_NODES);
+
+    for (i = 0; i < max_cpus; i++) {
+        NodeState *node;
+        SocketState *ss;
+        gchar *name;
+        int node_id = 0, socket_id;
+        int j;
+
+        for (j = 0; j < nb_numa_nodes; j++) {
+            if (test_bit(i, node_cpumask[j])) {
+                node_id = j;
+                break;
+            }
+        }
+
+        if (test_bit(node_id, node_mask)) {
+            node = node_get(node_id);
+        } else {
+            node = NODE(object_new(TYPE_NODE));
+            node->node_id = node_id;
+            name = g_strdup_printf("node[%" PRIu32 "]", node_id);
+            object_property_add_child(qdev_get_machine(), name,
+                                      OBJECT(node), NULL);
+            set_bit(node_id, node_mask);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&nodes, node, next);
+        }
+
+        socket_id = i / (smp_cores * smp_threads);
+        ss = cpu_socket_find(socket_id);
+        if (!ss) {
+            ss = SOCKET(object_new(TYPE_SOCKET));
+            ss->socket_id = socket_id;
+            name = g_strdup_printf("socket[%" PRIu32 "]", socket_id);
+            object_property_add_child(OBJECT(node), name,
+                                      OBJECT(ss), NULL);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&sockets, ss, next);
+        }
+    }
+
+    g_free(node_mask);
+}
+
+CoreState *cpu_topo_object_core_find(X86CPUTopoInfo *topo)
+{
+    SocketState *ss;
+    CoreState *core;
+
+    ss = cpu_socket_find(topo->pkg_id);
+    if (!ss) {
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(core, &ss->children, sibling) {
+        if (core->core_id == topo->core_id) {
+            return core;
+        }
+    }
+    return NULL;
+}
+
+static void socket_initfn(Object *obj)
+{
+    SocketState *ss = SOCKET(obj);
+    int i;
+
+    QTAILQ_INIT(&ss->children);
+
+    for (i = 0; i < smp_cores; i++) {
+        gchar *name;
+        CoreState *core;
+
+        core = CORE(object_new(TYPE_CORE));
+        core->core_id = i;
+        QTAILQ_INSERT_TAIL(&ss->children, core, sibling);
+
+        name = g_strdup_printf("core[%" PRIu32 "]", i);
+        object_property_add_child(obj, name, OBJECT(core), NULL);
+        g_free(name);
+    }
+}
+
+static void core_initfn(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+    gchar *name;
+    int i;
+
+    cs->link_cpu = g_malloc0(sizeof(CPUState *) * smp_threads);
+    for (i = 0; i < smp_threads; i++) {
+        name = g_strdup_printf("cpu[%" PRIu32 "]", i);
+        object_property_add_link(obj, name, TYPE_CPU,
+                                 (Object **)&cs->link_cpu[i], NULL);
+        g_free(name);
+    }
+}
+
+static void core_fini(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+
+    g_free(cs->link_cpu);
+}
+
+static const TypeInfo core_type_info = {
+    .name = TYPE_CORE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(CoreState),
+    .instance_init = core_initfn,
+    .instance_finalize = core_fini,
+};
+
+static const TypeInfo socket_type_info = {
+    .name = TYPE_SOCKET,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SocketState),
+    .instance_init = socket_initfn,
+};
+
+static const TypeInfo node_type_info = {
+    .name = TYPE_NODE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(NodeState),
+};
+
+static void node_register_types(void)
+{
+    type_register_static(&node_type_info);
+    type_register_static(&socket_type_info);
+    type_register_static(&core_type_info);
+}
+
+type_init(node_register_types)
diff --git a/target-i386/cpu-topology.h b/target-i386/cpu-topology.h
new file mode 100644
index 0000000..6465d94
--- /dev/null
+++ b/target-i386/cpu-topology.h
@@ -0,0 +1,71 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_CPU_TOPOLOGY_H
+#define TARGET_I386_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "cpu.h"
+#include "qom/cpu.h"
+
+#define TYPE_NODE   "node"
+#define TYPE_SOCKET "socket"
+#define TYPE_CORE   "core"
+
+#define NODE(obj) OBJECT_CHECK(NodeState, (obj), TYPE_NODE)
+
+#define SOCKET(obj) OBJECT_CHECK(SocketState, (obj), TYPE_SOCKET)
+
+#define CORE(obj) OBJECT_CHECK(CoreState, (obj), TYPE_CORE)
+
+typedef struct CoreState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int core_id;
+    QTAILQ_ENTRY(CoreState) sibling;
+    CPUState **link_cpu;
+} CoreState;
+
+typedef struct SocketState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    unsigned socket_id;
+    QTAILQ_HEAD(ChildHread, CoreState) children;
+    QTAILQ_ENTRY(SocketState) next;
+} SocketState;
+
+typedef struct NodeState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int node_id;
+    QTAILQ_ENTRY(NodeState) next;
+} NodeState;
+
+CoreState *cpu_topo_object_core_find(X86CPUTopoInfo *topo);
+void cpu_topo_init(void);
+
+#endif /* TARGET_I386_CPU_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d8ad484..de2994c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2713,6 +2713,19 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
+void x86_topo_cpu_set_link(Object *obj, X86CPUTopoInfo *topo)
+{
+    gchar *name;
+    CoreState *core;
+
+    core = cpu_topo_object_core_find(topo);
+    if (core) {
+        name = g_strdup_printf("cpu[%" PRIu32 "]", topo->smt_id);
+        object_property_set_link(OBJECT(core), obj, name, NULL);
+        g_free(name);
+    }
+}
+
 static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a410b16..d7f74e2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -24,6 +24,7 @@
 
 #include "sysemu/cpus.h"
 #include "topology.h"
+#include "cpu-topology.h"
 
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
@@ -1293,6 +1294,8 @@ void x86_cpu_topo_ids_from_index(unsigned int cpu_index,
                                  X86CPUTopoInfo *topo);
 void enable_compat_apic_id_mode(void);
 
+void x86_topo_cpu_set_link(Object *obj, X86CPUTopoInfo *topo);
+
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
-- 
1.8.1.4

[Qemu-devel] [RFC v3 3/3] i386: introduce cpu QOM hierarchy tree

[chen.fan.fnst]

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

add cpu-topology.h cpu-topology.c files for prebuilding cpu qom tree
"/machine/node[X]/socket[Y]/core[Z]->link cpu"

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c               |   3 +
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-topology.c | 198 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++
 target-i386/cpu.c          |  13 +++
 target-i386/cpu.h          |   3 +
 6 files changed, 289 insertions(+), 1 deletion(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c754042..9c8ba80 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -942,6 +942,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, CpuTopoInfo *topo,
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
+    x86_topo_cpu_set_link(OBJECT(cpu), topo);
 
     if (local_err) {
         error_propagate(errp, local_err);
@@ -998,6 +999,8 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    cpu_topo_init();
+
     for (i = 0; i < smp_cpus; i++) {
         CpuTopoInfo topo;
         x86_topo_ids_from_idx(smp_cores, smp_threads, i, &topo);
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index 027b94e..239474d 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += translate.o helper.o cpu.o
+obj-y += translate.o helper.o cpu.o cpu-topology.o
 obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-y += gdbstub.o
diff --git a/target-i386/cpu-topology.c b/target-i386/cpu-topology.c
new file mode 100644
index 0000000..388ce85
--- /dev/null
+++ b/target-i386/cpu-topology.c
@@ -0,0 +1,198 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "hw/hw.h"
+#include "sysemu/cpus.h"
+#include "sysemu/sysemu.h"
+#include "cpu-topology.h"
+
+static QTAILQ_HEAD(, SocketState) sockets = QTAILQ_HEAD_INITIALIZER(sockets);
+static QTAILQ_HEAD(, NodeState) nodes = QTAILQ_HEAD_INITIALIZER(nodes);
+
+static NodeState *node_get(int node_id)
+{
+    NodeState *node;
+
+    QTAILQ_FOREACH(node, &nodes, next) {
+        if (node->node_id == node_id) {
+            return node;
+        }
+    }
+    return NULL;
+}
+
+static SocketState *cpu_socket_find(int pkg_id)
+{
+    SocketState *ss;
+
+    QTAILQ_FOREACH(ss, &sockets, next) {
+        if (ss->socket_id == pkg_id) {
+            return ss;
+        }
+    }
+    return NULL;
+}
+
+void cpu_topo_init(void)
+{
+    unsigned long *node_mask;
+    int i;
+
+    node_mask = bitmap_new(MAX_NODES);
+
+    for (i = 0; i < max_cpus; i++) {
+        NodeState *node;
+        SocketState *ss;
+        gchar *name;
+        int node_id = 0, socket_id;
+        int j;
+
+        for (j = 0; j < nb_numa_nodes; j++) {
+            if (test_bit(i, node_cpumask[j])) {
+                node_id = j;
+                break;
+            }
+        }
+
+        if (test_bit(node_id, node_mask)) {
+            node = node_get(node_id);
+        } else {
+            node = NODE(object_new(TYPE_NODE));
+            node->node_id = node_id;
+            name = g_strdup_printf("node[%" PRIu32 "]", node_id);
+            object_property_add_child(qdev_get_machine(), name,
+                                      OBJECT(node), NULL);
+            set_bit(node_id, node_mask);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&nodes, node, next);
+        }
+
+        socket_id = i / (smp_cores * smp_threads);
+        ss = cpu_socket_find(socket_id);
+        if (!ss) {
+            ss = SOCKET(object_new(TYPE_SOCKET));
+            ss->socket_id = socket_id;
+            name = g_strdup_printf("socket[%" PRIu32 "]",socket_id);
+            object_property_add_child(OBJECT(node), name,
+                                      OBJECT(ss), NULL);
+            g_free(name);
+            QTAILQ_INSERT_TAIL(&sockets, ss, next);
+        }
+    }
+
+    g_free(node_mask);
+}
+
+CoreState *cpu_topo_object_core_find(CpuTopoInfo *topo)
+{
+    SocketState *ss;
+    CoreState *core;
+
+    ss = cpu_socket_find(topo->pkg_id);
+    if (!ss) {
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(core, &ss->children, sibling) {
+        if (core->core_id == topo->core_id) {
+            return core;
+        }
+    }
+    return NULL;
+}
+
+static void socket_initfn(Object *obj)
+{
+    SocketState *ss = SOCKET(obj);
+    int i;
+
+    QTAILQ_INIT(&ss->children);
+
+    for (i= 0; i < smp_cores; i++) {
+        gchar *name;
+        CoreState *core;
+
+        core = CORE(object_new(TYPE_CORE));
+        QTAILQ_INSERT_TAIL(&ss->children, core, sibling);
+
+        name = g_strdup_printf("core[%" PRIu32 "]", i);
+        object_property_add_child(obj, name, OBJECT(core), NULL);
+        g_free(name);
+    }
+}
+
+static void core_initfn(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+    gchar *name;
+    int i;
+
+    cs->link_cpu = g_malloc0(sizeof(CPUState*) * smp_threads);
+    for (i = 0; i < smp_threads; i++) {
+        name = g_strdup_printf("cpu[%" PRIu32 "]", i);
+        object_property_add_link(obj, name, TYPE_CPU,
+                                 (Object **)&cs->link_cpu[i], NULL);
+        g_free(name);
+    }
+}
+
+static void core_fini(Object *obj)
+{
+    CoreState *cs = CORE(obj);
+
+    g_free(cs->link_cpu);
+}
+
+static const TypeInfo core_type_info = {
+    .name = TYPE_CORE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(CoreState),
+    .instance_init = core_initfn,
+    .instance_finalize = core_fini,
+};
+
+static const TypeInfo socket_type_info = {
+    .name = TYPE_SOCKET,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SocketState),
+    .instance_init = socket_initfn,
+};
+
+static const TypeInfo node_type_info = {
+    .name = TYPE_NODE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(NodeState),
+};
+
+static void node_register_types(void)
+{
+    type_register_static(&node_type_info);
+    type_register_static(&socket_type_info);
+    type_register_static(&core_type_info);
+}
+
+type_init(node_register_types)
diff --git a/target-i386/cpu-topology.h b/target-i386/cpu-topology.h
new file mode 100644
index 0000000..837b0e9
--- /dev/null
+++ b/target-i386/cpu-topology.h
@@ -0,0 +1,71 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_CPU_TOPOLOGY_H
+#define TARGET_I386_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "cpu.h"
+#include "qom/cpu.h"
+
+#define TYPE_NODE   "node"
+#define TYPE_SOCKET "socket"
+#define TYPE_CORE   "core"
+
+#define NODE(obj) OBJECT_CHECK(NodeState, (obj), TYPE_NODE)
+
+#define SOCKET(obj) OBJECT_CHECK(SocketState, (obj), TYPE_SOCKET)
+
+#define CORE(obj) OBJECT_CHECK(CoreState, (obj), TYPE_CORE)
+
+typedef struct CoreState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int core_id;
+    QTAILQ_ENTRY(CoreState) sibling;
+    CPUState **link_cpu;
+} CoreState;
+
+typedef struct SocketState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    unsigned socket_id;
+    QTAILQ_HEAD(ChildHread, CoreState) children;
+    QTAILQ_ENTRY(SocketState) next;
+} SocketState;
+
+typedef struct NodeState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    int node_id;
+    QTAILQ_ENTRY(NodeState) next;
+} NodeState;
+
+CoreState *cpu_topo_object_core_find(CpuTopoInfo *topo);
+void cpu_topo_init(void);
+
+#endif /* TARGET_I386_CPU_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a5df33..720b58a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2693,6 +2693,19 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
+void x86_topo_cpu_set_link(Object *obj, CpuTopoInfo *topo)
+{
+    gchar *name;
+    CoreState *core;
+
+    core = cpu_topo_object_core_find(topo);
+    if (core) {
+        name = g_strdup_printf("cpu[%" PRIu32 "]", topo->smt_id);
+        object_property_set_link(OBJECT(core), obj, name, NULL);
+        g_free(name);
+    }
+}
+
 static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8bc7b45..0616de8 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -24,6 +24,7 @@
 
 #include "sysemu/cpus.h"
 #include "topology.h"
+#include "cpu-topology.h"
 
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
@@ -1291,6 +1292,8 @@ const char *get_register_name_32(unsigned int reg);
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
+void x86_topo_cpu_set_link(Object *obj, CpuTopoInfo *topo);
+
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
-- 
1.8.1.4

[Qemu-devel] [RFC v2 2/2] i386: introduce cpu QOM hierarchy tree

[Chen Fan]

add cpu-topology.h cpu-topology.c files for prebuild cpu qom tree
"/machine/node[X]/socket[Y]/core[Z]/thread[N]/cpu"

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c               |   6 +-
 target-i386/Makefile.objs  |   2 +-
 target-i386/cpu-qom.h      |   1 +
 target-i386/cpu-topology.c | 163 +++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu-topology.h |  71 ++++++++++++++++++++
 target-i386/cpu.c          |  29 ++++++++
 target-i386/cpu.h          |   2 +
 7 files changed, 272 insertions(+), 2 deletions(-)
 create mode 100644 target-i386/cpu-topology.c
 create mode 100644 target-i386/cpu-topology.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a4d539e..6beeb82 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -942,7 +942,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, X86TopoInfo *topo,
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
-
+    x86_topo_cpu_set_link(topo, OBJECT(cpu));
     if (local_err) {
         error_propagate(errp, local_err);
         object_unref(OBJECT(cpu));
@@ -999,6 +999,10 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    for (i = 0; i < max_cpus; i++) {
+        x86_topo_cpu_create(i);
+    }
+
     for (i = 0; i < smp_cpus; i++) {
         X86TopoInfo *topo = x86_cpu_topo_from_index(i);
         cpu = pc_new_cpu(cpu_model, topo, icc_bridge, &error);
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index 027b94e..c150ad1 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += translate.o helper.o cpu.o
+obj-y += translate.o helper.o cpu.o cpu-topology.c
 obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-y += gdbstub.o
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 722f11a..12818a5 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -23,6 +23,7 @@
 #include "qom/cpu.h"
 #include "cpu.h"
 #include "qapi/error.h"
+#include "cpu-topology.h"
 
 #ifdef TARGET_X86_64
 #define TYPE_X86_CPU "x86_64-cpu"
diff --git a/target-i386/cpu-topology.c b/target-i386/cpu-topology.c
new file mode 100644
index 0000000..b2f90c4
--- /dev/null
+++ b/target-i386/cpu-topology.c
@@ -0,0 +1,163 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "hw/hw.h"
+#include "sysemu/cpus.h"
+#include "sysemu/sysemu.h"
+#include "cpu-topology.h"
+
+static NodeState *cpu_topo_node_get(unsigned cpu_index)
+{
+    int i, node_id = 0;
+    gchar *name;
+    char path[128] = "/machine/";
+    Object *node;
+
+    for (i = 0; i < nb_numa_nodes; i++) {
+        if (test_bit(cpu_index, node_cpumask[i])) {
+            node_id = i;
+            break;
+        }
+    }
+
+    name = g_strdup_printf("node[%" PRIu32 "]", node_id);
+    pstrcat(path, sizeof(path), name);
+    node = object_resolve_path(path, NULL);
+    if (!node) {
+        node = object_new(TYPE_NODE);
+        object_property_add_child(qdev_get_machine(), name,
+                                  node, NULL);
+    }
+    g_free(name);
+
+    return NODE(node);
+}
+
+static SocketState *cpu_topo_socket_get(X86TopoInfo *topo)
+{
+    gchar *name;
+    NodeState *node;
+    Object *socket;
+
+    name = g_strdup_printf("socket[%" PRIu32 "]", topo->pkg_id);
+    node = cpu_topo_node_get(topo->cpu_index);
+    socket = object_resolve_path_component(OBJECT(node), name);
+    if (!socket) {
+        socket = object_new(TYPE_SOCKET);
+        object_property_add_child(OBJECT(node), name, socket, NULL);
+    }
+    g_free(name);
+
+    return SOCKET(socket);
+}
+
+static CoreState *cpu_topo_core_get(X86TopoInfo *topo)
+{
+    gchar *name;
+    SocketState *socket;
+    Object *core;
+
+    name = g_strdup_printf("core[%" PRIu32 "]", topo->core_id);
+    socket = cpu_topo_socket_get(topo);
+    core = object_resolve_path_component(OBJECT(socket), name);
+    if (!core) {
+        core = object_new(TYPE_CORE);
+        object_property_add_child(OBJECT(socket), name, core, NULL);
+    }
+    g_free(name);
+
+    return CORE(core);
+}
+
+ThreadState *cpu_topo_thread_get(X86TopoInfo *topo)
+{
+    gchar *name;
+    CoreState *core;
+    Object *thread;
+
+    name = g_strdup_printf("thread[%" PRIu32 "]", topo->smt_id);
+
+    core = cpu_topo_core_get(topo);
+    thread = object_resolve_path_component(OBJECT(core), name);
+    if (!thread) {
+        thread = object_new(TYPE_THREAD);
+        object_property_add_child(OBJECT(core), name, thread, NULL);
+    }
+    g_free(name);
+
+    return THREAD(thread);
+}
+
+static void thread_initfn(Object *obj)
+{
+    ThreadState *ts = THREAD(obj);
+
+    ts->link_cpu = g_malloc0(sizeof(CPUState*));
+}
+
+static void thread_fini(Object *obj)
+{
+    ThreadState *ts = THREAD(obj);
+
+    g_free(ts->link_cpu);
+}
+
+static const TypeInfo thread_type_info = {
+    .name = TYPE_THREAD,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(ThreadState),
+    .instance_init = thread_initfn,
+    .instance_finalize = thread_fini,
+};
+
+static const TypeInfo core_type_info = {
+    .name = TYPE_CORE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(CoreState),
+};
+
+static const TypeInfo socket_type_info = {
+    .name = TYPE_SOCKET,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SocketState),
+};
+
+static const TypeInfo node_type_info = {
+    .name = TYPE_NODE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(NodeState),
+};
+
+static void node_register_types(void)
+{
+    type_register_static(&node_type_info);
+    type_register_static(&socket_type_info);
+    type_register_static(&core_type_info);
+    type_register_static(&thread_type_info);
+}
+
+type_init(node_register_types)
diff --git a/target-i386/cpu-topology.h b/target-i386/cpu-topology.h
new file mode 100644
index 0000000..16f7312
--- /dev/null
+++ b/target-i386/cpu-topology.h
@@ -0,0 +1,71 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2014 Fujitsu Ltd.
+ * Author: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_CPU_TOPOLOGY_H
+#define TARGET_I386_CPU_TOPOLOGY_H
+
+#include "hw/qdev-core.h"
+#include "topology.h"
+
+#define TYPE_NODE   "node"
+#define TYPE_SOCKET "socket"
+#define TYPE_CORE   "core"
+#define TYPE_THREAD "thread"
+
+#define NODE(obj) OBJECT_CHECK(NodeState, (obj), TYPE_NODE)
+
+#define SOCKET(obj) OBJECT_CHECK(SocketState, (obj), TYPE_SOCKET)
+
+#define CORE(obj) OBJECT_CHECK(CoreState, (obj), TYPE_CORE)
+
+#define THREAD(obj) OBJECT_CHECK(ThreadState, (obj), TYPE_THREAD)
+
+typedef struct ThreadState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+    CPUState **link_cpu;
+} ThreadState;
+
+typedef struct CoreState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} CoreState;
+
+typedef struct SocketState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} SocketState;
+
+typedef struct NodeState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+} NodeState;
+
+ThreadState *cpu_topo_thread_get(X86TopoInfo *topo);
+
+#endif /* TARGET_I386_CPU_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1858a66..c1a2ba3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2711,6 +2711,35 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
+void x86_topo_cpu_set_link(X86TopoInfo *topo, Object *obj)
+{
+    gchar *name;
+    ThreadState *thread;
+
+    name = g_strdup_printf("cpu[%" PRIu32 "]",
+                           x86_cpu_apic_id_from_index(topo->cpu_index));
+
+    thread = cpu_topo_thread_get(topo);
+    object_property_set_link(OBJECT(thread), obj, name, NULL);
+    g_free(name);
+}
+
+void x86_topo_cpu_create(unsigned int cpu_index)
+{
+    X86TopoInfo *topo = x86_cpu_topo_from_index(cpu_index);
+    ThreadState *thread;
+    gchar *name;
+
+    thread = cpu_topo_thread_get(topo);
+
+    name = g_strdup_printf("cpu[%" PRIu32 "]",
+                           x86_cpu_apic_id_from_index(cpu_index));
+
+    object_property_add_link(OBJECT(thread), name, TYPE_CPU,
+                             (Object **)thread->link_cpu, NULL);
+    g_free(name);
+}
+
 static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1647394..29fa730 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1286,6 +1286,8 @@ void enable_compat_apic_id_mode(void);
 #include "topology.h"
 
 X86TopoInfo *x86_cpu_topo_from_index(unsigned int cpu_index);
+void x86_topo_cpu_create(unsigned int cpu_index);
+void x86_topo_cpu_set_link(X86TopoInfo *topo, Object *obj);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2][RFC] cpu: link each new cpu to QOM tree /machine/node/socket/core/thread/cpu respectively.

[Chen Fan]

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 348b15f..4e07ef9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -57,6 +57,7 @@
 #include "hw/boards.h"
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
+#include "qom/node.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -927,11 +928,13 @@ 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, int64_t id,
                           DeviceState *icc_bridge, Error **errp)
 {
     X86CPU *cpu;
     Error *local_err = NULL;
+    Object *thread;
+    int64_t apic_id = x86_cpu_apic_id_from_index(id);
 
     cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
     if (local_err != NULL) {
@@ -942,11 +945,17 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
+    thread = object_get_thread_from_index(id);
+    if (thread) {
+        object_property_set_link(thread, OBJECT(cpu), "cpu", &local_err);
+    }
+
     if (local_err) {
         error_propagate(errp, local_err);
         object_unref(OBJECT(cpu));
         cpu = NULL;
     }
+
     return cpu;
 }
 
@@ -976,7 +985,29 @@ 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);
+    pc_new_cpu(current_cpu_model, id, icc_bridge, errp);
+}
+
+static void qdev_cpus_init(void)
+{
+    int i, nodes;
+    gchar *node_name;
+    Object *obj;
+
+    if (nb_numa_nodes) {
+        nodes = nb_numa_nodes;
+    } else {
+        nodes = 1;
+    }
+
+    for (i = 0; i < nodes; i++) {
+        obj = object_new(TYPE_NODE);
+        node_name = g_strdup_printf("node[%" PRIi32 "]", i);
+        object_property_add_child(qdev_get_machine(),
+                                  node_name, obj, NULL);
+        g_free(node_name);
+    }
+
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
@@ -995,9 +1026,10 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
+    qdev_cpus_init();
+
     for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);
+        cpu = pc_new_cpu(cpu_model, i, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/2][RFC] cpu: link each new cpu to QOM tree /machine/node/socket/core/thread/cpu respectively.

[Eduardo Habkost]

On Tue, Feb 25, 2014 at 05:07:32PM +0800, Chen Fan wrote:
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 348b15f..4e07ef9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -57,6 +57,7 @@
>  #include "hw/boards.h"
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
> +#include "qom/node.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -927,11 +928,13 @@ 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, int64_t id,
>                            DeviceState *icc_bridge, Error **errp)

What about "decoding" the CPU index as soon as possible, and making this
function get the full CPU "location", instead of adding more
CPU-index-based logic to the rest of the code?


>  {
>      X86CPU *cpu;
>      Error *local_err = NULL;
> +    Object *thread;
> +    int64_t apic_id = x86_cpu_apic_id_from_index(id);
>  
>      cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
>      if (local_err != NULL) {
> @@ -942,11 +945,17 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>  
> +    thread = object_get_thread_from_index(id);

If you just separate the topo_ids_from_idx() and topo_make_apicid()
calls, the IDs calculated by topo_ids_from_idx() will be useful for two
things: 1) QOM object path construction; and 2) APIC ID calculation
(topo_make_apicid()), and you won't even need to implement
object_get_thread_from_index().

(...except when compat_apic_id_mode bug-compatibility is enabled. In
this case, either the APIC IDs won't match the QOM hierarchy, or we will
present a QOM hierarchy different from the one requested by the user.)

By the way: a hierarchical node/socket/core/thread abstraction will
probably break with current NUMA command-lines that are working. e.g.:

  $ qemu-system-x86_64 -smp 8,cores=2,threads=2 \
                       -numa node,cpus=0-2 \
                       -numa node,cpus=3-5 \
                       -numa node,cpus=6-7

The above command-line makes threads on the same socket/core appear on
different NUMA nodes. Do we want to keep this (weird) topology
configuration working?


> +    if (thread) {
> +        object_property_set_link(thread, OBJECT(cpu), "cpu", &local_err);
> +    }
> +
>      if (local_err) {
>          error_propagate(errp, local_err);
>          object_unref(OBJECT(cpu));
>          cpu = NULL;
>      }
> +
>      return cpu;
>  }
>  
> @@ -976,7 +985,29 @@ 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);
> +    pc_new_cpu(current_cpu_model, id, icc_bridge, errp);
> +}
> +
> +static void qdev_cpus_init(void)
> +{
> +    int i, nodes;
> +    gchar *node_name;
> +    Object *obj;
> +
> +    if (nb_numa_nodes) {
> +        nodes = nb_numa_nodes;
> +    } else {
> +        nodes = 1;
> +    }
> +
> +    for (i = 0; i < nodes; i++) {
> +        obj = object_new(TYPE_NODE);
> +        node_name = g_strdup_printf("node[%" PRIi32 "]", i);
> +        object_property_add_child(qdev_get_machine(),
> +                                  node_name, obj, NULL);
> +        g_free(node_name);
> +    }
> +
>  }
>  
>  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> @@ -995,9 +1026,10 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>      }
>      current_cpu_model = cpu_model;
>  
> +    qdev_cpus_init();
> +
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
> -                         icc_bridge, &error);
> +        cpu = pc_new_cpu(cpu_model, i, icc_bridge, &error);
>          if (error) {
>              error_report("%s", error_get_pretty(error));
>              error_free(error);
> -- 
> 1.8.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn())

[Igor Mammedov]

On Tue, 21 Jan 2014 11:10:30 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 21.01.2014 10:51, schrieb Chen Fan:
> > On Tue, 2014-01-21 at 10:31 +0100, Igor Mammedov wrote:
> >> On Tue, 21 Jan 2014 15:12:45 +0800
> >> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >>> On Mon, 2014-01-20 at 13:29 +0100, Igor Mammedov wrote:
> >>>> On Fri, 17 Jan 2014 17:13:55 -0200
> >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>> On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote:
> >>>>>> 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.
> >>>>>
> >>>>> 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.
> >>>>
> >>>>>
> >>>>> 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.
> >>>>
> >>> 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
> >>  
> > 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.
In terms of parent/child relationship, I guess we are not going to come up
with uniform design, since boards could differ very much in that aspect.

I was rather thinking in terms of providing stable/uniform CLI/QMP NUMA
interface using QOM tree.
At startup we potentially have cpu topology information and set of NUMA
nodes, so we could pre-build containers up to the point where CPU threads
are attached and pre-create empty links<CPU> and fill them later with actual
CPU threads.

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

[Qemu-devel] [RFC 2/3] target-i386: add -smp X,apics=0x option

[Chen Fan]

This option provides the infrastructure for specifying apicids when
boot VM, For example:

 #boot with apicid 0 and 2:
 -smp 2,apics=0xA,maxcpus=4  /* 1010 */
 #boot with apicid 1 and 7:
 -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c            |  9 +++++--
 include/sysemu/sysemu.h |  4 ++++
 qemu-options.hx         | 15 +++++++++---
 vl.c                    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 963446f..3582167 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -991,8 +991,13 @@ 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);
+        int64_t apic_id;
+        if (nb_boot_apics == 0) {
+            apic_id = x86_cpu_apic_id_from_index(i);
+        } else {
+            apic_id = x86_cpu_apic_id_from_index(boot_apics[i]);
+        }
+        cpu = pc_new_cpu(cpu_model, apic_id, icc_bridge, &error);
         if (error) {
             error_report("%s", error_get_pretty(error));
             error_free(error);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 495dae8..510a626 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -149,6 +149,10 @@ extern int nb_option_roms;
 extern const char *prom_envs[MAX_PROM_ENVS];
 extern unsigned int nb_prom_envs;
 
+#define MAX_APICS 255
+extern int nb_boot_apics;
+extern int64_t boot_apics[MAX_APICS];
+
 /* pci-hotplug */
 void pci_device_hot_add(Monitor *mon, const QDict *qdict);
 int pci_drive_hot_add(Monitor *mon, const QDict *qdict, DriveInfo *dinfo);
diff --git a/qemu-options.hx b/qemu-options.hx
index bcfe9ea..7f86519 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -73,16 +73,17 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
 ETEXI
 
 DEF("smp", HAS_ARG, QEMU_OPTION_smp,
-    "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets]\n"
+    "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets][,apics=apics]\n"
     "                set the number of CPUs to 'n' [default=1]\n"
     "                maxcpus= maximum number of total cpus, including\n"
     "                offline CPUs for hotplug, etc\n"
     "                cores= number of CPU cores on one socket\n"
     "                threads= number of threads on one CPU core\n"
-    "                sockets= number of discrete sockets in the system\n",
+    "                sockets= number of discrete sockets in the system\n"
+    "                apics= a hex number with leading '0x' as boot bitmap of existed apicid\n",
         QEMU_ARCH_ALL)
 STEXI
-@item -smp [cpus=]@var{n}[,cores=@var{cores}][,threads=@var{threads}][,sockets=@var{sockets}][,maxcpus=@var{maxcpus}]
+@item -smp [cpus=]@var{n}[,cores=@var{cores}][,threads=@var{threads}][,sockets=@var{sockets}][,maxcpus=@var{maxcpus}][,apics=@var{apics}]
 @findex -smp
 Simulate an SMP system with @var{n} CPUs. On the PC target, up to 255
 CPUs are supported. On Sparc32 target, Linux limits the number of usable CPUs
@@ -92,6 +93,14 @@ of @var{threads} per cores and the total number of @var{sockets} can be
 specified. Missing values will be computed. If any on the three values is
 given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
 specifies the maximum number of hotpluggable CPUs.
+@var{apics} specifies the boot bitmap of existed apicid.
+
+@example
+#specify the boot bitmap of apicid with 0 and 2:
+qemu-system-i386 -smp 2,apics=0xA,maxcpus=4  /* 1010 */
+#specify the boot bitmap of apicid with 1 and 7:
+qemu-system-i386 -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
+@end example
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
diff --git a/vl.c b/vl.c
index 7511e70..870b1bd 100644
--- a/vl.c
+++ b/vl.c
@@ -254,6 +254,9 @@ unsigned long *node_cpumask[MAX_NODES];
 uint8_t qemu_uuid[16];
 bool qemu_uuid_set;
 
+int nb_boot_apics;
+int64_t boot_apics[MAX_APICS];
+
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
 
@@ -1379,6 +1382,9 @@ static QemuOptsList qemu_smp_opts = {
         }, {
             .name = "maxcpus",
             .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "apics",
+            .type = QEMU_OPT_STRING,
         },
         { /*End of list */ }
     },
@@ -1392,6 +1398,7 @@ static void smp_parse(QemuOpts *opts)
         unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
         unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
         unsigned threads = qemu_opt_get_number(opts, "threads", 0);
+        const char *apics = qemu_opt_get(opts, "apics");
 
         /* compute missing values, prefer sockets over cores over threads */
         if (cpus == 0 || sockets == 0) {
@@ -1416,6 +1423,55 @@ static void smp_parse(QemuOpts *opts)
         smp_cores = cores > 0 ? cores : 1;
         smp_threads = threads > 0 ? threads : 1;
 
+        if (apics) {
+            if (strstart(apics, "0x", &apics)) {
+                if (*apics != '\0') {
+                    int i, count;
+                    int64_t max_apicid = 0;
+                    uint32_t val;
+                    char tmp[2];
+
+                    count = strlen(apics);
+
+                    for (i = 0; i < count; i++) {
+                        tmp[0] = apics[i];
+                        tmp[1] = '\0';
+                        sscanf(tmp, "%x", &val);
+
+                        if (val & 1 << 3) {
+                            boot_apics[nb_boot_apics++] = i * 4;
+                        }
+                        if (val & 1 << 2) {
+                            boot_apics[nb_boot_apics++] = i * 4 + 1;
+                        }
+                        if (val & 1 << 1) {
+                            boot_apics[nb_boot_apics++] = i * 4 + 2;
+                        }
+                        if (val & 1 << 0) {
+                            boot_apics[nb_boot_apics++] = i * 4 + 3;
+                        }
+                        if (max_apicid < boot_apics[nb_boot_apics - 1]) {
+                            max_apicid = boot_apics[nb_boot_apics - 1];
+                        }
+                    }
+
+                    if ((max_cpus == 0 && max_apicid > smp_cpus - 1) ||
+                        (max_cpus != 0 && max_apicid > max_cpus - 1)) {
+                        fprintf(stderr, "specified apicid"
+                                " must be smaller than maxcpus\n");
+                        exit(1);
+                    }
+                } else {
+                    fprintf(stderr, "must specify"
+                            " one or more digits in apics\n");
+                    exit(1);
+                }
+            } else {
+                fprintf(stderr, "could not parse apics: %s,"
+                        " apics must use a leading '0x' digits\n", apics);
+                exit(1);
+            }
+        }
     }
 
     if (max_cpus == 0) {
@@ -1430,7 +1486,11 @@ static void smp_parse(QemuOpts *opts)
         fprintf(stderr, "maxcpus must be equal to or greater than smp\n");
         exit(1);
     }
-
+    if (nb_boot_apics && nb_boot_apics != smp_cpus) {
+        fprintf(stderr, "the count of specified apics: %d"
+                " is not equal to smp: %d\n", nb_boot_apics, smp_cpus);
+        exit(1);
+    }
 }
 
 static void configure_realtime(QemuOpts *opts)
-- 
1.8.1.4

Re: [Qemu-devel] [RFC 2/3] target-i386: add -smp X,apics=0x option

[Eric Blake]

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

On 01/14/2014 02:27 AM, Chen Fan wrote:
> This option provides the infrastructure for specifying apicids when
> boot VM, For example:
> 
>  #boot with apicid 0 and 2:
>  -smp 2,apics=0xA,maxcpus=4  /* 1010 */
>  #boot with apicid 1 and 7:
>  -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */

This syntax feels a bit odd when maxcpus is not a multiple of 8, and
even harder when not a multiple of 4.  I think part of my confusion
stems from you treating the lsb as the left-most bit, but expect me to
write in hex where I'm used to the right-most bit being lsb   Wouldn't
it be easier to express:

msb .... lsb

with leading 0s implied as needed, as in:

0x5 => 0101 => id 0 (lsb) and id 2 are enabled, regardless of whether
maxcpus=4 or maxcpus=8
0x82 => 1000 0010 => id 1 and id 7 are enabled, regardless of whether
maxcpus=8 or maxcpus=256

0x100000000 => id 32 is enabled

Or even better, why not reuse existing parsers that take cpu ids
directly as numbers instead of making me compute a bitmap (as in
maxcpus=4,id=0,id=2 - although I don't quite know QemuOpts well enough
to know if you can repeat id= for forming a list of disjoint id numbers)

> @@ -92,6 +93,14 @@ of @var{threads} per cores and the total number of @var{sockets} can be
>  specified. Missing values will be computed. If any on the three values is
>  given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
>  specifies the maximum number of hotpluggable CPUs.
> +@var{apics} specifies the boot bitmap of existed apicid.

s/existed/existing/

> +
> +@example
> +#specify the boot bitmap of apicid with 0 and 2:
> +qemu-system-i386 -smp 2,apics=0xA,maxcpus=4  /* 1010 */
> +#specify the boot bitmap of apicid with 1 and 7:
> +qemu-system-i386 -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
> +@end example

These examples would need updating to match my concerns.

> @@ -1379,6 +1382,9 @@ static QemuOptsList qemu_smp_opts = {
>          }, {
>              .name = "maxcpus",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "apics",
> +            .type = QEMU_OPT_STRING,

Why a string with your own ad-hoc parser?  Can't we reuse some of the
existing parsers that already know how to handle (possibly-disjoint)
lists of cpu numbers?

> +        if (apics) {
> +            if (strstart(apics, "0x", &apics)) {

Why not also allow 0X?

> +                if (*apics != '\0') {
> +                    int i, count;
> +                    int64_t max_apicid = 0;
> +                    uint32_t val;
> +                    char tmp[2];
> +
> +                    count = strlen(apics);
> +
> +                    for (i = 0; i < count; i++) {
> +                        tmp[0] = apics[i];
> +                        tmp[1] = '\0';
> +                        sscanf(tmp, "%x", &val);

sscanf is evil.  It has undefined behavior on input overflow (that is,
if I say 0x10000000000000000, there is no guarantee what sscanf will
stick into val).  All the more reason you should be using an existing
parser which gracefully handles overflow.

-- 
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] [RFC 2/3] target-i386: add -smp X,apics=0x option

[Chen Fan]

On Mon, 2014-02-17 at 11:37 -0700, Eric Blake wrote:
> On 01/14/2014 02:27 AM, Chen Fan wrote:
> > This option provides the infrastructure for specifying apicids when
> > boot VM, For example:
> > 
> >  #boot with apicid 0 and 2:
> >  -smp 2,apics=0xA,maxcpus=4  /* 1010 */
> >  #boot with apicid 1 and 7:
> >  -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
> 
> This syntax feels a bit odd when maxcpus is not a multiple of 8, and
> even harder when not a multiple of 4.  I think part of my confusion
> stems from you treating the lsb as the left-most bit, but expect me to
> write in hex where I'm used to the right-most bit being lsb   Wouldn't
> it be easier to express:
> 
> msb .... lsb
> 
> with leading 0s implied as needed, as in:
> 
> 0x5 => 0101 => id 0 (lsb) and id 2 are enabled, regardless of whether
> maxcpus=4 or maxcpus=8
> 0x82 => 1000 0010 => id 1 and id 7 are enabled, regardless of whether
> maxcpus=8 or maxcpus=256
> 
> 0x100000000 => id 32 is enabled
> 
> Or even better, why not reuse existing parsers that take cpu ids
> directly as numbers instead of making me compute a bitmap (as in
> maxcpus=4,id=0,id=2 - although I don't quite know QemuOpts well enough
> to know if you can repeat id= for forming a list of disjoint id numbers)
> 
Thanks for your review, but this form was deprecated. Igor proposed
using -device /-device-add to specify the disjoint id numbers.

Thanks,
Chen 

> > @@ -92,6 +93,14 @@ of @var{threads} per cores and the total number of @var{sockets} can be
> >  specified. Missing values will be computed. If any on the three values is
> >  given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
> >  specifies the maximum number of hotpluggable CPUs.
> > +@var{apics} specifies the boot bitmap of existed apicid.
> 
> s/existed/existing/
> 
> > +
> > +@example
> > +#specify the boot bitmap of apicid with 0 and 2:
> > +qemu-system-i386 -smp 2,apics=0xA,maxcpus=4  /* 1010 */
> > +#specify the boot bitmap of apicid with 1 and 7:
> > +qemu-system-i386 -smp 2,apics=0x41,maxcpus=8 /* 0100 0001 */
> > +@end example
> 
> These examples would need updating to match my concerns.
> 
> > @@ -1379,6 +1382,9 @@ static QemuOptsList qemu_smp_opts = {
> >          }, {
> >              .name = "maxcpus",
> >              .type = QEMU_OPT_NUMBER,
> > +        }, {
> > +            .name = "apics",
> > +            .type = QEMU_OPT_STRING,
> 
> Why a string with your own ad-hoc parser?  Can't we reuse some of the
> existing parsers that already know how to handle (possibly-disjoint)
> lists of cpu numbers?
> 
> > +        if (apics) {
> > +            if (strstart(apics, "0x", &apics)) {
> 
> Why not also allow 0X?
> 
> > +                if (*apics != '\0') {
> > +                    int i, count;
> > +                    int64_t max_apicid = 0;
> > +                    uint32_t val;
> > +                    char tmp[2];
> > +
> > +                    count = strlen(apics);
> > +
> > +                    for (i = 0; i < count; i++) {
> > +                        tmp[0] = apics[i];
> > +                        tmp[1] = '\0';
> > +                        sscanf(tmp, "%x", &val);
> 
> sscanf is evil.  It has undefined behavior on input overflow (that is,
> if I say 0x10000000000000000, there is no guarantee what sscanf will
> stick into val).  All the more reason you should be using an existing
> parser which gracefully handles overflow.
> 

[Qemu-devel] [RFC 3/3] target-i386: add qmp command 'query-cpus' to display apic_id

[Chen Fan]

this patch provided the apic_id display as using command 'query-cpus'.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 cpus.c           | 1 +
 qapi-schema.json | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index ca4c59f..e6ed098 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1351,6 +1351,7 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 #if defined(TARGET_I386)
         info->value->has_pc = true;
         info->value->pc = env->eip + env->segs[R_CS].base;
+        info->value->apic_id = env->cpuid_apic_id;
 #elif defined(TARGET_PPC)
         info->value->has_nip = true;
         info->value->nip = env->nip;
diff --git a/qapi-schema.json b/qapi-schema.json
index c3c939c..40c67ac 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -783,6 +783,8 @@
 #
 # @thread_id: ID of the underlying host thread
 #
+# @apic_id: the apic id of the virtual CPU
+#
 # Since: 0.14.0
 #
 # Notes: @halted is a transient state that changes frequently.  By the time the
@@ -790,7 +792,7 @@
 ##
 { 'type': 'CpuInfo',
   'data': {'CPU': 'int', 'current': 'bool', 'halted': 'bool', '*pc': 'int',
-           '*nip': 'int', '*npc': 'int', '*PC': 'int', 'thread_id': 'int'} }
+           '*nip': 'int', '*npc': 'int', '*PC': 'int', 'thread_id': 'int', 'apic_id': 'int'} }
 
 ##
 # @query-cpus:
-- 
1.8.1.4

Re: [Qemu-devel] [RFC 3/3] target-i386: add qmp command 'query-cpus' to display apic_id

[Eric Blake]

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

On 01/14/2014 02:27 AM, Chen Fan wrote:
> this patch provided the apic_id display as using command 'query-cpus'.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  cpus.c           | 1 +
>  qapi-schema.json | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 

> +++ b/qapi-schema.json
> @@ -783,6 +783,8 @@
>  #
>  # @thread_id: ID of the underlying host thread
>  #
> +# @apic_id: the apic id of the virtual CPU
> +#

Needs a (since 2.0) designation on this line.

-- 
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 --]