[PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Collin Walling]

The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
that must be intercepted via SIE, handled in userspace, and the
information set by the instruction is communicated back to KVM.

To test the instruction interception, an ad-hoc handler is defined which
simply has a VM execute the instruction and then userspace will extract
the necessary info. The handler is defined such that the instruction
invocation occurs only once. It is up the the caller to determine how the
info returned by this handler should be used.

The diag318 info is communicated from userspace to KVM via a sync_regs
call. This is tested during a sync_regs test, where the diag318 info is
requested via the handler, then the info is stored in the appropriate
register in KVM via a sync registers call.

The diag318 info is checked to be 0 after a normal and clear reset.

If KVM does not support diag318, then the tests will print a message
stating that diag318 was skipped, and the asserts will simply test
against a value of 0.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
 .../kvm/lib/s390x/diag318_test_handler.c      | 80 +++++++++++++++++++
 tools/testing/selftests/kvm/s390x/resets.c    |  6 ++
 .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
 5 files changed, 115 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
 create mode 100644 tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 4a166588d99f..ed172a0b83b6 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -36,7 +36,7 @@ endif
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c
 LIBKVM_x86_64 = lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c
 LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
-LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c
+LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
diff --git a/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h b/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
new file mode 100644
index 000000000000..d8233ebf317b
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Test handler for the s390x DIAGNOSE 0x0318 instruction.
+ *
+ * Copyright (C) 2020, IBM
+ */
+
+#ifndef SELFTEST_KVM_DIAG318_TEST_HANDLER
+#define SELFTEST_KVM_DIAG318_TEST_HANDLER
+
+uint64_t get_diag318_info(void);
+
+#endif
diff --git a/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
new file mode 100644
index 000000000000..024e3a9ffda7
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Test handler for the s390x DIAGNOSE 0x0318 instruction.
+ *
+ * Copyright (C) 2020, IBM
+ */
+
+#include "test_util.h"
+#include "kvm_util.h"
+
+#define VCPU_ID	5
+
+#define ICPT_INSTRUCTION	0x04
+#define IPA0_DIAG		0x8300
+
+static void guest_code(void)
+{
+	uint64_t diag318_info = 0x12345678;
+
+	asm volatile ("diag %0,0,0x318\n" : : "d" (diag318_info));
+}
+
+/*
+ * The DIAGNOSE 0x0318 instruction call must be handled via userspace. As such,
+ * we create an ad-hoc VM here to handle the instruction then extract the
+ * necessary data. It is up to the caller to decide what to do with that data.
+ */
+static uint64_t diag318_handler(void)
+{
+	struct kvm_vm *vm;
+	struct kvm_run *run;
+	uint64_t reg;
+	uint64_t diag318_info;
+
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+	vcpu_run(vm, VCPU_ID);
+	run = vcpu_state(vm, VCPU_ID);
+
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
+		    "DIAGNOSE 0x0318 instruction was not intercepted");
+	TEST_ASSERT(run->s390_sieic.icptcode == ICPT_INSTRUCTION,
+		    "Unexpected intercept code: 0x%x", run->s390_sieic.icptcode);
+	TEST_ASSERT((run->s390_sieic.ipa & 0xff00) == IPA0_DIAG,
+		    "Unexpected IPA0 code: 0x%x", (run->s390_sieic.ipa & 0xff00));
+
+	reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
+	diag318_info = run->s.regs.gprs[reg];
+
+	kvm_vm_free(vm);
+
+	return diag318_info;
+}
+
+uint64_t get_diag318_info(void)
+{
+	static uint64_t diag318_info;
+	static bool printed_skip;
+
+	/*
+	 * If KVM does not support diag318, then return 0 to
+	 * ensure tests do not break.
+	 */
+	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
+		if (!printed_skip) {
+			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
+				"Skipping diag318 test.\n");
+			printed_skip = true;
+		}
+		return 0;
+	}
+
+	/*
+	 * If a test has previously requested the diag318 info,
+	 * then don't bother spinning up a temporary VM again.
+	 */
+	if (!diag318_info)
+		diag318_info = diag318_handler();
+
+	return diag318_info;
+}
diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
index b143db6d8693..d0416ba94ec5 100644
--- a/tools/testing/selftests/kvm/s390x/resets.c
+++ b/tools/testing/selftests/kvm/s390x/resets.c
@@ -12,6 +12,7 @@
 
 #include "test_util.h"
 #include "kvm_util.h"
+#include "diag318_test_handler.h"
 
 #define VCPU_ID 3
 #define LOCAL_IRQS 32
@@ -110,6 +111,8 @@ static void assert_clear(void)
 
 	TEST_ASSERT(!memcmp(sync_regs->vrs, regs_null, sizeof(sync_regs->vrs)),
 		    "vrs0-15 == 0 (sync_regs)");
+
+	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
 }
 
 static void assert_initial_noclear(void)
@@ -182,6 +185,7 @@ static void assert_normal(void)
 	test_one_reg(KVM_REG_S390_PFTOKEN, KVM_S390_PFAULT_TOKEN_INVALID);
 	TEST_ASSERT(sync_regs->pft == KVM_S390_PFAULT_TOKEN_INVALID,
 			"pft == 0xff.....  (sync_regs)");
+	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
 	assert_noirq();
 }
 
@@ -206,6 +210,7 @@ static void test_normal(void)
 	/* Create VM */
 	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
 	run = vcpu_state(vm, VCPU_ID);
+	run->s.regs.diag318 = get_diag318_info();
 	sync_regs = &run->s.regs;
 
 	vcpu_run(vm, VCPU_ID);
@@ -250,6 +255,7 @@ static void test_clear(void)
 	pr_info("Testing clear reset\n");
 	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
 	run = vcpu_state(vm, VCPU_ID);
+	run->s.regs.diag318 = get_diag318_info();
 	sync_regs = &run->s.regs;
 
 	vcpu_run(vm, VCPU_ID);
diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index 5731ccf34917..caf7b8859a94 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -20,6 +20,7 @@
 
 #include "test_util.h"
 #include "kvm_util.h"
+#include "diag318_test_handler.h"
 
 #define VCPU_ID 5
 
@@ -70,7 +71,7 @@ static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
 
 #undef REG_COMPARE
 
-#define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS)
+#define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS|KVM_SYNC_DIAG318)
 #define INVALID_SYNC_FIELD 0x80000000
 
 int main(int argc, char *argv[])
@@ -152,6 +153,12 @@ int main(int argc, char *argv[])
 
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	run->kvm_dirty_regs = KVM_SYNC_GPRS | KVM_SYNC_ACRS;
+
+	if (get_diag318_info() > 0) {
+		run->s.regs.diag318 = get_diag318_info();
+		run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+	}
+
 	rv = _vcpu_run(vm, VCPU_ID);
 	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
@@ -164,6 +171,9 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(run->s.regs.acrs[0]  == 1 << 11,
 		    "acr0 sync regs value incorrect 0x%x.",
 		    run->s.regs.acrs[0]);
+	TEST_ASSERT(run->s.regs.diag318 == get_diag318_info(),
+		    "diag318 sync regs value incorrect 0x%llx.",
+		    run->s.regs.diag318);
 
 	vcpu_regs_get(vm, VCPU_ID, &regs);
 	compare_regs(&regs, &run->s.regs);
@@ -177,6 +187,7 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	run->kvm_dirty_regs = 0;
 	run->s.regs.gprs[11] = 0xDEADBEEF;
+	run->s.regs.diag318 = 0x4B1D;
 	rv = _vcpu_run(vm, VCPU_ID);
 	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
@@ -186,6 +197,9 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(run->s.regs.gprs[11] != 0xDEADBEEF,
 		    "r11 sync regs value incorrect 0x%llx.",
 		    run->s.regs.gprs[11]);
+	TEST_ASSERT(run->s.regs.diag318 != 0x4B1D,
+		    "diag318 sync regs value incorrect 0x%llx.",
+		    run->s.regs.diag318);
 
 	kvm_vm_free(vm);
 
-- 
2.26.2

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Thomas Huth]

On 14/10/2020 21.27, Collin Walling wrote:
> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
> that must be intercepted via SIE, handled in userspace, and the
> information set by the instruction is communicated back to KVM.
> 
> To test the instruction interception, an ad-hoc handler is defined which
> simply has a VM execute the instruction and then userspace will extract
> the necessary info. The handler is defined such that the instruction
> invocation occurs only once. It is up the the caller to determine how the
> info returned by this handler should be used.
> 
> The diag318 info is communicated from userspace to KVM via a sync_regs
> call. This is tested during a sync_regs test, where the diag318 info is
> requested via the handler, then the info is stored in the appropriate
> register in KVM via a sync registers call.
> 
> The diag318 info is checked to be 0 after a normal and clear reset.
> 
> If KVM does not support diag318, then the tests will print a message
> stating that diag318 was skipped, and the asserts will simply test
> against a value of 0.

Thanks a lot for writing the test! Looks pretty good already, but I still
have some comments / questions below...

> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Test handler for the s390x DIAGNOSE 0x0318 instruction.
> + *
> + * Copyright (C) 2020, IBM
> + */
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +
> +#define VCPU_ID	5
> +
> +#define ICPT_INSTRUCTION	0x04
> +#define IPA0_DIAG		0x8300
> +
> +static void guest_code(void)
> +{
> +	uint64_t diag318_info = 0x12345678;
> +
> +	asm volatile ("diag %0,0,0x318\n" : : "d" (diag318_info));
> +}
> +
> +/*
> + * The DIAGNOSE 0x0318 instruction call must be handled via userspace. As such,
> + * we create an ad-hoc VM here to handle the instruction then extract the
> + * necessary data. It is up to the caller to decide what to do with that data.
> + */
> +static uint64_t diag318_handler(void)
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_run *run;
> +	uint64_t reg;
> +	uint64_t diag318_info;
> +
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +	vcpu_run(vm, VCPU_ID);
> +	run = vcpu_state(vm, VCPU_ID);
> +
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
> +		    "DIAGNOSE 0x0318 instruction was not intercepted");
> +	TEST_ASSERT(run->s390_sieic.icptcode == ICPT_INSTRUCTION,
> +		    "Unexpected intercept code: 0x%x", run->s390_sieic.icptcode);
> +	TEST_ASSERT((run->s390_sieic.ipa & 0xff00) == IPA0_DIAG,
> +		    "Unexpected IPA0 code: 0x%x", (run->s390_sieic.ipa & 0xff00));
> +
> +	reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +	diag318_info = run->s.regs.gprs[reg];
> +
> +	kvm_vm_free(vm);

Could you please add a

	TEST_ASSERT(diag_318_info == 0x12345678, ...)

here?

> +	return diag318_info;
> +}
> +
> +uint64_t get_diag318_info(void)
> +{
> +	static uint64_t diag318_info;
> +	static bool printed_skip;
> +
> +	/*
> +	 * If KVM does not support diag318, then return 0 to
> +	 * ensure tests do not break.
> +	 */
> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
> +		if (!printed_skip) {
> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
> +				"Skipping diag318 test.\n");
> +			printed_skip = true;
> +		}
> +		return 0;
> +	}
> +
> +	/*
> +	 * If a test has previously requested the diag318 info,
> +	 * then don't bother spinning up a temporary VM again.
> +	 */
> +	if (!diag318_info)
> +		diag318_info = diag318_handler();
> +
> +	return diag318_info;
> +}
> diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
> index b143db6d8693..d0416ba94ec5 100644
> --- a/tools/testing/selftests/kvm/s390x/resets.c
> +++ b/tools/testing/selftests/kvm/s390x/resets.c
> @@ -12,6 +12,7 @@
>  
>  #include "test_util.h"
>  #include "kvm_util.h"
> +#include "diag318_test_handler.h"
>  
>  #define VCPU_ID 3
>  #define LOCAL_IRQS 32
> @@ -110,6 +111,8 @@ static void assert_clear(void)
>  
>  	TEST_ASSERT(!memcmp(sync_regs->vrs, regs_null, sizeof(sync_regs->vrs)),
>  		    "vrs0-15 == 0 (sync_regs)");
> +
> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>  }
>  
>  static void assert_initial_noclear(void)
> @@ -182,6 +185,7 @@ static void assert_normal(void)
>  	test_one_reg(KVM_REG_S390_PFTOKEN, KVM_S390_PFAULT_TOKEN_INVALID);
>  	TEST_ASSERT(sync_regs->pft == KVM_S390_PFAULT_TOKEN_INVALID,
>  			"pft == 0xff.....  (sync_regs)");
> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>  	assert_noirq();
>  }
>  
> @@ -206,6 +210,7 @@ static void test_normal(void)
>  	/* Create VM */
>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>  	run = vcpu_state(vm, VCPU_ID);
> +	run->s.regs.diag318 = get_diag318_info();
>  	sync_regs = &run->s.regs;

Not sure, but don't you have to mark KVM_SYNC_DIAG318 in run->kvm_valid_regs
and run->kvm_dirty_regs here...

>  	vcpu_run(vm, VCPU_ID);
> @@ -250,6 +255,7 @@ static void test_clear(void)
>  	pr_info("Testing clear reset\n");
>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>  	run = vcpu_state(vm, VCPU_ID);
> +	run->s.regs.diag318 = get_diag318_info();
>  	sync_regs = &run->s.regs;

... and here?

 Thomas

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Collin Walling]

On 10/15/20 3:55 AM, Thomas Huth wrote:
> On 14/10/2020 21.27, Collin Walling wrote:
>> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
>> that must be intercepted via SIE, handled in userspace, and the
>> information set by the instruction is communicated back to KVM.
>>
>> To test the instruction interception, an ad-hoc handler is defined which
>> simply has a VM execute the instruction and then userspace will extract
>> the necessary info. The handler is defined such that the instruction
>> invocation occurs only once. It is up the the caller to determine how the
>> info returned by this handler should be used.
>>
>> The diag318 info is communicated from userspace to KVM via a sync_regs
>> call. This is tested during a sync_regs test, where the diag318 info is
>> requested via the handler, then the info is stored in the appropriate
>> register in KVM via a sync registers call.
>>
>> The diag318 info is checked to be 0 after a normal and clear reset.
>>
>> If KVM does not support diag318, then the tests will print a message
>> stating that diag318 was skipped, and the asserts will simply test
>> against a value of 0.
> 
> Thanks a lot for writing the test! Looks pretty good already, but I still
> have some comments / questions below...
> 
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
>> @@ -0,0 +1,80 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Test handler for the s390x DIAGNOSE 0x0318 instruction.
>> + *
>> + * Copyright (C) 2020, IBM
>> + */
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +
>> +#define VCPU_ID	5
>> +
>> +#define ICPT_INSTRUCTION	0x04
>> +#define IPA0_DIAG		0x8300
>> +
>> +static void guest_code(void)
>> +{
>> +	uint64_t diag318_info = 0x12345678;
>> +
>> +	asm volatile ("diag %0,0,0x318\n" : : "d" (diag318_info));
>> +}
>> +
>> +/*
>> + * The DIAGNOSE 0x0318 instruction call must be handled via userspace. As such,
>> + * we create an ad-hoc VM here to handle the instruction then extract the
>> + * necessary data. It is up to the caller to decide what to do with that data.
>> + */
>> +static uint64_t diag318_handler(void)
>> +{
>> +	struct kvm_vm *vm;
>> +	struct kvm_run *run;
>> +	uint64_t reg;
>> +	uint64_t diag318_info;
>> +
>> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
>> +	vcpu_run(vm, VCPU_ID);
>> +	run = vcpu_state(vm, VCPU_ID);
>> +
>> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
>> +		    "DIAGNOSE 0x0318 instruction was not intercepted");
>> +	TEST_ASSERT(run->s390_sieic.icptcode == ICPT_INSTRUCTION,
>> +		    "Unexpected intercept code: 0x%x", run->s390_sieic.icptcode);
>> +	TEST_ASSERT((run->s390_sieic.ipa & 0xff00) == IPA0_DIAG,
>> +		    "Unexpected IPA0 code: 0x%x", (run->s390_sieic.ipa & 0xff00));
>> +
>> +	reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>> +	diag318_info = run->s.regs.gprs[reg];
>> +
>> +	kvm_vm_free(vm);
> 
> Could you please add a
> 
> 	TEST_ASSERT(diag_318_info == 0x12345678, ...)
> 
> here?

Will do.

> 
>> +	return diag318_info;
>> +}
>> +
>> +uint64_t get_diag318_info(void)
>> +{
>> +	static uint64_t diag318_info;
>> +	static bool printed_skip;
>> +
>> +	/*
>> +	 * If KVM does not support diag318, then return 0 to
>> +	 * ensure tests do not break.
>> +	 */
>> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
>> +		if (!printed_skip) {
>> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
>> +				"Skipping diag318 test.\n");
>> +			printed_skip = true;
>> +		}
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * If a test has previously requested the diag318 info,
>> +	 * then don't bother spinning up a temporary VM again.
>> +	 */
>> +	if (!diag318_info)
>> +		diag318_info = diag318_handler();
>> +
>> +	return diag318_info;
>> +}
>> diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
>> index b143db6d8693..d0416ba94ec5 100644
>> --- a/tools/testing/selftests/kvm/s390x/resets.c
>> +++ b/tools/testing/selftests/kvm/s390x/resets.c
>> @@ -12,6 +12,7 @@
>>  
>>  #include "test_util.h"
>>  #include "kvm_util.h"
>> +#include "diag318_test_handler.h"
>>  
>>  #define VCPU_ID 3
>>  #define LOCAL_IRQS 32
>> @@ -110,6 +111,8 @@ static void assert_clear(void)
>>  
>>  	TEST_ASSERT(!memcmp(sync_regs->vrs, regs_null, sizeof(sync_regs->vrs)),
>>  		    "vrs0-15 == 0 (sync_regs)");
>> +
>> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>>  }
>>  
>>  static void assert_initial_noclear(void)
>> @@ -182,6 +185,7 @@ static void assert_normal(void)
>>  	test_one_reg(KVM_REG_S390_PFTOKEN, KVM_S390_PFAULT_TOKEN_INVALID);
>>  	TEST_ASSERT(sync_regs->pft == KVM_S390_PFAULT_TOKEN_INVALID,
>>  			"pft == 0xff.....  (sync_regs)");
>> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>>  	assert_noirq();
>>  }
>>  
>> @@ -206,6 +210,7 @@ static void test_normal(void)
>>  	/* Create VM */
>>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>>  	run = vcpu_state(vm, VCPU_ID);
>> +	run->s.regs.diag318 = get_diag318_info();
>>  	sync_regs = &run->s.regs;
> 
> Not sure, but don't you have to mark KVM_SYNC_DIAG318 in run->kvm_valid_regs
> and run->kvm_dirty_regs here...

Hmm... you're right. I need to do that...

Looks like the normal reset case is failing now. I must've missed
setting the value to 0 in KVM's normal reset handler...

It's a one-line fix (and truthfully, there isn't much harm done). I'll
toss it up on the list.

> 
>>  	vcpu_run(vm, VCPU_ID);
>> @@ -250,6 +255,7 @@ static void test_clear(void)
>>  	pr_info("Testing clear reset\n");
>>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>>  	run = vcpu_state(vm, VCPU_ID);
>> +	run->s.regs.diag318 = get_diag318_info();
>>  	sync_regs = &run->s.regs;
> 
> ... and here?
> 
>  Thomas
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Thomas Huth]

On 15/10/2020 17.26, Collin Walling wrote:
> On 10/15/20 3:55 AM, Thomas Huth wrote:
[...]
>>> @@ -206,6 +210,7 @@ static void test_normal(void)
>>>  	/* Create VM */
>>>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>>>  	run = vcpu_state(vm, VCPU_ID);
>>> +	run->s.regs.diag318 = get_diag318_info();
>>>  	sync_regs = &run->s.regs;
>>
>> Not sure, but don't you have to mark KVM_SYNC_DIAG318 in run->kvm_valid_regs
>> and run->kvm_dirty_regs here...
> 
> Hmm... you're right. I need to do that...
> 
> Looks like the normal reset case is failing now. I must've missed
> setting the value to 0 in KVM's normal reset handler...
> 
> It's a one-line fix (and truthfully, there isn't much harm done). I'll
> toss it up on the list.

Even if it's just a small issue, it's always great to see that the KVM
selftests help to find bugs :-)

 Thomas

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 10072 bytes --]

On 10/14/20 9:27 PM, Collin Walling wrote:
> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
> that must be intercepted via SIE, handled in userspace, and the
> information set by the instruction is communicated back to KVM.

It might be nice to have a few words in here about what information can
be set via the diag.

> 
> To test the instruction interception, an ad-hoc handler is defined which
> simply has a VM execute the instruction and then userspace will extract
> the necessary info. The handler is defined such that the instruction
> invocation occurs only once. It is up the the caller to determine how the
> info returned by this handler should be used.
> 
> The diag318 info is communicated from userspace to KVM via a sync_regs
> call. This is tested during a sync_regs test, where the diag318 info is
> requested via the handler, then the info is stored in the appropriate
> register in KVM via a sync registers call.
> 
> The diag318 info is checked to be 0 after a normal and clear reset.
> 
> If KVM does not support diag318, then the tests will print a message
> stating that diag318 was skipped, and the asserts will simply test
> against a value of 0.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Checkpatch throws lots of errors on this patch.
Could you check if my workflow misteriously introduced windows line
endings or if they were introduced on your side?

> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>  .../kvm/lib/s390x/diag318_test_handler.c      | 80 +++++++++++++++++++
>  tools/testing/selftests/kvm/s390x/resets.c    |  6 ++
>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>  5 files changed, 115 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
>  create mode 100644 tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 4a166588d99f..ed172a0b83b6 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -36,7 +36,7 @@ endif
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/sparsebit.c lib/test_util.c
>  LIBKVM_x86_64 = lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c
>  LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
> -LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c
> +LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
>  TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
> diff --git a/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h b/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
> new file mode 100644
> index 000000000000..d8233ebf317b
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/s390x/diag318_test_handler.h
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Test handler for the s390x DIAGNOSE 0x0318 instruction.
> + *
> + * Copyright (C) 2020, IBM
> + */
> +
> +#ifndef SELFTEST_KVM_DIAG318_TEST_HANDLER
> +#define SELFTEST_KVM_DIAG318_TEST_HANDLER
> +
> +uint64_t get_diag318_info(void);
> +
> +#endif
> diff --git a/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> new file mode 100644
> index 000000000000..024e3a9ffda7
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Test handler for the s390x DIAGNOSE 0x0318 instruction.
> + *
> + * Copyright (C) 2020, IBM
> + */
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +
> +#define VCPU_ID	5
> +
> +#define ICPT_INSTRUCTION	0x04
> +#define IPA0_DIAG		0x8300
> +
> +static void guest_code(void)
> +{
> +	uint64_t diag318_info = 0x12345678;
> +
> +	asm volatile ("diag %0,0,0x318\n" : : "d" (diag318_info));
> +}
> +
> +/*
> + * The DIAGNOSE 0x0318 instruction call must be handled via userspace. As such,
> + * we create an ad-hoc VM here to handle the instruction then extract the
> + * necessary data. It is up to the caller to decide what to do with that data.
> + */
> +static uint64_t diag318_handler(void)
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_run *run;
> +	uint64_t reg;
> +	uint64_t diag318_info;
> +
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +	vcpu_run(vm, VCPU_ID);
> +	run = vcpu_state(vm, VCPU_ID);
> +
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
> +		    "DIAGNOSE 0x0318 instruction was not intercepted");
> +	TEST_ASSERT(run->s390_sieic.icptcode == ICPT_INSTRUCTION,
> +		    "Unexpected intercept code: 0x%x", run->s390_sieic.icptcode);
> +	TEST_ASSERT((run->s390_sieic.ipa & 0xff00) == IPA0_DIAG,
> +		    "Unexpected IPA0 code: 0x%x", (run->s390_sieic.ipa & 0xff00));
> +
> +	reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +	diag318_info = run->s.regs.gprs[reg];
> +
> +	kvm_vm_free(vm);
> +
> +	return diag318_info;
> +}
> +
> +uint64_t get_diag318_info(void)
> +{
> +	static uint64_t diag318_info;
> +	static bool printed_skip;
> +
> +	/*
> +	 * If KVM does not support diag318, then return 0 to
> +	 * ensure tests do not break.
> +	 */
> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
> +		if (!printed_skip) {
> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "

Whitespace after .

> +				"Skipping diag318 test.\n");
> +			printed_skip = true;
> +		}
> +		return 0;
> +	}
> +
> +	/*
> +	 * If a test has previously requested the diag318 info,
> +	 * then don't bother spinning up a temporary VM again.
> +	 */
> +	if (!diag318_info)
> +		diag318_info = diag318_handler();
> +
> +	return diag318_info;
> +}
> diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
> index b143db6d8693..d0416ba94ec5 100644
> --- a/tools/testing/selftests/kvm/s390x/resets.c
> +++ b/tools/testing/selftests/kvm/s390x/resets.c
> @@ -12,6 +12,7 @@
>  
>  #include "test_util.h"
>  #include "kvm_util.h"
> +#include "diag318_test_handler.h"
>  
>  #define VCPU_ID 3
>  #define LOCAL_IRQS 32
> @@ -110,6 +111,8 @@ static void assert_clear(void)
>  
>  	TEST_ASSERT(!memcmp(sync_regs->vrs, regs_null, sizeof(sync_regs->vrs)),
>  		    "vrs0-15 == 0 (sync_regs)");
> +
> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>  }
>  
>  static void assert_initial_noclear(void)
> @@ -182,6 +185,7 @@ static void assert_normal(void)
>  	test_one_reg(KVM_REG_S390_PFTOKEN, KVM_S390_PFAULT_TOKEN_INVALID);
>  	TEST_ASSERT(sync_regs->pft == KVM_S390_PFAULT_TOKEN_INVALID,
>  			"pft == 0xff.....  (sync_regs)");
> +	TEST_ASSERT(sync_regs->diag318 == 0, "diag318 == 0 (sync_regs)");
>  	assert_noirq();
>  }
>  
> @@ -206,6 +210,7 @@ static void test_normal(void)
>  	/* Create VM */
>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>  	run = vcpu_state(vm, VCPU_ID);
> +	run->s.regs.diag318 = get_diag318_info();
>  	sync_regs = &run->s.regs;
>  
>  	vcpu_run(vm, VCPU_ID);
> @@ -250,6 +255,7 @@ static void test_clear(void)
>  	pr_info("Testing clear reset\n");
>  	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
>  	run = vcpu_state(vm, VCPU_ID);
> +	run->s.regs.diag318 = get_diag318_info();
>  	sync_regs = &run->s.regs;
>  
>  	vcpu_run(vm, VCPU_ID);
> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> index 5731ccf34917..caf7b8859a94 100644
> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> @@ -20,6 +20,7 @@
>  
>  #include "test_util.h"
>  #include "kvm_util.h"
> +#include "diag318_test_handler.h"
>  
>  #define VCPU_ID 5
>  
> @@ -70,7 +71,7 @@ static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
>  
>  #undef REG_COMPARE
>  
> -#define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS)
> +#define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS|KVM_SYNC_DIAG318)
>  #define INVALID_SYNC_FIELD 0x80000000
>  
>  int main(int argc, char *argv[])
> @@ -152,6 +153,12 @@ int main(int argc, char *argv[])
>  
>  	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>  	run->kvm_dirty_regs = KVM_SYNC_GPRS | KVM_SYNC_ACRS;
> +
> +	if (get_diag318_info() > 0) {
> +		run->s.regs.diag318 = get_diag318_info();
> +		run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +	}
> +
>  	rv = _vcpu_run(vm, VCPU_ID);
>  	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
>  	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
> @@ -164,6 +171,9 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(run->s.regs.acrs[0]  == 1 << 11,
>  		    "acr0 sync regs value incorrect 0x%x.",
>  		    run->s.regs.acrs[0]);
> +	TEST_ASSERT(run->s.regs.diag318 == get_diag318_info(),
> +		    "diag318 sync regs value incorrect 0x%llx.",
> +		    run->s.regs.diag318);
>  
>  	vcpu_regs_get(vm, VCPU_ID, &regs);
>  	compare_regs(&regs, &run->s.regs);
> @@ -177,6 +187,7 @@ int main(int argc, char *argv[])
>  	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>  	run->kvm_dirty_regs = 0;
>  	run->s.regs.gprs[11] = 0xDEADBEEF;
> +	run->s.regs.diag318 = 0x4B1D;
>  	rv = _vcpu_run(vm, VCPU_ID);
>  	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
>  	TEST_ASSERT(run->exit_reason == KVM_EXIT_S390_SIEIC,
> @@ -186,6 +197,9 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(run->s.regs.gprs[11] != 0xDEADBEEF,
>  		    "r11 sync regs value incorrect 0x%llx.",
>  		    run->s.regs.gprs[11]);
> +	TEST_ASSERT(run->s.regs.diag318 != 0x4B1D,
> +		    "diag318 sync regs value incorrect 0x%llx.",
> +		    run->s.regs.diag318);
>  
>  	kvm_vm_free(vm);
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Thomas Huth]

On 15/10/2020 14.40, Janosch Frank wrote:
> On 10/14/20 9:27 PM, Collin Walling wrote:
>> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
>> that must be intercepted via SIE, handled in userspace, and the
>> information set by the instruction is communicated back to KVM.
> 
> It might be nice to have a few words in here about what information can
> be set via the diag.
> 
>>
>> To test the instruction interception, an ad-hoc handler is defined which
>> simply has a VM execute the instruction and then userspace will extract
>> the necessary info. The handler is defined such that the instruction
>> invocation occurs only once. It is up the the caller to determine how the
>> info returned by this handler should be used.
>>
>> The diag318 info is communicated from userspace to KVM via a sync_regs
>> call. This is tested during a sync_regs test, where the diag318 info is
>> requested via the handler, then the info is stored in the appropriate
>> register in KVM via a sync registers call.
>>
>> The diag318 info is checked to be 0 after a normal and clear reset.
>>
>> If KVM does not support diag318, then the tests will print a message
>> stating that diag318 was skipped, and the asserts will simply test
>> against a value of 0.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Checkpatch throws lots of errors on this patch.
> Could you check if my workflow misteriously introduced windows line
> endings or if they were introduced on your side?

How did you feed the patch into checkpatch? IIRC mails are often sent with
CR-LF line endings by default - it's "git am" that is converting the line
endings back to the Unix default. So for running a patch through checkpatch,
you might need to do "git am" first and then export it again.

>> +uint64_t get_diag318_info(void)
>> +{
>> +	static uint64_t diag318_info;
>> +	static bool printed_skip;
>> +
>> +	/*
>> +	 * If KVM does not support diag318, then return 0 to
>> +	 * ensure tests do not break.
>> +	 */
>> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
>> +		if (!printed_skip) {
>> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
> 
> Whitespace after .
> 
>> +				"Skipping diag318 test.\n");

It's a multi-line text, so the whitespace is needed, isn't it?

>> +			printed_skip = true;
>> +		}
>> +		return 0;
>> +	}

 Thomas

Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]

On 10/15/20 3:56 PM, Thomas Huth wrote:
> On 15/10/2020 14.40, Janosch Frank wrote:
>> On 10/14/20 9:27 PM, Collin Walling wrote:
>>> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
>>> that must be intercepted via SIE, handled in userspace, and the
>>> information set by the instruction is communicated back to KVM.
>>
>> It might be nice to have a few words in here about what information can
>> be set via the diag.
>>
>>>
>>> To test the instruction interception, an ad-hoc handler is defined which
>>> simply has a VM execute the instruction and then userspace will extract
>>> the necessary info. The handler is defined such that the instruction
>>> invocation occurs only once. It is up the the caller to determine how the
>>> info returned by this handler should be used.
>>>
>>> The diag318 info is communicated from userspace to KVM via a sync_regs
>>> call. This is tested during a sync_regs test, where the diag318 info is
>>> requested via the handler, then the info is stored in the appropriate
>>> register in KVM via a sync registers call.
>>>
>>> The diag318 info is checked to be 0 after a normal and clear reset.
>>>
>>> If KVM does not support diag318, then the tests will print a message
>>> stating that diag318 was skipped, and the asserts will simply test
>>> against a value of 0.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>
>> Checkpatch throws lots of errors on this patch.
>> Could you check if my workflow misteriously introduced windows line
>> endings or if they were introduced on your side?
> 
> How did you feed the patch into checkpatch? IIRC mails are often sent with
> CR-LF line endings by default - it's "git am" that is converting the line
> endings back to the Unix default. So for running a patch through checkpatch,
> you might need to do "git am" first and then export it again.

Uh right, that's why I asked in the first place
With git am checkpatch is a lot happier and it would be even happier if
I would have looked at the V1 and not at the RFC I reviewed on Monday...

> 
>>> +uint64_t get_diag318_info(void)
>>> +{
>>> +	static uint64_t diag318_info;
>>> +	static bool printed_skip;
>>> +
>>> +	/*
>>> +	 * If KVM does not support diag318, then return 0 to
>>> +	 * ensure tests do not break.
>>> +	 */
>>> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
>>> +		if (!printed_skip) {
>>> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
>>
>> Whitespace after .
>>
>>> +				"Skipping diag318 test.\n");
> 
> It's a multi-line text, so the whitespace is needed, isn't it?

I missed that second line as it is indented to the stdout and not to the
first string.

> 
>>> +			printed_skip = true;
>>> +		}
>>> +		return 0;
>>> +	}
> 
>  Thomas
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]