[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as a
From: |
Chen Fan |
Subject: |
Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu() |
Date: |
Wed, 12 Mar 2014 13:53:05 +0800 |
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" <address@hidden>
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> > 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
> >
>
- [Qemu-devel] [RFC v2 0/2] prebuild cpu QOM tree /machine/node/socket/core/thread/.., Chen Fan, 2014/03/04
- [Qemu-devel] [RFC v2 1/2] i386: introduce "struct X86TopoInfo" for saving cpu topology information, Chen Fan, 2014/03/04
- Re: [Qemu-devel] [RFC v2 1/2] i386: introduce "struct X86TopoInfo" for saving cpu topology information, Eduardo Habkost, 2014/03/04
- [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(), chen.fan.fnst, 2014/03/11
- Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(), Eduardo Habkost, 2014/03/11
- Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(),
Chen Fan <=
- [Qemu-devel] [RFC v4 0/3] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu, Chen Fan, 2014/03/12
- [Qemu-devel] [RFC v4 1/3] cpu: introduce CpuTopoInfo structure for argument simplification, Chen Fan, 2014/03/12
- Re: [Qemu-devel] [RFC v4 1/3] cpu: introduce CpuTopoInfo structure for argument simplification, Eduardo Habkost, 2014/03/12
- [Qemu-devel] [PATCH v1 0/4] prebuild cpu QOM tree /machine/node/socket/core ->link-cpu, Chen Fan, 2014/03/19
- [Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(), Chen Fan, 2014/03/19
- Re: [Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(), Eduardo Habkost, 2014/03/19
- Re: [Qemu-devel] [PATCH v1 2/4] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu(), Chen Fan, 2014/03/20
- [Qemu-devel] [PATCH v1 3/4] topo unit-test: update Unit tests to test-x86-cpuid.c, Chen Fan, 2014/03/19
- [Qemu-devel] [PATCH v1 1/4] cpu: introduce CpuTopoInfo structure for argument simplification, Chen Fan, 2014/03/19
- [Qemu-devel] [PATCH v1 4/4] i386: introduce cpu QOM hierarchy tree, Chen Fan, 2014/03/19