[PATCH kvm-unit-tests v2 0/6] s390x SIGP fixes

[PATCH kvm-unit-tests v2 0/6] s390x SIGP fixes

[Eric Farman]

Hi all,

Here is a second version of the SIGNAL PROCESSOR tests, addressing
the comments that were made to v1.

The biggest change is Patch 6, which has effectively been replaced.
Considering comments from Nico and Janosch, and making sure
consumers of smp_sigp get their SIGP retried, I opted to convert
smp_sigp() to perform the CC2 retry under the covers, and remove
smp_sigp_retry() itself. This effectively reverts patch 1, but I
thought the end result was cleaner and less prone to confusion
going forward. If a test needs a no-retry interface going forward,
that would be one thing, but the only places that do that are
the testing of STOP and RESTART itself, which handle this as seen
in patches 4 and 5.

So, thoughts?

v2:
 - Patch 1-2: Applied r-b from Claudio, Janosch, and Nico (thank you!)
 - [JF] Patch 3: Clarified commit message that it's dealing with semantics
   rather than a bugfix.
 - [NB] Patch 4-5: Added return code checks of the _nowait() routines
   (Claudio: I appreciate the r-b on 4, but didn't apply it because of the
   new checks that were made here)
 - [EF] Patch 4-5: Use the non-retry sigp() call directly, rather than
   smp_sigp()
v1: https://lore.kernel.org/r/20220303210425.1693486-1-farman@linux.ibm.com/

Eric Farman (6):
  lib: s390x: smp: Retry SIGP SENSE on CC2
  s390x: smp: Test SIGP RESTART against stopped CPU
  s390x: smp: Fix checks for SIGP STOP STORE STATUS
  s390x: smp: Create and use a non-waiting CPU stop
  s390x: smp: Create and use a non-waiting CPU restart
  lib: s390x: smp: Remove smp_sigp_retry

 lib/s390x/smp.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------
 lib/s390x/smp.h |  3 ++-
 s390x/smp.c     | 53 +++++++++++++++++++++++++----------------
 3 files changed, 89 insertions(+), 30 deletions(-)

-- 
2.32.0

[PATCH kvm-unit-tests v2 1/6] lib: s390x: smp: Retry SIGP SENSE on CC2

[Eric Farman]

The routine smp_cpu_stopped() issues a SIGP SENSE, and returns true
if it received a CC1 (STATUS STORED) with the STOPPED or CHECK STOP
bits enabled. Otherwise, it returns false.

This is misleading, because a CC2 (BUSY) merely indicates that the
order code could not be processed, not that the CPU is operating.
It could be operating but in the process of being stopped.

Convert the invocation of the SIGP SENSE to retry when a CC2 is
received, so we get a more definitive answer.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 46e1b022..368d6add 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -78,7 +78,7 @@ bool smp_cpu_stopped(uint16_t idx)
 {
 	uint32_t status;
 
-	if (smp_sigp(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
+	if (smp_sigp_retry(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
 		return false;
 	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
 }
-- 
2.32.0

[PATCH kvm-unit-tests v2 2/6] s390x: smp: Test SIGP RESTART against stopped CPU

[Eric Farman]

test_restart() makes two smp_cpu_restart() calls against CPU 1.
It claims to perform both of them against running (operating) CPUs,
but the first invocation tries to achieve this by calling
smp_cpu_stop() to CPU 0. This will be rejected by the library.

Let's fix this by making the first restart operate on a stopped CPU,
to ensure it gets test coverage instead of relying on other callers.

Fixes: 166da884d ("s390x: smp: Add restart when running test")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/smp.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index 068ac74d..2f4af820 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -50,10 +50,6 @@ static void test_start(void)
 	report_pass("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_idx(1);
@@ -62,8 +58,8 @@ static void test_restart(void)
 	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);
+	/* Make sure cpu is stopped */
+	smp_cpu_stop(1);
 	set_flag(0);
 	smp_cpu_restart(1);
 	wait_for_flag();
-- 
2.32.0

[PATCH kvm-unit-tests v2 3/6] s390x: smp: Fix checks for SIGP STOP STORE STATUS

[Eric Farman]

In the routine test_stop_store_status(), the "running" part of
the test checks a few of the fields in lowcore (to verify the
"STORE STATUS" part of the SIGP order), and then ensures that
the CPU has stopped. But this is backwards, according to the
Principles of Operation:
  The addressed CPU performs the stop function, fol-
  lowed by the store-status operation (see “Store Sta-
  tus” on page 4-82).

If the CPU were not yet stopped, the contents of the lowcore
fields would be unpredictable. It works today because the
library functions wait on the stop function, so the CPU is
stopped by the time it comes back. Let's first check that the
CPU is stopped first, just to be clear.

While here, add the same check to the second part of the test,
even though the CPU is explicitly stopped prior to the SIGP.

Fixes: fc67b07a4 ("s390x: smp: Test stop and store status on a running and stopped cpu")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 s390x/smp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/s390x/smp.c b/s390x/smp.c
index 2f4af820..50811bd0 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -98,9 +98,9 @@ static void test_stop_store_status(void)
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
 	mb();
+	report(smp_cpu_stopped(1), "cpu stopped");
 	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");
@@ -108,6 +108,7 @@ static void test_stop_store_status(void)
 	lc->grs_sa[15] = 0;
 	smp_cpu_stop_store_status(1);
 	mb();
+	report(smp_cpu_stopped(1), "cpu stopped");
 	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
 	report(lc->grs_sa[15], "stack");
 	report_prefix_pop();
-- 
2.32.0

[PATCH kvm-unit-tests v2 4/6] s390x: smp: Create and use a non-waiting CPU stop

[Eric Farman]

When stopping a CPU, kvm-unit-tests serializes/waits for everything
to finish, in order to get a consistent result whenever those
functions are used.

But to test the SIGP STOP itself, these additional measures could
mask other problems. For example, did the STOP work, or is the CPU
still operating?

Let's create a non-waiting SIGP STOP and use it here, to ensure that
the CPU is correctly stopped. A smp_cpu_stopped() call will still
be used to see that the SIGP STOP has been processed, and the state
of the CPU can be used to determine whether the test passes/fails.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/smp.c | 27 +++++++++++++++++++++++++++
 lib/s390x/smp.h |  1 +
 s390x/smp.c     | 17 +++++++++--------
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 368d6add..b69c0e09 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -119,6 +119,33 @@ int smp_cpu_stop(uint16_t idx)
 	return rc;
 }
 
+/*
+ * Functionally equivalent to smp_cpu_stop(), but without the
+ * elements that wait/serialize matters itself.
+ * Used to see if KVM itself is serialized correctly.
+ */
+int smp_cpu_stop_nowait(uint16_t idx)
+{
+	check_idx(idx);
+
+	/* refuse to work on the boot CPU */
+	if (idx == 0)
+		return -1;
+
+	spin_lock(&lock);
+
+	/* Don't suppress a CC2 with sigp_retry() */
+	if (sigp(cpus[idx].addr, SIGP_STOP, 0, NULL)) {
+		spin_unlock(&lock);
+		return -1;
+	}
+
+	cpus[idx].active = false;
+	spin_unlock(&lock);
+
+	return 0;
+}
+
 int smp_cpu_stop_store_status(uint16_t idx)
 {
 	int rc;
diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index 1e69a7de..bae03dfd 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -44,6 +44,7 @@ bool smp_sense_running_status(uint16_t idx);
 int smp_cpu_restart(uint16_t idx);
 int smp_cpu_start(uint16_t idx, struct psw psw);
 int smp_cpu_stop(uint16_t idx);
+int smp_cpu_stop_nowait(uint16_t idx);
 int smp_cpu_stop_store_status(uint16_t idx);
 int smp_cpu_destroy(uint16_t idx);
 int smp_cpu_setup(uint16_t idx, struct psw psw);
diff --git a/s390x/smp.c b/s390x/smp.c
index 50811bd0..f70a9c54 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -76,14 +76,15 @@ static void test_restart(void)
 
 static void test_stop(void)
 {
-	smp_cpu_stop(1);
-	/*
-	 * The smp library waits for the CPU to shut down, but let's
-	 * also do it here, so we don't rely on the library
-	 * implementation
-	 */
-	while (!smp_cpu_stopped(1)) {}
-	report_pass("stop");
+	int rc;
+
+	report_prefix_push("stop");
+
+	rc = smp_cpu_stop_nowait(1);
+	report(!rc, "return code");
+	report(smp_cpu_stopped(1), "cpu stopped");
+
+	report_prefix_pop();
 }
 
 static void test_stop_store_status(void)
-- 
2.32.0

[PATCH kvm-unit-tests v2 5/6] s390x: smp: Create and use a non-waiting CPU restart

[Eric Farman]

The kvm-unit-tests infrastructure for a CPU restart waits for the
SIGP RESTART to complete. In order to test the restart itself,
create a variation that does not wait, and test the state of the
CPU directly.

While here, add some better report prefixes/messages, to clarify
which condition is being examined (similar to test_stop_store_status()).

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 lib/s390x/smp.c | 24 ++++++++++++++++++++++++
 lib/s390x/smp.h |  1 +
 s390x/smp.c     | 21 ++++++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index b69c0e09..5be29d36 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -194,6 +194,30 @@ int smp_cpu_restart(uint16_t idx)
 	return rc;
 }
 
+/*
+ * Functionally equivalent to smp_cpu_restart(), but without the
+ * elements that wait/serialize matters here in the test.
+ * Used to see if KVM itself is serialized correctly.
+ */
+int smp_cpu_restart_nowait(uint16_t idx)
+{
+	check_idx(idx);
+
+	spin_lock(&lock);
+
+	/* Don't suppress a CC2 with sigp_retry() */
+	if (sigp(cpus[idx].addr, SIGP_RESTART, 0, NULL)) {
+		spin_unlock(&lock);
+		return -1;
+	}
+
+	cpus[idx].active = true;
+
+	spin_unlock(&lock);
+
+	return 0;
+}
+
 int smp_cpu_start(uint16_t idx, struct psw psw)
 {
 	int rc;
diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index bae03dfd..24a0e2e0 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -42,6 +42,7 @@ uint16_t smp_cpu_addr(uint16_t idx);
 bool smp_cpu_stopped(uint16_t idx);
 bool smp_sense_running_status(uint16_t idx);
 int smp_cpu_restart(uint16_t idx);
+int smp_cpu_restart_nowait(uint16_t idx);
 int smp_cpu_start(uint16_t idx, struct psw psw);
 int smp_cpu_stop(uint16_t idx);
 int smp_cpu_stop_nowait(uint16_t idx);
diff --git a/s390x/smp.c b/s390x/smp.c
index f70a9c54..913da155 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -54,6 +54,10 @@ static void test_restart(void)
 {
 	struct cpu *cpu = smp_cpu_from_idx(1);
 	struct lowcore *lc = cpu->lowcore;
+	int rc;
+
+	report_prefix_push("restart");
+	report_prefix_push("stopped");
 
 	lc->restart_new_psw.mask = extract_psw_mask();
 	lc->restart_new_psw.addr = (unsigned long)test_func;
@@ -61,17 +65,28 @@ static void test_restart(void)
 	/* Make sure cpu is stopped */
 	smp_cpu_stop(1);
 	set_flag(0);
-	smp_cpu_restart(1);
+	rc = smp_cpu_restart_nowait(1);
+	report(!rc, "return code");
+	report(!smp_cpu_stopped(1), "cpu started");
 	wait_for_flag();
+	report_pass("test flag");
+
+	report_prefix_pop();
+	report_prefix_push("running");
 
 	/*
 	 * Wait until cpu 1 has set the flag because it executed the
 	 * restart function.
 	 */
 	set_flag(0);
-	smp_cpu_restart(1);
+	rc = smp_cpu_restart_nowait(1);
+	report(!rc, "return code");
+	report(!smp_cpu_stopped(1), "cpu started");
 	wait_for_flag();
-	report_pass("restart while running");
+	report_pass("test flag");
+
+	report_prefix_pop();
+	report_prefix_pop();
 }
 
 static void test_stop(void)
-- 
2.32.0

[PATCH kvm-unit-tests v2 6/6] lib: s390x: smp: Remove smp_sigp_retry

[Eric Farman]

The SIGP instruction presents a CC0 when an order is accepted,
though the work for the order may be performed asynchronously.
While any such work is outstanding, nearly any other SIGP order
sent to the same CPU will be returned with a CC2.

Currently, there are two library functions that perform a SIGP,
one which retries a SIGP that gets a CC2, and one which doesn't.
In practice, the users of this functionality want the CC2 to be
handled by the library itself, rather than determine whether it
needs to retry the request or not.

To avoid confusion, let's convert the smp_sigp() routine to
perform the sigp_retry() logic, and then convert any users of
smp_sigp_retry() to smp_sigp(). This of course means that the
external _retry() interface can be removed for simplicity.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 lib/s390x/smp.c | 14 ++++----------
 lib/s390x/smp.h |  1 -
 s390x/smp.c     |  4 ++--
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 5be29d36..a0495cd9 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -40,12 +40,6 @@ int smp_query_num_cpus(void)
 }
 
 int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
-{
-	check_idx(idx);
-	return sigp(cpus[idx].addr, order, parm, status);
-}
-
-int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
 {
 	check_idx(idx);
 	return sigp_retry(cpus[idx].addr, order, parm, status);
@@ -78,7 +72,7 @@ bool smp_cpu_stopped(uint16_t idx)
 {
 	uint32_t status;
 
-	if (smp_sigp_retry(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
+	if (smp_sigp(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
 		return false;
 	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
 }
@@ -99,7 +93,7 @@ static int smp_cpu_stop_nolock(uint16_t idx, bool store)
 	if (idx == 0)
 		return -1;
 
-	if (smp_sigp_retry(idx, order, 0, NULL))
+	if (smp_sigp(idx, order, 0, NULL))
 		return -1;
 
 	while (!smp_cpu_stopped(idx))
@@ -251,11 +245,11 @@ static int smp_cpu_setup_nolock(uint16_t idx, struct psw psw)
 	if (cpus[idx].active)
 		return -1;
 
-	smp_sigp_retry(idx, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	smp_sigp(idx, SIGP_INITIAL_CPU_RESET, 0, NULL);
 
 	lc = alloc_pages_flags(1, AREA_DMA31);
 	cpus[idx].lowcore = lc;
-	smp_sigp_retry(idx, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
+	smp_sigp(idx, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
 
 	/* Copy all exception psws. */
 	memcpy(lc, cpus[0].lowcore, 512);
diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
index 24a0e2e0..df184cb8 100644
--- a/lib/s390x/smp.h
+++ b/lib/s390x/smp.h
@@ -52,6 +52,5 @@ int smp_cpu_setup(uint16_t idx, struct psw psw);
 void smp_teardown(void);
 void smp_setup(void);
 int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
-int smp_sigp_retry(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status);
 
 #endif
diff --git a/s390x/smp.c b/s390x/smp.c
index 913da155..81e02195 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -266,7 +266,7 @@ static void test_reset_initial(void)
 	smp_cpu_start(1, psw);
 	wait_for_flag();
 
-	smp_sigp_retry(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
+	smp_sigp(1, SIGP_INITIAL_CPU_RESET, 0, NULL);
 	smp_sigp(1, SIGP_STORE_STATUS_AT_ADDRESS, (uintptr_t)status, NULL);
 
 	report_prefix_push("clear");
@@ -316,7 +316,7 @@ static void test_reset(void)
 	smp_sigp(1, SIGP_EXTERNAL_CALL, 0, NULL);
 	smp_cpu_start(1, psw);
 
-	smp_sigp_retry(1, SIGP_CPU_RESET, 0, NULL);
+	smp_sigp(1, SIGP_CPU_RESET, 0, NULL);
 	report(smp_cpu_stopped(1), "cpu stopped");
 
 	set_flag(0);
-- 
2.32.0

Re: [PATCH kvm-unit-tests v2 3/6] s390x: smp: Fix checks for SIGP STOP STORE STATUS

[Nico Boehr]

On Fri, 2022-03-11 at 18:38 +0100, Eric Farman wrote:
> In the routine test_stop_store_status(), the "running" part of
> the test checks a few of the fields in lowcore (to verify the
> "STORE STATUS" part of the SIGP order), and then ensures that
> the CPU has stopped. But this is backwards, according to the
> Principles of Operation:
>   The addressed CPU performs the stop function, fol-
>   lowed by the store-status operation (see “Store Sta-
>   tus” on page 4-82).
> 
> If the CPU were not yet stopped, the contents of the lowcore
> fields would be unpredictable. It works today because the
> library functions wait on the stop function, so the CPU is
> stopped by the time it comes back. Let's first check that the
> CPU is stopped first, just to be clear.
> 
> While here, add the same check to the second part of the test,
> even though the CPU is explicitly stopped prior to the SIGP.
> 
> Fixes: fc67b07a4 ("s390x: smp: Test stop and store status on a
> running and stopped cpu")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH kvm-unit-tests v2 4/6] s390x: smp: Create and use a non-waiting CPU stop

[Nico Boehr]

On Fri, 2022-03-11 at 18:38 +0100, Eric Farman wrote:
> When stopping a CPU, kvm-unit-tests serializes/waits for everything
> to finish, in order to get a consistent result whenever those
> functions are used.
> 
> But to test the SIGP STOP itself, these additional measures could
> mask other problems. For example, did the STOP work, or is the CPU
> still operating?
> 
> Let's create a non-waiting SIGP STOP and use it here, to ensure that
> the CPU is correctly stopped. A smp_cpu_stopped() call will still
> be used to see that the SIGP STOP has been processed, and the state
> of the CPU can be used to determine whether the test passes/fails.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH kvm-unit-tests v2 5/6] s390x: smp: Create and use a non-waiting CPU restart

[Nico Boehr]

On Fri, 2022-03-11 at 18:38 +0100, Eric Farman wrote:
> The kvm-unit-tests infrastructure for a CPU restart waits for the
> SIGP RESTART to complete. In order to test the restart itself,
> create a variation that does not wait, and test the state of the
> CPU directly.
> 
> While here, add some better report prefixes/messages, to clarify
> which condition is being examined (similar to
> test_stop_store_status()).
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH kvm-unit-tests v2 6/6] lib: s390x: smp: Remove smp_sigp_retry

[Nico Boehr]

On Fri, 2022-03-11 at 18:38 +0100, Eric Farman wrote:
> The SIGP instruction presents a CC0 when an order is accepted,
> though the work for the order may be performed asynchronously.
> While any such work is outstanding, nearly any other SIGP order
> sent to the same CPU will be returned with a CC2.
> 
> Currently, there are two library functions that perform a SIGP,
> one which retries a SIGP that gets a CC2, and one which doesn't.
> In practice, the users of this functionality want the CC2 to be
> handled by the library itself, rather than determine whether it
> needs to retry the request or not.
> 
> To avoid confusion, let's convert the smp_sigp() routine to
> perform the sigp_retry() logic, and then convert any users of
> smp_sigp_retry() to smp_sigp(). This of course means that the
> external _retry() interface can be removed for simplicity.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH kvm-unit-tests v2 3/6] s390x: smp: Fix checks for SIGP STOP STORE STATUS

[Claudio Imbrenda]

On Fri, 11 Mar 2022 18:38:19 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> In the routine test_stop_store_status(), the "running" part of
> the test checks a few of the fields in lowcore (to verify the
> "STORE STATUS" part of the SIGP order), and then ensures that
> the CPU has stopped. But this is backwards, according to the
> Principles of Operation:
>   The addressed CPU performs the stop function, fol-
>   lowed by the store-status operation (see “Store Sta-
>   tus” on page 4-82).
> 
> If the CPU were not yet stopped, the contents of the lowcore
> fields would be unpredictable. It works today because the
> library functions wait on the stop function, so the CPU is
> stopped by the time it comes back. Let's first check that the
> CPU is stopped first, just to be clear.
> 
> While here, add the same check to the second part of the test,
> even though the CPU is explicitly stopped prior to the SIGP.
> 
> Fixes: fc67b07a4 ("s390x: smp: Test stop and store status on a running and stopped cpu")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  s390x/smp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 2f4af820..50811bd0 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -98,9 +98,9 @@ static void test_stop_store_status(void)
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
>  	mb();
> +	report(smp_cpu_stopped(1), "cpu stopped");
>  	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");
> @@ -108,6 +108,7 @@ static void test_stop_store_status(void)
>  	lc->grs_sa[15] = 0;
>  	smp_cpu_stop_store_status(1);
>  	mb();
> +	report(smp_cpu_stopped(1), "cpu stopped");
>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>  	report(lc->grs_sa[15], "stack");
>  	report_prefix_pop();

Re: [PATCH kvm-unit-tests v2 5/6] s390x: smp: Create and use a non-waiting CPU restart

[Janosch Frank]

On 3/11/22 18:38, Eric Farman wrote:
> The kvm-unit-tests infrastructure for a CPU restart waits for the
> SIGP RESTART to complete. In order to test the restart itself,
> create a variation that does not wait, and test the state of the
> CPU directly.
> 
> While here, add some better report prefixes/messages, to clarify
> which condition is being examined (similar to test_stop_store_status()).
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   lib/s390x/smp.c | 24 ++++++++++++++++++++++++
>   lib/s390x/smp.h |  1 +
>   s390x/smp.c     | 21 ++++++++++++++++++---
>   3 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index b69c0e09..5be29d36 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -194,6 +194,30 @@ int smp_cpu_restart(uint16_t idx)
>   	return rc;
>   }
>   
> +/*
> + * Functionally equivalent to smp_cpu_restart(), but without the
> + * elements that wait/serialize matters here in the test.
> + * Used to see if KVM itself is serialized correctly.
> + */
> +int smp_cpu_restart_nowait(uint16_t idx)
> +{
> +	check_idx(idx);
> +
> +	spin_lock(&lock);
> +
> +	/* Don't suppress a CC2 with sigp_retry() */
> +	if (sigp(cpus[idx].addr, SIGP_RESTART, 0, NULL)) {
> +		spin_unlock(&lock);
> +		return -1;
> +	}
> +
> +	cpus[idx].active = true;
> +
> +	spin_unlock(&lock);
> +
> +	return 0;
> +}
> +
>   int smp_cpu_start(uint16_t idx, struct psw psw)
>   {
>   	int rc;
> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> index bae03dfd..24a0e2e0 100644
> --- a/lib/s390x/smp.h
> +++ b/lib/s390x/smp.h
> @@ -42,6 +42,7 @@ uint16_t smp_cpu_addr(uint16_t idx);
>   bool smp_cpu_stopped(uint16_t idx);
>   bool smp_sense_running_status(uint16_t idx);
>   int smp_cpu_restart(uint16_t idx);
> +int smp_cpu_restart_nowait(uint16_t idx);
>   int smp_cpu_start(uint16_t idx, struct psw psw);
>   int smp_cpu_stop(uint16_t idx);
>   int smp_cpu_stop_nowait(uint16_t idx);
> diff --git a/s390x/smp.c b/s390x/smp.c
> index f70a9c54..913da155 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -54,6 +54,10 @@ static void test_restart(void)
>   {
>   	struct cpu *cpu = smp_cpu_from_idx(1);
>   	struct lowcore *lc = cpu->lowcore;
> +	int rc;
> +
> +	report_prefix_push("restart");
> +	report_prefix_push("stopped");
>   
>   	lc->restart_new_psw.mask = extract_psw_mask();
>   	lc->restart_new_psw.addr = (unsigned long)test_func;
> @@ -61,17 +65,28 @@ static void test_restart(void)
>   	/* Make sure cpu is stopped */
>   	smp_cpu_stop(1);
>   	set_flag(0);
> -	smp_cpu_restart(1);
> +	rc = smp_cpu_restart_nowait(1);
> +	report(!rc, "return code");
> +	report(!smp_cpu_stopped(1), "cpu started");
>   	wait_for_flag();
> +	report_pass("test flag");
> +
> +	report_prefix_pop();
> +	report_prefix_push("running");
>   
>   	/*
>   	 * Wait until cpu 1 has set the flag because it executed the
>   	 * restart function.
>   	 */
>   	set_flag(0);
> -	smp_cpu_restart(1);
> +	rc = smp_cpu_restart_nowait(1);
> +	report(!rc, "return code");
> +	report(!smp_cpu_stopped(1), "cpu started");
>   	wait_for_flag();
> -	report_pass("restart while running");
> +	report_pass("test flag");
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
>   }
>   
>   static void test_stop(void)

Re: [PATCH kvm-unit-tests v2 1/6] lib: s390x: smp: Retry SIGP SENSE on CC2

[Thomas Huth]

On 11/03/2022 18.38, Eric Farman wrote:
> The routine smp_cpu_stopped() issues a SIGP SENSE, and returns true
> if it received a CC1 (STATUS STORED) with the STOPPED or CHECK STOP
> bits enabled. Otherwise, it returns false.
> 
> This is misleading, because a CC2 (BUSY) merely indicates that the
> order code could not be processed, not that the CPU is operating.
> It could be operating but in the process of being stopped.
> 
> Convert the invocation of the SIGP SENSE to retry when a CC2 is
> received, so we get a more definitive answer.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   lib/s390x/smp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 46e1b022..368d6add 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -78,7 +78,7 @@ bool smp_cpu_stopped(uint16_t idx)
>   {
>   	uint32_t status;
>   
> -	if (smp_sigp(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
> +	if (smp_sigp_retry(idx, SIGP_SENSE, 0, &status) != SIGP_CC_STATUS_STORED)
>   		return false;
>   	return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH kvm-unit-tests v2 2/6] s390x: smp: Test SIGP RESTART against stopped CPU

[Thomas Huth]

On 11/03/2022 18.38, Eric Farman wrote:
> test_restart() makes two smp_cpu_restart() calls against CPU 1.
> It claims to perform both of them against running (operating) CPUs,
> but the first invocation tries to achieve this by calling
> smp_cpu_stop() to CPU 0. This will be rejected by the library.
> 
> Let's fix this by making the first restart operate on a stopped CPU,
> to ensure it gets test coverage instead of relying on other callers.
> 
> Fixes: 166da884d ("s390x: smp: Add restart when running test")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   s390x/smp.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index 068ac74d..2f4af820 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -50,10 +50,6 @@ static void test_start(void)
>   	report_pass("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_idx(1);
> @@ -62,8 +58,8 @@ static void test_restart(void)
>   	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);
> +	/* Make sure cpu is stopped */
> +	smp_cpu_stop(1);

It might be a good idea to check the return code of smp_cpu_stop() for 
success ... but that can also be done in a later patch, so for this one:

Reviewed-by: Thomas Huth <thuth@redhat.com>


>   	set_flag(0);
>   	smp_cpu_restart(1);
>   	wait_for_flag();