Re: [kvm-unit-tests PATCH v2 2/2] s390x: firq: floating interrupt test

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 3 Dec 2021 11:55:31 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 02/12/2021 13.35, David Hildenbrand wrote:
> > We had a KVM BUG fixed by kernel commit a3e03bc1368c ("KVM: s390: index
> > kvm->arch.idle_mask by vcpu_idx"), whereby a floating interrupt might get
> > stuck forever because a CPU in the wait state would not get woken up.
> > 
> > The issue can be triggered when CPUs are created in a nonlinear fashion,
> > such that the CPU address ("core-id") and the KVM cpu id don't match.
> > 
> > So let's start with a floating interrupt test that will trigger a
> > floating interrupt (via SCLP) to be delivered to a CPU in the wait state.  
> 
> Thank you very much for tackling this! Some remarks below...
> 
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >   lib/s390x/sclp.c    |  11 ++--
> >   lib/s390x/sclp.h    |   1 +
> >   s390x/Makefile      |   1 +
> >   s390x/firq.c        | 122 ++++++++++++++++++++++++++++++++++++++++++++
> >   s390x/unittests.cfg |  10 ++++
> >   5 files changed, 142 insertions(+), 3 deletions(-)
> >   create mode 100644 s390x/firq.c
> > 
> > diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> > index 0272249..33985eb 100644
> > --- a/lib/s390x/sclp.c
> > +++ b/lib/s390x/sclp.c
> > @@ -60,9 +60,7 @@ void sclp_setup_int(void)
> >   void sclp_handle_ext(void)
> >   {
> >   	ctl_clear_bit(0, CTL0_SERVICE_SIGNAL);
> > -	spin_lock(&sclp_lock);
> > -	sclp_busy = false;
> > -	spin_unlock(&sclp_lock);
> > +	sclp_clear_busy();
> >   }
> >   
> >   void sclp_wait_busy(void)
> > @@ -89,6 +87,13 @@ void sclp_mark_busy(void)
> >   	}
> >   }
> >   
> > +void sclp_clear_busy(void)
> > +{
> > +	spin_lock(&sclp_lock);
> > +	sclp_busy = false;
> > +	spin_unlock(&sclp_lock);
> > +}
> > +
> >   static void sclp_read_scp_info(ReadInfo *ri, int length)
> >   {
> >   	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
> > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> > index 61e9cf5..fead007 100644
> > --- a/lib/s390x/sclp.h
> > +++ b/lib/s390x/sclp.h
> > @@ -318,6 +318,7 @@ void sclp_setup_int(void);
> >   void sclp_handle_ext(void);
> >   void sclp_wait_busy(void);
> >   void sclp_mark_busy(void);
> > +void sclp_clear_busy(void);
> >   void sclp_console_setup(void);
> >   void sclp_print(const char *str);
> >   void sclp_read_info(void);
> > diff --git a/s390x/Makefile b/s390x/Makefile
> > index f95f2e6..1e567c1 100644
> > --- a/s390x/Makefile
> > +++ b/s390x/Makefile
> > @@ -25,6 +25,7 @@ tests += $(TEST_DIR)/uv-host.elf
> >   tests += $(TEST_DIR)/edat.elf
> >   tests += $(TEST_DIR)/mvpg-sie.elf
> >   tests += $(TEST_DIR)/spec_ex-sie.elf
> > +tests += $(TEST_DIR)/firq.elf
> >   
> >   tests_binary = $(patsubst %.elf,%.bin,$(tests))
> >   ifneq ($(HOST_KEY_DOCUMENT),)
> > diff --git a/s390x/firq.c b/s390x/firq.c
> > new file mode 100644
> > index 0000000..1f87718
> > --- /dev/null
> > +++ b/s390x/firq.c
> > @@ -0,0 +1,122 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Floating interrupt tests.
> > + *
> > + * Copyright 2021 Red Hat Inc
> > + *
> > + * Authors:
> > + *    David Hildenbrand <david@redhat.com>
> > + */
> > +#include <libcflat.h>
> > +#include <asm/asm-offsets.h>
> > +#include <asm/interrupt.h>
> > +#include <asm/page.h>
> > +#include <asm-generic/barrier.h>
> > +
> > +#include <sclp.h>
> > +#include <smp.h>
> > +#include <alloc_page.h>
> > +
> > +static void wait_for_sclp_int(void)
> > +{
> > +	/* Enable SCLP interrupts on this CPU only. */
> > +	ctl_set_bit(0, CTL0_SERVICE_SIGNAL);
> > +
> > +	/* Enable external interrupts and go to the wait state. */
> > +	wait_for_interrupt(PSW_MASK_EXT);
> > +}  
> 
> What happens if the CPU got an interrupt? Should there be a "while (true)" 

it should not get any interrupts, but if it does anyway...

> at the end of the function to avoid that the CPU ends up crashing at the end 
> of the function?

... we have this in smp_cpu_setup_state, after the call to the actual
function body:

/* If the function returns, just loop here */
0:      j       0

so if the function returns, it will hang in there anyway

>
> > +/*
> > + * Some KVM versions might mix CPUs when looking for a floating IRQ target,
> > + * accidentially detecting a stopped CPU as waiting and resulting in the actually
> > + * waiting CPU not getting woken up for the interrupt.
> > + */
> > +static void test_wait_state_delivery(void)
> > +{
> > +	struct psw psw;
> > +	SCCBHeader *h;
> > +	int ret;
> > +
> > +	report_prefix_push("wait state delivery");
> > +
> > +	if (smp_query_num_cpus() < 3) {
> > +		report_skip("need at least 3 CPUs for this test");
> > +		goto out;
> > +	}
> > +
> > +	if (stap()) {
> > +		report_skip("need to start on CPU #0");
> > +		goto out;
> > +	}  
> 
> I think I'd rather turn this into an assert() instead ... no strong opinion 
> about it, though.

I agree, including the part about no strong opinions (which is why I
did not comment on it before)

> 
> > +
> > +	/*
> > +	 * We want CPU #2 to be stopped. This should be the case at this
> > +	 * point, however, we want to sense if it even exists as well.
> > +	 */
> > +	ret = smp_cpu_stop(2);
> > +	if (ret) {
> > +		report_skip("CPU #2 not found");  
> 
> Since you already queried for the availablity of at least 3 CPUs above, I 
> think you could turn this into a report_fail() instead?

either that or an assert, but again, no strong opinions

> 
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * We're going to perform an SCLP service call but expect
> > +	 * the interrupt on CPU #1 while it is in the wait state.
> > +	 */
> > +	sclp_mark_busy();
> > +
> > +	/* Start CPU #1 and let it wait for the interrupt. */
> > +	psw.mask = extract_psw_mask();
> > +	psw.addr = (unsigned long)wait_for_sclp_int;
> > +	ret = smp_cpu_setup(1, psw);
> > +	if (ret) {
> > +		sclp_clear_busy();
> > +		report_skip("cpu #1 not found");
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * We'd have to jump trough some hoops to sense e.g., via SIGP
> > +	 * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the
> > +	 * wait state.
> > +	 *
> > +	 * Although not completely reliable, use SIGP SENSE RUNNING STATUS
> > +	 * until not reported as running -- after all, our SCLP processing
> > +	 * will take some time as well and smp_cpu_setup() returns when we're
> > +	 * either already in wait_for_sclp_int() or just about to execute it.
> > +	 */
> > +	while(smp_sense_running_status(1));
> > +
> > +	h = alloc_page();
> > +	h->length = 4096;
> > +	ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
> > +	if (ret) {
> > +		sclp_clear_busy();
> > +		report_fail("SCLP_CMDW_READ_CPU_INFO failed");
> > +		goto out_destroy;
> > +	}
> > +
> > +	/*
> > +	 * Wait until the interrupt gets delivered on CPU #1, marking the
> > +	 * SCLP requests as done.
> > +	 */
> > +	sclp_wait_busy();
> > +
> > +	report(true, "sclp interrupt delivered");
> > +
> > +out_destroy:
> > +	free_page(h);
> > +	smp_cpu_destroy(1);
> > +out:
> > +	report_prefix_pop();
> > +}  
> 
> Anyway, code looks fine for me, either with my comments addressed or not:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>