From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MDwqJ-0000Zo-BH for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:38:03 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MDwqE-0000Z1-DZ for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:38:02 -0400 Received: from [199.232.76.173] (port=48638 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MDwqE-0000Yw-7f for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:37:58 -0400 Received: from lizzard.sbs.de ([194.138.37.39]:18875) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MDwqD-00076q-FL for qemu-devel@nongnu.org; Tue, 09 Jun 2009 04:37:57 -0400 Message-ID: <4A2E1F61.6040406@siemens.com> Date: Tue, 09 Jun 2009 10:37:53 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20090608125946.GF27210@redhat.com> <4A2D1A27.4080504@siemens.com> <20090609081401.GR27210@redhat.com> In-Reply-To: <20090609081401.GR27210@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Gleb Natapov Cc: qemu-devel@nongnu.org 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. > 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. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux