All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/2] Use DIAG318 to set Control Program Name & Version Codes
@ 2020-06-18 22:22 Collin Walling
  2020-06-18 22:22 ` [PATCH v8 1/2] s390/setup: diag 318: refactor struct Collin Walling
  2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
  0 siblings, 2 replies; 16+ messages in thread
From: Collin Walling @ 2020-06-18 22:22 UTC (permalink / raw)
  To: kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, david, cohuck, imbrenda,
	heiko.carstens, gor, thuth

Changelog:

    v8

    • Reset is handled in KVM during initial and clear resets
    • Sync/Store register handling
    • Removed device IOCTL code
    • Added KVM Capability for DIAG318
        - this is for determining if the CPU model can enable this feature
    • Reverted changes introduced by bullet 3 in v7
    • Unshadowing CPNC again, as it makes sense if the guest executing in
        VSIE sets a unique name/version code; this data should be preserved
        - reverts bullet 4 in v3
    • Diag318 is no longer reported via VM or CPU events
        - no place to put this such that the messages aren't flooding the logs
        - not necessary, as this data is primarily for IBM hardware/firmware
            service events, and is observable via such events (e.g. CPU ring
            dump)
        - was nice for testing purposes
    • A copy of the diag318 info (name & version code) is now stored in
        the kvm_vcpu_arch struct, as opposed to the kvm_arch struct

    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 [removed in v8]
        - 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 [removed in v8]

    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 [removed in v8]
    • 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 the SIE block, and
the CPNC & CPVC pair is stored in the kvm_vcpu_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 (2):
  s390/setup: diag 318: refactor struct
  s390/kvm: diagnose 0x318 sync and reset

 arch/s390/include/asm/diag.h     |  6 ++----
 arch/s390/include/asm/kvm_host.h |  4 +++-
 arch/s390/include/uapi/asm/kvm.h |  5 ++++-
 arch/s390/kernel/setup.c         |  3 +--
 arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
 arch/s390/kvm/vsie.c             |  3 +++
 include/uapi/linux/kvm.h         |  1 +
 7 files changed, 24 insertions(+), 9 deletions(-)

-- 
2.21.3

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v8 1/2] s390/setup: diag 318: refactor struct
  2020-06-18 22:22 [PATCH v8 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
@ 2020-06-18 22:22 ` Collin Walling
  2020-06-22 14:56   ` Christian Borntraeger
  2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
  1 sibling, 1 reply; 16+ messages in thread
From: Collin Walling @ 2020-06-18 22:22 UTC (permalink / raw)
  To: kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, david, cohuck, imbrenda,
	heiko.carstens, gor, thuth

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 5853c9872dfe..878cacfc9c3e 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -1021,8 +1021,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-18 22:22 [PATCH v8 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
  2020-06-18 22:22 ` [PATCH v8 1/2] s390/setup: diag 318: refactor struct Collin Walling
@ 2020-06-18 22:22 ` Collin Walling
  2020-06-19 11:02   ` Janosch Frank
                     ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Collin Walling @ 2020-06-18 22:22 UTC (permalink / raw)
  To: kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, david, cohuck, imbrenda,
	heiko.carstens, gor, thuth

DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
is communicated via VCPU register synchronization.

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_vcpu_arch struct.

The CPNC is shadowed/unshadowed in VSIE.

This data is reset on load normal and clear resets.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  4 +++-
 arch/s390/include/uapi/asm/kvm.h |  5 ++++-
 arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
 arch/s390/kvm/vsie.c             |  3 +++
 include/uapi/linux/kvm.h         |  1 +
 5 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 3d554887794e..8bdf6f1607ca 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 */
@@ -745,6 +746,7 @@ struct kvm_vcpu_arch {
 	bool gs_enabled;
 	bool skey_enabled;
 	struct kvm_s390_pv_vcpu pv;
+	union diag318_info diag318_info;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 436ec7636927..2ae1b660086c 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+#define KVM_SYNC_DIAG318 (1UL << 12)
 
 #define KVM_SYNC_S390_VALID_FIELDS \
 	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
 	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
-	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
+	 KVM_SYNC_DIAG318)
 
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
@@ -254,6 +256,7 @@ struct kvm_sync_regs {
 	__u64 pft;	/* pfault token [PFAULT] */
 	__u64 pfs;	/* pfault select [PFAULT] */
 	__u64 pfc;	/* pfault compare [PFAULT] */
+	__u64 diag318;	/* diagnose 0x318 info */
 	union {
 		__u64 vrs[32][2];	/* vector registers (KVM_SYNC_VRS) */
 		__u64 fprs[16];		/* fp registers (KVM_SYNC_FPRS) */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d0ff26d157bc..b05ad718b64b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -545,6 +545,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_AIS_MIGRATION:
 	case KVM_CAP_S390_VCPU_RESETS:
 	case KVM_CAP_SET_GUEST_DEBUG:
+	case KVM_CAP_S390_DIAG318:
 		r = 1;
 		break;
 	case KVM_CAP_S390_HPAGE_1M:
@@ -3267,7 +3268,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 				    KVM_SYNC_ACRS |
 				    KVM_SYNC_CRS |
 				    KVM_SYNC_ARCH0 |
-				    KVM_SYNC_PFAULT;
+				    KVM_SYNC_PFAULT |
+				    KVM_SYNC_DIAG318;
 	kvm_s390_set_prefix(vcpu, 0);
 	if (test_kvm_facility(vcpu->kvm, 64))
 		vcpu->run->kvm_valid_regs |= KVM_SYNC_RICCB;
@@ -3562,6 +3564,7 @@ static void kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 		vcpu->arch.sie_block->pp = 0;
 		vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
 		vcpu->arch.sie_block->todpr = 0;
+		vcpu->arch.sie_block->cpnc = 0;
 	}
 }
 
@@ -3579,6 +3582,7 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
 
 	regs->etoken = 0;
 	regs->etoken_extension = 0;
+	regs->diag318 = 0;
 }
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
@@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
 			kvm_clear_async_pf_completion_queue(vcpu);
 	}
+	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
+		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
+		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
+	}
 	/*
 	 * If userspace sets the riccb (e.g. after migration) to a valid state,
 	 * we should enable RI here instead of doing the lazy enablement.
@@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
 	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
 	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
+	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
 	if (MACHINE_HAS_GS) {
 		__ctl_set_bit(2, 4);
 		if (vcpu->arch.gs_enabled)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 9e9056cebfcf..ba83d0568bc7 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		break;
 	}
 
+	scb_o->cpnc = scb_s->cpnc;
+
 	if (scb_s->ihcpu != 0xffffU)
 		scb_o->ihcpu = scb_s->ihcpu;
 }
@@ -548,6 +550,7 @@ 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;
+	scb_s->cpnc = scb_o->cpnc;
 
 	prepare_ibc(vcpu, vsie_page);
 	rc = shadow_crycb(vcpu, vsie_page);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4fdf30316582..35cdb4307904 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_S390_DIAG318 184
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.21.3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
@ 2020-06-19 11:02   ` Janosch Frank
  2020-06-19 14:45     ` Collin Walling
  2020-06-19 14:52   ` David Hildenbrand
  2020-06-22 10:24   ` Cornelia Huck
  2 siblings, 1 reply; 16+ messages in thread
From: Janosch Frank @ 2020-06-19 11:02 UTC (permalink / raw)
  To: Collin Walling, kvm, linux-s390
  Cc: pbonzini, borntraeger, david, cohuck, imbrenda, heiko.carstens,
	gor, thuth


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

On 6/19/20 12:22 AM, Collin Walling wrote:
> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
> is communicated via VCPU register synchronization.
> 
> 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_vcpu_arch struct.
> 
> The CPNC is shadowed/unshadowed in VSIE.
> 
> This data is reset on load normal and clear resets.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Could you extend the s390 kvm selftests sync_regs test with diag318 please?

I'd also like to have it added to the kvm unit tests. You can either do
that yourself or I'll add it when I go over my pending patches. Since we
can't retrieve these values from the VM, a simple check for the sclp
feature bit and an execution of the instruction would be enough.

> ---
>  arch/s390/include/asm/kvm_host.h |  4 +++-
>  arch/s390/include/uapi/asm/kvm.h |  5 ++++-
>  arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
>  arch/s390/kvm/vsie.c             |  3 +++
>  include/uapi/linux/kvm.h         |  1 +
>  5 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 3d554887794e..8bdf6f1607ca 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 */
> @@ -745,6 +746,7 @@ struct kvm_vcpu_arch {
>  	bool gs_enabled;
>  	bool skey_enabled;
>  	struct kvm_s390_pv_vcpu pv;
> +	union diag318_info diag318_info;
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 436ec7636927..2ae1b660086c 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
>  #define KVM_SYNC_GSCB   (1UL << 9)
>  #define KVM_SYNC_BPBC   (1UL << 10)
>  #define KVM_SYNC_ETOKEN (1UL << 11)
> +#define KVM_SYNC_DIAG318 (1UL << 12)
>  
>  #define KVM_SYNC_S390_VALID_FIELDS \
>  	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>  	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
> -	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
> +	 KVM_SYNC_DIAG318)
>  
>  /* length and alignment of the sdnx as a power of two */
>  #define SDNXC 8
> @@ -254,6 +256,7 @@ struct kvm_sync_regs {
>  	__u64 pft;	/* pfault token [PFAULT] */
>  	__u64 pfs;	/* pfault select [PFAULT] */
>  	__u64 pfc;	/* pfault compare [PFAULT] */
> +	__u64 diag318;	/* diagnose 0x318 info */
>  	union {
>  		__u64 vrs[32][2];	/* vector registers (KVM_SYNC_VRS) */
>  		__u64 fprs[16];		/* fp registers (KVM_SYNC_FPRS) */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d0ff26d157bc..b05ad718b64b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -545,6 +545,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_AIS_MIGRATION:
>  	case KVM_CAP_S390_VCPU_RESETS:
>  	case KVM_CAP_SET_GUEST_DEBUG:
> +	case KVM_CAP_S390_DIAG318:
>  		r = 1;
>  		break;
>  	case KVM_CAP_S390_HPAGE_1M:
> @@ -3267,7 +3268,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  				    KVM_SYNC_ACRS |
>  				    KVM_SYNC_CRS |
>  				    KVM_SYNC_ARCH0 |
> -				    KVM_SYNC_PFAULT;
> +				    KVM_SYNC_PFAULT |
> +				    KVM_SYNC_DIAG318;
>  	kvm_s390_set_prefix(vcpu, 0);
>  	if (test_kvm_facility(vcpu->kvm, 64))
>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_RICCB;
> @@ -3562,6 +3564,7 @@ static void kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>  		vcpu->arch.sie_block->pp = 0;
>  		vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>  		vcpu->arch.sie_block->todpr = 0;
> +		vcpu->arch.sie_block->cpnc = 0;
>  	}
>  }
>  
> @@ -3579,6 +3582,7 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>  
>  	regs->etoken = 0;
>  	regs->etoken_extension = 0;
> +	regs->diag318 = 0;
>  }
>  
>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>  			kvm_clear_async_pf_completion_queue(vcpu);
>  	}
> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
> +	}
>  	/*
>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>  	 * we should enable RI here instead of doing the lazy enablement.
> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>  	if (MACHINE_HAS_GS) {
>  		__ctl_set_bit(2, 4);
>  		if (vcpu->arch.gs_enabled)
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 9e9056cebfcf..ba83d0568bc7 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		break;
>  	}
>  
> +	scb_o->cpnc = scb_s->cpnc;
> +
>  	if (scb_s->ihcpu != 0xffffU)
>  		scb_o->ihcpu = scb_s->ihcpu;
>  }
> @@ -548,6 +550,7 @@ 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;
> +	scb_s->cpnc = scb_o->cpnc;
>  
>  	prepare_ibc(vcpu, vsie_page);
>  	rc = shadow_crycb(vcpu, vsie_page);
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4fdf30316582..35cdb4307904 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PPC_SECURE_GUEST 181
>  #define KVM_CAP_HALT_POLL 182
>  #define KVM_CAP_ASYNC_PF_INT 183
> +#define KVM_CAP_S390_DIAG318 184
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> 



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 11:02   ` Janosch Frank
@ 2020-06-19 14:45     ` Collin Walling
  0 siblings, 0 replies; 16+ messages in thread
From: Collin Walling @ 2020-06-19 14:45 UTC (permalink / raw)
  To: Janosch Frank, kvm, linux-s390
  Cc: pbonzini, borntraeger, david, cohuck, imbrenda, heiko.carstens,
	gor, thuth

On 6/19/20 7:02 AM, Janosch Frank wrote:
> On 6/19/20 12:22 AM, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>> is communicated via VCPU register synchronization.
>>
>> 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_vcpu_arch struct.
>>
>> The CPNC is shadowed/unshadowed in VSIE.
>>
>> This data is reset on load normal and clear resets.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Could you extend the s390 kvm selftests sync_regs test with diag318 please?
> 

Can do!

> I'd also like to have it added to the kvm unit tests. You can either do
> that yourself or I'll add it when I go over my pending patches. Since we
> can't retrieve these values from the VM, a simple check for the sclp
> feature bit and an execution of the instruction would be enough.
> 
>> ---
>>  arch/s390/include/asm/kvm_host.h |  4 +++-
>>  arch/s390/include/uapi/asm/kvm.h |  5 ++++-
>>  arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
>>  arch/s390/kvm/vsie.c             |  3 +++
>>  include/uapi/linux/kvm.h         |  1 +
>>  5 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 3d554887794e..8bdf6f1607ca 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 */
>> @@ -745,6 +746,7 @@ struct kvm_vcpu_arch {
>>  	bool gs_enabled;
>>  	bool skey_enabled;
>>  	struct kvm_s390_pv_vcpu pv;
>> +	union diag318_info diag318_info;
>>  };
>>  
>>  struct kvm_vm_stat {
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 436ec7636927..2ae1b660086c 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {
>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>> +#define KVM_SYNC_DIAG318 (1UL << 12)
>>  
>>  #define KVM_SYNC_S390_VALID_FIELDS \
>>  	(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>  	 KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>> -	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>> +	 KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \
>> +	 KVM_SYNC_DIAG318)
>>  
>>  /* length and alignment of the sdnx as a power of two */
>>  #define SDNXC 8
>> @@ -254,6 +256,7 @@ struct kvm_sync_regs {
>>  	__u64 pft;	/* pfault token [PFAULT] */
>>  	__u64 pfs;	/* pfault select [PFAULT] */
>>  	__u64 pfc;	/* pfault compare [PFAULT] */
>> +	__u64 diag318;	/* diagnose 0x318 info */
>>  	union {
>>  		__u64 vrs[32][2];	/* vector registers (KVM_SYNC_VRS) */
>>  		__u64 fprs[16];		/* fp registers (KVM_SYNC_FPRS) */
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d0ff26d157bc..b05ad718b64b 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -545,6 +545,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_S390_AIS_MIGRATION:
>>  	case KVM_CAP_S390_VCPU_RESETS:
>>  	case KVM_CAP_SET_GUEST_DEBUG:
>> +	case KVM_CAP_S390_DIAG318:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_S390_HPAGE_1M:
>> @@ -3267,7 +3268,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>  				    KVM_SYNC_ACRS |
>>  				    KVM_SYNC_CRS |
>>  				    KVM_SYNC_ARCH0 |
>> -				    KVM_SYNC_PFAULT;
>> +				    KVM_SYNC_PFAULT |
>> +				    KVM_SYNC_DIAG318;
>>  	kvm_s390_set_prefix(vcpu, 0);
>>  	if (test_kvm_facility(vcpu->kvm, 64))
>>  		vcpu->run->kvm_valid_regs |= KVM_SYNC_RICCB;
>> @@ -3562,6 +3564,7 @@ static void kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>  		vcpu->arch.sie_block->pp = 0;
>>  		vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>>  		vcpu->arch.sie_block->todpr = 0;
>> +		vcpu->arch.sie_block->cpnc = 0;
>>  	}
>>  }
>>  
>> @@ -3579,6 +3582,7 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
>>  
>>  	regs->etoken = 0;
>>  	regs->etoken_extension = 0;
>> +	regs->diag318 = 0;
>>  }
>>  
>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>  			kvm_clear_async_pf_completion_queue(vcpu);
>>  	}
>> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>> +	}
>>  	/*
>>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>>  	 * we should enable RI here instead of doing the lazy enablement.
>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>  	if (MACHINE_HAS_GS) {
>>  		__ctl_set_bit(2, 4);
>>  		if (vcpu->arch.gs_enabled)
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 9e9056cebfcf..ba83d0568bc7 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  		break;
>>  	}
>>  
>> +	scb_o->cpnc = scb_s->cpnc;
>> +
>>  	if (scb_s->ihcpu != 0xffffU)
>>  		scb_o->ihcpu = scb_s->ihcpu;
>>  }
>> @@ -548,6 +550,7 @@ 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;
>> +	scb_s->cpnc = scb_o->cpnc;
>>  
>>  	prepare_ibc(vcpu, vsie_page);
>>  	rc = shadow_crycb(vcpu, vsie_page);
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 4fdf30316582..35cdb4307904 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_PPC_SECURE_GUEST 181
>>  #define KVM_CAP_HALT_POLL 182
>>  #define KVM_CAP_ASYNC_PF_INT 183
>> +#define KVM_CAP_S390_DIAG318 184
>>  
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>  
>>
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
  2020-06-19 11:02   ` Janosch Frank
@ 2020-06-19 14:52   ` David Hildenbrand
  2020-06-19 15:47     ` Collin Walling
  2020-06-22 10:24   ` Cornelia Huck
  2 siblings, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2020-06-19 14:52 UTC (permalink / raw)
  To: Collin Walling, kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, cohuck, imbrenda, heiko.carstens,
	gor, thuth

On 19.06.20 00:22, Collin Walling wrote:
> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
> is communicated via VCPU register synchronization.
> 
> 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_vcpu_arch struct.
> 
> The CPNC is shadowed/unshadowed in VSIE.
> 

[...]

>  
>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>  			kvm_clear_async_pf_completion_queue(vcpu);
>  	}
> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
> +	}
>  	/*
>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>  	 * we should enable RI here instead of doing the lazy enablement.
> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>  	if (MACHINE_HAS_GS) {
>  		__ctl_set_bit(2, 4);
>  		if (vcpu->arch.gs_enabled)
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 9e9056cebfcf..ba83d0568bc7 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		break;
>  	}
>  
> +	scb_o->cpnc = scb_s->cpnc;

"This is a privileged s390x instruction that must be intercepted", how
can the cpnc change, then, while in SIE?

Apart from that LGTM.

-- 
Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 14:52   ` David Hildenbrand
@ 2020-06-19 15:47     ` Collin Walling
  2020-06-19 17:17       ` David Hildenbrand
  0 siblings, 1 reply; 16+ messages in thread
From: Collin Walling @ 2020-06-19 15:47 UTC (permalink / raw)
  To: David Hildenbrand, kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, cohuck, imbrenda, heiko.carstens,
	gor, thuth

On 6/19/20 10:52 AM, David Hildenbrand wrote:
> On 19.06.20 00:22, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>> is communicated via VCPU register synchronization.
>>
>> 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_vcpu_arch struct.
>>
>> The CPNC is shadowed/unshadowed in VSIE.
>>
> 
> [...]
> 
>>  
>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>  			kvm_clear_async_pf_completion_queue(vcpu);
>>  	}
>> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>> +	}
>>  	/*
>>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>>  	 * we should enable RI here instead of doing the lazy enablement.
>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>  	if (MACHINE_HAS_GS) {
>>  		__ctl_set_bit(2, 4);
>>  		if (vcpu->arch.gs_enabled)
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 9e9056cebfcf..ba83d0568bc7 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  		break;
>>  	}
>>  
>> +	scb_o->cpnc = scb_s->cpnc;
> 
> "This is a privileged s390x instruction that must be intercepted", how
> can the cpnc change, then, while in SIE?
> 
> Apart from that LGTM.
> 

I thought shadow/unshadow was a load/store (respectively) when executing
in SIE for a level 3+ guest (where LPAR is level 1)?

* Shadow SCB (load shadow VSIE page; originally CPNC is 0)

* Execute diag318 (under SIE)

* Unshadow SCB (store in original VSIE page; CPNC is whatever code the
guest decided to set)

Don't we need to preserve the CPNC for the level 3+ guest somehow?

-- 
Regards,
Collin

Stay safe and stay healthy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 15:47     ` Collin Walling
@ 2020-06-19 17:17       ` David Hildenbrand
  2020-06-19 17:55         ` Collin Walling
  0 siblings, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2020-06-19 17:17 UTC (permalink / raw)
  To: Collin Walling, kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, cohuck, imbrenda, heiko.carstens,
	gor, thuth

On 19.06.20 17:47, Collin Walling wrote:
> On 6/19/20 10:52 AM, David Hildenbrand wrote:
>> On 19.06.20 00:22, Collin Walling wrote:
>>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>>> is communicated via VCPU register synchronization.
>>>
>>> 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_vcpu_arch struct.
>>>
>>> The CPNC is shadowed/unshadowed in VSIE.
>>>
>>
>> [...]
>>
>>>  
>>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>>  			kvm_clear_async_pf_completion_queue(vcpu);
>>>  	}
>>> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>>> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>>> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>>> +	}
>>>  	/*
>>>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>>>  	 * we should enable RI here instead of doing the lazy enablement.
>>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>>> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>>  	if (MACHINE_HAS_GS) {
>>>  		__ctl_set_bit(2, 4);
>>>  		if (vcpu->arch.gs_enabled)
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index 9e9056cebfcf..ba83d0568bc7 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  		break;
>>>  	}
>>>  
>>> +	scb_o->cpnc = scb_s->cpnc;
>>
>> "This is a privileged s390x instruction that must be intercepted", how
>> can the cpnc change, then, while in SIE?
>>
>> Apart from that LGTM.
>>
> 
> I thought shadow/unshadow was a load/store (respectively) when executing
> in SIE for a level 3+ guest (where LPAR is level 1)?
> 
> * Shadow SCB (load shadow VSIE page; originally CPNC is 0)

1. Here, you copy the cpnc from the pinned (original) SCB to the shadow SCB.

> 
> * Execute diag318 (under SIE)

2. Here the SIE runs using the shadow SCB.

> 
> * Unshadow SCB (store in original VSIE page; CPNC is whatever code the
> guest decided to set)

3. Here you copy back the cpnc from the shadow SCB to the pinned
(original) SCB.


If 2. cannot modify the cpnc residing in the shadow SCB, 3. can be
dropped, because the values will always match.


If guest3 tries to modify the cpnc (via diag 318), we exit the SIE
(intercept) in 2., return to our guest 2. guest 2 will perform the
change and adapt the original SCB.

(yep, it's confusing)

Or did I miss anything?

-- 
Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 17:17       ` David Hildenbrand
@ 2020-06-19 17:55         ` Collin Walling
  2020-06-19 18:13           ` David Hildenbrand
  0 siblings, 1 reply; 16+ messages in thread
From: Collin Walling @ 2020-06-19 17:55 UTC (permalink / raw)
  To: David Hildenbrand, kvm, linux-s390
  Cc: pbonzini, borntraeger, frankja, cohuck, imbrenda, heiko.carstens,
	gor, thuth

On 6/19/20 1:17 PM, David Hildenbrand wrote:
> On 19.06.20 17:47, Collin Walling wrote:
>> On 6/19/20 10:52 AM, David Hildenbrand wrote:
>>> On 19.06.20 00:22, Collin Walling wrote:
>>>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>>>> is communicated via VCPU register synchronization.
>>>>
>>>> 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_vcpu_arch struct.
>>>>
>>>> The CPNC is shadowed/unshadowed in VSIE.
>>>>
>>>
>>> [...]
>>>
>>>>  
>>>>  int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>  		if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>>>  			kvm_clear_async_pf_completion_queue(vcpu);
>>>>  	}
>>>> +	if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>>>> +		vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>>>> +		vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>>>> +	}
>>>>  	/*
>>>>  	 * If userspace sets the riccb (e.g. after migration) to a valid state,
>>>>  	 * we should enable RI here instead of doing the lazy enablement.
>>>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>  	kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>>>  	kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>>>  	kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>>>> +	kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>>>  	if (MACHINE_HAS_GS) {
>>>>  		__ctl_set_bit(2, 4);
>>>>  		if (vcpu->arch.gs_enabled)
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index 9e9056cebfcf..ba83d0568bc7 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  		break;
>>>>  	}
>>>>  
>>>> +	scb_o->cpnc = scb_s->cpnc;
>>>
>>> "This is a privileged s390x instruction that must be intercepted", how
>>> can the cpnc change, then, while in SIE?
>>>
>>> Apart from that LGTM.
>>>
>>
>> I thought shadow/unshadow was a load/store (respectively) when executing
>> in SIE for a level 3+ guest (where LPAR is level 1)?
>>
>> * Shadow SCB (load shadow VSIE page; originally CPNC is 0)
> 
> 1. Here, you copy the cpnc from the pinned (original) SCB to the shadow SCB.
> 
>>
>> * Execute diag318 (under SIE)
> 
> 2. Here the SIE runs using the shadow SCB.
> 
>>
>> * Unshadow SCB (store in original VSIE page; CPNC is whatever code the
>> guest decided to set)
> 
> 3. Here you copy back the cpnc from the shadow SCB to the pinned
> (original) SCB.
> 
> 
> If 2. cannot modify the cpnc residing in the shadow SCB, 3. can be
> dropped, because the values will always match.
> 
> 
> If guest3 tries to modify the cpnc (via diag 318), we exit the SIE
> (intercept) in 2., return to our guest 2. guest 2 will perform the
> change and adapt the original SCB.
> 
> (yep, it's confusing)
> 
> Or did I miss anything?
> 

Ah, I see. So the shadowing isn't necessarily for SIE block values, but
for storing the register / PSW / clock states, as well as facility bits
for the level 3+ guests? Looking at what the vsie code does, that seems
to make sense.

So we don't need to shadow OR unshadow the CPNC, then?

-- 
Regards,
Collin

Stay safe and stay healthy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 17:55         ` Collin Walling
@ 2020-06-19 18:13           ` David Hildenbrand
  2020-06-19 18:46             ` Collin Walling
  0 siblings, 1 reply; 16+ messages in thread
From: David Hildenbrand @ 2020-06-19 18:13 UTC (permalink / raw)
  To: Collin Walling
  Cc: David Hildenbrand, kvm, linux-s390, pbonzini, borntraeger,
	frankja, cohuck, imbrenda, heiko.carstens, gor, thuth



> Am 19.06.2020 um 19:56 schrieb Collin Walling <walling@linux.ibm.com>:
> 
> On 6/19/20 1:17 PM, David Hildenbrand wrote:
>>> On 19.06.20 17:47, Collin Walling wrote:
>>> On 6/19/20 10:52 AM, David Hildenbrand wrote:
>>>> On 19.06.20 00:22, Collin Walling wrote:
>>>>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>>>>> is communicated via VCPU register synchronization.
>>>>> 
>>>>> 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_vcpu_arch struct.
>>>>> 
>>>>> The CPNC is shadowed/unshadowed in VSIE.
>>>>> 
>>>> 
>>>> [...]
>>>> 
>>>>> 
>>>>> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>>        if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>>>>            kvm_clear_async_pf_completion_queue(vcpu);
>>>>>    }
>>>>> +    if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>>>>> +        vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>>>>> +        vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>>>>> +    }
>>>>>    /*
>>>>>     * If userspace sets the riccb (e.g. after migration) to a valid state,
>>>>>     * we should enable RI here instead of doing the lazy enablement.
>>>>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>>    kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>>>>    kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>>>>    kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>>>>> +    kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>>>>    if (MACHINE_HAS_GS) {
>>>>>        __ctl_set_bit(2, 4);
>>>>>        if (vcpu->arch.gs_enabled)
>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>> index 9e9056cebfcf..ba83d0568bc7 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>        break;
>>>>>    }
>>>>> 
>>>>> +    scb_o->cpnc = scb_s->cpnc;
>>>> 
>>>> "This is a privileged s390x instruction that must be intercepted", how
>>>> can the cpnc change, then, while in SIE?
>>>> 
>>>> Apart from that LGTM.
>>>> 
>>> 
>>> I thought shadow/unshadow was a load/store (respectively) when executing
>>> in SIE for a level 3+ guest (where LPAR is level 1)?
>>> 
>>> * Shadow SCB (load shadow VSIE page; originally CPNC is 0)
>> 
>> 1. Here, you copy the cpnc from the pinned (original) SCB to the shadow SCB.
>> 
>>> 
>>> * Execute diag318 (under SIE)
>> 
>> 2. Here the SIE runs using the shadow SCB.
>> 
>>> 
>>> * Unshadow SCB (store in original VSIE page; CPNC is whatever code the
>>> guest decided to set)
>> 
>> 3. Here you copy back the cpnc from the shadow SCB to the pinned
>> (original) SCB.
>> 
>> 
>> If 2. cannot modify the cpnc residing in the shadow SCB, 3. can be
>> dropped, because the values will always match.
>> 
>> 
>> If guest3 tries to modify the cpnc (via diag 318), we exit the SIE
>> (intercept) in 2., return to our guest 2. guest 2 will perform the
>> change and adapt the original SCB.
>> 
>> (yep, it's confusing)
>> 
>> Or did I miss anything?
>> 
> 
> Ah, I see. So the shadowing isn't necessarily for SIE block values, but
> for storing the register / PSW / clock states, as well as facility bits
> for the level 3+ guests? Looking at what the

We have to forward all values the SIE has to see and copy back only what could have been changed by the SIE.

> vsie code does, that seems
> to make sense.
> 
> So we don't need to shadow OR unshadow the CPNC, then?

I think you have to shadow (forward the value) but not unshadow (value cannot change).

Cheers!

> 
> -- 
> Regards,
> Collin
> 
> Stay safe and stay healthy
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-19 18:13           ` David Hildenbrand
@ 2020-06-19 18:46             ` Collin Walling
  0 siblings, 0 replies; 16+ messages in thread
From: Collin Walling @ 2020-06-19 18:46 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: kvm, linux-s390, pbonzini, borntraeger, frankja, cohuck,
	imbrenda, heiko.carstens, gor, thuth

On 6/19/20 2:13 PM, David Hildenbrand wrote:
> 
> 
>> Am 19.06.2020 um 19:56 schrieb Collin Walling <walling@linux.ibm.com>:
>>
>> On 6/19/20 1:17 PM, David Hildenbrand wrote:
>>>> On 19.06.20 17:47, Collin Walling wrote:
>>>> On 6/19/20 10:52 AM, David Hildenbrand wrote:
>>>>> On 19.06.20 00:22, Collin Walling wrote:
>>>>>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>>>>>> is communicated via VCPU register synchronization.
>>>>>>
>>>>>> 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_vcpu_arch struct.
>>>>>>
>>>>>> The CPNC is shadowed/unshadowed in VSIE.
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>>>>>> @@ -4194,6 +4198,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>>>        if (vcpu->arch.pfault_token == KVM_S390_PFAULT_TOKEN_INVALID)
>>>>>>            kvm_clear_async_pf_completion_queue(vcpu);
>>>>>>    }
>>>>>> +    if (kvm_run->kvm_dirty_regs & KVM_SYNC_DIAG318) {
>>>>>> +        vcpu->arch.diag318_info.val = kvm_run->s.regs.diag318;
>>>>>> +        vcpu->arch.sie_block->cpnc = vcpu->arch.diag318_info.cpnc;
>>>>>> +    }
>>>>>>    /*
>>>>>>     * If userspace sets the riccb (e.g. after migration) to a valid state,
>>>>>>     * we should enable RI here instead of doing the lazy enablement.
>>>>>> @@ -4295,6 +4303,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>>>>    kvm_run->s.regs.pp = vcpu->arch.sie_block->pp;
>>>>>>    kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea;
>>>>>>    kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf & FPF_BPBC) == FPF_BPBC;
>>>>>> +    kvm_run->s.regs.diag318 = vcpu->arch.diag318_info.val;
>>>>>>    if (MACHINE_HAS_GS) {
>>>>>>        __ctl_set_bit(2, 4);
>>>>>>        if (vcpu->arch.gs_enabled)
>>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>>> index 9e9056cebfcf..ba83d0568bc7 100644
>>>>>> --- a/arch/s390/kvm/vsie.c
>>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>>> @@ -423,6 +423,8 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>>        break;
>>>>>>    }
>>>>>>
>>>>>> +    scb_o->cpnc = scb_s->cpnc;
>>>>>
>>>>> "This is a privileged s390x instruction that must be intercepted", how
>>>>> can the cpnc change, then, while in SIE?
>>>>>
>>>>> Apart from that LGTM.
>>>>>
>>>>
>>>> I thought shadow/unshadow was a load/store (respectively) when executing
>>>> in SIE for a level 3+ guest (where LPAR is level 1)?
>>>>
>>>> * Shadow SCB (load shadow VSIE page; originally CPNC is 0)
>>>
>>> 1. Here, you copy the cpnc from the pinned (original) SCB to the shadow SCB.
>>>
>>>>
>>>> * Execute diag318 (under SIE)
>>>
>>> 2. Here the SIE runs using the shadow SCB.
>>>
>>>>
>>>> * Unshadow SCB (store in original VSIE page; CPNC is whatever code the
>>>> guest decided to set)
>>>
>>> 3. Here you copy back the cpnc from the shadow SCB to the pinned
>>> (original) SCB.
>>>
>>>
>>> If 2. cannot modify the cpnc residing in the shadow SCB, 3. can be
>>> dropped, because the values will always match.
>>>
>>>
>>> If guest3 tries to modify the cpnc (via diag 318), we exit the SIE
>>> (intercept) in 2., return to our guest 2. guest 2 will perform the
>>> change and adapt the original SCB.
>>>
>>> (yep, it's confusing)
>>>
>>> Or did I miss anything?
>>>
>>
>> Ah, I see. So the shadowing isn't necessarily for SIE block values, but
>> for storing the register / PSW / clock states, as well as facility bits
>> for the level 3+ guests? Looking at what the
> 
> We have to forward all values the SIE has to see and copy back only what could have been changed by the SIE.
> 
>> vsie code does, that seems
>> to make sense.
>>
>> So we don't need to shadow OR unshadow the CPNC, then?
> 
> I think you have to shadow (forward the value) but not unshadow (value cannot change).
> 
> Cheers!
> 

Gotcha. Very tricky. I'll have to study on it some more. Thanks for the
info!

Take care.

>>
>> -- 
>> Regards,
>> Collin
>>
>> Stay safe and stay healthy
>>
> 


-- 
Regards,
Collin

Stay safe and stay healthy

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
  2020-06-19 11:02   ` Janosch Frank
  2020-06-19 14:52   ` David Hildenbrand
@ 2020-06-22 10:24   ` Cornelia Huck
  2020-06-22 14:50     ` Christian Borntraeger
  2 siblings, 1 reply; 16+ messages in thread
From: Cornelia Huck @ 2020-06-22 10:24 UTC (permalink / raw)
  To: Collin Walling
  Cc: kvm, linux-s390, pbonzini, borntraeger, frankja, david, imbrenda,
	heiko.carstens, gor, thuth

On Thu, 18 Jun 2020 18:22:22 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
> is communicated via VCPU register synchronization.
> 
> 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_vcpu_arch struct.
> 
> The CPNC is shadowed/unshadowed in VSIE.
> 
> This data is reset on load normal and clear resets.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  4 +++-
>  arch/s390/include/uapi/asm/kvm.h |  5 ++++-
>  arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
>  arch/s390/kvm/vsie.c             |  3 +++
>  include/uapi/linux/kvm.h         |  1 +
>  5 files changed, 21 insertions(+), 3 deletions(-)
> 

(...)

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4fdf30316582..35cdb4307904 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PPC_SECURE_GUEST 181
>  #define KVM_CAP_HALT_POLL 182
>  #define KVM_CAP_ASYNC_PF_INT 183
> +#define KVM_CAP_S390_DIAG318 184

Do we strictly need this new cap, or would checking against the sync
regs capabilities be enough?

>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-22 10:24   ` Cornelia Huck
@ 2020-06-22 14:50     ` Christian Borntraeger
  2020-06-22 15:03       ` Cornelia Huck
  0 siblings, 1 reply; 16+ messages in thread
From: Christian Borntraeger @ 2020-06-22 14:50 UTC (permalink / raw)
  To: Cornelia Huck, Collin Walling
  Cc: kvm, linux-s390, pbonzini, frankja, david, imbrenda,
	heiko.carstens, gor, thuth



On 22.06.20 12:24, Cornelia Huck wrote:
> On Thu, 18 Jun 2020 18:22:22 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
>> is communicated via VCPU register synchronization.
>>
>> 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_vcpu_arch struct.
>>
>> The CPNC is shadowed/unshadowed in VSIE.
>>
>> This data is reset on load normal and clear resets.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h |  4 +++-
>>  arch/s390/include/uapi/asm/kvm.h |  5 ++++-
>>  arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
>>  arch/s390/kvm/vsie.c             |  3 +++
>>  include/uapi/linux/kvm.h         |  1 +
>>  5 files changed, 21 insertions(+), 3 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 4fdf30316582..35cdb4307904 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_PPC_SECURE_GUEST 181
>>  #define KVM_CAP_HALT_POLL 182
>>  #define KVM_CAP_ASYNC_PF_INT 183
>> +#define KVM_CAP_S390_DIAG318 184
> 
> Do we strictly need this new cap, or would checking against the sync
> regs capabilities be enough?

We could check the sync_regs valid field to decide about the sync. We do
that for ETOKEN as well and QEMU also uses it in handle_diag_318.

I think what this is used for is actually to tell the QEMU CPU model
if this is there. And for that the sync_reg validity seems wrong. So better
keep the CAP?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 1/2] s390/setup: diag 318: refactor struct
  2020-06-18 22:22 ` [PATCH v8 1/2] s390/setup: diag 318: refactor struct Collin Walling
@ 2020-06-22 14:56   ` Christian Borntraeger
  2020-06-22 15:37     ` Collin Walling
  0 siblings, 1 reply; 16+ messages in thread
From: Christian Borntraeger @ 2020-06-22 14:56 UTC (permalink / raw)
  To: Collin Walling, kvm, linux-s390
  Cc: pbonzini, frankja, david, cohuck, imbrenda, heiko.carstens, gor, thuth


On 19.06.20 00:22, Collin Walling wrote:
> 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>

Series looks good to me. Can you respin the 2nd patch regarding the VSIE things
and I can then apply it.


> ---
>  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 5853c9872dfe..878cacfc9c3e 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -1021,8 +1021,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)
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset
  2020-06-22 14:50     ` Christian Borntraeger
@ 2020-06-22 15:03       ` Cornelia Huck
  0 siblings, 0 replies; 16+ messages in thread
From: Cornelia Huck @ 2020-06-22 15:03 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Collin Walling, kvm, linux-s390, pbonzini, frankja, david,
	imbrenda, heiko.carstens, gor, thuth

On Mon, 22 Jun 2020 16:50:41 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 22.06.20 12:24, Cornelia Huck wrote:
> > On Thu, 18 Jun 2020 18:22:22 -0400
> > Collin Walling <walling@linux.ibm.com> wrote:
> >   
> >> DIAGNOSE 0x318 (diag318) 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 handles the instruction as well as migration. Data
> >> is communicated via VCPU register synchronization.
> >>
> >> 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_vcpu_arch struct.
> >>
> >> The CPNC is shadowed/unshadowed in VSIE.
> >>
> >> This data is reset on load normal and clear resets.
> >>
> >> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> >> ---
> >>  arch/s390/include/asm/kvm_host.h |  4 +++-
> >>  arch/s390/include/uapi/asm/kvm.h |  5 ++++-
> >>  arch/s390/kvm/kvm-s390.c         | 11 ++++++++++-
> >>  arch/s390/kvm/vsie.c             |  3 +++
> >>  include/uapi/linux/kvm.h         |  1 +
> >>  5 files changed, 21 insertions(+), 3 deletions(-)
> >>  
> > 
> > (...)
> >   
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index 4fdf30316582..35cdb4307904 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
> >>  #define KVM_CAP_PPC_SECURE_GUEST 181
> >>  #define KVM_CAP_HALT_POLL 182
> >>  #define KVM_CAP_ASYNC_PF_INT 183
> >> +#define KVM_CAP_S390_DIAG318 184  
> > 
> > Do we strictly need this new cap, or would checking against the sync
> > regs capabilities be enough?  
> 
> We could check the sync_regs valid field to decide about the sync. We do
> that for ETOKEN as well and QEMU also uses it in handle_diag_318.
> 
> I think what this is used for is actually to tell the QEMU CPU model
> if this is there. And for that the sync_reg validity seems wrong. So better
> keep the CAP?
> 

Ok, makes sense.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v8 1/2] s390/setup: diag 318: refactor struct
  2020-06-22 14:56   ` Christian Borntraeger
@ 2020-06-22 15:37     ` Collin Walling
  0 siblings, 0 replies; 16+ messages in thread
From: Collin Walling @ 2020-06-22 15:37 UTC (permalink / raw)
  To: Christian Borntraeger, kvm, linux-s390
  Cc: pbonzini, frankja, david, cohuck, imbrenda, heiko.carstens, gor, thuth

On 6/22/20 10:56 AM, Christian Borntraeger wrote:
> 
> On 19.06.20 00:22, Collin Walling wrote:
>> 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>
> 
> Series looks good to me. Can you respin the 2nd patch regarding the VSIE things
> and I can then apply it.
> 
> 

Will do. Thanks.

>> ---
>>  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 5853c9872dfe..878cacfc9c3e 100644
>> --- a/arch/s390/kernel/setup.c
>> +++ b/arch/s390/kernel/setup.c
>> @@ -1021,8 +1021,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)
>>


-- 
Regards,
Collin

Stay safe and stay healthy

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-06-22 15:37 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18 22:22 [PATCH v8 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
2020-06-18 22:22 ` [PATCH v8 1/2] s390/setup: diag 318: refactor struct Collin Walling
2020-06-22 14:56   ` Christian Borntraeger
2020-06-22 15:37     ` Collin Walling
2020-06-18 22:22 ` [PATCH v8 2/2] s390/kvm: diagnose 0x318 sync and reset Collin Walling
2020-06-19 11:02   ` Janosch Frank
2020-06-19 14:45     ` Collin Walling
2020-06-19 14:52   ` David Hildenbrand
2020-06-19 15:47     ` Collin Walling
2020-06-19 17:17       ` David Hildenbrand
2020-06-19 17:55         ` Collin Walling
2020-06-19 18:13           ` David Hildenbrand
2020-06-19 18:46             ` Collin Walling
2020-06-22 10:24   ` Cornelia Huck
2020-06-22 14:50     ` Christian Borntraeger
2020-06-22 15:03       ` Cornelia Huck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.