[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init
From: |
Scott Wood |
Subject: |
Re: [Qemu-devel] [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init |
Date: |
Mon, 11 Apr 2011 11:15:00 -0500 |
On Sat, 9 Apr 2011 02:18:34 +0200
Alexander Graf <address@hidden> 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