From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MDx6M-00015p-Nv for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:54:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MDx6I-00012r-QE for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:54:38 -0400 Received: from [199.232.76.173] (port=59399 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MDx6I-00012g-K3 for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:54:34 -0400 Received: from mx2.redhat.com ([66.187.237.31]:54785) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MDx6I-00028h-2N for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:54:34 -0400 Date: Tue, 9 Jun 2009 11:54:31 +0300 From: Gleb Natapov Message-ID: <20090609085431.GS27210@redhat.com> References: <20090608125946.GF27210@redhat.com> <4A2D1A27.4080504@siemens.com> <20090609081401.GR27210@redhat.com> <4A2E1F61.6040406@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4A2E1F61.6040406@siemens.com> Subject: [Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel@nongnu.org On Tue, Jun 09, 2009 at 10:37:53AM +0200, Jan Kiszka wrote: > Gleb Natapov wrote: > > On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote: > >> Gleb Natapov wrote: > >>> > >>> It should depend on whether cpu has APIC. > >>> > >>> Signed-off-by: Gleb Natapov > >>> diff --git a/hw/pc.c b/hw/pc.c > >>> index 0934778..cb49772 100644 > >>> --- a/hw/pc.c > >>> +++ b/hw/pc.c > >>> @@ -878,14 +878,10 @@ static void pc_init1(ram_addr_t ram_size, > >>> } > >>> if (i != 0) > >>> env->halted = 1; > >>> - if (smp_cpus > 1) { > >>> - /* XXX: enable it in all cases */ > >>> - env->cpuid_features |= CPUID_APIC; > >>> - } > >>> - qemu_register_reset(main_cpu_reset, 0, env); > >>> - if (pci_enabled) { > >>> + if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > >> Obviously :), I'm fine with that change. Needs testing, though. What > >> scenarios did you already check? > >> > >>> apic_init(env); > >>> } > >>> + qemu_register_reset(main_cpu_reset, 0, env); > >> But this line silently reorders CPU and APIC reset handlers. If you did > >> it intentionally (I vaguely recall it may have some benefit /wrt KVM > >> synchronizing kernel and user space states), I would suggest pushing it > >> as a separate patch. > >> > > BTW relying on order of callback registration is not a good idea > > especially since we have "order" parameter now. > > The order parameter is obsolete, I already posted a patch to revert this > idea again (reminds me of posting an update as a new reset handler > arrived in the meantime). > > The system-wide assumed and applied order is that earlier instantiated > devices will be reset first. That specifically makes sense if you think > of bus/device relations. > Sounds fragile to me, but explicit order far from ideal too. Why should order matter anyway. Device reset callbacks should not rely on the state of other devices. > > On the other hand apic > > reset handler already resets cpu so if apic is present there is no need to > > register main_cpu_reset(). > > OK, this is a special case as the APIC reset triggers an init IPI and > that resets the CPU, too. Then make this explicit, replace > main_cpu_reset with cpu_reset (so that no one adds code to the former > that is not run in the APIC case) and add some comment why. Already doing it on top of this patch. -- Gleb.