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

[Scott Wood <scottwood@freescale.com>]

On 07/03/2013 01:42:12 PM, Alexander Graf wrote:
> 
> On 03.07.2013, at 20:28, Scott Wood wrote:
> 
> > On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
> >> 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?

I think it should be "#if !defined(CONFIG_KVM_BOOKE_HV) &&  
defined(CONFIG_SPE)" for the reasons I described in my second paragraph.

-Scott