Re: [Qemu-devel] [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init

[Scott Wood <scottwood@freescale.com>]

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