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

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

[Eric Farman]

Hi all,

Some of you may remember the recent discussions to straighten out
some serialization issues with the SIGNAL PROCESSOR instruction,
and its interaction with userspace. This resulted in a few kernel
patches [1] that reliably solve the issues I was seeing.

The attached kvm-unit-tests series adapts the existing smp tests
such that it can reproduce the problems I saw when those patches
are reverted (typically by simultaneously doing a kernel compile),
and also demonstrate that those patches indeed fix the issue.

There's some cleanup in here too, based on my understanding of
the smp tests as I was walking through here. Thoughts?

[1] 812de04661c4 KVM: s390: Clarify SIGP orders versus STOP/RESTART
    67cf68b6a5cc KVM: s390: Add a routine for setting userspace CPU state
    8eeba194a32e KVM: s390: Simplify SIGP Set Arch handling

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: Convert remaining smp_sigp to _retry

 lib/s390x/smp.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++---
 lib/s390x/smp.h |  2 ++
 s390x/smp.c     | 39 +++++++++++++++++++-----------------
 3 files changed, 73 insertions(+), 21 deletions(-)

-- 
2.32.0

[PATCH kvm-unit-tests v1 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>
---
 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

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

[Nico Boehr]

On Thu, 2022-03-03 at 22:04 +0100, 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: Nico Boehr <nrb@linux.ibm.com>

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

[Claudio Imbrenda]

On Thu,  3 Mar 2022 22:04:20 +0100
Eric Farman <farman@linux.ibm.com> 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: 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)); }

[PATCH kvm-unit-tests v1 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>
---
 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

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

[Janosch Frank]

On 3/3/22 22:04, 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.

I played myself there :)

> 
> 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>


If you want to you can add a report_pass() after the first wait flag.

Reviewed-by: Janosch Frank <frankja@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();

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

[Eric Farman]

On Fri, 2022-03-04 at 11:43 +0100, Janosch Frank wrote:
> On 3/3/22 22:04, 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.
> 
> I played myself there :)

:)

> 
> > 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>
> 
> If you want to you can add a report_pass() after the first wait flag.

I did in patch 5. I can move that here, but that patch has some
additional prefix changes for those reports, so didn't want to move TOO
much of that to this fix.

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

Thank you!

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

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

[Nico Boehr]

On Thu, 2022-03-03 at 22:04 +0100, 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: Nico Boehr <nrb@linux.ibm.com>

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

[Claudio Imbrenda]

On Thu,  3 Mar 2022 22:04:21 +0100
Eric Farman <farman@linux.ibm.com> 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: 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();

[PATCH kvm-unit-tests v1 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, and leads to false
errors.

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).

By checking the results how they are today, the contents of
the lowcore fields are unreliable until the CPU is stopped.
Thus, check that the CPU is stopped first, before ensuring
that the STORE STATUS was performed correctly.

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

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

[Janosch Frank]

On 3/3/22 22:04, 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, and leads to false
> errors.
> 
> 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).
> 
> By checking the results how they are today, the contents of
> the lowcore fields are unreliable until the CPU is stopped.
> Thus, check that the CPU is stopped first, before ensuring
> that the STORE STATUS was performed correctly.

The results are undefined until the cpu is not busy via SIGP sense, no?
You cover that via doing the smp_cpu_stopped() check since that does a 
sigp sense.

Where the stop check is located doesn't really matter since the library 
waits until the cpu is stopped and it does that via smp_cpu_stopped()


So:
Are we really fixing something here?

Please improve the commit description.
For me this looks more like making checks more explicit and symmetrical 
which I'm generally ok with. We just need to specify correctly why we're 
doing that.

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

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

[Eric Farman]

On Fri, 2022-03-04 at 11:40 +0100, Janosch Frank wrote:
> On 3/3/22 22:04, 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, and leads to false
> > errors.
> > 
> > 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).
> > 
> > By checking the results how they are today, the contents of
> > the lowcore fields are unreliable until the CPU is stopped.
> > Thus, check that the CPU is stopped first, before ensuring
> > that the STORE STATUS was performed correctly.
> 
> The results are undefined until the cpu is not busy via SIGP sense,
> no?
> You cover that via doing the smp_cpu_stopped() check since that does
> a 
> sigp sense.
> 
> Where the stop check is located doesn't really matter since the
> library 
> waits until the cpu is stopped and it does that via smp_cpu_stopped()
> 
> 
> So:
> Are we really fixing something here?

Hrm, I thought so, but I got focused on the order of these checks and
overlooked the point that the library already does this looping. I do
trip up on these checks; let me revisit them.

> 
> Please improve the commit description.
> For me this looks more like making checks more explicit and
> symmetrical 
> which I'm generally ok with. We just need to specify correctly why
> we're 
> doing that.
> 
> > 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();

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

[Eric Farman]

On Fri, 2022-03-04 at 09:38 -0500, Eric Farman wrote:
> On Fri, 2022-03-04 at 11:40 +0100, Janosch Frank wrote:
> > On 3/3/22 22:04, 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, and leads to false
> > > errors.
> > > 
> > > 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).
> > > 
> > > By checking the results how they are today, the contents of
> > > the lowcore fields are unreliable until the CPU is stopped.
> > > Thus, check that the CPU is stopped first, before ensuring
> > > that the STORE STATUS was performed correctly.
> > 
> > The results are undefined until the cpu is not busy via SIGP sense,
> > no?
> > You cover that via doing the smp_cpu_stopped() check since that
> > does
> > a 
> > sigp sense.
> > 
> > Where the stop check is located doesn't really matter since the
> > library 
> > waits until the cpu is stopped and it does that via
> > smp_cpu_stopped()
> > 
> > 
> > So:
> > Are we really fixing something here?
> 
> Hrm, I thought so, but I got focused on the order of these checks and
> overlooked the point that the library already does this looping. I do
> trip up on these checks; let me revisit them.

Ah, my turn to fool myself. To test all the different combinations, I
had both old/new SIGP behavior in otherwise identical kernels and QEMU
binaries. But I appear to have mislabeled QEMU, so the failures I was
seeing was due to running the old QEMU, and not anything in kvm-unit-
tests itself. My apologies.

So, per your next paragraph, I'll keep this patch but tidy up the
commit message accordingly.

> 
> > Please improve the commit description.
> > For me this looks more like making checks more explicit and
> > symmetrical 
> > which I'm generally ok with. We just need to specify correctly why
> > we're 
> > doing that.
> > 
> > > 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();

[PATCH kvm-unit-tests v1 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>
---
 lib/s390x/smp.c | 25 +++++++++++++++++++++++++
 lib/s390x/smp.h |  1 +
 s390x/smp.c     | 10 ++--------
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 368d6add..84e536e8 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -119,6 +119,31 @@ 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)
+{
+	/* refuse to work on the boot CPU */
+	if (idx == 0)
+		return -1;
+
+	spin_lock(&lock);
+
+	/* Don't suppress a CC2 with sigp_retry() */
+	if (smp_sigp(idx, 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..11c2c673 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -76,14 +76,8 @@ 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");
+	smp_cpu_stop_nowait(1);
+	report(smp_cpu_stopped(1), "stop");
 }
 
 static void test_stop_store_status(void)
-- 
2.32.0

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

[Nico Boehr]

On Thu, 2022-03-03 at 22:04 +0100, Eric Farman wrote:
[...]

> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -76,14 +76,8 @@ 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");
> +       smp_cpu_stop_nowait(1);

Now that this can fail because of CC=2, should we check the return
value here?

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

[Eric Farman]

On Mon, 2022-03-07 at 14:31 +0100, Nico Boehr wrote:
> On Thu, 2022-03-03 at 22:04 +0100, Eric Farman wrote:
> [...]
> 
> > --- a/s390x/smp.c
> > +++ b/s390x/smp.c
> > @@ -76,14 +76,8 @@ 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");
> > +       smp_cpu_stop_nowait(1);
> 
> Now that this can fail because of CC=2, should we check the return
> value here?

Ah, yes it should. Thanks,

Eric

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

[Claudio Imbrenda]

On Thu,  3 Mar 2022 22:04:23 +0100
Eric Farman <farman@linux.ibm.com> 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>
> ---
>  lib/s390x/smp.c | 25 +++++++++++++++++++++++++
>  lib/s390x/smp.h |  1 +
>  s390x/smp.c     | 10 ++--------
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 368d6add..84e536e8 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -119,6 +119,31 @@ 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)
> +{
> +	/* refuse to work on the boot CPU */
> +	if (idx == 0)
> +		return -1;
> +
> +	spin_lock(&lock);
> +
> +	/* Don't suppress a CC2 with sigp_retry() */
> +	if (smp_sigp(idx, 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..11c2c673 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -76,14 +76,8 @@ 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");
> +	smp_cpu_stop_nowait(1);

can it happen that the SIGP STOP order is accepted, but the target CPU
is still running (and not even busy)?

> +	report(smp_cpu_stopped(1), "stop");

e.g. can this ^ check race with the actual stopping of the CPU?

>  }
>  
>  static void test_stop_store_status(void)

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

[Eric Farman]

On Mon, 2022-03-07 at 16:30 +0100, Claudio Imbrenda wrote:
> On Thu,  3 Mar 2022 22:04:23 +0100
> Eric Farman <farman@linux.ibm.com> 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>
> > ---
> >  lib/s390x/smp.c | 25 +++++++++++++++++++++++++
> >  lib/s390x/smp.h |  1 +
> >  s390x/smp.c     | 10 ++--------
> >  3 files changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > index 368d6add..84e536e8 100644
> > --- a/lib/s390x/smp.c
> > +++ b/lib/s390x/smp.c
> > @@ -119,6 +119,31 @@ 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)
> > +{
> > +	/* refuse to work on the boot CPU */
> > +	if (idx == 0)
> > +		return -1;
> > +
> > +	spin_lock(&lock);
> > +
> > +	/* Don't suppress a CC2 with sigp_retry() */
> > +	if (smp_sigp(idx, 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..11c2c673 100644
> > --- a/s390x/smp.c
> > +++ b/s390x/smp.c
> > @@ -76,14 +76,8 @@ 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");
> > +	smp_cpu_stop_nowait(1);
> 
> can it happen that the SIGP STOP order is accepted, but the target
> CPU
> is still running (and not even busy)?

Of course. A SIGP that's processed by userspace (which is many of them)
injects a STOP IRQ back to the kernel, which means the CPU might not be
stopped for some time. But...

> 
> > +	report(smp_cpu_stopped(1), "stop");
> 
> e.g. can this ^ check race with the actual stopping of the CPU?

...the smp_cpu_stopped() routine now loops on the CC2 that SIGP SENSE
returns because of that pending IRQ. If SIGP SENSE returns CC0/1, then
the CPU can correctly be identified stopped/operating, and the test can
correctly pass/fail.

> 
> >  }
> >  
> >  static void test_stop_store_status(void)

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

[Claudio Imbrenda]

On Mon, 07 Mar 2022 14:03:45 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On Mon, 2022-03-07 at 16:30 +0100, Claudio Imbrenda wrote:
> > On Thu,  3 Mar 2022 22:04:23 +0100
> > Eric Farman <farman@linux.ibm.com> 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>
> > > ---
> > >  lib/s390x/smp.c | 25 +++++++++++++++++++++++++
> > >  lib/s390x/smp.h |  1 +
> > >  s390x/smp.c     | 10 ++--------
> > >  3 files changed, 28 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > > index 368d6add..84e536e8 100644
> > > --- a/lib/s390x/smp.c
> > > +++ b/lib/s390x/smp.c
> > > @@ -119,6 +119,31 @@ 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)
> > > +{
> > > +	/* refuse to work on the boot CPU */
> > > +	if (idx == 0)
> > > +		return -1;
> > > +
> > > +	spin_lock(&lock);
> > > +
> > > +	/* Don't suppress a CC2 with sigp_retry() */
> > > +	if (smp_sigp(idx, 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..11c2c673 100644
> > > --- a/s390x/smp.c
> > > +++ b/s390x/smp.c
> > > @@ -76,14 +76,8 @@ 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");
> > > +	smp_cpu_stop_nowait(1);  
> > 
> > can it happen that the SIGP STOP order is accepted, but the target
> > CPU
> > is still running (and not even busy)?  
> 
> Of course. A SIGP that's processed by userspace (which is many of them)
> injects a STOP IRQ back to the kernel, which means the CPU might not be
> stopped for some time. But...
> 
> >   
> > > +	report(smp_cpu_stopped(1), "stop");  
> > 
> > e.g. can this ^ check race with the actual stopping of the CPU?  
> 
> ...the smp_cpu_stopped() routine now loops on the CC2 that SIGP SENSE
> returns because of that pending IRQ. If SIGP SENSE returns CC0/1, then
> the CPU can correctly be identified stopped/operating, and the test can
> correctly pass/fail.

my question was: is it possible architecturally that there is a window
where the STOP order is accepted, but a SENSE on the target CPU still
successfully returns that the CPU is running?

in other words: is it specified architecturally that, once an order is
accepted for a target CPU, that CPU can't accept any other order (and
will return CC2), including SENSE, until the order has been completed
successfully?

> 
> >   
> > >  }
> > >  
> > >  static void test_stop_store_status(void)  
> 

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

[Eric Farman]

On Tue, 2022-03-08 at 11:31 +0100, Claudio Imbrenda wrote:
> On Mon, 07 Mar 2022 14:03:45 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > On Mon, 2022-03-07 at 16:30 +0100, Claudio Imbrenda wrote:
> > > On Thu,  3 Mar 2022 22:04:23 +0100
> > > Eric Farman <farman@linux.ibm.com> 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>
> > > > ---
> > > >  lib/s390x/smp.c | 25 +++++++++++++++++++++++++
> > > >  lib/s390x/smp.h |  1 +
> > > >  s390x/smp.c     | 10 ++--------
> > > >  3 files changed, 28 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > > > index 368d6add..84e536e8 100644
> > > > --- a/lib/s390x/smp.c
> > > > +++ b/lib/s390x/smp.c
> > > > @@ -119,6 +119,31 @@ 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)
> > > > +{
> > > > +	/* refuse to work on the boot CPU */
> > > > +	if (idx == 0)
> > > > +		return -1;
> > > > +
> > > > +	spin_lock(&lock);
> > > > +
> > > > +	/* Don't suppress a CC2 with sigp_retry() */
> > > > +	if (smp_sigp(idx, 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..11c2c673 100644
> > > > --- a/s390x/smp.c
> > > > +++ b/s390x/smp.c
> > > > @@ -76,14 +76,8 @@ 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");
> > > > +	smp_cpu_stop_nowait(1);  
> > > 
> > > can it happen that the SIGP STOP order is accepted, but the
> > > target
> > > CPU
> > > is still running (and not even busy)?  
> > 
> > Of course. A SIGP that's processed by userspace (which is many of
> > them)
> > injects a STOP IRQ back to the kernel, which means the CPU might
> > not be
> > stopped for some time. But...
> > 
> > >   
> > > > +	report(smp_cpu_stopped(1), "stop");  
> > > 
> > > e.g. can this ^ check race with the actual stopping of the CPU?  
> > 
> > ...the smp_cpu_stopped() routine now loops on the CC2 that SIGP
> > SENSE
> > returns because of that pending IRQ. If SIGP SENSE returns CC0/1,
> > then
> > the CPU can correctly be identified stopped/operating, and the test
> > can
> > correctly pass/fail.
> 
> my question was: is it possible architecturally that there is a
> window
> where the STOP order is accepted, but a SENSE on the target CPU still
> successfully returns that the CPU is running?

Not to my knowledge. The "Conditions Determining Response" section of
POPS (v12, page 4-95; also below) specifically states that the SIGP
SENSE will return CC2 when a SIGP STOP order is outstanding.

In our implementation, the stop IRQ will be injected before the STOP
order gets accepted, such that a SIGP SENSE would see it immediately.

"""
A previously issued start, stop, restart, stop-
and-store-status, set-prefix, store-status-at-
address order, or store-additional-status-at-
address 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, external
call, emergency signal, start, stop, restart, stop
and store status, set prefix, store status at
address, set architecture, set multithreading, or
store additional status at address, then the order
is rejected, and condition code 2 is set.
"""

> 
> in other words: is it specified architecturally that, once an order
> is
> accepted for a target CPU, that CPU can't accept any other order (and
> will return CC2), including SENSE, until the order has been completed
> successfully?

The POPS quote I placed above excludes store-extended-status-address,
conditional-emergency-signal, and sense-running-status orders as
returning a CC2. That's something Janosch and I have chatted about
offline, but don't have an answer to at this time. Besides that, any
other order would get a CC2 until the STOP has been completed.

> 
> > >   
> > > >  }
> > > >  
> > > >  static void test_stop_store_status(void)  

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

[Claudio Imbrenda]

On Tue, 08 Mar 2022 16:18:16 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On Tue, 2022-03-08 at 11:31 +0100, Claudio Imbrenda wrote:
> > On Mon, 07 Mar 2022 14:03:45 -0500
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> > > On Mon, 2022-03-07 at 16:30 +0100, Claudio Imbrenda wrote:  
> > > > On Thu,  3 Mar 2022 22:04:23 +0100
> > > > Eric Farman <farman@linux.ibm.com> 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>
> > > > > ---
> > > > >  lib/s390x/smp.c | 25 +++++++++++++++++++++++++
> > > > >  lib/s390x/smp.h |  1 +
> > > > >  s390x/smp.c     | 10 ++--------
> > > > >  3 files changed, 28 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > > > > index 368d6add..84e536e8 100644
> > > > > --- a/lib/s390x/smp.c
> > > > > +++ b/lib/s390x/smp.c
> > > > > @@ -119,6 +119,31 @@ 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)
> > > > > +{
> > > > > +	/* refuse to work on the boot CPU */
> > > > > +	if (idx == 0)
> > > > > +		return -1;
> > > > > +
> > > > > +	spin_lock(&lock);
> > > > > +
> > > > > +	/* Don't suppress a CC2 with sigp_retry() */
> > > > > +	if (smp_sigp(idx, 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..11c2c673 100644
> > > > > --- a/s390x/smp.c
> > > > > +++ b/s390x/smp.c
> > > > > @@ -76,14 +76,8 @@ 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");
> > > > > +	smp_cpu_stop_nowait(1);    
> > > > 
> > > > can it happen that the SIGP STOP order is accepted, but the
> > > > target
> > > > CPU
> > > > is still running (and not even busy)?    
> > > 
> > > Of course. A SIGP that's processed by userspace (which is many of
> > > them)
> > > injects a STOP IRQ back to the kernel, which means the CPU might
> > > not be
> > > stopped for some time. But...
> > >   
> > > >     
> > > > > +	report(smp_cpu_stopped(1), "stop");    
> > > > 
> > > > e.g. can this ^ check race with the actual stopping of the CPU?    
> > > 
> > > ...the smp_cpu_stopped() routine now loops on the CC2 that SIGP
> > > SENSE
> > > returns because of that pending IRQ. If SIGP SENSE returns CC0/1,
> > > then
> > > the CPU can correctly be identified stopped/operating, and the test
> > > can
> > > correctly pass/fail.  
> > 
> > my question was: is it possible architecturally that there is a
> > window
> > where the STOP order is accepted, but a SENSE on the target CPU still
> > successfully returns that the CPU is running?  
> 
> Not to my knowledge. The "Conditions Determining Response" section of
> POPS (v12, page 4-95; also below) specifically states that the SIGP
> SENSE will return CC2 when a SIGP STOP order is outstanding.
> 
> In our implementation, the stop IRQ will be injected before the STOP
> order gets accepted, such that a SIGP SENSE would see it immediately.
> 
> """
> A previously issued start, stop, restart, stop-
> and-store-status, set-prefix, store-status-at-
> address order, or store-additional-status-at-
> address 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, external
> call, emergency signal, start, stop, restart, stop
> and store status, set prefix, store status at
> address, set architecture, set multithreading, or
> store additional status at address, then the order
> is rejected, and condition code 2 is set.
> """
> 
> > 
> > in other words: is it specified architecturally that, once an order
> > is
> > accepted for a target CPU, that CPU can't accept any other order (and
> > will return CC2), including SENSE, until the order has been completed
> > successfully?  
> 
> The POPS quote I placed above excludes store-extended-status-address,
> conditional-emergency-signal, and sense-running-status orders as
> returning a CC2. That's something Janosch and I have chatted about
> offline, but don't have an answer to at this time. Besides that, any
> other order would get a CC2 until the STOP has been completed.

this is what I wanted to know, thanks

you can add

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


> 
> >   
> > > >     
> > > > >  }
> > > > >  
> > > > >  static void test_stop_store_status(void)    
> 

[PATCH kvm-unit-tests v1 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 | 22 ++++++++++++++++++++++
 lib/s390x/smp.h |  1 +
 s390x/smp.c     | 18 +++++++++++++++---
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 84e536e8..85b046a5 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -192,6 +192,28 @@ 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)
+{
+	spin_lock(&lock);
+
+	/* Don't suppress a CC2 with sigp_retry() */
+	if (smp_sigp(idx, 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 11c2c673..03160b80 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -55,23 +55,35 @@ static void test_restart(void)
 	struct cpu *cpu = smp_cpu_from_idx(1);
 	struct lowcore *lc = cpu->lowcore;
 
+	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;
 
 	/* Make sure cpu is stopped */
 	smp_cpu_stop(1);
 	set_flag(0);
-	smp_cpu_restart(1);
+	smp_cpu_restart_nowait(1);
+	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);
+	smp_cpu_restart_nowait(1);
+	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

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

[Claudio Imbrenda]

On Thu,  3 Mar 2022 22:04:24 +0100
Eric Farman <farman@linux.ibm.com> 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>
> ---
>  lib/s390x/smp.c | 22 ++++++++++++++++++++++
>  lib/s390x/smp.h |  1 +
>  s390x/smp.c     | 18 +++++++++++++++---
>  3 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 84e536e8..85b046a5 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -192,6 +192,28 @@ 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)
> +{
> +	spin_lock(&lock);
> +
> +	/* Don't suppress a CC2 with sigp_retry() */
> +	if (smp_sigp(idx, 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 11c2c673..03160b80 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -55,23 +55,35 @@ static void test_restart(void)
>  	struct cpu *cpu = smp_cpu_from_idx(1);
>  	struct lowcore *lc = cpu->lowcore;
>  
> +	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;
>  
>  	/* Make sure cpu is stopped */
>  	smp_cpu_stop(1);
>  	set_flag(0);
> -	smp_cpu_restart(1);
> +	smp_cpu_restart_nowait(1);
> +	report(!smp_cpu_stopped(1), "cpu started");

can this check ^ race?
we are using the flag to check if the CPU actually restarts, right?

>  	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);
> +	smp_cpu_restart_nowait(1);
> +	report(!smp_cpu_stopped(1), "cpu started");

same here

>  	wait_for_flag();
> -	report_pass("restart while running");
> +	report_pass("test flag");
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
>  }
>  
>  static void test_stop(void)

[PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Eric Farman]

A SIGP SENSE is used to determine if a CPU is stopped or operating,
and thus has a vested interest in ensuring it received a CC0 or CC1,
instead of a CC2 (BUSY). But, any order could receive a CC2 response,
and is probably ill-equipped to respond to it.

In practice, the order is likely to only encounter this when racing
with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which are
unlikely. But, since it's not impossible, let's convert the library
calls that issue a SIGP to loop on CC2 so the callers do not need
to react to that possible outcome.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 lib/s390x/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 85b046a5..2e476264 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx)
 
 bool smp_sense_running_status(uint16_t idx)
 {
-	if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
+	if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
 		return true;
 	/* Status stored condition code is equivalent to cpu not running. */
 	return false;
@@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw)
 	 * running after the restart.
 	 */
 	smp_cpu_stop_nolock(idx, false);
-	rc = smp_sigp(idx, SIGP_RESTART, 0, NULL);
+	rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL);
 	if (rc)
 		return rc;
 	/*
-- 
2.32.0

Re: [PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Janosch Frank]

On 3/3/22 22:04, Eric Farman wrote:
> A SIGP SENSE is used to determine if a CPU is stopped or operating,
> and thus has a vested interest in ensuring it received a CC0 or CC1,
> instead of a CC2 (BUSY). But, any order could receive a CC2 response,
> and is probably ill-equipped to respond to it.

sigp sense running status doesn't return a cc2, only sigp sense does afaik.
Looking at the KVM implementation tells me that it's not doing more than 
looking at the R bit in the sblk.

> 
> In practice, the order is likely to only encounter this when racing
> with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which are
> unlikely. But, since it's not impossible, let's convert the library
> calls that issue a SIGP to loop on CC2 so the callers do not need
> to react to that possible outcome.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   lib/s390x/smp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 85b046a5..2e476264 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx)
>   
>   bool smp_sense_running_status(uint16_t idx)
>   {
> -	if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
> +	if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
>   		return true;
>   	/* Status stored condition code is equivalent to cpu not running. */
>   	return false;
> @@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw)
>   	 * running after the restart.
>   	 */
>   	smp_cpu_stop_nolock(idx, false);
> -	rc = smp_sigp(idx, SIGP_RESTART, 0, NULL);
> +	rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL);
>   	if (rc)
>   		return rc;
>   	/*

Re: [PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Eric Farman]

On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote:
> On 3/3/22 22:04, Eric Farman wrote:
> > A SIGP SENSE is used to determine if a CPU is stopped or operating,
> > and thus has a vested interest in ensuring it received a CC0 or
> > CC1,
> > instead of a CC2 (BUSY). But, any order could receive a CC2
> > response,
> > and is probably ill-equipped to respond to it.
> 
> sigp sense running status doesn't return a cc2, only sigp sense does
> afaik.

The KVM routine handle_sigp_dst() returns the CC2 if a STOP/RESTART IRQ
is pending for any non-reset order, before it gets to the switch
statement that would route to the SIGP SENSE RUNNING handler.

> Looking at the KVM implementation tells me that it's not doing more
> than 
> looking at the R bit in the sblk.
> 
> > In practice, the order is likely to only encounter this when racing
> > with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which
> > are
> > unlikely. But, since it's not impossible, let's convert the library
> > calls that issue a SIGP to loop on CC2 so the callers do not need
> > to react to that possible outcome.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   lib/s390x/smp.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > index 85b046a5..2e476264 100644
> > --- a/lib/s390x/smp.c
> > +++ b/lib/s390x/smp.c
> > @@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx)
> >   
> >   bool smp_sense_running_status(uint16_t idx)
> >   {
> > -	if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) !=
> > SIGP_CC_STATUS_STORED)
> > +	if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) !=
> > SIGP_CC_STATUS_STORED)
> >   		return true;
> >   	/* Status stored condition code is equivalent to cpu not
> > running. */
> >   	return false;
> > @@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx,
> > struct psw *psw)
> >   	 * running after the restart.
> >   	 */
> >   	smp_cpu_stop_nolock(idx, false);
> > -	rc = smp_sigp(idx, SIGP_RESTART, 0, NULL);
> > +	rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL);
> >   	if (rc)
> >   		return rc;
> >   	/*

Re: [PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Nico Boehr]

On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote:
> On 3/3/22 22:04, Eric Farman wrote:
> > A SIGP SENSE is used to determine if a CPU is stopped or operating,
> > and thus has a vested interest in ensuring it received a CC0 or
> > CC1,
> > instead of a CC2 (BUSY). But, any order could receive a CC2
> > response,
> > and is probably ill-equipped to respond to it.
> 
> sigp sense running status doesn't return a cc2, only sigp sense does
> afaik.
> Looking at the KVM implementation tells me that it's not doing more
> than 
> looking at the R bit in the sblk.

From the POP I read _all_ orders may indeed return CC=2: case 1 under
"Conditions precluding Interpretation of the Order Code".

That being said, there are a few more users of smp_sigp (no retry) in
smp.c (the test, not the lib). 

Does it make sense to fix them aswell?

Re: [PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Eric Farman]

On Mon, 2022-03-07 at 15:42 +0100, Nico Boehr wrote:
> On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote:
> > On 3/3/22 22:04, Eric Farman wrote:
> > > A SIGP SENSE is used to determine if a CPU is stopped or
> > > operating,
> > > and thus has a vested interest in ensuring it received a CC0 or
> > > CC1,
> > > instead of a CC2 (BUSY). But, any order could receive a CC2
> > > response,
> > > and is probably ill-equipped to respond to it.
> > 
> > sigp sense running status doesn't return a cc2, only sigp sense
> > does
> > afaik.
> > Looking at the KVM implementation tells me that it's not doing more
> > than 
> > looking at the R bit in the sblk.
> 
> From the POP I read _all_ orders may indeed return CC=2: case 1 under
> "Conditions precluding Interpretation of the Order Code".
> 
> That being said, there are a few more users of smp_sigp (no retry) in
> smp.c (the test, not the lib). 
> 
> Does it make sense to fix them aswell?

I thought it made sense to do the lib, since other places expect those
things to "just work."

But for the tests themselves, I struggle to convince myself with one
path over another. The only way KVM returns a CC2 is because of a
concurrent STOP/RESTART, which isn't a possibility because of the
waiting the lib itself does when invoking the STOP/RESTART. So should
the tests be looking for an unexpected CC2? Or just loop when they
occur? If the latter, shouldn't the lib itself do that?

I'm happy to make changes, I just can't decide which it should be. Any
opinions?

Eric

Re: [PATCH kvm-unit-tests v1 6/6] lib: s390x: smp: Convert remaining smp_sigp to _retry

[Janosch Frank]

On 3/7/22 21:15, Eric Farman wrote:
> On Mon, 2022-03-07 at 15:42 +0100, Nico Boehr wrote:
>> On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote:
>>> On 3/3/22 22:04, Eric Farman wrote:
>>>> A SIGP SENSE is used to determine if a CPU is stopped or
>>>> operating,
>>>> and thus has a vested interest in ensuring it received a CC0 or
>>>> CC1,
>>>> instead of a CC2 (BUSY). But, any order could receive a CC2
>>>> response,
>>>> and is probably ill-equipped to respond to it.
>>>
>>> sigp sense running status doesn't return a cc2, only sigp sense
>>> does
>>> afaik.
>>> Looking at the KVM implementation tells me that it's not doing more
>>> than
>>> looking at the R bit in the sblk.
>>
>>  From the POP I read _all_ orders may indeed return CC=2: case 1 under
>> "Conditions precluding Interpretation of the Order Code".
>>
>> That being said, there are a few more users of smp_sigp (no retry) in
>> smp.c (the test, not the lib).
>>
>> Does it make sense to fix them aswell?
> 
> I thought it made sense to do the lib, since other places expect those
> things to "just work."
> 
> But for the tests themselves, I struggle to convince myself with one
> path over another. The only way KVM returns a CC2 is because of a
> concurrent STOP/RESTART, which isn't a possibility because of the
> waiting the lib itself does when invoking the STOP/RESTART. So should
> the tests be looking for an unexpected CC2? Or just loop when they
> occur? If the latter, shouldn't the lib itself do that?
> 
> I'm happy to make changes, I just can't decide which it should be. Any
> opinions?

Before we continue bikeshedding, let's add the cc2 retry. If it never 
returns cc2 we'll never loop on it but the dead code won't kill us either.