[PATCH RFC 0/2] KVM: s390: vsie: protect from VCPUS modifying the SCB

[PATCH RFC 0/2] KVM: s390: vsie: protect from VCPUS modifying the SCB

[David Hildenbrand]

As the (nested) SCB is just guest memory, other VCPUs can try to modify it
while we are building our shadow SCB.

Let's avoid unpredictable behavior by adding some READ_ONCE and refactoring
unpinning of satellite control blocks.

Compile tested only for now.

David Hildenbrand (2):
  KVM: s390 vsie: use READ_ONCE to access some SCB fields
  KVM: s390: vsie: store guest addresses of satellite blocks in
    vsie_page

 arch/s390/kvm/vsie.c | 87 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 34 deletions(-)

-- 
2.14.3

[PATCH RFC 1/2] KVM: s390 vsie: use READ_ONCE to access some SCB fields

[David Hildenbrand]

Another VCPU might try to modify the SCB while we are creating the
shadow SCB. In general this is no problem - unless the compiler decides
to not load values once, but e.g. twice.

For us, this is only relevant when checking/working with such values.
E.g. the prefix value, the mso, state of transactional execution and
addresses of satellite blocks.

E.g. if we blindly forwards values (e.g. general purpose registers or
execution controls after masking), we don't care.

Leaving unpin_blocks() untouched for now, will handle it separatly.

The worst thing right now that I can see would be a missed prefix
un/remap (mso, prefix, tx) or using wrong guest addresses. Nothing
critical, but let's try to avoid unpredictable behavior.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 5d6ae0326d9e..79ea444a0534 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -28,7 +28,11 @@ struct vsie_page {
 	 * the same offset as that in struct sie_page!
 	 */
 	struct mcck_volatile_info mcck_info;    /* 0x0200 */
-	/* the pinned originial scb */
+	/*
+	 * The pinned originial scb. Be aware that other VCPUs can modify
+	 * it while we read from it. Values that are used for conditions or
+	 * are reused conditionally, should be accessed via READ_ONCE.
+	 */
 	struct kvm_s390_sie_block *scb_o;	/* 0x0218 */
 	/* the shadow gmap in use by the vsie_page */
 	struct gmap *gmap;			/* 0x0220 */
@@ -140,12 +144,13 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
-	u32 crycb_addr = scb_o->crycbd & 0x7ffffff8U;
+	const uint32_t crycbd_o = READ_ONCE(scb_o->crycbd);
+	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
 	unsigned long *b1, *b2;
 	u8 ecb3_flags;
 
 	scb_s->crycbd = 0;
-	if (!(scb_o->crycbd & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
+	if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
 		return 0;
 	/* format-1 is supported with message-security-assist extension 3 */
 	if (!test_kvm_facility(vcpu->kvm, 76))
@@ -183,12 +188,15 @@ static void prepare_ibc(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
+	/* READ_ONCE does not work on bitfields - use a temporary variable */
+	const uint32_t __new_ibc = scb_o->ibc;
+	const uint32_t new_ibc = READ_ONCE(__new_ibc) & 0x0fffU;
 	__u64 min_ibc = (sclp.ibc >> 16) & 0x0fffU;
 
 	scb_s->ibc = 0;
 	/* ibc installed in g2 and requested for g3 */
-	if (vcpu->kvm->arch.model.ibc && (scb_o->ibc & 0x0fffU)) {
-		scb_s->ibc = scb_o->ibc & 0x0fffU;
+	if (vcpu->kvm->arch.model.ibc && new_ibc) {
+		scb_s->ibc = new_ibc;
 		/* takte care of the minimum ibc level of the machine */
 		if (scb_s->ibc < min_ibc)
 			scb_s->ibc = min_ibc;
@@ -253,6 +261,10 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
+	/* READ_ONCE does not work on bitfields - use a temporary variable */
+	const uint32_t __new_prefix = scb_o->prefix;
+	const uint32_t new_prefix = READ_ONCE(__new_prefix);
+	const bool wants_tx = READ_ONCE(scb_o->ecb) & ECB_TE;
 	bool had_tx = scb_s->ecb & ECB_TE;
 	unsigned long new_mso = 0;
 	int rc;
@@ -299,14 +311,14 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	scb_s->icpua = scb_o->icpua;
 
 	if (!(atomic_read(&scb_s->cpuflags) & CPUSTAT_SM))
-		new_mso = scb_o->mso & 0xfffffffffff00000UL;
+		new_mso = READ_ONCE(scb_o->mso) & 0xfffffffffff00000UL;
 	/* if the hva of the prefix changes, we have to remap the prefix */
-	if (scb_s->mso != new_mso || scb_s->prefix != scb_o->prefix)
+	if (scb_s->mso != new_mso || scb_s->prefix != new_prefix)
 		prefix_unmapped(vsie_page);
 	 /* SIE will do mso/msl validity and exception checks for us */
 	scb_s->msl = scb_o->msl & 0xfffffffffff00000UL;
 	scb_s->mso = new_mso;
-	scb_s->prefix = scb_o->prefix;
+	scb_s->prefix = new_prefix;
 
 	/* We have to definetly flush the tlb if this scb never ran */
 	if (scb_s->ihcpu != 0xffffU)
@@ -318,11 +330,11 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
 		scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
 	/* transactional execution */
-	if (test_kvm_facility(vcpu->kvm, 73)) {
+	if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
 		/* remap the prefix is tx is toggled on */
-		if ((scb_o->ecb & ECB_TE) && !had_tx)
+		if (!had_tx)
 			prefix_unmapped(vsie_page);
-		scb_s->ecb |= scb_o->ecb & ECB_TE;
+		scb_s->ecb |= ECB_TE;
 	}
 	/* SIMD */
 	if (test_kvm_facility(vcpu->kvm, 129)) {
@@ -529,9 +541,9 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	gpa_t gpa;
 	int rc = 0;
 
-	gpa = scb_o->scaol & ~0xfUL;
+	gpa = READ_ONCE(scb_o->scaol) & ~0xfUL;
 	if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_64BSCAO))
-		gpa |= (u64) scb_o->scaoh << 32;
+		gpa |= (u64) READ_ONCE(scb_o->scaoh) << 32;
 	if (gpa) {
 		if (!(gpa & ~0x1fffUL))
 			rc = set_validity_icpt(scb_s, 0x0038U);
@@ -551,7 +563,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		scb_s->scaol = (u32)(u64)hpa;
 	}
 
-	gpa = scb_o->itdba & ~0xffUL;
+	gpa = READ_ONCE(scb_o->itdba) & ~0xffUL;
 	if (gpa && (scb_s->ecb & ECB_TE)) {
 		if (!(gpa & ~0x1fffU)) {
 			rc = set_validity_icpt(scb_s, 0x0080U);
@@ -566,7 +578,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		scb_s->itdba = hpa;
 	}
 
-	gpa = scb_o->gvrd & ~0x1ffUL;
+	gpa = READ_ONCE(scb_o->gvrd) & ~0x1ffUL;
 	if (gpa && (scb_s->eca & ECA_VX) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
 		if (!(gpa & ~0x1fffUL)) {
 			rc = set_validity_icpt(scb_s, 0x1310U);
@@ -584,7 +596,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		scb_s->gvrd = hpa;
 	}
 
-	gpa = scb_o->riccbd & ~0x3fUL;
+	gpa = READ_ONCE(scb_o->riccbd) & ~0x3fUL;
 	if (gpa && (scb_s->ecb3 & ECB3_RI)) {
 		if (!(gpa & ~0x1fffUL)) {
 			rc = set_validity_icpt(scb_s, 0x0043U);
@@ -602,8 +614,8 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
 		unsigned long sdnxc;
 
-		gpa = scb_o->sdnxo & ~0xfUL;
-		sdnxc = scb_o->sdnxo & 0xfUL;
+		gpa = READ_ONCE(scb_o->sdnxo) & ~0xfUL;
+		sdnxc = READ_ONCE(scb_o->sdnxo) & 0xfUL;
 		if (!gpa || !(gpa & ~0x1fffUL)) {
 			rc = set_validity_icpt(scb_s, 0x10b0U);
 			goto unpin;
@@ -768,7 +780,7 @@ static void retry_vsie_icpt(struct vsie_page *vsie_page)
 static int handle_stfle(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
-	__u32 fac = vsie_page->scb_o->fac & 0x7ffffff8U;
+	__u32 fac = READ_ONCE(vsie_page->scb_o->fac) & 0x7ffffff8U;
 
 	if (fac && test_kvm_facility(vcpu->kvm, 7)) {
 		retry_vsie_icpt(vsie_page);
-- 
2.14.3

[PATCH RFC 2/2] KVM: s390: vsie: store guest addresses of satellite blocks in vsie_page

[David Hildenbrand]

This way, the values cannot changed, even if another VCPU might try to
mess with the nested SCB currently getting executed by another VCPU.

We now always use the same gpa for pinning and unpinning a page (for
unpinning, it is only relevant to mark the guest page dirty for
migration).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/vsie.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 79ea444a0534..546d8b106ee0 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -38,7 +38,13 @@ struct vsie_page {
 	struct gmap *gmap;			/* 0x0220 */
 	/* address of the last reported fault to guest2 */
 	unsigned long fault_addr;		/* 0x0228 */
-	__u8 reserved[0x0700 - 0x0230];		/* 0x0230 */
+	/* calculated guest addresses of satellite control blocks */
+	gpa_t sca_gpa;				/* 0x0230 */
+	gpa_t itdba_gpa;			/* 0x0238 */
+	gpa_t gvrd_gpa;				/* 0x0240 */
+	gpa_t riccbd_gpa;			/* 0x0248 */
+	gpa_t sdnx_gpa;				/* 0x0250 */
+	__u8 reserved[0x0700 - 0x0258];		/* 0x0258 */
 	struct kvm_s390_crypto_cb crycb;	/* 0x0700 */
 	__u8 fac[S390_ARCH_FAC_LIST_SIZE_BYTE];	/* 0x0800 */
 };
@@ -475,46 +481,42 @@ static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
 /* unpin all blocks previously pinned by pin_blocks(), marking them dirty */
 static void unpin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
-	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	hpa_t hpa;
-	gpa_t gpa;
 
 	hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
 	if (hpa) {
-		gpa = scb_o->scaol & ~0xfUL;
-		if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_64BSCAO))
-			gpa |= (u64) scb_o->scaoh << 32;
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+		unpin_guest_page(vcpu->kvm, vsie_page->sca_gpa, hpa);
+		vsie_page->sca_gpa = 0;
 		scb_s->scaol = 0;
 		scb_s->scaoh = 0;
 	}
 
 	hpa = scb_s->itdba;
 	if (hpa) {
-		gpa = scb_o->itdba & ~0xffUL;
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+		unpin_guest_page(vcpu->kvm, vsie_page->itdba_gpa, hpa);
+		vsie_page->itdba_gpa = 0;
 		scb_s->itdba = 0;
 	}
 
 	hpa = scb_s->gvrd;
 	if (hpa) {
-		gpa = scb_o->gvrd & ~0x1ffUL;
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+		unpin_guest_page(vcpu->kvm, vsie_page->gvrd_gpa, hpa);
+		vsie_page->gvrd_gpa = 0;
 		scb_s->gvrd = 0;
 	}
 
 	hpa = scb_s->riccbd;
 	if (hpa) {
-		gpa = scb_o->riccbd & ~0x3fUL;
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+		unpin_guest_page(vcpu->kvm, vsie_page->riccbd_gpa, hpa);
+		vsie_page->riccbd_gpa = 0;
 		scb_s->riccbd = 0;
 	}
 
 	hpa = scb_s->sdnxo;
 	if (hpa) {
-		gpa = scb_o->sdnxo;
-		unpin_guest_page(vcpu->kvm, gpa, hpa);
+		unpin_guest_page(vcpu->kvm, vsie_page->sdnx_gpa, hpa);
+		vsie_page->sdnx_gpa = 0;
 		scb_s->sdnxo = 0;
 	}
 }
@@ -559,6 +561,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		}
 		if (rc)
 			goto unpin;
+		vsie_page->sca_gpa = gpa;
 		scb_s->scaoh = (u32)((u64)hpa >> 32);
 		scb_s->scaol = (u32)(u64)hpa;
 	}
@@ -575,6 +578,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			rc = set_validity_icpt(scb_s, 0x0080U);
 			goto unpin;
 		}
+		vsie_page->itdba_gpa = gpa;
 		scb_s->itdba = hpa;
 	}
 
@@ -593,6 +597,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			rc = set_validity_icpt(scb_s, 0x1310U);
 			goto unpin;
 		}
+		vsie_page->gvrd_gpa = gpa;
 		scb_s->gvrd = hpa;
 	}
 
@@ -609,6 +614,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			goto unpin;
 		}
 		/* Validity 0x0044 will be checked by SIE */
+		vsie_page->riccbd_gpa = gpa;
 		scb_s->riccbd = hpa;
 	}
 	if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
@@ -636,6 +642,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			rc = set_validity_icpt(scb_s, 0x10b0U);
 			goto unpin;
 		}
+		vsie_page->sdnx_gpa = gpa;
 		scb_s->sdnxo = hpa | sdnxc;
 	}
 	return 0;
-- 
2.14.3

Re: [PATCH RFC 1/2] KVM: s390 vsie: use READ_ONCE to access some SCB fields

[Christian Borntraeger]

On 01/16/2018 06:15 PM, David Hildenbrand wrote:
> Another VCPU might try to modify the SCB while we are creating the
> shadow SCB. In general this is no problem - unless the compiler decides
> to not load values once, but e.g. twice.
> 
> For us, this is only relevant when checking/working with such values.
> E.g. the prefix value, the mso, state of transactional execution and
> addresses of satellite blocks.
> 
> E.g. if we blindly forwards values (e.g. general purpose registers or
> execution controls after masking), we don't care.
> 
> Leaving unpin_blocks() untouched for now, will handle it separatly.
> 
> The worst thing right now that I can see would be a missed prefix
> un/remap (mso, prefix, tx) or using wrong guest addresses. Nothing
> critical, but let's try to avoid unpredictable behavior.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

thanks applied.


> ---
>  arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 5d6ae0326d9e..79ea444a0534 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -28,7 +28,11 @@ struct vsie_page {
>  	 * the same offset as that in struct sie_page!
>  	 */
>  	struct mcck_volatile_info mcck_info;    /* 0x0200 */
> -	/* the pinned originial scb */
> +	/*
> +	 * The pinned originial scb. Be aware that other VCPUs can modify
> +	 * it while we read from it. Values that are used for conditions or
> +	 * are reused conditionally, should be accessed via READ_ONCE.
> +	 */
>  	struct kvm_s390_sie_block *scb_o;	/* 0x0218 */
>  	/* the shadow gmap in use by the vsie_page */
>  	struct gmap *gmap;			/* 0x0220 */
> @@ -140,12 +144,13 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> -	u32 crycb_addr = scb_o->crycbd & 0x7ffffff8U;
> +	const uint32_t crycbd_o = READ_ONCE(scb_o->crycbd);
> +	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>  	unsigned long *b1, *b2;
>  	u8 ecb3_flags;
> 
>  	scb_s->crycbd = 0;
> -	if (!(scb_o->crycbd & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
> +	if (!(crycbd_o & vcpu->arch.sie_block->crycbd & CRYCB_FORMAT1))
>  		return 0;
>  	/* format-1 is supported with message-security-assist extension 3 */
>  	if (!test_kvm_facility(vcpu->kvm, 76))
> @@ -183,12 +188,15 @@ static void prepare_ibc(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> +	/* READ_ONCE does not work on bitfields - use a temporary variable */
> +	const uint32_t __new_ibc = scb_o->ibc;
> +	const uint32_t new_ibc = READ_ONCE(__new_ibc) & 0x0fffU;
>  	__u64 min_ibc = (sclp.ibc >> 16) & 0x0fffU;
> 
>  	scb_s->ibc = 0;
>  	/* ibc installed in g2 and requested for g3 */
> -	if (vcpu->kvm->arch.model.ibc && (scb_o->ibc & 0x0fffU)) {
> -		scb_s->ibc = scb_o->ibc & 0x0fffU;
> +	if (vcpu->kvm->arch.model.ibc && new_ibc) {
> +		scb_s->ibc = new_ibc;
>  		/* takte care of the minimum ibc level of the machine */
>  		if (scb_s->ibc < min_ibc)
>  			scb_s->ibc = min_ibc;
> @@ -253,6 +261,10 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
> +	/* READ_ONCE does not work on bitfields - use a temporary variable */
> +	const uint32_t __new_prefix = scb_o->prefix;
> +	const uint32_t new_prefix = READ_ONCE(__new_prefix);
> +	const bool wants_tx = READ_ONCE(scb_o->ecb) & ECB_TE;
>  	bool had_tx = scb_s->ecb & ECB_TE;
>  	unsigned long new_mso = 0;
>  	int rc;
> @@ -299,14 +311,14 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	scb_s->icpua = scb_o->icpua;
> 
>  	if (!(atomic_read(&scb_s->cpuflags) & CPUSTAT_SM))
> -		new_mso = scb_o->mso & 0xfffffffffff00000UL;
> +		new_mso = READ_ONCE(scb_o->mso) & 0xfffffffffff00000UL;
>  	/* if the hva of the prefix changes, we have to remap the prefix */
> -	if (scb_s->mso != new_mso || scb_s->prefix != scb_o->prefix)
> +	if (scb_s->mso != new_mso || scb_s->prefix != new_prefix)
>  		prefix_unmapped(vsie_page);
>  	 /* SIE will do mso/msl validity and exception checks for us */
>  	scb_s->msl = scb_o->msl & 0xfffffffffff00000UL;
>  	scb_s->mso = new_mso;
> -	scb_s->prefix = scb_o->prefix;
> +	scb_s->prefix = new_prefix;
> 
>  	/* We have to definetly flush the tlb if this scb never ran */
>  	if (scb_s->ihcpu != 0xffffU)
> @@ -318,11 +330,11 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_ESOP))
>  		scb_s->ecb |= scb_o->ecb & ECB_HOSTPROTINT;
>  	/* transactional execution */
> -	if (test_kvm_facility(vcpu->kvm, 73)) {
> +	if (test_kvm_facility(vcpu->kvm, 73) && wants_tx) {
>  		/* remap the prefix is tx is toggled on */
> -		if ((scb_o->ecb & ECB_TE) && !had_tx)
> +		if (!had_tx)
>  			prefix_unmapped(vsie_page);
> -		scb_s->ecb |= scb_o->ecb & ECB_TE;
> +		scb_s->ecb |= ECB_TE;
>  	}
>  	/* SIMD */
>  	if (test_kvm_facility(vcpu->kvm, 129)) {
> @@ -529,9 +541,9 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	gpa_t gpa;
>  	int rc = 0;
> 
> -	gpa = scb_o->scaol & ~0xfUL;
> +	gpa = READ_ONCE(scb_o->scaol) & ~0xfUL;
>  	if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_64BSCAO))
> -		gpa |= (u64) scb_o->scaoh << 32;
> +		gpa |= (u64) READ_ONCE(scb_o->scaoh) << 32;
>  	if (gpa) {
>  		if (!(gpa & ~0x1fffUL))
>  			rc = set_validity_icpt(scb_s, 0x0038U);
> @@ -551,7 +563,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_s->scaol = (u32)(u64)hpa;
>  	}
> 
> -	gpa = scb_o->itdba & ~0xffUL;
> +	gpa = READ_ONCE(scb_o->itdba) & ~0xffUL;
>  	if (gpa && (scb_s->ecb & ECB_TE)) {
>  		if (!(gpa & ~0x1fffU)) {
>  			rc = set_validity_icpt(scb_s, 0x0080U);
> @@ -566,7 +578,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_s->itdba = hpa;
>  	}
> 
> -	gpa = scb_o->gvrd & ~0x1ffUL;
> +	gpa = READ_ONCE(scb_o->gvrd) & ~0x1ffUL;
>  	if (gpa && (scb_s->eca & ECA_VX) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
>  		if (!(gpa & ~0x1fffUL)) {
>  			rc = set_validity_icpt(scb_s, 0x1310U);
> @@ -584,7 +596,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		scb_s->gvrd = hpa;
>  	}
> 
> -	gpa = scb_o->riccbd & ~0x3fUL;
> +	gpa = READ_ONCE(scb_o->riccbd) & ~0x3fUL;
>  	if (gpa && (scb_s->ecb3 & ECB3_RI)) {
>  		if (!(gpa & ~0x1fffUL)) {
>  			rc = set_validity_icpt(scb_s, 0x0043U);
> @@ -602,8 +614,8 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
>  		unsigned long sdnxc;
> 
> -		gpa = scb_o->sdnxo & ~0xfUL;
> -		sdnxc = scb_o->sdnxo & 0xfUL;
> +		gpa = READ_ONCE(scb_o->sdnxo) & ~0xfUL;
> +		sdnxc = READ_ONCE(scb_o->sdnxo) & 0xfUL;
>  		if (!gpa || !(gpa & ~0x1fffUL)) {
>  			rc = set_validity_icpt(scb_s, 0x10b0U);
>  			goto unpin;
> @@ -768,7 +780,7 @@ static void retry_vsie_icpt(struct vsie_page *vsie_page)
>  static int handle_stfle(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
> -	__u32 fac = vsie_page->scb_o->fac & 0x7ffffff8U;
> +	__u32 fac = READ_ONCE(vsie_page->scb_o->fac) & 0x7ffffff8U;
> 
>  	if (fac && test_kvm_facility(vcpu->kvm, 7)) {
>  		retry_vsie_icpt(vsie_page);
> 

Re: [PATCH RFC 2/2] KVM: s390: vsie: store guest addresses of satellite blocks in vsie_page

[Christian Borntraeger]

On 01/16/2018 06:15 PM, David Hildenbrand wrote:
> This way, the values cannot changed, even if another VCPU might try to
> mess with the nested SCB currently getting executed by another VCPU.
> 
> We now always use the same gpa for pinning and unpinning a page (for
> unpinning, it is only relevant to mark the guest page dirty for
> migration).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

thanks applied.

> ---
>  arch/s390/kvm/vsie.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 79ea444a0534..546d8b106ee0 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -38,7 +38,13 @@ struct vsie_page {
>  	struct gmap *gmap;			/* 0x0220 */
>  	/* address of the last reported fault to guest2 */
>  	unsigned long fault_addr;		/* 0x0228 */
> -	__u8 reserved[0x0700 - 0x0230];		/* 0x0230 */
> +	/* calculated guest addresses of satellite control blocks */
> +	gpa_t sca_gpa;				/* 0x0230 */
> +	gpa_t itdba_gpa;			/* 0x0238 */
> +	gpa_t gvrd_gpa;				/* 0x0240 */
> +	gpa_t riccbd_gpa;			/* 0x0248 */
> +	gpa_t sdnx_gpa;				/* 0x0250 */
> +	__u8 reserved[0x0700 - 0x0258];		/* 0x0258 */
>  	struct kvm_s390_crypto_cb crycb;	/* 0x0700 */
>  	__u8 fac[S390_ARCH_FAC_LIST_SIZE_BYTE];	/* 0x0800 */
>  };
> @@ -475,46 +481,42 @@ static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
>  /* unpin all blocks previously pinned by pin_blocks(), marking them dirty */
>  static void unpin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
> -	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>  	hpa_t hpa;
> -	gpa_t gpa;
> 
>  	hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
>  	if (hpa) {
> -		gpa = scb_o->scaol & ~0xfUL;
> -		if (test_kvm_cpu_feat(vcpu->kvm, KVM_S390_VM_CPU_FEAT_64BSCAO))
> -			gpa |= (u64) scb_o->scaoh << 32;
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
> +		unpin_guest_page(vcpu->kvm, vsie_page->sca_gpa, hpa);
> +		vsie_page->sca_gpa = 0;
>  		scb_s->scaol = 0;
>  		scb_s->scaoh = 0;
>  	}
> 
>  	hpa = scb_s->itdba;
>  	if (hpa) {
> -		gpa = scb_o->itdba & ~0xffUL;
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
> +		unpin_guest_page(vcpu->kvm, vsie_page->itdba_gpa, hpa);
> +		vsie_page->itdba_gpa = 0;
>  		scb_s->itdba = 0;
>  	}
> 
>  	hpa = scb_s->gvrd;
>  	if (hpa) {
> -		gpa = scb_o->gvrd & ~0x1ffUL;
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
> +		unpin_guest_page(vcpu->kvm, vsie_page->gvrd_gpa, hpa);
> +		vsie_page->gvrd_gpa = 0;
>  		scb_s->gvrd = 0;
>  	}
> 
>  	hpa = scb_s->riccbd;
>  	if (hpa) {
> -		gpa = scb_o->riccbd & ~0x3fUL;
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
> +		unpin_guest_page(vcpu->kvm, vsie_page->riccbd_gpa, hpa);
> +		vsie_page->riccbd_gpa = 0;
>  		scb_s->riccbd = 0;
>  	}
> 
>  	hpa = scb_s->sdnxo;
>  	if (hpa) {
> -		gpa = scb_o->sdnxo;
> -		unpin_guest_page(vcpu->kvm, gpa, hpa);
> +		unpin_guest_page(vcpu->kvm, vsie_page->sdnx_gpa, hpa);
> +		vsie_page->sdnx_gpa = 0;
>  		scb_s->sdnxo = 0;
>  	}
>  }
> @@ -559,6 +561,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		}
>  		if (rc)
>  			goto unpin;
> +		vsie_page->sca_gpa = gpa;
>  		scb_s->scaoh = (u32)((u64)hpa >> 32);
>  		scb_s->scaol = (u32)(u64)hpa;
>  	}
> @@ -575,6 +578,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  			rc = set_validity_icpt(scb_s, 0x0080U);
>  			goto unpin;
>  		}
> +		vsie_page->itdba_gpa = gpa;
>  		scb_s->itdba = hpa;
>  	}
> 
> @@ -593,6 +597,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  			rc = set_validity_icpt(scb_s, 0x1310U);
>  			goto unpin;
>  		}
> +		vsie_page->gvrd_gpa = gpa;
>  		scb_s->gvrd = hpa;
>  	}
> 
> @@ -609,6 +614,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  			goto unpin;
>  		}
>  		/* Validity 0x0044 will be checked by SIE */
> +		vsie_page->riccbd_gpa = gpa;
>  		scb_s->riccbd = hpa;
>  	}
>  	if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
> @@ -636,6 +642,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  			rc = set_validity_icpt(scb_s, 0x10b0U);
>  			goto unpin;
>  		}
> +		vsie_page->sdnx_gpa = gpa;
>  		scb_s->sdnxo = hpa | sdnxc;
>  	}
>  	return 0;
> 

Re: [PATCH RFC 1/2] KVM: s390 vsie: use READ_ONCE to access some SCB fields

[Cornelia Huck]

On Tue, 16 Jan 2018 18:15:25 +0100
David Hildenbrand <david@redhat.com> wrote:

> Another VCPU might try to modify the SCB while we are creating the
> shadow SCB. In general this is no problem - unless the compiler decides
> to not load values once, but e.g. twice.
> 
> For us, this is only relevant when checking/working with such values.
> E.g. the prefix value, the mso, state of transactional execution and
> addresses of satellite blocks.
> 
> E.g. if we blindly forwards values (e.g. general purpose registers or

s/forwards/forward/

> execution controls after masking), we don't care.
> 
> Leaving unpin_blocks() untouched for now, will handle it separatly.
> 
> The worst thing right now that I can see would be a missed prefix
> un/remap (mso, prefix, tx) or using wrong guest addresses. Nothing
> critical, but let's try to avoid unpredictable behavior.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 5d6ae0326d9e..79ea444a0534 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -28,7 +28,11 @@ struct vsie_page {
>  	 * the same offset as that in struct sie_page!
>  	 */
>  	struct mcck_volatile_info mcck_info;    /* 0x0200 */
> -	/* the pinned originial scb */
> +	/*
> +	 * The pinned originial scb. Be aware that other VCPUs can modify

s/originial/original/

(as that line is touched anyway)

> +	 * it while we read from it. Values that are used for conditions or
> +	 * are reused conditionally, should be accessed via READ_ONCE.
> +	 */
>  	struct kvm_s390_sie_block *scb_o;	/* 0x0218 */
>  	/* the shadow gmap in use by the vsie_page */
>  	struct gmap *gmap;			/* 0x0220 */

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

Re: [PATCH RFC 2/2] KVM: s390: vsie: store guest addresses of satellite blocks in vsie_page

[Cornelia Huck]

On Tue, 16 Jan 2018 18:15:26 +0100
David Hildenbrand <david@redhat.com> wrote:

> This way, the values cannot changed, even if another VCPU might try to

s/changed/be changed/ (or change?)

> mess with the nested SCB currently getting executed by another VCPU.
> 
> We now always use the same gpa for pinning and unpinning a page (for
> unpinning, it is only relevant to mark the guest page dirty for
> migration).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/vsie.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)

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

Re: [PATCH RFC 2/2] KVM: s390: vsie: store guest addresses of satellite blocks in vsie_page

[David Hildenbrand]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

On 06.03.2018 14:14, Christian Borntraeger wrote:
> 
> 
> On 03/06/2018 02:01 PM, David Hildenbrand wrote:
>> On 06.03.2018 12:53, Christian Borntraeger wrote:
>>>
>>>
>>> On 03/06/2018 12:38 PM, Christian Borntraeger wrote:
>>>> On 03/06/2018 12:32 PM, Christian Borntraeger wrote:
>>>>> David, 
>>>>>
>>>>> this seems to have broken VSIE f�r guests with >64vcpus as we only pin
>>>>> the first page of the SCA. Starting guest3 with 240 vcpu triggers
>>>>> quickly guest2 crashes. Can you have a look?
>>>>>
>>>>
>>>> Hmm, or maybe it was always broken and we never supported pinning of
>>>> ESCA properly?
>>>
>>> 4.15.0 is also broken, must be an earlier change.
>>>
>>
>> Maybe something as simple as this:
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 77d7818130db..321bfbc67d3d 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2146,6 +2146,7 @@ static void sca_add_vcpu(struct kvm_vcpu *vcpu)
>>                 /* we still need the basic sca for the ipte control */
>>                 vcpu->arch.sie_block->scaoh = (__u32)(((__u64)sca) >> 32);
>>                 vcpu->arch.sie_block->scaol = (__u32)(__u64)sca;
>> +               return;
>>         }
>>         read_lock(&vcpu->kvm->arch.sca_lock);
>>         if (vcpu->kvm->arch.use_esca) {
>>
>>
> 
> 
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> can you spin a patch with proper Fixes tags?
> 

Sure, thanks for reporting!

-- 

Thanks,

David / dhildenb