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

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

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