| Message ID | 20111221013445.GO8378@schlenkerla.am.freescale.net |
|---|---|
| State | New, archived |
| Headers | show |
On 21.12.2011, at 02:34, Scott Wood wrote: > e500mc has a normal PPC FPU, rather than SPE which is found > on e500v1/v2. > > Based on code from Liu Yu <yu.liu@freescale.com>. > > Signed-off-by: Scott Wood <scottwood@freescale.com> > --- > arch/powerpc/include/asm/system.h | 1 + > arch/powerpc/kvm/booke.c | 44 +++++++++++++++++++++++++++++++++++++ > arch/powerpc/kvm/booke.h | 30 +++++++++++++++++++++++++ > 3 files changed, 75 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h > index e30a13d..0561356 100644 > --- a/arch/powerpc/include/asm/system.h > +++ b/arch/powerpc/include/asm/system.h > @@ -140,6 +140,7 @@ extern void via_cuda_init(void); > extern void read_rtc_time(void); > extern void pmac_find_display(void); > extern void giveup_fpu(struct task_struct *); > +extern void load_up_fpu(void); > extern void disable_kernel_fp(void); > extern void enable_kernel_fp(void); > extern void flush_fp_to_thread(struct task_struct *); > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index cf63b93..4bf43f9 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -460,6 +460,11 @@ void kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) > int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) > { > int ret; > +#ifdef CONFIG_PPC_FPU > + unsigned int fpscr; > + int fpexc_mode; > + u64 fpr[32]; > +#endif > > if (!vcpu->arch.sane) { > kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > @@ -482,7 +487,46 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) > } > > kvm_guest_enter(); > + > +#ifdef CONFIG_PPC_FPU > + /* Save userspace FPU state in stack */ > + enable_kernel_fp(); > + memcpy(fpr, current->thread.fpr, sizeof(current->thread.fpr)); > + fpscr = current->thread.fpscr.val; > + fpexc_mode = current->thread.fpexc_mode; > + > + /* Restore guest FPU state to thread */ > + memcpy(current->thread.fpr, vcpu->arch.fpr, sizeof(vcpu->arch.fpr)); > + current->thread.fpscr.val = vcpu->arch.fpscr; > + > + /* > + * Since we can't trap on MSR_FP in GS-mode, we consider the guest > + * as always using the FPU. Kernel usage of FP (via > + * enable_kernel_fp()) in this thread must not occur while > + * vcpu->fpu_active is set. > + */ > + vcpu->fpu_active = 1; > + > + kvmppc_load_guest_fp(vcpu); > +#endif Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here? > + > ret = __kvmppc_vcpu_run(kvm_run, vcpu); > + > +#ifdef CONFIG_PPC_FPU > + kvmppc_save_guest_fp(vcpu); > + > + vcpu->fpu_active = 0; > + > + /* Save guest FPU state from thread */ > + memcpy(vcpu->arch.fpr, current->thread.fpr, sizeof(vcpu->arch.fpr)); > + vcpu->arch.fpscr = current->thread.fpscr.val; > + > + /* Restore userspace FPU state from stack */ > + memcpy(current->thread.fpr, fpr, sizeof(current->thread.fpr)); > + current->thread.fpscr.val = fpscr; > + current->thread.fpexc_mode = fpexc_mode; > +#endif > + > kvm_guest_exit(); > > out: > diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h > index d53bcf2..3bf5eda 100644 > --- a/arch/powerpc/kvm/booke.h > +++ b/arch/powerpc/kvm/booke.h > @@ -96,4 +96,34 @@ enum int_class { > > void kvmppc_set_pending_interrupt(struct kvm_vcpu *vcpu, enum int_class type); > > +/* > + * Load up guest vcpu FP state if it's needed. > + * It also set the MSR_FP in thread so that host know > + * we're holding FPU, and then host can help to save > + * guest vcpu FP state if other threads require to use FPU. > + * This simulates an FP unavailable fault. > + * > + * It requires to be called with preemption disabled. > + */ > +static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) > +{ > +#ifdef CONFIG_PPC_FPU > + if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) { > + load_up_fpu(); > + current->thread.regs->msr |= MSR_FP; I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/09/2012 11:48 AM, Alexander Graf wrote: > > On 21.12.2011, at 02:34, Scott Wood wrote: >> +#ifdef CONFIG_PPC_FPU >> + /* Save userspace FPU state in stack */ >> + enable_kernel_fp(); >> + memcpy(fpr, current->thread.fpr, sizeof(current->thread.fpr)); >> + fpscr = current->thread.fpscr.val; >> + fpexc_mode = current->thread.fpexc_mode; >> + >> + /* Restore guest FPU state to thread */ >> + memcpy(current->thread.fpr, vcpu->arch.fpr, sizeof(vcpu->arch.fpr)); >> + current->thread.fpscr.val = vcpu->arch.fpscr; >> + >> + /* >> + * Since we can't trap on MSR_FP in GS-mode, we consider the guest >> + * as always using the FPU. Kernel usage of FP (via >> + * enable_kernel_fp()) in this thread must not occur while >> + * vcpu->fpu_active is set. >> + */ >> + vcpu->fpu_active = 1; >> + >> + kvmppc_load_guest_fp(vcpu); >> +#endif > > Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here? book3s_pr is a bit different in that it can trap when the guest sets MSR[FP]. Maybe a few lines could be factored out (the first memcpy, fpscr, fpexc_mode). I'm not sure that it makes sense given the lack of isolation between what it's doing and what the rest of the code is doing. >> +/* >> + * Load up guest vcpu FP state if it's needed. >> + * It also set the MSR_FP in thread so that host know >> + * we're holding FPU, and then host can help to save >> + * guest vcpu FP state if other threads require to use FPU. >> + * This simulates an FP unavailable fault. >> + * >> + * It requires to be called with preemption disabled. >> + */ >> +static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) >> +{ >> +#ifdef CONFIG_PPC_FPU >> + if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) { >> + load_up_fpu(); >> + current->thread.regs->msr |= MSR_FP; > > I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). shadow_msr is the real MSR. shared->msr is the guest's view of MSR. current->thread.regs->msr is nominally userspace's MSR. In this case we use it to tell host Linux that FP is in use and must be saved on context switch. The actual userspace MSR_FP is known to be clear at this point because we called enable_kernel_fp(). It will be clear again when we return to userspace because we'll call giveup_fpu(). -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.01.2012, at 22:48, Scott Wood wrote: > On 01/09/2012 11:48 AM, Alexander Graf wrote: >> >> On 21.12.2011, at 02:34, Scott Wood wrote: >>> +#ifdef CONFIG_PPC_FPU >>> + /* Save userspace FPU state in stack */ >>> + enable_kernel_fp(); >>> + memcpy(fpr, current->thread.fpr, sizeof(current->thread.fpr)); >>> + fpscr = current->thread.fpscr.val; >>> + fpexc_mode = current->thread.fpexc_mode; >>> + >>> + /* Restore guest FPU state to thread */ >>> + memcpy(current->thread.fpr, vcpu->arch.fpr, sizeof(vcpu->arch.fpr)); >>> + current->thread.fpscr.val = vcpu->arch.fpscr; >>> + >>> + /* >>> + * Since we can't trap on MSR_FP in GS-mode, we consider the guest >>> + * as always using the FPU. Kernel usage of FP (via >>> + * enable_kernel_fp()) in this thread must not occur while >>> + * vcpu->fpu_active is set. >>> + */ >>> + vcpu->fpu_active = 1; >>> + >>> + kvmppc_load_guest_fp(vcpu); >>> +#endif >> >> Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here? > > book3s_pr is a bit different in that it can trap when the guest sets > MSR[FP]. Ah, there's no doorbell? So you always have to swap fpu registers? You still have to enable it manually when preempting in, right? IIRC ppc32 does lazy fpu activation. > Maybe a few lines could be factored out (the first memcpy, fpscr, > fpexc_mode). I'm not sure that it makes sense given the lack of > isolation between what it's doing and what the rest of the code is doing. Yeah, looking at the code it does look pretty different. Too bad - I would've hoped to throw the vmx code in as well so we could get vmx/vsx/whatever for free later. > >>> +/* >>> + * Load up guest vcpu FP state if it's needed. >>> + * It also set the MSR_FP in thread so that host know >>> + * we're holding FPU, and then host can help to save >>> + * guest vcpu FP state if other threads require to use FPU. >>> + * This simulates an FP unavailable fault. >>> + * >>> + * It requires to be called with preemption disabled. >>> + */ >>> +static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) >>> +{ >>> +#ifdef CONFIG_PPC_FPU >>> + if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) { >>> + load_up_fpu(); >>> + current->thread.regs->msr |= MSR_FP; >> >> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). > > shadow_msr is the real MSR. > > shared->msr is the guest's view of MSR. > > current->thread.regs->msr is nominally userspace's MSR. In this case we > use it to tell host Linux that FP is in use and must be saved on context > switch. The actual userspace MSR_FP is known to be clear at this point > because we called enable_kernel_fp(). It will be clear again when we > return to userspace because we'll call giveup_fpu(). Ah, this is thread.regs, not vcpu.regs. Sorry, I misread that part. This way it obviously makes a lot more sense. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/09/2012 04:17 PM, Alexander Graf wrote: > > On 09.01.2012, at 22:48, Scott Wood wrote: > >> On 01/09/2012 11:48 AM, Alexander Graf wrote: >>> >>> Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here? >> >> book3s_pr is a bit different in that it can trap when the guest sets >> MSR[FP]. > > Ah, there's no doorbell? So you always have to swap fpu registers? You still have to enable it manually when preempting in, right? IIRC ppc32 does lazy fpu activation. Right. Preempting in is handled by calling kvmppc_load_guest_fp() (which should be renamed to be booke-specific, since the semantics are tied to booke.c) from kvmppc_core_vcpu_load() in e500mc.c. >>> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). >> >> shadow_msr is the real MSR. >> >> shared->msr is the guest's view of MSR. Correction -- this applies to PR-mode (e500v2). In GS-mode, shadow_msr is not used. The guest sees the real MSR (hw silently prevents it from modifying certain bits), which gets saved on exit into shared->msr. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.01.2012, at 23:39, Scott Wood wrote: > On 01/09/2012 04:17 PM, Alexander Graf wrote: >> >> On 09.01.2012, at 22:48, Scott Wood wrote: >> >>> On 01/09/2012 11:48 AM, Alexander Graf wrote: >>>> >>>> Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here? >>> >>> book3s_pr is a bit different in that it can trap when the guest sets >>> MSR[FP]. >> >> Ah, there's no doorbell? So you always have to swap fpu registers? You still have to enable it manually when preempting in, right? IIRC ppc32 does lazy fpu activation. > > Right. > > Preempting in is handled by calling kvmppc_load_guest_fp() (which should > be renamed to be booke-specific, since the semantics are tied to > booke.c) from kvmppc_core_vcpu_load() in e500mc.c. Ah, and that one's called on sched_in. All is well then :). > >>>> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). >>> >>> shadow_msr is the real MSR. >>> >>> shared->msr is the guest's view of MSR. > > Correction -- this applies to PR-mode (e500v2). > > In GS-mode, shadow_msr is not used. The guest sees the real MSR (hw > silently prevents it from modifying certain bits), which gets saved on > exit into shared->msr. Hrm. Can we maybe #ifdef out shadow_msr on HV then? I'm really getting confused with having 3 potential msr variables in the vcpu struct. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/09/2012 04:47 PM, Alexander Graf wrote: > > On 09.01.2012, at 23:39, Scott Wood wrote: > >> On 01/09/2012 04:17 PM, Alexander Graf wrote: >>> >>> On 09.01.2012, at 22:48, Scott Wood wrote: >>> >>>> On 01/09/2012 11:48 AM, Alexander Graf wrote: >>>>> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). >>>> >>>> shadow_msr is the real MSR. >>>> >>>> shared->msr is the guest's view of MSR. >> >> Correction -- this applies to PR-mode (e500v2). >> >> In GS-mode, shadow_msr is not used. The guest sees the real MSR (hw >> silently prevents it from modifying certain bits), which gets saved on >> exit into shared->msr. > > Hrm. Can we maybe #ifdef out shadow_msr on HV then? I'm really getting confused with having 3 potential msr variables in the vcpu struct. An ifdef would take us further down the road of not being able to support both in the same kernel image (not sure whether that's a long-term goal -- probably won't happen any time soon with e500v2+e500mc even disregarding KVM, but maybe it'll be relevant on some other chips), and in general increase the mess in the struct definition. How about a comment? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.01.2012, at 23:54, Scott Wood wrote: > On 01/09/2012 04:47 PM, Alexander Graf wrote: >> >> On 09.01.2012, at 23:39, Scott Wood wrote: >> >>> On 01/09/2012 04:17 PM, Alexander Graf wrote: >>>> >>>> On 09.01.2012, at 22:48, Scott Wood wrote: >>>> >>>>> On 01/09/2012 11:48 AM, Alexander Graf wrote: >>>>>> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :). >>>>> >>>>> shadow_msr is the real MSR. >>>>> >>>>> shared->msr is the guest's view of MSR. >>> >>> Correction -- this applies to PR-mode (e500v2). >>> >>> In GS-mode, shadow_msr is not used. The guest sees the real MSR (hw >>> silently prevents it from modifying certain bits), which gets saved on >>> exit into shared->msr. >> >> Hrm. Can we maybe #ifdef out shadow_msr on HV then? I'm really getting confused with having 3 potential msr variables in the vcpu struct. > > An ifdef would take us further down the road of not being able to > support both in the same kernel image (not sure whether that's a > long-term goal -- probably won't happen any time soon with e500v2+e500mc > even disregarding KVM, but maybe it'll be relevant on some other chips), > and in general increase the mess in the struct definition. How about a > comment? Well, I'd like to make sure we don't accidentally access the wrong field. But yes, a comment should be ok. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/include/asm/system.h b/arch/powerpc/include/asm/system.h index e30a13d..0561356 100644 --- a/arch/powerpc/include/asm/system.h +++ b/arch/powerpc/include/asm/system.h @@ -140,6 +140,7 @@ extern void via_cuda_init(void); extern void read_rtc_time(void); extern void pmac_find_display(void); extern void giveup_fpu(struct task_struct *); +extern void load_up_fpu(void); extern void disable_kernel_fp(void); extern void enable_kernel_fp(void); extern void flush_fp_to_thread(struct task_struct *); diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index cf63b93..4bf43f9 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -460,6 +460,11 @@ void kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) { int ret; +#ifdef CONFIG_PPC_FPU + unsigned int fpscr; + int fpexc_mode; + u64 fpr[32]; +#endif if (!vcpu->arch.sane) { kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR; @@ -482,7 +487,46 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) } kvm_guest_enter(); + +#ifdef CONFIG_PPC_FPU + /* Save userspace FPU state in stack */ + enable_kernel_fp(); + memcpy(fpr, current->thread.fpr, sizeof(current->thread.fpr)); + fpscr = current->thread.fpscr.val; + fpexc_mode = current->thread.fpexc_mode; + + /* Restore guest FPU state to thread */ + memcpy(current->thread.fpr, vcpu->arch.fpr, sizeof(vcpu->arch.fpr)); + current->thread.fpscr.val = vcpu->arch.fpscr; + + /* + * Since we can't trap on MSR_FP in GS-mode, we consider the guest + * as always using the FPU. Kernel usage of FP (via + * enable_kernel_fp()) in this thread must not occur while + * vcpu->fpu_active is set. + */ + vcpu->fpu_active = 1; + + kvmppc_load_guest_fp(vcpu); +#endif + ret = __kvmppc_vcpu_run(kvm_run, vcpu); + +#ifdef CONFIG_PPC_FPU + kvmppc_save_guest_fp(vcpu); + + vcpu->fpu_active = 0; + + /* Save guest FPU state from thread */ + memcpy(vcpu->arch.fpr, current->thread.fpr, sizeof(vcpu->arch.fpr)); + vcpu->arch.fpscr = current->thread.fpscr.val; + + /* Restore userspace FPU state from stack */ + memcpy(current->thread.fpr, fpr, sizeof(current->thread.fpr)); + current->thread.fpscr.val = fpscr; + current->thread.fpexc_mode = fpexc_mode; +#endif + kvm_guest_exit(); out: diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h index d53bcf2..3bf5eda 100644 --- a/arch/powerpc/kvm/booke.h +++ b/arch/powerpc/kvm/booke.h @@ -96,4 +96,34 @@ enum int_class { void kvmppc_set_pending_interrupt(struct kvm_vcpu *vcpu, enum int_class type); +/* + * Load up guest vcpu FP state if it's needed. + * It also set the MSR_FP in thread so that host know + * we're holding FPU, and then host can help to save + * guest vcpu FP state if other threads require to use FPU. + * This simulates an FP unavailable fault. + * + * It requires to be called with preemption disabled. + */ +static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) +{ +#ifdef CONFIG_PPC_FPU + if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) { + load_up_fpu(); + current->thread.regs->msr |= MSR_FP; + } +#endif +} + +/* + * Save guest vcpu FP state into thread. + * It requires to be called with preemption disabled. + */ +static inline void kvmppc_save_guest_fp(struct kvm_vcpu *vcpu) +{ +#ifdef CONFIG_PPC_FPU + if (vcpu->fpu_active && (current->thread.regs->msr & MSR_FP)) + giveup_fpu(current); +#endif +} #endif /* __KVM_BOOKE_H__ */
e500mc has a normal PPC FPU, rather than SPE which is found on e500v1/v2. Based on code from Liu Yu <yu.liu@freescale.com>. Signed-off-by: Scott Wood <scottwood@freescale.com> --- arch/powerpc/include/asm/system.h | 1 + arch/powerpc/kvm/booke.c | 44 +++++++++++++++++++++++++++++++++++++ arch/powerpc/kvm/booke.h | 30 +++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 0 deletions(-)