From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Nn6XE-0007MQ-VA for qemu-devel@nongnu.org; Thu, 04 Mar 2010 03:35:57 -0500 Received: from [199.232.76.173] (port=46122 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Nn6XD-0007Ly-V1 for qemu-devel@nongnu.org; Thu, 04 Mar 2010 03:35:56 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Nn6XC-00007C-8O for qemu-devel@nongnu.org; Thu, 04 Mar 2010 03:35:55 -0500 Received: from mx20.gnu.org ([199.232.41.8]:61216) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Nn6XB-000070-Uh for qemu-devel@nongnu.org; Thu, 04 Mar 2010 03:35:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Nn6XB-0007Y1-6g for qemu-devel@nongnu.org; Thu, 04 Mar 2010 03:35:53 -0500 Date: Thu, 4 Mar 2010 10:35:49 +0200 From: Gleb Natapov Message-ID: <20100304083549.GI16909@redhat.com> References: <20100303160022.GG16909@redhat.com> <4B8EF1FE.7020000@web.de> <20100304064723.GH16909@redhat.com> <4B8F6E12.8040707@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B8F6E12.8040707@web.de> Subject: [Qemu-devel] Re: [PATCH v4 03/10] x86: Extend validity of cpu_is_bsp List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Marcelo Tosatti , Avi Kivity , kvm@vger.kernel.org, qemu-devel@nongnu.org On Thu, Mar 04, 2010 at 09:23:46AM +0100, Jan Kiszka wrote: > Gleb Natapov wrote: > > On Thu, Mar 04, 2010 at 12:34:22AM +0100, Jan Kiszka wrote: > >> Gleb Natapov wrote: > >>> On Mon, Mar 01, 2010 at 06:17:22PM +0100, Jan Kiszka wrote: > >>>> As we hard-wire the BSP to CPU 0 anyway and cpuid_apic_id equals > >>>> cpu_index, cpu_is_bsp can also be based on the latter directly. This > >>>> will help an early user of it: KVM while initializing mp_state. > >>>> > >>>> Signed-off-by: Jan Kiszka > >>>> --- > >>>> hw/pc.c | 3 ++- > >>>> 1 files changed, 2 insertions(+), 1 deletions(-) > >>>> > >>>> diff --git a/hw/pc.c b/hw/pc.c > >>>> index b90a79e..58c32ea 100644 > >>>> --- a/hw/pc.c > >>>> +++ b/hw/pc.c > >>>> @@ -767,7 +767,8 @@ static void pc_init_ne2k_isa(NICInfo *nd) > >>>> > >>>> int cpu_is_bsp(CPUState *env) > >>>> { > >>>> - return env->cpuid_apic_id == 0; > >>>> + /* We hard-wire the BSP to the first CPU. */ > >>>> + return env->cpu_index == 0; > >>>> } > >>> We should not assume that. The function was written like that > >>> specifically so the code around it will not rely on this assumption. > >>> Now you change that specifically to write code that will do incorrect > >>> assumptions. I don't see the logic here. > >> The logic is that we do not support any other mapping yet - with or > >> without this change. Without it, we complicate the APIC initialization > >> for (so far) no good reason. Once we want to support different BSP > >> assignments, we need to go through the code and rework some parts anyway. > >> > > As far as I remember the only part that was missing was a command line to > > specify apic IDs for each CPU and what CPU is BSP. The code was ready > > otherwise. I's very sad if this was broken by other modifications. But > > changes like that actually pushes us back from our goal. Why not rework > > code so it will work with correct cpu_is_bsp() function instead of > > introducing this hack? > > If you can confirm that there is a serious use case behind it, I will > look into this again. But so far, I did not find it. > Firs of all it is correctness issue. We should emulate x86 platform and nothing there says that BSP apic id is zero. Second part of CPU topology information is encoded in apic id. i.e when socket/core/ht topology is used we can't just arbitrary specify apic ids for each logical cpu, we should follow the rules described in SDM. For instance when more then 16 CPUs are present AMD advices to start numbering apic ids from 16 and leave first 16 IDs for IOAPICs. And third introduction of this hack shows that something is done wrong in other places of the code. Somewhere initialization order is incorrect. -- Gleb.