Re: [RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling
On 06/05/2013 02:29:47 AM, Caraman Mihai Claudiu-B02008 wrote: > > case BOOKE_INTERRUPT_SPE_FP_ROUND: > > +#ifdef CONFIG_SPE > > kvmppc_booke_queue_irqprio(vcpu, > > BOOKE_IRQPRIO_SPE_FP_ROUND); > > r = RESUME_GUEST; > > break; > > Why not use kvmppc_supports_spe() here, for consistency? I added cpu_has_feature(CPU_FTR_SPE) for the case specified above, but here SPE_FP_ROUND is not shared with ALTIVEC. CONFIG_SPE is used in other places in KVM without this check, shouldn't be all or nothing? I'd rather it be consistent, at least between handling one exception and another. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling
> > + /* > > +* The interrupt is shared, KVM support for the > > featured unit > > +* is detected at run-time. > > +*/ > > This is a decent comment for the changelog, but for the code itself it > seems fairly obvious if you look at the definition of > kvmppc_supports_spe(). I will move it to change log. > > > + bool handled = false; > > + > > + if (kvmppc_supports_spe()) { > > +#ifdef CONFIG_SPE > > + if (cpu_has_feature(CPU_FTR_SPE)) > > Didn't you already check this using kvmppc_supports_spe()? It makes sense with the next patch. It handles the improbable case of having CONFIG_ALTIVEC and CONFIG_SPE defined: if (kvmppc_supports_altivec() || kvmppc_supports_spe()). > > > case BOOKE_INTERRUPT_SPE_FP_ROUND: > > +#ifdef CONFIG_SPE > > kvmppc_booke_queue_irqprio(vcpu, > > BOOKE_IRQPRIO_SPE_FP_ROUND); > > r = RESUME_GUEST; > > break; > > Why not use kvmppc_supports_spe() here, for consistency? I added cpu_has_feature(CPU_FTR_SPE) for the case specified above, but here SPE_FP_ROUND is not shared with ALTIVEC. CONFIG_SPE is used in other places in KVM without this check, shouldn't be all or nothing? -Mike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling
On 06/03/2013 03:54:24 PM, Mihai Caraman wrote: SPE_FP interrupts are shared with ALTIVEC. Refactor SPE_FP exit handling to detect KVM support for the featured unit at run-time, in order to accommodate ALTIVEC later. Signed-off-by: Mihai Caraman --- arch/powerpc/kvm/booke.c | 80 ++ 1 files changed, 59 insertions(+), 21 deletions(-) diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 1020119..d082bbc 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -822,6 +822,15 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu, } } +static inline bool kvmppc_supports_spe(void) +{ +#ifdef CONFIG_SPE + if (cpu_has_feature(CPU_FTR_SPE)) + return true; +#endif + return false; +} Whitespace /** * kvmppc_handle_exit * @@ -931,42 +940,71 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, r = RESUME_GUEST; break; -#ifdef CONFIG_SPE case BOOKE_INTERRUPT_SPE_UNAVAIL: { - if (vcpu->arch.shared->msr & MSR_SPE) - kvmppc_vcpu_enable_spe(vcpu); - else - kvmppc_booke_queue_irqprio(vcpu, - BOOKE_IRQPRIO_SPE_UNAVAIL); + /* + * The interrupt is shared, KVM support for the featured unit +* is detected at run-time. +*/ This is a decent comment for the changelog, but for the code itself it seems fairly obvious if you look at the definition of kvmppc_supports_spe(). + bool handled = false; + + if (kvmppc_supports_spe()) { +#ifdef CONFIG_SPE + if (cpu_has_feature(CPU_FTR_SPE)) Didn't you already check this using kvmppc_supports_spe()? case BOOKE_INTERRUPT_SPE_FP_ROUND: +#ifdef CONFIG_SPE kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_ROUND); r = RESUME_GUEST; break; Why not use kvmppc_supports_spe() here, for consistency? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling
SPE_FP interrupts are shared with ALTIVEC. Refactor SPE_FP exit handling to detect KVM support for the featured unit at run-time, in order to accommodate ALTIVEC later. Signed-off-by: Mihai Caraman --- arch/powerpc/kvm/booke.c | 80 ++ 1 files changed, 59 insertions(+), 21 deletions(-) diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 1020119..d082bbc 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -822,6 +822,15 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu, } } +static inline bool kvmppc_supports_spe(void) +{ +#ifdef CONFIG_SPE + if (cpu_has_feature(CPU_FTR_SPE)) + return true; +#endif + return false; +} + /** * kvmppc_handle_exit * @@ -931,42 +940,71 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, r = RESUME_GUEST; break; -#ifdef CONFIG_SPE case BOOKE_INTERRUPT_SPE_UNAVAIL: { - if (vcpu->arch.shared->msr & MSR_SPE) - kvmppc_vcpu_enable_spe(vcpu); - else - kvmppc_booke_queue_irqprio(vcpu, - BOOKE_IRQPRIO_SPE_UNAVAIL); + /* +* The interrupt is shared, KVM support for the featured unit +* is detected at run-time. +*/ + bool handled = false; + + if (kvmppc_supports_spe()) { +#ifdef CONFIG_SPE + if (cpu_has_feature(CPU_FTR_SPE)) + if (vcpu->arch.shared->msr & MSR_SPE) { + kvmppc_vcpu_enable_spe(vcpu); + handled = true; + } +#endif + if (!handled) + kvmppc_booke_queue_irqprio(vcpu, + BOOKE_IRQPRIO_SPE_UNAVAIL); + } else { + /* +* Guest wants SPE, but host kernel doesn't support it. +* Send an "unimplemented operation" program check to +* the guest. +*/ + kvmppc_core_queue_program(vcpu, ESR_PUO | ESR_SPV); + } + r = RESUME_GUEST; break; } case BOOKE_INTERRUPT_SPE_FP_DATA: - kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_DATA); - r = RESUME_GUEST; + /* +* The interrupt is shared, KVM support for the featured unit +* is detected at run-time. +*/ + if (kvmppc_supports_spe()) { + kvmppc_booke_queue_irqprio(vcpu, + BOOKE_IRQPRIO_SPE_FP_DATA); + r = RESUME_GUEST; + } else { + /* +* These really should never happen without CONFIG_SPE, +* as we should never enable the real MSR[SPE] in the +* guest. +*/ + printk(KERN_CRIT "%s: unexpected SPE interrupt %u at \ + %08lx\n", __func__, exit_nr, vcpu->arch.pc); + run->hw.hardware_exit_reason = exit_nr; + r = RESUME_HOST; + } + break; case BOOKE_INTERRUPT_SPE_FP_ROUND: +#ifdef CONFIG_SPE kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_SPE_FP_ROUND); r = RESUME_GUEST; break; #else - case BOOKE_INTERRUPT_SPE_UNAVAIL: /* -* Guest wants SPE, but host kernel doesn't support it. Send -* an "unimplemented operation" program check to the guest. +* These really should never happen without CONFIG_SPE, +* as we should never enable the real MSR[SPE] in the +* guest. */ - kvmppc_core_queue_program(vcpu, ESR_PUO | ESR_SPV); - r = RESUME_GUEST; - break; - - /* -* These really should never happen without CONFIG_SPE, -* as we should never enable the real MSR[SPE] in the guest. -*/ - case BOOKE_INTERRUPT_SPE_FP_DATA: - case BOOKE_INTERRUPT_SPE_FP_ROUND: printk(KERN_CRIT "%s: unexpected SPE interrupt %u at %08lx\n", __func__, exit_nr, vcpu->arch.pc); run->hw.hardware_exit_reason = exit_nr; -- 1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html