All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: s390: Introduce storage key removal facility
@ 2020-09-07 13:14 Janosch Frank
  2020-09-07 13:50 ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: Janosch Frank @ 2020-09-07 13:14 UTC (permalink / raw)
  To: kvm; +Cc: borntraeger, imbrenda, david, linux-s390

The storage key removal facility makes skey related instructions
result in special operation program exceptions. It is based on the
Keyless Subset Facility.

The usual suspects are iske, sske, rrbe and their respective
variants. lpsw(e), pfmf and tprot can also specify a key and essa with
an ORC of 4 will consult the change bit, hence they all result in
exceptions.

Unfortunately storage keys were so essential to the architecture, that
there is no facility bit that we could deactivate. That's why the
removal facility (bit 169) was introduced which makes it necessary,
that, if active, the skey related facilities 10, 14, 66, 145 and 149
are zero. Managing this requirement and migratability has to be done
in userspace, as KVM does not check the facilities it receives to be
able to easily implement userspace emulation.

Removing storage key support allows us to circumvent complicated
emulation code and makes huge page support tremendously easier.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
 arch/s390/kvm/kvm-s390.c  |  5 +++++
 arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
 3 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index e7a7c499a73f..99dd042d7dea 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
 	case ICPT_OPEREXC:
 	case ICPT_PARTEXEC:
 	case ICPT_IOINST:
+	case ICPT_KSS:
 		/* instruction only stored for these icptcodes */
 		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
 		/* Use the length of the EXECUTE instruction if necessary */
@@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 		rc = handle_partial_execution(vcpu);
 		break;
 	case ICPT_KSS:
-		rc = kvm_s390_skey_check_enable(vcpu);
+		if (likely(!test_kvm_facility(vcpu->kvm, 169))) {
+			rc = kvm_s390_skey_check_enable(vcpu);
+		} else {
+			/*
+			 * Storage key removal facility emulation.
+			 *
+			 * KSS is the same priority as instruction
+			 * interception. Hence we need handling here
+			 * and in the instruction emulation code.
+			 *
+			 * lpsw(e) needs to store the problematic psw
+			 * as the program old psw. Calling the
+			 * handlers directly does that without falsely
+			 * increasing stat counters.
+			 */
+			switch (vcpu->arch.sie_block->ipa) {
+			case 0xb2b2:
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_handle_b2(vcpu);
+				break;
+			case 0x8200:
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_handle_lpsw(vcpu);
+				break;
+			case 0:
+				/* Interception caused by exception new PSW key */
+				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+				break;
+			default:
+				/*
+				 * KSS is nullifying (no psw forward),
+				 * SKRF issues suppressing SPECIAL
+				 * OPS, so we need to forward by hand.
+				 */
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+			}
+		}
 		break;
 	case ICPT_MCHKREQ:
 	case ICPT_INT_ENABLE:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6b74b92c1a58..85647f19311d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2692,6 +2692,11 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	/* we emulate STHYI in kvm */
 	set_kvm_facility(kvm->arch.model.fac_mask, 74);
 	set_kvm_facility(kvm->arch.model.fac_list, 74);
+	/* we emulate the storage key removal facility only with kss */
+	if (sclp.has_kss) {
+		set_kvm_facility(kvm->arch.model.fac_mask, 169);
+		set_kvm_facility(kvm->arch.model.fac_list, 169);
+	}
 	if (MACHINE_HAS_TLB_GUEST) {
 		set_kvm_facility(kvm->arch.model.fac_mask, 147);
 		set_kvm_facility(kvm->arch.model.fac_list, 147);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index cd74989ce0b0..d1923fbec341 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -207,6 +207,13 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
 	int rc;
 
 	trace_kvm_s390_skey_related_inst(vcpu);
+
+	if (test_kvm_facility(vcpu->kvm, 169)) {
+		rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+		if (!rc)
+			return -EOPNOTSUPP;
+	}
+
 	/* Already enabled? */
 	if (vcpu->arch.skey_enabled)
 		return 0;
@@ -257,7 +264,7 @@ static int handle_iske(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
 
@@ -304,7 +311,7 @@ static int handle_rrbe(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
 
@@ -355,7 +362,7 @@ static int handle_sske(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	if (!test_kvm_facility(vcpu->kvm, 8))
 		m3 &= ~SSKE_MB;
@@ -745,6 +752,8 @@ int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
 	if (!(new_psw.mask & PSW32_MASK_BASE))
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	if (new_psw.mask & PSW32_MASK_KEY && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	gpsw->mask = (new_psw.mask & ~PSW32_MASK_BASE) << 32;
 	gpsw->mask |= new_psw.addr & PSW32_ADDR_AMODE;
 	gpsw->addr = new_psw.addr & ~PSW32_ADDR_AMODE;
@@ -771,6 +780,8 @@ static int handle_lpswe(struct kvm_vcpu *vcpu)
 	rc = read_guest(vcpu, addr, ar, &new_psw, sizeof(new_psw));
 	if (rc)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
+	if ((new_psw.mask & PSW_MASK_KEY) && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	vcpu->arch.sie_block->gpsw = new_psw;
 	if (!is_valid_psw(&vcpu->arch.sie_block->gpsw))
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
@@ -1025,6 +1036,10 @@ static int handle_pfmf(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
 		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
 
+	if (vcpu->run->s.regs.gprs[reg1] & PFMF_SK &&
+	    test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+
 	if (vcpu->run->s.regs.gprs[reg1] & PFMF_RESERVED)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
 
@@ -1203,6 +1218,8 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
 	/* Check for invalid operation request code */
 	orc = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
+	if (orc == ESSA_SET_POT_VOLATILE && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	/* ORCs 0-6 are always valid */
 	if (orc > (test_kvm_facility(vcpu->kvm, 147) ? ESSA_SET_STABLE_NODAT
 						: ESSA_SET_STABLE_IF_RESIDENT))
@@ -1451,6 +1468,9 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
 
 	kvm_s390_get_base_disp_sse(vcpu, &address1, &address2, &ar, NULL);
 
+	if ((address2 & 0xf0) && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+
 	/* we only handle the Linux memory detection case:
 	 * access key == 0
 	 * everything else goes to userspace. */
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: s390: Introduce storage key removal facility
  2020-09-07 13:14 [PATCH] KVM: s390: Introduce storage key removal facility Janosch Frank
@ 2020-09-07 13:50 ` Christian Borntraeger
  2020-09-07 14:33   ` [PATCH v2] " Janosch Frank
  0 siblings, 1 reply; 8+ messages in thread
From: Christian Borntraeger @ 2020-09-07 13:50 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: imbrenda, david, linux-s390



On 07.09.20 15:14, Janosch Frank wrote:
[...]

> @@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>  		rc = handle_partial_execution(vcpu);
>  		break;
>  	case ICPT_KSS:
> -		rc = kvm_s390_skey_check_enable(vcpu);
> +		if (likely(!test_kvm_facility(vcpu->kvm, 169))) {

please no "likely" unless really necessary. This is for both cases slow path.

> +			rc = kvm_s390_skey_check_enable(vcpu);
> +		} else {
> +			/*
> +			 * Storage key removal facility emulation.
> +			 *
> +			 * KSS is the same priority as instruction
> +			 * interception. Hence we need handling here
> +			 * and in the instruction emulation code.
> +			 *
> +			 * lpsw(e) needs to store the problematic psw
> +			 * as the program old psw. Calling the
> +			 * handlers directly does that without falsely
> +			 * increasing stat counters.
> +			 */

I do not understand this comment and the code below. As the fault is suppressing
we should NOT store the program old psw?

> +			switch (vcpu->arch.sie_block->ipa) {
> +			case 0xb2b2:
> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_handle_b2(vcpu);
> +				break;
> +			case 0x8200:
 +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_handle_lpsw(vcpu);
> +				break;
> +			case 0:
> +				/* Interception caused by exception new PSW key */
> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +				break;
> +			default:
> +				/*
> +				 * KSS is nullifying (no psw forward),
> +				 * SKRF issues suppressing SPECIAL
> +				 * OPS, so we need to forward by hand.
> +				 */
> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
> +			}
> +		}
[...]

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-07 13:50 ` Christian Borntraeger
@ 2020-09-07 14:33   ` Janosch Frank
  2020-09-07 16:30     ` Cornelia Huck
  2020-09-08  6:49     ` kernel test robot
  0 siblings, 2 replies; 8+ messages in thread
From: Janosch Frank @ 2020-09-07 14:33 UTC (permalink / raw)
  To: kvm; +Cc: borntraeger, imbrenda, david, linux-s390

The storage key removal facility makes skey related instructions
result in special operation program exceptions. It is based on the
Keyless Subset Facility.

The usual suspects are iske, sske, rrbe and their respective
variants. lpsw(e), pfmf and tprot can also specify a key and essa with
an ORC of 4 will consult the change bit, hence they all result in
exceptions.

Unfortunately storage keys were so essential to the architecture, that
there is no facility bit that we could deactivate. That's why the
removal facility (bit 169) was introduced which makes it necessary,
that, if active, the skey related facilities 10, 14, 66, 145 and 149
are zero. Managing this requirement and migratability has to be done
in userspace, as KVM does not check the facilities it receives to be
able to easily implement userspace emulation.

Removing storage key support allows us to circumvent complicated
emulation code and makes huge page support tremendously easier.

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

v2:
	* Removed the likely
	* Updated and re-shuffeled the comments which had the wrong information

---
 arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
 arch/s390/kvm/kvm-s390.c  |  5 +++++
 arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
 3 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index e7a7c499a73f..983647ea2abe 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
 	case ICPT_OPEREXC:
 	case ICPT_PARTEXEC:
 	case ICPT_IOINST:
+	case ICPT_KSS:
 		/* instruction only stored for these icptcodes */
 		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
 		/* Use the length of the EXECUTE instruction if necessary */
@@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 		rc = handle_partial_execution(vcpu);
 		break;
 	case ICPT_KSS:
-		rc = kvm_s390_skey_check_enable(vcpu);
+		if (!test_kvm_facility(vcpu->kvm, 169)) {
+			rc = kvm_s390_skey_check_enable(vcpu);
+		} else {
+			/*
+			 * Storage key removal facility emulation.
+			 *
+			 * KSS is the same priority as an instruction
+			 * interception. Hence we need handling here
+			 * and in the instruction emulation code.
+			 *
+			 * KSS is nullifying (no psw forward), SKRF
+			 * issues suppressing SPECIAL OPS, so we need
+			 * to forward by hand.
+			 */
+			switch (vcpu->arch.sie_block->ipa) {
+			case 0xb2b2:
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_handle_b2(vcpu);
+				break;
+			case 0x8200:
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_handle_lpsw(vcpu);
+				break;
+			case 0:
+				/*
+				 * Interception caused by a key in a
+				 * exception new PSW mask. The guest
+				 * PSW has already been updated to the
+				 * non-valid PSW so we only need to
+				 * inject a PGM.
+				 */
+				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+				break;
+			default:
+				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+			}
+		}
 		break;
 	case ICPT_MCHKREQ:
 	case ICPT_INT_ENABLE:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6b74b92c1a58..85647f19311d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2692,6 +2692,11 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	/* we emulate STHYI in kvm */
 	set_kvm_facility(kvm->arch.model.fac_mask, 74);
 	set_kvm_facility(kvm->arch.model.fac_list, 74);
+	/* we emulate the storage key removal facility only with kss */
+	if (sclp.has_kss) {
+		set_kvm_facility(kvm->arch.model.fac_mask, 169);
+		set_kvm_facility(kvm->arch.model.fac_list, 169);
+	}
 	if (MACHINE_HAS_TLB_GUEST) {
 		set_kvm_facility(kvm->arch.model.fac_mask, 147);
 		set_kvm_facility(kvm->arch.model.fac_list, 147);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index cd74989ce0b0..d1923fbec341 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -207,6 +207,13 @@ int kvm_s390_skey_check_enable(struct kvm_vcpu *vcpu)
 	int rc;
 
 	trace_kvm_s390_skey_related_inst(vcpu);
+
+	if (test_kvm_facility(vcpu->kvm, 169)) {
+		rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+		if (!rc)
+			return -EOPNOTSUPP;
+	}
+
 	/* Already enabled? */
 	if (vcpu->arch.skey_enabled)
 		return 0;
@@ -257,7 +264,7 @@ static int handle_iske(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
 
@@ -304,7 +311,7 @@ static int handle_rrbe(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
 
@@ -355,7 +362,7 @@ static int handle_sske(struct kvm_vcpu *vcpu)
 
 	rc = try_handle_skey(vcpu);
 	if (rc)
-		return rc != -EAGAIN ? rc : 0;
+		return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
 
 	if (!test_kvm_facility(vcpu->kvm, 8))
 		m3 &= ~SSKE_MB;
@@ -745,6 +752,8 @@ int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
 	if (!(new_psw.mask & PSW32_MASK_BASE))
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	if (new_psw.mask & PSW32_MASK_KEY && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	gpsw->mask = (new_psw.mask & ~PSW32_MASK_BASE) << 32;
 	gpsw->mask |= new_psw.addr & PSW32_ADDR_AMODE;
 	gpsw->addr = new_psw.addr & ~PSW32_ADDR_AMODE;
@@ -771,6 +780,8 @@ static int handle_lpswe(struct kvm_vcpu *vcpu)
 	rc = read_guest(vcpu, addr, ar, &new_psw, sizeof(new_psw));
 	if (rc)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
+	if ((new_psw.mask & PSW_MASK_KEY) && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	vcpu->arch.sie_block->gpsw = new_psw;
 	if (!is_valid_psw(&vcpu->arch.sie_block->gpsw))
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
@@ -1025,6 +1036,10 @@ static int handle_pfmf(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
 		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
 
+	if (vcpu->run->s.regs.gprs[reg1] & PFMF_SK &&
+	    test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+
 	if (vcpu->run->s.regs.gprs[reg1] & PFMF_RESERVED)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
 
@@ -1203,6 +1218,8 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
 	/* Check for invalid operation request code */
 	orc = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
+	if (orc == ESSA_SET_POT_VOLATILE && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
 	/* ORCs 0-6 are always valid */
 	if (orc > (test_kvm_facility(vcpu->kvm, 147) ? ESSA_SET_STABLE_NODAT
 						: ESSA_SET_STABLE_IF_RESIDENT))
@@ -1451,6 +1468,9 @@ static int handle_tprot(struct kvm_vcpu *vcpu)
 
 	kvm_s390_get_base_disp_sse(vcpu, &address1, &address2, &ar, NULL);
 
+	if ((address2 & 0xf0) && test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+
 	/* we only handle the Linux memory detection case:
 	 * access key == 0
 	 * everything else goes to userspace. */
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-07 14:33   ` [PATCH v2] " Janosch Frank
@ 2020-09-07 16:30     ` Cornelia Huck
  2020-09-08  7:52       ` Janosch Frank
  2020-09-08  6:49     ` kernel test robot
  1 sibling, 1 reply; 8+ messages in thread
From: Cornelia Huck @ 2020-09-07 16:30 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, borntraeger, imbrenda, david, linux-s390

On Mon,  7 Sep 2020 10:33:52 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> The storage key removal facility makes skey related instructions
> result in special operation program exceptions. It is based on the
> Keyless Subset Facility.
> 
> The usual suspects are iske, sske, rrbe and their respective
> variants. lpsw(e), pfmf and tprot can also specify a key and essa with
> an ORC of 4 will consult the change bit, hence they all result in
> exceptions.
> 
> Unfortunately storage keys were so essential to the architecture, that
> there is no facility bit that we could deactivate. That's why the
> removal facility (bit 169) was introduced which makes it necessary,
> that, if active, the skey related facilities 10, 14, 66, 145 and 149
> are zero. Managing this requirement and migratability has to be done
> in userspace, as KVM does not check the facilities it receives to be
> able to easily implement userspace emulation.
> 
> Removing storage key support allows us to circumvent complicated
> emulation code and makes huge page support tremendously easier.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> 
> v2:
> 	* Removed the likely
> 	* Updated and re-shuffeled the comments which had the wrong information
> 
> ---
>  arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>  arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
>  3 files changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index e7a7c499a73f..983647ea2abe 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
>  	case ICPT_OPEREXC:
>  	case ICPT_PARTEXEC:
>  	case ICPT_IOINST:
> +	case ICPT_KSS:
>  		/* instruction only stored for these icptcodes */
>  		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
>  		/* Use the length of the EXECUTE instruction if necessary */
> @@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>  		rc = handle_partial_execution(vcpu);
>  		break;
>  	case ICPT_KSS:
> -		rc = kvm_s390_skey_check_enable(vcpu);
> +		if (!test_kvm_facility(vcpu->kvm, 169)) {
> +			rc = kvm_s390_skey_check_enable(vcpu);
> +		} else {

<bikeshed>Introduce a helper function? This is getting a bit hard to
read.</bikeshed>

> +			/*
> +			 * Storage key removal facility emulation.
> +			 *
> +			 * KSS is the same priority as an instruction
> +			 * interception. Hence we need handling here
> +			 * and in the instruction emulation code.
> +			 *
> +			 * KSS is nullifying (no psw forward), SKRF
> +			 * issues suppressing SPECIAL OPS, so we need
> +			 * to forward by hand.
> +			 */
> +			switch (vcpu->arch.sie_block->ipa) {
> +			case 0xb2b2:
> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_handle_b2(vcpu);
> +				break;
> +			case 0x8200:

Can we have speaking names? I can only guess that this is an lpsw...

> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_handle_lpsw(vcpu);
> +				break;
> +			case 0:
> +				/*
> +				 * Interception caused by a key in a
> +				 * exception new PSW mask. The guest
> +				 * PSW has already been updated to the
> +				 * non-valid PSW so we only need to
> +				 * inject a PGM.
> +				 */
> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +				break;
> +			default:
> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
> +			}
> +		}
>  		break;
>  	case ICPT_MCHKREQ:
>  	case ICPT_INT_ENABLE:

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-07 14:33   ` [PATCH v2] " Janosch Frank
  2020-09-07 16:30     ` Cornelia Huck
@ 2020-09-08  6:49     ` kernel test robot
  1 sibling, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-09-08  6:49 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 11848 bytes --]

Hi Janosch,

I love your patch! Perhaps something to improve:

[auto build test WARNING on vhost/linux-next]
[also build test WARNING on kvm/linux-next v5.9-rc4 next-20200903]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Janosch-Frank/KVM-s390-Introduce-storage-key-removal-facility/20200908-020840
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: s390-randconfig-r032-20200908 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5f5a0bb0872a9673bad08b38bc0b14c42263902a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

                                                             ^
   include/uapi/linux/swab.h:118:32: note: expanded from macro '__swab32'
           (__builtin_constant_p((__u32)(x)) ?     \
                                         ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                     ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from arch/s390/kvm/priv.c:27:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/kvm/priv.c:268:25: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^  ~~~~~~~~~~~
   arch/s390/kvm/priv.c:268:25: note: use '|' for a bitwise operation
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^~
                                         |
   arch/s390/kvm/priv.c:315:25: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^  ~~~~~~~~~~~
   arch/s390/kvm/priv.c:315:25: note: use '|' for a bitwise operation
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^~
                                         |
   arch/s390/kvm/priv.c:366:25: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^  ~~~~~~~~~~~
   arch/s390/kvm/priv.c:366:25: note: use '|' for a bitwise operation
                   return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
                                         ^~
                                         |
   23 warnings generated.

# https://github.com/0day-ci/linux/commit/37ada515878572b10185f577f4bbe9f5621e4227
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Janosch-Frank/KVM-s390-Introduce-storage-key-removal-facility/20200908-020840
git checkout 37ada515878572b10185f577f4bbe9f5621e4227
vim +268 arch/s390/kvm/priv.c

   252	
   253	static int handle_iske(struct kvm_vcpu *vcpu)
   254	{
   255		unsigned long gaddr, vmaddr;
   256		unsigned char key;
   257		int reg1, reg2;
   258		bool unlocked;
   259		int rc;
   260	
   261		vcpu->stat.instruction_iske++;
   262	
   263		if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
   264			return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
   265	
   266		rc = try_handle_skey(vcpu);
   267		if (rc)
 > 268			return rc != (-EAGAIN || -EOPNOTSUPP) ? rc : 0;
   269	
   270		kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
   271	
   272		gaddr = vcpu->run->s.regs.gprs[reg2] & PAGE_MASK;
   273		gaddr = kvm_s390_logical_to_effective(vcpu, gaddr);
   274		gaddr = kvm_s390_real_to_abs(vcpu, gaddr);
   275		vmaddr = gfn_to_hva(vcpu->kvm, gpa_to_gfn(gaddr));
   276		if (kvm_is_error_hva(vmaddr))
   277			return kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
   278	retry:
   279		unlocked = false;
   280		mmap_read_lock(current->mm);
   281		rc = get_guest_storage_key(current->mm, vmaddr, &key);
   282	
   283		if (rc) {
   284			rc = fixup_user_fault(current, current->mm, vmaddr,
   285					      FAULT_FLAG_WRITE, &unlocked);
   286			if (!rc) {
   287				mmap_read_unlock(current->mm);
   288				goto retry;
   289			}
   290		}
   291		mmap_read_unlock(current->mm);
   292		if (rc == -EFAULT)
   293			return kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
   294		if (rc < 0)
   295			return rc;
   296		vcpu->run->s.regs.gprs[reg1] &= ~0xff;
   297		vcpu->run->s.regs.gprs[reg1] |= key;
   298		return 0;
   299	}
   300	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 23850 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-07 16:30     ` Cornelia Huck
@ 2020-09-08  7:52       ` Janosch Frank
  2020-09-08  8:36         ` Cornelia Huck
  0 siblings, 1 reply; 8+ messages in thread
From: Janosch Frank @ 2020-09-08  7:52 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, borntraeger, imbrenda, david, linux-s390


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

On 9/7/20 6:30 PM, Cornelia Huck wrote:
> On Mon,  7 Sep 2020 10:33:52 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The storage key removal facility makes skey related instructions
>> result in special operation program exceptions. It is based on the
>> Keyless Subset Facility.
>>
>> The usual suspects are iske, sske, rrbe and their respective
>> variants. lpsw(e), pfmf and tprot can also specify a key and essa with
>> an ORC of 4 will consult the change bit, hence they all result in
>> exceptions.
>>
>> Unfortunately storage keys were so essential to the architecture, that
>> there is no facility bit that we could deactivate. That's why the
>> removal facility (bit 169) was introduced which makes it necessary,
>> that, if active, the skey related facilities 10, 14, 66, 145 and 149
>> are zero. Managing this requirement and migratability has to be done
>> in userspace, as KVM does not check the facilities it receives to be
>> able to easily implement userspace emulation.
>>
>> Removing storage key support allows us to circumvent complicated
>> emulation code and makes huge page support tremendously easier.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>
>> v2:
>> 	* Removed the likely
>> 	* Updated and re-shuffeled the comments which had the wrong information
>>
>> ---
>>  arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
>>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>>  arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
>>  3 files changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
>> index e7a7c499a73f..983647ea2abe 100644
>> --- a/arch/s390/kvm/intercept.c
>> +++ b/arch/s390/kvm/intercept.c
>> @@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
>>  	case ICPT_OPEREXC:
>>  	case ICPT_PARTEXEC:
>>  	case ICPT_IOINST:
>> +	case ICPT_KSS:
>>  		/* instruction only stored for these icptcodes */
>>  		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
>>  		/* Use the length of the EXECUTE instruction if necessary */
>> @@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>>  		rc = handle_partial_execution(vcpu);
>>  		break;
>>  	case ICPT_KSS:
>> -		rc = kvm_s390_skey_check_enable(vcpu);
>> +		if (!test_kvm_facility(vcpu->kvm, 169)) {
>> +			rc = kvm_s390_skey_check_enable(vcpu);
>> +		} else {
> 
> <bikeshed>Introduce a helper function? This is getting a bit hard to
> read.</bikeshed>
> 
>> +			/*
>> +			 * Storage key removal facility emulation.
>> +			 *
>> +			 * KSS is the same priority as an instruction
>> +			 * interception. Hence we need handling here
>> +			 * and in the instruction emulation code.
>> +			 *
>> +			 * KSS is nullifying (no psw forward), SKRF
>> +			 * issues suppressing SPECIAL OPS, so we need
>> +			 * to forward by hand.
>> +			 */
>> +			switch (vcpu->arch.sie_block->ipa) {
>> +			case 0xb2b2:
>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>> +				rc = kvm_s390_handle_b2(vcpu);
>> +				break;
>> +			case 0x8200:
> 
> Can we have speaking names? I can only guess that this is an lpsw...

You can only guess from the kvm_s390_handle_lpsw() call below? ;-)

I'd be happy to put this into an own function and add some comments to
the cases where we lack them. However, I don't really want to define
constants for speaking names.

> 
>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>> +				rc = kvm_s390_handle_lpsw(vcpu);
>> +				break;
>> +			case 0:
>> +				/*
>> +				 * Interception caused by a key in a
>> +				 * exception new PSW mask. The guest
>> +				 * PSW has already been updated to the
>> +				 * non-valid PSW so we only need to
>> +				 * inject a PGM.
>> +				 */
>> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +				break;
>> +			default:
>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
>> +			}
>> +		}
>>  		break;
>>  	case ICPT_MCHKREQ:
>>  	case ICPT_INT_ENABLE:
> 



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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-08  7:52       ` Janosch Frank
@ 2020-09-08  8:36         ` Cornelia Huck
  2020-09-08  9:18           ` Christian Borntraeger
  0 siblings, 1 reply; 8+ messages in thread
From: Cornelia Huck @ 2020-09-08  8:36 UTC (permalink / raw)
  To: Janosch Frank; +Cc: kvm, borntraeger, imbrenda, david, linux-s390

[-- Attachment #1: Type: text/plain, Size: 4606 bytes --]

On Tue, 8 Sep 2020 09:52:48 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 9/7/20 6:30 PM, Cornelia Huck wrote:
> > On Mon,  7 Sep 2020 10:33:52 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> The storage key removal facility makes skey related instructions
> >> result in special operation program exceptions. It is based on the
> >> Keyless Subset Facility.
> >>
> >> The usual suspects are iske, sske, rrbe and their respective
> >> variants. lpsw(e), pfmf and tprot can also specify a key and essa with
> >> an ORC of 4 will consult the change bit, hence they all result in
> >> exceptions.
> >>
> >> Unfortunately storage keys were so essential to the architecture, that
> >> there is no facility bit that we could deactivate. That's why the
> >> removal facility (bit 169) was introduced which makes it necessary,
> >> that, if active, the skey related facilities 10, 14, 66, 145 and 149
> >> are zero. Managing this requirement and migratability has to be done
> >> in userspace, as KVM does not check the facilities it receives to be
> >> able to easily implement userspace emulation.
> >>
> >> Removing storage key support allows us to circumvent complicated
> >> emulation code and makes huge page support tremendously easier.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>
> >> v2:
> >> 	* Removed the likely
> >> 	* Updated and re-shuffeled the comments which had the wrong information
> >>
> >> ---
> >>  arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
> >>  arch/s390/kvm/kvm-s390.c  |  5 +++++
> >>  arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
> >>  3 files changed, 67 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> >> index e7a7c499a73f..983647ea2abe 100644
> >> --- a/arch/s390/kvm/intercept.c
> >> +++ b/arch/s390/kvm/intercept.c
> >> @@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
> >>  	case ICPT_OPEREXC:
> >>  	case ICPT_PARTEXEC:
> >>  	case ICPT_IOINST:
> >> +	case ICPT_KSS:
> >>  		/* instruction only stored for these icptcodes */
> >>  		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
> >>  		/* Use the length of the EXECUTE instruction if necessary */
> >> @@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
> >>  		rc = handle_partial_execution(vcpu);
> >>  		break;
> >>  	case ICPT_KSS:
> >> -		rc = kvm_s390_skey_check_enable(vcpu);
> >> +		if (!test_kvm_facility(vcpu->kvm, 169)) {
> >> +			rc = kvm_s390_skey_check_enable(vcpu);
> >> +		} else {  
> > 
> > <bikeshed>Introduce a helper function? This is getting a bit hard to
> > read.</bikeshed>
> >   
> >> +			/*
> >> +			 * Storage key removal facility emulation.
> >> +			 *
> >> +			 * KSS is the same priority as an instruction
> >> +			 * interception. Hence we need handling here
> >> +			 * and in the instruction emulation code.
> >> +			 *
> >> +			 * KSS is nullifying (no psw forward), SKRF
> >> +			 * issues suppressing SPECIAL OPS, so we need
> >> +			 * to forward by hand.
> >> +			 */
> >> +			switch (vcpu->arch.sie_block->ipa) {
> >> +			case 0xb2b2:
> >> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> >> +				rc = kvm_s390_handle_b2(vcpu);
> >> +				break;
> >> +			case 0x8200:  
> > 
> > Can we have speaking names? I can only guess that this is an lpsw...  
> 
> You can only guess from the kvm_s390_handle_lpsw() call below? ;-)
> 
> I'd be happy to put this into an own function and add some comments to
> the cases where we lack them. However, I don't really want to define
> constants for speaking names.

Well, I can guess the lpsw here :) but not the b2b2 above. Maybe add a
comment like /* handle lpsw/lpswe */?

> 
> >   
> >> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> >> +				rc = kvm_s390_handle_lpsw(vcpu);
> >> +				break;
> >> +			case 0:
> >> +				/*
> >> +				 * Interception caused by a key in a
> >> +				 * exception new PSW mask. The guest
> >> +				 * PSW has already been updated to the
> >> +				 * non-valid PSW so we only need to
> >> +				 * inject a PGM.
> >> +				 */
> >> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> >> +				break;
> >> +			default:
> >> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> >> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
> >> +			}
> >> +		}
> >>  		break;
> >>  	case ICPT_MCHKREQ:
> >>  	case ICPT_INT_ENABLE:  
> >   
> 
> 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: s390: Introduce storage key removal facility
  2020-09-08  8:36         ` Cornelia Huck
@ 2020-09-08  9:18           ` Christian Borntraeger
  0 siblings, 0 replies; 8+ messages in thread
From: Christian Borntraeger @ 2020-09-08  9:18 UTC (permalink / raw)
  To: Cornelia Huck, Janosch Frank; +Cc: kvm, imbrenda, david, linux-s390



On 08.09.20 10:36, Cornelia Huck wrote:
> On Tue, 8 Sep 2020 09:52:48 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 9/7/20 6:30 PM, Cornelia Huck wrote:
>>> On Mon,  7 Sep 2020 10:33:52 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>   
>>>> The storage key removal facility makes skey related instructions
>>>> result in special operation program exceptions. It is based on the
>>>> Keyless Subset Facility.
>>>>
>>>> The usual suspects are iske, sske, rrbe and their respective
>>>> variants. lpsw(e), pfmf and tprot can also specify a key and essa with
>>>> an ORC of 4 will consult the change bit, hence they all result in
>>>> exceptions.
>>>>
>>>> Unfortunately storage keys were so essential to the architecture, that
>>>> there is no facility bit that we could deactivate. That's why the
>>>> removal facility (bit 169) was introduced which makes it necessary,
>>>> that, if active, the skey related facilities 10, 14, 66, 145 and 149
>>>> are zero. Managing this requirement and migratability has to be done
>>>> in userspace, as KVM does not check the facilities it receives to be
>>>> able to easily implement userspace emulation.
>>>>
>>>> Removing storage key support allows us to circumvent complicated
>>>> emulation code and makes huge page support tremendously easier.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>
>>>> v2:
>>>> 	* Removed the likely
>>>> 	* Updated and re-shuffeled the comments which had the wrong information
>>>>
>>>> ---
>>>>  arch/s390/kvm/intercept.c | 40 ++++++++++++++++++++++++++++++++++++++-
>>>>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>>>>  arch/s390/kvm/priv.c      | 26 ++++++++++++++++++++++---
>>>>  3 files changed, 67 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
>>>> index e7a7c499a73f..983647ea2abe 100644
>>>> --- a/arch/s390/kvm/intercept.c
>>>> +++ b/arch/s390/kvm/intercept.c
>>>> @@ -33,6 +33,7 @@ u8 kvm_s390_get_ilen(struct kvm_vcpu *vcpu)
>>>>  	case ICPT_OPEREXC:
>>>>  	case ICPT_PARTEXEC:
>>>>  	case ICPT_IOINST:
>>>> +	case ICPT_KSS:
>>>>  		/* instruction only stored for these icptcodes */
>>>>  		ilen = insn_length(vcpu->arch.sie_block->ipa >> 8);
>>>>  		/* Use the length of the EXECUTE instruction if necessary */
>>>> @@ -565,7 +566,44 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>>>>  		rc = handle_partial_execution(vcpu);
>>>>  		break;
>>>>  	case ICPT_KSS:
>>>> -		rc = kvm_s390_skey_check_enable(vcpu);
>>>> +		if (!test_kvm_facility(vcpu->kvm, 169)) {
>>>> +			rc = kvm_s390_skey_check_enable(vcpu);
>>>> +		} else {  
>>>
>>> <bikeshed>Introduce a helper function? This is getting a bit hard to
>>> read.</bikeshed>
>>>   
>>>> +			/*
>>>> +			 * Storage key removal facility emulation.
>>>> +			 *
>>>> +			 * KSS is the same priority as an instruction
>>>> +			 * interception. Hence we need handling here
>>>> +			 * and in the instruction emulation code.
>>>> +			 *
>>>> +			 * KSS is nullifying (no psw forward), SKRF
>>>> +			 * issues suppressing SPECIAL OPS, so we need
>>>> +			 * to forward by hand.
>>>> +			 */
>>>> +			switch (vcpu->arch.sie_block->ipa) {
>>>> +			case 0xb2b2:
>>>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>>>> +				rc = kvm_s390_handle_b2(vcpu);
>>>> +				break;
>>>> +			case 0x8200:  
>>>
>>> Can we have speaking names? I can only guess that this is an lpsw...  
>>
>> You can only guess from the kvm_s390_handle_lpsw() call below? ;-)
>>
>> I'd be happy to put this into an own function and add some comments to
>> the cases where we lack them. However, I don't really want to define
>> constants for speaking names.
> 
> Well, I can guess the lpsw here :) but not the b2b2 above. Maybe add a
> comment like /* handle lpsw/lpswe */?

I think we can remove these 2 case statements and rely on the default case anyway.
> 
>>
>>>   
>>>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>>>> +				rc = kvm_s390_handle_lpsw(vcpu);
>>>> +				break;
>>>> +			case 0:
>>>> +				/*
>>>> +				 * Interception caused by a key in a
>>>> +				 * exception new PSW mask. The guest
>>>> +				 * PSW has already been updated to the
>>>> +				 * non-valid PSW so we only need to
>>>> +				 * inject a PGM.
>>>> +				 */
>>>> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>>>> +				break;
>>>> +			default:
>>>> +				kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>>>> +				rc = kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
>>>> +			}
>>>> +		}
>>>>  		break;
>>>>  	case ICPT_MCHKREQ:
>>>>  	case ICPT_INT_ENABLE:  
>>>   
>>
>>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-08  9:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-07 13:14 [PATCH] KVM: s390: Introduce storage key removal facility Janosch Frank
2020-09-07 13:50 ` Christian Borntraeger
2020-09-07 14:33   ` [PATCH v2] " Janosch Frank
2020-09-07 16:30     ` Cornelia Huck
2020-09-08  7:52       ` Janosch Frank
2020-09-08  8:36         ` Cornelia Huck
2020-09-08  9:18           ` Christian Borntraeger
2020-09-08  6:49     ` kernel test robot

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.