Re: [RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling

[Scott Wood <scottwood@freescale.com>]

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 <mihai.caraman@freescale.com>
> ---
>  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