On 03.07.2013, at 20:28, Scott Wood wrote: > On 07/03/2013 10:13:57 AM, Alexander Graf wrote: >> On 03.07.2013, at 15:53, Caraman Mihai Claudiu-B02008 wrote: >> >>> -#ifdef CONFIG_SPE >> >>> case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: { >> >>> - if (vcpu->arch.shared->msr & MSR_SPE) >> >>> - kvmppc_vcpu_enable_spe(vcpu); >> >>> - else >> >>> - kvmppc_booke_queue_irqprio(vcpu, >> >>> - >> >> BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL); >> >>> + if (kvmppc_supports_spe()) { >> >>> + bool enabled = false; >> >>> + >> >>> +#ifndef CONFIG_KVM_BOOKE_HV >> >>> + if (vcpu->arch.shared->msr & MSR_SPE) { >> >>> + kvmppc_vcpu_enable_spe(vcpu); >> >>> + enabled = true; >> >>> + } >> >>> +#endif >> >> >> >> Why the #ifdef? On HV capable systems kvmppc_supports_spe() will just >> >> always return false. >> > >> > AltiVec and SPE unavailable exceptions follows the same path. While >> > kvmppc_supports_spe() will always return false kvmppc_supports_altivec() >> > may not. >> There is no chip that supports SPE and HV at the same time. So we'll never >> hit this anyway, since kvmppc_supports_spe() always returns false on HV >> capable systems. >> Just add a comment saying so and remove the ifdef :). > > kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined. More > seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't interpret it as > SPE unless CONFIG_SPE is defined. And we can't rely on the "if > (kvmppc_supports_spe())" here because a later patch changes it to "if > (kvmppc_supports_altivec() || kvmppc_supports_spe())". So I think we still > need the ifdef CONFIG_SPE here. > > As for the HV ifndef, we should try not to confuse HV/PR with e500mc/e500v2, > even if we happen to only run HV on e500mc and PR on e500v2. We would not > want to call kvmppc_vcpu_enable_spe() here on a hypothetical HV target with > SPE. And we *would* want to call kvmppc_vcpu_enable_fp() here on a > hypothetical PR target with normal FP. It's one thing to leave out the > latter, since it would involve writing actual code that we have no way to > test at this point, but quite another to leave out the proper conditions for > when we want to run code that we do have.
So we should make this an #ifdef CONFIG_SPE rather than #ifndef CONFIG_KVM_BOOKE_HV? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html