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

[Scott Wood <scottwood@freescale.com>]

  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.

-Scott