[kvm-unit-tests PATCH v3 0/1] Test spec exception interception

[kvm-unit-tests PATCH v3 0/1] Test spec exception interception

[Janis Schoetterl-Glausch]

When specification exception interpretation is enabled, specification
exceptions need not result in interceptions.
However, if the exception is due to an invalid program new PSW,
interception must occur.
Test this.
Also test that interpretation does occur if interpretation is disabled.

v2 -> v3
	drop patch converting assert to report
	drop non-ascii symbol
	use snippet constants

v1 -> v2
	Add license and test description
	Use lowcore pointer instead of magic value for program new PSW
		-> need to get rid of assert in arch_def.h
	Do not use odd program new PSW, even if irrelevant
	Use SIE lib
	Reword messages
	Fix nits

Janis Schoetterl-Glausch (1):
  s390x: Add specification exception interception test

 s390x/Makefile             |  2 +
 lib/s390x/sie.h            |  1 +
 s390x/snippets/c/spec_ex.c | 21 ++++++++++
 s390x/spec_ex-sie.c        | 82 ++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg        |  3 ++
 5 files changed, 109 insertions(+)
 create mode 100644 s390x/snippets/c/spec_ex.c
 create mode 100644 s390x/spec_ex-sie.c

Range-diff against v2:
1:  f8abbae ! 1:  319eb08 s390x: Add specification exception interception test
    @@ s390x/Makefile: tests += $(TEST_DIR)/mvpg.elf
      
      tests_binary = $(patsubst %.elf,%.bin,$(tests))
      ifneq ($(HOST_KEY_DOCUMENT),)
    -@@ s390x/Makefile: snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o
    +@@ s390x/Makefile: snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o lib/auxinfo.o
      # perquisites (=guests) for the snippet hosts.
      # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
      $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
    @@ s390x/snippets/c/spec_ex.c (new)
     @@
     +// SPDX-License-Identifier: GPL-2.0-only
     +/*
    -+ * <utf-8 (C) symbol> Copyright IBM Corp. 2021
    ++ * Copyright IBM Corp. 2021
     + *
     + * Snippet used by specification exception interception test.
     + */
    -+#include <stdint.h>
    ++#include <libcflat.h>
    ++#include <bitops.h>
     +#include <asm/arch_def.h>
     +
     +__attribute__((section(".text"))) int main(void)
    @@ s390x/snippets/c/spec_ex.c (new)
     +	uint64_t bad_psw = 0;
     +
     +	/* PSW bit 12 has no name or meaning and must be 0 */
    -+	lowcore->pgm_new_psw.mask = 1UL << (63 - 12);
    ++	lowcore->pgm_new_psw.mask = BIT(63 - 12);
     +	lowcore->pgm_new_psw.addr = 0xdeadbeee;
     +	asm volatile ("lpsw %0" :: "Q"(bad_psw));
     +	return 0;
    @@ s390x/spec_ex-sie.c (new)
     @@
     +// SPDX-License-Identifier: GPL-2.0-only
     +/*
    -+ * <utf-8 (C) symbol> Copyright IBM Corp. 2021
    ++ * Copyright IBM Corp. 2021
     + *
     + * Specification exception interception test.
     + * Checks that specification exception interceptions occur as expected when
    @@ s390x/spec_ex-sie.c (new)
     +#include <alloc_page.h>
     +#include <vm.h>
     +#include <sie.h>
    ++#include <snippet.h>
     +
     +static struct vm vm;
    -+extern const char _binary_s390x_snippets_c_spec_ex_gbin_start[];
    -+extern const char _binary_s390x_snippets_c_spec_ex_gbin_end[];
    ++extern const char SNIPPET_NAME_START(c, spec_ex)[];
    ++extern const char SNIPPET_NAME_END(c, spec_ex)[];
     +
     +static void setup_guest(void)
     +{
     +	char *guest;
    -+	int binary_size = ((uintptr_t)_binary_s390x_snippets_c_spec_ex_gbin_end -
    -+			   (uintptr_t)_binary_s390x_snippets_c_spec_ex_gbin_start);
    ++	int binary_size = SNIPPET_LEN(c, spec_ex);
     +
     +	setup_vm();
     +	guest = alloc_pages(8);
    -+	memcpy(guest, _binary_s390x_snippets_c_spec_ex_gbin_start, binary_size);
    ++	memcpy(guest, SNIPPET_NAME_START(c, spec_ex), binary_size);
     +	sie_guest_create(&vm, (uint64_t) guest, HPAGE_SIZE);
     +}
     +
     +static void reset_guest(void)
     +{
    -+	vm.sblk->gpsw.addr = PAGE_SIZE * 4;
    -+	vm.sblk->gpsw.mask = PSW_MASK_64;
    ++	vm.sblk->gpsw = snippet_psw;
     +	vm.sblk->icptcode = 0;
     +}
     +

base-commit: 3ac97f8fc847d05d0a5555aefd34e2cac26fdc0c
-- 
2.31.1

[kvm-unit-tests PATCH v3 1/1] s390x: Add specification exception interception test

[Janis Schoetterl-Glausch]

Check that specification exceptions cause intercepts when
specification exception interpretation is off.
Check that specification exceptions caused by program new PSWs
cause interceptions.
We cannot assert that non program new PSW specification exceptions
are interpreted because whether interpretation occurs or not is
configuration dependent.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@de.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 s390x/Makefile             |  2 +
 lib/s390x/sie.h            |  1 +
 s390x/snippets/c/spec_ex.c | 21 ++++++++++
 s390x/spec_ex-sie.c        | 82 ++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg        |  3 ++
 5 files changed, 109 insertions(+)
 create mode 100644 s390x/snippets/c/spec_ex.c
 create mode 100644 s390x/spec_ex-sie.c

diff --git a/s390x/Makefile b/s390x/Makefile
index d18b08b..f95f2e6 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
 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_binary = $(patsubst %.elf,%.bin,$(tests))
 ifneq ($(HOST_KEY_DOCUMENT),)
@@ -86,6 +87,7 @@ snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o lib/auxinfo.o
 # perquisites (=guests) for the snippet hosts.
 # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
 $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
+$(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin
 
 $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
 	$(OBJCOPY) -O binary $(patsubst %.gbin,%.o,$@) $@
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index ca514ef..7ef7251 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -98,6 +98,7 @@ struct kvm_s390_sie_block {
 	uint8_t		fpf;			/* 0x0060 */
 #define ECB_GS		0x40
 #define ECB_TE		0x10
+#define ECB_SPECI	0x08
 #define ECB_SRSI	0x04
 #define ECB_HOSTPROTINT	0x02
 	uint8_t		ecb;			/* 0x0061 */
diff --git a/s390x/snippets/c/spec_ex.c b/s390x/snippets/c/spec_ex.c
new file mode 100644
index 0000000..71655dd
--- /dev/null
+++ b/s390x/snippets/c/spec_ex.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright IBM Corp. 2021
+ *
+ * Snippet used by specification exception interception test.
+ */
+#include <libcflat.h>
+#include <bitops.h>
+#include <asm/arch_def.h>
+
+__attribute__((section(".text"))) int main(void)
+{
+	struct lowcore *lowcore = (struct lowcore *) 0;
+	uint64_t bad_psw = 0;
+
+	/* PSW bit 12 has no name or meaning and must be 0 */
+	lowcore->pgm_new_psw.mask = BIT(63 - 12);
+	lowcore->pgm_new_psw.addr = 0xdeadbeee;
+	asm volatile ("lpsw %0" :: "Q"(bad_psw));
+	return 0;
+}
diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
new file mode 100644
index 0000000..5dea411
--- /dev/null
+++ b/s390x/spec_ex-sie.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright IBM Corp. 2021
+ *
+ * Specification exception interception test.
+ * Checks that specification exception interceptions occur as expected when
+ * specification exception interpretation is off/on.
+ */
+#include <libcflat.h>
+#include <sclp.h>
+#include <asm/page.h>
+#include <asm/arch_def.h>
+#include <alloc_page.h>
+#include <vm.h>
+#include <sie.h>
+#include <snippet.h>
+
+static struct vm vm;
+extern const char SNIPPET_NAME_START(c, spec_ex)[];
+extern const char SNIPPET_NAME_END(c, spec_ex)[];
+
+static void setup_guest(void)
+{
+	char *guest;
+	int binary_size = SNIPPET_LEN(c, spec_ex);
+
+	setup_vm();
+	guest = alloc_pages(8);
+	memcpy(guest, SNIPPET_NAME_START(c, spec_ex), binary_size);
+	sie_guest_create(&vm, (uint64_t) guest, HPAGE_SIZE);
+}
+
+static void reset_guest(void)
+{
+	vm.sblk->gpsw = snippet_psw;
+	vm.sblk->icptcode = 0;
+}
+
+static void test_spec_ex_sie(void)
+{
+	setup_guest();
+
+	report_prefix_push("SIE spec ex interpretation");
+	report_prefix_push("off");
+	reset_guest();
+	sie(&vm);
+	/* interpretation off -> initial exception must cause interception */
+	report(vm.sblk->icptcode == ICPT_PROGI
+	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION
+	       && vm.sblk->gpsw.addr != 0xdeadbeee,
+	       "Received specification exception intercept for initial exception");
+	report_prefix_pop();
+
+	report_prefix_push("on");
+	vm.sblk->ecb |= ECB_SPECI;
+	reset_guest();
+	sie(&vm);
+	/* interpretation on -> configuration dependent if initial exception causes
+	 * interception, but invalid new program PSW must
+	 */
+	report(vm.sblk->icptcode == ICPT_PROGI
+	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
+	       "Received specification exception intercept");
+	if (vm.sblk->gpsw.addr == 0xdeadbeee)
+		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
+	else
+		report_info("Did not interpret initial exception");
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
+int main(int argc, char **argv)
+{
+	if (!sclp_facilities.has_sief2) {
+		report_skip("SIEF2 facility unavailable");
+		goto out;
+	}
+
+	test_spec_ex_sie();
+out:
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 9e1802f..3b454b7 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -109,3 +109,6 @@ file = edat.elf
 
 [mvpg-sie]
 file = mvpg-sie.elf
+
+[spec_ex-sie]
+file = spec_ex-sie.elf
-- 
2.31.1

Re: [kvm-unit-tests PATCH v3 1/1] s390x: Add specification exception interception test

[Thomas Huth]

On 22/10/2021 15.10, Janis Schoetterl-Glausch wrote:
> Check that specification exceptions cause intercepts when
> specification exception interpretation is off.
> Check that specification exceptions caused by program new PSWs
> cause interceptions.
> We cannot assert that non program new PSW specification exceptions
> are interpreted because whether interpretation occurs or not is
> configuration dependent.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@de.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   s390x/Makefile             |  2 +
>   lib/s390x/sie.h            |  1 +
>   s390x/snippets/c/spec_ex.c | 21 ++++++++++
>   s390x/spec_ex-sie.c        | 82 ++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg        |  3 ++
>   5 files changed, 109 insertions(+)
>   create mode 100644 s390x/snippets/c/spec_ex.c
>   create mode 100644 s390x/spec_ex-sie.c

Thanks, I've pushed it to the repository now.

  Thomas

Re: [kvm-unit-tests PATCH v3 1/1] s390x: Add specification exception interception test

[Thomas Huth]

On 22/10/2021 15.10, Janis Schoetterl-Glausch wrote:
> Check that specification exceptions cause intercepts when
> specification exception interpretation is off.
> Check that specification exceptions caused by program new PSWs
> cause interceptions.
> We cannot assert that non program new PSW specification exceptions
> are interpreted because whether interpretation occurs or not is
> configuration dependent.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Reviewed-by: Janosch Frank <frankja@de.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
...
> +	report_prefix_push("on");
> +	vm.sblk->ecb |= ECB_SPECI;
> +	reset_guest();
> +	sie(&vm);
> +	/* interpretation on -> configuration dependent if initial exception causes
> +	 * interception, but invalid new program PSW must
> +	 */
> +	report(vm.sblk->icptcode == ICPT_PROGI
> +	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
> +	       "Received specification exception intercept");
> +	if (vm.sblk->gpsw.addr == 0xdeadbeee)
> +		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
> +	else
> +		report_info("Did not interpret initial exception");

  Hi Janis!

While using this test in our downstream verification of the backport of the 
related kernel patch, it occurred that the way of only reporting the 
interpreted exception via report_info() is rather unfortunate for using this 
test in automatic regression runs. For such regression runs, it would be 
good if the test would be marked with FAIL if the exception was not 
interpreted. I know, the interpretation facility is not always there, but 
still would it be somehow possible to add such a mode? E.g. by checking the 
machine generation (is this always available with z15 and newer?) and maybe 
adding a CLI option to force the hard check (so that e.g. "-f" triggers the 
failure if the exception has not been interpreted, while running the test 
without "-f" would still do the old behavior instead)?

  Thomas

Re: [kvm-unit-tests PATCH v3 1/1] s390x: Add specification exception interception test

[Janis Schoetterl-Glausch]

On 11/11/21 08:47, Thomas Huth wrote:
> On 22/10/2021 15.10, Janis Schoetterl-Glausch wrote:
>> Check that specification exceptions cause intercepts when
>> specification exception interpretation is off.
>> Check that specification exceptions caused by program new PSWs
>> cause interceptions.
>> We cannot assert that non program new PSW specification exceptions
>> are interpreted because whether interpretation occurs or not is
>> configuration dependent.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@de.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
> ...
>> +    report_prefix_push("on");
>> +    vm.sblk->ecb |= ECB_SPECI;
>> +    reset_guest();
>> +    sie(&vm);
>> +    /* interpretation on -> configuration dependent if initial exception causes
>> +     * interception, but invalid new program PSW must
>> +     */
>> +    report(vm.sblk->icptcode == ICPT_PROGI
>> +           && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
>> +           "Received specification exception intercept");
>> +    if (vm.sblk->gpsw.addr == 0xdeadbeee)
>> +        report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
>> +    else
>> +        report_info("Did not interpret initial exception");
> 
>  Hi Janis!
> 
> While using this test in our downstream verification of the backport of the related kernel patch, it occurred that the way of only reporting the interpreted exception via report_info() is rather unfortunate for using this test in automatic regression runs. For such regression runs, it would be good if the test would be marked with FAIL if the exception was not interpreted. I know, the interpretation facility is not always there, but still would it be somehow possible to add such a mode? E.g. by checking the machine generation (is this always available with z15 and newer?) and maybe adding a CLI option to force the hard check (so that e.g. "-f" triggers the failure if the exception has not been interpreted, while running the test without "-f" would still do the old behavior instead)?
> 
>  Thomas
> 
Sounds good, I'll look into it.

[kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Janis Schoetterl-Glausch]

While specification exception interpretation is not required to occur,
it can be useful for automatic regression testing to fail the test if it
does not occur.
Add a `--strict` argument to enable this.
`--strict` takes a list of machine types (as reported by STIDP)
for which to enable strict mode, for example
`--strict 8562,8561,3907,3906,2965,2964`
will enable it for models z15 - z13.
Alternatively, strict mode can be enabled for all but the listed machine
types by prefixing the list with a `!`, for example
`--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
will enable it for z/Architecture models except those older than z13.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---

Apparently my message with inline patch did not make it to the mailing
list for some reason, so here's the patch again.

 s390x/spec_ex-sie.c | 59 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 6 deletions(-)

diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
index 5dea411..9a063f9 100644
--- a/s390x/spec_ex-sie.c
+++ b/s390x/spec_ex-sie.c
@@ -7,6 +7,7 @@
  * specification exception interpretation is off/on.
  */
 #include <libcflat.h>
+#include <stdlib.h>
 #include <sclp.h>
 #include <asm/page.h>
 #include <asm/arch_def.h>
@@ -36,7 +37,7 @@ static void reset_guest(void)
 	vm.sblk->icptcode = 0;
 }
 
-static void test_spec_ex_sie(void)
+static void test_spec_ex_sie(bool strict)
 {
 	setup_guest();
 
@@ -61,14 +62,60 @@ static void test_spec_ex_sie(void)
 	report(vm.sblk->icptcode == ICPT_PROGI
 	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
 	       "Received specification exception intercept");
-	if (vm.sblk->gpsw.addr == 0xdeadbeee)
-		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
-	else
-		report_info("Did not interpret initial exception");
+	{
+		const char *msg;
+
+		msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
+		if (strict)
+			report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
+		else if (vm.sblk->gpsw.addr == 0xdeadbeee)
+			report_info(msg);
+		else
+			report_info("Did not interpret initial exception");
+	}
 	report_prefix_pop();
 	report_prefix_pop();
 }
 
+static bool parse_strict(char **argv)
+{
+	uint16_t machine_id;
+	char *list;
+	bool ret;
+
+	if (!*argv)
+		return false;
+	if (strcmp("--strict", *argv))
+		return false;
+
+	machine_id = get_machine_id();
+	list = argv[1];
+	if (!list) {
+		printf("No argument to --strict, ignoring\n");
+		return false;
+	}
+	if (list[0] == '!') {
+		ret = true;
+		list++;
+	} else
+		ret = false;
+	while (true) {
+		long input = 0;
+
+		if (strlen(list) == 0)
+			return ret;
+		input = strtol(list, &list, 16);
+		if (*list == ',')
+			list++;
+		else if (*list != '\0')
+			break;
+		if (input == machine_id)
+			return !ret;
+	}
+	printf("Invalid --strict argument \"%s\", ignoring\n", list);
+	return ret;
+}
+
 int main(int argc, char **argv)
 {
 	if (!sclp_facilities.has_sief2) {
@@ -76,7 +123,7 @@ int main(int argc, char **argv)
 		goto out;
 	}
 
-	test_spec_ex_sie();
+	test_spec_ex_sie(parse_strict(argv + 1));
 out:
 	return report_summary();
 }
-- 
2.31.1

Re: [kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Thomas Huth]

On 25/11/2021 15.47, Janis Schoetterl-Glausch wrote:
> While specification exception interpretation is not required to occur,
> it can be useful for automatic regression testing to fail the test if it
> does not occur.
> Add a `--strict` argument to enable this.

Thank you very much for adding this!

Some comments below...

> `--strict` takes a list of machine types (as reported by STIDP)
> for which to enable strict mode, for example
> `--strict 8562,8561,3907,3906,2965,2964`
> will enable it for models z15 - z13.
> Alternatively, strict mode can be enabled for all but the listed machine
> types by prefixing the list with a `!`, for example
> `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
> will enable it for z/Architecture models except those older than z13.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> Apparently my message with inline patch did not make it to the mailing
> list for some reason, so here's the patch again.
> 
>   s390x/spec_ex-sie.c | 59 ++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
> index 5dea411..9a063f9 100644
> --- a/s390x/spec_ex-sie.c
> +++ b/s390x/spec_ex-sie.c
> @@ -7,6 +7,7 @@
>    * specification exception interpretation is off/on.
>    */
>   #include <libcflat.h>
> +#include <stdlib.h>
>   #include <sclp.h>
>   #include <asm/page.h>
>   #include <asm/arch_def.h>
> @@ -36,7 +37,7 @@ static void reset_guest(void)
>   	vm.sblk->icptcode = 0;
>   }
>   
> -static void test_spec_ex_sie(void)
> +static void test_spec_ex_sie(bool strict)
>   {
>   	setup_guest();
>   
> @@ -61,14 +62,60 @@ static void test_spec_ex_sie(void)
>   	report(vm.sblk->icptcode == ICPT_PROGI
>   	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
>   	       "Received specification exception intercept");
> -	if (vm.sblk->gpsw.addr == 0xdeadbeee)
> -		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
> -	else
> -		report_info("Did not interpret initial exception");
> +	{
> +		const char *msg;

Could you please move the variable declaration to the beginning of the 
function? Then you could get rid of the curly brackets and one level of 
indentation.

> +		msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
> +		if (strict)
> +			report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
> +		else if (vm.sblk->gpsw.addr == 0xdeadbeee)
> +			report_info(msg);
> +		else
> +			report_info("Did not interpret initial exception");
> +	}
>   	report_prefix_pop();
>   	report_prefix_pop();
>   }
>   
> +static bool parse_strict(char **argv)
> +{
> +	uint16_t machine_id;
> +	char *list;
> +	bool ret;
> +
> +	if (!*argv)
> +		return false;

I think this works ok with out current implementation of argv, but that's an 
"inofficial" implementation detail, so in case this ever gets changed, it 
might be better to check argc first before dereferencing argv here ... so 
could you please add a check for argc, too?

> +	if (strcmp("--strict", *argv))
> +		return false;
> +
> +	machine_id = get_machine_id();
> +	list = argv[1];
> +	if (!list) {
> +		printf("No argument to --strict, ignoring\n");
> +		return false;

You could also support --strict without arguments - that could turn on the 
strict mode unconditionally, I think.

> +	}
> +	if (list[0] == '!') {
> +		ret = true;
> +		list++;
> +	} else
> +		ret = false;
> +	while (true) {
> +		long input = 0;
> +
> +		if (strlen(list) == 0)
> +			return ret;
> +		input = strtol(list, &list, 16);
> +		if (*list == ',')
> +			list++;
> +		else if (*list != '\0')
> +			break;
> +		if (input == machine_id)
> +			return !ret;
> +	}
> +	printf("Invalid --strict argument \"%s\", ignoring\n", list);
> +	return ret;
> +}
> +
>   int main(int argc, char **argv)
>   {
>   	if (!sclp_facilities.has_sief2) {
> @@ -76,7 +123,7 @@ int main(int argc, char **argv)
>   		goto out;
>   	}
>   
> -	test_spec_ex_sie();
> +	test_spec_ex_sie(parse_strict(argv + 1));
>   out:
>   	return report_summary();
>   }
> 

  Thomas

Re: [kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Janis Schoetterl-Glausch]

On 12/3/21 12:15, Thomas Huth wrote:
> On 25/11/2021 15.47, Janis Schoetterl-Glausch wrote:
>> While specification exception interpretation is not required to occur,
>> it can be useful for automatic regression testing to fail the test if it
>> does not occur.
>> Add a `--strict` argument to enable this.
> 
> Thank you very much for adding this!

Sure :)
> 
> Some comments below...
> 
>> `--strict` takes a list of machine types (as reported by STIDP)
>> for which to enable strict mode, for example
>> `--strict 8562,8561,3907,3906,2965,2964`
>> will enable it for models z15 - z13.
>> Alternatively, strict mode can be enabled for all but the listed machine
>> types by prefixing the list with a `!`, for example
>> `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
>> will enable it for z/Architecture models except those older than z13.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>
>> Apparently my message with inline patch did not make it to the mailing
>> list for some reason, so here's the patch again.
>>
>>   s390x/spec_ex-sie.c | 59 ++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 53 insertions(+), 6 deletions(-)
>>
>> diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
>> index 5dea411..9a063f9 100644
>> --- a/s390x/spec_ex-sie.c
>> +++ b/s390x/spec_ex-sie.c
>> @@ -7,6 +7,7 @@
>>    * specification exception interpretation is off/on.
>>    */
>>   #include <libcflat.h>
>> +#include <stdlib.h>
>>   #include <sclp.h>
>>   #include <asm/page.h>
>>   #include <asm/arch_def.h>
>> @@ -36,7 +37,7 @@ static void reset_guest(void)
>>       vm.sblk->icptcode = 0;
>>   }
>>   -static void test_spec_ex_sie(void)
>> +static void test_spec_ex_sie(bool strict)
>>   {
>>       setup_guest();
>>   @@ -61,14 +62,60 @@ static void test_spec_ex_sie(void)
>>       report(vm.sblk->icptcode == ICPT_PROGI
>>              && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
>>              "Received specification exception intercept");
>> -    if (vm.sblk->gpsw.addr == 0xdeadbeee)
>> -        report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
>> -    else
>> -        report_info("Did not interpret initial exception");
>> +    {
>> +        const char *msg;
> 
> Could you please move the variable declaration to the beginning of the function? Then you could get rid of the curly brackets and one level of indentation.

Yes.
> 
>> +        msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
>> +        if (strict)
>> +            report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
>> +        else if (vm.sblk->gpsw.addr == 0xdeadbeee)
>> +            report_info(msg);
>> +        else
>> +            report_info("Did not interpret initial exception");
>> +    }
>>       report_prefix_pop();
>>       report_prefix_pop();
>>   }
>>   +static bool parse_strict(char **argv)
>> +{
>> +    uint16_t machine_id;
>> +    char *list;
>> +    bool ret;
>> +
>> +    if (!*argv)
>> +        return false;
> 
> I think this works ok with out current implementation of argv, but that's an "inofficial" implementation detail, so in case this ever gets changed, it might be better to check argc first before dereferencing argv here ... so could you please add a check for argc, too?

This is required by POSIX, isn't it? But then whether or not we comply with it is another question.
> 
>> +    if (strcmp("--strict", *argv))
>> +        return false;
>> +
>> +    machine_id = get_machine_id();
>> +    list = argv[1];

This needs to be covered by the argc check too, then.

>> +    if (!list) {
>> +        printf("No argument to --strict, ignoring\n");
>> +        return false;
> 
> You could also support --strict without arguments - that could turn on the strict mode unconditionally, I think.

--strict ! works. Granted it's not the best UI, but it is more consistent,
you could do --strict $(get list of ids from somewhere) and if the list is empty the semantics stay the same. 
> 
>> +    }
>> +    if (list[0] == '!') {
>> +        ret = true;
>> +        list++;
>> +    } else
>> +        ret = false;
>> +    while (true) {
>> +        long input = 0;
>> +
>> +        if (strlen(list) == 0)
>> +            return ret;
>> +        input = strtol(list, &list, 16);
>> +        if (*list == ',')
>> +            list++;
>> +        else if (*list != '\0')
>> +            break;
>> +        if (input == machine_id)
>> +            return !ret;
>> +    }
>> +    printf("Invalid --strict argument \"%s\", ignoring\n", list);
>> +    return ret;
>> +}
>> +
>>   int main(int argc, char **argv)
>>   {
>>       if (!sclp_facilities.has_sief2) {
>> @@ -76,7 +123,7 @@ int main(int argc, char **argv)
>>           goto out;
>>       }
>>   -    test_spec_ex_sie();
>> +    test_spec_ex_sie(parse_strict(argv + 1));
>>   out:
>>       return report_summary();
>>   }
>>
> 
>  Thomas
> 

Re: [kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Janis Schoetterl-Glausch]

On 2/28/22 14:27, Claudio Imbrenda wrote:
> On Fri, 25 Feb 2022 18:23:55 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> While specification exception interpretation is not required to occur,
>> it can be useful for automatic regression testing to fail the test if it
>> does not occur.
>> Add a `--strict` argument to enable this.
>> `--strict` takes a list of machine types (as reported by STIDP)
>> for which to enable strict mode, for example
>> `--strict 8562,8561,3907,3906,2965,2964`
>> will enable it for models z15 - z13.
>> Alternatively, strict mode can be enabled for all but the listed machine
>> types by prefixing the list with a `!`, for example
>> `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
>> will enable it for z/Architecture models except those older than z13.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
> 
> [...]
> 
>> +static bool parse_strict(int argc, char **argv)
>> +{
>> +	uint16_t machine_id;
>> +	char *list;
>> +	bool ret;
>> +
>> +	if (argc < 1)
>> +		return false;
>> +	if (strcmp("--strict", argv[0]))
>> +		return false;
>> +
>> +	machine_id = get_machine_id();
>> +	if (argc < 2) {
>> +		printf("No argument to --strict, ignoring\n");
>> +		return false;
>> +	}
>> +	list = argv[1];
>> +	if (list[0] == '!') {
>> +		ret = true;
>> +		list++;
>> +	} else
>> +		ret = false;
>> +	while (true) {
>> +		long input = 0;
>> +
>> +		if (strlen(list) == 0)
>> +			return ret;
>> +		input = strtol(list, &list, 16);
>> +		if (*list == ',')
>> +			list++;
>> +		else if (*list != '\0')
>> +			break;
>> +		if (input == machine_id)
>> +			return !ret;
>> +	}
>> +	printf("Invalid --strict argument \"%s\", ignoring\n", list);
>> +	return ret;
>> +}
> 
> probably I should write a few parsing functions for command line
> arguments, so we don't have to re-invent the wheel every time

Maybe, would depend on what you have in mind, I'm not sure most
use cases can be covered by a reasonable set of abstractions.
> 
>> +
>>  int main(int argc, char **argv)
>>  {
>>  	if (!sclp_facilities.has_sief2) {
>> @@ -76,7 +121,7 @@ int main(int argc, char **argv)
>>  		goto out;
>>  	}
>>  
>> -	test_spec_ex_sie();
>> +	test_spec_ex_sie(parse_strict(argc - 1, argv + 1));
> 
> hmmm... maybe it would be more readable and more uniform with the other
> tests to parse the command line during initialization of the unit test,
> and set a global flag.

More uniform maybe, but I tend to dislike globals from a readability point
of view. I'm inclined to keep it as is.
> 
>>  out:
>>  	return report_summary();
>>  }
>>
>> base-commit: 257c962f3d1b2d0534af59de4ad18764d734903a
> 

Re: [kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Claudio Imbrenda]

On Fri, 25 Feb 2022 18:23:55 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> While specification exception interpretation is not required to occur,
> it can be useful for automatic regression testing to fail the test if it
> does not occur.
> Add a `--strict` argument to enable this.
> `--strict` takes a list of machine types (as reported by STIDP)
> for which to enable strict mode, for example
> `--strict 8562,8561,3907,3906,2965,2964`
> will enable it for models z15 - z13.
> Alternatively, strict mode can be enabled for all but the listed machine
> types by prefixing the list with a `!`, for example
> `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
> will enable it for z/Architecture models except those older than z13.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---

[...]

> +static bool parse_strict(int argc, char **argv)
> +{
> +	uint16_t machine_id;
> +	char *list;
> +	bool ret;
> +
> +	if (argc < 1)
> +		return false;
> +	if (strcmp("--strict", argv[0]))
> +		return false;
> +
> +	machine_id = get_machine_id();
> +	if (argc < 2) {
> +		printf("No argument to --strict, ignoring\n");
> +		return false;
> +	}
> +	list = argv[1];
> +	if (list[0] == '!') {
> +		ret = true;
> +		list++;
> +	} else
> +		ret = false;
> +	while (true) {
> +		long input = 0;
> +
> +		if (strlen(list) == 0)
> +			return ret;
> +		input = strtol(list, &list, 16);
> +		if (*list == ',')
> +			list++;
> +		else if (*list != '\0')
> +			break;
> +		if (input == machine_id)
> +			return !ret;
> +	}
> +	printf("Invalid --strict argument \"%s\", ignoring\n", list);
> +	return ret;
> +}

probably I should write a few parsing functions for command line
arguments, so we don't have to re-invent the wheel every time

> +
>  int main(int argc, char **argv)
>  {
>  	if (!sclp_facilities.has_sief2) {
> @@ -76,7 +121,7 @@ int main(int argc, char **argv)
>  		goto out;
>  	}
>  
> -	test_spec_ex_sie();
> +	test_spec_ex_sie(parse_strict(argc - 1, argv + 1));

hmmm... maybe it would be more readable and more uniform with the other
tests to parse the command line during initialization of the unit test,
and set a global flag.

>  out:
>  	return report_summary();
>  }
> 
> base-commit: 257c962f3d1b2d0534af59de4ad18764d734903a

[kvm-unit-tests PATCH] s390x: Add strict mode to specification exception interpretation test

[Janis Schoetterl-Glausch]

While specification exception interpretation is not required to occur,
it can be useful for automatic regression testing to fail the test if it
does not occur.
Add a `--strict` argument to enable this.
`--strict` takes a list of machine types (as reported by STIDP)
for which to enable strict mode, for example
`--strict 8562,8561,3907,3906,2965,2964`
will enable it for models z15 - z13.
Alternatively, strict mode can be enabled for all but the listed machine
types by prefixing the list with a `!`, for example
`--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
will enable it for z/Architecture models except those older than z13.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
Range-diff:
1:  5d91f693 < -:  -------- s390x: Add specification exception interception test
2:  950eafd7 ! 1:  e9c36970 s390x: Add strict mode to specification exception interpretation test
    @@ s390x/spec_ex-sie.c: static void reset_guest(void)
     -static void test_spec_ex_sie(void)
     +static void test_spec_ex_sie(bool strict)
      {
    ++	const char *msg;
    ++
      	setup_guest();
      
    + 	report_prefix_push("SIE spec ex interpretation");
     @@ s390x/spec_ex-sie.c: static void test_spec_ex_sie(void)
      	report(vm.sblk->icptcode == ICPT_PROGI
      	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
      	       "Received specification exception intercept");
     -	if (vm.sblk->gpsw.addr == 0xdeadbeee)
     -		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
    --	else
    --		report_info("Did not interpret initial exception");
    -+	{
    -+		const char *msg;
    -+
    -+		msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
    -+		if (strict)
    -+			report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
    -+		else if (vm.sblk->gpsw.addr == 0xdeadbeee)
    -+			report_info(msg);
    -+		else
    -+			report_info("Did not interpret initial exception");
    -+	}
    ++	msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
    ++	if (strict)
    ++		report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
    ++	else if (vm.sblk->gpsw.addr == 0xdeadbeee)
    ++		report_info(msg);
    + 	else
    + 		report_info("Did not interpret initial exception");
      	report_prefix_pop();
      	report_prefix_pop();
      }
      
    -+static bool parse_strict(char **argv)
    ++static bool parse_strict(int argc, char **argv)
     +{
     +	uint16_t machine_id;
     +	char *list;
     +	bool ret;
     +
    -+	if (!*argv)
    ++	if (argc < 1)
     +		return false;
    -+	if (strcmp("--strict", *argv))
    ++	if (strcmp("--strict", argv[0]))
     +		return false;
     +
     +	machine_id = get_machine_id();
    -+	list = argv[1];
    -+	if (!list) {
    ++	if (argc < 2) {
     +		printf("No argument to --strict, ignoring\n");
     +		return false;
     +	}
    ++	list = argv[1];
     +	if (list[0] == '!') {
     +		ret = true;
     +		list++;
    @@ s390x/spec_ex-sie.c: int main(int argc, char **argv)
      	}
      
     -	test_spec_ex_sie();
    -+	test_spec_ex_sie(parse_strict(argv + 1));
    ++	test_spec_ex_sie(parse_strict(argc - 1, argv + 1));
      out:
      	return report_summary();
      }

 s390x/spec_ex-sie.c | 53 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
index 5dea4115..071110e3 100644
--- a/s390x/spec_ex-sie.c
+++ b/s390x/spec_ex-sie.c
@@ -7,6 +7,7 @@
  * specification exception interpretation is off/on.
  */
 #include <libcflat.h>
+#include <stdlib.h>
 #include <sclp.h>
 #include <asm/page.h>
 #include <asm/arch_def.h>
@@ -36,8 +37,10 @@ static void reset_guest(void)
 	vm.sblk->icptcode = 0;
 }
 
-static void test_spec_ex_sie(void)
+static void test_spec_ex_sie(bool strict)
 {
+	const char *msg;
+
 	setup_guest();
 
 	report_prefix_push("SIE spec ex interpretation");
@@ -61,14 +64,56 @@ static void test_spec_ex_sie(void)
 	report(vm.sblk->icptcode == ICPT_PROGI
 	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
 	       "Received specification exception intercept");
-	if (vm.sblk->gpsw.addr == 0xdeadbeee)
-		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
+	msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
+	if (strict)
+		report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
+	else if (vm.sblk->gpsw.addr == 0xdeadbeee)
+		report_info(msg);
 	else
 		report_info("Did not interpret initial exception");
 	report_prefix_pop();
 	report_prefix_pop();
 }
 
+static bool parse_strict(int argc, char **argv)
+{
+	uint16_t machine_id;
+	char *list;
+	bool ret;
+
+	if (argc < 1)
+		return false;
+	if (strcmp("--strict", argv[0]))
+		return false;
+
+	machine_id = get_machine_id();
+	if (argc < 2) {
+		printf("No argument to --strict, ignoring\n");
+		return false;
+	}
+	list = argv[1];
+	if (list[0] == '!') {
+		ret = true;
+		list++;
+	} else
+		ret = false;
+	while (true) {
+		long input = 0;
+
+		if (strlen(list) == 0)
+			return ret;
+		input = strtol(list, &list, 16);
+		if (*list == ',')
+			list++;
+		else if (*list != '\0')
+			break;
+		if (input == machine_id)
+			return !ret;
+	}
+	printf("Invalid --strict argument \"%s\", ignoring\n", list);
+	return ret;
+}
+
 int main(int argc, char **argv)
 {
 	if (!sclp_facilities.has_sief2) {
@@ -76,7 +121,7 @@ int main(int argc, char **argv)
 		goto out;
 	}
 
-	test_spec_ex_sie();
+	test_spec_ex_sie(parse_strict(argc - 1, argv + 1));
 out:
 	return report_summary();
 }

base-commit: 257c962f3d1b2d0534af59de4ad18764d734903a
-- 
2.33.1