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