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

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

On Thu,  2 Dec 2021 10:58:43 +0100
David Hildenbrand <david@redhat.com> 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.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/firq.c        | 141 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |  10 ++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 s390x/firq.c
> 
> 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..3e60681
> --- /dev/null
> +++ b/s390x/firq.c
> @@ -0,0 +1,141 @@
> +/* 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 int testflag = 0;
> +
> +static void wait_for_flag(void)
> +{
> +	while (!testflag)
> +		mb();
> +}
> +
> +static void set_flag(int val)
> +{
> +	mb();
> +	testflag = val;
> +	mb();
> +}
> +
> +static void wait_for_sclp_int(void)
> +{
> +	/* Enable SCLP interrupts on this CPU only. */
> +	ctl_set_bit(0, CTL0_SERVICE_SIGNAL);
> +
> +	set_flag(1);

why not just WRITE_ONCE/READ_ONCE?

> +
> +	/* Enable external interrupts and go to the wait state. */
> +	wait_for_interrupt(PSW_MASK_EXT);
> +}
> +
> +/*
> + * 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;
> +	}
> +
> +	/*
> +	 * 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");
> +		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();

this means that now no SCLP command can happen as long as the flag
stays set....

> +	set_flag(0);
> +
> +	/* 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) {
> +		report_skip("cpu #1 not found");

...which means that this will hang, and so will all the other report*
functions. maybe you should manually unset the flag before calling the
various report* functions.

> +		goto out;
> +	}
> +
> +	/* Wait until the CPU #1 at least enabled SCLP interrupts. */
> +	wait_for_flag();
> +
> +	/*
> +	 * 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 make races very rare.
> +	 */
> +	while(smp_sense_running_status(1));
> +
> +	h = alloc_page();

do you really need to dynamically allocate one page?
is there a reason for not using a simple static buffer? (which you can
have aligned and statically initialized)

> +	memset(h, 0, sizeof(*h));

otherwise, if you really want to allocate the memory, get rid of the
memset; the allocator always returns zeroed memory (unless you
explicitly ask not to by using flags)

> +	h->length = 4096;
> +	ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
> +	if (ret) {
> +		report_fail("SCLP_CMDW_READ_CPU_INFO failed");
> +		goto out_destroy;
> +	}
> +
> +	/*
> +	 * Wait until the interrupt gets delivered on CPU #1, marking the

why do you expect the interrupt to be delivered on CPU1? could it not
be delivered on CPU0?

> +	 * SCLP requests as done.
> +	 */
> +	sclp_wait_busy();

this is logically not wrong (and should stay, because it makes clear
what you are trying to do), but strictly speaking it's not needed since
the report below will hang as long as the SCLP busy flag is set. 

> +
> +	report(true, "firq delivered");
> +
> +out_destroy:
> +	smp_cpu_destroy(1);
> +	free_page(h);
> +out:
> +	report_prefix_pop();
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("firq");
> +
> +	test_wait_state_delivery();
> +
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3b454b7..054560c 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -112,3 +112,13 @@ file = mvpg-sie.elf
>  
>  [spec_ex-sie]
>  file = spec_ex-sie.elf
> +
> +[firq-linear-cpu-ids]
> +file = firq.elf
> +timeout = 20
> +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=1 -device qemu-s390x-cpu,core-id=2
> +
> +[firq-nonlinear-cpu-ids]
> +file = firq.elf
> +timeout = 20
> +extra_params = -smp 1,maxcpus=3 -cpu qemu -device qemu-s390x-cpu,core-id=2 -device qemu-s390x-cpu,core-id=1