On Sat, 9 Apr 2011 02:18:34 +0200 Alexander Graf <ag...@suse.de> 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