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

[Eric Farman <farman@linux.ibm.com>]

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