[kvm-unit-tests PATCH v3 0/4] Add panic test support

[kvm-unit-tests PATCH v3 0/4] Add panic test support

[Nico Boehr]

v2->v3:
---
* rename TIMING_S390_SHIFT_US->S390_CLOCK_SHIFT_US (thanks Claudio)
* rename cpu_timer_set -> cpu_timer_set_ms (thanks Claudio)

v1->v2:
---
* fix spelling mistakes/long lines (thanks Thomas)
* quoting improvments (thanks Thomas)
* smaller timeout for tests (thanks Thomas and Janis)
* move function and defines for cpu timer to time.h (thanks Janosch)
* fence tests for non-QEMU/KVM (thanks Janosch)

QEMU suports a guest state "guest-panicked" which indicates something in
the guest went wrong, for example on s390x, when an external interrupt
loop was triggered.

Since the guest does not continue to run when it is in the
guest-panicked state, it is currently impossible to write panicking
tests in kvm-unit-tests. Support from the runtime is needed to check
that the guest enters the guest-panicked state.

This series adds the required support to the runtime together with two
tests for s390x which cause guest panics.

Nico Boehr (4):
  runtime: add support for panic tests
  lib: s390x: add CPU timer functions to time.h
  s390x: add extint loop test
  s390x: add pgm spec interrupt loop test

 lib/s390x/asm/time.h      | 17 ++++++++++-
 s390x/Makefile            |  2 ++
 s390x/panic-loop-extint.c | 60 +++++++++++++++++++++++++++++++++++++++
 s390x/panic-loop-pgm.c    | 53 ++++++++++++++++++++++++++++++++++
 s390x/run                 |  2 +-
 s390x/unittests.cfg       | 12 ++++++++
 scripts/arch-run.bash     | 49 ++++++++++++++++++++++++++++++++
 scripts/runtime.bash      |  3 ++
 8 files changed, 196 insertions(+), 2 deletions(-)
 create mode 100644 s390x/panic-loop-extint.c
 create mode 100644 s390x/panic-loop-pgm.c

-- 
2.36.1

[kvm-unit-tests PATCH v3 1/4] runtime: add support for panic tests

[Nico Boehr]

QEMU supports a guest state "guest-panicked" which indicates something
in the guest went wrong, for example on s390x, when an external
interrupt loop was triggered.

Since the guest does not continue to run when it is in the
guest-panicked state, it is currently impossible to write panicking
tests in kvm-unit-tests. Support from the runtime is needed to check
that the guest enters the guest-panicked state.

Similar to migration tests, add a new group panic. Tests in this
group must enter the guest-panicked state to succeed.

The runtime will spawn a QEMU instance, connect to the QMP and listen
for events. To parse the QMP protocol, jq[1] is used. Same as with
netcat in the migration tests, panic tests won't run if jq is not
installed.

The guest is created in the stopped state and only continued when
connection to the QMP was successful. This ensures no events are missed
between QEMU start and the connect to the QMP.

[1] https://stedolan.github.io/jq/

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 s390x/run             |  2 +-
 scripts/arch-run.bash | 49 +++++++++++++++++++++++++++++++++++++++++++
 scripts/runtime.bash  |  3 +++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/s390x/run b/s390x/run
index 24138f6803be..f1111dbdbe62 100755
--- a/s390x/run
+++ b/s390x/run
@@ -30,7 +30,7 @@ M+=",accel=$ACCEL"
 command="$qemu -nodefaults -nographic $M"
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
 command+=" -kernel"
-command="$(migration_cmd) $(timeout_cmd) $command"
+command="$(panic_cmd) $(migration_cmd) $(timeout_cmd) $command"
 
 # We return the exit code via stdout, not via the QEMU return code
 run_qemu_status $command "$@"
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 0dfaf017db0a..739490bc7da2 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -104,6 +104,14 @@ qmp ()
 	echo '{ "execute": "qmp_capabilities" }{ "execute":' "$2" '}' | ncat -U $1
 }
 
+qmp_events ()
+{
+	while ! test -S "$1"; do sleep 0.1; done
+	echo '{ "execute": "qmp_capabilities" }{ "execute": "cont" }' \
+		| ncat --no-shutdown -U $1 \
+		| jq -c 'select(has("event"))'
+}
+
 run_migration ()
 {
 	if ! command -v ncat >/dev/null 2>&1; then
@@ -164,6 +172,40 @@ run_migration ()
 	return $ret
 }
 
+run_panic ()
+{
+	if ! command -v ncat >/dev/null 2>&1; then
+		echo "${FUNCNAME[0]} needs ncat (netcat)" >&2
+		return 77
+	fi
+
+	if ! command -v jq >/dev/null 2>&1; then
+		echo "${FUNCNAME[0]} needs jq" >&2
+		return 77
+	fi
+
+	qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX)
+
+	trap 'kill 0; exit 2' INT TERM
+	trap 'rm -f ${qmp}' RETURN EXIT
+
+	# start VM stopped so we don't miss any events
+	eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \
+		-mon chardev=mon1,mode=control -S &
+
+	panic_event_count=$(qmp_events ${qmp} | jq -c 'select(.event == "GUEST_PANICKED")' | wc -l)
+	if [ "$panic_event_count" -lt 1 ]; then
+		echo "FAIL: guest did not panic"
+		ret=3
+	else
+		# some QEMU versions report multiple panic events
+		echo "PASS: guest panicked"
+		ret=1
+	fi
+
+	return $ret
+}
+
 migration_cmd ()
 {
 	if [ "$MIGRATION" = "yes" ]; then
@@ -171,6 +213,13 @@ migration_cmd ()
 	fi
 }
 
+panic_cmd ()
+{
+	if [ "$PANIC" = "yes" ]; then
+		echo "run_panic"
+	fi
+}
+
 search_qemu_binary ()
 {
 	local save_path=$PATH
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 7d0180bf14bd..8072f3bb536a 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -145,6 +145,9 @@ function run()
     if find_word "migration" "$groups"; then
         cmdline="MIGRATION=yes $cmdline"
     fi
+    if find_word "panic" "$groups"; then
+        cmdline="PANIC=yes $cmdline"
+    fi
     if [ "$verbose" = "yes" ]; then
         echo $cmdline
     fi
-- 
2.36.1

[kvm-unit-tests PATCH v3 2/4] lib: s390x: add CPU timer functions to time.h

[Nico Boehr]

Upcoming changes will make use of the CPU timer, so add a convenience
function to set the CPU timer.

Since shifts for both CPU timer and TOD clock are the same, introduce a
new define S390_CLOCK_SHIFT_US. The respective shifts for CPU timer and
TOD clock reference it, so the semantic difference between the two
defines is kept.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/time.h | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
index 7652a151e87a..d8d91d68a667 100644
--- a/lib/s390x/asm/time.h
+++ b/lib/s390x/asm/time.h
@@ -11,9 +11,13 @@
 #ifndef _ASMS390X_TIME_H_
 #define _ASMS390X_TIME_H_
 
-#define STCK_SHIFT_US	(63 - 51)
+#define S390_CLOCK_SHIFT_US	(63 - 51)
+
+#define STCK_SHIFT_US	S390_CLOCK_SHIFT_US
 #define STCK_MAX	((1UL << 52) - 1)
 
+#define CPU_TIMER_SHIFT_US	S390_CLOCK_SHIFT_US
+
 static inline uint64_t get_clock_us(void)
 {
 	uint64_t clk;
@@ -45,4 +49,15 @@ static inline void mdelay(unsigned long ms)
 	udelay(ms * 1000);
 }
 
+static inline void cpu_timer_set_ms(int64_t timeout_ms)
+{
+	int64_t timer_value = (timeout_ms * 1000) << CPU_TIMER_SHIFT_US;
+
+	asm volatile (
+		"spt %[timer_value]\n"
+		:
+		: [timer_value] "Q" (timer_value)
+	);
+}
+
 #endif
-- 
2.36.1

[kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Nico Boehr]

The CPU timer interrupt stays pending as long as the CPU timer value is
negative. This can lead to interruption loops when the ext_new_psw mask
has external interrupts enabled.

QEMU is able to detect this situation and panic the guest, so add a test
for it.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 s390x/Makefile            |  1 +
 s390x/panic-loop-extint.c | 60 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg       |  6 ++++
 3 files changed, 67 insertions(+)
 create mode 100644 s390x/panic-loop-extint.c

diff --git a/s390x/Makefile b/s390x/Makefile
index efd5e0c13102..e4649da50d9d 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -34,6 +34,7 @@ tests += $(TEST_DIR)/migration.elf
 tests += $(TEST_DIR)/pv-attest.elf
 tests += $(TEST_DIR)/migration-cmm.elf
 tests += $(TEST_DIR)/migration-skey.elf
+tests += $(TEST_DIR)/panic-loop-extint.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/panic-loop-extint.c b/s390x/panic-loop-extint.c
new file mode 100644
index 000000000000..d3a3f06d9a34
--- /dev/null
+++ b/s390x/panic-loop-extint.c
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * External interrupt loop test
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+#include <asm/interrupt.h>
+#include <asm/barrier.h>
+#include <asm/time.h>
+#include <hardware.h>
+
+static void ext_int_handler(void)
+{
+	/*
+	 * return to ext_old_psw. This gives us the chance to print the return_fail
+	 * in case something goes wrong.
+	 */
+	asm volatile (
+		"lpswe %[ext_old_psw]\n"
+		:
+		: [ext_old_psw] "Q"(lowcore.ext_old_psw)
+		: "memory"
+	);
+}
+
+int main(void)
+{
+	struct psw ext_new_psw_orig;
+
+	report_prefix_push("panic-loop-extint");
+
+	if (!host_is_qemu() || host_is_tcg()) {
+		report_skip("QEMU-KVM-only test");
+		goto out;
+	}
+
+	ext_new_psw_orig = lowcore.ext_new_psw;
+	lowcore.ext_new_psw.addr = (uint64_t)ext_int_handler;
+	lowcore.ext_new_psw.mask |= PSW_MASK_EXT;
+
+	load_psw_mask(extract_psw_mask() | PSW_MASK_EXT);
+	ctl_set_bit(0, CTL0_CLOCK_COMPARATOR);
+
+	cpu_timer_set_ms(1);
+
+	mdelay(2000);
+
+	/* restore previous ext_new_psw so QEMU can properly terminate */
+	lowcore.ext_new_psw = ext_new_psw_orig;
+
+	report_fail("survived extint loop");
+
+out:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index f7b1fc3dbca1..b1b25f118ff6 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -185,3 +185,9 @@ groups = migration
 [migration-skey]
 file = migration-skey.elf
 groups = migration
+
+[panic-loop-extint]
+file = panic-loop-extint.elf
+groups = panic
+accel = kvm
+timeout = 5
-- 
2.36.1

[kvm-unit-tests PATCH v3 4/4] s390x: add pgm spec interrupt loop test

[Nico Boehr]

An invalid PSW causes a program interrupt. When an invalid PSW is
introduced in the pgm_new_psw, an interrupt loop occurs as soon as a
program interrupt is caused.

QEMU should detect that and panic the guest, hence add a test for it.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile         |  1 +
 s390x/panic-loop-pgm.c | 53 ++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg    |  6 +++++
 3 files changed, 60 insertions(+)
 create mode 100644 s390x/panic-loop-pgm.c

diff --git a/s390x/Makefile b/s390x/Makefile
index e4649da50d9d..66415d0b588d 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -35,6 +35,7 @@ tests += $(TEST_DIR)/pv-attest.elf
 tests += $(TEST_DIR)/migration-cmm.elf
 tests += $(TEST_DIR)/migration-skey.elf
 tests += $(TEST_DIR)/panic-loop-extint.elf
+tests += $(TEST_DIR)/panic-loop-pgm.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/panic-loop-pgm.c b/s390x/panic-loop-pgm.c
new file mode 100644
index 000000000000..68934057a251
--- /dev/null
+++ b/s390x/panic-loop-pgm.c
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Program interrupt loop test
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+#include <bitops.h>
+#include <asm/interrupt.h>
+#include <asm/barrier.h>
+#include <hardware.h>
+
+static void pgm_int_handler(void)
+{
+	/*
+	 * return to pgm_old_psw. This gives us the chance to print the return_fail
+	 * in case something goes wrong.
+	 */
+	asm volatile (
+		"lpswe %[pgm_old_psw]\n"
+		:
+		: [pgm_old_psw] "Q"(lowcore.pgm_old_psw)
+		: "memory"
+	);
+}
+
+int main(void)
+{
+	report_prefix_push("panic-loop-pgm");
+
+	if (!host_is_qemu() || host_is_tcg()) {
+		report_skip("QEMU-KVM-only test");
+		goto out;
+	}
+
+	lowcore.pgm_new_psw.addr = (uint64_t) pgm_int_handler;
+	/* bit 12 set is invalid */
+	lowcore.pgm_new_psw.mask = extract_psw_mask() | BIT(63 - 12);
+	mb();
+
+	/* cause a pgm int */
+	*((int *)-4) = 0x42;
+	mb();
+
+	report_fail("survived pgmint loop");
+
+out:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index b1b25f118ff6..f9f102abfa89 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -191,3 +191,9 @@ file = panic-loop-extint.elf
 groups = panic
 accel = kvm
 timeout = 5
+
+[panic-loop-pgm]
+file = panic-loop-pgm.elf
+groups = panic
+accel = kvm
+timeout = 5
-- 
2.36.1

Re: [kvm-unit-tests PATCH v3 1/4] runtime: add support for panic tests

[Claudio Imbrenda]

On Fri, 22 Jul 2022 08:00:40 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> QEMU supports a guest state "guest-panicked" which indicates something
> in the guest went wrong, for example on s390x, when an external
> interrupt loop was triggered.
> 
> Since the guest does not continue to run when it is in the
> guest-panicked state, it is currently impossible to write panicking
> tests in kvm-unit-tests. Support from the runtime is needed to check
> that the guest enters the guest-panicked state.
> 
> Similar to migration tests, add a new group panic. Tests in this
> group must enter the guest-panicked state to succeed.
> 
> The runtime will spawn a QEMU instance, connect to the QMP and listen
> for events. To parse the QMP protocol, jq[1] is used. Same as with
> netcat in the migration tests, panic tests won't run if jq is not
> installed.
> 
> The guest is created in the stopped state and only continued when
> connection to the QMP was successful. This ensures no events are missed
> between QEMU start and the connect to the QMP.
> 
> [1] https://stedolan.github.io/jq/
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  s390x/run             |  2 +-
>  scripts/arch-run.bash | 49 +++++++++++++++++++++++++++++++++++++++++++
>  scripts/runtime.bash  |  3 +++
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/s390x/run b/s390x/run
> index 24138f6803be..f1111dbdbe62 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -30,7 +30,7 @@ M+=",accel=$ACCEL"
>  command="$qemu -nodefaults -nographic $M"
>  command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
>  command+=" -kernel"
> -command="$(migration_cmd) $(timeout_cmd) $command"
> +command="$(panic_cmd) $(migration_cmd) $(timeout_cmd) $command"
>  
>  # We return the exit code via stdout, not via the QEMU return code
>  run_qemu_status $command "$@"
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 0dfaf017db0a..739490bc7da2 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -104,6 +104,14 @@ qmp ()
>  	echo '{ "execute": "qmp_capabilities" }{ "execute":' "$2" '}' | ncat -U $1
>  }
>  
> +qmp_events ()
> +{
> +	while ! test -S "$1"; do sleep 0.1; done
> +	echo '{ "execute": "qmp_capabilities" }{ "execute": "cont" }' \

if you put the pipe at the end of the line, instead of the beginning,
then you don't need the \ . I think it is easier to read without the \
and it is also more robust (no need to worry about spaces)

> +		| ncat --no-shutdown -U $1 \
> +		| jq -c 'select(has("event"))'
> +}
> +
>  run_migration ()
>  {
>  	if ! command -v ncat >/dev/null 2>&1; then
> @@ -164,6 +172,40 @@ run_migration ()
>  	return $ret
>  }
>  
> +run_panic ()
> +{
> +	if ! command -v ncat >/dev/null 2>&1; then
> +		echo "${FUNCNAME[0]} needs ncat (netcat)" >&2
> +		return 77
> +	fi
> +
> +	if ! command -v jq >/dev/null 2>&1; then
> +		echo "${FUNCNAME[0]} needs jq" >&2
> +		return 77
> +	fi
> +
> +	qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX)
> +
> +	trap 'kill 0; exit 2' INT TERM
> +	trap 'rm -f ${qmp}' RETURN EXIT
> +
> +	# start VM stopped so we don't miss any events
> +	eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \
> +		-mon chardev=mon1,mode=control -S &
> +
> +	panic_event_count=$(qmp_events ${qmp} | jq -c 'select(.event == "GUEST_PANICKED")' | wc -l)
> +	if [ "$panic_event_count" -lt 1 ]; then
> +		echo "FAIL: guest did not panic"
> +		ret=3
> +	else
> +		# some QEMU versions report multiple panic events
> +		echo "PASS: guest panicked"
> +		ret=1

so we never return 0? is that intentional?

> +	fi
> +
> +	return $ret
> +}
> +
>  migration_cmd ()
>  {
>  	if [ "$MIGRATION" = "yes" ]; then
> @@ -171,6 +213,13 @@ migration_cmd ()
>  	fi
>  }
>  
> +panic_cmd ()
> +{
> +	if [ "$PANIC" = "yes" ]; then
> +		echo "run_panic"
> +	fi
> +}
> +
>  search_qemu_binary ()
>  {
>  	local save_path=$PATH
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 7d0180bf14bd..8072f3bb536a 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -145,6 +145,9 @@ function run()
>      if find_word "migration" "$groups"; then
>          cmdline="MIGRATION=yes $cmdline"
>      fi
> +    if find_word "panic" "$groups"; then
> +        cmdline="PANIC=yes $cmdline"
> +    fi
>      if [ "$verbose" = "yes" ]; then
>          echo $cmdline
>      fi

Re: [kvm-unit-tests PATCH v3 2/4] lib: s390x: add CPU timer functions to time.h

[Claudio Imbrenda]

On Fri, 22 Jul 2022 08:00:41 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Upcoming changes will make use of the CPU timer, so add a convenience
> function to set the CPU timer.
> 
> Since shifts for both CPU timer and TOD clock are the same, introduce a
> new define S390_CLOCK_SHIFT_US. The respective shifts for CPU timer and
> TOD clock reference it, so the semantic difference between the two
> defines is kept.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

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

> ---
>  lib/s390x/asm/time.h | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> index 7652a151e87a..d8d91d68a667 100644
> --- a/lib/s390x/asm/time.h
> +++ b/lib/s390x/asm/time.h
> @@ -11,9 +11,13 @@
>  #ifndef _ASMS390X_TIME_H_
>  #define _ASMS390X_TIME_H_
>  
> -#define STCK_SHIFT_US	(63 - 51)
> +#define S390_CLOCK_SHIFT_US	(63 - 51)
> +
> +#define STCK_SHIFT_US	S390_CLOCK_SHIFT_US
>  #define STCK_MAX	((1UL << 52) - 1)
>  
> +#define CPU_TIMER_SHIFT_US	S390_CLOCK_SHIFT_US
> +
>  static inline uint64_t get_clock_us(void)
>  {
>  	uint64_t clk;
> @@ -45,4 +49,15 @@ static inline void mdelay(unsigned long ms)
>  	udelay(ms * 1000);
>  }
>  
> +static inline void cpu_timer_set_ms(int64_t timeout_ms)
> +{
> +	int64_t timer_value = (timeout_ms * 1000) << CPU_TIMER_SHIFT_US;
> +
> +	asm volatile (
> +		"spt %[timer_value]\n"
> +		:
> +		: [timer_value] "Q" (timer_value)
> +	);
> +}
> +
>  #endif

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Claudio Imbrenda]

On Fri, 22 Jul 2022 08:00:42 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> The CPU timer interrupt stays pending as long as the CPU timer value is
> negative. This can lead to interruption loops when the ext_new_psw mask
> has external interrupts enabled.
> 
> QEMU is able to detect this situation and panic the guest, so add a test
> for it.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  s390x/Makefile            |  1 +
>  s390x/panic-loop-extint.c | 60 +++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg       |  6 ++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 s390x/panic-loop-extint.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c13102..e4649da50d9d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -34,6 +34,7 @@ tests += $(TEST_DIR)/migration.elf
>  tests += $(TEST_DIR)/pv-attest.elf
>  tests += $(TEST_DIR)/migration-cmm.elf
>  tests += $(TEST_DIR)/migration-skey.elf
> +tests += $(TEST_DIR)/panic-loop-extint.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/panic-loop-extint.c b/s390x/panic-loop-extint.c
> new file mode 100644
> index 000000000000..d3a3f06d9a34
> --- /dev/null
> +++ b/s390x/panic-loop-extint.c
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * External interrupt loop test
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include <asm/interrupt.h>
> +#include <asm/barrier.h>
> +#include <asm/time.h>
> +#include <hardware.h>
> +
> +static void ext_int_handler(void)
> +{
> +	/*
> +	 * return to ext_old_psw. This gives us the chance to print the return_fail
> +	 * in case something goes wrong.
> +	 */
> +	asm volatile (
> +		"lpswe %[ext_old_psw]\n"
> +		:
> +		: [ext_old_psw] "Q"(lowcore.ext_old_psw)
> +		: "memory"
> +	);
> +}

why should ext_old_psw contain a good PSW? wouldn't it contain the
PSW at the time of the interrupt? (which in this case is the new PSW)

but this should never happen anyway, right?

> +
> +int main(void)
> +{
> +	struct psw ext_new_psw_orig;
> +
> +	report_prefix_push("panic-loop-extint");
> +
> +	if (!host_is_qemu() || host_is_tcg()) {
> +		report_skip("QEMU-KVM-only test");
> +		goto out;
> +	}
> +
> +	ext_new_psw_orig = lowcore.ext_new_psw;
> +	lowcore.ext_new_psw.addr = (uint64_t)ext_int_handler;
> +	lowcore.ext_new_psw.mask |= PSW_MASK_EXT;
> +
> +	load_psw_mask(extract_psw_mask() | PSW_MASK_EXT);
> +	ctl_set_bit(0, CTL0_CLOCK_COMPARATOR);
> +
> +	cpu_timer_set_ms(1);
> +
> +	mdelay(2000);
> +
> +	/* restore previous ext_new_psw so QEMU can properly terminate */
> +	lowcore.ext_new_psw = ext_new_psw_orig;
> +
> +	report_fail("survived extint loop");
> +
> +out:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index f7b1fc3dbca1..b1b25f118ff6 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -185,3 +185,9 @@ groups = migration
>  [migration-skey]
>  file = migration-skey.elf
>  groups = migration
> +
> +[panic-loop-extint]
> +file = panic-loop-extint.elf
> +groups = panic
> +accel = kvm
> +timeout = 5

Re: [kvm-unit-tests PATCH v3 4/4] s390x: add pgm spec interrupt loop test

[Claudio Imbrenda]

On Fri, 22 Jul 2022 08:00:43 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> An invalid PSW causes a program interrupt. When an invalid PSW is
> introduced in the pgm_new_psw, an interrupt loop occurs as soon as a
> program interrupt is caused.
> 
> QEMU should detect that and panic the guest, hence add a test for it.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile         |  1 +
>  s390x/panic-loop-pgm.c | 53 ++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg    |  6 +++++
>  3 files changed, 60 insertions(+)
>  create mode 100644 s390x/panic-loop-pgm.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index e4649da50d9d..66415d0b588d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -35,6 +35,7 @@ tests += $(TEST_DIR)/pv-attest.elf
>  tests += $(TEST_DIR)/migration-cmm.elf
>  tests += $(TEST_DIR)/migration-skey.elf
>  tests += $(TEST_DIR)/panic-loop-extint.elf
> +tests += $(TEST_DIR)/panic-loop-pgm.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/panic-loop-pgm.c b/s390x/panic-loop-pgm.c
> new file mode 100644
> index 000000000000..68934057a251
> --- /dev/null
> +++ b/s390x/panic-loop-pgm.c
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Program interrupt loop test
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include <bitops.h>
> +#include <asm/interrupt.h>
> +#include <asm/barrier.h>
> +#include <hardware.h>
> +
> +static void pgm_int_handler(void)
> +{
> +	/*
> +	 * return to pgm_old_psw. This gives us the chance to print the return_fail
> +	 * in case something goes wrong.
> +	 */
> +	asm volatile (
> +		"lpswe %[pgm_old_psw]\n"
> +		:
> +		: [pgm_old_psw] "Q"(lowcore.pgm_old_psw)
> +		: "memory"
> +	);
> +}

same consideration here as in the previous patch

> +
> +int main(void)
> +{
> +	report_prefix_push("panic-loop-pgm");
> +
> +	if (!host_is_qemu() || host_is_tcg()) {
> +		report_skip("QEMU-KVM-only test");
> +		goto out;
> +	}
> +
> +	lowcore.pgm_new_psw.addr = (uint64_t) pgm_int_handler;
> +	/* bit 12 set is invalid */
> +	lowcore.pgm_new_psw.mask = extract_psw_mask() | BIT(63 - 12);
> +	mb();
> +
> +	/* cause a pgm int */
> +	*((int *)-4) = 0x42;
> +	mb();
> +
> +	report_fail("survived pgmint loop");
> +
> +out:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b1b25f118ff6..f9f102abfa89 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -191,3 +191,9 @@ file = panic-loop-extint.elf
>  groups = panic
>  accel = kvm
>  timeout = 5
> +
> +[panic-loop-pgm]
> +file = panic-loop-pgm.elf
> +groups = panic
> +accel = kvm
> +timeout = 5

Re: [kvm-unit-tests PATCH v3 1/4] runtime: add support for panic tests

[Nico Boehr]

Quoting Claudio Imbrenda (2022-08-10 11:58:08)
[...]
> > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> > index 0dfaf017db0a..739490bc7da2 100644
> > --- a/scripts/arch-run.bash
> > +++ b/scripts/arch-run.bash
> > @@ -104,6 +104,14 @@ qmp ()
> >       echo '{ "execute": "qmp_capabilities" }{ "execute":' "$2" '}' | ncat -U $1
> >  }
> >  
> > +qmp_events ()
> > +{
> > +     while ! test -S "$1"; do sleep 0.1; done
> > +     echo '{ "execute": "qmp_capabilities" }{ "execute": "cont" }' \
> 
> if you put the pipe at the end of the line, instead of the beginning,
> then you don't need the \ . I think it is easier to read without the \
> and it is also more robust (no need to worry about spaces)

Makes sense, changed.

> > +run_panic ()
> > +{
> > +     if ! command -v ncat >/dev/null 2>&1; then
> > +             echo "${FUNCNAME[0]} needs ncat (netcat)" >&2
> > +             return 77
> > +     fi
> > +
> > +     if ! command -v jq >/dev/null 2>&1; then
> > +             echo "${FUNCNAME[0]} needs jq" >&2
> > +             return 77
> > +     fi
> > +
> > +     qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX)
> > +
> > +     trap 'kill 0; exit 2' INT TERM
> > +     trap 'rm -f ${qmp}' RETURN EXIT
> > +
> > +     # start VM stopped so we don't miss any events
> > +     eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \
> > +             -mon chardev=mon1,mode=control -S &
> > +
> > +     panic_event_count=$(qmp_events ${qmp} | jq -c 'select(.event == "GUEST_PANICKED")' | wc -l)
> > +     if [ "$panic_event_count" -lt 1 ]; then
> > +             echo "FAIL: guest did not panic"
> > +             ret=3
> > +     else
> > +             # some QEMU versions report multiple panic events
> > +             echo "PASS: guest panicked"
> > +             ret=1
> 
> so we never return 0? is that intentional?

Yes, as far as I understand things, this is correct:

run_panic's status code is (in the end) fed to run_qemu or run_qemu_status. These two functions rewrite the QEMU status code to determine whether tests succeeded.

Before the rewrite, return 3 means unit test failed; return 1 means unit test succeeded. So I *think* this is appropriate as-is.

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Nico Boehr]

Quoting Claudio Imbrenda (2022-08-10 12:08:22)
> > diff --git a/s390x/panic-loop-extint.c b/s390x/panic-loop-extint.c
> > new file mode 100644
> > index 000000000000..d3a3f06d9a34
> > --- /dev/null
> > +++ b/s390x/panic-loop-extint.c
[...]
> > +static void ext_int_handler(void)
> > +{
> > +     /*
> > +      * return to ext_old_psw. This gives us the chance to print the return_fail
> > +      * in case something goes wrong.
> > +      */
> > +     asm volatile (
> > +             "lpswe %[ext_old_psw]\n"
> > +             :
> > +             : [ext_old_psw] "Q"(lowcore.ext_old_psw)
> > +             : "memory"
> > +     );
> > +}
> 
> why should ext_old_psw contain a good PSW? wouldn't it contain the
> PSW at the time of the interrupt? (which in this case is the new PSW)

That's right in case the interrupt loop occurs, it doesn't make much sense to return to ext_old_psw. But in this case lpswe will never be executed anyways.

> but this should never happen anyway, right?

Exactly, this is just supposed to be a safety net in case the interrupt loop doesn't happen. If you want, we could remove it and just leave an empty function here. Then, we will just run into the kvm-unit-tests timeout and fail as well, but I prefer the fail fast.

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Claudio Imbrenda]

On Wed, 10 Aug 2022 14:29:27 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-08-10 12:08:22)
> > > diff --git a/s390x/panic-loop-extint.c b/s390x/panic-loop-extint.c
> > > new file mode 100644
> > > index 000000000000..d3a3f06d9a34
> > > --- /dev/null
> > > +++ b/s390x/panic-loop-extint.c  
> [...]
> > > +static void ext_int_handler(void)
> > > +{
> > > +     /*
> > > +      * return to ext_old_psw. This gives us the chance to print the return_fail
> > > +      * in case something goes wrong.
> > > +      */
> > > +     asm volatile (
> > > +             "lpswe %[ext_old_psw]\n"
> > > +             :
> > > +             : [ext_old_psw] "Q"(lowcore.ext_old_psw)
> > > +             : "memory"
> > > +     );
> > > +}  
> > 
> > why should ext_old_psw contain a good PSW? wouldn't it contain the
> > PSW at the time of the interrupt? (which in this case is the new PSW)  
> 
> That's right in case the interrupt loop occurs, it doesn't make much sense to return to ext_old_psw. But in this case lpswe will never be executed anyways.
> 
> > but this should never happen anyway, right?  
> 
> Exactly, this is just supposed to be a safety net in case the interrupt loop doesn't happen. If you want, we could remove it and just leave an empty function here. Then, we will just run into the kvm-unit-tests timeout and fail as well, but I prefer the fail fast.

just add a comment to explain that

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Nico Boehr]

Quoting Claudio Imbrenda (2022-08-10 15:13:02)
[...]
> just add a comment to explain that

Yes makes sense, thanks. I came up with this:

/*
 * When the interrupt loop occurs, this instruction will never be
 * executed.
 * In case the loop isn't triggered return to pgm_old_psw so we can fail
 * fast and don't have to rely on the kvm-unit-tests timeout.
 */

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Claudio Imbrenda]

On Thu, 11 Aug 2022 09:54:37 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-08-10 15:13:02)
> [...]
> > just add a comment to explain that  
> 
> Yes makes sense, thanks. I came up with this:
> 
> /*
>  * When the interrupt loop occurs, this instruction will never be
>  * executed.
>  * In case the loop isn't triggered return to pgm_old_psw so we can fail
>  * fast and don't have to rely on the kvm-unit-tests timeout.
>  */

lgtm

Re: [kvm-unit-tests PATCH v3 3/4] s390x: add extint loop test

[Nico Boehr]

Quoting Claudio Imbrenda (2022-08-10 12:08:22)
[...]
> > diff --git a/s390x/panic-loop-extint.c b/s390x/panic-loop-extint.c
> > new file mode 100644
> > index 000000000000..d3a3f06d9a34
> > --- /dev/null
> > +++ b/s390x/panic-loop-extint.c
[...]
> > +static void ext_int_handler(void)
> > +{
> > +     /*
> > +      * return to ext_old_psw. This gives us the chance to print the return_fail
> > +      * in case something goes wrong.
> > +      */
> > +     asm volatile (
> > +             "lpswe %[ext_old_psw]\n"
> > +             :
> > +             : [ext_old_psw] "Q"(lowcore.ext_old_psw)
> > +             : "memory"
> > +     );
> > +}
> 
> why should ext_old_psw contain a good PSW? wouldn't it contain the
> PSW at the time of the interrupt? (which in this case is the new PSW)
> 
> but this should never happen anyway, right?

Well, after your remark, I thought about this a little more and found several issues with my implementation:

- I enabled the clock comparator subclass mask, but set the CPU timer. The test also works with the clock comparator since it also stays pending. Doesn't really matter which one to use as long as you stay consistent. :-)
- returning to ext_old_psw is not enough, since the CPU timer subclass mask is still enabled and the CPU timer is negative. This means the CPU timer will fire once we enable external interruptions and hence impede printing the report_fail()
- the whole lpswe is redundant, since the default kvm-unit-test handler already does that. I will refactor this to leave the default kut handler in place, which is much nicer and safer.

So upcoming version will refactor this test a bit and rely on the default kvm-unit-test int handler and the nice new register_ext_cleanup_func().