[v4] self_tests/kvm: sync_regs test for diag318

Commit Message

Collin Walling Dec. 7, 2020, 3:41 p.m. UTC

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

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 <[e-mail]>
---
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
 .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
 .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
 4 files changed, 111 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

Comments

Collin Walling Dec. 7, 2020, 3:43 p.m. UTC | #1

On 12/7/20 10:41 AM, 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 to 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.
> 
> 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 <[e-mail]>

Please add

Acked-by: Janosch Frank <[e-mail]>

[...]

Cornelia Huck Dec. 7, 2020, 4:55 p.m. UTC | #2

On Mon,  7 Dec 2020 10:41:25 -0500
Collin Walling <[e-mail]> 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 to 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

s/During/during/

> requested via the handler, then the info is stored in the appropriate
> register in KVM via a sync registers call.
> 
> 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 <[e-mail]>
> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>  .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>  4 files changed, 111 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

Looks reasonable to me.

Acked-by: Cornelia Huck <[e-mail]>

Collin Walling Dec. 7, 2020, 7:09 p.m. UTC | #3

On 12/7/20 11:55 AM, Cornelia Huck wrote:
> On Mon,  7 Dec 2020 10:41:25 -0500
> Collin Walling <[e-mail]> 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 to 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
> 
> s/During/during/
> 
>> requested via the handler, then the info is stored in the appropriate
>> register in KVM via a sync registers call.
>>
>> 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 <[e-mail]>
>> ---
>>  tools/testing/selftests/kvm/Makefile          |  2 +-
>>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>>  .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
>>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>>  4 files changed, 111 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
> 
> Looks reasonable to me.
> 
> Acked-by: Cornelia Huck <[e-mail]>
> 

Thanks. I'll resubmit the entire patch as a reply to the top-most email
with the proposed commit message changes.

Collin Walling Dec. 7, 2020, 7:12 p.m. UTC | #4

The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged
instruction 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 to 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.

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 <[e-mail]>
Acked-by: Janosch Frank <[e-mail]>
Acked-by: Cornelia Huck <[e-mail]>
---

@Christian: if you haven't picked the patch yet, here is the entire
patch with the properly updated commit message.

---
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
 .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
 .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
 4 files changed, 111 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 3d14ef77755e..426c78449044 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 lib/x86_64/handlers.S
 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..b0ed71302722
--- /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..86b9e611ad87
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
@@ -0,0 +1,82 @@
+// 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	6
+
+#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];
+
+	TEST_ASSERT(diag318_info != 0, "DIAGNOSE 0x0318 info not set");
+
+	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/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);

Christian Borntraeger Dec. 7, 2020, 7:14 p.m. UTC | #5

On 07.12.20 20:12, Collin Walling wrote:
> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged
> instruction 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 to 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.
> 
> 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 <[e-mail]>
> Acked-by: Janosch Frank <[e-mail]>
> Acked-by: Cornelia Huck <[e-mail]>
> ---
> 
> @Christian: if you haven't picked the patch yet, here is the entire
> patch with the properly updated commit message.

No worries, I already picked and fixed this.

Reviewed-by: Christian Borntraeger <[e-mail]>

thanks applied. 

> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>  .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>  4 files changed, 111 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 3d14ef77755e..426c78449044 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 lib/x86_64/handlers.S
>  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..b0ed71302722
> --- /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..86b9e611ad87
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> @@ -0,0 +1,82 @@
> +// 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	6
> +
> +#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];
> +
> +	TEST_ASSERT(diag318_info != 0, "DIAGNOSE 0x0318 info not set");
> +
> +	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/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);
>

Christian Borntraeger Dec. 7, 2020, 7:32 p.m. UTC | #6

On 07.12.20 16:41, 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 to 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.
> 
> 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 <[e-mail]>

Interestingly enough, this testcase actually trigger a bug:
While we gracefully handle this (no crash)
debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
is certainly not ideal....


> ---
>  tools/testing/selftests/kvm/Makefile          |  2 +-
>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>  .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>  4 files changed, 111 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 3d14ef77755e..426c78449044 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 lib/x86_64/handlers.S
>  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..b0ed71302722
> --- /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..86b9e611ad87
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
> @@ -0,0 +1,82 @@
> +// 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	6
> +
> +#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];
> +
> +	TEST_ASSERT(diag318_info != 0, "DIAGNOSE 0x0318 info not set");
> +
> +	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/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);
>

Collin Walling Dec. 7, 2020, 8:06 p.m. UTC | #7

On 12/7/20 2:32 PM, Christian Borntraeger wrote:
> On 07.12.20 16:41, 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 to 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.
>>
>> 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 <[e-mail]>
> 
> Interestingly enough, this testcase actually trigger a bug:
> While we gracefully handle this (no crash)
> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
> is certainly not ideal....
> 

Odd... I wonder what triggered this behavior?

I run my tests with a simple command:

make summary=0 TARGETS=kvm kselftest

This must have something to do with spinning up another VM to get the
diag318 data. I think if I have the sync_regs test call the diag handler
first, and then have the sync regs create a VM, that might solve that
issue...

May I ask how you encountered this bug so I may replicate in on my end?

> 
>> ---
>>  tools/testing/selftests/kvm/Makefile          |  2 +-
>>  .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
>>  .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
>>  .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
>>  4 files changed, 111 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 3d14ef77755e..426c78449044 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 lib/x86_64/handlers.S
>>  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..b0ed71302722
>> --- /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..86b9e611ad87
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
>> @@ -0,0 +1,82 @@
>> +// 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	6
>> +
>> +#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];
>> +
>> +	TEST_ASSERT(diag318_info != 0, "DIAGNOSE 0x0318 info not set");
>> +
>> +	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/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);
>>

Christian Borntraeger Dec. 7, 2020, 8:09 p.m. UTC | #8

On 07.12.20 21:06, Collin Walling wrote:
> On 12/7/20 2:32 PM, Christian Borntraeger wrote:
>> On 07.12.20 16:41, 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 to 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.
>>>
>>> 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 <[e-mail]>
>>
>> Interestingly enough, this testcase actually trigger a bug:
>> While we gracefully handle this (no crash)
>> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
>> is certainly not ideal....
>>
> 
> Odd... I wonder what triggered this behavior?
> 
> I run my tests with a simple command:
> 
> make summary=0 TARGETS=kvm kselftest
> 
> This must have something to do with spinning up another VM to get the
> diag318 data. I think if I have the sync_regs test call the diag handler
> first, and then have the sync regs create a VM, that might solve that
> issue...

Yes, the s390dbf code will try to create a file named kvm-%pid. With
2 VMs the 2nd one fails. Luckily the kvm will be created anyway and 
also the shutdown seems to be fine, still....

> 
> May I ask how you encountered this bug so I may replicate in on my end?

I just did
make TARGETS=kvm selftests

and then the error is on dmesg.

Collin Walling Dec. 7, 2020, 8:13 p.m. UTC | #9

On 12/7/20 3:09 PM, Christian Borntraeger wrote:
> 
> 
> On 07.12.20 21:06, Collin Walling wrote:
>> On 12/7/20 2:32 PM, Christian Borntraeger wrote:
>>> On 07.12.20 16:41, 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 to 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.
>>>>
>>>> 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 <[e-mail]>
>>>
>>> Interestingly enough, this testcase actually trigger a bug:
>>> While we gracefully handle this (no crash)
>>> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
>>> is certainly not ideal....
>>>
>>
>> Odd... I wonder what triggered this behavior?
>>
>> I run my tests with a simple command:
>>
>> make summary=0 TARGETS=kvm kselftest
>>
>> This must have something to do with spinning up another VM to get the
>> diag318 data. I think if I have the sync_regs test call the diag handler
>> first, and then have the sync regs create a VM, that might solve that
>> issue...
> 
> Yes, the s390dbf code will try to create a file named kvm-%pid. With
> 2 VMs the 2nd one fails. Luckily the kvm will be created anyway and 
> also the shutdown seems to be fine, still....
> 
>>
>> May I ask how you encountered this bug so I may replicate in on my end?
> 
> I just did
> make TARGETS=kvm selftests
> 
> and then the error is on dmesg.
> 

Thanks. v5 with fix incoming.

Christian Borntraeger Dec. 7, 2020, 8:16 p.m. UTC | #10

On 07.12.20 21:13, Collin Walling wrote:
> On 12/7/20 3:09 PM, Christian Borntraeger wrote:
>>
>>
>> On 07.12.20 21:06, Collin Walling wrote:
>>> On 12/7/20 2:32 PM, Christian Borntraeger wrote:
>>>> On 07.12.20 16:41, 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 to 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.
>>>>>
>>>>> 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 <[e-mail]>
>>>>
>>>> Interestingly enough, this testcase actually trigger a bug:
>>>> While we gracefully handle this (no crash)
>>>> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
>>>> is certainly not ideal....
>>>>
>>>
>>> Odd... I wonder what triggered this behavior?
>>>
>>> I run my tests with a simple command:
>>>
>>> make summary=0 TARGETS=kvm kselftest
>>>
>>> This must have something to do with spinning up another VM to get the
>>> diag318 data. I think if I have the sync_regs test call the diag handler
>>> first, and then have the sync regs create a VM, that might solve that
>>> issue...
>>
>> Yes, the s390dbf code will try to create a file named kvm-%pid. With
>> 2 VMs the 2nd one fails. Luckily the kvm will be created anyway and 
>> also the shutdown seems to be fine, still....
>>
>>>
>>> May I ask how you encountered this bug so I may replicate in on my end?
>>
>> I just did
>> make TARGETS=kvm selftests
>>
>> and then the error is on dmesg.
>>
> 
> Thanks. v5 with fix incoming.

I think the test is actually fine and we should rather fix the kvm module to
gracefully handle a userspace that starts up 2 or more guests.

Collin Walling Dec. 7, 2020, 8:59 p.m. UTC | #11

On 12/7/20 3:16 PM, Christian Borntraeger wrote:
> 
> 
> On 07.12.20 21:13, Collin Walling wrote:
>> On 12/7/20 3:09 PM, Christian Borntraeger wrote:
>>>
>>>
>>> On 07.12.20 21:06, Collin Walling wrote:
>>>> On 12/7/20 2:32 PM, Christian Borntraeger wrote:
>>>>> On 07.12.20 16:41, 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 to 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.
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>
>>>>> Interestingly enough, this testcase actually trigger a bug:
>>>>> While we gracefully handle this (no crash)
>>>>> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
>>>>> is certainly not ideal....
>>>>>
>>>>
>>>> Odd... I wonder what triggered this behavior?
>>>>
>>>> I run my tests with a simple command:
>>>>
>>>> make summary=0 TARGETS=kvm kselftest
>>>>
>>>> This must have something to do with spinning up another VM to get the
>>>> diag318 data. I think if I have the sync_regs test call the diag handler
>>>> first, and then have the sync regs create a VM, that might solve that
>>>> issue...
>>>
>>> Yes, the s390dbf code will try to create a file named kvm-%pid. With
>>> 2 VMs the 2nd one fails. Luckily the kvm will be created anyway and 
>>> also the shutdown seems to be fine, still....
>>>
>>>>
>>>> May I ask how you encountered this bug so I may replicate in on my end?
>>>
>>> I just did
>>> make TARGETS=kvm selftests
>>>
>>> and then the error is on dmesg.
>>>
>>
>> Thanks. v5 with fix incoming.
> 
> I think the test is actually fine and we should rather fix the kvm module to
> gracefully handle a userspace that starts up 2 or more guests.
> 

Looks like the root cause is within the inode.c file used for the debug
filesystem. Essentially, the 2nd VM starts / ends just fine as you
mentioned, but doesn't get a dbfs.

Since this touches the dbfs related area, and I'm unsure how common this
problem is out in the wild, should we ping the kernel list and see if it
catches anyone's attention?

A first thought would be to append a per-userspace incrementing value to
the end of the kvm-%pid part to account for any collisions, but that's
up to the folks that know more than I do.

Janosch Frank Dec. 8, 2020, 10:09 a.m. UTC | #12

On 12/7/20 9:59 PM, Collin Walling wrote:
> On 12/7/20 3:16 PM, Christian Borntraeger wrote:
>>
>>
>> On 07.12.20 21:13, Collin Walling wrote:
>>> On 12/7/20 3:09 PM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 07.12.20 21:06, Collin Walling wrote:
>>>>> On 12/7/20 2:32 PM, Christian Borntraeger wrote:
>>>>>> On 07.12.20 16:41, 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 to 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.
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>
>>>>>> Interestingly enough, this testcase actually trigger a bug:
>>>>>> While we gracefully handle this (no crash)
>>>>>> debugfs: Directory 'kvm-200206' with parent 's390dbf' already present!
>>>>>> is certainly not ideal....
>>>>>>
>>>>>
>>>>> Odd... I wonder what triggered this behavior?
>>>>>
>>>>> I run my tests with a simple command:
>>>>>
>>>>> make summary=0 TARGETS=kvm kselftest
>>>>>
>>>>> This must have something to do with spinning up another VM to get the
>>>>> diag318 data. I think if I have the sync_regs test call the diag handler
>>>>> first, and then have the sync regs create a VM, that might solve that
>>>>> issue...
>>>>
>>>> Yes, the s390dbf code will try to create a file named kvm-%pid. With
>>>> 2 VMs the 2nd one fails. Luckily the kvm will be created anyway and 
>>>> also the shutdown seems to be fine, still....
>>>>
>>>>>
>>>>> May I ask how you encountered this bug so I may replicate in on my end?
>>>>
>>>> I just did
>>>> make TARGETS=kvm selftests
>>>>
>>>> and then the error is on dmesg.
>>>>
>>>
>>> Thanks. v5 with fix incoming.
>>
>> I think the test is actually fine and we should rather fix the kvm module to
>> gracefully handle a userspace that starts up 2 or more guests.
>>
> 
> Looks like the root cause is within the inode.c file used for the debug
> filesystem. Essentially, the 2nd VM starts / ends just fine as you
> mentioned, but doesn't get a dbfs.
> 
> Since this touches the dbfs related area, and I'm unsure how common this
> problem is out in the wild, should we ping the kernel list and see if it
> catches anyone's attention?
> 
> A first thought would be to append a per-userspace incrementing value to
> the end of the kvm-%pid part to account for any collisions, but that's
> up to the folks that know more than I do.
> 

Seems like I have a deja-vu, I remember having to fix that for the
common code :-)
For the KVM debugfs I solved that by appending the file descriptor after
the pid. Maybe we can steal the string from the dentry.

Have a look into vm_create_vm_debugfs(struct kvm *kvm, int fd).

Patch

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 3d14ef77755e..426c78449044 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 lib/x86_64/handlers.S
 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..b0ed71302722
--- /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..86b9e611ad87
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/s390x/diag318_test_handler.c
@@ -0,0 +1,82 @@ 
+// 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	6
+
+#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];
+
+	TEST_ASSERT(diag318_info != 0, "DIAGNOSE 0x0318 info not set");
+
+	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/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);