[PATCH v2 0/2] DIAG 318 tests and fix

[PATCH v2 0/2] DIAG 318 tests and fix

[Collin Walling]

Two patches: one that fixes a case where the DIAG 318 info was not 
actually being reset during load normal, and another patch to inroduce
selftests for DIAG 318 -- which helped discover the aforementioned issue ;)

Collin Walling (2):
  s390/kvm: fix diag318 reset
  self_tests/kvm: sync_regs and reset tests for diag318

 arch/s390/kvm/kvm-s390.c                      |  2 +-
 tools/testing/selftests/kvm/Makefile          |  2 +-
 .../kvm/include/s390x/diag318_test_handler.h  | 13 +++
 .../kvm/lib/s390x/diag318_test_handler.c      | 82 +++++++++++++++++++
 tools/testing/selftests/kvm/s390x/resets.c    | 14 ++++
 .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
 6 files changed, 126 insertions(+), 3 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

-- 
2.26.2

[PATCH v2 1/2] s390/kvm: fix diag318 reset

[Collin Walling]

The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
reset. However, this was missed for the clear reset case.

Let's fix this by resetting the information during a normal reset. 
Since clear reset is a superset of normal reset, the info will
still reset on a clear reset.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6b74b92c1a58..b0cf8367e261 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
 	vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI;
 	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
 	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
+	vcpu->run->s.regs.diag318 = 0;
 
 	kvm_clear_async_pf_completion_queue(vcpu);
 	if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
@@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
 
 	regs->etoken = 0;
 	regs->etoken_extension = 0;
-	regs->diag318 = 0;
 }
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
-- 
2.26.2

[PATCH v2 2/2] 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.

This information relates to a Control Program Name & Version code that
helps diagnose the environment a guest is running in.

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      | 82 +++++++++++++++++++
 tools/testing/selftests/kvm/s390x/resets.c    | 14 ++++
 .../selftests/kvm/s390x/sync_regs_test.c      | 16 +++-
 5 files changed, 125 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..1e0b766efeb7
--- /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	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];
+
+	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/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
index b143db6d8693..d0853cebc402 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();
 }
 
@@ -208,6 +212,11 @@ static void test_normal(void)
 	run = vcpu_state(vm, VCPU_ID);
 	sync_regs = &run->s.regs;
 
+	if (get_diag318_info() > 0) {
+		run->s.regs.diag318 = get_diag318_info();
+		run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+	}
+
 	vcpu_run(vm, VCPU_ID);
 
 	inject_irq(VCPU_ID);
@@ -252,6 +261,11 @@ static void test_clear(void)
 	run = vcpu_state(vm, VCPU_ID);
 	sync_regs = &run->s.regs;
 
+	if (get_diag318_info() > 0) {
+		run->s.regs.diag318 = get_diag318_info();
+		run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+	}
+
 	vcpu_run(vm, VCPU_ID);
 
 	inject_irq(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 v2 1/2] s390/kvm: fix diag318 reset

[Janosch Frank]

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

On 10/15/20 9:59 PM, Collin Walling wrote:
> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
> reset. However, this was missed for the clear reset case.
> 
> Let's fix this by resetting the information during a normal reset. 
> Since clear reset is a superset of normal reset, the info will
> still reset on a clear reset.

The architecture really confuses me here but I think we don't want this
in the kernel VCPU reset handlers at all.

This needs to be reset per VM *NOT* per VCPU.
Hence the resets are bound to diag308 and not SIGP.

I.e. we need to clear it in QEMU's VM reset handler.
It's still early and I have yet to consume my first coffee, am I missing
something?

> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6b74b92c1a58..b0cf8367e261 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>  	vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI;
>  	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>  	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
> +	vcpu->run->s.regs.diag318 = 0;
>  
>  	kvm_clear_async_pf_completion_queue(vcpu);
>  	if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>  
>  	regs->etoken = 0;
>  	regs->etoken_extension = 0;
> -	regs->diag318 = 0;
>  }
>  
>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> 



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

Re: [PATCH v2 1/2] s390/kvm: fix diag318 reset

[Christian Borntraeger]

On 16.10.20 09:34, Janosch Frank wrote:
> On 10/15/20 9:59 PM, Collin Walling wrote:
>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
>> reset. However, this was missed for the clear reset case.
>>
>> Let's fix this by resetting the information during a normal reset. 
>> Since clear reset is a superset of normal reset, the info will
>> still reset on a clear reset.
> 
> The architecture really confuses me here but I think we don't want this
> in the kernel VCPU reset handlers at all.
> 
> This needs to be reset per VM *NOT* per VCPU.
> Hence the resets are bound to diag308 and not SIGP.
> 
> I.e. we need to clear it in QEMU's VM reset handler.
> It's still early and I have yet to consume my first coffee, am I missing
> something?

I agree with Janosch. architecture indicates that this should only be reset
for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned
to NOT reset the value.

> 
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 6b74b92c1a58..b0cf8367e261 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>  	vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI;
>>  	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>>  	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
>> +	vcpu->run->s.regs.diag318 = 0;
>>  
>>  	kvm_clear_async_pf_completion_queue(vcpu);
>>  	if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>>  
>>  	regs->etoken = 0;
>>  	regs->etoken_extension = 0;
>> -	regs->diag318 = 0;
>>  }
>>  
>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>
> 
> 

Re: [PATCH v2 1/2] s390/kvm: fix diag318 reset

[Collin Walling]

On 10/16/20 3:44 AM, Christian Borntraeger wrote:
> 
> 
> On 16.10.20 09:34, Janosch Frank wrote:
>> On 10/15/20 9:59 PM, Collin Walling wrote:
>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
>>> reset. However, this was missed for the clear reset case.
>>>
>>> Let's fix this by resetting the information during a normal reset. 
>>> Since clear reset is a superset of normal reset, the info will
>>> still reset on a clear reset.
>>
>> The architecture really confuses me here but I think we don't want this
>> in the kernel VCPU reset handlers at all.
>>
>> This needs to be reset per VM *NOT* per VCPU.
>> Hence the resets are bound to diag308 and not SIGP.
>>
>> I.e. we need to clear it in QEMU's VM reset handler.
>> It's still early and I have yet to consume my first coffee, am I missing
>> something?
> 
> I agree with Janosch. architecture indicates that this should only be reset
> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned
> to NOT reset the value.
> 

A few questions regarding how resets for diag318 should work here:

The AR states that any copies retained by the diag318 should be set to 0
on a clear reset and load normal -- I thought both of those resets were
implicitly called by diag308 as well?

Should the register used to store diag318 info not be set to 0 *by KVM*
then? Should the values be set *by QEMU* and a subsequent sync_regs will
ensure things are sane on the KVM side?

>>
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>  arch/s390/kvm/kvm-s390.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 6b74b92c1a58..b0cf8367e261 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>>  	vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI;
>>>  	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>>>  	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
>>> +	vcpu->run->s.regs.diag318 = 0;
>>>  
>>>  	kvm_clear_async_pf_completion_queue(vcpu);
>>>  	if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>>> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>>>  
>>>  	regs->etoken = 0;
>>>  	regs->etoken_extension = 0;
>>> -	regs->diag318 = 0;
>>>  }
>>>  
>>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>
>>
>>


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v2 1/2] s390/kvm: fix diag318 reset

[Janosch Frank]

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

On 10/22/20 3:43 PM, Collin Walling wrote:
> On 10/16/20 3:44 AM, Christian Borntraeger wrote:
>>
>>
>> On 16.10.20 09:34, Janosch Frank wrote:
>>> On 10/15/20 9:59 PM, Collin Walling wrote:
>>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
>>>> reset. However, this was missed for the clear reset case.
>>>>
>>>> Let's fix this by resetting the information during a normal reset. 
>>>> Since clear reset is a superset of normal reset, the info will
>>>> still reset on a clear reset.
>>>
>>> The architecture really confuses me here but I think we don't want this
>>> in the kernel VCPU reset handlers at all.
>>>
>>> This needs to be reset per VM *NOT* per VCPU.
>>> Hence the resets are bound to diag308 and not SIGP.
>>>
>>> I.e. we need to clear it in QEMU's VM reset handler.
>>> It's still early and I have yet to consume my first coffee, am I missing
>>> something?
>>
>> I agree with Janosch. architecture indicates that this should only be reset
>> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned
>> to NOT reset the value.
>>
> 
> A few questions regarding how resets for diag318 should work here:
> 
> The AR states that any copies retained by the diag318 should be set to 0
> on a clear reset and load normal -- I thought both of those resets were
> implicitly called by diag308 as well?
> 
> Should the register used to store diag318 info not be set to 0 *by KVM*
> then? Should the values be set *by QEMU* and a subsequent sync_regs will
> ensure things are sane on the KVM side?

Just a FYI for the non-IBMers reading in:

I have spoken to the author of the architecture and cleared up our way
forward.

* We need to clear on diag 308 subcodes 0,1,3,4
* SIGP does not alter diag318 data in any way
* We need to set the cpnc on all VCPUs of the VM


> 
>>>
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>  arch/s390/kvm/kvm-s390.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 6b74b92c1a58..b0cf8367e261 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>>>>  	vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI;
>>>>  	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>>>>  	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
>>>> +	vcpu->run->s.regs.diag318 = 0;
>>>>  
>>>>  	kvm_clear_async_pf_completion_queue(vcpu);
>>>>  	if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>>>> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>>>>  
>>>>  	regs->etoken = 0;
>>>>  	regs->etoken_extension = 0;
>>>> -	regs->diag318 = 0;
>>>>  }
>>>>  
>>>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>>
>>>
>>>
> 
> 



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

Re: [PATCH v2 1/2] s390/kvm: fix diag318 reset

[Collin Walling]

On 10/23/20 3:25 AM, Janosch Frank wrote:
> On 10/22/20 3:43 PM, Collin Walling wrote:
>> On 10/16/20 3:44 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 16.10.20 09:34, Janosch Frank wrote:
>>>> On 10/15/20 9:59 PM, Collin Walling wrote:
>>>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
>>>>> reset. However, this was missed for the clear reset case.
>>>>>
>>>>> Let's fix this by resetting the information during a normal reset. 
>>>>> Since clear reset is a superset of normal reset, the info will
>>>>> still reset on a clear reset.
>>>>
>>>> The architecture really confuses me here but I think we don't want this
>>>> in the kernel VCPU reset handlers at all.
>>>>
>>>> This needs to be reset per VM *NOT* per VCPU.
>>>> Hence the resets are bound to diag308 and not SIGP.
>>>>
>>>> I.e. we need to clear it in QEMU's VM reset handler.
>>>> It's still early and I have yet to consume my first coffee, am I missing
>>>> something?
>>>
>>> I agree with Janosch. architecture indicates that this should only be reset
>>> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned
>>> to NOT reset the value.
>>>
>>
>> A few questions regarding how resets for diag318 should work here:
>>
>> The AR states that any copies retained by the diag318 should be set to 0
>> on a clear reset and load normal -- I thought both of those resets were
>> implicitly called by diag308 as well?
>>
>> Should the register used to store diag318 info not be set to 0 *by KVM*
>> then? Should the values be set *by QEMU* and a subsequent sync_regs will
>> ensure things are sane on the KVM side?
> 
> Just a FYI for the non-IBMers reading in:
> 
> I have spoken to the author of the architecture and cleared up our way
> forward.
>
> * We need to clear on diag 308 subcodes 0,1,3,4
> * SIGP does not alter diag318 data in any way

Just to make sure I fully understand the changes required here (since
the wording in the documentation is a bit tricky) -- this means I need
to remove the kvm_arch_vcpu_ioctl_*_reset code for the diag318 data and
instead implement a way for this to be reset via 308 only?

If true, then resetting this data becomes tricky with the current
implementation...

> * We need to set the cpnc on all VCPUs of the VM
> 
> 

This begs a few questions about the current design of the diag318 code.

Since the diag318 info (CPNC and CPVC) are *attributes of the
configuration*, then is the current implementation of the diag318
handling incorrect?

Right now, the diag318 data is synced per-vcpu via a sync_regs call, and
only the CPU involved in the instruction interception will have its
diag318 register synced. However, if we shouldn't use the kvm_vcpu_ioctl
functions to reset the diag318 data, then does it make sense to use the
sync_regs interface?

There are two approaches I can think of:

1. Modify the current implementation by "dirtying all the VCPU's
   registers" -- this will ensure the CPNC is set in all SIE blocks.

2. Fallback to the old implementation of this item and use an ioctl
   (only for setting the data)

   This new ioctl can be utilized to reset the diag318 data (including
   the copy retained by QEMU for migration) during the respective 308
   resets.

Any insights before I fall down the wrong rabbit hole?

[...]

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v2 1/2] s390/kvm: fix diag318 reset

[Janosch Frank]

[-- Attachment #1.1.1: Type: text/plain, Size: 4217 bytes --]

On 10/28/20 7:06 AM, Collin Walling wrote:
> On 10/23/20 3:25 AM, Janosch Frank wrote:
>> On 10/22/20 3:43 PM, Collin Walling wrote:
>>> On 10/16/20 3:44 AM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 16.10.20 09:34, Janosch Frank wrote:
>>>>> On 10/15/20 9:59 PM, Collin Walling wrote:
>>>>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear
>>>>>> reset. However, this was missed for the clear reset case.
>>>>>>
>>>>>> Let's fix this by resetting the information during a normal reset. 
>>>>>> Since clear reset is a superset of normal reset, the info will
>>>>>> still reset on a clear reset.
>>>>>
>>>>> The architecture really confuses me here but I think we don't want this
>>>>> in the kernel VCPU reset handlers at all.
>>>>>
>>>>> This needs to be reset per VM *NOT* per VCPU.
>>>>> Hence the resets are bound to diag308 and not SIGP.
>>>>>
>>>>> I.e. we need to clear it in QEMU's VM reset handler.
>>>>> It's still early and I have yet to consume my first coffee, am I missing
>>>>> something?
>>>>
>>>> I agree with Janosch. architecture indicates that this should only be reset
>>>> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned
>>>> to NOT reset the value.
>>>>
>>>
>>> A few questions regarding how resets for diag318 should work here:
>>>
>>> The AR states that any copies retained by the diag318 should be set to 0
>>> on a clear reset and load normal -- I thought both of those resets were
>>> implicitly called by diag308 as well?
>>>
>>> Should the register used to store diag318 info not be set to 0 *by KVM*
>>> then? Should the values be set *by QEMU* and a subsequent sync_regs will
>>> ensure things are sane on the KVM side?
>>
>> Just a FYI for the non-IBMers reading in:
>>
>> I have spoken to the author of the architecture and cleared up our way
>> forward.
>>
>> * We need to clear on diag 308 subcodes 0,1,3,4
>> * SIGP does not alter diag318 data in any way
> 
> Just to make sure I fully understand the changes required here (since
> the wording in the documentation is a bit tricky) -- this means I need
> to remove the kvm_arch_vcpu_ioctl_*_reset code for the diag318 data and
> instead implement a way for this to be reset via 308 only?

Yes

> 
> If true, then resetting this data becomes tricky with the current
> implementation...

Not really, it's just a bit of a pain to iterate through all VCPUs

> 
>> * We need to set the cpnc on all VCPUs of the VM
>>
>>
> 
> This begs a few questions about the current design of the diag318 code.
> 
> Since the diag318 info (CPNC and CPVC) are *attributes of the
> configuration*, then is the current implementation of the diag318
> handling incorrect?
> 
> Right now, the diag318 data is synced per-vcpu via a sync_regs call, and
> only the CPU involved in the instruction interception will have its
> diag318 register synced. However, if we shouldn't use the kvm_vcpu_ioctl
> functions to reset the diag318 data, then does it make sense to use the
> sync_regs interface?
> 
> There are two approaches I can think of:
> 
> 1. Modify the current implementation by "dirtying all the VCPU's
>    registers" -- this will ensure the CPNC is set in all SIE blocks.
> 
> 2. Fallback to the old implementation of this item and use an ioctl
>    (only for setting the data)
> 
>    This new ioctl can be utilized to reset the diag318 data (including
>    the copy retained by QEMU for migration) during the respective 308
>    resets.

I don't think that we can tear down old IOCTLs that easily, we have to
keep the API consistent.

Luckily a diag 308 reset also means VCPU resets, so all VCPUs are out of
SIE and QEMU will change a lot of the sync reg contents anyway.

The tricky part is the instruction itself as synchronizing the CPNC
change to all VCPUs will not be easy.

I'd suggest you start with the d308 changes and I'll help you next week
after the KVM Forum

@Christian: Any other thoughts on this?


At the end of the year I'll take a day to find out how we went so wrong
with this item...

> 
> Any insights before I fall down the wrong rabbit hole?
> 
> [...]
> 


[-- Attachment #1.1.2: OpenPGP_0xE354E6B8E238B9F8.asc --]
[-- Type: application/pgp-keys, Size: 7995 bytes --]

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