[PATCH V2 2/2] powerpc/kvm: Update kvmppc_set_arch_compat() for ISA v3.00

[Paul Mackerras]

On Mon, Oct 31, 2016 at 11:28:23AM +1100, Suraj Jitindar Singh wrote:
> The function kvmppc_set_arch_compat() is used to determine the value of the
> processor compatibility register (PCR) for a guest running in a given
> compatibility mode. There is currently no support for v3.00 of the ISA.
> 
> Add support for v3.00 of the ISA which adds an ISA v2.07 compatilibity mode
> to the PCR.
> 
> We also add a check to ensure the processor we are running on is capable of
> emulating the chosen processor (for example a POWER7 cannot emulate a
> POWER8, similarly with a POWER8 and a POWER9).
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh at gmail.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 3686471..24681e7 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -311,24 +311,38 @@ static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
>  			 * If an arch bit is set in PCR, all the defined
>  			 * higher-order arch bits also have to be set.
>  			 */
> -			pcr = PCR_ARCH_206 | PCR_ARCH_205;
> +			if (cpu_has_feature(CPU_FTR_ARCH_206))
> +				pcr |= PCR_ARCH_205;
> +			if (cpu_has_feature(CPU_FTR_ARCH_207S))
> +				pcr |= PCR_ARCH_206;
> +			if (cpu_has_feature(CPU_FTR_ARCH_300))
> +				pcr |= PCR_ARCH_207;
>  			break;
>  		case PVR_ARCH_206:
>  		case PVR_ARCH_206p:
> -			pcr = PCR_ARCH_206;
> +			/* Must be at least v2.06 to (emulate) it */
> +			if (!cpu_has_feature(CPU_FTR_ARCH_206))
> +				return -EINVAL;
> +			if (cpu_has_feature(CPU_FTR_ARCH_207S))
> +				pcr |= PCR_ARCH_206;
> +			if (cpu_has_feature(CPU_FTR_ARCH_300))
> +				pcr |= PCR_ARCH_207;
>  			break;
>  		case PVR_ARCH_207:
> +			/* Must be at least v2.07 to (emulate) it */
> +			if (!cpu_has_feature(CPU_FTR_ARCH_207S))
> +				return -EINVAL;
> +			if (cpu_has_feature(CPU_FTR_ARCH_300))
> +				pcr |= PCR_ARCH_207;
> +			break;
> +		case PVR_ARCH_300:
> +			/* Must be at least v3.00 to (emulate) it */
> +			if (!cpu_has_feature(CPU_FTR_ARCH_300))
> +				return -EINVAL;
>  			break;

I can't help thinking that the repetitive structure of the lines
you're adding must imply a regularity that could be expressed more
concisely.  If you defined a dummy PCR_ARCH_300 bit as 0x10, perhaps
you could do something like this:

	if (cpu_has_feature(CPU_FTR_ARCH_300))
		host_pcr_bit = PCR_ARCH_300;
	else if (cpu_has_feature(CPU_FTR_ARCH_207S))
		host_pcr_bit = PCR_ARCH_207;
	else
		host_pcr_bit = PCR_ARCH_206;

	switch (arch_compat) {
	case PVR_ARCH_205:
		guest_pcr_bit = PCR_ARCH_205;
		break;
	case PVR_ARCH_206:
		guest_pcr_bit = PCR_ARCH_206;
		break;
	case PVR_ARCH_207:
	case PVR_ARCH_207S:
		guest_pcr_bit = PCR_ARCH_207;
		break;
	case PVR_ARCH_300:
		guest_pcr_bit = PCR_ARCH_300;
		break;
	default:
		return -EINVAL;
	}

	if (guest_pcr_bit > host_pcr_bit)
		return -EINVAL;

	pcr = host_pcr_bit - guest_pcr_bit;

The translation from arch_compat to guest_pcr_bit might look neater as
a table lookup on the low bits of arch_compat, after a bounds check.

Paul.