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