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

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

[David Hildenbrand]

From patch #2:

"
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.
"

David Hildenbrand (2):
  s390x: make smp_cpu_setup() return 0 on success
  s390x: firq: floating interrupt test

 lib/s390x/smp.c     |   1 +
 s390x/Makefile      |   1 +
 s390x/firq.c        | 141 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |  10 ++++
 4 files changed, 153 insertions(+)
 create mode 100644 s390x/firq.c

-- 
2.31.1

[kvm-unit-tests PATCH v1 1/2] s390x: make smp_cpu_setup() return 0 on success

[David Hildenbrand]

Properly return "0" on success so callers can check if the setup was
successful.

The return value is yet unused, which is why this wasn't noticed so far.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 lib/s390x/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index da6d32f..b753eab 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -212,6 +212,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	/* Wait until the cpu has finished setup and started the provided psw */
 	while (lc->restart_new_psw.addr != psw.addr)
 		mb();
+	rc = 0;
 out:
 	spin_unlock(&lock);
 	return rc;
-- 
2.31.1

Re: [kvm-unit-tests PATCH v1 1/2] s390x: make smp_cpu_setup() return 0 on success

[Thomas Huth]

On 02/12/2021 10.58, David Hildenbrand wrote:
> Properly return "0" on success so callers can check if the setup was
> successful.
> 
> The return value is yet unused, which is why this wasn't noticed so far.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   lib/s390x/smp.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index da6d32f..b753eab 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -212,6 +212,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>   	/* Wait until the cpu has finished setup and started the provided psw */
>   	while (lc->restart_new_psw.addr != psw.addr)
>   		mb();
> +	rc = 0;
>   out:
>   	spin_unlock(&lock);
>   	return rc;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v1 1/2] s390x: make smp_cpu_setup() return 0 on success

[Claudio Imbrenda]

On Thu,  2 Dec 2021 10:58:42 +0100
David Hildenbrand <david@redhat.com> wrote:

> Properly return "0" on success so callers can check if the setup was
> successful.
> 
> The return value is yet unused, which is why this wasn't noticed so far.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/smp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index da6d32f..b753eab 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -212,6 +212,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>  	/* Wait until the cpu has finished setup and started the provided psw */
>  	while (lc->restart_new_psw.addr != psw.addr)
>  		mb();
> +	rc = 0;
>  out:
>  	spin_unlock(&lock);
>  	return rc;

Re: [kvm-unit-tests PATCH v1 1/2] s390x: make smp_cpu_setup() return 0 on success

[Janosch Frank]

On 12/2/21 10:58, David Hildenbrand wrote:
> Properly return "0" on success so callers can check if the setup was
> successful.
> 
> The return value is yet unused, which is why this wasn't noticed so far.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   lib/s390x/smp.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index da6d32f..b753eab 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -212,6 +212,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
>   	/* Wait until the cpu has finished setup and started the provided psw */
>   	while (lc->restart_new_psw.addr != psw.addr)
>   		mb();
> +	rc = 0;
>   out:
>   	spin_unlock(&lock);
>   	return rc;
> 

oops

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

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

[David Hildenbrand]

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);
+
+	/* 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();
+	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");
+		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();
+	memset(h, 0, sizeof(*h));
+	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
+	 * SCLP requests as done.
+	 */
+	sclp_wait_busy();
+
+	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
-- 
2.31.1

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

[Claudio Imbrenda]

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

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

[David Hildenbrand]

>> +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?

Because I shamelessly copied that from s390x/smp.c ;)

>> +	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.

Good point, thanks!

> 
>> +		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)

I don't really have a strong opinion. I do prefer dynamic alloctions,
though, if there isn't a good reason not to use them. No need to mess
with page alignments manually.

> 
>> +	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)

Right. "special" FLAG_DONTZERO in that semantics in that allocator.

> 
>> +	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?

We don't enable SCLP interrupts + external interrupts on CPU #0 because
we'll only call sclp_setup_int() on CPU #1.

> 
>> +	 * 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. 

Right. But it's really clearer to just have this in the code.


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 02.12.21 12:13, David Hildenbrand wrote:
>>> +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?
> 
> Because I shamelessly copied that from s390x/smp.c ;)

Oh, and also because WRITE_ONCE/READ_ONCE are semantically the wrong
thing to use:

"Prevent the compiler from merging or refetching reads or writes. The
compiler is also forbidden from reordering successive instances of
READ_ONCE and WRITE_ONCE"

We need memory barriers to prevent reordering with surrounding code.

-- 
Thanks,

David / dhildenb

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

[Claudio Imbrenda]

On Thu, 2 Dec 2021 12:13:08 +0100
David Hildenbrand <david@redhat.com> wrote:

> >> +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?  
> 
> Because I shamelessly copied that from s390x/smp.c ;)
> 
> >> +	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.  
> 
> Good point, thanks!
> 
> >   
> >> +		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));

if you wait here for CPU1 to be in wait state, then why did you need to
wait until it has set the flag earlier? can't you just wait here and not
use the whole wait_for_flag logic? smp_cpu_setup only returns after the
new CPU has started running.

I guess this was also inspired by smp.c :)

> >> +
> >> +	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)  
> 
> I don't really have a strong opinion. I do prefer dynamic alloctions,
> though, if there isn't a good reason not to use them. No need to mess
> with page alignments manually.

fair enough, I also do not have a strong opinion

> >   
> >> +	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)  
> 
> Right. "special" FLAG_DONTZERO in that semantics in that allocator.
> 
> >   
> >> +	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?  
> 
> We don't enable SCLP interrupts + external interrupts on CPU #0 because
> we'll only call sclp_setup_int() on CPU #1.

oh right, I had missed that

> 
> >   
> >> +	 * 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.   
> 
> Right. But it's really clearer to just have this in the code.

yep, which is why I said that this should stay :)

> 
> 

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

[David Hildenbrand]

On 02.12.21 13:07, Claudio Imbrenda wrote:
> On Thu, 2 Dec 2021 12:13:08 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>> +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?  
>>
>> Because I shamelessly copied that from s390x/smp.c ;)
>>
>>>> +	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.  
>>
>> Good point, thanks!
>>
>>>   
>>>> +		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));
> 
> if you wait here for CPU1 to be in wait state, then why did you need to
> wait until it has set the flag earlier? can't you just wait here and not
> use the whole wait_for_flag logic? smp_cpu_setup only returns after the
> new CPU has started running.

I use the flag right now as a mechanism to make the race window as short
as possible. But you're right, we might not need the flag logic as
knowing that we processed the setup part and will jump/jumped to the
target code might be good enough.

Thanks!

-- 
Thanks,

David / dhildenb