If a memop fails due to key checked protection, after already having
written to the guest, don't indicate suppression to the guest, as that
would imply that memory wasn't modified.
This could be considered a fix to the code introducing storage key
support, however this is a bug in KVM only if we emulate an
instructions writing to an operand spanning multiple pages, which I
don't believe we do.
v2 -> v3
* tweak commit message
* explicitly reset the protection code to 0 on termination
* use variable to pass termination arg
* add documentation
* fix magic constant in selftest
Given the changes I did not pick up the r-b's.
v1 -> v2
* Reword commit message of patch 1
Janis Schoetterl-Glausch (2):
KVM: s390: Don't indicate suppression on dirtying, failing memop
KVM: s390: selftest: Test suppression indication on key prot exception
Documentation/virt/kvm/api.rst | 6 +++
arch/s390/kvm/gaccess.c | 22 +++++++++--
tools/testing/selftests/kvm/s390x/memop.c | 46 ++++++++++++++++++++++-
3 files changed, 69 insertions(+), 5 deletions(-)
Range-diff against v2:
1: b5725a836f1a ! 1: e1dae6522b22 KVM: s390: Don't indicate suppression on dirtying, failing memop
@@ Commit message
Instruction execution can end in different ways, one of which is
suppression, which requires that the instruction execute like a no-op.
A writing memop that spans multiple pages and fails due to key
- protection can modified guest memory, as a result, the likely
- correct ending is termination. Therefore do not indicate a
+ protection may have modified guest memory, as a result, the likely
+ correct ending is termination. Therefore, do not indicate a
suppressing instruction ending in this case.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
+ ## Documentation/virt/kvm/api.rst ##
+@@ Documentation/virt/kvm/api.rst: in case of KVM_S390_MEMOP_F_CHECK_ONLY), the ioctl returns a positive
+ error number indicating the type of exception. This exception is also
+ raised directly at the corresponding VCPU if the flag
+ KVM_S390_MEMOP_F_INJECT_EXCEPTION is set.
++On protection exceptions, unless specified otherwise, the injected
++translation-exception identifier (TEID) indicates suppression.
+
+ If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key
+ protection is also in effect and may cause exceptions if accesses are
+ prohibited given the access key designated by "key"; the valid range is 0..15.
+ KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION
+ is > 0.
++Since the accessed memory may span multiple pages and those pages might have
++different storage keys, it is possible that a protection exception occurs
++after memory has been modified. In this case, if the exception is injected,
++the TEID does not indicate suppression.
+
+ Absolute read/write:
+ ^^^^^^^^^^^^^^^^^^^^
+
## arch/s390/kvm/gaccess.c ##
@@ arch/s390/kvm/gaccess.c: enum prot_type {
PROT_TYPE_IEP = 4,
@@ arch/s390/kvm/gaccess.c: enum prot_type {
-static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
- u8 ar, enum gacc_mode mode, enum prot_type prot)
+static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
-+ enum gacc_mode mode, enum prot_type prot, bool suppress)
++ enum gacc_mode mode, enum prot_type prot, bool terminate)
{
struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm;
struct trans_exc_code_bits *tec;
@@ arch/s390/kvm/gaccess.c: static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
-
- switch (code) {
- case PGM_PROTECTION:
-- switch (prot) {
-- case PROT_TYPE_IEP:
-- tec->b61 = 1;
-- fallthrough;
-- case PROT_TYPE_LA:
-- tec->b56 = 1;
-- break;
-- case PROT_TYPE_KEYC:
-- tec->b60 = 1;
-- break;
-- case PROT_TYPE_ALC:
-- tec->b60 = 1;
-- fallthrough;
-- case PROT_TYPE_DAT:
-- tec->b61 = 1;
-- break;
-+ if (suppress) {
-+ switch (prot) {
-+ case PROT_TYPE_IEP:
-+ tec->b61 = 1;
-+ fallthrough;
-+ case PROT_TYPE_LA:
-+ tec->b56 = 1;
-+ break;
-+ case PROT_TYPE_KEYC:
-+ tec->b60 = 1;
-+ break;
-+ case PROT_TYPE_ALC:
-+ tec->b60 = 1;
-+ fallthrough;
-+ case PROT_TYPE_DAT:
-+ tec->b61 = 1;
-+ break;
-+ }
+ tec->b61 = 1;
+ break;
}
++ if (terminate) {
++ tec->b56 = 0;
++ tec->b60 = 0;
++ tec->b61 = 0;
++ }
fallthrough;
case PGM_ASCE_TYPE:
+ case PGM_PAGE_TRANSLATION:
@@ arch/s390/kvm/gaccess.c: static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
return code;
}
@@ arch/s390/kvm/gaccess.c: static int trans_exc(struct kvm_vcpu *vcpu, int code, u
+static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
+ enum gacc_mode mode, enum prot_type prot)
+{
-+ return trans_exc_ending(vcpu, code, gva, ar, mode, prot, true);
++ return trans_exc_ending(vcpu, code, gva, ar, mode, prot, false);
+}
+
static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce,
unsigned long ga, u8 ar, enum gacc_mode mode)
{
@@ arch/s390/kvm/gaccess.c: int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
+ data += fragment_len;
ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len);
}
- if (rc > 0)
+- if (rc > 0)
- rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
-+ rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot,
-+ (mode != GACC_STORE) || (idx == 0));
++ if (rc > 0) {
++ bool terminate = (mode == GACC_STORE) && (idx > 0);
++
++ rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, terminate);
++ }
out_unlock:
if (need_ipte_lock)
ipte_unlock(vcpu);
2: 434d96c63cb5 ! 2: d3a152fe6aec KVM: s390: selftest: Test suppression indication on key prot exception
@@ Commit message
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
## tools/testing/selftests/kvm/s390x/memop.c ##
+@@
+ #include <string.h>
+ #include <sys/ioctl.h>
+
++#include <linux/bits.h>
++
+ #include "test_util.h"
+ #include "kvm_util.h"
+
@@ tools/testing/selftests/kvm/s390x/memop.c: static int err_memop_ioctl(struct test_vcpu vcpu, struct kvm_s390_mem_op *ksmo)
#define SIDA_OFFSET(o) ._sida_offset = 1, .sida_offset = (o)
#define AR(a) ._ar = 1, .ar = (a)
@@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key(void)
+ struct test_default t = test_default_init(guest_error_key);
+ uint64_t prefix;
+ uint64_t teid;
++ uint64_t teid_mask = BIT(63 - 56) | BIT(63 - 60) | BIT(63 - 61);
+ uint64_t psw[2];
+
+ HOST_SYNC(t.vcpu, STAGE_INITED);
@@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key(void)
+ HOST_SYNC(t.vcpu, STAGE_IDLED);
+ MOP(t.vm, ABSOLUTE, READ, &teid, sizeof(teid), GADDR(prefix + 168));
+ /* Bits 56, 60, 61 form a code, 0 being the only one allowing for termination */
-+ ASSERT_EQ(teid & 0x4c, 0);
++ ASSERT_EQ(teid & teid_mask, 0);
+
+ kvm_vm_free(t.kvm_vm);
+}
base-commit: c5eb0a61238dd6faf37f58c9ce61c9980aaffd7a
--
2.32.0
Check that suppression is not indicated on injection of a key checked
protection exception caused by a memop after it already modified guest
memory, as that violates the definition of suppression.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
---
tools/testing/selftests/kvm/s390x/memop.c | 46 ++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index b04c2c1b3c30..49f26f544127 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -10,6 +10,8 @@
#include <string.h>
#include <sys/ioctl.h>
+#include <linux/bits.h>
+
#include "test_util.h"
#include "kvm_util.h"
@@ -194,6 +196,7 @@ static int err_memop_ioctl(struct test_vcpu vcpu, struct kvm_s390_mem_op *ksmo)
#define SIDA_OFFSET(o) ._sida_offset = 1, .sida_offset = (o)
#define AR(a) ._ar = 1, .ar = (a)
#define KEY(a) .f_key = 1, .key = (a)
+#define INJECT .f_inject = 1
#define CHECK_N_DO(f, ...) ({ f(__VA_ARGS__, CHECK_ONLY); f(__VA_ARGS__); })
@@ -430,9 +433,18 @@ static void test_copy_key_fetch_prot(void)
TEST_ASSERT(rv == 4, "Should result in protection exception"); \
})
+static void guest_error_key(void)
+{
+ GUEST_SYNC(STAGE_INITED);
+ set_storage_key_range(mem1, PAGE_SIZE, 0x18);
+ set_storage_key_range(mem1 + PAGE_SIZE, sizeof(mem1) - PAGE_SIZE, 0x98);
+ GUEST_SYNC(STAGE_SKEYS_SET);
+ GUEST_SYNC(STAGE_IDLED);
+}
+
static void test_errors_key(void)
{
- struct test_default t = test_default_init(guest_copy_key_fetch_prot);
+ struct test_default t = test_default_init(guest_error_key);
HOST_SYNC(t.vcpu, STAGE_INITED);
HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
@@ -446,6 +458,37 @@ static void test_errors_key(void)
kvm_vm_free(t.kvm_vm);
}
+static void test_termination(void)
+{
+ struct test_default t = test_default_init(guest_error_key);
+ uint64_t prefix;
+ uint64_t teid;
+ uint64_t teid_mask = BIT(63 - 56) | BIT(63 - 60) | BIT(63 - 61);
+ uint64_t psw[2];
+
+ HOST_SYNC(t.vcpu, STAGE_INITED);
+ HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
+
+ /* vcpu, mismatching keys after first page */
+ ERR_PROT_MOP(t.vcpu, LOGICAL, WRITE, mem1, t.size, GADDR_V(mem1), KEY(1), INJECT);
+ /*
+ * The memop injected a program exception and the test needs to check the
+ * Translation-Exception Identification (TEID). It is necessary to run
+ * the guest in order to be able to read the TEID from guest memory.
+ * Set the guest program new PSW, so the guest state is not clobbered.
+ */
+ prefix = t.run->s.regs.prefix;
+ psw[0] = t.run->psw_mask;
+ psw[1] = t.run->psw_addr;
+ MOP(t.vm, ABSOLUTE, WRITE, psw, sizeof(psw), GADDR(prefix + 464));
+ HOST_SYNC(t.vcpu, STAGE_IDLED);
+ MOP(t.vm, ABSOLUTE, READ, &teid, sizeof(teid), GADDR(prefix + 168));
+ /* Bits 56, 60, 61 form a code, 0 being the only one allowing for termination */
+ ASSERT_EQ(teid & teid_mask, 0);
+
+ kvm_vm_free(t.kvm_vm);
+}
+
static void test_errors_key_storage_prot_override(void)
{
struct test_default t = test_default_init(guest_copy_key_fetch_prot);
@@ -668,6 +711,7 @@ int main(int argc, char *argv[])
test_copy_key_fetch_prot();
test_copy_key_fetch_prot_override();
test_errors_key();
+ test_termination();
test_errors_key_storage_prot_override();
test_errors_key_fetch_prot_override_not_enabled();
test_errors_key_fetch_prot_override_enabled();
--
2.32.0
If user space uses a memop to emulate an instruction and that
memop fails, the execution of the instruction ends.
Instruction execution can end in different ways, one of which is
suppression, which requires that the instruction execute like a no-op.
A writing memop that spans multiple pages and fails due to key
protection may have modified guest memory, as a result, the likely
correct ending is termination. Therefore, do not indicate a
suppressing instruction ending in this case.
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
---
Documentation/virt/kvm/api.rst | 6 ++++++
arch/s390/kvm/gaccess.c | 22 ++++++++++++++++++----
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 4a900cdbc62e..b6aba4f50db7 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -3754,12 +3754,18 @@ in case of KVM_S390_MEMOP_F_CHECK_ONLY), the ioctl returns a positive
error number indicating the type of exception. This exception is also
raised directly at the corresponding VCPU if the flag
KVM_S390_MEMOP_F_INJECT_EXCEPTION is set.
+On protection exceptions, unless specified otherwise, the injected
+translation-exception identifier (TEID) indicates suppression.
If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key
protection is also in effect and may cause exceptions if accesses are
prohibited given the access key designated by "key"; the valid range is 0..15.
KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION
is > 0.
+Since the accessed memory may span multiple pages and those pages might have
+different storage keys, it is possible that a protection exception occurs
+after memory has been modified. In this case, if the exception is injected,
+the TEID does not indicate suppression.
Absolute read/write:
^^^^^^^^^^^^^^^^^^^^
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index d53a183c2005..227ed0009354 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -491,8 +491,8 @@ enum prot_type {
PROT_TYPE_IEP = 4,
};
-static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
- u8 ar, enum gacc_mode mode, enum prot_type prot)
+static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
+ enum gacc_mode mode, enum prot_type prot, bool terminate)
{
struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm;
struct trans_exc_code_bits *tec;
@@ -520,6 +520,11 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
tec->b61 = 1;
break;
}
+ if (terminate) {
+ tec->b56 = 0;
+ tec->b60 = 0;
+ tec->b61 = 0;
+ }
fallthrough;
case PGM_ASCE_TYPE:
case PGM_PAGE_TRANSLATION:
@@ -552,6 +557,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
return code;
}
+static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
+ enum gacc_mode mode, enum prot_type prot)
+{
+ return trans_exc_ending(vcpu, code, gva, ar, mode, prot, false);
+}
+
static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce,
unsigned long ga, u8 ar, enum gacc_mode mode)
{
@@ -1109,8 +1120,11 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
data += fragment_len;
ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len);
}
- if (rc > 0)
- rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
+ if (rc > 0) {
+ bool terminate = (mode == GACC_STORE) && (idx > 0);
+
+ rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, terminate);
+ }
out_unlock:
if (need_ipte_lock)
ipte_unlock(vcpu);
--
2.32.0
On Thu, 12 May 2022 15:10:17 +0200
Janis Schoetterl-Glausch <[email protected]> wrote:
> If user space uses a memop to emulate an instruction and that
> memop fails, the execution of the instruction ends.
> Instruction execution can end in different ways, one of which is
> suppression, which requires that the instruction execute like a no-op.
> A writing memop that spans multiple pages and fails due to key
> protection may have modified guest memory, as a result, the likely
> correct ending is termination. Therefore, do not indicate a
> suppressing instruction ending in this case.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 6 ++++++
> arch/s390/kvm/gaccess.c | 22 ++++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 4a900cdbc62e..b6aba4f50db7 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -3754,12 +3754,18 @@ in case of KVM_S390_MEMOP_F_CHECK_ONLY), the ioctl returns a positive
> error number indicating the type of exception. This exception is also
> raised directly at the corresponding VCPU if the flag
> KVM_S390_MEMOP_F_INJECT_EXCEPTION is set.
> +On protection exceptions, unless specified otherwise, the injected
> +translation-exception identifier (TEID) indicates suppression.
>
> If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key
> protection is also in effect and may cause exceptions if accesses are
> prohibited given the access key designated by "key"; the valid range is 0..15.
> KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION
> is > 0.
> +Since the accessed memory may span multiple pages and those pages might have
> +different storage keys, it is possible that a protection exception occurs
> +after memory has been modified. In this case, if the exception is injected,
> +the TEID does not indicate suppression.
>
> Absolute read/write:
> ^^^^^^^^^^^^^^^^^^^^
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index d53a183c2005..227ed0009354 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -491,8 +491,8 @@ enum prot_type {
> PROT_TYPE_IEP = 4,
> };
>
> -static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> - u8 ar, enum gacc_mode mode, enum prot_type prot)
> +static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
> + enum gacc_mode mode, enum prot_type prot, bool terminate)
> {
> struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm;
> struct trans_exc_code_bits *tec;
> @@ -520,6 +520,11 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> tec->b61 = 1;
> break;
> }
> + if (terminate) {
> + tec->b56 = 0;
> + tec->b60 = 0;
> + tec->b61 = 0;
> + }
> fallthrough;
> case PGM_ASCE_TYPE:
> case PGM_PAGE_TRANSLATION:
> @@ -552,6 +557,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> return code;
> }
>
> +static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
> + enum gacc_mode mode, enum prot_type prot)
> +{
> + return trans_exc_ending(vcpu, code, gva, ar, mode, prot, false);
> +}
> +
> static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce,
> unsigned long ga, u8 ar, enum gacc_mode mode)
> {
> @@ -1109,8 +1120,11 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> data += fragment_len;
> ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len);
> }
> - if (rc > 0)
> - rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
> + if (rc > 0) {
> + bool terminate = (mode == GACC_STORE) && (idx > 0);
> +
> + rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, terminate);
> + }
> out_unlock:
> if (need_ipte_lock)
> ipte_unlock(vcpu);
Am 12.05.22 um 15:10 schrieb Janis Schoetterl-Glausch:
> If a memop fails due to key checked protection, after already having
> written to the guest, don't indicate suppression to the guest, as that
> would imply that memory wasn't modified.
>
> This could be considered a fix to the code introducing storage key
> support, however this is a bug in KVM only if we emulate an
> instructions writing to an operand spanning multiple pages, which I
> don't believe we do.
>
> v2 -> v3
> * tweak commit message
> * explicitly reset the protection code to 0 on termination
> * use variable to pass termination arg
> * add documentation
> * fix magic constant in selftest
>
> Given the changes I did not pick up the r-b's.
Claudio, you had reviewed the first one. Is this still valid?
Am 12.05.22 um 15:10 schrieb Janis Schoetterl-Glausch:
> Check that suppression is not indicated on injection of a key checked
> protection exception caused by a memop after it already modified guest
> memory, as that violates the definition of suppression.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
> ---
> tools/testing/selftests/kvm/s390x/memop.c | 46 ++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index b04c2c1b3c30..49f26f544127 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -10,6 +10,8 @@
> #include <string.h>
> #include <sys/ioctl.h>
>
> +#include <linux/bits.h>
> +
> #include "test_util.h"
> #include "kvm_util.h"
>
> @@ -194,6 +196,7 @@ static int err_memop_ioctl(struct test_vcpu vcpu, struct kvm_s390_mem_op *ksmo)
> #define SIDA_OFFSET(o) ._sida_offset = 1, .sida_offset = (o)
> #define AR(a) ._ar = 1, .ar = (a)
> #define KEY(a) .f_key = 1, .key = (a)
> +#define INJECT .f_inject = 1
>
> #define CHECK_N_DO(f, ...) ({ f(__VA_ARGS__, CHECK_ONLY); f(__VA_ARGS__); })
>
> @@ -430,9 +433,18 @@ static void test_copy_key_fetch_prot(void)
> TEST_ASSERT(rv == 4, "Should result in protection exception"); \
> })
>
> +static void guest_error_key(void)
> +{
> + GUEST_SYNC(STAGE_INITED);
> + set_storage_key_range(mem1, PAGE_SIZE, 0x18);
> + set_storage_key_range(mem1 + PAGE_SIZE, sizeof(mem1) - PAGE_SIZE, 0x98);
> + GUEST_SYNC(STAGE_SKEYS_SET);
> + GUEST_SYNC(STAGE_IDLED);
> +}
> +
> static void test_errors_key(void)
> {
> - struct test_default t = test_default_init(guest_copy_key_fetch_prot);
> + struct test_default t = test_default_init(guest_error_key);
>
> HOST_SYNC(t.vcpu, STAGE_INITED);
> HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
> @@ -446,6 +458,37 @@ static void test_errors_key(void)
> kvm_vm_free(t.kvm_vm);
> }
>
> +static void test_termination(void)
> +{
> + struct test_default t = test_default_init(guest_error_key);
> + uint64_t prefix;
> + uint64_t teid;
> + uint64_t teid_mask = BIT(63 - 56) | BIT(63 - 60) | BIT(63 - 61);
> + uint64_t psw[2];
> +
> + HOST_SYNC(t.vcpu, STAGE_INITED);
> + HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
> +
> + /* vcpu, mismatching keys after first page */
> + ERR_PROT_MOP(t.vcpu, LOGICAL, WRITE, mem1, t.size, GADDR_V(mem1), KEY(1), INJECT);
> + /*
> + * The memop injected a program exception and the test needs to check the
> + * Translation-Exception Identification (TEID). It is necessary to run
> + * the guest in order to be able to read the TEID from guest memory.
> + * Set the guest program new PSW, so the guest state is not clobbered.
> + */
> + prefix = t.run->s.regs.prefix;
> + psw[0] = t.run->psw_mask;
> + psw[1] = t.run->psw_addr;
> + MOP(t.vm, ABSOLUTE, WRITE, psw, sizeof(psw), GADDR(prefix + 464));
> + HOST_SYNC(t.vcpu, STAGE_IDLED);
> + MOP(t.vm, ABSOLUTE, READ, &teid, sizeof(teid), GADDR(prefix + 168));
> + /* Bits 56, 60, 61 form a code, 0 being the only one allowing for termination */
> + ASSERT_EQ(teid & teid_mask, 0);
> +
> + kvm_vm_free(t.kvm_vm);
> +}
> +
> static void test_errors_key_storage_prot_override(void)
> {
> struct test_default t = test_default_init(guest_copy_key_fetch_prot);
> @@ -668,6 +711,7 @@ int main(int argc, char *argv[])
> test_copy_key_fetch_prot();
> test_copy_key_fetch_prot_override();
> test_errors_key();
> + test_termination();
> test_errors_key_storage_prot_override();
> test_errors_key_fetch_prot_override_not_enabled();
> test_errors_key_fetch_prot_override_enabled();
Am 12.05.22 um 15:10 schrieb Janis Schoetterl-Glausch:
> If user space uses a memop to emulate an instruction and that
> memop fails, the execution of the instruction ends.
> Instruction execution can end in different ways, one of which is
> suppression, which requires that the instruction execute like a no-op.
> A writing memop that spans multiple pages and fails due to key
> protection may have modified guest memory, as a result, the likely
> correct ending is termination. Therefore, do not indicate a
> suppressing instruction ending in this case.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
> ---
> Documentation/virt/kvm/api.rst | 6 ++++++
> arch/s390/kvm/gaccess.c | 22 ++++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 4a900cdbc62e..b6aba4f50db7 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -3754,12 +3754,18 @@ in case of KVM_S390_MEMOP_F_CHECK_ONLY), the ioctl returns a positive
> error number indicating the type of exception. This exception is also
> raised directly at the corresponding VCPU if the flag
> KVM_S390_MEMOP_F_INJECT_EXCEPTION is set.
> +On protection exceptions, unless specified otherwise, the injected
> +translation-exception identifier (TEID) indicates suppression.
>
> If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key
> protection is also in effect and may cause exceptions if accesses are
> prohibited given the access key designated by "key"; the valid range is 0..15.
> KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION
> is > 0.
> +Since the accessed memory may span multiple pages and those pages might have
> +different storage keys, it is possible that a protection exception occurs
> +after memory has been modified. In this case, if the exception is injected,
> +the TEID does not indicate suppression.
>
> Absolute read/write:
> ^^^^^^^^^^^^^^^^^^^^
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index d53a183c2005..227ed0009354 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -491,8 +491,8 @@ enum prot_type {
> PROT_TYPE_IEP = 4,
> };
>
> -static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> - u8 ar, enum gacc_mode mode, enum prot_type prot)
> +static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
> + enum gacc_mode mode, enum prot_type prot, bool terminate)
> {
> struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm;
> struct trans_exc_code_bits *tec;
> @@ -520,6 +520,11 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> tec->b61 = 1;
> break;
> }
> + if (terminate) {
> + tec->b56 = 0;
> + tec->b60 = 0;
> + tec->b61 = 0;
> + }
> fallthrough;
> case PGM_ASCE_TYPE:
> case PGM_PAGE_TRANSLATION:
> @@ -552,6 +557,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
> return code;
> }
>
> +static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
> + enum gacc_mode mode, enum prot_type prot)
> +{
> + return trans_exc_ending(vcpu, code, gva, ar, mode, prot, false);
> +}
> +
> static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce,
> unsigned long ga, u8 ar, enum gacc_mode mode)
> {
> @@ -1109,8 +1120,11 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> data += fragment_len;
> ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len);
> }
> - if (rc > 0)
> - rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
> + if (rc > 0) {
> + bool terminate = (mode == GACC_STORE) && (idx > 0);
> +
> + rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, terminate);
> + }
> out_unlock:
> if (need_ipte_lock)
> ipte_unlock(vcpu);