From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56933) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9Sog-0003Ql-J3 for qemu-devel@nongnu.org; Mon, 11 Apr 2011 21:55:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q9Jle-0003fH-0o for qemu-devel@nongnu.org; Mon, 11 Apr 2011 12:15:11 -0400 Received: from tx2ehsobe003.messaging.microsoft.com ([65.55.88.13]:28674 helo=TX2EHSOBE005.bigfish.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9Jld-0003f8-Sc for qemu-devel@nongnu.org; Mon, 11 Apr 2011 12:15:09 -0400 Date: Mon, 11 Apr 2011 11:15:00 -0500 From: Scott Wood Message-ID: <20110411111500.2cf48662@schlenkerla.am.freescale.net> In-Reply-To: <17191B6E-7EDE-42A0-88CE-65F382E60DC6@suse.de> References: <20110408214810.GA16631@schlenkerla.am.freescale.net> <17191B6E-7EDE-42A0-88CE-65F382E60DC6@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-devel@nongnu.org On Sat, 9 Apr 2011 02:18:34 +0200 Alexander Graf wrote: > > -int kvm_arch_init_vcpu(CPUState *cenv) > > +static int kvm_arch_sync_sregs(CPUState *cenv) > > huh? So what about the previous caller of this? It's a new function. kvm_arch_init_vcpu still exists as a public function, "introduced" later in the patch. Diff doesn't know why this line is more important than the sregs definition. > > { > > - int ret = 0; > > struct kvm_sregs sregs; > > + int ret; > > Eh - this makes the patch less readable :) I can flip them around in the new function if you want, though having the longer declaration first looks a bit nicer to me. > > +#ifdef TARGET_PPC > > +#ifdef KVM_CAP_PPC_SEGSTATE > > This code never gets compiled without TARGET_PPC? Hmm, thought I checked that TARGET_PPC wasn't set in a TARGET_PPCEMB build, but now I see it is. Would be nice if we had a define specifically for non-PPCEMB. > > + if (!kvm_check_extension(cenv->kvm_state, KVM_CAP_PPC_SEGSTATE)) { > > + return 0; > > + } > > +#else > > + return 0; > > Doing a simple return 0 might lead to warnings (which become errors with -Werror) due to unused variables. I'm not sure how to handle this well. Maybe define KVM_CAP_PPC_SEGSTATE to something invalid when it's not defined? That way the capability check would fail and we don't need the duplicate code paths. Which variables would be unused? sregs/ret are used, just in a dead portion of the function. If the rest of the function had been ifdeffed out instead, it would be an issue. > > +#endif > > +#else /* TARGET_PPCEMB */ > > I guess you were #ifdefing on PPCEMB before? I don't think you really need to care about PPCEMB. The only reason it exists is for page size < 4k, which you don't hit on e500 IIUC. PPCEMB is how we've been running this so far... it also involves a larger target_phys_addr_t. I didn't know it was supposed to be supported at all under plain PPC. If that really is supposed to be supported, then we'll need a dynamic check here instead (based on excp_model?), but I don't see the value in supporting that. I did find it odd that all ppc platforms are being built for both PPC and PPCEMB. -Scott