[PATCH v7 0/3] Use DIAG318 to set Control Program Name & Version Codes

[PATCH v7 0/3] Use DIAG318 to set Control Program Name & Version Codes

[Collin Walling]

Changelog:

    v7

    • Removed diag handler, as it will now take place within userspace
    • Removed KVM_S390_VM_MISC_ENABLE_DIAG318 (undoes first bullet in v6)
    • Misc clean ups and fixes
        - introduced a new patch to s/diag318/diag_318 and s/byte_134/fac134
          to keep things consistent with the rest of the code

    v6

    • KVM disables diag318 get/set by default [removed in v7]
    • added new IOCTL to tell KVM to enable diag318 [removed in v7]
    • removed VCPU event message in favor of VM_EVENT only

    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 (aka fac134),
bit 0.

DIAG 0x318's is handled by userspace and may be enabled for a guest even if the
host kernel cannot support it.

The diag318_info is composed of a Control Program Name Code (CPNC) and a
Control Program Version Code (CPVC). The CPNC is stored in SIE blocks, and
the CPNC & CPVC pair is stored in the kvm_arch struct. 

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 (such as Linux version and potentially some value denoting a specific 
distro + release). For now, we set this field to 0 until we come up with a solid 
plan.

Collin Walling (3):
  s390/setup: diag 318: refactor struct
  s390: keep diag 318 variables consistent with the rest
  s390/kvm: diagnose 0x318 get/set handling

 Documentation/virt/kvm/devices/vm.rst | 21 +++++++
 arch/s390/include/asm/diag.h          |  8 +--
 arch/s390/include/asm/kvm_host.h      |  5 +-
 arch/s390/include/asm/sclp.h          |  2 +-
 arch/s390/include/uapi/asm/kvm.h      |  4 ++
 arch/s390/kernel/setup.c              |  9 ++-
 arch/s390/kvm/kvm-s390.c              | 82 +++++++++++++++++++++++++++
 arch/s390/kvm/vsie.c                  |  2 +
 drivers/s390/char/sclp.h              |  2 +-
 drivers/s390/char/sclp_early.c        |  2 +-
 10 files changed, 123 insertions(+), 14 deletions(-)

-- 
2.21.3

[PATCH v7 1/3] s390/setup: diag 318: refactor struct

[Collin Walling]

The diag 318 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.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 0036eab14391..ca8f85b53a90 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 36445dd40fdb..1aaaf11acc6b 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -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)
-- 
2.21.3

[PATCH v7 2/3] s390: keep diag 318 variables consistent with the rest

[Collin Walling]

Rename diag318 to diag_318 and byte_134 to fac134 in order to keep
naming schemes consistent with other diags and the read info struct
and make grepping easier.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 arch/s390/include/asm/diag.h   | 2 +-
 arch/s390/include/asm/sclp.h   | 2 +-
 arch/s390/kernel/setup.c       | 6 +++---
 drivers/s390/char/sclp.h       | 2 +-
 drivers/s390/char/sclp_early.c | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index ca8f85b53a90..19da822e494c 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -295,7 +295,7 @@ struct diag26c_mac_resp {
 } __aligned(8);
 
 #define CPNC_LINUX		0x4
-union diag318_info {
+union diag_318_info {
 	unsigned long val;
 	struct {
 		unsigned long cpnc : 8;
diff --git a/arch/s390/include/asm/sclp.h b/arch/s390/include/asm/sclp.h
index c563f8368b19..a45967cfc1ae 100644
--- a/arch/s390/include/asm/sclp.h
+++ b/arch/s390/include/asm/sclp.h
@@ -78,7 +78,7 @@ struct sclp_info {
 	unsigned char has_skey : 1;
 	unsigned char has_kss : 1;
 	unsigned char has_gisaf : 1;
-	unsigned char has_diag318 : 1;
+	unsigned char has_diag_318 : 1;
 	unsigned char has_sipl : 1;
 	unsigned char has_dirq : 1;
 	unsigned int ibc;
diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
index 1aaaf11acc6b..8925a1ac14c9 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -1026,16 +1026,16 @@ static void __init setup_task_size(void)
  */
 static void __init setup_control_program_code(void)
 {
-	union diag318_info diag318_info = {
+	union diag_318_info diag_318_info = {
 		.cpnc = CPNC_LINUX,
 		.cpvc = 0,
 	};
 
-	if (!sclp.has_diag318)
+	if (!sclp.has_diag_318)
 		return;
 
 	diag_stat_inc(DIAG_STAT_X318);
-	asm volatile("diag %0,0,0x318\n" : : "d" (diag318_info.val));
+	asm volatile("diag %0,0,0x318\n" : : "d" (diag_318_info.val));
 }
 
 /*
diff --git a/drivers/s390/char/sclp.h b/drivers/s390/char/sclp.h
index 196333013e54..d6a91f3b8e2b 100644
--- a/drivers/s390/char/sclp.h
+++ b/drivers/s390/char/sclp.h
@@ -196,7 +196,7 @@ struct read_info_sccb {
 	u8	_pad_122[124 - 122];	/* 122-123 */
 	u32	hmfai;			/* 124-127 */
 	u8	_pad_128[134 - 128];	/* 128-133 */
-	u8	byte_134;			/* 134 */
+	u8	fac134;			/* 134 */
 	u8	cpudirq;		/* 135 */
 	u16	cbl;			/* 136-137 */
 	u8	_pad_138[4096 - 138];	/* 138-4095 */
diff --git a/drivers/s390/char/sclp_early.c b/drivers/s390/char/sclp_early.c
index cc5e84b80c69..388d24c65e74 100644
--- a/drivers/s390/char/sclp_early.c
+++ b/drivers/s390/char/sclp_early.c
@@ -46,7 +46,7 @@ static void __init sclp_early_facilities_detect(struct read_info_sccb *sccb)
 	if (sccb->fac91 & 0x40)
 		S390_lowcore.machine_flags |= MACHINE_FLAG_TLB_GUEST;
 	if (sccb->cpuoff > 134)
-		sclp.has_diag318 = !!(sccb->byte_134 & 0x80);
+		sclp.has_diag_318 = !!(sccb->fac134 & 0x80);
 	sclp.rnmax = sccb->rnmax ? sccb->rnmax : sccb->rnmax2;
 	sclp.rzm = sccb->rnsize ? sccb->rnsize : sccb->rnsize2;
 	sclp.rzm <<= 20;
-- 
2.21.3

[PATCH v7 3/3] s390/kvm: diagnose 0x318 get/set handling

[Collin Walling]

DIAGNOSE 0x318 (diag 318) sets information regarding the environment
the VM is running in (Linux, z/VM, etc) and is observed via
firmware/service events.

This is a privileged s390x instruction that must be intercepted by
SIE. Userspace handling is required, so let's introduce some functions
to communicate between userspace and KVM via ioctls. These will be used
to get/set the diag 318 related information.

The Control Program Name Code (CPNC) is stored in the SIE block. The
CPNC along with the Control Program Version Code (CPVC) are stored in
the kvm_arch struct.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 Documentation/virt/kvm/devices/vm.rst | 21 +++++++
 arch/s390/include/asm/kvm_host.h      |  5 +-
 arch/s390/include/uapi/asm/kvm.h      |  4 ++
 arch/s390/kvm/kvm-s390.c              | 82 +++++++++++++++++++++++++++
 arch/s390/kvm/vsie.c                  |  2 +
 5 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
index 0aa5b1cfd700..52cc906dd7bd 100644
--- a/Documentation/virt/kvm/devices/vm.rst
+++ b/Documentation/virt/kvm/devices/vm.rst
@@ -314,3 +314,24 @@ 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
+==========================
+
+:Architectures: s390
+
+6.1. KVM_S390_VM_MISC_DIAG_318 (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.
+
+: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
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index d6bcd34f3ec3..c03222051043 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -260,7 +260,8 @@ struct kvm_s390_sie_block {
 	__u32	scaol;			/* 0x0064 */
 	__u8	sdf;			/* 0x0068 */
 	__u8    epdx;			/* 0x0069 */
-	__u8    reserved6a[2];		/* 0x006a */
+	__u8	cpnc;			/* 0x006a */
+	__u8	reserved6b;		/* 0x006b */
 	__u32	todpr;			/* 0x006c */
 #define GISA_FORMAT1 0x00000001
 	__u32	gd;			/* 0x0070 */
@@ -938,6 +939,7 @@ struct kvm_arch{
 	int user_sigp;
 	int user_stsi;
 	int user_instr0;
+	int use_diag_318;
 	struct s390_io_adapter *adapters[MAX_S390_IO_ADAPTERS];
 	wait_queue_head_t ipte_wq;
 	int ipte_lock_count;
@@ -956,6 +958,7 @@ struct kvm_arch{
 	DECLARE_BITMAP(idle_mask, KVM_MAX_VCPUS);
 	struct kvm_s390_gisa_interrupt gisa_int;
 	struct kvm_s390_pv pv;
+	union diag_318_info diag_318_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 436ec7636927..2ce8e6f72206 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_DIAG_318	0
+
 /* for KVM_GET_REGS and KVM_SET_REGS */
 struct kvm_regs {
 	/* general purpose regs for s390 */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d05bb040fd42..cf8a3f1be839 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1243,6 +1243,70 @@ static int kvm_s390_get_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+static int kvm_s390_set_diag_318_info(struct kvm *kvm, __u64 addr)
+{
+	struct kvm_vcpu *vcpu;
+	u64 info;
+	int i;
+
+	if (get_user(info, (u64 __user *)addr))
+		return -EFAULT;
+
+	kvm->arch.diag_318_info.val = info;
+
+	if (sclp.has_diag_318) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			vcpu->arch.sie_block->cpnc = kvm->arch.diag_318_info.cpnc;
+		}
+	}
+
+	VM_EVENT(kvm, 3, "SET: CPNC: 0x%x CPVC: 0x%llx",
+		 kvm->arch.diag_318_info.cpnc, (u64)kvm->arch.diag_318_info.cpvc);
+
+	return 0;
+}
+
+static int kvm_s390_vm_set_misc(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_MISC_DIAG_318:
+		ret = kvm_s390_set_diag_318_info(kvm, attr->addr);
+		break;
+	default:
+		ret = -ENXIO;
+		break;
+	}
+	return ret;
+}
+
+static int kvm_s390_get_diag_318_info(struct kvm *kvm, __u64 addr)
+{
+	if (put_user(kvm->arch.diag_318_info.val, (u64 __user *)addr))
+		return -EFAULT;
+
+	VM_EVENT(kvm, 3, "QUERY: CPNC: 0x%x, CPVC: 0x%llx",
+		 kvm->arch.diag_318_info.cpnc, (u64)kvm->arch.diag_318_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_DIAG_318:
+		ret = kvm_s390_get_diag_318_info(kvm, attr->addr);
+		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;
@@ -1689,6 +1753,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_vm_set_misc(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1714,6 +1781,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;
@@ -1787,6 +1857,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_DIAG_318:
+			ret = 0;
+			break;
+		default:
+			ret = -ENXIO;
+			break;
+		}
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -3075,6 +3155,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_diag_318)
+		vcpu->arch.sie_block->cpnc = vcpu->kvm->arch.diag_318_info.cpnc;
 }
 
 static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 4f6c22d72072..a6820267e661 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_diag_318)
+		scb_s->cpnc = scb_o->cpnc;
 
 	prepare_ibc(vcpu, vsie_page);
 	rc = shadow_crycb(vcpu, vsie_page);
-- 
2.21.3

Re: [PATCH v7 2/3] s390: keep diag 318 variables consistent with the rest

[Thomas Huth]

On 16/05/2020 00.19, Collin Walling wrote:
> Rename diag318 to diag_318 and byte_134 to fac134 in order to keep
> naming schemes consistent with other diags and the read info struct
> and make grepping easier.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  arch/s390/include/asm/diag.h   | 2 +-
>  arch/s390/include/asm/sclp.h   | 2 +-
>  arch/s390/kernel/setup.c       | 6 +++---
>  drivers/s390/char/sclp.h       | 2 +-
>  drivers/s390/char/sclp_early.c | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
> index ca8f85b53a90..19da822e494c 100644
> --- a/arch/s390/include/asm/diag.h
> +++ b/arch/s390/include/asm/diag.h
> @@ -295,7 +295,7 @@ struct diag26c_mac_resp {
>  } __aligned(8);
>  
>  #define CPNC_LINUX		0x4
> -union diag318_info {
> +union diag_318_info {

$ grep -r diag.*info arch/s390/include/asm/diag.h
struct diag204_info_blk_hdr {
struct diag204_x_info_blk_hdr {
struct diag204_cpu_info {
struct diag204_x_cpu_info {
	struct diag204_x_cpu_info cpus[];
union diag318_info {

... none of these seem to use an underscore between the "diag" and the
number ... so this seems unnecessary to me ... or what do I miss?

 Thomas

Re: [PATCH v7 3/3] s390/kvm: diagnose 0x318 get/set handling

[Christian Borntraeger]

On 16.05.20 00:19, Collin Walling wrote:
> DIAGNOSE 0x318 (diag 318) sets information regarding the environment
> the VM is running in (Linux, z/VM, etc) and is observed via
> firmware/service events.
> 
> This is a privileged s390x instruction that must be intercepted by
> SIE. Userspace handling is required, so let's introduce some functions
> to communicate between userspace and KVM via ioctls. These will be used
> to get/set the diag 318 related information.
> 
> The Control Program Name Code (CPNC) is stored in the SIE block. The
> CPNC along with the Control Program Version Code (CPVC) are stored in
> the kvm_arch struct.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  Documentation/virt/kvm/devices/vm.rst | 21 +++++++
>  arch/s390/include/asm/kvm_host.h      |  5 +-
>  arch/s390/include/uapi/asm/kvm.h      |  4 ++
>  arch/s390/kvm/kvm-s390.c              | 82 +++++++++++++++++++++++++++
>  arch/s390/kvm/vsie.c                  |  2 +
>  5 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
> index 0aa5b1cfd700..52cc906dd7bd 100644
> --- a/Documentation/virt/kvm/devices/vm.rst
> +++ b/Documentation/virt/kvm/devices/vm.rst
> @@ -314,3 +314,24 @@ 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
> +==========================
> +
> +:Architectures: s390
> +
> +6.1. KVM_S390_VM_MISC_DIAG_318 (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.
> +
> +: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


An alternative would be a new sync_reg value + KVM capability.

Re: [PATCH v7 2/3] s390: keep diag 318 variables consistent with the rest

[Collin Walling]

On 5/18/20 2:30 AM, Thomas Huth wrote:
> On 16/05/2020 00.19, Collin Walling wrote:
>> Rename diag318 to diag_318 and byte_134 to fac134 in order to keep
>> naming schemes consistent with other diags and the read info struct
>> and make grepping easier.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  arch/s390/include/asm/diag.h   | 2 +-
>>  arch/s390/include/asm/sclp.h   | 2 +-
>>  arch/s390/kernel/setup.c       | 6 +++---
>>  drivers/s390/char/sclp.h       | 2 +-
>>  drivers/s390/char/sclp_early.c | 2 +-
>>  5 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
>> index ca8f85b53a90..19da822e494c 100644
>> --- a/arch/s390/include/asm/diag.h
>> +++ b/arch/s390/include/asm/diag.h
>> @@ -295,7 +295,7 @@ struct diag26c_mac_resp {
>>  } __aligned(8);
>>  
>>  #define CPNC_LINUX		0x4
>> -union diag318_info {
>> +union diag_318_info {
> 
> $ grep -r diag.*info arch/s390/include/asm/diag.h
> struct diag204_info_blk_hdr {
> struct diag204_x_info_blk_hdr {
> struct diag204_cpu_info {
> struct diag204_x_cpu_info {
> 	struct diag204_x_cpu_info cpus[];
> union diag318_info {
> 
> ... none of these seem to use an underscore between the "diag" and the
> number ... so this seems unnecessary to me ... or what do I miss?
> 
>  Thomas
> 

I could have sworn I saw more cases with the underscore. I think I was
honing-in on a few cases in QEMU for whatever reason.

Let's just forget this patch was posted :)

-- 
--
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v7 3/3] s390/kvm: diagnose 0x318 get/set handling

[Collin Walling]

On 5/18/20 5:15 AM, Christian Borntraeger wrote:
> 
> 
> On 16.05.20 00:19, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag 318) sets information regarding the environment
>> the VM is running in (Linux, z/VM, etc) and is observed via
>> firmware/service events.
>>
>> This is a privileged s390x instruction that must be intercepted by
>> SIE. Userspace handling is required, so let's introduce some functions
>> to communicate between userspace and KVM via ioctls. These will be used
>> to get/set the diag 318 related information.
>>
>> The Control Program Name Code (CPNC) is stored in the SIE block. The
>> CPNC along with the Control Program Version Code (CPVC) are stored in
>> the kvm_arch struct.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  Documentation/virt/kvm/devices/vm.rst | 21 +++++++
>>  arch/s390/include/asm/kvm_host.h      |  5 +-
>>  arch/s390/include/uapi/asm/kvm.h      |  4 ++
>>  arch/s390/kvm/kvm-s390.c              | 82 +++++++++++++++++++++++++++
>>  arch/s390/kvm/vsie.c                  |  2 +
>>  5 files changed, 113 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
>> index 0aa5b1cfd700..52cc906dd7bd 100644
>> --- a/Documentation/virt/kvm/devices/vm.rst
>> +++ b/Documentation/virt/kvm/devices/vm.rst
>> @@ -314,3 +314,24 @@ 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
>> +==========================
>> +
>> +:Architectures: s390
>> +
>> +6.1. KVM_S390_VM_MISC_DIAG_318 (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.
>> +
>> +: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
> 
> 
> An alternative would be a new sync_reg value + KVM capability.
> 
> 

I did some investigation into this (new grounds for me)

sync_reg would handle the migration part, and then we'd just need the
ioctl so the diag instruction handler can set the data. Does that sound
right?

-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v7 3/3] s390/kvm: diagnose 0x318 get/set handling

[Christian Borntraeger]

On 21.05.20 08:15, Collin Walling wrote:
> On 5/18/20 5:15 AM, Christian Borntraeger wrote:
>>
>>
>> On 16.05.20 00:19, Collin Walling wrote:
>>> DIAGNOSE 0x318 (diag 318) sets information regarding the environment
>>> the VM is running in (Linux, z/VM, etc) and is observed via
>>> firmware/service events.
>>>
>>> This is a privileged s390x instruction that must be intercepted by
>>> SIE. Userspace handling is required, so let's introduce some functions
>>> to communicate between userspace and KVM via ioctls. These will be used
>>> to get/set the diag 318 related information.
>>>
>>> The Control Program Name Code (CPNC) is stored in the SIE block. The
>>> CPNC along with the Control Program Version Code (CPVC) are stored in
>>> the kvm_arch struct.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>  Documentation/virt/kvm/devices/vm.rst | 21 +++++++
>>>  arch/s390/include/asm/kvm_host.h      |  5 +-
>>>  arch/s390/include/uapi/asm/kvm.h      |  4 ++
>>>  arch/s390/kvm/kvm-s390.c              | 82 +++++++++++++++++++++++++++
>>>  arch/s390/kvm/vsie.c                  |  2 +
>>>  5 files changed, 113 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/virt/kvm/devices/vm.rst b/Documentation/virt/kvm/devices/vm.rst
>>> index 0aa5b1cfd700..52cc906dd7bd 100644
>>> --- a/Documentation/virt/kvm/devices/vm.rst
>>> +++ b/Documentation/virt/kvm/devices/vm.rst
>>> @@ -314,3 +314,24 @@ 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
>>> +==========================
>>> +
>>> +:Architectures: s390
>>> +
>>> +6.1. KVM_S390_VM_MISC_DIAG_318 (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.
>>> +
>>> +: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
>>
>>
>> An alternative would be a new sync_reg value + KVM capability.
>>
>>
> 
> I did some investigation into this (new grounds for me)
> 
> sync_reg would handle the migration part, and then we'd just need the
> ioctl so the diag instruction handler can set the data. Does that sound
> right?

We would not even need the ioctl. We could just export/import this all the
time. See the sync_regs and store_regs functions.