[PATCH v5 0/2] Use DIAG318 to set Control Program Name & Version Codes

[PATCH v5 0/2] Use DIAG318 to set Control Program Name & Version Codes

[Collin Walling]

Changelog:

    v5
        - s/cpc/diag318_info in order to make the relevant data more clear
        - removed mutex locks for diag318_info get/set

    v4
        - removed setup.c changes introduced in bullet 1 of v3
        - kept diag318_info struct cleanup
        - analogous QEMU patches:
            https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00164.html

    v3
        - kernel patch for diag 0x318 instruction call fixup [removed in v4]
        - removed CPU model code
        - cleaned up diag318_info struct
        - cpnc is no longer unshadowed as it was not needed
        - rebased on 5.1.0-rc3

This instruction call is executed once-and-only-once during Kernel setup.
The availability of this instruction depends on Read Info byte 134, bit 0.

DIAG 318's functionality is also emulated by KVM, which means we can enable 
this feature for a guest even if the host kernel cannot support it. This
feature is made available starting with the zEC12-full model (see analogous
QEMU patches).

The diag318_info is composed of a Control Program Name Code (CPNC) and a
Control Program Version Code (CPVC). These values are used for problem 
diagnosis and allows IBM to identify control program information by answering 
the following question:

    "What environment is this guest running in?" (CPNC)
    "What are more details regarding the OS?" (CPVC)

In the future, we will implement the CPVC to convey more information about the 
OS. For now, we set this field to 0 until we come up with a solid plan.

Collin Walling (2):
  s390/setup: diag318: refactor struct
  s390/kvm: diagnose 318 handling

 Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
 arch/s390/include/asm/diag.h             |  6 +--
 arch/s390/include/asm/kvm_host.h         |  5 +-
 arch/s390/include/uapi/asm/kvm.h         |  4 ++
 arch/s390/kernel/setup.c                 |  3 +-
 arch/s390/kvm/diag.c                     | 17 +++++++
 arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h                 |  1 +
 arch/s390/kvm/vsie.c                     |  2 +
 9 files changed, 126 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v5 1/2] s390/setup: diag318: refactor struct

[Collin Walling]

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 <walling@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/include/asm/diag.h | 6 ++----
 arch/s390/kernel/setup.c     | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 0036eab..ca8f85b 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -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;
 	};
 };
 
diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
index f8544d5..b66c41f 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -1045,8 +1045,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)
-- 
2.7.4

[PATCH v5 2/2] s390/kvm: diagnose 318 handling

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

The get/set functions are introduced primarily for VM migration and
reset, though no harm could be done to the system if a userspace
program decides to alter this data (this is highly discouraged).

The Control Program Name Code (CPNC) is stored in the SIE block (if
host hardware supports it) 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.

At this time, the CPVC is not reported during a VM_EVENT as its
format is yet to be properly defined.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
 arch/s390/include/asm/kvm_host.h         |  5 +-
 arch/s390/include/uapi/asm/kvm.h         |  4 ++
 arch/s390/kvm/diag.c                     | 17 +++++++
 arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h                 |  1 +
 arch/s390/kvm/vsie.c                     |  2 +
 7 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
index 4ffb82b..56f7d9c 100644
--- a/Documentation/virtual/kvm/devices/vm.txt
+++ b/Documentation/virtual/kvm/devices/vm.txt
@@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
 	    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
+Architectures: s390
+
+6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
+
+Allows userspace to access the DIAGNOSE 0x318 information which consists of a
+1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
+This information is initialized during IPL and must be preserved during
+migration.
+
+Parameters: address of a buffer in user space to store the data (u64) to
+Returns:    -EFAULT if the given address is not accessible from kernel space
+	     0 in case of success.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 2b00a3e..b70e8a4 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
 	__u32	scaol;			/* 0x0064 */
 	__u8	reserved68;		/* 0x0068 */
 	__u8    epdx;			/* 0x0069 */
-	__u8    reserved6a[2];		/* 0x006a */
+	__u8	cpnc;			/* 0x006a */
+	__u8	reserved6b;		/* 0x006b */
 	__u32	todpr;			/* 0x006c */
 #define GISA_FORMAT1 0x00000001
 	__u32	gd;			/* 0x0070 */
@@ -393,6 +394,7 @@ struct kvm_vcpu_stat {
 	u64 diagnose_9c;
 	u64 diagnose_258;
 	u64 diagnose_308;
+	u64 diagnose_318;
 	u64 diagnose_500;
 	u64 diagnose_other;
 };
@@ -868,6 +870,7 @@ struct kvm_arch{
 	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	DECLARE_BITMAP(idle_mask, KVM_MAX_VCPUS);
 	struct kvm_s390_gisa_interrupt gisa_int;
+	union diag318_info diag318_info;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 47104e5..e0684da 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
 #define KVM_S390_VM_CRYPTO		2
 #define KVM_S390_VM_CPU_MODEL		3
 #define KVM_S390_VM_MIGRATION		4
+#define KVM_S390_VM_MISC		5
 
 /* kvm attributes for mem_ctrl */
 #define KVM_S390_VM_MEM_ENABLE_CMMA	0
@@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
 #define KVM_S390_VM_MIGRATION_START	1
 #define KVM_S390_VM_MIGRATION_STATUS	2
 
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_DIAG318	0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 45634b3d..42a8db3 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -235,6 +235,21 @@ 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];
+
+	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;
@@ -254,6 +269,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);
 	default:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 28ebd64..8be9867 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
 	{ "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 }
@@ -1228,6 +1229,68 @@ 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, (u64)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_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+	u64 diag318_info;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_DIAG318:
+		ret = -EFAULT;
+		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;
+	}
+	return ret;
+}
+
+static int kvm_s390_get_diag318_info(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	if (put_user(kvm->arch.diag318_info.val, (u64 __user *)attr->addr))
+		return -EFAULT;
+
+	VM_EVENT(kvm, 3, "QUERY: CPNC: 0x%x, CPVC: 0x%llx",
+		 kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
+	return 0;
+}
+
+static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_DIAG318:
+		ret = kvm_s390_get_diag318_info(kvm, attr);
+		break;
+	default:
+		ret = -ENXIO;
+		break;
+	}
+	return ret;
+}
+
 static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	struct kvm_s390_vm_cpu_processor *proc;
@@ -1674,6 +1737,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_set_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_MISC:
+		ret = kvm_s390_set_misc(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1699,6 +1765,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_get_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_MISC:
+		ret = kvm_s390_get_misc(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1772,6 +1841,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = 0;
 		break;
+	case KVM_S390_VM_MISC:
+		switch (attr->attr) {
+		case KVM_S390_VM_MISC_DIAG318:
+			ret = 0;
+			break;
+		default:
+			ret = -ENXIO;
+			break;
+		}
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -2892,6 +2971,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
 	/* make vcpu_load load the right gmap on the first trigger */
 	vcpu->arch.enabled_gmap = vcpu->arch.gmap;
+	if (sclp.has_diag318)
+		vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
 }
 
 static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 6d9448d..70a21b4 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
 
 /* implemented in kvm-s390.c */
+void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info);
 void kvm_s390_set_tod_clock(struct kvm *kvm,
 			    const struct kvm_s390_vm_tod_clock *gtod);
 long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 076090f..50e522e0 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -548,6 +548,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		scb_s->ecd |= scb_o->ecd & ECD_ETOKENF;
 
 	scb_s->hpid = HPID_VSIE;
+	if (sclp.has_diag318)
+		scb_s->cpnc = scb_o->cpnc;
 
 	prepare_ibc(vcpu, vsie_page);
 	rc = shadow_crycb(vcpu, vsie_page);
-- 
2.7.4

Re: [PATCH v5 0/2] Use DIAG318 to set Control Program Name & Version Codes

[Collin Walling]

On 6/25/19 11:03 AM, Collin Walling wrote:
> Changelog:
> 
>      v5
>          - s/cpc/diag318_info in order to make the relevant data more clear
>          - removed mutex locks for diag318_info get/set
> 
>      v4
>          - removed setup.c changes introduced in bullet 1 of v3
>          - kept diag318_info struct cleanup
>          - analogous QEMU patches:

Correct link to QEMU patches:
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05535.html

	

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[David Hildenbrand]

On 25.06.19 17:03, Collin Walling 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.
> 
> The get/set functions are introduced primarily for VM migration and
> reset, though no harm could be done to the system if a userspace
> program decides to alter this data (this is highly discouraged).
> 
> The Control Program Name Code (CPNC) is stored in the SIE block (if
> host hardware supports it) 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.
> 
> At this time, the CPVC is not reported during a VM_EVENT as its
> format is yet to be properly defined.
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>

Only some minor comments.


> ---
>  Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
>  arch/s390/include/asm/kvm_host.h         |  5 +-
>  arch/s390/include/uapi/asm/kvm.h         |  4 ++
>  arch/s390/kvm/diag.c                     | 17 +++++++
>  arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h                 |  1 +
>  arch/s390/kvm/vsie.c                     |  2 +
>  7 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
> index 4ffb82b..56f7d9c 100644
> --- a/Documentation/virtual/kvm/devices/vm.txt
> +++ b/Documentation/virtual/kvm/devices/vm.txt
> @@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>  	    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
> +Architectures: s390
> +
> +6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
> +
> +Allows userspace to access the DIAGNOSE 0x318 information which consists of a
> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
> +This information is initialized during IPL and must be preserved during
> +migration.
> +
> +Parameters: address of a buffer in user space to store the data (u64) to
> +Returns:    -EFAULT if the given address is not accessible from kernel space
> +	     0 in case of success.
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 2b00a3e..b70e8a4 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
>  	__u32	scaol;			/* 0x0064 */
>  	__u8	reserved68;		/* 0x0068 */
>  	__u8    epdx;			/* 0x0069 */
> -	__u8    reserved6a[2];		/* 0x006a */
> +	__u8	cpnc;			/* 0x006a */
> +	__u8	reserved6b;		/* 0x006b */
>  	__u32	todpr;			/* 0x006c */
>  #define GISA_FORMAT1 0x00000001
>  	__u32	gd;			/* 0x0070 */
> @@ -393,6 +394,7 @@ struct kvm_vcpu_stat {
>  	u64 diagnose_9c;
>  	u64 diagnose_258;
>  	u64 diagnose_308;
> +	u64 diagnose_318;
>  	u64 diagnose_500;
>  	u64 diagnose_other;
>  };
> @@ -868,6 +870,7 @@ struct kvm_arch{
>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>  	DECLARE_BITMAP(idle_mask, KVM_MAX_VCPUS);
>  	struct kvm_s390_gisa_interrupt gisa_int;
> +	union diag318_info diag318_info;
>  };
>  
>  #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 47104e5..e0684da 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>  #define KVM_S390_VM_CRYPTO		2
>  #define KVM_S390_VM_CPU_MODEL		3
>  #define KVM_S390_VM_MIGRATION		4
> +#define KVM_S390_VM_MISC		5
>  
>  /* kvm attributes for mem_ctrl */
>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
> @@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
>  #define KVM_S390_VM_MIGRATION_START	1
>  #define KVM_S390_VM_MIGRATION_STATUS	2
>  
> +/* kvm attributes for KVM_S390_VM_MISC */
> +#define KVM_S390_VM_MISC_DIAG318	0
> +
>  /* for KVM_GET_REGS and KVM_SET_REGS */
>  struct kvm_regs {
>  	/* general purpose regs for s390 */
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 45634b3d..42a8db3 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -235,6 +235,21 @@ 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];
> +
> +	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);

We have the event in kvm_s390_set_diag318_info(), do we really need this
one, too?

> +
> +	return 0;
> +}
> +
>  int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>  {
>  	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
> @@ -254,6 +269,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);
>  	default:
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 28ebd64..8be9867 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
>  	{ "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 }
> @@ -1228,6 +1229,68 @@ 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, (u64)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;
> +		}

If two CPUs would be executing this in parallel, we could create an
inconsistent cpnc value in the HW. Not sure if we care.

Also, there is a possible race with CPU hotplug, depending on the
compiler optimizations if I'm not wrong.

Sure we don't want to protect this by a mutex just to avoid having to
worry about such stuff?

I guess, for ordinary guest operations, these races shouldn't matter.

> +	}
> +}
> +
> +static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +	u64 diag318_info;

nit: reorder both

> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_DIAG318:
> +		ret = -EFAULT;
> +		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;
> +	}
> +	return ret;
> +}
> +
> +static int kvm_s390_get_diag318_info(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	if (put_user(kvm->arch.diag318_info.val, (u64 __user *)attr->addr))
> +		return -EFAULT;
> +
> +	VM_EVENT(kvm, 3, "QUERY: CPNC: 0x%x, CPVC: 0x%llx",
> +		 kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);

Sure we need that cast / can't avoid it?

> +	return 0;
> +}
> +
> +static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_DIAG318:
> +		ret = kvm_s390_get_diag318_info(kvm, attr);
> +		break;
> +	default:
> +		ret = -ENXIO;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
>  	struct kvm_s390_vm_cpu_processor *proc;
> @@ -1674,6 +1737,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_set_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_set_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1699,6 +1765,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_get_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_get_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1772,6 +1841,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = 0;
>  		break;
> +	case KVM_S390_VM_MISC:
> +		switch (attr->attr) {
> +		case KVM_S390_VM_MISC_DIAG318:
> +			ret = 0;
> +			break;
> +		default:
> +			ret = -ENXIO;
> +			break;
> +		}
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -2892,6 +2971,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
>  	/* make vcpu_load load the right gmap on the first trigger */
>  	vcpu->arch.enabled_gmap = vcpu->arch.gmap;
> +	if (sclp.has_diag318)
> +		vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
>  }
>  
>  static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 6d9448d..70a21b4 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>  int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>  
>  /* implemented in kvm-s390.c */
> +void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info);
>  void kvm_s390_set_tod_clock(struct kvm *kvm,
>  			    const struct kvm_s390_vm_tod_clock *gtod);
>  long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 076090f..50e522e0 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -548,6 +548,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_s->ecd |= scb_o->ecd & ECD_ETOKENF;
>  
>  	scb_s->hpid = HPID_VSIE;
> +	if (sclp.has_diag318)
> +		scb_s->cpnc = scb_o->cpnc;
>  
>  	prepare_ibc(vcpu, vsie_page);
>  	rc = shadow_crycb(vcpu, vsie_page);
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[David Hildenbrand]

On 25.06.19 17:03, Collin Walling 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.
> 
> The get/set functions are introduced primarily for VM migration and
> reset, though no harm could be done to the system if a userspace
> program decides to alter this data (this is highly discouraged).
> 
> The Control Program Name Code (CPNC) is stored in the SIE block (if
> host hardware supports it) 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.
> 
> At this time, the CPVC is not reported during a VM_EVENT as its
> format is yet to be properly defined.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
>  arch/s390/include/asm/kvm_host.h         |  5 +-
>  arch/s390/include/uapi/asm/kvm.h         |  4 ++
>  arch/s390/kvm/diag.c                     | 17 +++++++
>  arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h                 |  1 +
>  arch/s390/kvm/vsie.c                     |  2 +
>  7 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
> index 4ffb82b..56f7d9c 100644
> --- a/Documentation/virtual/kvm/devices/vm.txt
> +++ b/Documentation/virtual/kvm/devices/vm.txt
> @@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>  	    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
> +Architectures: s390
> +
> +6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
> +
> +Allows userspace to access the DIAGNOSE 0x318 information which consists of a
> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
> +This information is initialized during IPL and must be preserved during
> +migration.
> +
> +Parameters: address of a buffer in user space to store the data (u64) to
> +Returns:    -EFAULT if the given address is not accessible from kernel space
> +	     0 in case of success.
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 2b00a3e..b70e8a4 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
>  	__u32	scaol;			/* 0x0064 */
>  	__u8	reserved68;		/* 0x0068 */
>  	__u8    epdx;			/* 0x0069 */
> -	__u8    reserved6a[2];		/* 0x006a */
> +	__u8	cpnc;			/* 0x006a */
> +	__u8	reserved6b;		/* 0x006b */
>  	__u32	todpr;			/* 0x006c */
>  #define GISA_FORMAT1 0x00000001
>  	__u32	gd;			/* 0x0070 */
> @@ -393,6 +394,7 @@ struct kvm_vcpu_stat {
>  	u64 diagnose_9c;
>  	u64 diagnose_258;
>  	u64 diagnose_308;
> +	u64 diagnose_318;
>  	u64 diagnose_500;
>  	u64 diagnose_other;
>  };
> @@ -868,6 +870,7 @@ struct kvm_arch{
>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>  	DECLARE_BITMAP(idle_mask, KVM_MAX_VCPUS);
>  	struct kvm_s390_gisa_interrupt gisa_int;
> +	union diag318_info diag318_info;
>  };
>  
>  #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 47104e5..e0684da 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>  #define KVM_S390_VM_CRYPTO		2
>  #define KVM_S390_VM_CPU_MODEL		3
>  #define KVM_S390_VM_MIGRATION		4
> +#define KVM_S390_VM_MISC		5
>  
>  /* kvm attributes for mem_ctrl */
>  #define KVM_S390_VM_MEM_ENABLE_CMMA	0
> @@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
>  #define KVM_S390_VM_MIGRATION_START	1
>  #define KVM_S390_VM_MIGRATION_STATUS	2
>  
> +/* kvm attributes for KVM_S390_VM_MISC */
> +#define KVM_S390_VM_MISC_DIAG318	0
> +
>  /* for KVM_GET_REGS and KVM_SET_REGS */
>  struct kvm_regs {
>  	/* general purpose regs for s390 */
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 45634b3d..42a8db3 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -235,6 +235,21 @@ 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];
> +
> +	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;
> @@ -254,6 +269,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);
>  	default:
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 28ebd64..8be9867 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
>  	{ "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 }
> @@ -1228,6 +1229,68 @@ 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, (u64)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_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +	u64 diag318_info;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_DIAG318:
> +		ret = -EFAULT;
> +		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;
> +	}
> +	return ret;
> +}
> +
> +static int kvm_s390_get_diag318_info(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	if (put_user(kvm->arch.diag318_info.val, (u64 __user *)attr->addr))
> +		return -EFAULT;
> +
> +	VM_EVENT(kvm, 3, "QUERY: CPNC: 0x%x, CPVC: 0x%llx",
> +		 kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
> +	return 0;
> +}
> +
> +static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int ret;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_MISC_DIAG318:
> +		ret = kvm_s390_get_diag318_info(kvm, attr);
> +		break;
> +	default:
> +		ret = -ENXIO;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
>  	struct kvm_s390_vm_cpu_processor *proc;
> @@ -1674,6 +1737,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_set_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_set_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1699,6 +1765,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = kvm_s390_vm_get_migration(kvm, attr);
>  		break;
> +	case KVM_S390_VM_MISC:
> +		ret = kvm_s390_get_misc(kvm, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -1772,6 +1841,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>  	case KVM_S390_VM_MIGRATION:
>  		ret = 0;
>  		break;
> +	case KVM_S390_VM_MISC:
> +		switch (attr->attr) {
> +		case KVM_S390_VM_MISC_DIAG318:
> +			ret = 0;
> +			break;
> +		default:
> +			ret = -ENXIO;
> +			break;
> +		}
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -2892,6 +2971,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
>  	/* make vcpu_load load the right gmap on the first trigger */
>  	vcpu->arch.enabled_gmap = vcpu->arch.gmap;
> +	if (sclp.has_diag318)
> +		vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
>  }
>  
>  static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 6d9448d..70a21b4 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>  int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>  
>  /* implemented in kvm-s390.c */
> +void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info);
>  void kvm_s390_set_tod_clock(struct kvm *kvm,
>  			    const struct kvm_s390_vm_tod_clock *gtod);
>  long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 076090f..50e522e0 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -548,6 +548,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_s->ecd |= scb_o->ecd & ECD_ETOKENF;
>  
>  	scb_s->hpid = HPID_VSIE;
> +	if (sclp.has_diag318)
> +		scb_s->cpnc = scb_o->cpnc;
>  
>  	prepare_ibc(vcpu, vsie_page);
>  	rc = shadow_crycb(vcpu, vsie_page);
> 

BTW. there is currently no mechanism to fake absence of diag318. Should
we have one? (in contrast, for CMMA we have, which is also a CPU feature)

-- 

Thanks,

David / dhildenb

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Christian Borntraeger]

On 26.06.19 11:45, David Hildenbrand wrote:

> 
> BTW. there is currently no mechanism to fake absence of diag318. Should
> we have one? (in contrast, for CMMA we have, which is also a CPU feature)

Yes, we want to be able to disable diag318 via a CPU model feature. That actually
means that the kernel must not answer this if we disable it. 

Re: [PATCH v5 1/2] s390/setup: diag318: refactor struct

[Cornelia Huck]

On Tue, 25 Jun 2019 11:03:41 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> 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 <walling@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/include/asm/diag.h | 6 ++----
>  arch/s390/kernel/setup.c     | 3 +--
>  2 files changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Cornelia Huck]

On Tue, 25 Jun 2019 11:03:42 -0400
Collin Walling <walling@linux.ibm.com> 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.
> 
> The get/set functions are introduced primarily for VM migration and
> reset, though no harm could be done to the system if a userspace
> program decides to alter this data (this is highly discouraged).
> 
> The Control Program Name Code (CPNC) is stored in the SIE block (if
> host hardware supports it) 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.
> 
> At this time, the CPVC is not reported during a VM_EVENT as its
> format is yet to be properly defined.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
>  arch/s390/include/asm/kvm_host.h         |  5 +-
>  arch/s390/include/uapi/asm/kvm.h         |  4 ++
>  arch/s390/kvm/diag.c                     | 17 +++++++
>  arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h                 |  1 +
>  arch/s390/kvm/vsie.c                     |  2 +
>  7 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
> index 4ffb82b..56f7d9c 100644
> --- a/Documentation/virtual/kvm/devices/vm.txt
> +++ b/Documentation/virtual/kvm/devices/vm.txt
> @@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>  	    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
> +Architectures: s390
> +
> +6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
> +
> +Allows userspace to access the DIAGNOSE 0x318 information which consists of a
> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
> +This information is initialized during IPL and must be preserved during
> +migration.
> +
> +Parameters: address of a buffer in user space to store the data (u64) to
> +Returns:    -EFAULT if the given address is not accessible from kernel space
> +	     0 in case of success.

Hm, this looks a bit incomplete to me. IIUC, the guest will set this
via diag 318, and this interface is intended to be used by user space
for retrieving/setting this during migration. What about the following:


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.

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
	     0 in case of success

Otherwise, no further comments from me.

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Collin Walling]

On 6/26/19 8:11 AM, Cornelia Huck wrote:
> On Tue, 25 Jun 2019 11:03:42 -0400
> Collin Walling <walling@linux.ibm.com> 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.
>>
>> The get/set functions are introduced primarily for VM migration and
>> reset, though no harm could be done to the system if a userspace
>> program decides to alter this data (this is highly discouraged).
>>
>> The Control Program Name Code (CPNC) is stored in the SIE block (if
>> host hardware supports it) 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.
>>
>> At this time, the CPVC is not reported during a VM_EVENT as its
>> format is yet to be properly defined.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
>>   arch/s390/include/asm/kvm_host.h         |  5 +-
>>   arch/s390/include/uapi/asm/kvm.h         |  4 ++
>>   arch/s390/kvm/diag.c                     | 17 +++++++
>>   arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
>>   arch/s390/kvm/kvm-s390.h                 |  1 +
>>   arch/s390/kvm/vsie.c                     |  2 +
>>   7 files changed, 123 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
>> index 4ffb82b..56f7d9c 100644
>> --- a/Documentation/virtual/kvm/devices/vm.txt
>> +++ b/Documentation/virtual/kvm/devices/vm.txt
>> @@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>>   	    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
>> +Architectures: s390
>> +
>> +6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
>> +
>> +Allows userspace to access the DIAGNOSE 0x318 information which consists of a
>> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
>> +This information is initialized during IPL and must be preserved during
>> +migration.
>> +
>> +Parameters: address of a buffer in user space to store the data (u64) to
>> +Returns:    -EFAULT if the given address is not accessible from kernel space
>> +	     0 in case of success.
> 
> Hm, this looks a bit incomplete to me. IIUC, the guest will set this
> via diag 318, and this interface is intended to be used by user space
> for retrieving/setting this during migration. What about the following:
> 
> 
> 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.
> 
> 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
> 	     0 in case of success
> 
> Otherwise, no further comments from me.
> 

This is much more concise, thank you!

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Collin Walling]

On 6/26/19 5:08 AM, David Hildenbrand wrote:
> On 25.06.19 17:03, Collin Walling 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.
>>
>> The get/set functions are introduced primarily for VM migration and
>> reset, though no harm could be done to the system if a userspace
>> program decides to alter this data (this is highly discouraged).
>>
>> The Control Program Name Code (CPNC) is stored in the SIE block (if
>> host hardware supports it) 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.
>>
>> At this time, the CPVC is not reported during a VM_EVENT as its
>> format is yet to be properly defined.
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Only some minor comments.
> 
> 
>> ---
>>   Documentation/virtual/kvm/devices/vm.txt | 14 ++++++
>>   arch/s390/include/asm/kvm_host.h         |  5 +-
>>   arch/s390/include/uapi/asm/kvm.h         |  4 ++
>>   arch/s390/kvm/diag.c                     | 17 +++++++
>>   arch/s390/kvm/kvm-s390.c                 | 81 ++++++++++++++++++++++++++++++++
>>   arch/s390/kvm/kvm-s390.h                 |  1 +
>>   arch/s390/kvm/vsie.c                     |  2 +
>>   7 files changed, 123 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virtual/kvm/devices/vm.txt b/Documentation/virtual/kvm/devices/vm.txt
>> index 4ffb82b..56f7d9c 100644
>> --- a/Documentation/virtual/kvm/devices/vm.txt
>> +++ b/Documentation/virtual/kvm/devices/vm.txt
>> @@ -268,3 +268,17 @@ Parameters: address of a buffer in user space to store the data (u64) to;
>>   	    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
>> +Architectures: s390
>> +
>> +6.1. KVM_S390_VM_MISC_DIAG318 (r/w)
>> +
>> +Allows userspace to access the DIAGNOSE 0x318 information which consists of a
>> +1-byte "Control Program Name Code" and a 7-byte "Control Program Version Code".
>> +This information is initialized during IPL and must be preserved during
>> +migration.
>> +
>> +Parameters: address of a buffer in user space to store the data (u64) to
>> +Returns:    -EFAULT if the given address is not accessible from kernel space
>> +	     0 in case of success.
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 2b00a3e..b70e8a4 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -229,7 +229,8 @@ struct kvm_s390_sie_block {
>>   	__u32	scaol;			/* 0x0064 */
>>   	__u8	reserved68;		/* 0x0068 */
>>   	__u8    epdx;			/* 0x0069 */
>> -	__u8    reserved6a[2];		/* 0x006a */
>> +	__u8	cpnc;			/* 0x006a */
>> +	__u8	reserved6b;		/* 0x006b */
>>   	__u32	todpr;			/* 0x006c */
>>   #define GISA_FORMAT1 0x00000001
>>   	__u32	gd;			/* 0x0070 */
>> @@ -393,6 +394,7 @@ struct kvm_vcpu_stat {
>>   	u64 diagnose_9c;
>>   	u64 diagnose_258;
>>   	u64 diagnose_308;
>> +	u64 diagnose_318;
>>   	u64 diagnose_500;
>>   	u64 diagnose_other;
>>   };
>> @@ -868,6 +870,7 @@ struct kvm_arch{
>>   	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
>>   	DECLARE_BITMAP(idle_mask, KVM_MAX_VCPUS);
>>   	struct kvm_s390_gisa_interrupt gisa_int;
>> +	union diag318_info diag318_info;
>>   };
>>   
>>   #define KVM_HVA_ERR_BAD		(-1UL)
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 47104e5..e0684da 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
>>   #define KVM_S390_VM_CRYPTO		2
>>   #define KVM_S390_VM_CPU_MODEL		3
>>   #define KVM_S390_VM_MIGRATION		4
>> +#define KVM_S390_VM_MISC		5
>>   
>>   /* kvm attributes for mem_ctrl */
>>   #define KVM_S390_VM_MEM_ENABLE_CMMA	0
>> @@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
>>   #define KVM_S390_VM_MIGRATION_START	1
>>   #define KVM_S390_VM_MIGRATION_STATUS	2
>>   
>> +/* kvm attributes for KVM_S390_VM_MISC */
>> +#define KVM_S390_VM_MISC_DIAG318	0
>> +
>>   /* for KVM_GET_REGS and KVM_SET_REGS */
>>   struct kvm_regs {
>>   	/* general purpose regs for s390 */
>> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
>> index 45634b3d..42a8db3 100644
>> --- a/arch/s390/kvm/diag.c
>> +++ b/arch/s390/kvm/diag.c
>> @@ -235,6 +235,21 @@ 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];
>> +
>> +	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);
> 
> We have the event in kvm_s390_set_diag318_info(), do we really need this
> one, too?
> 

The other is a VM_EVENT, which I find more helpful than the VCPU_EVENT
imho. I put this in here for consistency's sake with the rest of the
__diag_* functions, but it certainly isn't needed.

I'd vote to remove the VCPU event.

>> +
>> +	return 0;
>> +}
>> +
>>   int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
>>   {
>>   	int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
>> @@ -254,6 +269,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);
>>   	default:
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 28ebd64..8be9867 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -157,6 +157,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>>   	{ "instruction_diag_9c", VCPU_STAT(diagnose_9c) },
>>   	{ "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 }
>> @@ -1228,6 +1229,68 @@ 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, (u64)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;
>> +		}
> 
> If two CPUs would be executing this in parallel, we could create an
> inconsistent cpnc value in the HW. Not sure if we care.
> 
> Also, there is a possible race with CPU hotplug, depending on the
> compiler optimizations if I'm not wrong.
> 
> Sure we don't want to protect this by a mutex just to avoid having to
> worry about such stuff?
> 
> I guess, for ordinary guest operations, these races shouldn't matter.
> 

The CPNC is harmless, but if we have the opportunity to protect
ourselves from unwanted behavior, why not take it? I'll put the mutexes
back in for the set operation.

>> +	}
>> +}
>> +
>> +static int kvm_s390_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int ret;
>> +	u64 diag318_info;
> 
> nit: reorder both
> 

Noted.

>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_MISC_DIAG318:
>> +		ret = -EFAULT;
>> +		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;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int kvm_s390_get_diag318_info(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	if (put_user(kvm->arch.diag318_info.val, (u64 __user *)attr->addr))
>> +		return -EFAULT;
>> +
>> +	VM_EVENT(kvm, 3, "QUERY: CPNC: 0x%x, CPVC: 0x%llx",
>> +		 kvm->arch.diag318_info.cpnc, (u64)kvm->arch.diag318_info.cpvc);
> 
> Sure we need that cast / can't avoid it?
> 

Compiler says we don't need it :)

>> +	return 0;
>> +}
>> +
>> +static int kvm_s390_get_misc(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int ret;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_MISC_DIAG318:
>> +		ret = kvm_s390_get_diag318_info(kvm, attr);
>> +		break;
>> +	default:
>> +		ret = -ENXIO;
>> +		break;
>> +	}
>> +	return ret;
>> +}
>> +
>>   static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
>>   {
>>   	struct kvm_s390_vm_cpu_processor *proc;
>> @@ -1674,6 +1737,9 @@ static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>   	case KVM_S390_VM_MIGRATION:
>>   		ret = kvm_s390_vm_set_migration(kvm, attr);
>>   		break;
>> +	case KVM_S390_VM_MISC:
>> +		ret = kvm_s390_set_misc(kvm, attr);
>> +		break;
>>   	default:
>>   		ret = -ENXIO;
>>   		break;
>> @@ -1699,6 +1765,9 @@ static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>   	case KVM_S390_VM_MIGRATION:
>>   		ret = kvm_s390_vm_get_migration(kvm, attr);
>>   		break;
>> +	case KVM_S390_VM_MISC:
>> +		ret = kvm_s390_get_misc(kvm, attr);
>> +		break;
>>   	default:
>>   		ret = -ENXIO;
>>   		break;
>> @@ -1772,6 +1841,16 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
>>   	case KVM_S390_VM_MIGRATION:
>>   		ret = 0;
>>   		break;
>> +	case KVM_S390_VM_MISC:
>> +		switch (attr->attr) {
>> +		case KVM_S390_VM_MISC_DIAG318:
>> +			ret = 0;
>> +			break;
>> +		default:
>> +			ret = -ENXIO;
>> +			break;
>> +		}
>> +		break;
>>   	default:
>>   		ret = -ENXIO;
>>   		break;
>> @@ -2892,6 +2971,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>>   		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
>>   	/* make vcpu_load load the right gmap on the first trigger */
>>   	vcpu->arch.enabled_gmap = vcpu->arch.gmap;
>> +	if (sclp.has_diag318)
>> +		vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag318_info.cpnc;
>>   }
>>   
>>   static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 6d9448d..70a21b4 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -281,6 +281,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>>   int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>>   
>>   /* implemented in kvm-s390.c */
>> +void kvm_s390_set_diag318_info(struct kvm *kvm, u64 info);
>>   void kvm_s390_set_tod_clock(struct kvm *kvm,
>>   			    const struct kvm_s390_vm_tod_clock *gtod);
>>   long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 076090f..50e522e0 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -548,6 +548,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>   		scb_s->ecd |= scb_o->ecd & ECD_ETOKENF;
>>   
>>   	scb_s->hpid = HPID_VSIE;
>> +	if (sclp.has_diag318)
>> +		scb_s->cpnc = scb_o->cpnc;
>>   
>>   	prepare_ibc(vcpu, vsie_page);
>>   	rc = shadow_crycb(vcpu, vsie_page);
>>
> 
> 

Thanks for the review!

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Collin Walling]

On 6/26/19 6:28 AM, Christian Borntraeger wrote:
> 
> 
> On 26.06.19 11:45, David Hildenbrand wrote:
> 
>>
>> BTW. there is currently no mechanism to fake absence of diag318. Should
>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
> 
> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
> means that the kernel must not answer this if we disable it.
> 
Correct. If the guest specifies diag318=off, then the instruction 
shouldn't be executed (it is fenced off in the kernel by checking the 
Read SCP Info bit).

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[David Hildenbrand]

On 26.06.19 16:30, Collin Walling wrote:
> On 6/26/19 6:28 AM, Christian Borntraeger wrote:
>>
>>
>> On 26.06.19 11:45, David Hildenbrand wrote:
>>
>>>
>>> BTW. there is currently no mechanism to fake absence of diag318. Should
>>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
>>
>> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
>> means that the kernel must not answer this if we disable it.
>>
> Correct. If the guest specifies diag318=off, then the instruction 
> shouldn't be executed (it is fenced off in the kernel by checking the 
> Read SCP Info bit).

But the guest *could* execute it and not get an exception.

-- 

Thanks,

David / dhildenb

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Collin Walling]

On 6/26/19 10:31 AM, David Hildenbrand wrote:
> On 26.06.19 16:30, Collin Walling wrote:
>> On 6/26/19 6:28 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 26.06.19 11:45, David Hildenbrand wrote:
>>>
>>>>
>>>> BTW. there is currently no mechanism to fake absence of diag318. Should
>>>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
>>>
>>> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
>>> means that the kernel must not answer this if we disable it.
>>>
>> Correct. If the guest specifies diag318=off, then the instruction
>> shouldn't be executed (it is fenced off in the kernel by checking the
>> Read SCP Info bit).
> 
> But the guest *could* execute it and not get an exception.
> 

IIUC, you're talking about the situation where QEMU supports diag318,
but KVM does not. The worst case is the guest specifies diag318=on, and
nothing will stop the guest from attempting to execute the instruction.

However, this is fenced in my QEMU patches. In (v5), I have this
following snippet:

@@ -2323,6 +2345,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel 
*model, Error **errp)
          KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
          set_bit(S390_FEAT_AP, model->features);
      }
+
+    /* if KVM supports interception of diag318, then let's provide the 
bit */
+    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_MISC,
+        KVM_S390_VM_MISC_DIAG318)) {
+        set_bit(S390_FEAT_DIAG318, model->features);
+    }
+
      /* strip of features that are not part of the maximum model */
      bitmap_and(model->features, model->features, model->def->full_feat,
                 S390_FEAT_MAX);

If the guest specifies diag318=on, and KVM does *not* support emulation,
then the following message will be observed:

qemu-system-s390x: Some features requested in the CPU model are not
available in the configuration: diag318

and the guest will fail to start. Does this suffice?

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Christian Borntraeger]

On 02.07.19 21:50, Collin Walling wrote:
> On 6/26/19 10:31 AM, David Hildenbrand wrote:
>> On 26.06.19 16:30, Collin Walling wrote:
>>> On 6/26/19 6:28 AM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 26.06.19 11:45, David Hildenbrand wrote:
>>>>
>>>>>
>>>>> BTW. there is currently no mechanism to fake absence of diag318. Should
>>>>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
>>>>
>>>> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
>>>> means that the kernel must not answer this if we disable it.
>>>>
>>> Correct. If the guest specifies diag318=off, then the instruction
>>> shouldn't be executed (it is fenced off in the kernel by checking the
>>> Read SCP Info bit).
>>
>> But the guest *could* execute it and not get an exception.
>>
> 
> IIUC, you're talking about the situation where QEMU supports diag318,
> but KVM does not. The worst case is the guest specifies diag318=on, and
> nothing will stop the guest from attempting to execute the instruction.
> 
> However, this is fenced in my QEMU patches. In (v5), I have this
> following snippet:
> 
> @@ -2323,6 +2345,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>          KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
>          set_bit(S390_FEAT_AP, model->features);
>      }
> +
> +    /* if KVM supports interception of diag318, then let's provide the bit */
> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_MISC,
> +        KVM_S390_VM_MISC_DIAG318)) {
> +        set_bit(S390_FEAT_DIAG318, model->features);
> +    }
> +
>      /* strip of features that are not part of the maximum model */
>      bitmap_and(model->features, model->features, model->def->full_feat,
>                 S390_FEAT_MAX);
> 
> If the guest specifies diag318=on, and KVM does *not* support emulation,
> then the following message will be observed:
> 
> qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: diag318
> 
> and the guest will fail to start. Does this suffice?

But what happens if the guest ignores read scp info and executes diag318 anyway.
It should get a specification exception, but it does not instead diag318 is
executed.

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Collin Walling]

On 7/2/19 4:00 PM, Christian Borntraeger wrote:
> 
> 
> On 02.07.19 21:50, Collin Walling wrote:
>> On 6/26/19 10:31 AM, David Hildenbrand wrote:
>>> On 26.06.19 16:30, Collin Walling wrote:
>>>> On 6/26/19 6:28 AM, Christian Borntraeger wrote:
>>>>>
>>>>>
>>>>> On 26.06.19 11:45, David Hildenbrand wrote:
>>>>>
>>>>>>
>>>>>> BTW. there is currently no mechanism to fake absence of diag318. Should
>>>>>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
>>>>>
>>>>> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
>>>>> means that the kernel must not answer this if we disable it.
>>>>>
>>>> Correct. If the guest specifies diag318=off, then the instruction
>>>> shouldn't be executed (it is fenced off in the kernel by checking the
>>>> Read SCP Info bit).
>>>
>>> But the guest *could* execute it and not get an exception.
>>>
>>
>> IIUC, you're talking about the situation where QEMU supports diag318,
>> but KVM does not. The worst case is the guest specifies diag318=on, and
>> nothing will stop the guest from attempting to execute the instruction.
>>
>> However, this is fenced in my QEMU patches. In (v5), I have this
>> following snippet:
>>
>> @@ -2323,6 +2345,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>           KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
>>           set_bit(S390_FEAT_AP, model->features);
>>       }
>> +
>> +    /* if KVM supports interception of diag318, then let's provide the bit */
>> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_MISC,
>> +        KVM_S390_VM_MISC_DIAG318)) {
>> +        set_bit(S390_FEAT_DIAG318, model->features);
>> +    }
>> +
>>       /* strip of features that are not part of the maximum model */
>>       bitmap_and(model->features, model->features, model->def->full_feat,
>>                  S390_FEAT_MAX);
>>
>> If the guest specifies diag318=on, and KVM does *not* support emulation,
>> then the following message will be observed:
>>
>> qemu-system-s390x: Some features requested in the CPU model are not
>> available in the configuration: diag318
>>
>> and the guest will fail to start. Does this suffice?
> 
> But what happens if the guest ignores read scp info and executes diag318 anyway.
> It should get a specification exception, but it does not instead diag318 is
> executed.
> 

Hmm... I'm not following the logic here. When or how could diag318 be
executed without checking the feature bit? Are we concerned about other
hypervisors running a linux guest with diag318 enabled? Understanding
the scenario will help me follow along with this issue.

Re: [PATCH v5 2/2] s390/kvm: diagnose 318 handling

[Christian Borntraeger]

On 02.07.19 22:04, Collin Walling wrote:
> On 7/2/19 4:00 PM, Christian Borntraeger wrote:
>>
>>
>> On 02.07.19 21:50, Collin Walling wrote:
>>> On 6/26/19 10:31 AM, David Hildenbrand wrote:
>>>> On 26.06.19 16:30, Collin Walling wrote:
>>>>> On 6/26/19 6:28 AM, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 26.06.19 11:45, David Hildenbrand wrote:
>>>>>>
>>>>>>>
>>>>>>> BTW. there is currently no mechanism to fake absence of diag318. Should
>>>>>>> we have one? (in contrast, for CMMA we have, which is also a CPU feature)
>>>>>>
>>>>>> Yes, we want to be able to disable diag318 via a CPU model feature. That actually
>>>>>> means that the kernel must not answer this if we disable it.
>>>>>>
>>>>> Correct. If the guest specifies diag318=off, then the instruction
>>>>> shouldn't be executed (it is fenced off in the kernel by checking the
>>>>> Read SCP Info bit).
>>>>
>>>> But the guest *could* execute it and not get an exception.
>>>>
>>>
>>> IIUC, you're talking about the situation where QEMU supports diag318,
>>> but KVM does not. The worst case is the guest specifies diag318=on, and
>>> nothing will stop the guest from attempting to execute the instruction.
>>>
>>> However, this is fenced in my QEMU patches. In (v5), I have this
>>> following snippet:
>>>
>>> @@ -2323,6 +2345,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>           KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
>>>           set_bit(S390_FEAT_AP, model->features);
>>>       }
>>> +
>>> +    /* if KVM supports interception of diag318, then let's provide the bit */
>>> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_MISC,
>>> +        KVM_S390_VM_MISC_DIAG318)) {
>>> +        set_bit(S390_FEAT_DIAG318, model->features);
>>> +    }
>>> +
>>>       /* strip of features that are not part of the maximum model */
>>>       bitmap_and(model->features, model->features, model->def->full_feat,
>>>                  S390_FEAT_MAX);
>>>
>>> If the guest specifies diag318=on, and KVM does *not* support emulation,
>>> then the following message will be observed:
>>>
>>> qemu-system-s390x: Some features requested in the CPU model are not
>>> available in the configuration: diag318
>>>
>>> and the guest will fail to start. Does this suffice?
>>
>> But what happens if the guest ignores read scp info and executes diag318 anyway.
>> It should get a specification exception, but it does not instead diag318 is
>> executed.
>>

> 
> Hmm... I'm not following the logic here. When or how could diag318 be
> executed without checking the feature bit? Are we concerned about other
> hypervisors running a linux guest with diag318 enabled? Understanding
> the scenario will help me follow along with this issue.

A malicious or broken guest could just ignore the scp info bit.

In the end it is just architectural compliance. If the scp info bit is off, diag318 must
result in a specification exception, which is does not right now.