From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56243 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q8C3n-0008DS-GE for qemu-devel@nongnu.org; Fri, 08 Apr 2011 09:49:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q8C3l-0007Ii-Hs for qemu-devel@nongnu.org; Fri, 08 Apr 2011 09:49:15 -0400 Received: from goliath.siemens.de ([192.35.17.28]:21022) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q8C3l-0007Hd-8m for qemu-devel@nongnu.org; Fri, 08 Apr 2011 09:49:13 -0400 Message-ID: <4D9F1249.6080305@siemens.com> Date: Fri, 08 Apr 2011 15:48:57 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1301423290-12443-1-git-send-email-anthony.perard@citrix.com> <1301423290-12443-6-git-send-email-anthony.perard@citrix.com> In-Reply-To: <1301423290-12443-6-git-send-email-anthony.perard@citrix.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH V12 05/17] xen: Add xenfv machine List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: anthony.perard@citrix.com Cc: Stefano Stabellini , Xen Devel , QEMU-devel , Alexander Graf [ Late comments, I know, sorry. Just happen to came across this. ] On 2011-03-29 20:27, anthony.perard@citrix.com wrote: > From: Anthony PERARD > > Introduce the Xen FV (Fully Virtualized) machine to Qemu, some more Xen > specific call will be added in further patches. > > Signed-off-by: Anthony PERARD > --- > hw/pc.c | 19 +++++++++++++++++-- > hw/pc_piix.c | 17 +++++++++++++++++ > hw/xen.h | 4 ++++ > 3 files changed, 38 insertions(+), 2 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 6939c04..d7732d4 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -41,6 +41,7 @@ > #include "sysemu.h" > #include "blockdev.h" > #include "ui/qemu-spice.h" > +#include "xen.h" > > /* output Bochs bios info messages */ > //#define DEBUG_BIOS > @@ -918,7 +919,11 @@ static void pc_cpu_reset(void *opaque) > CPUState *env = opaque; > > cpu_reset(env); > - env->halted = !cpu_is_bsp(env); > + if (!xen_enabled()) { > + env->halted = !cpu_is_bsp(env); > + } else { > + env->halted = 1; > + } Not a fault of your patch, but pc_cpu_reset should not exist in the first place. Setting env->halted should be done in i386's cpu_reset. I think Xen would be better off with installing a custom VCPU reset handler and overwrite halted according to its own needs. KVM is doing the same. Then we could clean up pc_cpu_reset without bothering Xen. > } > > static CPUState *pc_new_cpu(const char *cpu_model) > @@ -952,7 +957,12 @@ void pc_cpus_init(const char *cpu_model) > #endif > } > > - for(i = 0; i < smp_cpus; i++) { > + if (!xen_enabled()) { > + for(i = 0; i < smp_cpus; i++) { > + pc_new_cpu(cpu_model); > + } > + } else { > + /* Xen require only one Qemu VCPU */ > pc_new_cpu(cpu_model); This looks a bit fishy. What is the semantic of -smp 2 or more in Xen mode? If that is an invalid/unused configuration option, catch that and reject it instead of installing this workaround. If it has a valid semantic, please elaborate why you need to restrict the number of instantiated cpus. Just to optimize memory usage? > } > } > @@ -980,6 +990,11 @@ void pc_memory_init(ram_addr_t ram_size, > *above_4g_mem_size_p = above_4g_mem_size; > *below_4g_mem_size_p = below_4g_mem_size; > > + if (xen_enabled()) { > + /* Nothing to do for Xen */ > + return; > + } > + This looks fragile /wrt potential future changes of pc_memory_init. Can't those bits Xen is interested in, ie. the above/below_4g_mem_size calculation, be moved into a separate function or even to the caller (should be trivial enough, the interface of pc_memory_init is clumsy in this regard anyway) so that you can simply skip pc_memory_init when in Xen mode? Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux