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