PV tests part 1

Sometimes we need to do cleanup which we don't necessarily want to add to interrupt.c, so lets add a way to register a cleanup function.

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

Reviewed-by Thomas Huth <[email protected]>

---
 lib/s390x/asm/interrupt.h |  1 +
 lib/s390x/interrupt.c     | 10 ++++++++++
 2 files changed, 11 insertions(+)

lib/s390x/asm/interrupt.h +1 -0

index 4cfade9..b2a7c83 100644
@@ -15,6 +15,7 @@
 #define EXT_IRQ_EXTERNAL_CALL	0x1202
 #define EXT_IRQ_SERVICE_SIG	0x2401
 
+void register_pgm_int_func(void (*f)(void));
 void handle_pgm_int(void);
 void handle_ext_int(void);
 void handle_mcck_int(void);

lib/s390x/interrupt.c +0 -0

index 243b9c2..51cc733 100644

lib/s390x/interrupt.c +4 -0

+{
+	pgm_int_func = f;
+}
+
 static void fixup_pgm_int(void)
 {
 	switch (lc->pgm_int_code) {

lib/s390x/interrupt.c +1 -0

+
 	fixup_pgm_int();
 }
 

When an exception new psw with a storage key in its mask is loaded from lowcore, a specification exception is raised. This differs from the behavior when trying to execute skey related instructions, which will result in special operation exceptions.

Also let's add the test unittests.cfg so it is run more often.

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 s390x/skrf.c        | 80 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 84 insertions(+)

s390x/skrf.c +21 -0

index 9cae589..fe78711 100644
@@ -11,12 +11,16 @@
  */
 #include <libcflat.h>
 #include <asm/asm-offsets.h>
+#include <asm-generic/barrier.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/mem.h>
+#include <asm/sigp.h>
+#include <smp.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static int testflag = 0;
 
 static void test_facilities(void)
 {

@@ -106,6 +110,81 @@ static void test_tprot(void)
 	report_prefix_pop();
 }
 
+static void wait_for_flag(void)
+{
+	while (!testflag)
+		mb();
+}
+
+static void set_flag(int val)
+{
+	mb();
+	testflag = val;
+	mb();
+}
+
+static void ecall_cleanup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+

s390x/skrf.c +48 -0

+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	set_flag(1);
+}
+
+/* Set a key into the external new psw mask and open external call masks */
+static void ecall_setup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+	uint64_t mask;
+
+	register_pgm_int_func(ecall_cleanup);
+	expect_pgm_int();
+	/* Put a skey into the ext new psw */
+	lc->ext_new_psw.mask = 0x00F0000180000000UL;
+	/* Open up ext masks */
+	ctl_set_bit(0, 13);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	/* Tell cpu 0 that we're ready */
+	set_flag(1);
+}
+
+static void test_exception_ext_new(void)
+{
+	struct psw psw = {
+		.mask = extract_psw_mask(),
+		.addr = (unsigned long)ecall_setup
+	};
+
+	report_prefix_push("exception external new");
+	if (smp_query_num_cpus() < 2) {
+		report_skip("Need second cpu for exception external new test.");
+		report_prefix_pop();
+		return;
+	}
+
+	smp_cpu_setup(1, psw);
+	wait_for_flag();
+	set_flag(0);
+
+	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	wait_for_flag();
+	smp_cpu_stop(1);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skrf");

@@ -121,6 +200,7 @@ int main(void)
 	test_mvcos();
 	test_spka();
 	test_tprot();
+	test_exception_ext_new();
 
 done:
 	report_prefix_pop();

s390x/unittests.cfg +4 -0

index 0f156af..b35269b 100644
@@ -88,3 +88,7 @@ extra_params = -m 3G
 [css]
 file = css.elf
 extra_params = -device virtio-net-ccw
+
+[skrf]
+file = skrf.elf
+smp = 2

Quoted 64 lines · Janosch Frank

When an exception new psw with a storage key in its mask is loaded from lowcore, a specification exception is raised. This differs from the behavior when trying to execute skey related instructions, which will result in special operation exceptions.

Also let's add the test unittests.cfg so it is run more often.

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 s390x/skrf.c        | 80 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 84 insertions(+)

s390x/skrf.c +29 -0

index 9cae589..fe78711 100644
@@ -11,12 +11,16 @@
  */
 #include <libcflat.h>
 #include <asm/asm-offsets.h>
+#include <asm-generic/barrier.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
 #include <asm/facility.h>
 #include <asm/mem.h>
+#include <asm/sigp.h>
+#include <smp.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
+static int testflag = 0;
 
 static void test_facilities(void)
 {

@@ -106,6 +110,81 @@ static void test_tprot(void)
 	report_prefix_pop();
 }
 
+static void wait_for_flag(void)
+{
+	while (!testflag)
+		mb();
+}
+
+static void set_flag(int val)
+{
+	mb();
+	testflag = val;
+	mb();
+}
+
+static void ecall_cleanup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+
+	lc->ext_new_psw.mask = 0x0000000180000000UL;
+	lc->sw_int_crs[0] = 0x0000000000040000;
+
+	/*
+	 * PGM old contains the ext new PSW, we need to clean it up,
+	 * so we don't get a special operation exception on the lpswe
+	 * of pgm old.
+	 */

s390x/skrf.c static void test_tprot(void) · line 139

Quoted 2 lines

s390x/skrf.c +2 -0

+	lc->pgm_old_psw.mask = 0x0000000180000000UL;
+	lc->pgm_old_psw.addr = (unsigned long)wait_for_flag;

I don't quite understand why you are using wait_for_flag here? Won't that function return immediately due to the set_flag(1) below? And if it returns, where does the cpu continue to exec code in that case? Wouldn't it be better to leave the .addr unchanged, so that the CPU returns to the endless loop in smp_cpu_setup_state ?

Thomas

Quoted 71 lines

s390x/skrf.c +48 -0

+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	set_flag(1);
+}
+
+/* Set a key into the external new psw mask and open external call masks */
+static void ecall_setup(void)
+{
+	struct lowcore *lc = (void *)0x0;
+	uint64_t mask;
+
+	register_pgm_int_func(ecall_cleanup);
+	expect_pgm_int();
+	/* Put a skey into the ext new psw */
+	lc->ext_new_psw.mask = 0x00F0000180000000UL;
+	/* Open up ext masks */
+	ctl_set_bit(0, 13);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	/* Tell cpu 0 that we're ready */
+	set_flag(1);
+}
+
+static void test_exception_ext_new(void)
+{
+	struct psw psw = {
+		.mask = extract_psw_mask(),
+		.addr = (unsigned long)ecall_setup
+	};
+
+	report_prefix_push("exception external new");
+	if (smp_query_num_cpus() < 2) {
+		report_skip("Need second cpu for exception external new test.");
+		report_prefix_pop();
+		return;
+	}
+
+	smp_cpu_setup(1, psw);
+	wait_for_flag();
+	set_flag(0);
+
+	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
+	wait_for_flag();
+	smp_cpu_stop(1);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skrf");

@@ -121,6 +200,7 @@ int main(void)
 	test_mvcos();
 	test_spka();
 	test_tprot();
+	test_exception_ext_new();
 
 done:
 	report_prefix_pop();

s390x/unittests.cfg +4 -0

index 0f156af..b35269b 100644
@@ -88,3 +88,7 @@ extra_params = -m 3G
 [css]
 file = css.elf
 extra_params = -device virtio-net-ccw
+
+[skrf]
+file = skrf.elf
+smp = 2

When an exception new psw with a storage key in its mask is loaded from lowcore, a specification exception is raised. This differs from the behavior when trying to execute skey related instructions, which will result in special operation exceptions.

Also let's add the test unittests.cfg so it is run more often.

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 s390x/skrf.c        | 80 +++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  4 +++
 2 files changed, 84 insertions(+)

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

Quoted 16 lines · On Mon, 27 Jul 2020 05:54:15 -0400 Janosch Frank <[email protected]> wrote:

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

(...)

Quoted 4 lines

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

lib/s390x/asm/uv.h line 66

Quoted 10 lines

lib/s390x/asm/uv.h new file +10 -0

+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}

This returns the condition code, but no caller seems to check it (instead, they look at header.rc, which is presumably only set if the instruction executed successfully in some way?)

Looking at the kernel, it retries for cc > 1 (presumably busy conditions), and cc != 0 seems to be considered a failure. Do we want to look at the cc here as well?

Quoted 41 lines · Cornelia Huck

Quoted 16 lines · On Mon, 27 Jul 2020 05:54:15 -0400 Janosch Frank <[email protected]> wrote:

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

(...)

Quoted 4 lines

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

lib/s390x/asm/uv.h line 66

Quoted 10 lines

lib/s390x/asm/uv.h new file +10 -0

+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}

This returns the condition code, but no caller seems to check it (instead, they look at header.rc, which is presumably only set if the instruction executed successfully in some way?)

Quoted 3 lines

Looking at the kernel, it retries for cc > 1 (presumably busy conditions), and cc != 0 seems to be considered a failure. Do we want to look at the cc here as well?

It's there - but here it's in the assembly code, the "brc 3,0b".

Patch looks ok to me (but I didn't do a full review):

Acked-by Thomas Huth <[email protected]>

Quoted 42 lines · Thomas Huth

Quoted 41 lines · Cornelia Huck

Quoted 16 lines · On Mon, 27 Jul 2020 05:54:15 -0400 Janosch Frank <[email protected]> wrote:

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

(...)

Quoted 4 lines

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

lib/s390x/asm/uv.h line 66

Quoted 10 lines

lib/s390x/asm/uv.h new file +10 -0

+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}  

This returns the condition code, but no caller seems to check it (instead, they look at header.rc, which is presumably only set if the instruction executed successfully in some way?)

Quoted 5 lines

Quoted 3 lines

Looking at the kernel, it retries for cc > 1 (presumably busy conditions), and cc != 0 seems to be considered a failure. Do we want to look at the cc here as well?

It's there - but here it's in the assembly code, the "brc 3,0b".

Yes, we needed to factor that out in KVM because we sometimes need to schedule and then it looks nicer handling that in C code. The branch on condition will jump back for cc 2 and 3. cc 0 and 1 are success and error respectively and only then the rc and rrc in the UV header are set.

Quoted 6 lines

Patch looks ok to me (but I didn't do a full review):

Acked-by Thomas Huth <[email protected]>

Quoted 43 lines · On Fri, 31 Jul 2020 09:34:41 +0200 Janosch Frank <[email protected]> wrote:

Quoted 42 lines · Thomas Huth

Quoted 41 lines · Cornelia Huck

Quoted 16 lines · On Mon, 27 Jul 2020 05:54:15 -0400 Janosch Frank <[email protected]> wrote:

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

(...)

Quoted 4 lines

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

lib/s390x/asm/uv.h line 66

Quoted 10 lines

lib/s390x/asm/uv.h new file +10 -0

+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}  

This returns the condition code, but no caller seems to check it (instead, they look at header.rc, which is presumably only set if the instruction executed successfully in some way?)

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Looking at the kernel, it retries for cc > 1 (presumably busy conditions), and cc != 0 seems to be considered a failure. Do we want to look at the cc here as well?

It's there - but here it's in the assembly code, the "brc 3,0b".

Ah yes, I missed that.

Quoted 1 line

Quoted 4 lines

Yes, we needed to factor that out in KVM because we sometimes need to schedule and then it looks nicer handling that in C code. The branch on condition will jump back for cc 2 and 3. cc 0 and 1 are success and error respectively and only then the rc and rrc in the UV header are set.

Yeah, it's a bit surprising that rc/rrc are also set with cc 1.

(Can you add a comment? Just so that it is clear that callers never need to check the cc, as rc/rrc already contain more information than that.)

Quoted 9 lines

Quoted 7 lines

Patch looks ok to me (but I didn't do a full review):

Acked-by Thomas Huth <[email protected]>

Quoted 60 lines · Cornelia Huck

Quoted 43 lines · On Fri, 31 Jul 2020 09:34:41 +0200 Janosch Frank <[email protected]> wrote:

Quoted 42 lines · Thomas Huth

Quoted 41 lines · Cornelia Huck

Quoted 16 lines · On Mon, 27 Jul 2020 05:54:15 -0400 Janosch Frank <[email protected]> wrote:

Test the error conditions of guest 2 Ultravisor calls, namely:
* Query Ultravisor information
* Set shared access
* Remove shared access

Signed-off-by Janosch Frank <[email protected]>

Reviewed-by Claudio Imbrenda <[email protected]>

---
 lib/s390x/asm/uv.h  |  68 +++++++++++++++++++
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   3 +
 s390x/uv-guest.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 lib/s390x/asm/uv.h
 create mode 100644 s390x/uv-guest.c

(...)

Quoted 4 lines

lib/s390x/asm/uv.h new file +4 -0

+static inline int uv_call(unsigned long r1, unsigned long r2)
+{
+	int cc;
+

lib/s390x/asm/uv.h line 66

Quoted 10 lines

lib/s390x/asm/uv.h new file +10 -0

+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],0,0\n"
+		"		brc	3,0b\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (r2)
+		: "memory", "cc");
+	return cc;
+}    

This returns the condition code, but no caller seems to check it (instead, they look at header.rc, which is presumably only set if the instruction executed successfully in some way?)

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Looking at the kernel, it retries for cc > 1 (presumably busy conditions), and cc != 0 seems to be considered a failure. Do we want to look at the cc here as well?

It's there - but here it's in the assembly code, the "brc 3,0b".

Ah yes, I missed that.

Quoted 5 lines

Yes, we needed to factor that out in KVM because we sometimes need to schedule and then it looks nicer handling that in C code. The branch on condition will jump back for cc 2 and 3. cc 0 and 1 are success and error respectively and only then the rc and rrc in the UV header are set.

Quoted 1 line

Yeah, it's a bit surprising that rc/rrc are also set with cc 1.

Is it?
The (r)rc *only* contain meaningful information on CC 1. On CC 0 they will simply say everything is fine which CC 0 states already anyway.

Quoted 5 lines

(Can you add a comment? Just so that it is clear that callers never need to check the cc, as rc/rrc already contain more information than that.)

I'd rather fix my test code and also check the CC.
I did check it for my other UV tests so I've no idea why I didn't do it here...

How about adding a comment for the cc 2/3 case?
"The brc instruction will take care of the cc 2/3 case where we need to continue the execution because we were interrupted.
The inline assembly will only return on success/error i.e. cc 0/1."