[kvm-unit-tests PATCH v2] s390x: Interception tests

[kvm-unit-tests PATCH v2] s390x: Interception tests

[Thomas Huth]

Certain CPU instructions will cause an exit of the virtual
machine. Run some of these instructions to check whether
they are emulated right by KVM (or QEMU).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Note: Test have been verified to pass with KVM of a 4.11 kernel.
 Running the tests with QEMU TCG emulation does not work yet ...
 QEMU first requires a bunch of fixes before this can pass there.
 
 v2:
 - Added entry in s390x/unittests.cfg
 - Use low-core GEN_LC_STFL definition instead of hard-coded magic value
 - Added lots of exception tests (thanks to David's interrupt framework!)
 - Fixed constraints of inline assembler
 
 (I haven't added a timing/iteration infrastructure like Paolo suggested
 yet - will do that later once the basic tests have been accepted)
 
 lib/s390x/asm/interrupt.h |   1 +
 lib/s390x/interrupt.c     |   5 ++
 s390x/Makefile            |   1 +
 s390x/intercept.c         | 170 ++++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg       |   3 +
 5 files changed, 180 insertions(+)
 create mode 100644 s390x/intercept.c

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 383d312..926f858 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -14,5 +14,6 @@
 void handle_pgm_int(void);
 void expect_pgm_int(void);
 void check_pgm_int_code(uint16_t code);
+uint16_t get_pgm_int_code(void);
 
 #endif
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 8d861a2..b5cc7ce 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -23,6 +23,11 @@ void expect_pgm_int(void)
 	mb();
 }
 
+uint16_t get_pgm_int_code(void)
+{
+	return lc->pgm_int_code;
+}
+
 void check_pgm_int_code(uint16_t code)
 {
 	mb();
diff --git a/s390x/Makefile b/s390x/Makefile
index b48f8ab..a61e163 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -1,4 +1,5 @@
 tests = $(TEST_DIR)/selftest.elf
+tests += $(TEST_DIR)/intercept.elf
 
 all: directories test_cases
 
diff --git a/s390x/intercept.c b/s390x/intercept.c
new file mode 100644
index 0000000..4e3fb57
--- /dev/null
+++ b/s390x/intercept.c
@@ -0,0 +1,170 @@
+/*
+ * Interception tests - for s390x CPU instruction that cause a VM exit
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#include <libcflat.h>
+#include <asm/asm-offsets.h>
+#include <asm/interrupt.h>
+#include <asm/page.h>
+
+static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
+
+/* Enable or disable low-address protection */
+static void set_low_prot(bool enable)
+{
+	uint64_t cr0;
+
+	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));
+	if (enable)
+		cr0 |= 1ULL << (63-35);
+	else
+		cr0 &= ~(1ULL << (63-35));
+	asm volatile (" lctlg 0,0,%0 " : : "Q"(cr0));
+}
+
+/* Test the SET PREFIX and STORE PREFIX instructions */
+static void test_prefix(void)
+{
+	uint32_t old_prefix = -1U, tst_prefix = -1U;
+	uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf;
+
+	memset(pagebuf, 0, PAGE_SIZE * 2);
+
+	/*
+	 * Temporarily change the prefix page to our buffer, and store
+	 * some facility bits there ... at least some of them should be
+	 * set in our buffer afterwards.
+	 */
+	asm volatile (
+		" stpx	%0\n"
+		" spx	%2\n"
+		" stfl	0\n"
+		" stpx	%1\n"
+		" spx	%0\n"
+		: "+Q"(old_prefix), "+Q"(tst_prefix)
+		: "Q"(new_prefix)
+		: "memory");
+	report("spx + stfl", pagebuf[GEN_LC_STFL] != 0 &&
+			     old_prefix == 0 && tst_prefix == new_prefix);
+
+	expect_pgm_int();
+	asm volatile(" spx 0(%0) " : : "r"(1));
+	report("spx alignment",
+	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
+
+	expect_pgm_int();
+	asm volatile(" spx 0(%0) " : : "r"(-8));
+	report("spx addressing",
+	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
+
+	expect_pgm_int();
+	set_low_prot(true);
+	asm volatile(" stpx 0(%0) " : : "r"(8));
+	set_low_prot(false);
+	report("stpx low-address protection",
+	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
+
+	expect_pgm_int();
+	asm volatile(" stpx 0(%0) " : : "r"(1));
+	report("stpx alignment",
+	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
+
+	expect_pgm_int();
+	asm volatile(" stpx 0(%0) " : : "r"(-8));
+	report("stpx addressing",
+	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
+}
+
+/* Test the STORE CPU ADDRESS instruction */
+static void test_stap(void)
+{
+	uint16_t cpuid = 0xffff;
+
+	asm volatile ("stap %0\n" : "+Q"(cpuid));
+	report("get cpu id", cpuid != 0xffff);
+
+	expect_pgm_int();
+	set_low_prot(true);
+	asm volatile ("stap 0(%0)\n" : : "r"(8));
+	set_low_prot(false);
+	report("low-address protection",
+	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
+
+	expect_pgm_int();
+	asm volatile ("stap 0(%0)\n" : : "r"(1));
+	report("alignment", get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
+
+	expect_pgm_int();
+	asm volatile ("stap 0(%0)\n" : : "r"(-8));
+	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
+}
+
+/* Test the TEST BLOCK instruction */
+static void test_testblock(void)
+{
+	int cc;
+
+	memset(pagebuf, 0xaa, PAGE_SIZE);
+
+	asm volatile (
+		" lghi	0,0\n"
+		" tb	%1\n"
+		" ipm	%0\n"
+		" srl	%0,28\n"
+		: "=d" (cc)
+		: "a"(pagebuf + 0x123)
+		: "memory", "0", "cc");
+	report("page cleared",
+	       cc == 0 && pagebuf[0] == 0 &&  pagebuf[PAGE_SIZE - 1] == 0);
+
+	expect_pgm_int();
+	set_low_prot(true);
+	asm volatile (" tb %0 " : : "r"(4096));
+	set_low_prot(false);
+	report("low-address protection",
+	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
+
+	expect_pgm_int();
+	asm volatile (" tb %0 " : : "r"(-4096));
+	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
+}
+
+struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "prefix", test_prefix },
+	{ "stap", test_stap },
+	{ "testblock", test_testblock },
+	{ NULL, NULL }
+};
+
+int main(int argc, char **argv)
+{
+	int all = 0;
+	int i;
+
+	report_prefix_push("intercept");
+
+	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "all")))
+		all = 1;
+
+	for (i = 0; tests[i].name != NULL; i++) {
+		report_prefix_push(tests[i].name);
+		if (all || strcmp(argv[1], tests[i].name) == 0) {
+			tests[i].func();
+		}
+		report_prefix_pop();
+	}
+
+	report_prefix_pop();
+
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 92e01ab..3b6b892 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -22,3 +22,6 @@
 file = selftest.elf
 groups = selftest
 extra_params = -append 'test 123'
+
+[intercept]
+file = intercept.elf
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[David Hildenbrand]

On 02.06.2017 14:44, Thomas Huth wrote:
> Certain CPU instructions will cause an exit of the virtual
> machine. Run some of these instructions to check whether
> they are emulated right by KVM (or QEMU).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Note: Test have been verified to pass with KVM of a 4.11 kernel.
>  Running the tests with QEMU TCG emulation does not work yet ...
>  QEMU first requires a bunch of fixes before this can pass there.
>  
>  v2:
>  - Added entry in s390x/unittests.cfg
>  - Use low-core GEN_LC_STFL definition instead of hard-coded magic value
>  - Added lots of exception tests (thanks to David's interrupt framework!)
>  - Fixed constraints of inline assembler
>  
>  (I haven't added a timing/iteration infrastructure like Paolo suggested
>  yet - will do that later once the basic tests have been accepted)
>  
>  lib/s390x/asm/interrupt.h |   1 +
>  lib/s390x/interrupt.c     |   5 ++
>  s390x/Makefile            |   1 +
>  s390x/intercept.c         | 170 ++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg       |   3 +
>  5 files changed, 180 insertions(+)
>  create mode 100644 s390x/intercept.c
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 383d312..926f858 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -14,5 +14,6 @@
>  void handle_pgm_int(void);
>  void expect_pgm_int(void);
>  void check_pgm_int_code(uint16_t code);
> +uint16_t get_pgm_int_code(void);
>  
>  #endif
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 8d861a2..b5cc7ce 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -23,6 +23,11 @@ void expect_pgm_int(void)
>  	mb();
>  }
>  
> +uint16_t get_pgm_int_code(void)
> +{
> +	return lc->pgm_int_code;
> +}
> +
>  void check_pgm_int_code(uint16_t code)
>  {
>  	mb();
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b48f8ab..a61e163 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -1,4 +1,5 @@
>  tests = $(TEST_DIR)/selftest.elf
> +tests += $(TEST_DIR)/intercept.elf
>  
>  all: directories test_cases
>  
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> new file mode 100644
> index 0000000..4e3fb57
> --- /dev/null
> +++ b/s390x/intercept.c
> @@ -0,0 +1,170 @@
> +/*
> + * Interception tests - for s390x CPU instruction that cause a VM exit
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
> +
> +/* Enable or disable low-address protection */
> +static void set_low_prot(bool enable)
> +{
> +	uint64_t cr0;
> +
> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));

Use %c0 instead?

> +	if (enable)
> +		cr0 |= 1ULL << (63-35);
> +	else
> +		cr0 &= ~(1ULL << (63-35));
> +	asm volatile (" lctlg 0,0,%0 " : : "Q"(cr0));

dito.

> +}

Think it makes sense to move this to interrupt.c / interrupt.h, so other
tests can use it. But we can do this later.

> +
> +/* Test the SET PREFIX and STORE PREFIX instructions */
> +static void test_prefix(void)
> +{
> +	uint32_t old_prefix = -1U, tst_prefix = -1U;
> +	uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf;
> +
> +	memset(pagebuf, 0, PAGE_SIZE * 2);
> +
> +	/*
> +	 * Temporarily change the prefix page to our buffer, and store
> +	 * some facility bits there ... at least some of them should be
> +	 * set in our buffer afterwards.
> +	 */
> +	asm volatile (
> +		" stpx	%0\n"
> +		" spx	%2\n"
> +		" stfl	0\n"
> +		" stpx	%1\n"
> +		" spx	%0\n"
> +		: "+Q"(old_prefix), "+Q"(tst_prefix)
> +		: "Q"(new_prefix)
> +		: "memory");
> +	report("spx + stfl", pagebuf[GEN_LC_STFL] != 0 &&
> +			     old_prefix == 0 && tst_prefix == new_prefix);

I would split this into two tests.

> +
> +	expect_pgm_int();
> +	asm volatile(" spx 0(%0) " : : "r"(1));
> +	report("spx alignment",
> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);

Wonder if it makes sense to pass check_pgm_int_code() an string like
"spx alignment", and let it handle the output. But this can also be
changed later.

> +
> +	expect_pgm_int();
> +	asm volatile(" spx 0(%0) " : : "r"(-8));
> +	report("spx addressing",
> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile(" stpx 0(%0) " : : "r"(8));
> +	set_low_prot(false);
> +	report("stpx low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
> +
> +	expect_pgm_int();
> +	asm volatile(" stpx 0(%0) " : : "r"(1));
> +	report("stpx alignment",
> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
> +
> +	expect_pgm_int();
> +	asm volatile(" stpx 0(%0) " : : "r"(-8));
> +	report("stpx addressing",
> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +/* Test the STORE CPU ADDRESS instruction */
> +static void test_stap(void)
> +{
> +	uint16_t cpuid = 0xffff;
> +
> +	asm volatile ("stap %0\n" : "+Q"(cpuid));
> +	report("get cpu id", cpuid != 0xffff);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile ("stap 0(%0)\n" : : "r"(8));
> +	set_low_prot(false);
> +	report("low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION)> +
> +	expect_pgm_int();
> +	asm volatile ("stap 0(%0)\n" : : "r"(1));
> +	report("alignment", get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
> +
> +	expect_pgm_int();
> +	asm volatile ("stap 0(%0)\n" : : "r"(-8));
> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +/* Test the TEST BLOCK instruction */
> +static void test_testblock(void)
> +{
> +	int cc;
> +
> +	memset(pagebuf, 0xaa, PAGE_SIZE);
> +
> +	asm volatile (
> +		" lghi	0,0\n"

%0,0 ?

> +		" tb	%1\n"
> +		" ipm	%0\n"
> +		" srl	%0,28\n"
> +		: "=d" (cc)
> +		: "a"(pagebuf + 0x123)
> +		: "memory", "0", "cc");
> +	report("page cleared",
> +	       cc == 0 && pagebuf[0] == 0 &&  pagebuf[PAGE_SIZE - 1] == 0);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile (" tb %0 " : : "r"(4096));
> +	set_low_prot(false);
> +	report("low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
> +
> +	expect_pgm_int();
> +	asm volatile (" tb %0 " : : "r"(-4096));
> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +struct {
> +	const char *name;
> +	void (*func)(void);
> +} tests[] = {
> +	{ "prefix", test_prefix },
> +	{ "stap", test_stap },
> +	{ "testblock", test_testblock },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	int all = 0;
> +	int i;
> +
> +	report_prefix_push("intercept");
> +
> +	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "all")))
> +		all = 1;
> +
> +	for (i = 0; tests[i].name != NULL; i++) {
> +		report_prefix_push(tests[i].name);
> +		if (all || strcmp(argv[1], tests[i].name) == 0) {
> +			tests[i].func();
> +		}
> +		report_prefix_pop();
> +	}
> +
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 92e01ab..3b6b892 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -22,3 +22,6 @@
>  file = selftest.elf
>  groups = selftest
>  extra_params = -append 'test 123'
> +
> +[intercept]
> +file = intercept.elf
> 

Nice! With or without these nits fixed.

Once upstream, I'll add a stidp test.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[Thomas Huth]

On 02.06.2017 15:49, David Hildenbrand wrote:
> On 02.06.2017 14:44, Thomas Huth wrote:
>> Certain CPU instructions will cause an exit of the virtual
>> machine. Run some of these instructions to check whether
>> they are emulated right by KVM (or QEMU).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  Note: Test have been verified to pass with KVM of a 4.11 kernel.
>>  Running the tests with QEMU TCG emulation does not work yet ...
>>  QEMU first requires a bunch of fixes before this can pass there.
>>  
>>  v2:
>>  - Added entry in s390x/unittests.cfg
>>  - Use low-core GEN_LC_STFL definition instead of hard-coded magic value
>>  - Added lots of exception tests (thanks to David's interrupt framework!)
>>  - Fixed constraints of inline assembler
>>  
>>  (I haven't added a timing/iteration infrastructure like Paolo suggested
>>  yet - will do that later once the basic tests have been accepted)
>>  
>>  lib/s390x/asm/interrupt.h |   1 +
>>  lib/s390x/interrupt.c     |   5 ++
>>  s390x/Makefile            |   1 +
>>  s390x/intercept.c         | 170 ++++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg       |   3 +
>>  5 files changed, 180 insertions(+)
>>  create mode 100644 s390x/intercept.c
>>
>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>> index 383d312..926f858 100644
>> --- a/lib/s390x/asm/interrupt.h
>> +++ b/lib/s390x/asm/interrupt.h
>> @@ -14,5 +14,6 @@
>>  void handle_pgm_int(void);
>>  void expect_pgm_int(void);
>>  void check_pgm_int_code(uint16_t code);
>> +uint16_t get_pgm_int_code(void);
>>  
>>  #endif
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 8d861a2..b5cc7ce 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -23,6 +23,11 @@ void expect_pgm_int(void)
>>  	mb();
>>  }
>>  
>> +uint16_t get_pgm_int_code(void)
>> +{
>> +	return lc->pgm_int_code;
>> +}
>> +
>>  void check_pgm_int_code(uint16_t code)
>>  {
>>  	mb();
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index b48f8ab..a61e163 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -1,4 +1,5 @@
>>  tests = $(TEST_DIR)/selftest.elf
>> +tests += $(TEST_DIR)/intercept.elf
>>  
>>  all: directories test_cases
>>  
>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>> new file mode 100644
>> index 0000000..4e3fb57
>> --- /dev/null
>> +++ b/s390x/intercept.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * Interception tests - for s390x CPU instruction that cause a VM exit
>> + *
>> + * Copyright (c) 2017 Red Hat Inc
>> + *
>> + * Authors:
>> + *  Thomas Huth <thuth@redhat.com>
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +#include <libcflat.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/page.h>
>> +
>> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
>> +
>> +/* Enable or disable low-address protection */
>> +static void set_low_prot(bool enable)
>> +{
>> +	uint64_t cr0;
>> +
>> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));
> 
> Use %c0 instead?

It's got to be %%c0 ... not sure whether this looks really nicer here?

>> +	if (enable)
>> +		cr0 |= 1ULL << (63-35);
>> +	else
>> +		cr0 &= ~(1ULL << (63-35));
>> +	asm volatile (" lctlg 0,0,%0 " : : "Q"(cr0));
> 
> dito.
> 
>> +}
> 
> Think it makes sense to move this to interrupt.c / interrupt.h, so other
> tests can use it. But we can do this later.

OK, I can do that (I guess I have to respin anyway).

>> +
>> +/* Test the SET PREFIX and STORE PREFIX instructions */
>> +static void test_prefix(void)
>> +{
>> +	uint32_t old_prefix = -1U, tst_prefix = -1U;
>> +	uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf;
>> +
>> +	memset(pagebuf, 0, PAGE_SIZE * 2);
>> +
>> +	/*
>> +	 * Temporarily change the prefix page to our buffer, and store
>> +	 * some facility bits there ... at least some of them should be
>> +	 * set in our buffer afterwards.
>> +	 */
>> +	asm volatile (
>> +		" stpx	%0\n"
>> +		" spx	%2\n"
>> +		" stfl	0\n"
>> +		" stpx	%1\n"
>> +		" spx	%0\n"
>> +		: "+Q"(old_prefix), "+Q"(tst_prefix)
>> +		: "Q"(new_prefix)
>> +		: "memory");
>> +	report("spx + stfl", pagebuf[GEN_LC_STFL] != 0 &&
>> +			     old_prefix == 0 && tst_prefix == new_prefix);
> 
> I would split this into two tests.

Sorry, I can't follow you here ... Do you mean to just split the
report() or the whole sequence of assembler instructions?

>> +
>> +	expect_pgm_int();
>> +	asm volatile(" spx 0(%0) " : : "r"(1));
>> +	report("spx alignment",
>> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
> 
> Wonder if it makes sense to pass check_pgm_int_code() an string like
> "spx alignment", and let it handle the output. But this can also be
> changed later.

I guess I could also work with report_prefix_push() here ... not sure
whether that looks nicer ... will give it a try...

>> +
>> +	expect_pgm_int();
>> +	asm volatile(" spx 0(%0) " : : "r"(-8));
>> +	report("spx addressing",
>> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
>> +
>> +	expect_pgm_int();
>> +	set_low_prot(true);
>> +	asm volatile(" stpx 0(%0) " : : "r"(8));
>> +	set_low_prot(false);
>> +	report("stpx low-address protection",
>> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
>> +
>> +	expect_pgm_int();
>> +	asm volatile(" stpx 0(%0) " : : "r"(1));
>> +	report("stpx alignment",
>> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
>> +
>> +	expect_pgm_int();
>> +	asm volatile(" stpx 0(%0) " : : "r"(-8));
>> +	report("stpx addressing",
>> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
>> +}
>> +
>> +/* Test the STORE CPU ADDRESS instruction */
>> +static void test_stap(void)
>> +{
>> +	uint16_t cpuid = 0xffff;
>> +
>> +	asm volatile ("stap %0\n" : "+Q"(cpuid));
>> +	report("get cpu id", cpuid != 0xffff);
>> +
>> +	expect_pgm_int();
>> +	set_low_prot(true);
>> +	asm volatile ("stap 0(%0)\n" : : "r"(8));
>> +	set_low_prot(false);
>> +	report("low-address protection",
>> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION)> +
>> +	expect_pgm_int();
>> +	asm volatile ("stap 0(%0)\n" : : "r"(1));
>> +	report("alignment", get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
>> +
>> +	expect_pgm_int();
>> +	asm volatile ("stap 0(%0)\n" : : "r"(-8));
>> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
>> +}
>> +
>> +/* Test the TEST BLOCK instruction */
>> +static void test_testblock(void)
>> +{
>> +	int cc;
>> +
>> +	memset(pagebuf, 0xaa, PAGE_SIZE);
>> +
>> +	asm volatile (
>> +		" lghi	0,0\n"
> 
> %0,0 ?

That's certainly wrong, since %0 is the reference to the first output
parameter (cc). You likely mean %%0 or %%r0 ... and that looks rather
cumbersome, too. I think I prefer the plain "0" here, what do you think?

>> +		" tb	%1\n"
>> +		" ipm	%0\n"
>> +		" srl	%0,28\n"
>> +		: "=d" (cc)
>> +		: "a"(pagebuf + 0x123)
>> +		: "memory", "0", "cc");
>> +	report("page cleared",
>> +	       cc == 0 && pagebuf[0] == 0 &&  pagebuf[PAGE_SIZE - 1] == 0);
>> +
>> +	expect_pgm_int();
>> +	set_low_prot(true);
>> +	asm volatile (" tb %0 " : : "r"(4096));
>> +	set_low_prot(false);
>> +	report("low-address protection",
>> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
>> +
>> +	expect_pgm_int();
>> +	asm volatile (" tb %0 " : : "r"(-4096));
>> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
>> +}
>> +
>> +struct {
>> +	const char *name;
>> +	void (*func)(void);
>> +} tests[] = {
>> +	{ "prefix", test_prefix },
>> +	{ "stap", test_stap },
>> +	{ "testblock", test_testblock },
>> +	{ NULL, NULL }
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	int all = 0;
>> +	int i;
>> +
>> +	report_prefix_push("intercept");
>> +
>> +	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "all")))
>> +		all = 1;
>> +
>> +	for (i = 0; tests[i].name != NULL; i++) {
>> +		report_prefix_push(tests[i].name);
>> +		if (all || strcmp(argv[1], tests[i].name) == 0) {
>> +			tests[i].func();
>> +		}
>> +		report_prefix_pop();
>> +	}
>> +
>> +	report_prefix_pop();
>> +
>> +	return report_summary();
>> +}
>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>> index 92e01ab..3b6b892 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -22,3 +22,6 @@
>>  file = selftest.elf
>>  groups = selftest
>>  extra_params = -append 'test 123'
>> +
>> +[intercept]
>> +file = intercept.elf
>>
> 
> Nice! With or without these nits fixed.
> 
> Once upstream, I'll add a stidp test.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 

Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[David Hildenbrand]

>>> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
>>> +
>>> +/* Enable or disable low-address protection */
>>> +static void set_low_prot(bool enable)
>>> +{
>>> +	uint64_t cr0;
>>> +
>>> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));
>>
>> Use %c0 instead?
> 
> It's got to be %%c0 ... not sure whether this looks really nicer here?

%c0 should work, e.g. do a "git grep "%c0" in linux.git

arch/s390/kernel/base.S:        lctlg   %c0,%c0,0(%r4)
...

>>> +	report("spx + stfl", pagebuf[GEN_LC_STFL] != 0 &&
>>> +			     );
>>
>> I would split this into two tests.
> 
> Sorry, I can't follow you here ... Do you mean to just split the
> report() or the whole sequence of assembler instructions?

Sorry, I meant I would split it into:

report("spx + stpx", old_prefix == 0 && tst_prefix == new_prefix);
report("spx + stfl", pagebuf[GEN_LC_STFL] != 0);

>>> +
>>> +/* Test the TEST BLOCK instruction */
>>> +static void test_testblock(void)
>>> +{
>>> +	int cc;
>>> +
>>> +	memset(pagebuf, 0xaa, PAGE_SIZE);
>>> +
>>> +	asm volatile (
>>> +		" lghi	0,0\n"
>>
>> %0,0 ?
> 
> That's certainly wrong, since %0 is the reference to the first output
> parameter (cc). You likely mean %%0 or %%r0 ... and that looks rather
> cumbersome, too. I think I prefer the plain "0" here, what do you think?

Sorry, of course I meant %r0. My keyboard ate one character ;)

(again, only one % is required, see e.g. arch/s390/kernel/entry.S)


-- 

Thanks,

David

Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[Thomas Huth]

On 02.06.2017 18:22, David Hildenbrand wrote:
> 
>>>> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
>>>> +
>>>> +/* Enable or disable low-address protection */
>>>> +static void set_low_prot(bool enable)
>>>> +{
>>>> +	uint64_t cr0;
>>>> +
>>>> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));
>>>
>>> Use %c0 instead?
>>
>> It's got to be %%c0 ... not sure whether this looks really nicer here?
> 
> %c0 should work, e.g. do a "git grep "%c0" in linux.git
> 
> arch/s390/kernel/base.S:        lctlg   %c0,%c0,0(%r4)

Well, that's a .S file ... for inline assembly in .c files, you need two
percentage characters.

>>>> +/* Test the TEST BLOCK instruction */
>>>> +static void test_testblock(void)
>>>> +{
>>>> +	int cc;
>>>> +
>>>> +	memset(pagebuf, 0xaa, PAGE_SIZE);
>>>> +
>>>> +	asm volatile (
>>>> +		" lghi	0,0\n"
>>>
>>> %0,0 ?
>>
>> That's certainly wrong, since %0 is the reference to the first output
>> parameter (cc). You likely mean %%0 or %%r0 ... and that looks rather
>> cumbersome, too. I think I prefer the plain "0" here, what do you think?
> 
> Sorry, of course I meant %r0. My keyboard ate one character ;)
> 
> (again, only one % is required, see e.g. arch/s390/kernel/entry.S)

Dito, also %% required here, since we're not in a .S file.

 Thomas

Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[David Hildenbrand]

On 04.06.2017 10:59, Thomas Huth wrote:
> On 02.06.2017 18:22, David Hildenbrand wrote:
>>
>>>>> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE)));
>>>>> +
>>>>> +/* Enable or disable low-address protection */
>>>>> +static void set_low_prot(bool enable)
>>>>> +{
>>>>> +	uint64_t cr0;
>>>>> +
>>>>> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));
>>>>
>>>> Use %c0 instead?
>>>
>>> It's got to be %%c0 ... not sure whether this looks really nicer here?
>>
>> %c0 should work, e.g. do a "git grep "%c0" in linux.git
>>
>> arch/s390/kernel/base.S:        lctlg   %c0,%c0,0(%r4)
> 
> Well, that's a .S file ... for inline assembly in .c files, you need two
> percentage characters.
> 

Oh right, wasn't aware of that! As I said, keep it like that if you prefer.

-- 

Thanks,

David