Use DIAG318 to set Control Program Name & Version Codes

The diag318 struct introduced in include/asm/diag.h can be reused in KVM, so let's condense the version code fields in the diag318_info struct for easier usage and simplify it until we can determine how the data should be formatted.

Signed-off-by Collin Walling <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 arch/s390/include/asm/diag.h | 6 ++----
 arch/s390/kernel/setup.c     | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +18 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+

arch/s390/kvm/kvm-s390.c +5 -0

+	case KVM_S390_VM_MISC_DIAG318:
+		ret = -EFAULT;
+		if (!kvm->arch.use_diag318)
+			return -EOPNOTSUPP;
+		if (get_user(diag318_info, (u64 __user *)attr->addr))

The diag318 struct introduced in include/asm/diag.h can be reused in KVM, so let's condense the version code fields in the diag318_info struct for easier usage and simplify it until we can determine how the data should be formatted.

Signed-off-by Collin Walling <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

Reviewed-by Cornelia Huck <[email protected]>

---
 arch/s390/include/asm/diag.h | 6 ++----
 arch/s390/kernel/setup.c     | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

arch/s390/include/asm/diag.h +2 -4

index 0036eab14391..ca8f85b53a90 100644
@@ -298,10 +298,8 @@ struct diag26c_mac_resp {
 union diag318_info {
 	unsigned long val;
 	struct {
-		unsigned int cpnc : 8;
-		unsigned int cpvc_linux : 24;
-		unsigned char cpvc_distro[3];
-		unsigned char zero;
+		unsigned long cpnc : 8;
+		unsigned long cpvc : 56;
 	};
 };
 

arch/s390/kernel/setup.c +1 -2

index 36445dd40fdb..1aaaf11acc6b 100644
@@ -1028,8 +1028,7 @@ static void __init setup_control_program_code(void)
 {
 	union diag318_info diag318_info = {
 		.cpnc = CPNC_LINUX,
-		.cpvc_linux = 0,
-		.cpvc_distro = {0},
+		.cpvc = 0,
 	};
 
 	if (!sclp.has_diag318)

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 30 lines

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 40 lines

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +24 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1273

Quoted 4 lines

arch/s390/kvm/kvm-s390.c +4 -0

+	case KVM_S390_VM_MISC_ENABLE_DIAG318:
+		kvm->arch.use_diag318 = 1;
+		ret = 0;
+		break;

Would it make sense to set kvm->arch.use_diag318 = 1 during the first execution of KVM_S390_VM_MISC_DIAG318 instead, so that we could get along without the KVM_S390_VM_MISC_ENABLE_DIAG318 ?

Quoted 14 lines

arch/s390/kvm/kvm-s390.c +13 -0

+	case KVM_S390_VM_MISC_DIAG318:
+		ret = -EFAULT;
+		if (!kvm->arch.use_diag318)
+			return -EOPNOTSUPP;
+		if (get_user(diag318_info, (u64 __user *)attr->addr))
+			break;
+		kvm_s390_set_diag318_info(kvm, diag318_info);
+		ret = 0;
+		break;
+	default:
+		ret = -ENXIO;
+		break;
+	}

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1288

Quoted 2 lines

arch/s390/kvm/kvm-s390.c +2 -0

+	return ret;
+}

What about a reset of the guest VM? If a user first boots into a Linux kernel that supports diag 318, then reboots and selects a Linux kernel that does not support diag 318? I'd expect that the cpnc / cpnv values need to be cleared here somewhere? Otherwise the information might not be accurate anymore?

Thomas

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

Documentation/virt/kvm/devices/vm.rst +0 -0

index 0aa5b1cfd700..9344d45ace6d 100644

Documentation/virt/kvm/devices/vm.rst +11 -0

+Architectures: s390
+
+ 6.1. KVM_S390_VM_MISC_ENABLE_DIAG318
+
+ Allows userspace to enable the DIAGNOSE 0x318 instruction call for a
+ guest OS. By default, KVM will not allow this instruction to be executed
+ by a guest, even if support is in place. Userspace must explicitly enable
+ the instruction handling for DIAGNOSE 0x318 via this call.
+
+ Parameters: none
+ Returns:    0 after setting a flag telling KVM to enable this feature

Documentation/virt/kvm/devices/vm.rst +6 -0

+
+ Parameters: address of a buffer in user space (u64), where the
+	     information is read from or stored into
+ Returns:    -EFAULT if the given address is not accessible from kernel space;
+	     -EOPNOTSUPP if feature has not been requested to be enabled first;
+	     0 in case of success

Quoted 117 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 30 lines

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 40 lines

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +24 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {

Quoted 8 lines

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1273

Quoted 4 lines

arch/s390/kvm/kvm-s390.c +4 -0

+	case KVM_S390_VM_MISC_ENABLE_DIAG318:
+		kvm->arch.use_diag318 = 1;
+		ret = 0;
+		break;

Would it make sense to set kvm->arch.use_diag318 = 1 during the first execution of KVM_S390_VM_MISC_DIAG318 instead, so that we could get along without the KVM_S390_VM_MISC_ENABLE_DIAG318 ?

I'm not an expert in feature negotiation, but why isn't this a cpu feature like sief2 instead of a attribute?

@David?

Quoted 130 lines · Janosch Frank

Quoted 117 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 30 lines

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 40 lines

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +24 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {

Quoted 8 lines

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1273

Quoted 4 lines

arch/s390/kvm/kvm-s390.c +4 -0

+	case KVM_S390_VM_MISC_ENABLE_DIAG318:
+		kvm->arch.use_diag318 = 1;
+		ret = 0;
+		break;

Would it make sense to set kvm->arch.use_diag318 = 1 during the first execution of KVM_S390_VM_MISC_DIAG318 instead, so that we could get along without the KVM_S390_VM_MISC_ENABLE_DIAG318 ?

I'm not an expert in feature negotiation, but why isn't this a cpu feature like sief2 instead of a attribute?

Quoted 1 line

@David?

In the end you want to have it somehow in the CPU model I guess. You cannot glue it to QEMU machines, because availability depends on HW+KVM support.

How does the guest detect that it can use diag318? I assume/hope via a a STFLE feature.

Quoted 133 lines · David Hildenbrand

Quoted 131 lines · Janosch Frank

Quoted 117 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 30 lines

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 40 lines

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +24 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {

Quoted 8 lines

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1273

Quoted 4 lines

arch/s390/kvm/kvm-s390.c +4 -0

+	case KVM_S390_VM_MISC_ENABLE_DIAG318:
+		kvm->arch.use_diag318 = 1;
+		ret = 0;
+		break;

Would it make sense to set kvm->arch.use_diag318 = 1 during the first execution of KVM_S390_VM_MISC_DIAG318 instead, so that we could get along without the KVM_S390_VM_MISC_ENABLE_DIAG318 ?

I'm not an expert in feature negotiation, but why isn't this a cpu feature like sief2 instead of a attribute?

@David?

Quoted 7 lines

In the end you want to have it somehow in the CPU model I guess. You cannot glue it to QEMU machines, because availability depends on HW+KVM support.

How does the guest detect that it can use diag318? I assume/hope via a a STFLE feature.

SCLP

Quoted 33 lines · On Wed, 13 May 2020 18:15:57 -0400 Collin Walling <[email protected]> wrote:

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

Documentation/virt/kvm/devices/vm.rst +0 -0

index 0aa5b1cfd700..9344d45ace6d 100644

Documentation/virt/kvm/devices/vm.rst Allows userspace to query the status of migration mode. · line 318

Quoted 5 lines

Documentation/virt/kvm/devices/vm.rst +2 -0

@@ -314,3 +314,32 @@ Allows userspace to query the status of migration mode.
 	     if it is enabled
 :Returns:   -EFAULT if the given address is not accessible from kernel space;
 	    0 in case of success.
+
+6. GROUP: KVM_S390_VM_MISC

This needs to be rstyfied, matching the remainder of the file.

Quoted 11 lines

Documentation/virt/kvm/devices/vm.rst +11 -0

+Architectures: s390
+
+ 6.1. KVM_S390_VM_MISC_ENABLE_DIAG318
+
+ Allows userspace to enable the DIAGNOSE 0x318 instruction call for a
+ guest OS. By default, KVM will not allow this instruction to be executed
+ by a guest, even if support is in place. Userspace must explicitly enable
+ the instruction handling for DIAGNOSE 0x318 via this call.
+
+ Parameters: none
+ Returns:    0 after setting a flag telling KVM to enable this feature

Documentation/virt/kvm/devices/vm.rst Allows userspace to query the status of migration mode. · line 339

Quoted 10 lines

Documentation/virt/kvm/devices/vm.rst +10 -0

+
+ 6.2. KVM_S390_VM_MISC_DIAG318 (r/w)
+
+ Allows userspace to retrieve and set the DIAGNOSE 0x318 information,
+ which consists of a 1-byte "Control Program Name Code" and a 7-byte
+ "Control Program Version Code" (a 64 bit value all in all). This
+ information is set by the guest (usually during IPL). This interface is
+ intended to allow retrieving and setting it during migration; while no
+ real harm is done if the information is changed outside of migration,
+ it is strongly discouraged.

(Sorry if we discussed that already, but that was some time ago and the info has dropped out of my cache...)

Had we considered doing this in userspace only? If QEMU wanted to emulate diag 318 in tcg, it would basically need to mirror what KVM does; diag 318 does not seem like something where we want to optimize for performance, so dropping to userspace seems feasible? We'd just need an interface for userspace to forward anything set by the guest.

Quoted 6 lines

Documentation/virt/kvm/devices/vm.rst +6 -0

+
+ Parameters: address of a buffer in user space (u64), where the
+	     information is read from or stored into
+ Returns:    -EFAULT if the given address is not accessible from kernel space;
+	     -EOPNOTSUPP if feature has not been requested to be enabled first;
+	     0 in case of success

Quoted 69 lines · Cornelia Huck

Quoted 33 lines · On Wed, 13 May 2020 18:15:57 -0400 Collin Walling <[email protected]> wrote:

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

Documentation/virt/kvm/devices/vm.rst +0 -0

index 0aa5b1cfd700..9344d45ace6d 100644

Documentation/virt/kvm/devices/vm.rst Allows userspace to query the status of migration mode. · line 318

Quoted 5 lines

Documentation/virt/kvm/devices/vm.rst +2 -0

@@ -314,3 +314,32 @@ Allows userspace to query the status of migration mode.
 	     if it is enabled
 :Returns:   -EFAULT if the given address is not accessible from kernel space;
 	    0 in case of success.
+
+6. GROUP: KVM_S390_VM_MISC

This needs to be rstyfied, matching the remainder of the file.

Quoted 11 lines

Documentation/virt/kvm/devices/vm.rst +11 -0

+Architectures: s390
+
+ 6.1. KVM_S390_VM_MISC_ENABLE_DIAG318
+
+ Allows userspace to enable the DIAGNOSE 0x318 instruction call for a
+ guest OS. By default, KVM will not allow this instruction to be executed
+ by a guest, even if support is in place. Userspace must explicitly enable
+ the instruction handling for DIAGNOSE 0x318 via this call.
+
+ Parameters: none
+ Returns:    0 after setting a flag telling KVM to enable this feature

Documentation/virt/kvm/devices/vm.rst Allows userspace to query the status of migration mode. · line 339

Quoted 10 lines

Documentation/virt/kvm/devices/vm.rst +10 -0

+
+ 6.2. KVM_S390_VM_MISC_DIAG318 (r/w)
+
+ Allows userspace to retrieve and set the DIAGNOSE 0x318 information,
+ which consists of a 1-byte "Control Program Name Code" and a 7-byte
+ "Control Program Version Code" (a 64 bit value all in all). This
+ information is set by the guest (usually during IPL). This interface is
+ intended to allow retrieving and setting it during migration; while no
+ real harm is done if the information is changed outside of migration,
+ it is strongly discouraged.

(Sorry if we discussed that already, but that was some time ago and the info has dropped out of my cache...)

Quoted 6 lines

Had we considered doing this in userspace only? If QEMU wanted to emulate diag 318 in tcg, it would basically need to mirror what KVM does; diag 318 does not seem like something where we want to optimize for performance, so dropping to userspace seems feasible? We'd just need an interface for userspace to forward anything set by the guest.

My reservation with respect to handling this in userspace only is that the data set by the instruction call is relevant to both host-level and guest-level kernels. That data is set during kernel setup.

Right now, the instruction call is used to set a hard-coded "name code" value, but later we want to use this instruction to also set some sort of unique version code. The format of the version code is not yet determined.

If guest support is handled in userspace only, then we'll have to update the version codes in both the Linux kernel /and/ QEMU, which might be a bit messy if things go out of sync.

Quoted 7 lines

Quoted 6 lines

Documentation/virt/kvm/devices/vm.rst +6 -0

+
+ Parameters: address of a buffer in user space (u64), where the
+	     information is read from or stored into
+ Returns:    -EFAULT if the given address is not accessible from kernel space;
+	     -EOPNOTSUPP if feature has not been requested to be enabled first;
+	     0 in case of success

Quoted 40 lines · Janosch Frank

Quoted 39 lines · David Hildenbrand

Quoted 38 lines · Janosch Frank

Quoted 37 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 8 lines

arch/s390/kvm/diag.c +0 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/s390/kvm/diag.c +6 -0

+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/s390/kvm/diag.c static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu) · line 269

Quoted 8 lines

arch/s390/kvm/diag.c +8 -0

+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

arch/s390/kvm/diag.c +4 -0

+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 42 lines

Quoted 42 lines

Quoted 42 lines

Quoted 42 lines

Quoted 42 lines

arch/s390/kvm/diag.c +0 -0

 	default:

arch/s390/kvm/kvm-s390.c +25 -0

index d05bb040fd42..c3eee468815f 100644
@@ -159,6 +159,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "diag_9c_ignored", VCPU_STAT(diagnose_9c_ignored) },
 	{ "instruction_diag_258", VCPU_STAT(diagnose_258) },
 	{ "instruction_diag_308", VCPU_STAT(diagnose_308) },
+	{ "instruction_diag_318", VCPU_STAT(diagnose_318) },
 	{ "instruction_diag_500", VCPU_STAT(diagnose_500) },
 	{ "instruction_diag_other", VCPU_STAT(diagnose_other) },
 	{ NULL }

@@ -1243,6 +1244,76 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm->arch.diag318_info.val = info;
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, kvm->arch.diag318_info.cpvc);
+
+	if (sclp.has_diag318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag318_info.cpnc;
+		}
+	}
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_ENABLE_DIAG318:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

arch/s390/kvm/kvm-s390.c static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr) · line 1273

Quoted 3 lines

arch/s390/kvm/kvm-s390.c +3 -0

+		kvm->arch.use_diag318 = 1;
+		ret = 0;
+		break;

Would it make sense to set kvm->arch.use_diag318 = 1 during the first execution of KVM_S390_VM_MISC_DIAG318 instead, so that we could get along without the KVM_S390_VM_MISC_ENABLE_DIAG318 ?

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

I'm not an expert in feature negotiation, but why isn't this a cpu feature like sief2 instead of a attribute?

@David?

In the end you want to have it somehow in the CPU model I guess. You cannot glue it to QEMU machines, because availability depends on HW+KVM support.

How does the guest detect that it can use diag318? I assume/hope via a a STFLE feature.

Quoted 1 line

SCLP

Okay, so just another feature flag, which implies it belongs into the CPU model. How we communicate support from kvm to QEMU can be done via features, bot also via attributes. Important part is that we can sense/enable/disable.

Quoted 41 lines · David Hildenbrand

Quoted 40 lines · Janosch Frank

Quoted 39 lines · David Hildenbrand

Quoted 38 lines · Janosch Frank

Quoted 37 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 8 lines

arch/s390/kvm/diag.c +0 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/s390/kvm/diag.c +6 -0

+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/s390/kvm/diag.c static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu) · line 269

Quoted 8 lines

arch/s390/kvm/diag.c +8 -0

+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);

Errr.. thought I dropped this message. We favored just using the VM_EVENT from last time.

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

arch/s390/kvm/diag.c +4 -0

+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Pardon my ignorance, but I do not think I fully understand what exactly should be dropped in favor of doing things in userspace.

My assumption: if a diag handler is not found in KVM, then we fallthrough to userspace handling?

Quoted 76 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 30 lines

arch/s390/kvm/diag.c +18 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 
+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)
+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);
+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Quoted 5 lines

What about a reset of the guest VM? If a user first boots into a Linux kernel that supports diag 318, then reboots and selects a Linux kernel that does not support diag 318? I'd expect that the cpnc / cpnv values need to be cleared here somewhere? Otherwise the information might not be accurate anymore?

He resets via QEMU on a machine reset.

Quoted 83 lines · Collin Walling

Quoted 41 lines · David Hildenbrand

Quoted 40 lines · Janosch Frank

Quoted 39 lines · David Hildenbrand

Quoted 38 lines · Janosch Frank

Quoted 37 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)

[...]

Quoted 8 lines

arch/s390/kvm/diag.c +0 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/s390/kvm/diag.c +6 -0

+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/s390/kvm/diag.c static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu) · line 269

Quoted 8 lines

arch/s390/kvm/diag.c +8 -0

+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);

Errr.. thought I dropped this message. We favored just using the VM_EVENT from last time.

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

arch/s390/kvm/diag.c +4 -0

+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Pardon my ignorance, but I do not think I fully understand what exactly should be dropped in favor of doing things in userspace.

My assumption: if a diag handler is not found in KVM, then we fallthrough to userspace handling?

Right, if you simply omit this change to diag.c, the default case returns -EOPNOTSUPP which then should cause an exit to userspace. You can then add the code in QEMU to handle_diag() in target/s390x/kvm.c instead.

Thomas

Quoted 98 lines · Thomas Huth

Quoted 83 lines · Collin Walling

Quoted 41 lines · David Hildenbrand

Quoted 40 lines · Janosch Frank

Quoted 39 lines · David Hildenbrand

Quoted 38 lines · Janosch Frank

Quoted 37 lines · Thomas Huth

Quoted 27 lines · Collin Walling

DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must be intercepted by SIE and handled via KVM. Let's introduce some functions to communicate between userspace and KVM via ioctls. These will be used to get/set the diag318 related information, as well as check the system if KVM supports handling this instruction.

This information can help with diagnosing the environment the VM is running in (Linux, z/VM, etc) if the OS calls this instruction.

By default, this feature is disabled and can only be enabled if a user space program (such as QEMU) explicitly requests it.

The Control Program Name Code (CPNC) is stored in the SIE block and a copy is retained in each VCPU. The Control Program Version Code (CPVC) is not designed to be stored in the SIE block, so we retain a copy in each VCPU next to the CPNC.

Signed-off-by Collin Walling <[email protected]>

---
 Documentation/virt/kvm/devices/vm.rst | 29 +++++++++
 arch/s390/include/asm/kvm_host.h      |  6 +-
 arch/s390/include/uapi/asm/kvm.h      |  5 ++
 arch/s390/kvm/diag.c                  | 20 ++++++
 arch/s390/kvm/kvm-s390.c              | 89 +++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h              |  1 +
 arch/s390/kvm/vsie.c                  |  2 +
 7 files changed, 151 insertions(+), 1 deletion(-)  

[...]

Quoted 8 lines

arch/s390/kvm/diag.c +0 -0

index 563429dece03..3caed4b880c8 100644
@@ -253,6 +253,24 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	return ret < 0 ? ret : 0;
 }
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/s390/kvm/diag.c +6 -0

+static int __diag_set_diag318_info(struct kvm_vcpu *vcpu)
+{
+	unsigned int reg = (vcpu->arch.sie_block->ipa & 0xf0) >> 4;
+	u64 info = vcpu->run->s.regs.gprs[reg];
+
+	if (!vcpu->kvm->arch.use_diag318)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/s390/kvm/diag.c static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu) · line 269

Quoted 8 lines

arch/s390/kvm/diag.c +8 -0

+		return -EOPNOTSUPP;
+
+	vcpu->stat.diagnose_318++;
+	kvm_s390_set_diag318_info(vcpu->kvm, info);
+
+	VCPU_EVENT(vcpu, 3, "diag 0x318 cpnc: 0x%x cpvc: 0x%llx",
+		   vcpu->kvm->arch.diag318_info.cpnc,
+		   (u64)vcpu->kvm->arch.diag318_info.cpvc);  

Errr.. thought I dropped this message. We favored just using the VM_EVENT from last time.

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

arch/s390/kvm/diag.c +4 -0

+
+	return 0;
+}
+
 int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 {
 	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;

arch/s390/kvm/diag.c int kvm_s390_handle_diag(struct kvm_vcpu *vcpu) · line 296

Quoted 7 lines

arch/s390/kvm/diag.c +2 -0

@@ -272,6 +290,8 @@ int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
 		return __diag_page_ref_service(vcpu);
 	case 0x308:
 		return __diag_ipl_functions(vcpu);
+	case 0x318:
+		return __diag_set_diag318_info(vcpu);
 	case 0x500:
 		return __diag_virtio_hypercall(vcpu);  

I wonder whether it would make more sense to simply drop to userspace and handle the diag 318 call there? That way the userspace would always be up-to-date, and as we've seen in the past (e.g. with the various SIGP handling), it's better if the userspace is in control... e.g. userspace could also decide to only use KVM_S390_VM_MISC_ENABLE_DIAG318 if the guest just executed the diag 318 instruction.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And you need the kvm_s390_vm_get/set_misc functions anyway, so these could also be simply used by the diag 318 handler in userspace?

Pardon my ignorance, but I do not think I fully understand what exactly should be dropped in favor of doing things in userspace.

My assumption: if a diag handler is not found in KVM, then we fallthrough to userspace handling?

Right, if you simply omit this change to diag.c, the default case returns -EOPNOTSUPP which then should cause an exit to userspace. You can then add the code in QEMU to handle_diag() in target/s390x/kvm.c instead.

Quoted 2 lines

Thomas

Very cool! Okay, I think this makes sense, then. I'll look into this. Thanks for the tip.

@Conny, I assume this is what you meant as well? If so, ignore my response I sent earlier :)