From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53796) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5Dyc-0003LA-Gk for qemu-devel@nongnu.org; Mon, 20 Jan 2014 07:29:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W5DyY-0006wK-8Q for qemu-devel@nongnu.org; Mon, 20 Jan 2014 07:29:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48353) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W5DyX-0006wE-VQ for qemu-devel@nongnu.org; Mon, 20 Jan 2014 07:29:10 -0500 Date: Mon, 20 Jan 2014 13:29:01 +0100 From: Igor Mammedov Message-ID: <20140120132901.57999b8c@nial.usersys.redhat.com> In-Reply-To: <20140117191355.GB2221@otherpad.lan.raisama.net> References: <602453e20a4b721f596cd727b981e9e698c83159.1389685621.git.chen.fan.fnst@cn.fujitsu.com> <20140114114003.6001b070@nial.usersys.redhat.com> <1389788641.2726.13.camel@G08FNSTD131468> <20140115153704.08a47320@thinkpad> <20140117191355.GB2221@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Exposing and calculating CPU APIC IDs (was Re: [RFC 1/3] target-i386: moving registers of vmstate from cpu_exec_init() to x86_cpu_realizefn()) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Chen Fan , libvir-list@redhat.com, qemu-devel@nongnu.org, Andreas =?ISO-8859-1?B?RuRyYmVy?= On Fri, 17 Jan 2014 17:13:55 -0200 Eduardo Habkost wrote: > On Wed, Jan 15, 2014 at 03:37:04PM +0100, Igor Mammedov wrote: > > On Wed, 15 Jan 2014 20:24:01 +0800 > > Chen Fan wrote: > > > On Tue, 2014-01-14 at 11:40 +0100, Igor Mammedov wrote: > > > > On Tue, 14 Jan 2014 17:27:20 +0800 > > > > Chen Fan wrote: > > > > > > > > > the intend of this patch is to register cpu vmstates with apic id instead of cpu > > > > > index, due to the property setting of apic_id is behind the cpu initialization. so > > > > > we move the registers of cpu vmstate from cpu_exec_init() to x86_cpu_realizefn() to > > > > > let the set apicid as the parameter. > > > > > > > > > > Signed-off-by: Chen Fan > > > > > --- > > > > > exec.c | 5 +++++ > > > > > target-i386/cpu.c | 9 +++++++++ > > > > > 2 files changed, 14 insertions(+) > > > > > > > > > > diff --git a/exec.c b/exec.c > > > > > index 7e49e8e..9be5855 100644 > > > > > --- a/exec.c > > > > > +++ b/exec.c > > > > > @@ -438,7 +438,9 @@ CPUState *qemu_get_cpu(int index) > > > > > void cpu_exec_init(CPUArchState *env) > > > > > { > > > > > CPUState *cpu = ENV_GET_CPU(env); > > > > > +#if !defined(TARGET_I386) > > > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > > > +#endif > > > > > CPUState *some_cpu; > > > > > int cpu_index; > > > > > > > > > > @@ -460,6 +462,8 @@ void cpu_exec_init(CPUArchState *env) > > > > > #if defined(CONFIG_USER_ONLY) > > > > > cpu_list_unlock(); > > > > > #endif > > > > > + > > > > > +#if !defined(TARGET_I386) > > > > > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > > > > > vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu); > > > > > } > > > > > @@ -472,6 +476,7 @@ void cpu_exec_init(CPUArchState *env) > > > > > if (cc->vmsd != NULL) { > > > > > vmstate_register(NULL, cpu_index, cc->vmsd, cpu); > > > > > } > > > > > +#endif /* !TARGET_I386 */ > > > > > } > > > > > > > > > > #if defined(TARGET_HAS_ICE) > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > index 967529a..dada6f6 100644 > > > > > --- a/target-i386/cpu.c > > > > > +++ b/target-i386/cpu.c > > > > > @@ -2552,6 +2552,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp) > > > > > static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > > > > { > > > > > CPUState *cs = CPU(dev); > > > > > + CPUClass *cc = CPU_GET_CLASS(cs); > > > > > X86CPU *cpu = X86_CPU(dev); > > > > > X86CPUClass *xcc = X86_CPU_GET_CLASS(dev); > > > > > CPUX86State *env = &cpu->env; > > > > > @@ -2615,6 +2616,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > > > > > cpu_reset(cs); > > > > > > > > > > xcc->parent_realize(dev, &local_err); > > > > > + > > > > > + if (qdev_get_vmsd(DEVICE(cs)) == NULL) { > > > > > + vmstate_register(NULL, env->cpuid_apic_id, &vmstate_cpu_common, cs); > > > > > + } > > > > > + > > > > > + if (cc->vmsd != NULL) { > > > > > + vmstate_register(NULL, env->cpuid_apic_id, cc->vmsd, cs); > > > > > + } > > > > how about doing it in common CPUclass.realize() > > > > you can use get_arch_id() for getting CPU id, it returns cpu_index by default > > > > and apic_id for target-i386. > > > > > > Thanks for your kind suggestion, does this mean we can directly move > > > vmstate_register to cpu_common_realizefn()? > > yep, that is a gist of it. > > > > There is more to the issue with discontinuous CPUs, a lot of code still > > use cpu_index and the way it's allocated is not compatible with discontinuous > > CPUs, so this issue should be fixed as well. > > > > Also you propose to use a raw apic id with CLI/user interface. > > I recall there were objections to it since APIC ID contains topology > > information and it's not trivial for user to get it right. > > The last idea that was discussed to fix it was not expose APIC ID to > > user but rather introduce QOM hierarchy like: > > /machine/node/N/socket/X/core/Y/thread/Z > > and use it in user interface as a means to specify an arbitrary CPU > > and let QEMU calculate APIC ID based on this path. > > > > But nobody took on implementing it yet. > > We're taking so long to get a decent interface implemented, that part of > me is considering exposing the APIC ID directly like suggested before, > and requiring libvirt to calculate topology-aware APIC IDs[1] to > properly implement CPU hotplug (and possibly for other tasks). If you are speaking about 'qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"' http://patchwork.ozlabs.org/patch/301272/ bug then it's limitation of ACPI implementation, I'm going to refactor it to use full APIC ids instead of using bitmap, so that we won't ever run into issue regardless of cpu supported CPU count. > > Another part of me is hoping that the libvirt developers ask us to > please not do that, so I can use it as argument against exposing the > APIC IDs directly the next time we discuss this. :) why not try your /machine/node/N/socket/X/core/Y/thread/Z idea first. It will benefit not only cpu hotplug but also '-numa' and topology description in general. > [1] See target-i386/topology.h for references >