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