From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Tue, 04 Jun 2013 22:14:48 +0000
Subject: Re: [RFC PATCH 2/6] KVM: PPC: Book3E: Refactor SPE_FP exit handling
Message-Id: <1370384088.748.17@snotra>
List-Id: <kvm-ppc.vger.kernel.org>
References: <1370292868-2697-1-git-send-email-mihai.caraman@freescale.com>
	<1370292868-2697-3-git-send-email-mihai.caraman@freescale.com>
In-Reply-To: <1370292868-2697-3-git-send-email-mihai.caraman@freescale.com>
	(from mihai.caraman@freescale.com on Mon Jun  3 15:54:24 2013)
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Mihai Caraman <mihai.caraman@freescale.com>
Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Mihai Caraman <mihai.caraman@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