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
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 | 2 ++
arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++++---------
2 files changed, 25 insertions(+), 9 deletions(-)
--
2.5.5
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).
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/reg.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 9cd4e8c..74c8079 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -377,6 +377,7 @@
#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) */
+#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 +1219,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
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@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;
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;
- }
}
spin_lock(&vc->lock);--
2.5.5
On Mon, Oct 31, 2016 at 11:28:23AM +1100, Suraj Jitindar Singh wrote:
quoted hunk
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@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.
On Mon, 2016-10-31 at 16:44 +1100, Paul Mackerras wrote:
On Mon, Oct 31, 2016 at 11:28:23AM +1100, Suraj Jitindar Singh wrote:
quoted
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@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
else if
host_pcr_bit = PCR_ARCH_206;
else
host_pcr_bit = PCR_ARCH_205;
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;
That approach is simpler and more extensible, I guess I don't really
like that it relies on the assumption that the PCR bits remain
consecutive and breaks if that assumption becomes invalid, which may
never be the case. I guess we can assume they will be for now and fix
it in the event that ever changes.
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.
Is that really necessary? I don't see the benefit and the code is more
readable in it's current form IMO.
Something like this?
unsigned long guest_pcr_bits = {0, /* 0 */
0, /* 1 */
PCR_ARCH_205, /* 0x0F000002 */
PCR_ARCH_206, /* 0x0F000003 */
PCR_ARCH_207, /* 0x0F000004 */
PCR_ARCH_300 }; /* 0x0F000005 */
if (arch_compat <= PVR_ARCH_300 && arch_compat >= PVR_ARCH_205)
guest_pcr_bit = guest_pcr_bits[arch_compat & 0xF];
else
return -EINVAL;
Paul.