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

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

v3:
	* Added some rev-bys and acks
	* Add a workaround for stop and store status
	* Beautified cr checking with loop

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

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          |  11 +++++
 lib/s390x/smp.h          |   1 +
 s390x/cstart64.S         |   5 +-
 s390x/smp.c              | 101 +++++++++++++++++++++++++++++++++++----
 5 files changed, 112 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v3 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>
Reviewed-by: David Hildenbrand <david@redhat.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

[PATCH v3 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 7144c9b..b07cb66 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 v3 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>
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 b07cb66..4c50183 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 v3 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 | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index 4c50183..5e2e517 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -245,6 +245,19 @@ static void test_reset_initial(void)
 	report_prefix_pop();
 }
 
+static void test_local_ints(void)
+{
+	unsigned long mask;
+
+	/* 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;
@@ -253,10 +266,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

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

[David Hildenbrand]

On 29.04.20 16:35, 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 | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 4c50183..5e2e517 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -245,6 +245,19 @@ static void test_reset_initial(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_local_ints(void)
> +{
> +	unsigned long mask;
> +
> +	/* 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;
> @@ -253,10 +266,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();
>  }
>  
> 

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

-- 
Thanks,

David / dhildenb

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

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

[David Hildenbrand]

On 29.04.20 16:35, 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
> 

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

-- 
Thanks,

David / dhildenb

[PATCH v3 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 5e2e517..c7ff0ee 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)
@@ -293,7 +287,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 v3 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>
Reviewed-by: David Hildenbrand <david@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 v3 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.

KVM currently needs a workaround for the stop and store status test,
since KVM's SIGP Sense implementation doesn't honor pending SIGPs at
it should. Hopefully we can fix that in the future.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/smp.c |  9 +++++++++
 lib/s390x/smp.h |  1 +
 s390x/smp.c     | 12 ++++++++++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 6ef0335..8628a3d 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -49,6 +49,14 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
 	return NULL;
 }
 
+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);
+}
+
 bool smp_cpu_stopped(uint16_t addr)
 {
 	uint32_t status;
@@ -100,6 +108,7 @@ int smp_cpu_stop_store_status(uint16_t addr)
 
 	spin_lock(&lock);
 	rc = smp_cpu_stop_nolock(addr, true);
+	smp_cpu_wait_for_completion(addr);
 	spin_unlock(&lock);
 	return rc;
 }
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 c7ff0ee..bad2131 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -75,7 +75,12 @@ static void test_stop_store_status(void)
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
-	mb();
+	/*
+	 * This loop is workaround for KVM not reporting cc 2 for SIGP
+	 * sense if a stop and store status is pending.
+	 */
+	while (!lc->prefix_sa)
+		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");
@@ -85,7 +90,8 @@ static void test_stop_store_status(void)
 	lc->prefix_sa = 0;
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
-	mb();
+	while (!lc->prefix_sa)
+		mb();
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
 	report_prefix_pop();
@@ -215,6 +221,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");
@@ -265,6 +272,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

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

[David Hildenbrand]

On 29.04.20 16:35, 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.
> 
> KVM currently needs a workaround for the stop and store status test,
> since KVM's SIGP Sense implementation doesn't honor pending SIGPs at
> it should. Hopefully we can fix that in the future.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/smp.c |  9 +++++++++
>  lib/s390x/smp.h |  1 +
>  s390x/smp.c     | 12 ++++++++++--
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 6ef0335..8628a3d 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -49,6 +49,14 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
>  	return NULL;
>  }
>  
> +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);
> +}
> +
>  bool smp_cpu_stopped(uint16_t addr)
>  {
>  	uint32_t status;
> @@ -100,6 +108,7 @@ int smp_cpu_stop_store_status(uint16_t addr)
>  
>  	spin_lock(&lock);
>  	rc = smp_cpu_stop_nolock(addr, true);
> +	smp_cpu_wait_for_completion(addr);
>  	spin_unlock(&lock);
>  	return rc;
>  }
> 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 c7ff0ee..bad2131 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -75,7 +75,12 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> -	mb();
> +	/*
> +	 * This loop is workaround for KVM not reporting cc 2 for SIGP
> +	 * sense if a stop and store status is pending.
> +	 */
> +	while (!lc->prefix_sa)
> +		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");
> @@ -85,7 +90,8 @@ static void test_stop_store_status(void)
>  	lc->prefix_sa = 0;
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
> -	mb();
> +	while (!lc->prefix_sa)
> +		mb();
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
>  	report_prefix_pop();
> @@ -215,6 +221,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);

^ is this really helpful? The next order already properly synchronizes, no?

>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>  
>  	report_prefix_push("clear");
> @@ -265,6 +272,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);

Isn't this racy for KVM as well?

I would have expected a loop until it is actually stopped.

>  	report(smp_cpu_stopped(1), "cpu stopped");
>  
>  	set_flag(0);
> 


-- 
Thanks,

David / dhildenb

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

[Janosch Frank]

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

On 4/29/20 5:15 PM, David Hildenbrand wrote:
> On 29.04.20 16:35, 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.
>>
>> KVM currently needs a workaround for the stop and store status test,
>> since KVM's SIGP Sense implementation doesn't honor pending SIGPs at
>> it should. Hopefully we can fix that in the future.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  lib/s390x/smp.c |  9 +++++++++
>>  lib/s390x/smp.h |  1 +
>>  s390x/smp.c     | 12 ++++++++++--
>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>> index 6ef0335..8628a3d 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -49,6 +49,14 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
>>  	return NULL;
>>  }
>>  
>> +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);
>> +}
>> +
>>  bool smp_cpu_stopped(uint16_t addr)
>>  {
>>  	uint32_t status;
>> @@ -100,6 +108,7 @@ int smp_cpu_stop_store_status(uint16_t addr)
>>  
>>  	spin_lock(&lock);
>>  	rc = smp_cpu_stop_nolock(addr, true);
>> +	smp_cpu_wait_for_completion(addr);
>>  	spin_unlock(&lock);
>>  	return rc;
>>  }
>> 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 c7ff0ee..bad2131 100644
>> --- a/s390x/smp.c
>> +++ b/s390x/smp.c
>> @@ -75,7 +75,12 @@ static void test_stop_store_status(void)
>>  	lc->prefix_sa = 0;
>>  	lc->grs_sa[15] = 0;
>>  	smp_cpu_stop_store_status(1);
>> -	mb();
>> +	/*
>> +	 * This loop is workaround for KVM not reporting cc 2 for SIGP
>> +	 * sense if a stop and store status is pending.
>> +	 */
>> +	while (!lc->prefix_sa)
>> +		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");
>> @@ -85,7 +90,8 @@ static void test_stop_store_status(void)
>>  	lc->prefix_sa = 0;
>>  	lc->grs_sa[15] = 0;
>>  	smp_cpu_stop_store_status(1);
>> -	mb();
>> +	while (!lc->prefix_sa)
>> +		mb();
>>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>  	report(lc->grs_sa[15], "stack");
>>  	report_prefix_pop();
>> @@ -215,6 +221,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);
> 
> ^ is this really helpful? The next order already properly synchronizes, no?

Well, the next order isn't issued with sigp_retry, so we could actually
get a cc 2 on the store. I need a cpu stopped loop here as well.

> 
>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>>  
>>  	report_prefix_push("clear");
>> @@ -265,6 +272,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);
> 
> Isn't this racy for KVM as well?
> 
> I would have expected a loop until it is actually stopped.

I'd add a loop with a comment, but also keep the wait for completion.

> 
>>  	report(smp_cpu_stopped(1), "cpu stopped");
>>  
>>  	set_flag(0);
>>
> 
> 



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

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

[David Hildenbrand]

On 30.04.20 09:40, Janosch Frank wrote:
> On 4/29/20 5:15 PM, David Hildenbrand wrote:
>> On 29.04.20 16:35, 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.
>>>
>>> KVM currently needs a workaround for the stop and store status test,
>>> since KVM's SIGP Sense implementation doesn't honor pending SIGPs at
>>> it should. Hopefully we can fix that in the future.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  lib/s390x/smp.c |  9 +++++++++
>>>  lib/s390x/smp.h |  1 +
>>>  s390x/smp.c     | 12 ++++++++++--
>>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>>> index 6ef0335..8628a3d 100644
>>> --- a/lib/s390x/smp.c
>>> +++ b/lib/s390x/smp.c
>>> @@ -49,6 +49,14 @@ struct cpu *smp_cpu_from_addr(uint16_t addr)
>>>  	return NULL;
>>>  }
>>>  
>>> +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);
>>> +}
>>> +
>>>  bool smp_cpu_stopped(uint16_t addr)
>>>  {
>>>  	uint32_t status;
>>> @@ -100,6 +108,7 @@ int smp_cpu_stop_store_status(uint16_t addr)
>>>  
>>>  	spin_lock(&lock);
>>>  	rc = smp_cpu_stop_nolock(addr, true);
>>> +	smp_cpu_wait_for_completion(addr);
>>>  	spin_unlock(&lock);
>>>  	return rc;
>>>  }
>>> 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 c7ff0ee..bad2131 100644
>>> --- a/s390x/smp.c
>>> +++ b/s390x/smp.c
>>> @@ -75,7 +75,12 @@ static void test_stop_store_status(void)
>>>  	lc->prefix_sa = 0;
>>>  	lc->grs_sa[15] = 0;
>>>  	smp_cpu_stop_store_status(1);
>>> -	mb();
>>> +	/*
>>> +	 * This loop is workaround for KVM not reporting cc 2 for SIGP
>>> +	 * sense if a stop and store status is pending.
>>> +	 */
>>> +	while (!lc->prefix_sa)
>>> +		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");
>>> @@ -85,7 +90,8 @@ static void test_stop_store_status(void)
>>>  	lc->prefix_sa = 0;
>>>  	lc->grs_sa[15] = 0;
>>>  	smp_cpu_stop_store_status(1);
>>> -	mb();
>>> +	while (!lc->prefix_sa)
>>> +		mb();
>>>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>>  	report(lc->grs_sa[15], "stack");
>>>  	report_prefix_pop();
>>> @@ -215,6 +221,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);
>>
>> ^ is this really helpful? The next order already properly synchronizes, no?
> 
> Well, the next order isn't issued with sigp_retry, so we could actually
> get a cc 2 on the store. I need a cpu stopped loop here as well.

... should that one then simply have a retry?

> 
>>
>>>  	sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
>>>  
>>>  	report_prefix_push("clear");
>>> @@ -265,6 +272,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);
>>
>> Isn't this racy for KVM as well?
>>
>> I would have expected a loop until it is actually stopped.
> 
> I'd add a loop with a comment, but also keep the wait for completion.

I don't see how the wait for completion is really useful here. The wait
for stop will do the very same then.


-- 
Thanks,

David / dhildenb

[PATCH v3 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 s390x/smp.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/s390x/smp.c b/s390x/smp.c
index bad2131..e6db8f0 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);
@@ -300,6 +328,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 v3 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>
Reviewed-by: David Hildenbrand <david@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 v3 00/10] s390x: smp: Improve smp code part 2

[David Hildenbrand]

On 29.04.20 16:35, Janosch Frank wrote:
> 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
> 
> v3:
> 	* Added some rev-bys and acks
> 	* Add a workaround for stop and store status
> 	* Beautified cr checking with loop
> 
> v2:
> 	* Added some rev-bys and acks
> 	* Explicitly stop and start cpu before hot restart test
> 
> 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          |  11 +++++
>  lib/s390x/smp.h          |   1 +
>  s390x/cstart64.S         |   5 +-
>  s390x/smp.c              | 101 +++++++++++++++++++++++++++++++++++----
>  5 files changed, 112 insertions(+), 14 deletions(-)
> 

I already had #1 applied. Applied everything else except #8, that might
require more thought. Gave it a test under z/VM and TCG.

Under z/VM I get:

ABORT: smp: cpu reset: Unexpected external call interrupt (code 0x1201):
on cpu 1 at 0x110f4

Which - I think - is expected with older kernels (on 5.4.13) that miss
the reset of local interrupts via the new kernel interface

https://github.com/davidhildenbrand/qemu.git s390-tcg-next

Thanks!

-- 
Thanks,

David / dhildenb