[PATCH 2/6] KVM: PPC: Book3E: Refactor SPE/FP exit handling

[Alexander Graf]

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