[PATCH v2 00/10] s390x: smp: Improve smp code part 2

[PATCH v2 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

v2:
	* Added some rev-bys and acks
	* Explicitly stop and start cpu before hot restart test

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              | 94 ++++++++++++++++++++++++++++++++++++----
 5 files changed, 105 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v2 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>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 s390x/smp.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index fa40753..8c9b98a 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

[PATCH v2 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 8c9b98a..95df8c4 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 s390x/smp.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 95df8c4..8a6cd1d 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 s390x/smp.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 8a6cd1d..a8e3dd7 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 s390x/cstart64.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 9af6bb3..ecffbe0 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

[PATCH v2 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 a8e3dd7..7462211 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.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 3f86243..6ef0335 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 ecffbe0..e084f13 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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

[PATCH v2 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 s390x/smp.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 48321f4..35ca9c7 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -52,6 +52,34 @@ 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;
+
+	/* Make sure cpu is running */
+	smp_cpu_stop(0);
+	set_flag(0);
+	smp_cpu_restart(1);
+	wait_for_flag();
+
+	/*
+	 * Wait until cpu 1 has set the flag because it executed the
+	 * restart function.
+	 */
+	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 +323,7 @@ int main(void)
 	smp_cpu_stop(1);
 
 	test_start();
+	test_restart();
 	test_stop();
 	test_stop_store_status();
 	test_store_status();
-- 
2.25.1

[PATCH v2 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. Let's make the x lowercase again.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.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 15a4d49..1b3bb0c 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: [PATCH v2 01/10] s390x: smp: Test all CRs on initial reset

[David Hildenbrand]

On 23.04.20 11:10, 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>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  s390x/smp.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index fa40753..8c9b98a 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] = {};

static const uint64_t nullp[12]; ? but see below

>  	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");

sizeof(nullp) ?

> +	report(status->crs[15] == 0, "cr15 == 0");

I'd actually prefer a simple loop


for (i = 1; i <= 13; i++) {
	report(status->crs[i] == 0, "cr%d == 0", i);
}
report(status->crs[15] == 0, "cr15 == 0");

>  	report_prefix_pop();
>  
>  	report_prefix_push("initialized");
> 

Apart from that looks good to me.


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 23.04.20 11:10, Janosch Frank 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: David Hildenbrand <david@redhat.com>

> ---
>  s390x/smp.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 95df8c4..8a6cd1d 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();
>  }
>  
> 


-- 
Thanks,

David / dhildenb

[PATCH v3] 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>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 s390x/smp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index fa40753..7144c9b 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);
 	struct psw psw;
+	int i;
 
 	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,10 @@ static void test_reset_initial(void)
 	report(!status->fpc, "fpc");
 	report(!status->cputm, "cpu timer");
 	report(!status->todpr, "todpr");
+	for (i = 1; i <= 13; i++) {
+		report(status->crs[i] == 0, "cr%d == 0", i);
+	}
+	report(status->crs[15] == 0, "cr15 == 0");
 	report_prefix_pop();
 
 	report_prefix_push("initialized");
-- 
2.25.1

Re: [PATCH v3] s390x: smp: Test all CRs on initial reset

[David Hildenbrand]

On 24.04.20 11:33, 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>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  s390x/smp.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index fa40753..7144c9b 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);
>  	struct psw psw;
> +	int i;
>  
>  	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,10 @@ static void test_reset_initial(void)
>  	report(!status->fpc, "fpc");
>  	report(!status->cputm, "cpu timer");
>  	report(!status->todpr, "todpr");
> +	for (i = 1; i <= 13; i++) {
> +		report(status->crs[i] == 0, "cr%d == 0", i);
> +	}
> +	report(status->crs[15] == 0, "cr15 == 0");
>  	report_prefix_pop();
>  
>  	report_prefix_push("initialized");
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

Queued to

https://github.com/davidhildenbrand/kvm-unit-tests.git s390x-next

thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 04/10] s390x: smp: Test local interrupts after cpu reset

[David Hildenbrand]

On 23.04.20 11:10, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  s390x/smp.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 8a6cd1d..a8e3dd7 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);
> +}

I think last time I looked at this I got it all wrong. So, we actually
don't expect that an interrupt triggers here, correct?

The SIGP_CPU_RESET should have cleared both interrupts on this cpu. So
once we enable them, none should trigger.

Why do we have "expect_ext_int()" ?

> +
>  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();
>  }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 05/10] s390x: smp: Loop if secondary cpu returns into cpu setup again

[David Hildenbrand]

On 23.04.20 11:10, Janosch Frank 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  s390x/cstart64.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 9af6bb3..ecffbe0 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
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 07/10] s390x: smp: Use full PSW to bringup new cpu

[David Hildenbrand]

On 23.04.20 11:10, Janosch Frank 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.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 3f86243..6ef0335 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;

Do we still have to set sw_int_grs[14] ?

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 23.04.20 11:10, 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
> 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);

Just curious: Would it make sense to add that inside
smp_cpu_stop_store_status() instead?

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 09/10] s390x: smp: Add restart when running test

[David Hildenbrand]

On 23.04.20 11:10, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  s390x/smp.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 48321f4..35ca9c7 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -52,6 +52,34 @@ 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;
> +
> +	/* Make sure cpu is running */
> +	smp_cpu_stop(0);
> +	set_flag(0);
> +	smp_cpu_restart(1);
> +	wait_for_flag();
> +
> +	/*
> +	 * Wait until cpu 1 has set the flag because it executed the
> +	 * restart function.
> +	 */
> +	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 +323,7 @@ int main(void)
>  	smp_cpu_stop(1);
>  
>  	test_start();
> +	test_restart();
>  	test_stop();
>  	test_stop_store_status();
>  	test_store_status();
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 10/10] s390x: Fix library constant definitions

[David Hildenbrand]

On 23.04.20 11:10, Janosch Frank wrote:
> Seems like I uppercased the whole region instead of only the ULs when
> I added those definitions. Let's make the x lowercase again.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.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 15a4d49..1b3bb0c 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 */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 07/10] s390x: smp: Use full PSW to bringup new cpu

[Janosch Frank]

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

On 4/24/20 12:09 PM, David Hildenbrand wrote:
> On 23.04.20 11:10, Janosch Frank 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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 3f86243..6ef0335 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;
> 
> Do we still have to set sw_int_grs[14] ?
> 
r14 is saved to restart_new so we don't go through setup twice.
We could instead copy cpu->lowcore->sw_int_psw.addr to restart new, but
that means more changes than the removal of one line.

Also, what about backtraces or plain old debug?
Having r14 is a good backup to have IMHO.


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

Re: [PATCH v2 07/10] s390x: smp: Use full PSW to bringup new cpu

[David Hildenbrand]

On 24.04.20 13:16, Janosch Frank wrote:
> On 4/24/20 12:09 PM, David Hildenbrand wrote:
>> On 23.04.20 11:10, Janosch Frank 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>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 3f86243..6ef0335 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;
>>
>> Do we still have to set sw_int_grs[14] ?
>>
> r14 is saved to restart_new so we don't go through setup twice.
> We could instead copy cpu->lowcore->sw_int_psw.addr to restart new, but
> that means more changes than the removal of one line.
> 
> Also, what about backtraces or plain old debug?
> Having r14 is a good backup to have IMHO.
> 

Fine with me

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 07/10] s390x: smp: Use full PSW to bringup new cpu

[Janosch Frank]

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

On 4/24/20 1:23 PM, David Hildenbrand wrote:
> On 24.04.20 13:16, Janosch Frank wrote:
>> On 4/24/20 12:09 PM, David Hildenbrand wrote:
>>> On 23.04.20 11:10, Janosch Frank 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>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 3f86243..6ef0335 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;
>>>
>>> Do we still have to set sw_int_grs[14] ?
>>>
>> r14 is saved to restart_new so we don't go through setup twice.
>> We could instead copy cpu->lowcore->sw_int_psw.addr to restart new, but
>> that means more changes than the removal of one line.
>>
>> Also, what about backtraces or plain old debug?
>> Having r14 is a good backup to have IMHO.
>>
> 
> Fine with me
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Thanks!
Also we need that if the cpu returns from its assigned function, so it
drops into the infinite loop.


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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

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

On 4/24/20 12:11 PM, David Hildenbrand wrote:
> On 23.04.20 11:10, 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
>> 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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
> 
> Just curious: Would it make sense to add that inside
> smp_cpu_stop_store_status() instead?
> 

I think so, we also wait for stop and start to finish, so why not for
this order code.


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

Re: [PATCH v2 04/10] s390x: smp: Test local interrupts after cpu reset

[Janosch Frank]

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

On 4/24/20 12:07 PM, David Hildenbrand wrote:
> On 23.04.20 11:10, 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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  s390x/smp.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/s390x/smp.c b/s390x/smp.c
>> index 8a6cd1d..a8e3dd7 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);
>> +}
> 
> I think last time I looked at this I got it all wrong. So, we actually
> don't expect that an interrupt triggers here, correct?

Correct

> 
> The SIGP_CPU_RESET should have cleared both interrupts on this cpu. So
> once we enable them, none should trigger.

Yes

> 
> Why do we have "expect_ext_int()" ?

Excellent question, that should not be there.
Fortunately removing it doesn't change the test results.

> 
>> +
>>  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();
>>  }
>>  
>>
> 
> 



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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

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

On 4/24/20 1:40 PM, Janosch Frank wrote:
> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>> On 23.04.20 11:10, 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
>>> 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>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>
>> Just curious: Would it make sense to add that inside
>> smp_cpu_stop_store_status() instead?
>>
> 
> I think so, we also wait for stop and start to finish, so why not for
> this order code.
> 

I've moved the waiting into the smp library and now the prefix check for
stop and store status fails every so often if executed repeatedly.

I've tried making the lc ptr volatile, a print of the prefix before the
report seems to fix the issue, a print after the report still shows the
issue but according to the print both values are the same.

I'm currently at a loss...


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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 29.04.20 10:57, Janosch Frank wrote:
> On 4/24/20 1:40 PM, Janosch Frank wrote:
>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>> On 23.04.20 11:10, 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
>>>> 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>
>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>
>>> Just curious: Would it make sense to add that inside
>>> smp_cpu_stop_store_status() instead?
>>>
>>
>> I think so, we also wait for stop and start to finish, so why not for
>> this order code.
>>
> 
> I've moved the waiting into the smp library and now the prefix check for
> stop and store status fails every so often if executed repeatedly.
> 
> I've tried making the lc ptr volatile, a print of the prefix before the
> report seems to fix the issue, a print after the report still shows the
> issue but according to the print both values are the same.
> 
> I'm currently at a loss...

Are you missing a barrier() somewhere?


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

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

On 4/29/20 11:06 AM, David Hildenbrand wrote:
> On 29.04.20 10:57, Janosch Frank wrote:
>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>> On 23.04.20 11:10, 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
>>>>> 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>
>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>
>>>> Just curious: Would it make sense to add that inside
>>>> smp_cpu_stop_store_status() instead?
>>>>
>>>
>>> I think so, we also wait for stop and start to finish, so why not for
>>> this order code.
>>>
>>
>> I've moved the waiting into the smp library and now the prefix check for
>> stop and store status fails every so often if executed repeatedly.
>>
>> I've tried making the lc ptr volatile, a print of the prefix before the
>> report seems to fix the issue, a print after the report still shows the
>> issue but according to the print both values are the same.
>>
>> I'm currently at a loss...
> 
> Are you missing a barrier() somewhere?
> 

Maybe, but the question is where?

There's already one before the report:
smp_cpu_stop_store_status(1);
mb();
report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");




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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 29.04.20 11:37, Janosch Frank wrote:
> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>> On 29.04.20 10:57, Janosch Frank wrote:
>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>> On 23.04.20 11:10, 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
>>>>>> 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>
>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>>
>>>>> Just curious: Would it make sense to add that inside
>>>>> smp_cpu_stop_store_status() instead?
>>>>>
>>>>
>>>> I think so, we also wait for stop and start to finish, so why not for
>>>> this order code.
>>>>
>>>
>>> I've moved the waiting into the smp library and now the prefix check for
>>> stop and store status fails every so often if executed repeatedly.
>>>
>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>> report seems to fix the issue, a print after the report still shows the
>>> issue but according to the print both values are the same.
>>>
>>> I'm currently at a loss...
>>
>> Are you missing a barrier() somewhere?
>>
> 
> Maybe, but the question is where?
> 
> There's already one before the report:
> smp_cpu_stop_store_status(1);
> mb();
> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");

The issue here is:

SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
complete even before the target CPU executed the stop and store (in QEMU).

Reading the PoP:

"One of the following conditions exists at the
addressed CPU: ... A previously issued stop-
and-store-status ... has been accepted by the
addressed CPU, and execution of the func-
tion requested by the order has not yet been
completed.

"If the currently specified order is sense ... then the order
is rejected, and condition code 2 is set."

So, in case of KVM, SENSE does not wait for completion of the previous
order. I remember that was a performance improvements, because we wanted
to avoid going to user space just to sense if another CPU is running.
(and I remember that the documentation was inconsistent)

Let me guess, under TCG it works all the time?

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

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

On 4/29/20 11:55 AM, David Hildenbrand wrote:
> On 29.04.20 11:37, Janosch Frank wrote:
>> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>>> On 29.04.20 10:57, Janosch Frank wrote:
>>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>>> On 23.04.20 11:10, 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
>>>>>>> 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>
>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>>>
>>>>>> Just curious: Would it make sense to add that inside
>>>>>> smp_cpu_stop_store_status() instead?
>>>>>>
>>>>>
>>>>> I think so, we also wait for stop and start to finish, so why not for
>>>>> this order code.
>>>>>
>>>>
>>>> I've moved the waiting into the smp library and now the prefix check for
>>>> stop and store status fails every so often if executed repeatedly.
>>>>
>>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>>> report seems to fix the issue, a print after the report still shows the
>>>> issue but according to the print both values are the same.
>>>>
>>>> I'm currently at a loss...
>>>
>>> Are you missing a barrier() somewhere?
>>>
>>
>> Maybe, but the question is where?
>>
>> There's already one before the report:
>> smp_cpu_stop_store_status(1);
>> mb();
>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
> 
> The issue here is:
> 
> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
> complete even before the target CPU executed the stop and store (in QEMU).
> 
> Reading the PoP:
> 
> "One of the following conditions exists at the
> addressed CPU: ... A previously issued stop-
> and-store-status ... has been accepted by the
> addressed CPU, and execution of the func-
> tion requested by the order has not yet been
> completed.
> 
> "If the currently specified order is sense ... then the order
> is rejected, and condition code 2 is set."
> 
> So, in case of KVM, SENSE does not wait for completion of the previous
> order. I remember that was a performance improvements, because we wanted
> to avoid going to user space just to sense if another CPU is running.
> (and I remember that the documentation was inconsistent)

So, KVM is not architectural compliant when it comes to SIGP SENSE?
I guess I need to go back to looping until the prefix is > 0

> 
> Let me guess, under TCG it works all the time?
> 

Looks like it


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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 29.04.20 13:21, Janosch Frank wrote:
> On 4/29/20 11:55 AM, David Hildenbrand wrote:
>> On 29.04.20 11:37, Janosch Frank wrote:
>>> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>>>> On 29.04.20 10:57, Janosch Frank wrote:
>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>>>> On 23.04.20 11:10, 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
>>>>>>>> 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>
>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>>>>
>>>>>>> Just curious: Would it make sense to add that inside
>>>>>>> smp_cpu_stop_store_status() instead?
>>>>>>>
>>>>>>
>>>>>> I think so, we also wait for stop and start to finish, so why not for
>>>>>> this order code.
>>>>>>
>>>>>
>>>>> I've moved the waiting into the smp library and now the prefix check for
>>>>> stop and store status fails every so often if executed repeatedly.
>>>>>
>>>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>>>> report seems to fix the issue, a print after the report still shows the
>>>>> issue but according to the print both values are the same.
>>>>>
>>>>> I'm currently at a loss...
>>>>
>>>> Are you missing a barrier() somewhere?
>>>>
>>>
>>> Maybe, but the question is where?
>>>
>>> There's already one before the report:
>>> smp_cpu_stop_store_status(1);
>>> mb();
>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>
>> The issue here is:
>>
>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
>> complete even before the target CPU executed the stop and store (in QEMU).
>>
>> Reading the PoP:
>>
>> "One of the following conditions exists at the
>> addressed CPU: ... A previously issued stop-
>> and-store-status ... has been accepted by the
>> addressed CPU, and execution of the func-
>> tion requested by the order has not yet been
>> completed.
>>
>> "If the currently specified order is sense ... then the order
>> is rejected, and condition code 2 is set."
>>
>> So, in case of KVM, SENSE does not wait for completion of the previous
>> order. I remember that was a performance improvements, because we wanted
>> to avoid going to user space just to sense if another CPU is running.
>> (and I remember that the documentation was inconsistent)
> 
> So, KVM is not architectural compliant when it comes to SIGP SENSE?
> I guess I need to go back to looping until the prefix is > 0

Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I
remember that a tricky part was checking if external calls are pending
for a CPU from user space.

We could pass that information along with the intercept to QEMU.

AFAIKs, SIGP SENSE is not used on a hot path in Linux.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[Janosch Frank]

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

On 4/29/20 1:47 PM, David Hildenbrand wrote:
> On 29.04.20 13:21, Janosch Frank wrote:
>> On 4/29/20 11:55 AM, David Hildenbrand wrote:
>>> On 29.04.20 11:37, Janosch Frank wrote:
>>>> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>>>>> On 29.04.20 10:57, Janosch Frank wrote:
>>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>>>>> On 23.04.20 11:10, 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
>>>>>>>>> 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>
>>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>>>>>
>>>>>>>> Just curious: Would it make sense to add that inside
>>>>>>>> smp_cpu_stop_store_status() instead?
>>>>>>>>
>>>>>>>
>>>>>>> I think so, we also wait for stop and start to finish, so why not for
>>>>>>> this order code.
>>>>>>>
>>>>>>
>>>>>> I've moved the waiting into the smp library and now the prefix check for
>>>>>> stop and store status fails every so often if executed repeatedly.
>>>>>>
>>>>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>>>>> report seems to fix the issue, a print after the report still shows the
>>>>>> issue but according to the print both values are the same.
>>>>>>
>>>>>> I'm currently at a loss...
>>>>>
>>>>> Are you missing a barrier() somewhere?
>>>>>
>>>>
>>>> Maybe, but the question is where?
>>>>
>>>> There's already one before the report:
>>>> smp_cpu_stop_store_status(1);
>>>> mb();
>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>>
>>> The issue here is:
>>>
>>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
>>> complete even before the target CPU executed the stop and store (in QEMU).
>>>
>>> Reading the PoP:
>>>
>>> "One of the following conditions exists at the
>>> addressed CPU: ... A previously issued stop-
>>> and-store-status ... has been accepted by the
>>> addressed CPU, and execution of the func-
>>> tion requested by the order has not yet been
>>> completed.
>>>
>>> "If the currently specified order is sense ... then the order
>>> is rejected, and condition code 2 is set."
>>>
>>> So, in case of KVM, SENSE does not wait for completion of the previous
>>> order. I remember that was a performance improvements, because we wanted
>>> to avoid going to user space just to sense if another CPU is running.
>>> (and I remember that the documentation was inconsistent)
>>
>> So, KVM is not architectural compliant when it comes to SIGP SENSE?
>> I guess I need to go back to looping until the prefix is > 0
> 
> Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I
> remember that a tricky part was checking if external calls are pending
> for a CPU from user space.
> 
> We could pass that information along with the intercept to QEMU.
> 
> AFAIKs, SIGP SENSE is not used on a hot path in Linux.
> 

For now I'd rather have a workaround in the test until I can find cycles
to find a solution in KVM/QEMU.

SIGP SENSE has been working quite well for Linux for the last few years,
so I won't start running around now frantically fixing stuff.



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

Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand]

On 29.04.20 14:09, Janosch Frank wrote:
> On 4/29/20 1:47 PM, David Hildenbrand wrote:
>> On 29.04.20 13:21, Janosch Frank wrote:
>>> On 4/29/20 11:55 AM, David Hildenbrand wrote:
>>>> On 29.04.20 11:37, Janosch Frank wrote:
>>>>> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>>>>>> On 29.04.20 10:57, Janosch Frank wrote:
>>>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>>>>>> On 23.04.20 11:10, 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
>>>>>>>>>> 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>
>>>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.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 6ef0335..2555bf4 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 ce63a89..a8b98c0 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 7462211..48321f4 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);
>>>>>>>>>
>>>>>>>>> Just curious: Would it make sense to add that inside
>>>>>>>>> smp_cpu_stop_store_status() instead?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think so, we also wait for stop and start to finish, so why not for
>>>>>>>> this order code.
>>>>>>>>
>>>>>>>
>>>>>>> I've moved the waiting into the smp library and now the prefix check for
>>>>>>> stop and store status fails every so often if executed repeatedly.
>>>>>>>
>>>>>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>>>>>> report seems to fix the issue, a print after the report still shows the
>>>>>>> issue but according to the print both values are the same.
>>>>>>>
>>>>>>> I'm currently at a loss...
>>>>>>
>>>>>> Are you missing a barrier() somewhere?
>>>>>>
>>>>>
>>>>> Maybe, but the question is where?
>>>>>
>>>>> There's already one before the report:
>>>>> smp_cpu_stop_store_status(1);
>>>>> mb();
>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>>>
>>>> The issue here is:
>>>>
>>>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
>>>> complete even before the target CPU executed the stop and store (in QEMU).
>>>>
>>>> Reading the PoP:
>>>>
>>>> "One of the following conditions exists at the
>>>> addressed CPU: ... A previously issued stop-
>>>> and-store-status ... has been accepted by the
>>>> addressed CPU, and execution of the func-
>>>> tion requested by the order has not yet been
>>>> completed.
>>>>
>>>> "If the currently specified order is sense ... then the order
>>>> is rejected, and condition code 2 is set."
>>>>
>>>> So, in case of KVM, SENSE does not wait for completion of the previous
>>>> order. I remember that was a performance improvements, because we wanted
>>>> to avoid going to user space just to sense if another CPU is running.
>>>> (and I remember that the documentation was inconsistent)
>>>
>>> So, KVM is not architectural compliant when it comes to SIGP SENSE?
>>> I guess I need to go back to looping until the prefix is > 0
>>
>> Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I
>> remember that a tricky part was checking if external calls are pending
>> for a CPU from user space.
>>
>> We could pass that information along with the intercept to QEMU.
>>
>> AFAIKs, SIGP SENSE is not used on a hot path in Linux.
>>
> 
> For now I'd rather have a workaround in the test until I can find cycles
> to find a solution in KVM/QEMU.
> 
> SIGP SENSE has been working quite well for Linux for the last few years,
> so I won't start running around now frantically fixing stuff.

Huh. I thought that's why we have the SMP tests after all ;)


-- 
Thanks,

David / dhildenb