[kvm-unit-tests PATCH 00/10] s390x: smp: Improve smp code part 2

[kvm-unit-tests PATCH 00/10] s390x: smp: Improve smp code part 2

[Janosch Frank]

Let's continue cleaning up the smp test and smp related functions.

We add:
   * Test for external/emergency calls after reset
   * Test SIGP restart while running
   * SIGP stop and store status while running
   * CR testing on reset

We fix:
   * Proper check for sigp completion
   * smp_cpu_setup_state() loop and return address in r14


GIT: https://github.com/frankjaa/kvm-unit-tests/tree/smp_cleanup2


Janosch Frank (10):
  s390x: smp: Test all CRs on initial reset
  s390x: smp: Dirty fpc before initial reset test
  s390x: smp: Test stop and store status on a running and stopped cpu
  s390x: smp: Test local interrupts after cpu reset
  s390x: smp: Loop if secondary cpu returns into cpu setup again
  s390x: smp: Remove unneeded cpu loops
  s390x: smp: Use full PSW to bringup new cpu
  s390x: smp: Wait for sigp completion
  s390x: smp: Add restart when running test
  s390x: Fix library constant definitions

 lib/s390x/asm/arch_def.h |  8 ++--
 lib/s390x/smp.c          | 10 +++++
 lib/s390x/smp.h          |  1 +
 s390x/cstart64.S         |  5 ++-
 s390x/smp.c              | 84 ++++++++++++++++++++++++++++++++++++----
 5 files changed, 95 insertions(+), 13 deletions(-)

-- 
2.25.1

[kvm-unit-tests PATCH 01/10] s390x: smp: Test all CRs on initial reset

[Janosch Frank]

All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values,
so we also need to test 1-13 and 15 for 0.

And while we're at it, let's also set some values to cr 1, 7 and 13, so
we can actually be sure that they will be zeroed.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 s390x/smp.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index fa40753524f321d4..8c9b98aabd9e8222 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -182,16 +182,28 @@ static void test_emcall(void)
 	report_prefix_pop();
 }
 
+/* Used to dirty registers of cpu #1 before it is reset */
+static void test_func_initial(void)
+{
+	lctlg(1, 0x42000UL);
+	lctlg(7, 0x43000UL);
+	lctlg(13, 0x44000UL);
+	set_flag(1);
+}
+
 static void test_reset_initial(void)
 {
 	struct cpu_status *status = alloc_pages(0);
+	uint64_t nullp[12] = {};
 	struct psw psw;
 
 	psw.mask = extract_psw_mask();
-	psw.addr = (unsigned long)test_func;
+	psw.addr = (unsigned long)test_func_initial;
 
 	report_prefix_push("reset initial");
+	set_flag(0);
 	smp_cpu_start(1, psw);
+	wait_for_flag();
 
 	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
 	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
@@ -202,6 +214,8 @@ static void test_reset_initial(void)
 	report(!status->fpc, "fpc");
 	report(!status->cputm, "cpu timer");
 	report(!status->todpr, "todpr");
+	report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0");
+	report(status->crs[15] == 0, "cr15 == 0");
 	report_prefix_pop();
 
 	report_prefix_push("initialized");
-- 
2.25.1

[kvm-unit-tests PATCH 02/10] s390x: smp: Dirty fpc before initial reset test

[Janosch Frank]

Let's dirty the fpc, before we test if the initial reset sets it to 0.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 s390x/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 8c9b98aabd9e8222..95df8c49d2495391 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -185,6 +185,7 @@ static void test_emcall(void)
 /* Used to dirty registers of cpu #1 before it is reset */
 static void test_func_initial(void)
 {
+	asm volatile("sfpc %0" :: "d" (0x11));
 	lctlg(1, 0x42000UL);
 	lctlg(7, 0x43000UL);
 	lctlg(13, 0x44000UL);
-- 
2.25.1

[kvm-unit-tests PATCH 03/10] s390x: smp: Test stop and store status on a running and stopped cpu

[Janosch Frank]

Let's also test the stop portion of the "stop and store status" sigp
order.

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

diff --git a/s390x/smp.c b/s390x/smp.c
index 95df8c49d2495391..8a6cd1d8b17d76c6 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -76,12 +76,26 @@ static void test_stop_store_status(void)
 	struct lowcore *lc = (void *)0x0;
 
 	report_prefix_push("stop store status");
+	report_prefix_push("running");
+	smp_cpu_restart(1);
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
 	mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
+	report(smp_cpu_stopped(1), "cpu stopped");
+	report_prefix_pop();
+
+	report_prefix_push("stopped");
+	lc->prefix_sa = 0;
+	lc->grs_sa[15] = 0;
+	smp_cpu_stop_store_status(1);
+	mb();
+	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
+	report(lc->grs_sa[15], "stack");
+	report_prefix_pop();
+
 	report_prefix_pop();
 }
 
-- 
2.25.1

[kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[Janosch Frank]

Local interrupts (external and emergency call) should be cleared after
any cpu reset.

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

diff --git a/s390x/smp.c b/s390x/smp.c
index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -243,6 +243,20 @@ static void test_reset_initial(void)
 	report_prefix_pop();
 }
 
+static void test_local_ints(void)
+{
+	unsigned long mask;
+
+	expect_ext_int();
+	/* Open masks for ecall and emcall */
+	ctl_set_bit(0, 13);
+	ctl_set_bit(0, 14);
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_EXT;
+	load_psw_mask(mask);
+	set_flag(1);
+}
+
 static void test_reset(void)
 {
 	struct psw psw;
@@ -251,10 +265,18 @@ static void test_reset(void)
 	psw.addr = (unsigned long)test_func;
 
 	report_prefix_push("cpu reset");
+	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
+	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
 	smp_cpu_start(1, psw);
 
 	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
 	report(smp_cpu_stopped(1), "cpu stopped");
+
+	set_flag(0);
+	psw.addr = (unsigned long)test_local_ints;
+	smp_cpu_start(1, psw);
+	wait_for_flag();
+	report(true, "local interrupts cleared");
 	report_prefix_pop();
 }
 
-- 
2.25.1

[kvm-unit-tests PATCH 05/10] s390x: smp: Loop if secondary cpu returns into cpu setup again

[Janosch Frank]

Up to now a secondary cpu could have returned from the function it was
executing and ending up somewhere in cstart64.S. This was mostly
circumvented by an endless loop in the function that it executed.

Let's add a loop to the end of the cpu setup, so we don't have to rely
on added loops in the tests.

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

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 9af6bb3f28399fbc..ecffbe05c707fd7c 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -161,7 +161,9 @@ smp_cpu_setup_state:
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
 	/* We should only go once through cpu setup and not for every restart */
 	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
-	br	%r14
+	brasl	%r14, %r14
+	/* If the function returns, just loop here */
+0:	j	0
 
 pgm_int:
 	SAVE_REGS
-- 
2.25.1

[kvm-unit-tests PATCH 06/10] s390x: smp: Remove unneeded cpu loops

[Janosch Frank]

Now that we have a loop which is executed after we return from the
main function of a secondary cpu, we can remove the surplus loops.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
 s390x/smp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index a8e3dd7aac0c788c..74622113a2c4ad92 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -35,15 +35,9 @@ static void set_flag(int val)
 	mb();
 }
 
-static void cpu_loop(void)
-{
-	for (;;) {}
-}
-
 static void test_func(void)
 {
 	set_flag(1);
-	cpu_loop();
 }
 
 static void test_start(void)
@@ -292,7 +286,7 @@ int main(void)
 
 	/* Setting up the cpu to give it a stack and lowcore */
 	psw.mask = extract_psw_mask();
-	psw.addr = (unsigned long)cpu_loop;
+	psw.addr = (unsigned long)test_func;
 	smp_cpu_setup(1, psw);
 	smp_cpu_stop(1);
 
-- 
2.25.1

[kvm-unit-tests PATCH 07/10] s390x: smp: Use full PSW to bringup new cpu

[Janosch Frank]

Up to now we ignored the psw mask and only used the psw address when
bringing up a new cpu. For DAT we need to also load the mask, so let's
do that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/smp.c  | 2 ++
 s390x/cstart64.S | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 3f8624318f1aa5a1..6ef0335954fd4832 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -202,6 +202,8 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->stack = (uint64_t *)alloc_pages(2);
 
 	/* Start without DAT and any other mask bits. */
+	cpu->lowcore->sw_int_psw.mask = psw.mask;
+	cpu->lowcore->sw_int_psw.addr = psw.addr;
 	cpu->lowcore->sw_int_grs[14] = psw.addr;
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index ecffbe05c707fd7c..e084f1305aa90e45 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -161,7 +161,8 @@ smp_cpu_setup_state:
 	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
 	/* We should only go once through cpu setup and not for every restart */
 	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
-	brasl	%r14, %r14
+	larl	%r14, 0f
+	lpswe	GEN_LC_SW_INT_PSW
 	/* If the function returns, just loop here */
 0:	j	0
 
-- 
2.25.1

[kvm-unit-tests PATCH 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

Sigp orders are not necessarily finished when the processor finished
the sigp instruction. We need to poll if the order has been finished
before we continue.

For (re)start and stop we already use sigp sense running and sigp
sense loops. But we still lack completion checks for stop and store
status, as well as the cpu resets.

Let's add them.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/smp.c | 8 ++++++++
 lib/s390x/smp.h | 1 +
 s390x/smp.c     | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 6ef0335954fd4832..2555bf4f5e73d762 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw)
 	return rc;
 }
 
+void smp_cpu_wait_for_completion(uint16_t addr)
+{
+	uint32_t status;
+
+	/* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */
+	sigp_retry(1, SIGP_SENSE, 0, &status);
+}
+
 int smp_cpu_destroy(uint16_t addr)
 {
 	struct cpu *cpu;
diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index ce63a89880c045f3..a8b98c0fcf2b451c 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr);
 int smp_cpu_start(uint16_t addr, struct psw psw);
 int smp_cpu_stop(uint16_t addr);
 int smp_cpu_stop_store_status(uint16_t addr);
+void smp_cpu_wait_for_completion(uint16_t addr);
 int smp_cpu_destroy(uint16_t addr);
 int smp_cpu_setup(uint16_t addr, struct psw psw);
 void smp_teardown(void);
diff --git a/s390x/smp.c b/s390x/smp.c
index 74622113a2c4ad92..48321f4e346dc71d 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -75,6 +75,7 @@ static void test_stop_store_status(void)
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
+	smp_cpu_wait_for_completion(1);
 	mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
@@ -85,6 +86,7 @@ static void test_stop_store_status(void)
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
+	smp_cpu_wait_for_completion(1);
 	mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
@@ -215,6 +217,7 @@ static void test_reset_initial(void)
 	wait_for_flag();
 
 	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	smp_cpu_wait_for_completion(1);
 	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 
 	report_prefix_push("clear");
@@ -264,6 +267,7 @@ static void test_reset(void)
 	smp_cpu_start(1, psw);
 
 	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
+	smp_cpu_wait_for_completion(1);
 	report(smp_cpu_stopped(1), "cpu stopped");
 
 	set_flag(0);
-- 
2.25.1

[kvm-unit-tests PATCH 09/10] s390x: smp: Add restart when running test

[Janosch Frank]

Let's make sure we can restart a cpu that is already running.
Restarting it if it is stopped is implicitely tested by the the other
restart calls in the smp test.

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

diff --git a/s390x/smp.c b/s390x/smp.c
index 48321f4e346dc71d..79cdc1f6a4b0b491 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -52,6 +52,24 @@ static void test_start(void)
 	report(1, "start");
 }
 
+/*
+ * Does only test restart when the target is running.
+ * The other tests do restarts when stopped multiple times already.
+ */
+static void test_restart(void)
+{
+	struct cpu *cpu = smp_cpu_from_addr(1);
+	struct lowcore *lc = cpu->lowcore;
+
+	lc->restart_new_psw.mask = extract_psw_mask();
+	lc->restart_new_psw.addr = (unsigned long)test_func;
+
+	set_flag(0);
+	smp_cpu_restart(1);
+	wait_for_flag();
+	report(1, "restart while running");
+}
+
 static void test_stop(void)
 {
 	smp_cpu_stop(1);
@@ -295,6 +313,7 @@ int main(void)
 	smp_cpu_stop(1);
 
 	test_start();
+	test_restart();
 	test_stop();
 	test_stop_store_status();
 	test_store_status();
-- 
2.25.1

[kvm-unit-tests PATCH 10/10] s390x: Fix library constant definitions

[Janosch Frank]

Seems like I uppercased the whole region instead of only the ULs when
I added those definitions. Lets make the x lowercase again.

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

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 15a4d49ca97c9964..1b3bb0c1e7e1b626 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -19,10 +19,10 @@ struct psw {
 #define PSW_MASK_DAT			0x0400000000000000UL
 #define PSW_MASK_PSTATE			0x0001000000000000UL
 
-#define CR0_EXTM_SCLP			0X0000000000000200UL
-#define CR0_EXTM_EXTC			0X0000000000002000UL
-#define CR0_EXTM_EMGC			0X0000000000004000UL
-#define CR0_EXTM_MASK			0X0000000000006200UL
+#define CR0_EXTM_SCLP			0x0000000000000200UL
+#define CR0_EXTM_EXTC			0x0000000000002000UL
+#define CR0_EXTM_EMGC			0x0000000000004000UL
+#define CR0_EXTM_MASK			0x0000000000006200UL
 
 struct lowcore {
 	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
-- 
2.25.1

Re: [kvm-unit-tests PATCH 01/10] s390x: smp: Test all CRs on initial reset

[Christian Borntraeger]

On 24.03.20 09:12, Janosch Frank wrote:
> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values,
> so we also need to test 1-13 and 15 for 0.
> 
> And while we're at it, let's also set some values to cr 1, 7 and 13, so
> we can actually be sure that they will be zeroed.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  s390x/smp.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index fa40753524f321d4..8c9b98aabd9e8222 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -182,16 +182,28 @@ static void test_emcall(void)
>  	report_prefix_pop();
>  }
> 
> +/* Used to dirty registers of cpu #1 before it is reset */
> +static void test_func_initial(void)
> +{
> +	lctlg(1, 0x42000UL);
> +	lctlg(7, 0x43000UL);
> +	lctlg(13, 0x44000UL);
> +	set_flag(1);
> +}
> +
>  static void test_reset_initial(void)
>  {
>  	struct cpu_status *status = alloc_pages(0);
> +	uint64_t nullp[12] = {};
>  	struct psw psw;
> 
>  	psw.mask = extract_psw_mask();
> -	psw.addr = (unsigned long)test_func;
> +	psw.addr = (unsigned long)test_func_initial;
> 
>  	report_prefix_push("reset initial");
> +	set_flag(0);
>  	smp_cpu_start(1, psw);
> +	wait_for_flag();
> 
>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
> @@ -202,6 +214,8 @@ static void test_reset_initial(void)
>  	report(!status->fpc, "fpc");
>  	report(!status->cputm, "cpu timer");
>  	report(!status->todpr, "todpr");
> +	report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0");
> +	report(status->crs[15] == 0, "cr15 == 0");
>  	report_prefix_pop();

Why not add a check for crs[0] == 0xe0 
and crs[14] = 0xc2000000
?

Re: [kvm-unit-tests PATCH 01/10] s390x: smp: Test all CRs on initial reset

[Christian Borntraeger]

On 24.03.20 10:52, Christian Borntraeger wrote:
> 
> 
> On 24.03.20 09:12, Janosch Frank wrote:
>> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values,
>> so we also need to test 1-13 and 15 for 0.
>>
>> And while we're at it, let's also set some values to cr 1, 7 and 13, so
>> we can actually be sure that they will be zeroed.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  s390x/smp.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index fa40753524f321d4..8c9b98aabd9e8222 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -182,16 +182,28 @@ static void test_emcall(void)
>>  	report_prefix_pop();
>>  }
>>
>> +/* Used to dirty registers of cpu #1 before it is reset */
>> +static void test_func_initial(void)
>> +{
>> +	lctlg(1, 0x42000UL);
>> +	lctlg(7, 0x43000UL);
>> +	lctlg(13, 0x44000UL);
>> +	set_flag(1);
>> +}
>> +
>>  static void test_reset_initial(void)
>>  {
>>  	struct cpu_status *status = alloc_pages(0);
>> +	uint64_t nullp[12] = {};
>>  	struct psw psw;
>>
>>  	psw.mask = extract_psw_mask();
>> -	psw.addr = (unsigned long)test_func;
>> +	psw.addr = (unsigned long)test_func_initial;
>>
>>  	report_prefix_push("reset initial");
>> +	set_flag(0);
>>  	smp_cpu_start(1, psw);
>> +	wait_for_flag();
>>
>>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>> @@ -202,6 +214,8 @@ static void test_reset_initial(void)
>>  	report(!status->fpc, "fpc");
>>  	report(!status->cputm, "cpu timer");
>>  	report(!status->todpr, "todpr");
>> +	report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0");
>> +	report(status->crs[15] == 0, "cr15 == 0");
>>  	report_prefix_pop();
> 
> Why not add a check for crs[0] == 0xe0 
> and crs[14] = 0xc2000000

Of course this can be a future patch. This patch itself looks good. 

Re: [kvm-unit-tests PATCH 01/10] s390x: smp: Test all CRs on initial reset

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2174 bytes --]

On 3/24/20 10:52 AM, Christian Borntraeger wrote:
> 
> 
> On 24.03.20 09:12, Janosch Frank wrote:
>> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values,
>> so we also need to test 1-13 and 15 for 0.
>>
>> And while we're at it, let's also set some values to cr 1, 7 and 13, so
>> we can actually be sure that they will be zeroed.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  s390x/smp.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index fa40753524f321d4..8c9b98aabd9e8222 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -182,16 +182,28 @@ static void test_emcall(void)
>>  	report_prefix_pop();
>>  }
>>
>> +/* Used to dirty registers of cpu #1 before it is reset */
>> +static void test_func_initial(void)
>> +{
>> +	lctlg(1, 0x42000UL);
>> +	lctlg(7, 0x43000UL);
>> +	lctlg(13, 0x44000UL);
>> +	set_flag(1);
>> +}
>> +
>>  static void test_reset_initial(void)
>>  {
>>  	struct cpu_status *status = alloc_pages(0);
>> +	uint64_t nullp[12] = {};
>>  	struct psw psw;
>>
>>  	psw.mask = extract_psw_mask();
>> -	psw.addr = (unsigned long)test_func;
>> +	psw.addr = (unsigned long)test_func_initial;
>>
>>  	report_prefix_push("reset initial");
>> +	set_flag(0);
>>  	smp_cpu_start(1, psw);
>> +	wait_for_flag();
>>
>>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>> @@ -202,6 +214,8 @@ static void test_reset_initial(void)
>>  	report(!status->fpc, "fpc");
>>  	report(!status->cputm, "cpu timer");
>>  	report(!status->todpr, "todpr");
>> +	report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0");
>> +	report(status->crs[15] == 0, "cr15 == 0");
>>  	report_prefix_pop();
> 
> Why not add a check for crs[0] == 0xe0 
> and crs[14] = 0xc2000000

You mean the checks which are done a few lines below this?
This patch just actually dirties registers which should be set to 0 so
we can really be sure that they have been touched.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 01/10] s390x: smp: Test all CRs on initial reset

[Christian Borntraeger]

On 24.03.20 11:08, Janosch Frank wrote:
> On 3/24/20 10:52 AM, Christian Borntraeger wrote:
>>
>>
>> On 24.03.20 09:12, Janosch Frank wrote:
>>> All CRs are set to 0 and CRs 0 and 14 are set to pre-defined values,
>>> so we also need to test 1-13 and 15 for 0.
>>>
>>> And while we're at it, let's also set some values to cr 1, 7 and 13, so
>>> we can actually be sure that they will be zeroed.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  s390x/smp.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>> index fa40753524f321d4..8c9b98aabd9e8222 100644
>>> --- a/s390x/smp.c
>>> +++ b/s390x/smp.c
>>> @@ -182,16 +182,28 @@ static void test_emcall(void)
>>>  	report_prefix_pop();
>>>  }
>>>
>>> +/* Used to dirty registers of cpu #1 before it is reset */
>>> +static void test_func_initial(void)
>>> +{
>>> +	lctlg(1, 0x42000UL);
>>> +	lctlg(7, 0x43000UL);
>>> +	lctlg(13, 0x44000UL);
>>> +	set_flag(1);
>>> +}
>>> +
>>>  static void test_reset_initial(void)
>>>  {
>>>  	struct cpu_status *status = alloc_pages(0);
>>> +	uint64_t nullp[12] = {};
>>>  	struct psw psw;
>>>
>>>  	psw.mask = extract_psw_mask();
>>> -	psw.addr = (unsigned long)test_func;
>>> +	psw.addr = (unsigned long)test_func_initial;
>>>
>>>  	report_prefix_push("reset initial");
>>> +	set_flag(0);
>>>  	smp_cpu_start(1, psw);
>>> +	wait_for_flag();
>>>
>>>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
>>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>>> @@ -202,6 +214,8 @@ static void test_reset_initial(void)
>>>  	report(!status->fpc, "fpc");
>>>  	report(!status->cputm, "cpu timer");
>>>  	report(!status->todpr, "todpr");
>>> +	report(!memcmp(&status->crs[1], nullp, sizeof(status->crs[1]) * 12), "cr1-13 == 0");
>>> +	report(status->crs[15] == 0, "cr15 == 0");
>>>  	report_prefix_pop();
>>
>> Why not add a check for crs[0] == 0xe0 
>> and crs[14] = 0xc2000000
> 
> You mean the checks which are done a few lines below this?
> This patch just actually dirties registers which should be set to 0 so
> we can really be sure that they have been touched.

Right. So feel free to add my RB. 

Re: [kvm-unit-tests PATCH 10/10] s390x: Fix library constant definitions

[Christian Borntraeger]

On 24.03.20 09:12, Janosch Frank wrote:
> Seems like I uppercased the whole region instead of only the ULs when
> I added those definitions. Lets make the x lowercase again.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 15a4d49ca97c9964..1b3bb0c1e7e1b626 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -19,10 +19,10 @@ struct psw {
>  #define PSW_MASK_DAT			0x0400000000000000UL
>  #define PSW_MASK_PSTATE			0x0001000000000000UL
>  
> -#define CR0_EXTM_SCLP			0X0000000000000200UL
> -#define CR0_EXTM_EXTC			0X0000000000002000UL
> -#define CR0_EXTM_EMGC			0X0000000000004000UL
> -#define CR0_EXTM_MASK			0X0000000000006200UL
> +#define CR0_EXTM_SCLP			0x0000000000000200UL
> +#define CR0_EXTM_EXTC			0x0000000000002000UL
> +#define CR0_EXTM_EMGC			0x0000000000004000UL
> +#define CR0_EXTM_MASK			0x0000000000006200UL
>  
>  struct lowcore {
>  	uint8_t		pad_0x0000[0x0080 - 0x0000];	/* 0x0000 */
> 

Re: [kvm-unit-tests PATCH 03/10] s390x: smp: Test stop and store status on a running and stopped cpu

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:44 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's also test the stop portion of the "stop and store status" sigp
> order.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:45 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Local interrupts (external and emergency call) should be cleared after
> any cpu reset.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 05/10] s390x: smp: Loop if secondary cpu returns into cpu setup again

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:46 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Up to now a secondary cpu could have returned from the function it was
> executing and ending up somewhere in cstart64.S. This was mostly
> circumvented by an endless loop in the function that it executed.
> 
> Let's add a loop to the end of the cpu setup, so we don't have to rely
> on added loops in the tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/cstart64.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 10/10] s390x: Fix library constant definitions

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:51 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Seems like I uppercased the whole region instead of only the ULs when
> I added those definitions. Lets make the x lowercase again.

s/Lets/Let's/ :)

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

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 07/10] s390x: smp: Use full PSW to bringup new cpu

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:48 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Up to now we ignored the psw mask and only used the psw address when
> bringing up a new cpu. For DAT we need to also load the mask, so let's
> do that.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/smp.c  | 2 ++
>  s390x/cstart64.S | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 08/10] s390x: smp: Wait for sigp completion

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:49 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Sigp orders are not necessarily finished when the processor finished
> the sigp instruction. We need to poll if the order has been finished
> before we continue.
> 
> For (re)start and stop we already use sigp sense running and sigp
> sense loops. But we still lack completion checks for stop and store
> status, as well as the cpu resets.
> 
> Let's add them.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/smp.c | 8 ++++++++
>  lib/s390x/smp.h | 1 +
>  s390x/smp.c     | 4 ++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 6ef0335954fd4832..2555bf4f5e73d762 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw)
>  	return rc;
>  }
>  
> +void smp_cpu_wait_for_completion(uint16_t addr)

Hm, that is more wait-for-idle than wait-for-completion, I guess? But
only semantics, no need to change that.

> +{
> +	uint32_t status;
> +
> +	/* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */
> +	sigp_retry(1, SIGP_SENSE, 0, &status);
> +}
> +
>  int smp_cpu_destroy(uint16_t addr)
>  {
>  	struct cpu *cpu;
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index ce63a89880c045f3..a8b98c0fcf2b451c 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr);
>  int smp_cpu_start(uint16_t addr, struct psw psw);
>  int smp_cpu_stop(uint16_t addr);
>  int smp_cpu_stop_store_status(uint16_t addr);
> +void smp_cpu_wait_for_completion(uint16_t addr);
>  int smp_cpu_destroy(uint16_t addr);
>  int smp_cpu_setup(uint16_t addr, struct psw psw);
>  void smp_teardown(void);
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 74622113a2c4ad92..48321f4e346dc71d 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -75,6 +75,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> +	smp_cpu_wait_for_completion(1);
>  	mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
> @@ -85,6 +86,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> +	smp_cpu_wait_for_completion(1);
>  	mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
> @@ -215,6 +217,7 @@ static void test_reset_initial(void)
>  	wait_for_flag();
>  
>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
> +	smp_cpu_wait_for_completion(1);
>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>  
>  	report_prefix_push("clear");
> @@ -264,6 +267,7 @@ static void test_reset(void)
>  	smp_cpu_start(1, psw);
>  
>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
> +	smp_cpu_wait_for_completion(1);
>  	report(smp_cpu_stopped(1), "cpu stopped");
>  
>  	set_flag(0);

I'm wondering whether there's a place for a
sigp-and-wait-for-completion function. But that's probably overkill.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 09/10] s390x: smp: Add restart when running test

[Cornelia Huck]

On Tue, 24 Mar 2020 04:12:50 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's make sure we can restart a cpu that is already running.
> Restarting it if it is stopped is implicitely tested by the the other
> restart calls in the smp test.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[David Hildenbrand]

On 24.03.20 09:12, Janosch Frank wrote:
> Local interrupts (external and emergency call) should be cleared after
> any cpu reset.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -243,6 +243,20 @@ static void test_reset_initial(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_local_ints(void)
> +{
> +	unsigned long mask;
> +
> +	expect_ext_int();
> +	/* Open masks for ecall and emcall */
> +	ctl_set_bit(0, 13);
> +	ctl_set_bit(0, 14);
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_EXT;
> +	load_psw_mask(mask);
> +	set_flag(1);
> +}
> +
>  static void test_reset(void)
>  {
>  	struct psw psw;
> @@ -251,10 +265,18 @@ static void test_reset(void)
>  	psw.addr = (unsigned long)test_func;
>  
>  	report_prefix_push("cpu reset");
> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>  	smp_cpu_start(1, psw);
>  
>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
>  	report(smp_cpu_stopped(1), "cpu stopped");
> +
> +	set_flag(0);
> +	psw.addr = (unsigned long)test_local_ints;
> +	smp_cpu_start(1, psw);
> +	wait_for_flag();
> +	report(true, "local interrupts cleared");


How can you be sure they were actually cleared/delivered?


-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 24.03.20 09:12, Janosch Frank wrote:
> Sigp orders are not necessarily finished when the processor finished
> the sigp instruction. We need to poll if the order has been finished
> before we continue.
> 
> For (re)start and stop we already use sigp sense running and sigp

Nope, hopefully no longer "sense running".

> sense loops. But we still lack completion checks for stop and store
> status, as well as the cpu resets.
> 
> Let's add them.
> 


[...]

> @@ -75,6 +75,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> +	smp_cpu_wait_for_completion(1);
>  	mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
> @@ -85,6 +86,7 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> +	smp_cpu_wait_for_completion(1);
>  	mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
> @@ -215,6 +217,7 @@ static void test_reset_initial(void)
>  	wait_for_flag();
>  
>  	sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
> +	smp_cpu_wait_for_completion(1);
>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>  
>  	report_prefix_push("clear");
> @@ -264,6 +267,7 @@ static void test_reset(void)
>  	smp_cpu_start(1, psw);
>  
>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
> +	smp_cpu_wait_for_completion(1);
>  	report(smp_cpu_stopped(1), "cpu stopped");
>  
>  	set_flag(0);
> 

Looks sane to me.

-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 09/10] s390x: smp: Add restart when running test

[David Hildenbrand]

On 24.03.20 09:12, Janosch Frank wrote:
> Let's make sure we can restart a cpu that is already running.
> Restarting it if it is stopped is implicitely tested by the the other
> restart calls in the smp test.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/smp.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 48321f4e346dc71d..79cdc1f6a4b0b491 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -52,6 +52,24 @@ static void test_start(void)
>  	report(1, "start");
>  }
>  
> +/*
> + * Does only test restart when the target is running.
> + * The other tests do restarts when stopped multiple times already.
> + */
> +static void test_restart(void)
> +{
> +	struct cpu *cpu = smp_cpu_from_addr(1);
> +	struct lowcore *lc = cpu->lowcore;
> +

Maybe explicitly trigger a stop+start with a dummy here and wait until
it was processed. This make the test independent of the other (previous)
tests.

> +	lc->restart_new_psw.mask = extract_psw_mask();
> +	lc->restart_new_psw.addr = (unsigned long)test_func;
> +
> +	set_flag(0);
> +	smp_cpu_restart(1);
> +	wait_for_flag();
> +	report(1, "restart while running");
> +}
> +
>  static void test_stop(void)
>  {
>  	smp_cpu_stop(1);
> @@ -295,6 +313,7 @@ int main(void)
>  	smp_cpu_stop(1);
>  
>  	test_start();
> +	test_restart();
>  	test_stop();
>  	test_stop_store_status();
>  	test_store_status();
> 


-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1697 bytes --]

On 3/31/20 11:07 AM, David Hildenbrand wrote:
> On 24.03.20 09:12, Janosch Frank wrote:
>> Local interrupts (external and emergency call) should be cleared after
>> any cpu reset.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/smp.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -243,6 +243,20 @@ static void test_reset_initial(void)
>>  	report_prefix_pop();
>>  }
>>  
>> +static void test_local_ints(void)
>> +{
>> +	unsigned long mask;
>> +
>> +	expect_ext_int();
>> +	/* Open masks for ecall and emcall */
>> +	ctl_set_bit(0, 13);
>> +	ctl_set_bit(0, 14);
>> +	mask = extract_psw_mask();
>> +	mask |= PSW_MASK_EXT;
>> +	load_psw_mask(mask);
>> +	set_flag(1);
>> +}
>> +
>>  static void test_reset(void)
>>  {
>>  	struct psw psw;
>> @@ -251,10 +265,18 @@ static void test_reset(void)
>>  	psw.addr = (unsigned long)test_func;
>>  
>>  	report_prefix_push("cpu reset");
>> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
>> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>>  	smp_cpu_start(1, psw);
>>  
>>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
>>  	report(smp_cpu_stopped(1), "cpu stopped");
>> +
>> +	set_flag(0);
>> +	psw.addr = (unsigned long)test_local_ints;
>> +	smp_cpu_start(1, psw);
>> +	wait_for_flag();
>> +	report(true, "local interrupts cleared");
> 
> 
> How can you be sure they were actually cleared/delivered?
> 
Because cpu 1 would get a ext int it didn't expect and would do a
report_abort() as pecified in lib/s390x/interrupt.c



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[David Hildenbrand]

On 31.03.20 11:28, Janosch Frank wrote:
> On 3/31/20 11:07 AM, David Hildenbrand wrote:
>> On 24.03.20 09:12, Janosch Frank wrote:
>>> Local interrupts (external and emergency call) should be cleared after
>>> any cpu reset.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  s390x/smp.c | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>
>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
>>> --- a/s390x/smp.c
>>> +++ b/s390x/smp.c
>>> @@ -243,6 +243,20 @@ static void test_reset_initial(void)
>>>  	report_prefix_pop();
>>>  }
>>>  
>>> +static void test_local_ints(void)
>>> +{
>>> +	unsigned long mask;
>>> +
>>> +	expect_ext_int();
>>> +	/* Open masks for ecall and emcall */
>>> +	ctl_set_bit(0, 13);
>>> +	ctl_set_bit(0, 14);
>>> +	mask = extract_psw_mask();
>>> +	mask |= PSW_MASK_EXT;
>>> +	load_psw_mask(mask);
>>> +	set_flag(1);
>>> +}
>>> +
>>>  static void test_reset(void)
>>>  {
>>>  	struct psw psw;
>>> @@ -251,10 +265,18 @@ static void test_reset(void)
>>>  	psw.addr = (unsigned long)test_func;
>>>  
>>>  	report_prefix_push("cpu reset");
>>> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
>>> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>>>  	smp_cpu_start(1, psw);
>>>  
>>>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
>>>  	report(smp_cpu_stopped(1), "cpu stopped");
>>> +
>>> +	set_flag(0);
>>> +	psw.addr = (unsigned long)test_local_ints;
>>> +	smp_cpu_start(1, psw);
>>> +	wait_for_flag();
>>> +	report(true, "local interrupts cleared");
>>
>>
>> How can you be sure they were actually cleared/delivered?
>>
> Because cpu 1 would get a ext int it didn't expect and would do a
> report_abort() as pecified in lib/s390x/interrupt.c

But what if it *didn't* get the interrupts delivered.

Then cpu 1 will simply (test_local_ints()) unlock interrupts, load the
psw mask, set the flag and be done with it. What am I missing? How can
you be sure the interrupts on cpu 1 were actually delivered?


-- 
Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2584 bytes --]

On 3/31/20 7:27 PM, David Hildenbrand wrote:
> On 31.03.20 11:28, Janosch Frank wrote:
>> On 3/31/20 11:07 AM, David Hildenbrand wrote:
>>> On 24.03.20 09:12, Janosch Frank wrote:
>>>> Local interrupts (external and emergency call) should be cleared after
>>>> any cpu reset.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  s390x/smp.c | 22 ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
>>>> --- a/s390x/smp.c
>>>> +++ b/s390x/smp.c
>>>> @@ -243,6 +243,20 @@ static void test_reset_initial(void)
>>>>  	report_prefix_pop();
>>>>  }
>>>>  
>>>> +static void test_local_ints(void)
>>>> +{
>>>> +	unsigned long mask;
>>>> +
>>>> +	expect_ext_int();
>>>> +	/* Open masks for ecall and emcall */
>>>> +	ctl_set_bit(0, 13);
>>>> +	ctl_set_bit(0, 14);
>>>> +	mask = extract_psw_mask();
>>>> +	mask |= PSW_MASK_EXT;
>>>> +	load_psw_mask(mask);
>>>> +	set_flag(1);
>>>> +}
>>>> +
>>>>  static void test_reset(void)
>>>>  {
>>>>  	struct psw psw;
>>>> @@ -251,10 +265,18 @@ static void test_reset(void)
>>>>  	psw.addr = (unsigned long)test_func;
>>>>  
>>>>  	report_prefix_push("cpu reset");
>>>> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
>>>> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>>>>  	smp_cpu_start(1, psw);
>>>>  
>>>>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
>>>>  	report(smp_cpu_stopped(1), "cpu stopped");
>>>> +
>>>> +	set_flag(0);
>>>> +	psw.addr = (unsigned long)test_local_ints;
>>>> +	smp_cpu_start(1, psw);
>>>> +	wait_for_flag();
>>>> +	report(true, "local interrupts cleared");
>>>
>>>
>>> How can you be sure they were actually cleared/delivered?
>>>
>> Because cpu 1 would get a ext int it didn't expect and would do a
>> report_abort() as pecified in lib/s390x/interrupt.c
> 
> But what if it *didn't* get the interrupts delivered.

Well, the target is still looping until the reset initial test is
executed and from personal experience I can say that this test bites
rather fast.

We could execute an instruction with a mandatory exit which will prompt
KVM to inject any remaining IRQs before setting the flag to 1.

Unfortunately we do not have a polling instruction for non-IO interrupts
at the moment to verify this test.

> 
> Then cpu 1 will simply (test_local_ints()) unlock interrupts, load the
> psw mask, set the flag and be done with it. What am I missing? How can
> you be sure the interrupts on cpu 1 were actually delivered?
>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 04/10] s390x: smp: Test local interrupts after cpu reset

[David Hildenbrand]

On 01.04.20 09:19, Janosch Frank wrote:
> On 3/31/20 7:27 PM, David Hildenbrand wrote:
>> On 31.03.20 11:28, Janosch Frank wrote:
>>> On 3/31/20 11:07 AM, David Hildenbrand wrote:
>>>> On 24.03.20 09:12, Janosch Frank wrote:
>>>>> Local interrupts (external and emergency call) should be cleared after
>>>>> any cpu reset.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>  s390x/smp.c | 22 ++++++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>>>> index 8a6cd1d8b17d76c6..a8e3dd7aac0c788c 100644
>>>>> --- a/s390x/smp.c
>>>>> +++ b/s390x/smp.c
>>>>> @@ -243,6 +243,20 @@ static void test_reset_initial(void)
>>>>>  	report_prefix_pop();
>>>>>  }
>>>>>  
>>>>> +static void test_local_ints(void)
>>>>> +{
>>>>> +	unsigned long mask;
>>>>> +
>>>>> +	expect_ext_int();
>>>>> +	/* Open masks for ecall and emcall */
>>>>> +	ctl_set_bit(0, 13);
>>>>> +	ctl_set_bit(0, 14);
>>>>> +	mask = extract_psw_mask();
>>>>> +	mask |= PSW_MASK_EXT;
>>>>> +	load_psw_mask(mask);
>>>>> +	set_flag(1);
>>>>> +}
>>>>> +
>>>>>  static void test_reset(void)
>>>>>  {
>>>>>  	struct psw psw;
>>>>> @@ -251,10 +265,18 @@ static void test_reset(void)
>>>>>  	psw.addr = (unsigned long)test_func;
>>>>>  
>>>>>  	report_prefix_push("cpu reset");
>>>>> +	sigp(1, SIGP_EMERGENCY_SIGNAL, 0, NULL);
>>>>> +	sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
>>>>>  	smp_cpu_start(1, psw);
>>>>>  
>>>>>  	sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
>>>>>  	report(smp_cpu_stopped(1), "cpu stopped");
>>>>> +
>>>>> +	set_flag(0);
>>>>> +	psw.addr = (unsigned long)test_local_ints;
>>>>> +	smp_cpu_start(1, psw);
>>>>> +	wait_for_flag();
>>>>> +	report(true, "local interrupts cleared");
>>>>
>>>>
>>>> How can you be sure they were actually cleared/delivered?
>>>>
>>> Because cpu 1 would get a ext int it didn't expect and would do a
>>> report_abort() as pecified in lib/s390x/interrupt.c
>>
>> But what if it *didn't* get the interrupts delivered.
> 
> Well, the target is still looping until the reset initial test is
> executed and from personal experience I can say that this test bites
> rather fast.
> 
> We could execute an instruction with a mandatory exit which will prompt
> KVM to inject any remaining IRQs before setting the flag to 1.
> 
> Unfortunately we do not have a polling instruction for non-IO interrupts
> at the moment to verify this test.


I'd expect you should do something like this in your test instead

static void test_local_ints(void)
{
	unsigned long mask;

	/* Enable external interrupts */
	mask = extract_psw_mask();
	mask |= PSW_MASK_EXT;
	load_psw_mask(mask);

	expect_ext_int();
	/* Open masks for external call */
	ctl_set_bit(0, 13);
	if (lc->ext_int_code != $TODO1)
	       return;

	expect_ext_int();
	/* Open masks for emergency call */
	ctl_set_bit(0, 14);
	if (lc->ext_int_code != $TODO2)
	       return;

	set_flag(1);
}



-- 
Thanks,

David / dhildenb