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 <address@hidden>
> > ---
> >  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;
> > +}
> [...]