[PATCH v3] KVM: s390: Introduce storage key removal facility

[PATCH v3] KVM: s390: Introduce storage key removal facility

[Janosch Frank]

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

v3:
	* Put kss handling into own function
	* Removed some unneeded catch statements and converted others to ifs

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

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

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index e7a7c499a73f..9c699c3fcf84 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 */
@@ -531,6 +532,37 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
 	return handle_instruction(vcpu);
 }
 
+static int handle_kss(struct kvm_vcpu *vcpu)
+{
+	if (!test_kvm_facility(vcpu->kvm, 169))
+		return kvm_s390_skey_check_enable(vcpu);
+
+	/*
+	 * 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.
+	 */
+	if  (vcpu->arch.sie_block->ipa == 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.
+		 */
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	}
+
+	kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
+	return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
+}
+
 int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 {
 	int rc, per_rc = 0;
@@ -565,7 +597,7 @@ 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);
+		rc = handle_kss(vcpu);
 		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..5e3583b8b5e3 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 || rc != -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 || rc != -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 || rc != -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

Re: [PATCH v3] KVM: s390: Introduce storage key removal facility

[David Hildenbrand]

On 08.09.20 12:02, Janosch Frank 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>
> ---
> 
> v3:
> 	* Put kss handling into own function
> 	* Removed some unneeded catch statements and converted others to ifs
> 
> v2:
> 	* Removed the likely
> 	* Updated and re-shuffeled the comments which had the wrong information
> 
> ---
>  arch/s390/kvm/intercept.c | 34 +++++++++++++++++++++++++++++++++-
>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>  arch/s390/kvm/priv.c      | 26 +++++++++++++++++++++++---
>  3 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index e7a7c499a73f..9c699c3fcf84 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 */
> @@ -531,6 +532,37 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
>  	return handle_instruction(vcpu);
>  }
>  
> +static int handle_kss(struct kvm_vcpu *vcpu)
> +{
> +	if (!test_kvm_facility(vcpu->kvm, 169))
> +		return kvm_s390_skey_check_enable(vcpu);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if  (vcpu->arch.sie_block->ipa == 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.
> +		 */
> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +	}
> +
> +	kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +	return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
> +}
> +
>  int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>  {
>  	int rc, per_rc = 0;
> @@ -565,7 +597,7 @@ 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);
> +		rc = handle_kss(vcpu);
>  		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..5e3583b8b5e3 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 || rc != -EOPNOTSUPP) ? rc : 0;

If rc == -EAGAIN you used to return 0.

Now, "-EAGAIN != -EAGAIN || -EAGAIN != -EOPNOTSUPP"

evaluates to "false || true == true"

so you would return rc == -EAGAIN - is that what you really want?

(I've been on vacation for two weeks, my mind might not be fully back :D )

>  
>  	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 || rc != -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 || rc != -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);

You don't use parentheses around & here ...

>  	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 &&

... and here ...

> +	    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);
> +

... but you do here

>  	/* we only handle the Linux memory detection case:
>  	 * access key == 0
>  	 * everything else goes to userspace. */
> 


Do we have to are about vsie? If the g2 CPU does not have storage keys,
also g3 should not. I can spot KSS handling in vsie code - is that
sufficient?

-- 
Thanks,

David / dhildenb

Re: [PATCH v3] KVM: s390: Introduce storage key removal facility

[Cornelia Huck]

On Tue,  8 Sep 2020 06:02:49 -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>
> ---
> 
> v3:
> 	* Put kss handling into own function
> 	* Removed some unneeded catch statements and converted others to ifs
> 
> v2:
> 	* Removed the likely
> 	* Updated and re-shuffeled the comments which had the wrong information
> 
> ---
>  arch/s390/kvm/intercept.c | 34 +++++++++++++++++++++++++++++++++-
>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>  arch/s390/kvm/priv.c      | 26 +++++++++++++++++++++++---
>  3 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index e7a7c499a73f..9c699c3fcf84 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 */
> @@ -531,6 +532,37 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
>  	return handle_instruction(vcpu);
>  }
>  
> +static int handle_kss(struct kvm_vcpu *vcpu)
> +{
> +	if (!test_kvm_facility(vcpu->kvm, 169))
> +		return kvm_s390_skey_check_enable(vcpu);
> +
> +	/*
> +	 * Storage key removal facility emulation.
> +	 *
> +	 * KSS is the same priority as an instruction
> +	 * interception. Hence we need handling here

s/here/both here/ ?

(I think you can also format this slightly wider, now that indentation
is not so deep anymore.)

> +	 * and in the instruction emulation code.
> +	 *
> +	 * KSS is nullifying (no psw forward), SKRF
> +	 * issues suppressing SPECIAL OPS, so we need
> +	 * to forward by hand.
> +	 */
> +	if  (vcpu->arch.sie_block->ipa == 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.
> +		 */
> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +	}
> +
> +	kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
> +	return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
> +}
> +
>  int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>  {
>  	int rc, per_rc = 0;
> @@ -565,7 +597,7 @@ 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);
> +		rc = handle_kss(vcpu);
>  		break;
>  	case ICPT_MCHKREQ:
>  	case ICPT_INT_ENABLE:

(...)

> @@ -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 || rc != -EOPNOTSUPP) ? rc : 0;

As noticed by David, this probably needs to be &&, or maybe flipped to

		return (rc == -EAGAIN || rc == -EOPNOTSUPP) ? 0 : rc;

>  
>  	kvm_s390_get_regs_rre(vcpu, &reg1, &reg2);
>  

Re: [PATCH v3] KVM: s390: Introduce storage key removal facility

[Janosch Frank]

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

On 9/8/20 12:16 PM, David Hildenbrand wrote:
> On 08.09.20 12:02, Janosch Frank 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>
>> ---
>>
>> v3:
>> 	* Put kss handling into own function
>> 	* Removed some unneeded catch statements and converted others to ifs
>>
>> v2:
>> 	* Removed the likely
>> 	* Updated and re-shuffeled the comments which had the wrong information
>>
>> ---
>>  arch/s390/kvm/intercept.c | 34 +++++++++++++++++++++++++++++++++-
>>  arch/s390/kvm/kvm-s390.c  |  5 +++++
>>  arch/s390/kvm/priv.c      | 26 +++++++++++++++++++++++---
>>  3 files changed, 61 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
>> index e7a7c499a73f..9c699c3fcf84 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 */
>> @@ -531,6 +532,37 @@ static int handle_pv_notification(struct kvm_vcpu *vcpu)
>>  	return handle_instruction(vcpu);
>>  }
>>  
>> +static int handle_kss(struct kvm_vcpu *vcpu)
>> +{
>> +	if (!test_kvm_facility(vcpu->kvm, 169))
>> +		return kvm_s390_skey_check_enable(vcpu);
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	if  (vcpu->arch.sie_block->ipa == 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.
>> +		 */
>> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +	}
>> +
>> +	kvm_s390_forward_psw(vcpu, kvm_s390_get_ilen(vcpu));
>> +	return kvm_s390_inject_program_int(vcpu, PGM_SPECIAL_OPERATION);
>> +}
>> +
>>  int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>>  {
>>  	int rc, per_rc = 0;
>> @@ -565,7 +597,7 @@ 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);
>> +		rc = handle_kss(vcpu);
>>  		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..5e3583b8b5e3 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 || rc != -EOPNOTSUPP) ? rc : 0;
> 
> If rc == -EAGAIN you used to return 0.
> 
> Now, "-EAGAIN != -EAGAIN || -EAGAIN != -EOPNOTSUPP"
> 
> evaluates to "false || true == true"
> 
> so you would return rc == -EAGAIN - is that what you really want?
> 
> (I've been on vacation for two weeks, my mind might not be fully back :D )

As you can clearly tell, I'm waiting for my vacation to finally arrive.

> 
>>  
>>  	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 || rc != -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 || rc != -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);
> 
> You don't use parentheses around & here ...
> 
>>  	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 &&
> 
> ... and here ...
> 
>> +	    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);
>> +
> 
> ... but you do here

I'll add some

> 
>>  	/* we only handle the Linux memory detection case:
>>  	 * access key == 0
>>  	 * everything else goes to userspace. */
>>
> 
> 
> Do we have to are about vsie? If the g2 CPU does not have storage keys,
> also g3 should not. I can spot KSS handling in vsie code - is that
> sufficient?

You mean the two lines that take care of the cpuflags?
That's KSS passthrough and fallback to ICTLs if there is no KSS.

That's an interesting problem.
That would mean we would need to force KSS instead of doing a
passthrough for the G3 if SKRF is enabled in G2. The intercept priority
problem will make this even more awkward.

I'll need to think about this for a bit and speak to the CPU
architecture people to clear this up. This could take a bit of time
because of various vacations.


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