On Fri, 11 Mar 2022 18:38:19 +0100
Eric Farman <farman@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> ---
> 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();
