[kvm-unit-tests PATCH 0/5] s390x: Dirty cc before executing tested instructions

[kvm-unit-tests PATCH 0/5] s390x: Dirty cc before executing tested instructions

[Janosch Frank]

A recent s390 KVM fixpatch [1] showed us that checking the cc is not
enough when emulation code forgets to set the cc. There might just be
the correct cc in the PSW which would make the cc check succeed.

This series intentionally dirties the cc for sigp, uvc, some io
instructions and sclp to make cc setting errors more apparent. I had a
cursory look through the tested instructions and those are the most
prominent ones with defined cc values.

Since the issue appeared in PQAP my AP test series is now dependent on
this series.

[1] https://lore.kernel.org/kvm/20231201181657.1614645-1-farman@linux.ibm.com/

Janosch Frank (5):
  lib: s390x: Add spm cc shift constant
  lib: s390x: sigp: Dirty CC before sigp execution
  lib: s390x: uv: Dirty CC before uvc execution
  lib: s390x: css: Dirty CC before css instructions
  s390x: sclp: Dirty CC before sclp execution

 lib/s390x/asm/arch_def.h |  2 ++
 lib/s390x/asm/sigp.h     |  6 +++++-
 lib/s390x/asm/uv.h       |  6 +++++-
 lib/s390x/css.h          | 18 ++++++++++++++----
 s390x/mvpg.c             |  6 ++++--
 s390x/sclp.c             |  5 ++++-
 6 files changed, 34 insertions(+), 9 deletions(-)

-- 
2.40.1

[kvm-unit-tests PATCH 1/5] lib: s390x: Add spm cc shift constant

[Janosch Frank]

Set Program Mask expects the CC in bits 34 and 35 of a GPR.
Adding a constant makes setting the CC in inline assembly a bit easier.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 745a3387..65ae1e93 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -84,6 +84,8 @@ struct cpu {
 	bool in_interrupt_handler;
 };
 
+#define SPM_CC_SHIFT 28
+
 enum address_space {
 	AS_PRIM = 0,
 	AS_ACCR = 1,
-- 
2.40.1

[kvm-unit-tests PATCH 2/5] lib: s390x: sigp: Dirty CC before sigp execution

[Janosch Frank]

Dirtying the CC allows us to find missing CC changes when sigp is
emulated.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/sigp.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/sigp.h b/lib/s390x/asm/sigp.h
index 61d2c625..d73be1ef 100644
--- a/lib/s390x/asm/sigp.h
+++ b/lib/s390x/asm/sigp.h
@@ -49,13 +49,17 @@ static inline int sigp(uint16_t addr, uint8_t order, unsigned long parm,
 		       uint32_t *status)
 {
 	register unsigned long reg1 asm ("1") = parm;
+	uint64_t spm_cc = SIGP_CC_NOT_OPERATIONAL << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm	%[spm_cc]\n"
 		"	sigp	%1,%2,0(%3)\n"
 		"	ipm	%0\n"
 		"	srl	%0,28\n"
-		: "=d" (cc), "+d" (reg1) : "d" (addr), "a" (order) : "cc");
+		: "=d" (cc), "+d" (reg1)
+		: "d" (addr), "a" (order), [spm_cc] "d" (spm_cc)
+		: "cc");
 	if (status)
 		*status = reg1;
 	return cc;
-- 
2.40.1

[kvm-unit-tests PATCH 3/5] lib: s390x: uv: Dirty CC before uvc execution

[Janosch Frank]

Dirtying the CC allows us to find missing CC changes.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index e9fb19af..85f7b060 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -9,6 +9,8 @@
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2.
  */
+#include <asm/arch_def.h>
+
 #ifndef _ASMS390X_UV_H_
 #define _ASMS390X_UV_H_
 
@@ -216,14 +218,16 @@ struct uv_cb_ssc {
 
 static inline int uv_call_once(unsigned long r1, unsigned long r2)
 {
+	uint64_t spm_cc = 1 << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm %[spm_cc]\n"
 		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
 		"		ipm	%[cc]\n"
 		"		srl	%[cc],28\n"
 		: [cc] "=d" (cc)
-		: [r1] "a" (r1), [r2] "a" (r2)
+		: [r1] "a" (r1), [r2] "a" (r2), [spm_cc] "d" (spm_cc)
 		: "memory", "cc");
 
 	if (UVC_ERR_DEBUG && cc == 1)
-- 
2.40.1

[kvm-unit-tests PATCH 4/5] lib: s390x: css: Dirty CC before css instructions

[Janosch Frank]

Dirtying the CC allows us to find missing CC changes when css
instructions are emulated.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/css.h | 18 ++++++++++++++----
 s390x/mvpg.c    |  6 ++++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 0a19324b..47733d2d 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -6,6 +6,8 @@
  * Author: Pierre Morel <pmorel@linux.ibm.com>
  */
 
+#include <asm/arch_def.h>
+
 #ifndef _S390X_CSS_H_
 #define _S390X_CSS_H_
 
@@ -147,14 +149,16 @@ static inline int ssch(unsigned long schid, struct orb *addr)
 static inline int stsch(unsigned long schid, struct schib *addr)
 {
 	register unsigned long reg1 asm ("1") = schid;
+	uint64_t spm_cc = 1 << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm	%[spm_cc]\n"
 		"	stsch	0(%3)\n"
 		"	ipm	%0\n"
 		"	srl	%0,28"
 		: "=d" (cc), "=m" (*addr)
-		: "d" (reg1), "a" (addr)
+		: "d" (reg1), "a" (addr), [spm_cc] "d" (spm_cc)
 		: "cc");
 	return cc;
 }
@@ -177,14 +181,16 @@ static inline int msch(unsigned long schid, struct schib *addr)
 static inline int tsch(unsigned long schid, struct irb *addr)
 {
 	register unsigned long reg1 asm ("1") = schid;
+	uint64_t spm_cc = 2 << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm	%[spm_cc]\n"
 		"	tsch	0(%3)\n"
 		"	ipm	%0\n"
 		"	srl	%0,28"
 		: "=d" (cc), "=m" (*addr)
-		: "d" (reg1), "a" (addr)
+		: "d" (reg1), "a" (addr), [spm_cc] "d" (spm_cc)
 		: "cc");
 	return cc;
 }
@@ -252,28 +258,32 @@ static inline int rsch(unsigned long schid)
 static inline int rchp(unsigned long chpid)
 {
 	register unsigned long reg1 asm("1") = chpid;
+	uint64_t spm_cc = 1 << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm	%[spm_cc]\n"
 		"	rchp\n"
 		"	ipm	%0\n"
 		"	srl	%0,28"
 		: "=d" (cc)
-		: "d" (reg1)
+		: "d" (reg1), [spm_cc] "d" (spm_cc)
 		: "cc");
 	return cc;
 }
 
 static inline int stcrw(uint32_t *crw)
 {
+	uint64_t spm_cc = 1 << SPM_CC_SHIFT;
 	int cc;
 
 	asm volatile(
+		"	spm	%[spm_cc]\n"
 		"	stcrw	%[crw]\n"
 		"	ipm	%[cc]\n"
 		"	srl	%[cc],28"
 		: [cc] "=d" (cc)
-		: [crw] "Q" (*crw)
+		: [crw] "Q" (*crw), [spm_cc] "d" (spm_cc)
 		: "cc", "memory");
 	return cc;
 }
diff --git a/s390x/mvpg.c b/s390x/mvpg.c
index 296338d4..21e3ecc7 100644
--- a/s390x/mvpg.c
+++ b/s390x/mvpg.c
@@ -40,12 +40,14 @@ static uint8_t *fresh;
 static inline int mvpg(unsigned long r0, void *dest, void *src)
 {
 	register unsigned long reg0 asm ("0") = r0;
+	uint64_t spm_cc = 3 << SPM_CC_SHIFT;
 	int cc;
 
-	asm volatile("	mvpg    %1,%2\n"
+	asm volatile("	spm	%[spm_cc]\n"
+		     "	mvpg    %1,%2\n"
 		     "	ipm     %0\n"
 		     "	srl     %0,28"
-		: "=&d" (cc) : "a" (dest), "a" (src), "d" (reg0)
+		: "=&d" (cc) : "a" (dest), "a" (src), "d" (reg0), [spm_cc] "d" (spm_cc)
 		: "memory", "cc");
 	return cc;
 }
-- 
2.40.1

[kvm-unit-tests PATCH 5/5] s390x: sclp: Dirty CC before sclp execution

[Janosch Frank]

Dirtying the CC allows us to find missing CC changes when sclp is
emulated.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/sclp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/s390x/sclp.c b/s390x/sclp.c
index 1abb9029..bc6477eb 100644
--- a/s390x/sclp.c
+++ b/s390x/sclp.c
@@ -399,6 +399,7 @@ out:
 static void test_instbits(void)
 {
 	SCCBHeader *h = (SCCBHeader *)pagebuf;
+	uint64_t spm_cc = 1 << SPM_CC_SHIFT;
 	int cc;
 
 	sclp_mark_busy();
@@ -406,10 +407,12 @@ static void test_instbits(void)
 	sclp_setup_int();
 
 	asm volatile(
+		"       spm     %[spm_cc]\n"
 		"       .insn   rre,0xb2204200,%1,%2\n"  /* servc %1,%2 */
 		"       ipm     %0\n"
 		"       srl     %0,28"
-		: "=&d" (cc) : "d" (valid_code), "a" (__pa(pagebuf))
+		: "=&d" (cc)
+		: "d" (valid_code), "a" (__pa(pagebuf)), [spm_cc] "d" (spm_cc)
 		: "cc", "memory");
 	/* No exception, but also no command accepted, so no interrupt is
 	 * expected. We need to clear the flag manually otherwise we will
-- 
2.40.1

Re: [kvm-unit-tests PATCH 0/5] s390x: Dirty cc before executing tested instructions

[Nina Schoetterl-Glausch]

On Mon, 2024-01-08 at 13:29 +0000, Janosch Frank wrote:
> A recent s390 KVM fixpatch [1] showed us that checking the cc is not
> enough when emulation code forgets to set the cc. There might just be
> the correct cc in the PSW which would make the cc check succeed.
> 
> This series intentionally dirties the cc for sigp, uvc, some io
> instructions and sclp to make cc setting errors more apparent. I had a
> cursory look through the tested instructions and those are the most
> prominent ones with defined cc values.
> 
> Since the issue appeared in PQAP my AP test series is now dependent on
> this series.
> 
> [1] https://lore.kernel.org/kvm/20231201181657.1614645-1-farman@linux.ibm.com/

Using SET PROGRAM MASK the way you're doing in this series will also set the
program mask to 0, right?

In case you have some non zero register %[reg] and you want to set CC to 1 you
could do:

or	%[reg],%[reg] /* set CC to 1 */

In general, if I understand TEST UNDER MASK right, you could do:

tmll	%[set_cc],3

to set the CC to the value in %[set_cc] (without any shifting). 

Re: [kvm-unit-tests PATCH 0/5] s390x: Dirty cc before executing tested instructions

[Janosch Frank]

On 1/16/24 14:02, Nina Schoetterl-Glausch wrote:
> On Mon, 2024-01-08 at 13:29 +0000, Janosch Frank wrote:
>> A recent s390 KVM fixpatch [1] showed us that checking the cc is not
>> enough when emulation code forgets to set the cc. There might just be
>> the correct cc in the PSW which would make the cc check succeed.
>>
>> This series intentionally dirties the cc for sigp, uvc, some io
>> instructions and sclp to make cc setting errors more apparent. I had a
>> cursory look through the tested instructions and those are the most
>> prominent ones with defined cc values.
>>
>> Since the issue appeared in PQAP my AP test series is now dependent on
>> this series.
>>
>> [1] https://lore.kernel.org/kvm/20231201181657.1614645-1-farman@linux.ibm.com/
> 
> Using SET PROGRAM MASK the way you're doing in this series will also set the
> program mask to 0, right?
> 
> In case you have some non zero register %[reg] and you want to set CC to 1 you
> could do:
> 
> or	%[reg],%[reg] /* set CC to 1 */
> 
> In general, if I understand TEST UNDER MASK right, you could do:
> 
> tmll	%[set_cc],3
> 
> to set the CC to the value in %[set_cc] (without any shifting).


That is a wonderful solution to this problem.
I'll send out a new version in the next couple of days.