| Message ID | 20110408214810.GA16631@schlenkerla.am.freescale.net |
|---|---|
| State | New |
| Headers | show |
On 08.04.2011, at 23:48, Scott Wood wrote: > Classic/server ppc has had SREGS for a while now (though I think not > always?), but it's still missing for booke. Check the capability before > calling KVM_SET_SREGS. > > Without this, booke kvm fails to boot as of commit > 84b4915dd2c0eaa86c970ffc42a68ea8ba9e48b5 (kvm: Handle kvm_init_vcpu > errors). Oops :). > > Also, don't write random stack state into the non-PVR sregs fields -- > have kvm fill it in first. > > Eventually booke will have sregs and it will have its own capability to > be tested here. However, we will want a way for platform code to request > to look like the actual CPU we're running on, especially if SoC devices > are being directly assigned. > > Signed-off-by: Scott Wood <scottwood@freescale.com> > --- > target-ppc/kvm.c | 33 ++++++++++++++++++++++++++++++--- > 1 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index 2cfb24b..5401536 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -77,13 +77,40 @@ int kvm_arch_init(KVMState *s) > return 0; > } > > -int kvm_arch_init_vcpu(CPUState *cenv) > +static int kvm_arch_sync_sregs(CPUState *cenv) huh? So what about the previous caller of this? > { > - int ret = 0; > struct kvm_sregs sregs; > + int ret; Eh - this makes the patch less readable :) > + > +#ifdef TARGET_PPC > +#ifdef KVM_CAP_PPC_SEGSTATE This code never gets compiled without TARGET_PPC? > + 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. > +#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. > + return 0; > +#endif > + > + ret = kvm_vcpu_ioctl(cenv, KVM_GET_SREGS, &sregs); > + if (ret) { > + return ret; > + } > > sregs.pvr = cenv->spr[SPR_PVR]; > - ret = kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs); > + return kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs); > +} > + > +int kvm_arch_init_vcpu(CPUState *cenv) > +{ > + int ret; > + > + ret = kvm_arch_sync_sregs(cenv); > + if (ret) { > + return ret; > + } > > idle_timer = qemu_new_timer_ns(vm_clock, kvm_kick_env, cenv); > > -- > 1.7.1 > Alex
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
On 11.04.2011, at 18:15, Scott Wood wrote: > 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. I've had gcc be overly clever in situations like this before, where it detects the return is unconditional and just removes all the code after it, generating unused warnings. > >>> +#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. I don't disagree that it's odd, but that's the way it is. We even support 32-bit CPUs in the PPC64 target. This patch surely isn't in the scope of changing it :). So yes, all the code has to work with qemu-system-ppc. Alex
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 2cfb24b..5401536 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -77,13 +77,40 @@ int kvm_arch_init(KVMState *s) return 0; } -int kvm_arch_init_vcpu(CPUState *cenv) +static int kvm_arch_sync_sregs(CPUState *cenv) { - int ret = 0; struct kvm_sregs sregs; + int ret; + +#ifdef TARGET_PPC +#ifdef KVM_CAP_PPC_SEGSTATE + if (!kvm_check_extension(cenv->kvm_state, KVM_CAP_PPC_SEGSTATE)) { + return 0; + } +#else + return 0; +#endif +#else /* TARGET_PPCEMB */ + return 0; +#endif + + ret = kvm_vcpu_ioctl(cenv, KVM_GET_SREGS, &sregs); + if (ret) { + return ret; + } sregs.pvr = cenv->spr[SPR_PVR]; - ret = kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs); + return kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs); +} + +int kvm_arch_init_vcpu(CPUState *cenv) +{ + int ret; + + ret = kvm_arch_sync_sregs(cenv); + if (ret) { + return ret; + } idle_timer = qemu_new_timer_ns(vm_clock, kvm_kick_env, cenv);
Classic/server ppc has had SREGS for a while now (though I think not always?), but it's still missing for booke. Check the capability before calling KVM_SET_SREGS. Without this, booke kvm fails to boot as of commit 84b4915dd2c0eaa86c970ffc42a68ea8ba9e48b5 (kvm: Handle kvm_init_vcpu errors). Also, don't write random stack state into the non-PVR sregs fields -- have kvm fill it in first. Eventually booke will have sregs and it will have its own capability to be tested here. However, we will want a way for platform code to request to look like the actual CPU we're running on, especially if SoC devices are being directly assigned. Signed-off-by: Scott Wood <scottwood@freescale.com> --- target-ppc/kvm.c | 33 ++++++++++++++++++++++++++++++--- 1 files changed, 30 insertions(+), 3 deletions(-)