[PATCH V4 0/2] powerpc: add support for ISA v2.07 compat level

[PATCH V4 0/2] powerpc: add support for ISA v2.07 compat level

[Suraj Jitindar Singh]

Version v3.00 of the ISA added a new compat level to the processor
compatibility register (PCR), an ISA v2.07 compatibility mode.

Upstream QEMU already supports this so it may as well go into the kernel
now.

Change Log:

V1 -> V2: 
- Reworked logic to set and mask the PCR, no functional change

V2 -> V3:
- Reworked logic again, no functional change

V3 -> V4:
- Added a comment in the first patch to clarify why a 'dummy' PCR v3.00
  value is needed

Suraj Jitindar Singh (2):
  powerpc: Define new ISA v3.00 logical PVR value and PCR register value
  powerpc/kvm: Update kvmppc_set_arch_compat() for ISA v3.00

 arch/powerpc/include/asm/reg.h | 11 +++++++++++
 arch/powerpc/kvm/book3s_hv.c   | 38 +++++++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.5.5

[PATCH V4 1/2] powerpc: Define new ISA v3.00 logical PVR value and PCR register value

[Suraj Jitindar Singh]

ISA 3.00 adds the logical PVR value 0x0f000005, so add a definition for
this.

Define PCR_ARCH_207 to reflect ISA 2.07 compatibility mode in the processor
compatibility register (PCR).

The next patch changes the algorithm used to determine the required PCR
value in the function kvmppc_set_arch_compat(). We use the PCR_ARCH_XXX
bits to specify and determine the compatibility level which we want to
emulate as well as the compatibility levels which the host is capable
of emulating. To show that we can emulate a v3.00 guest (which is actually
a v3.00 host with no compatility bits set, at the moment) we need a
PCR_ARCH_300 bit to represent this, however currently there is no such bit
defined by the ISA. Thus we define a 'dummy' v3.00 compat bit to be used.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arch/powerpc/include/asm/reg.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 9cd4e8c..30d897a 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -377,6 +377,16 @@
 #define   PCR_VEC_DIS	(1ul << (63-0))	/* Vec. disable (bit NA since POWER8) */
 #define   PCR_VSX_DIS	(1ul << (63-1))	/* VSX disable (bit NA since POWER8) */
 #define   PCR_TM_DIS	(1ul << (63-2))	/* Trans. memory disable (POWER8) */
+/*
+ * These bits are used in the function kvmppc_set_arch_compat() to specify and
+ * determine both the compatibility level which we want to emulate and the
+ * compatibility level which the host is capable of emulating. Thus we need a
+ * bit to show that we are capable of emulating an ISA v3.00 guest however as
+ * yet no such bit has been defined in the PCR register. Thus we have to define
+ * a 'dummy' value to be used.
+ */
+#define   PCR_ARCH_300	0x10		/* Dummy Architecture 3.00 */
+#define   PCR_ARCH_207	0x8		/* Architecture 2.07 */
 #define   PCR_ARCH_206	0x4		/* Architecture 2.06 */
 #define   PCR_ARCH_205	0x2		/* Architecture 2.05 */
 #define	SPRN_HEIR	0x153	/* Hypervisor Emulated Instruction Register */
@@ -1218,6 +1228,7 @@
 #define PVR_ARCH_206	0x0f000003
 #define PVR_ARCH_206p	0x0f100003
 #define PVR_ARCH_207	0x0f000004
+#define PVR_ARCH_300	0x0f000005
 
 /* Macros for setting and retrieving special purpose registers */
 #ifndef __ASSEMBLY__
-- 
2.5.5

Re: [PATCH V4 1/2] powerpc: Define new ISA v3.00 logical PVR value and PCR register value

[Paul Mackerras]

On Mon, Nov 14, 2016 at 11:35:07AM +1100, Suraj Jitindar Singh wrote:
> ISA 3.00 adds the logical PVR value 0x0f000005, so add a definition for
> this.
> 
> Define PCR_ARCH_207 to reflect ISA 2.07 compatibility mode in the processor
> compatibility register (PCR).
> 
> The next patch changes the algorithm used to determine the required PCR
> value in the function kvmppc_set_arch_compat(). We use the PCR_ARCH_XXX
> bits to specify and determine the compatibility level which we want to
> emulate as well as the compatibility levels which the host is capable
> of emulating. To show that we can emulate a v3.00 guest (which is actually
> a v3.00 host with no compatility bits set, at the moment) we need a
> PCR_ARCH_300 bit to represent this, however currently there is no such bit
> defined by the ISA. Thus we define a 'dummy' v3.00 compat bit to be used.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arch/powerpc/include/asm/reg.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 9cd4e8c..30d897a 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -377,6 +377,16 @@
>  #define   PCR_VEC_DIS	(1ul << (63-0))	/* Vec. disable (bit NA since POWER8) */
>  #define   PCR_VSX_DIS	(1ul << (63-1))	/* VSX disable (bit NA since POWER8) */
>  #define   PCR_TM_DIS	(1ul << (63-2))	/* Trans. memory disable (POWER8) */
> +/*
> + * These bits are used in the function kvmppc_set_arch_compat() to specify and
> + * determine both the compatibility level which we want to emulate and the
> + * compatibility level which the host is capable of emulating. Thus we need a
> + * bit to show that we are capable of emulating an ISA v3.00 guest however as
> + * yet no such bit has been defined in the PCR register. Thus we have to define
> + * a 'dummy' value to be used.
> + */

I don't want to bikeshed this to death, but I would suggest moving the
PCR_ARCH_300 definition to the next patch and to book3s_hv.c.  That
way you don't have to add the part of the comment that explains the
dummy value either.

> +#define   PCR_ARCH_300	0x10		/* Dummy Architecture 3.00 */
> +#define   PCR_ARCH_207	0x8		/* Architecture 2.07 */
>  #define   PCR_ARCH_206	0x4		/* Architecture 2.06 */
>  #define   PCR_ARCH_205	0x2		/* Architecture 2.05 */
>  #define	SPRN_HEIR	0x153	/* Hypervisor Emulated Instruction Register */

Paul.

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

[Suraj Jitindar Singh]

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).

Based on work by: Paul Mackerras <paulus@samba.org>

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arch/powerpc/kvm/book3s_hv.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 3686471..5d83ecb 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -301,39 +301,47 @@ static void kvmppc_set_pvr_hv(struct kvm_vcpu *vcpu, u32 pvr)
 
 static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
 {
-	unsigned long pcr = 0;
+	unsigned long host_pcr_bit = 0, guest_pcr_bit = 0;
 	struct kvmppc_vcore *vc = vcpu->arch.vcore;
 
+	/* We can (emulate) our own architecture version and anything older */
+	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 if (cpu_has_feature(CPU_FTR_ARCH_206))
+		host_pcr_bit = PCR_ARCH_206;
+	else
+		host_pcr_bit = PCR_ARCH_205;
+
+	/* Determine lowest PCR bit needed to run guest in given PVR level */
 	if (arch_compat) {
 		switch (arch_compat) {
 		case PVR_ARCH_205:
-			/*
-			 * 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;
+			guest_pcr_bit = PCR_ARCH_205;
 			break;
 		case PVR_ARCH_206:
 		case PVR_ARCH_206p:
-			pcr = PCR_ARCH_206;
+			guest_pcr_bit = PCR_ARCH_206;
 			break;
 		case PVR_ARCH_207:
+			guest_pcr_bit = PCR_ARCH_207;
+			break;
+		case PVR_ARCH_300:
+			guest_pcr_bit = PCR_ARCH_300;
 			break;
 		default:
 			return -EINVAL;
 		}
-
-		if (!cpu_has_feature(CPU_FTR_ARCH_207S)) {
-			/* POWER7 can't emulate POWER8 */
-			if (!(pcr & PCR_ARCH_206))
-				return -EINVAL;
-			pcr &= ~PCR_ARCH_206;
-		}
 	}
 
+	/* Check requested PCR bits don't exceed our capabilities */
+	if (guest_pcr_bit > host_pcr_bit)
+		return -EINVAL;
+
 	spin_lock(&vc->lock);
 	vc->arch_compat = arch_compat;
-	vc->pcr = pcr;
+	vc->pcr = host_pcr_bit - guest_pcr_bit;
 	spin_unlock(&vc->lock);
 
 	return 0;
-- 
2.5.5

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

[Paul Mackerras]

On Mon, Nov 14, 2016 at 11:35:08AM +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).
> 
> Based on work by: Paul Mackerras <paulus@samba.org>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 3686471..5d83ecb 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -301,39 +301,47 @@ static void kvmppc_set_pvr_hv(struct kvm_vcpu *vcpu, u32 pvr)

I would suggest adding here:

/* Dummy value to be used until a successor to POWER9 appears */
#define PCR_ARCH_300 (PCR_ARCH_207 << 1)

>  static int kvmppc_set_arch_compat(struct kvm_vcpu *vcpu, u32 arch_compat)
>  {
> -	unsigned long pcr = 0;
> +	unsigned long host_pcr_bit = 0, guest_pcr_bit = 0;
>  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>  
> +	/* We can (emulate) our own architecture version and anything older */
> +	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 if (cpu_has_feature(CPU_FTR_ARCH_206))
> +		host_pcr_bit = PCR_ARCH_206;
> +	else
> +		host_pcr_bit = PCR_ARCH_205;

I don't think you need this last case because we refuse to run on any
CPU that doesn't at least have CPU_FTR_ARCH_206 - see
kvmppc_core_check_processor_compat_hv().

> +
> +	/* Determine lowest PCR bit needed to run guest in given PVR level */
>  	if (arch_compat) {
>  		switch (arch_compat) {
>  		case PVR_ARCH_205:
> -			/*
> -			 * 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;
> +			guest_pcr_bit = PCR_ARCH_205;
>  			break;
>  		case PVR_ARCH_206:
>  		case PVR_ARCH_206p:
> -			pcr = PCR_ARCH_206;
> +			guest_pcr_bit = PCR_ARCH_206;
>  			break;
>  		case PVR_ARCH_207:
> +			guest_pcr_bit = PCR_ARCH_207;
> +			break;
> +		case PVR_ARCH_300:
> +			guest_pcr_bit = PCR_ARCH_300;
>  			break;
>  		default:
>  			return -EINVAL;
>  		}
> -
> -		if (!cpu_has_feature(CPU_FTR_ARCH_207S)) {
> -			/* POWER7 can't emulate POWER8 */
> -			if (!(pcr & PCR_ARCH_206))
> -				return -EINVAL;
> -			pcr &= ~PCR_ARCH_206;
> -		}
>  	}
>  
> +	/* Check requested PCR bits don't exceed our capabilities */
> +	if (guest_pcr_bit > host_pcr_bit)
> +		return -EINVAL;
> +
>  	spin_lock(&vc->lock);
>  	vc->arch_compat = arch_compat;
> -	vc->pcr = pcr;
> +	vc->pcr = host_pcr_bit - guest_pcr_bit;

This is probably worth a comment.  Something like "This sets each bit
for which guest_pcr_bit <= bit < host_pcr_bit."

Paul.