[PATCH v3 0/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[PATCH v3 0/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch]

User space can use the MEM_OP ioctl to make storage key checked reads
and writes to the guest, however, it has no way of performing atomic,
key checked, accesses to the guest.
Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
mode. For now, support this mode for absolute accesses only.

This mode can be use, for example, to set the device-state-change
indicator and the adapter-local-summary indicator atomically.

Also contains some fixes/changes for the memop selftest independent of
the cmpxchg changes.

v2 -> v3
 * rebase onto the wip/cmpxchg_user_key branch in the s390 kernel repo
 * use __uint128_t instead of unsigned __int128
 * put moving of testlist into main into separate patch
 * pick up R-b's (thanks Nico)

v1 -> v2
 * get rid of xrk instruction for cmpxchg byte and short implementation
 * pass old parameter via pointer instead of in mem_op struct
 * indicate failure of cmpxchg due to wrong old value by special return
   code
 * picked up R-b's (thanks Thomas)

Janis Schoetterl-Glausch (9):
  KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
  Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG
  KVM: s390: selftest: memop: Pass mop_desc via pointer
  KVM: s390: selftest: memop: Replace macros by functions
  KVM: s390: selftest: memop: Move testlist into main
  KVM: s390: selftest: memop: Add cmpxchg tests
  KVM: s390: selftest: memop: Add bad address test
  KVM: s390: selftest: memop: Fix typo
  KVM: s390: selftest: memop: Fix wrong address being used in test

 Documentation/virt/kvm/api.rst            |  21 +-
 include/uapi/linux/kvm.h                  |   5 +
 arch/s390/kvm/gaccess.h                   |   3 +
 arch/s390/kvm/gaccess.c                   | 101 ++++
 arch/s390/kvm/kvm-s390.c                  |  35 +-
 tools/testing/selftests/kvm/s390x/memop.c | 670 +++++++++++++++++-----
 6 files changed, 683 insertions(+), 152 deletions(-)

Range-diff against v2:
 1:  c6731b0063ab !  1:  94820a73e9b0 KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
    @@ arch/s390/kvm/gaccess.h: int access_guest_with_key(struct kvm_vcpu *vcpu, unsign
      		      void *data, unsigned long len, enum gacc_mode mode);
      
     +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
    -+			       unsigned __int128 *old,
    -+			       unsigned __int128 new, u8 access_key);
    ++			       __uint128_t *old, __uint128_t new, u8 access_key);
     +
      /**
       * write_guest_with_key - copy data from kernel space to guest space
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     + *         * a program interruption code indicating the reason cmpxchg could
     + *           not be attempted
     + *         * -EINVAL: address misaligned or len not power of two
    ++ *         * -EAGAIN: transient failure (len 1 or 2)
     + */
     +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
    -+			       unsigned __int128 *old_p, unsigned __int128 new,
    ++			       __uint128_t *old_p, __uint128_t new,
     +			       u8 access_key)
     +{
     +	gfn_t gfn = gpa >> PAGE_SHIFT;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		return -EOPNOTSUPP;
     +
     +	hva += offset_in_page(gpa);
    -+	ret = cmpxchg_user_key_size(len, (void __user *)hva, old_p, new, access_key);
    ++	switch (len) {
    ++	case 1: {
    ++		u8 old;
    ++
    ++		ret = cmpxchg_user_key((u8 *)hva, &old, *old_p, new, access_key);
    ++		ret = ret < 0 ? ret : old != *old_p;
    ++		*old_p = old;
    ++		break;
    ++	}
    ++	case 2: {
    ++		u16 old;
    ++
    ++		ret = cmpxchg_user_key((u16 *)hva, &old, *old_p, new, access_key);
    ++		ret = ret < 0 ? ret : old != *old_p;
    ++		*old_p = old;
    ++		break;
    ++	}
    ++	case 4: {
    ++		u32 old;
    ++
    ++		ret = cmpxchg_user_key((u32 *)hva, &old, *old_p, new, access_key);
    ++		ret = ret < 0 ? ret : old != *old_p;
    ++		*old_p = old;
    ++		break;
    ++	}
    ++	case 8: {
    ++		u64 old;
    ++
    ++		ret = cmpxchg_user_key((u64 *)hva, &old, *old_p, new, access_key);
    ++		ret = ret < 0 ? ret : old != *old_p;
    ++		*old_p = old;
    ++		break;
    ++	}
    ++	case 16: {
    ++		__uint128_t old;
    ++
    ++		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_p, new, access_key);
    ++		ret = ret < 0 ? ret : old != *old_p;
    ++		*old_p = old;
    ++		break;
    ++	}
    ++	default:
    ++		 return -EINVAL;
    ++	}
     +	mark_page_dirty_in_slot(kvm, slot, gfn);
     +	/*
     +	 * Assume that the fault is caused by protection, either key protection
    @@ arch/s390/kvm/kvm-s390.c: static bool access_key_invalid(u8 access_key)
      	void __user *uaddr = (void __user *)mop->buf;
     +	void __user *old_p = (void __user *)mop->old_p;
     +	union {
    -+		unsigned __int128 quad;
    -+		char raw[sizeof(unsigned __int128)];
    ++		__uint128_t quad;
    ++		char raw[sizeof(__uint128_t)];
     +	} old = { .quad = 0}, new = { .quad = 0 };
    -+	unsigned int off_in_quad = sizeof(unsigned __int128) - mop->size;
    ++	unsigned int off_in_quad = sizeof(__uint128_t) - mop->size;
      	u64 supported_flags;
      	void *tmpbuf = NULL;
      	int r, srcu_idx;
 2:  6cb32b244899 =  2:  28637a03f63e Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG
 3:  5f1217ad9d31 =  3:  31f45e4377c5 KVM: s390: selftest: memop: Pass mop_desc via pointer
 4:  86a15b53846a =  4:  dba0a8ba3ba2 KVM: s390: selftest: memop: Replace macros by functions
 -:  ------------ >  5:  ba8cdd064c02 KVM: s390: selftest: memop: Move testlist into main
 5:  49e67d7559de !  6:  3cf7f710e5db KVM: s390: selftest: memop: Add cmpxchg tests
    @@ tools/testing/selftests/kvm/s390x/memop.c: struct mop_desc {
      	uint8_t ar;
      	uint8_t key;
      };
    - 
    -+typedef unsigned __int128 uint128;
    -+
    - const uint8_t NO_KEY = 0xff;
    - 
    - static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
     @@ tools/testing/selftests/kvm/s390x/memop.c: static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
      		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
      		ksmo.key = desc->key;
    @@ tools/testing/selftests/kvm/s390x/memop.c: enum stage {
      	vcpu_run(__vcpu);						\
      	get_ucall(__vcpu, &uc);						\
     +	if (uc.cmd == UCALL_ABORT) {					\
    -+		TEST_FAIL("line %lu: %s, hints: %lu, %lu",		\
    -+			  uc.args[1], (const char *)uc.args[0],		\
    -+			  uc.args[2], uc.args[3]);			\
    ++		REPORT_GUEST_ASSERT_2(uc, "hints: %lu, %lu");		\
     +	}								\
      	ASSERT_EQ(uc.cmd, UCALL_SYNC);					\
      	ASSERT_EQ(uc.args[1], __stage);					\
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	kvm_vm_free(t.kvm_vm);
     +}
     +
    -+static uint128 cut_to_size(int size, uint128 val)
    ++static __uint128_t cut_to_size(int size, __uint128_t val)
     +{
     +	switch (size) {
     +	case 1:
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	return 0;
     +}
     +
    -+static bool popcount_eq(uint128 a, uint128 b)
    ++static bool popcount_eq(__uint128_t a, __uint128_t b)
     +{
     +	unsigned int count_a, count_b;
     +
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	return count_a == count_b;
     +}
     +
    -+static uint128 rotate(int size, uint128 val, int amount)
    ++static __uint128_t rotate(int size, __uint128_t val, int amount)
     +{
     +	unsigned int bits = size * 8;
     +
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	}
     +}
     +
    -+static uint128 permutate_bits(bool guest, int i, int size, uint128 old)
    ++static __uint128_t permutate_bits(bool guest, int i, int size, __uint128_t old)
     +{
     +	unsigned int rand;
     +	bool swap;
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	swap = rand % 2 == 0;
     +	if (swap) {
     +		int i, j;
    -+		uint128 new;
    ++		__uint128_t new;
     +		uint8_t byte0, byte1;
     +
     +		rand = rand * 3 + 1;
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	}
     +}
     +
    -+static bool _cmpxchg(int size, void *target, uint128 *old_p, uint128 new)
    ++static bool _cmpxchg(int size, void *target, __uint128_t *old_p, __uint128_t new)
     +{
     +	bool ret;
     +
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +			return ret;
     +		}
     +	case 16: {
    -+			uint128 old = *old_p;
    ++			__uint128_t old = *old_p;
     +
     +			asm volatile ("cdsg %[old],%[new],%[address]"
     +			    : [old] "+d" (old),
    -+			      [address] "+Q" (*(uint128 *)(target))
    ++			      [address] "+Q" (*(__uint128_t *)(target))
     +			    : [new] "d" (new)
     +			    : "cc"
     +			);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +static void guest_cmpxchg_key(void)
     +{
     +	int size, offset;
    -+	uint128 old, new;
    ++	__uint128_t old, new;
     +
     +	set_storage_key_range(mem1, max_block, 0x10);
     +	set_storage_key_range(mem2, max_block, 0x10);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	return NULL;
     +}
     +
    -+static char *quad_to_char(uint128 *quad, int size)
    ++static char *quad_to_char(__uint128_t *quad, int size)
     +{
     +	return ((char *)quad) + (sizeof(*quad) - size);
     +}
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +{
     +	struct test_default t = test_default_init(guest_cmpxchg_key);
     +	int size, offset;
    -+	uint128 old, new;
    ++	__uint128_t old, new;
     +	bool success;
     +	pthread_t thread;
     +
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +	pthread_join(thread, NULL);
     +
     +	MOP(t.vcpu, LOGICAL, READ, mem2, max_block, GADDR_V(mem2));
    -+	TEST_ASSERT(popcount_eq(*(uint128 *)mem1, *(uint128 *)mem2),
    ++	TEST_ASSERT(popcount_eq(*(__uint128_t *)mem1, *(__uint128_t *)mem2),
     +		    "Must retain number of set bits");
     +
     +	kvm_vm_free(t.kvm_vm);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key(void)
     +	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
     +
     +	for (i = 1; i <= 16; i *= 2) {
    -+		uint128 old = 0;
    ++		__uint128_t old = 0;
     +
     +		CHECK_N_DO(ERR_PROT_MOP, t.vm, ABSOLUTE, WRITE, mem2, i, GADDR_V(mem2),
     +			   CMPXCHG_OLD(&old), KEY(2));
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
      	kvm_vm_free(t.kvm_vm);
      }
      
    --struct testdef {
    --	const char *name;
    --	void (*test)(void);
    --	int extension;
    --} testlist[] = {
    --	{
    --		.name = "simple copy",
    --		.test = test_copy,
    --	},
    --	{
    --		.name = "generic error checks",
    --		.test = test_errors,
    --	},
    --	{
    --		.name = "copy with storage keys",
    --		.test = test_copy_key,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "copy with key storage protection override",
    --		.test = test_copy_key_storage_prot_override,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "copy with key fetch protection",
    --		.test = test_copy_key_fetch_prot,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "copy with key fetch protection override",
    --		.test = test_copy_key_fetch_prot_override,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "error checks with key",
    --		.test = test_errors_key,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "termination",
    --		.test = test_termination,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "error checks with key storage protection override",
    --		.test = test_errors_key_storage_prot_override,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "error checks without key fetch prot override",
    --		.test = test_errors_key_fetch_prot_override_not_enabled,
    --		.extension = 1,
    --	},
    --	{
    --		.name = "error checks with key fetch prot override",
    --		.test = test_errors_key_fetch_prot_override_enabled,
    --		.extension = 1,
    --	},
    --};
     +static void test_errors_cmpxchg(void)
     +{
     +	struct test_default t = test_default_init(guest_idle);
    -+	uint128 old;
    ++	__uint128_t old;
     +	int rv, i, power = 1;
     +
     +	HOST_SYNC(t.vcpu, STAGE_INITED);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
      
      int main(int argc, char *argv[])
      {
    - 	int extension_cap, idx;
    - 
    -+	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
    - 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_MEM_OP));
    -+	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
    - 
    --	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
    -+	struct testdef {
    -+		const char *name;
    -+		void (*test)(void);
    -+		bool requirements_met;
    -+	} testlist[] = {
    -+		{
    -+			.name = "simple copy",
    -+			.test = test_copy,
    -+			.requirements_met = true,
    -+		},
    -+		{
    -+			.name = "generic error checks",
    -+			.test = test_errors,
    -+			.requirements_met = true,
    -+		},
    -+		{
    -+			.name = "copy with storage keys",
    -+			.test = test_copy_key,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    +@@ tools/testing/selftests/kvm/s390x/memop.c: int main(int argc, char *argv[])
    + 			.test = test_copy_key,
    + 			.requirements_met = extension_cap > 0,
    + 		},
     +		{
     +			.name = "cmpxchg with storage keys",
     +			.test = test_cmpxchg_key,
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
     +			.test = test_cmpxchg_key_concurrent,
     +			.requirements_met = extension_cap & 0x2,
     +		},
    -+		{
    -+			.name = "copy with key storage protection override",
    -+			.test = test_copy_key_storage_prot_override,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "copy with key fetch protection",
    -+			.test = test_copy_key_fetch_prot,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "copy with key fetch protection override",
    -+			.test = test_copy_key_fetch_prot_override,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "error checks with key",
    -+			.test = test_errors_key,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    + 		{
    + 			.name = "copy with key storage protection override",
    + 			.test = test_copy_key_storage_prot_override,
    +@@ tools/testing/selftests/kvm/s390x/memop.c: int main(int argc, char *argv[])
    + 			.test = test_errors_key,
    + 			.requirements_met = extension_cap > 0,
    + 		},
     +		{
     +			.name = "error checks for cmpxchg with key",
     +			.test = test_errors_cmpxchg_key,
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
     +			.test = test_errors_cmpxchg,
     +			.requirements_met = extension_cap & 0x2,
     +		},
    -+		{
    -+			.name = "termination",
    -+			.test = test_termination,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "error checks with key storage protection override",
    -+			.test = test_errors_key_storage_prot_override,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "error checks without key fetch prot override",
    -+			.test = test_errors_key_fetch_prot_override_not_enabled,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+		{
    -+			.name = "error checks with key fetch prot override",
    -+			.test = test_errors_key_fetch_prot_override_enabled,
    -+			.requirements_met = extension_cap > 0,
    -+		},
    -+	};
    - 
    - 	ksft_print_header();
    --
    - 	ksft_set_plan(ARRAY_SIZE(testlist));
    - 
    --	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
    - 	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
    --		if (extension_cap >= testlist[idx].extension) {
    -+		if (testlist[idx].requirements_met) {
    - 			testlist[idx].test();
    - 			ksft_test_result_pass("%s\n", testlist[idx].name);
    - 		} else {
    --			ksft_test_result_skip("%s - extension level %d not supported\n",
    --					      testlist[idx].name,
    --					      testlist[idx].extension);
    -+			ksft_test_result_skip("%s - requirements not met (kernel has extension cap %#x\n)",
    -+					      testlist[idx].name, extension_cap);
    - 		}
    - 	}
    - 
    + 		{
    + 			.name = "termination",
    + 			.test = test_termination,
 6:  faad9cf03ea6 !  7:  d81d5697ba4b KVM: s390: selftest: memop: Add bad address test
    @@ Commit message
         Add test that tries to access, instead of CHECK_ONLY.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void _test_errors_common(struct test_info info, enum mop_target target, i
 7:  8070036aa89a !  8:  22c9cd9589ba KVM: s390: selftest: memop: Fix typo
    @@ Commit message
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
         Reviewed-by: Thomas Huth <thuth@redhat.com>
    +    Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key_fetch_prot_override_enabled(void)
 8:  18c423e4e3ad !  9:  4647be0790c8 KVM: s390: selftest: memop: Fix wrong address being used in test
    @@ Commit message
         protection exception the test codes needs to address mem1.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key(void)

base-commit: b5b2a05e6de7b88bcfca9d4bbc8ac74e7db80c52
-- 
2.34.1

[PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch]

User space can use the MEM_OP ioctl to make storage key checked reads
and writes to the guest, however, it has no way of performing atomic,
key checked, accesses to the guest.
Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
mode. For now, support this mode for absolute accesses only.

This mode can be use, for example, to set the device-state-change
indicator and the adapter-local-summary indicator atomically.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 include/uapi/linux/kvm.h |   5 ++
 arch/s390/kvm/gaccess.h  |   3 ++
 arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
 4 files changed, 142 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d5d4419139a..1f36be5493e6 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
 		struct {
 			__u8 ar;	/* the access register number */
 			__u8 key;	/* access key, ignored if flag unset */
+			__u8 pad1[6];	/* ignored */
+			__u64 old_p;	/* ignored if flag unset */
 		};
 		__u32 sida_offset; /* offset into the sida */
 		__u8 reserved[32]; /* ignored */
@@ -604,6 +606,9 @@ struct kvm_s390_mem_op {
 #define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
 #define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)
 #define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
+#define KVM_S390_MEMOP_F_CMPXCHG		(1ULL << 3)
+/* Non program exception return codes (pgm codes are 16 bit) */
+#define KVM_S390_MEMOP_R_NO_XCHG		((1 << 16) + 0)
 
 /* for KVM_INTERRUPT */
 struct kvm_interrupt {
diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 9408d6cc8e2c..92a3b9fb31ec 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -206,6 +206,9 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
 int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
 		      void *data, unsigned long len, enum gacc_mode mode);
 
+int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
+			       __uint128_t *old, __uint128_t new, u8 access_key);
+
 /**
  * write_guest_with_key - copy data from kernel space to guest space
  * @vcpu: virtual cpu
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 0243b6e38d36..be042865d8a1 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1161,6 +1161,107 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
 	return rc;
 }
 
+/**
+ * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
+ * @kvm: Virtual machine instance.
+ * @gpa: Absolute guest address of the location to be changed.
+ * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
+ *       non power of two will result in failure.
+ * @old_p: Pointer to old value. If the location at @gpa contains this value, the
+ *         exchange will succeed. After calling cmpxchg_guest_abs_with_key() *@old
+ *         contains the value at @gpa before the attempt to exchange the value.
+ * @new: The value to place at @gpa.
+ * @access_key: The access key to use for the guest access.
+ *
+ * Atomically exchange the value at @gpa by @new, if it contains *@old.
+ * Honors storage keys.
+ *
+ * Return: * 0: successful exchange
+ *         * 1: exchange unsuccessful
+ *         * a program interruption code indicating the reason cmpxchg could
+ *           not be attempted
+ *         * -EINVAL: address misaligned or len not power of two
+ *         * -EAGAIN: transient failure (len 1 or 2)
+ */
+int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
+			       __uint128_t *old_p, __uint128_t new,
+			       u8 access_key)
+{
+	gfn_t gfn = gpa >> PAGE_SHIFT;
+	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
+	bool writable;
+	hva_t hva;
+	int ret;
+
+	if (!IS_ALIGNED(gpa, len))
+		return -EINVAL;
+
+	hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
+	if (kvm_is_error_hva(hva))
+		return PGM_ADDRESSING;
+	/*
+	 * Check if it's a read-only memslot, even though that cannot occur
+	 * since those are unsupported.
+	 * Don't try to actually handle that case.
+	 */
+	if (!writable)
+		return -EOPNOTSUPP;
+
+	hva += offset_in_page(gpa);
+	switch (len) {
+	case 1: {
+		u8 old;
+
+		ret = cmpxchg_user_key((u8 *)hva, &old, *old_p, new, access_key);
+		ret = ret < 0 ? ret : old != *old_p;
+		*old_p = old;
+		break;
+	}
+	case 2: {
+		u16 old;
+
+		ret = cmpxchg_user_key((u16 *)hva, &old, *old_p, new, access_key);
+		ret = ret < 0 ? ret : old != *old_p;
+		*old_p = old;
+		break;
+	}
+	case 4: {
+		u32 old;
+
+		ret = cmpxchg_user_key((u32 *)hva, &old, *old_p, new, access_key);
+		ret = ret < 0 ? ret : old != *old_p;
+		*old_p = old;
+		break;
+	}
+	case 8: {
+		u64 old;
+
+		ret = cmpxchg_user_key((u64 *)hva, &old, *old_p, new, access_key);
+		ret = ret < 0 ? ret : old != *old_p;
+		*old_p = old;
+		break;
+	}
+	case 16: {
+		__uint128_t old;
+
+		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_p, new, access_key);
+		ret = ret < 0 ? ret : old != *old_p;
+		*old_p = old;
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+	mark_page_dirty_in_slot(kvm, slot, gfn);
+	/*
+	 * Assume that the fault is caused by protection, either key protection
+	 * or user page write protection.
+	 */
+	if (ret == -EFAULT)
+		ret = PGM_PROTECTION;
+	return ret;
+}
+
 /**
  * guest_translate_address_with_key - translate guest logical into guest absolute address
  * @vcpu: virtual cpu
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 45d4b8182b07..2410b4044283 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -576,7 +576,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_VCPU_RESETS:
 	case KVM_CAP_SET_GUEST_DEBUG:
 	case KVM_CAP_S390_DIAG318:
-	case KVM_CAP_S390_MEM_OP_EXTENSION:
 		r = 1;
 		break;
 	case KVM_CAP_SET_GUEST_DEBUG2:
@@ -590,6 +589,14 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_MEM_OP:
 		r = MEM_OP_MAX_SIZE;
 		break;
+	case KVM_CAP_S390_MEM_OP_EXTENSION:
+		/*
+		 * Flag bits indicating which extensions are supported.
+		 * The first extension doesn't use a flag, but pretend it does,
+		 * this way that can be changed in the future.
+		 */
+		r = 0x3;
+		break;
 	case KVM_CAP_NR_VCPUS:
 	case KVM_CAP_MAX_VCPUS:
 	case KVM_CAP_MAX_VCPU_ID:
@@ -2714,12 +2721,19 @@ static bool access_key_invalid(u8 access_key)
 static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 {
 	void __user *uaddr = (void __user *)mop->buf;
+	void __user *old_p = (void __user *)mop->old_p;
+	union {
+		__uint128_t quad;
+		char raw[sizeof(__uint128_t)];
+	} old = { .quad = 0}, new = { .quad = 0 };
+	unsigned int off_in_quad = sizeof(__uint128_t) - mop->size;
 	u64 supported_flags;
 	void *tmpbuf = NULL;
 	int r, srcu_idx;
 
 	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
-			  | KVM_S390_MEMOP_F_CHECK_ONLY;
+			  | KVM_S390_MEMOP_F_CHECK_ONLY
+			  | KVM_S390_MEMOP_F_CMPXCHG;
 	if (mop->flags & ~supported_flags || !mop->size)
 		return -EINVAL;
 	if (mop->size > MEM_OP_MAX_SIZE)
@@ -2741,6 +2755,15 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	} else {
 		mop->key = 0;
 	}
+	if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {
+		if (mop->size > sizeof(new))
+			return -EINVAL;
+		/* off_in_quad has been validated */
+		if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
+			return -EFAULT;
+		if (copy_from_user(&old.raw[off_in_quad], old_p, mop->size))
+			return -EFAULT;
+	}
 	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
 		tmpbuf = vmalloc(mop->size);
 		if (!tmpbuf)
@@ -2771,6 +2794,14 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	case KVM_S390_MEMOP_ABSOLUTE_WRITE: {
 		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
 			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key);
+		} else if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {
+			r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size,
+						       &old.quad, new.quad, mop->key);
+			if (r == 1) {
+				r = KVM_S390_MEMOP_R_NO_XCHG;
+				if (copy_to_user(old_p, &old.raw[off_in_quad], mop->size))
+					r = -EFAULT;
+			}
 		} else {
 			if (copy_from_user(tmpbuf, uaddr, mop->size)) {
 				r = -EFAULT;
-- 
2.34.1

[PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Janis Schoetterl-Glausch]

Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for
absolute vm write memops which allows user space to perform (storage key
checked) cmpxchg operations on guest memory.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index eee9f857a986..204d128f23e0 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -3753,7 +3753,8 @@ The fields in each entry are defined as follows:
 :Parameters: struct kvm_s390_mem_op (in)
 :Returns: = 0 on success,
           < 0 on generic error (e.g. -EFAULT or -ENOMEM),
-          > 0 if an exception occurred while walking the page tables
+          16 bit program exception code if the access causes such an exception
+          other code > maximum 16 bit value with special meaning
 
 Read or write data from/to the VM's memory.
 The KVM_CAP_S390_MEM_OP_EXTENSION capability specifies what functionality is
@@ -3771,6 +3772,8 @@ Parameters are specified via the following structure::
 		struct {
 			__u8 ar;	/* the access register number */
 			__u8 key;	/* access key, ignored if flag unset */
+			__u8 pad1[6];	/* ignored */
+			__u64 old_p;	/* ignored if flag unset */
 		};
 		__u32 sida_offset; /* offset into the sida */
 		__u8 reserved[32]; /* ignored */
@@ -3853,8 +3856,22 @@ Absolute accesses are permitted for non-protected guests only.
 Supported flags:
   * ``KVM_S390_MEMOP_F_CHECK_ONLY``
   * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
+  * ``KVM_S390_MEMOP_F_CMPXCHG``
+
+The semantics of the flags common with logical acesses are as for logical
+accesses.
+
+For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
+In this case, instead of doing an unconditional write, the access occurs only
+if the target location contains the "size" byte long value pointed to by
+"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two
+up to and including 16.
+The value at the target location is written to the location "old_p" points to.
+If the exchange did not take place because the target value doesn't match the
+old value KVM_S390_MEMOP_R_NO_XCHG is returned.
+The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
+has bit 1 (i.e. bit with value 2) set.
 
-The semantics of the flags are as for logical accesses.
 
 SIDA read/write:
 ^^^^^^^^^^^^^^^^
-- 
2.34.1

[PATCH v3 3/9] KVM: s390: selftest: memop: Pass mop_desc via pointer

[Janis Schoetterl-Glausch]

The struct is quite large, so this seems nicer.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 44 +++++++++++------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 9113696d5178..69869c7e2ab1 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -48,53 +48,53 @@ struct mop_desc {
 	uint8_t key;
 };
 
-static struct kvm_s390_mem_op ksmo_from_desc(struct mop_desc desc)
+static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
 {
 	struct kvm_s390_mem_op ksmo = {
-		.gaddr = (uintptr_t)desc.gaddr,
-		.size = desc.size,
-		.buf = ((uintptr_t)desc.buf),
+		.gaddr = (uintptr_t)desc->gaddr,
+		.size = desc->size,
+		.buf = ((uintptr_t)desc->buf),
 		.reserved = "ignored_ignored_ignored_ignored"
 	};
 
-	switch (desc.target) {
+	switch (desc->target) {
 	case LOGICAL:
-		if (desc.mode == READ)
+		if (desc->mode == READ)
 			ksmo.op = KVM_S390_MEMOP_LOGICAL_READ;
-		if (desc.mode == WRITE)
+		if (desc->mode == WRITE)
 			ksmo.op = KVM_S390_MEMOP_LOGICAL_WRITE;
 		break;
 	case SIDA:
-		if (desc.mode == READ)
+		if (desc->mode == READ)
 			ksmo.op = KVM_S390_MEMOP_SIDA_READ;
-		if (desc.mode == WRITE)
+		if (desc->mode == WRITE)
 			ksmo.op = KVM_S390_MEMOP_SIDA_WRITE;
 		break;
 	case ABSOLUTE:
-		if (desc.mode == READ)
+		if (desc->mode == READ)
 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_READ;
-		if (desc.mode == WRITE)
+		if (desc->mode == WRITE)
 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_WRITE;
 		break;
 	case INVALID:
 		ksmo.op = -1;
 	}
-	if (desc.f_check)
+	if (desc->f_check)
 		ksmo.flags |= KVM_S390_MEMOP_F_CHECK_ONLY;
-	if (desc.f_inject)
+	if (desc->f_inject)
 		ksmo.flags |= KVM_S390_MEMOP_F_INJECT_EXCEPTION;
-	if (desc._set_flags)
-		ksmo.flags = desc.set_flags;
-	if (desc.f_key) {
+	if (desc->_set_flags)
+		ksmo.flags = desc->set_flags;
+	if (desc->f_key) {
 		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
-		ksmo.key = desc.key;
+		ksmo.key = desc->key;
 	}
-	if (desc._ar)
-		ksmo.ar = desc.ar;
+	if (desc->_ar)
+		ksmo.ar = desc->ar;
 	else
 		ksmo.ar = 0;
-	if (desc._sida_offset)
-		ksmo.sida_offset = desc.sida_offset;
+	if (desc->_sida_offset)
+		ksmo.sida_offset = desc->sida_offset;
 
 	return ksmo;
 }
@@ -183,7 +183,7 @@ static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
 		else								\
 			__desc.gaddr = __desc.gaddr_v;				\
 	}									\
-	__ksmo = ksmo_from_desc(__desc);					\
+	__ksmo = ksmo_from_desc(&__desc);					\
 	print_memop(__info.vcpu, &__ksmo);					\
 	err##memop_ioctl(__info, &__ksmo);					\
 })
-- 
2.34.1

[PATCH v3 4/9] KVM: s390: selftest: memop: Replace macros by functions

[Janis Schoetterl-Glausch]

Replace the DEFAULT_* test helpers by functions, as they don't
need the exta flexibility.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 82 +++++++++++------------
 1 file changed, 39 insertions(+), 43 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 69869c7e2ab1..286185a59238 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -48,6 +48,8 @@ struct mop_desc {
 	uint8_t key;
 };
 
+const uint8_t NO_KEY = 0xff;
+
 static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
 {
 	struct kvm_s390_mem_op ksmo = {
@@ -85,7 +87,7 @@ static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
 		ksmo.flags |= KVM_S390_MEMOP_F_INJECT_EXCEPTION;
 	if (desc->_set_flags)
 		ksmo.flags = desc->set_flags;
-	if (desc->f_key) {
+	if (desc->f_key && desc->key != NO_KEY) {
 		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
 		ksmo.key = desc->key;
 	}
@@ -268,34 +270,28 @@ static void prepare_mem12(void)
 #define ASSERT_MEM_EQ(p1, p2, size) \
 	TEST_ASSERT(!memcmp(p1, p2, size), "Memory contents do not match!")
 
-#define DEFAULT_WRITE_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
-({										\
-	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
-	enum mop_target __target = (mop_target_p);				\
-	uint32_t __size = (size);						\
-										\
-	prepare_mem12();							\
-	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
-			GADDR_V(mem1), ##__VA_ARGS__);				\
-	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
-	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size,		\
-			GADDR_V(mem2), ##__VA_ARGS__);				\
-	ASSERT_MEM_EQ(mem1, mem2, __size);					\
-})
+static void default_write_read(struct test_info copy_cpu, struct test_info mop_cpu,
+			       enum mop_target mop_target, uint32_t size, uint8_t key)
+{
+	prepare_mem12();
+	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size,
+		   GADDR_V(mem1), KEY(key));
+	HOST_SYNC(copy_cpu, STAGE_COPIED);
+	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
+		   GADDR_V(mem2), KEY(key));
+	ASSERT_MEM_EQ(mem1, mem2, size);
+}
 
-#define DEFAULT_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
-({										\
-	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
-	enum mop_target __target = (mop_target_p);				\
-	uint32_t __size = (size);						\
-										\
-	prepare_mem12();							\
-	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
-			GADDR_V(mem1));						\
-	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
-	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size, ##__VA_ARGS__);\
-	ASSERT_MEM_EQ(mem1, mem2, __size);					\
-})
+static void default_read(struct test_info copy_cpu, struct test_info mop_cpu,
+			 enum mop_target mop_target, uint32_t size, uint8_t key)
+{
+	prepare_mem12();
+	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size, GADDR_V(mem1));
+	HOST_SYNC(copy_cpu, STAGE_COPIED);
+	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
+		   GADDR_V(mem2), KEY(key));
+	ASSERT_MEM_EQ(mem1, mem2, size);
+}
 
 static void guest_copy(void)
 {
@@ -310,7 +306,7 @@ static void test_copy(void)
 
 	HOST_SYNC(t.vcpu, STAGE_INITED);
 
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size);
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, NO_KEY);
 
 	kvm_vm_free(t.kvm_vm);
 }
@@ -357,26 +353,26 @@ static void test_copy_key(void)
 	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
 
 	/* vm, no key */
-	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size);
+	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, NO_KEY);
 
 	/* vm/vcpu, machting key or key 0 */
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(0));
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(9));
-	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size, KEY(0));
-	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size, KEY(9));
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 0);
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 9);
+	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, 0);
+	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, 9);
 	/*
 	 * There used to be different code paths for key handling depending on
 	 * if the region crossed a page boundary.
 	 * There currently are not, but the more tests the merrier.
 	 */
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, 1, KEY(0));
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, 1, KEY(9));
-	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, 1, KEY(0));
-	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, 1, KEY(9));
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, 1, 0);
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, 1, 9);
+	default_write_read(t.vcpu, t.vm, ABSOLUTE, 1, 0);
+	default_write_read(t.vcpu, t.vm, ABSOLUTE, 1, 9);
 
 	/* vm/vcpu, mismatching keys on read, but no fetch protection */
-	DEFAULT_READ(t.vcpu, t.vcpu, LOGICAL, t.size, GADDR_V(mem2), KEY(2));
-	DEFAULT_READ(t.vcpu, t.vm, ABSOLUTE, t.size, GADDR_V(mem1), KEY(2));
+	default_read(t.vcpu, t.vcpu, LOGICAL, t.size, 2);
+	default_read(t.vcpu, t.vm, ABSOLUTE, t.size, 2);
 
 	kvm_vm_free(t.kvm_vm);
 }
@@ -409,7 +405,7 @@ static void test_copy_key_storage_prot_override(void)
 	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
 
 	/* vcpu, mismatching keys, storage protection override in effect */
-	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(2));
+	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 2);
 
 	kvm_vm_free(t.kvm_vm);
 }
@@ -422,8 +418,8 @@ static void test_copy_key_fetch_prot(void)
 	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
 
 	/* vm/vcpu, matching key, fetch protection in effect */
-	DEFAULT_READ(t.vcpu, t.vcpu, LOGICAL, t.size, GADDR_V(mem2), KEY(9));
-	DEFAULT_READ(t.vcpu, t.vm, ABSOLUTE, t.size, GADDR_V(mem2), KEY(9));
+	default_read(t.vcpu, t.vcpu, LOGICAL, t.size, 9);
+	default_read(t.vcpu, t.vm, ABSOLUTE, t.size, 9);
 
 	kvm_vm_free(t.kvm_vm);
 }
-- 
2.34.1

[PATCH v3 5/9] KVM: s390: selftest: memop: Move testlist into main

[Janis Schoetterl-Glausch]

This allows checking if the necessary requirements for a test case are
met via an arbitrary expression. In particular, it is easy to check if
certain bits are set in the memop extension capability.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 132 +++++++++++-----------
 1 file changed, 66 insertions(+), 66 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 286185a59238..10f34c629cac 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -690,87 +690,87 @@ static void test_errors(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
-struct testdef {
-	const char *name;
-	void (*test)(void);
-	int extension;
-} testlist[] = {
-	{
-		.name = "simple copy",
-		.test = test_copy,
-	},
-	{
-		.name = "generic error checks",
-		.test = test_errors,
-	},
-	{
-		.name = "copy with storage keys",
-		.test = test_copy_key,
-		.extension = 1,
-	},
-	{
-		.name = "copy with key storage protection override",
-		.test = test_copy_key_storage_prot_override,
-		.extension = 1,
-	},
-	{
-		.name = "copy with key fetch protection",
-		.test = test_copy_key_fetch_prot,
-		.extension = 1,
-	},
-	{
-		.name = "copy with key fetch protection override",
-		.test = test_copy_key_fetch_prot_override,
-		.extension = 1,
-	},
-	{
-		.name = "error checks with key",
-		.test = test_errors_key,
-		.extension = 1,
-	},
-	{
-		.name = "termination",
-		.test = test_termination,
-		.extension = 1,
-	},
-	{
-		.name = "error checks with key storage protection override",
-		.test = test_errors_key_storage_prot_override,
-		.extension = 1,
-	},
-	{
-		.name = "error checks without key fetch prot override",
-		.test = test_errors_key_fetch_prot_override_not_enabled,
-		.extension = 1,
-	},
-	{
-		.name = "error checks with key fetch prot override",
-		.test = test_errors_key_fetch_prot_override_enabled,
-		.extension = 1,
-	},
-};
 
 int main(int argc, char *argv[])
 {
 	int extension_cap, idx;
 
+	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_MEM_OP));
+	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
 
-	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
+	struct testdef {
+		const char *name;
+		void (*test)(void);
+		bool requirements_met;
+	} testlist[] = {
+		{
+			.name = "simple copy",
+			.test = test_copy,
+			.requirements_met = true,
+		},
+		{
+			.name = "generic error checks",
+			.test = test_errors,
+			.requirements_met = true,
+		},
+		{
+			.name = "copy with storage keys",
+			.test = test_copy_key,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "copy with key storage protection override",
+			.test = test_copy_key_storage_prot_override,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "copy with key fetch protection",
+			.test = test_copy_key_fetch_prot,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "copy with key fetch protection override",
+			.test = test_copy_key_fetch_prot_override,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "error checks with key",
+			.test = test_errors_key,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "termination",
+			.test = test_termination,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "error checks with key storage protection override",
+			.test = test_errors_key_storage_prot_override,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "error checks without key fetch prot override",
+			.test = test_errors_key_fetch_prot_override_not_enabled,
+			.requirements_met = extension_cap > 0,
+		},
+		{
+			.name = "error checks with key fetch prot override",
+			.test = test_errors_key_fetch_prot_override_enabled,
+			.requirements_met = extension_cap > 0,
+		},
+	};
 
 	ksft_print_header();
-
 	ksft_set_plan(ARRAY_SIZE(testlist));
 
-	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
 	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
-		if (extension_cap >= testlist[idx].extension) {
+		if (testlist[idx].requirements_met) {
 			testlist[idx].test();
 			ksft_test_result_pass("%s\n", testlist[idx].name);
 		} else {
-			ksft_test_result_skip("%s - extension level %d not supported\n",
-					      testlist[idx].name,
-					      testlist[idx].extension);
+			ksft_test_result_skip("%s - requirements not met (kernel has extension cap %#x\n)",
+					      testlist[idx].name, extension_cap);
 		}
 	}
 
-- 
2.34.1

[PATCH v3 6/9] KVM: s390: selftest: memop: Add cmpxchg tests

[Janis Schoetterl-Glausch]

Test successful exchange, unsuccessful exchange, storage key protection
and invalid arguments.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 404 +++++++++++++++++++++-
 1 file changed, 390 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 10f34c629cac..269b181731c8 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/ioctl.h>
+#include <pthread.h>
 
 #include <linux/bits.h>
 
@@ -44,6 +45,8 @@ struct mop_desc {
 	enum mop_access_mode mode;
 	void *buf;
 	uint32_t sida_offset;
+	void *old;
+	bool *cmpxchg_success;
 	uint8_t ar;
 	uint8_t key;
 };
@@ -91,6 +94,10 @@ static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
 		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
 		ksmo.key = desc->key;
 	}
+	if (desc->old) {
+		ksmo.flags |= KVM_S390_MEMOP_F_CMPXCHG;
+		ksmo.old_p = (uint64_t)desc->old;
+	}
 	if (desc->_ar)
 		ksmo.ar = desc->ar;
 	else
@@ -136,35 +143,45 @@ static void print_memop(struct kvm_vcpu *vcpu, const struct kvm_s390_mem_op *ksm
 		printf("ABSOLUTE, WRITE, ");
 		break;
 	}
-	printf("gaddr=%llu, size=%u, buf=%llu, ar=%u, key=%u",
-	       ksmo->gaddr, ksmo->size, ksmo->buf, ksmo->ar, ksmo->key);
+	printf("gaddr=%llu, size=%u, buf=%llu, ar=%u, key=%u, old_p=%llx",
+	       ksmo->gaddr, ksmo->size, ksmo->buf, ksmo->ar, ksmo->key,
+	       ksmo->old_p);
 	if (ksmo->flags & KVM_S390_MEMOP_F_CHECK_ONLY)
 		printf(", CHECK_ONLY");
 	if (ksmo->flags & KVM_S390_MEMOP_F_INJECT_EXCEPTION)
 		printf(", INJECT_EXCEPTION");
 	if (ksmo->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION)
 		printf(", SKEY_PROTECTION");
+	if (ksmo->flags & KVM_S390_MEMOP_F_CMPXCHG)
+		printf(", CMPXCHG");
 	puts(")");
 }
 
-static void memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
+static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo,
+			   struct mop_desc *desc)
 {
 	struct kvm_vcpu *vcpu = info.vcpu;
 
 	if (!vcpu)
-		vm_ioctl(info.vm, KVM_S390_MEM_OP, ksmo);
+		return __vm_ioctl(info.vm, KVM_S390_MEM_OP, ksmo);
 	else
-		vcpu_ioctl(vcpu, KVM_S390_MEM_OP, ksmo);
+		return __vcpu_ioctl(vcpu, KVM_S390_MEM_OP, ksmo);
 }
 
-static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
+static void memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo,
+			struct mop_desc *desc)
 {
-	struct kvm_vcpu *vcpu = info.vcpu;
+	int r;
+
+	r = err_memop_ioctl(info, ksmo, desc);
+	if (ksmo->flags & KVM_S390_MEMOP_F_CMPXCHG) {
+		if (desc->cmpxchg_success)
+			*desc->cmpxchg_success = !r;
+		if (r == KVM_S390_MEMOP_R_NO_XCHG)
+			r = 0;
+	}
+	TEST_ASSERT(!r, __KVM_IOCTL_ERROR("KVM_S390_MEM_OP", r));
 
-	if (!vcpu)
-		return __vm_ioctl(info.vm, KVM_S390_MEM_OP, ksmo);
-	else
-		return __vcpu_ioctl(vcpu, KVM_S390_MEM_OP, ksmo);
 }
 
 #define MEMOP(err, info_p, mop_target_p, access_mode_p, buf_p, size_p, ...)	\
@@ -187,7 +204,7 @@ static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
 	}									\
 	__ksmo = ksmo_from_desc(&__desc);					\
 	print_memop(__info.vcpu, &__ksmo);					\
-	err##memop_ioctl(__info, &__ksmo);					\
+	err##memop_ioctl(__info, &__ksmo, &__desc);				\
 })
 
 #define MOP(...) MEMOP(, __VA_ARGS__)
@@ -201,6 +218,8 @@ static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
 #define AR(a) ._ar = 1, .ar = (a)
 #define KEY(a) .f_key = 1, .key = (a)
 #define INJECT .f_inject = 1
+#define CMPXCHG_OLD(o) .old = (o)
+#define CMPXCHG_SUCCESS(s) .cmpxchg_success = (s)
 
 #define CHECK_N_DO(f, ...) ({ f(__VA_ARGS__, CHECK_ONLY); f(__VA_ARGS__); })
 
@@ -210,8 +229,8 @@ static int err_memop_ioctl(struct test_info info, struct kvm_s390_mem_op *ksmo)
 #define CR0_FETCH_PROTECTION_OVERRIDE	(1UL << (63 - 38))
 #define CR0_STORAGE_PROTECTION_OVERRIDE	(1UL << (63 - 39))
 
-static uint8_t mem1[65536];
-static uint8_t mem2[65536];
+static uint8_t __aligned(PAGE_SIZE) mem1[65536];
+static uint8_t __aligned(PAGE_SIZE) mem2[65536];
 
 struct test_default {
 	struct kvm_vm *kvm_vm;
@@ -243,6 +262,8 @@ enum stage {
 	STAGE_SKEYS_SET,
 	/* Guest copied memory (locations up to test case) */
 	STAGE_COPIED,
+	/* End of guest code reached */
+	STAGE_DONE,
 };
 
 #define HOST_SYNC(info_p, stage)					\
@@ -254,6 +275,9 @@ enum stage {
 									\
 	vcpu_run(__vcpu);						\
 	get_ucall(__vcpu, &uc);						\
+	if (uc.cmd == UCALL_ABORT) {					\
+		REPORT_GUEST_ASSERT_2(uc, "hints: %lu, %lu");		\
+	}								\
 	ASSERT_EQ(uc.cmd, UCALL_SYNC);					\
 	ASSERT_EQ(uc.args[1], __stage);					\
 })									\
@@ -293,6 +317,44 @@ static void default_read(struct test_info copy_cpu, struct test_info mop_cpu,
 	ASSERT_MEM_EQ(mem1, mem2, size);
 }
 
+static void default_cmpxchg(struct test_default *test, uint8_t key)
+{
+	for (int size = 1; size <= 16; size *= 2) {
+		for (int offset = 0; offset < 16; offset += size) {
+			uint8_t __aligned(16) new[16] = {};
+			uint8_t __aligned(16) old[16];
+			bool succ;
+
+			prepare_mem12();
+			default_write_read(test->vcpu, test->vcpu, LOGICAL, 16, NO_KEY);
+
+			memcpy(&old, mem1, 16);
+			CHECK_N_DO(MOP, test->vm, ABSOLUTE, WRITE, new + offset,
+				   size, GADDR_V(mem1 + offset),
+				   CMPXCHG_OLD(old + offset),
+				   CMPXCHG_SUCCESS(&succ), KEY(key));
+			HOST_SYNC(test->vcpu, STAGE_COPIED);
+			MOP(test->vm, ABSOLUTE, READ, mem2, 16, GADDR_V(mem2));
+			TEST_ASSERT(succ, "exchange of values should succeed");
+			memcpy(mem1 + offset, new + offset, size);
+			ASSERT_MEM_EQ(mem1, mem2, 16);
+
+			memcpy(&old, mem1, 16);
+			new[offset]++;
+			old[offset]++;
+			CHECK_N_DO(MOP, test->vm, ABSOLUTE, WRITE, new + offset,
+				   size, GADDR_V(mem1 + offset),
+				   CMPXCHG_OLD(old + offset),
+				   CMPXCHG_SUCCESS(&succ), KEY(key));
+			HOST_SYNC(test->vcpu, STAGE_COPIED);
+			MOP(test->vm, ABSOLUTE, READ, mem2, 16, GADDR_V(mem2));
+			TEST_ASSERT(!succ, "exchange of values should not succeed");
+			ASSERT_MEM_EQ(mem1, mem2, 16);
+			ASSERT_MEM_EQ(&old, mem1, 16);
+		}
+	}
+}
+
 static void guest_copy(void)
 {
 	GUEST_SYNC(STAGE_INITED);
@@ -377,6 +439,250 @@ static void test_copy_key(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
+static void test_cmpxchg_key(void)
+{
+	struct test_default t = test_default_init(guest_copy_key);
+
+	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
+
+	default_cmpxchg(&t, NO_KEY);
+	default_cmpxchg(&t, 0);
+	default_cmpxchg(&t, 9);
+
+	kvm_vm_free(t.kvm_vm);
+}
+
+static __uint128_t cut_to_size(int size, __uint128_t val)
+{
+	switch (size) {
+	case 1:
+		return (uint8_t)val;
+	case 2:
+		return (uint16_t)val;
+	case 4:
+		return (uint32_t)val;
+	case 8:
+		return (uint64_t)val;
+	case 16:
+		return val;
+	}
+	GUEST_ASSERT_1(false, "Invalid size");
+	return 0;
+}
+
+static bool popcount_eq(__uint128_t a, __uint128_t b)
+{
+	unsigned int count_a, count_b;
+
+	count_a = __builtin_popcountl((uint64_t)(a >> 64)) +
+		  __builtin_popcountl((uint64_t)a);
+	count_b = __builtin_popcountl((uint64_t)(b >> 64)) +
+		  __builtin_popcountl((uint64_t)b);
+	return count_a == count_b;
+}
+
+static __uint128_t rotate(int size, __uint128_t val, int amount)
+{
+	unsigned int bits = size * 8;
+
+	amount = (amount + bits) % bits;
+	val = cut_to_size(size, val);
+	return (val << (bits - amount)) | (val >> amount);
+}
+
+const unsigned int max_block = 16;
+
+static void choose_block(bool guest, int i, int *size, int *offset)
+{
+	unsigned int rand;
+
+	rand = i;
+	if (guest) {
+		rand = rand * 19 + 11;
+		*size = 1 << ((rand % 3) + 2);
+		rand = rand * 19 + 11;
+		*offset = (rand % max_block) & ~(*size - 1);
+	} else {
+		rand = rand * 17 + 5;
+		*size = 1 << (rand % 5);
+		rand = rand * 17 + 5;
+		*offset = (rand % max_block) & ~(*size - 1);
+	}
+}
+
+static __uint128_t permutate_bits(bool guest, int i, int size, __uint128_t old)
+{
+	unsigned int rand;
+	bool swap;
+
+	rand = i;
+	rand = rand * 3 + 1;
+	if (guest)
+		rand = rand * 3 + 1;
+	swap = rand % 2 == 0;
+	if (swap) {
+		int i, j;
+		__uint128_t new;
+		uint8_t byte0, byte1;
+
+		rand = rand * 3 + 1;
+		i = rand % size;
+		rand = rand * 3 + 1;
+		j = rand % size;
+		if (i == j)
+			return old;
+		new = rotate(16, old, i * 8);
+		byte0 = new & 0xff;
+		new &= ~0xff;
+		new = rotate(16, new, -i * 8);
+		new = rotate(16, new, j * 8);
+		byte1 = new & 0xff;
+		new = (new & ~0xff) | byte0;
+		new = rotate(16, new, -j * 8);
+		new = rotate(16, new, i * 8);
+		new = new | byte1;
+		new = rotate(16, new, -i * 8);
+		return new;
+	} else {
+		int amount;
+
+		rand = rand * 3 + 1;
+		amount = rand % (size * 8);
+		return rotate(size, old, amount);
+	}
+}
+
+static bool _cmpxchg(int size, void *target, __uint128_t *old_p, __uint128_t new)
+{
+	bool ret;
+
+	switch (size) {
+	case 4: {
+			uint32_t old = *old_p;
+
+			asm volatile ("cs %[old],%[new],%[address]"
+			    : [old] "+d" (old),
+			      [address] "+Q" (*(uint32_t *)(target))
+			    : [new] "d" ((uint32_t)new)
+			    : "cc"
+			);
+			ret = old == (uint32_t)*old_p;
+			*old_p = old;
+			return ret;
+		}
+	case 8: {
+			uint64_t old = *old_p;
+
+			asm volatile ("csg %[old],%[new],%[address]"
+			    : [old] "+d" (old),
+			      [address] "+Q" (*(uint64_t *)(target))
+			    : [new] "d" ((uint64_t)new)
+			    : "cc"
+			);
+			ret = old == (uint64_t)*old_p;
+			*old_p = old;
+			return ret;
+		}
+	case 16: {
+			__uint128_t old = *old_p;
+
+			asm volatile ("cdsg %[old],%[new],%[address]"
+			    : [old] "+d" (old),
+			      [address] "+Q" (*(__uint128_t *)(target))
+			    : [new] "d" (new)
+			    : "cc"
+			);
+			ret = old == *old_p;
+			*old_p = old;
+			return ret;
+		}
+	}
+	GUEST_ASSERT_1(false, "Invalid size");
+	return 0;
+}
+
+const unsigned int cmpxchg_iter_outer = 100, cmpxchg_iter_inner = 10000;
+
+static void guest_cmpxchg_key(void)
+{
+	int size, offset;
+	__uint128_t old, new;
+
+	set_storage_key_range(mem1, max_block, 0x10);
+	set_storage_key_range(mem2, max_block, 0x10);
+	GUEST_SYNC(STAGE_SKEYS_SET);
+
+	for (int i = 0; i < cmpxchg_iter_outer; i++) {
+		do {
+			old = 1;
+		} while (!_cmpxchg(16, mem1, &old, 0));
+		for (int j = 0; j < cmpxchg_iter_inner; j++) {
+			choose_block(true, i + j, &size, &offset);
+			do {
+				new = permutate_bits(true, i + j, size, old);
+			} while (!_cmpxchg(size, mem2 + offset, &old, new));
+		}
+	}
+
+	GUEST_SYNC(STAGE_DONE);
+}
+
+static void *run_guest(void *data)
+{
+	struct test_info *info = data;
+
+	HOST_SYNC(*info, STAGE_DONE);
+	return NULL;
+}
+
+static char *quad_to_char(__uint128_t *quad, int size)
+{
+	return ((char *)quad) + (sizeof(*quad) - size);
+}
+
+static void test_cmpxchg_key_concurrent(void)
+{
+	struct test_default t = test_default_init(guest_cmpxchg_key);
+	int size, offset;
+	__uint128_t old, new;
+	bool success;
+	pthread_t thread;
+
+	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
+	prepare_mem12();
+	MOP(t.vcpu, LOGICAL, WRITE, mem1, max_block, GADDR_V(mem2));
+	pthread_create(&thread, NULL, run_guest, &t.vcpu);
+
+	for (int i = 0; i < cmpxchg_iter_outer; i++) {
+		do {
+			old = 0;
+			new = 1;
+			MOP(t.vm, ABSOLUTE, WRITE, &new,
+			    sizeof(new), GADDR_V(mem1),
+			    CMPXCHG_OLD(&old),
+			    CMPXCHG_SUCCESS(&success), KEY(1));
+		} while (!success);
+		for (int j = 0; j < cmpxchg_iter_inner; j++) {
+			choose_block(false, i + j, &size, &offset);
+			do {
+				new = permutate_bits(false, i + j, size, old);
+				MOP(t.vm, ABSOLUTE, WRITE, quad_to_char(&new, size),
+				    size, GADDR_V(mem2 + offset),
+				    CMPXCHG_OLD(quad_to_char(&old, size)),
+				    CMPXCHG_SUCCESS(&success), KEY(1));
+			} while (!success);
+		}
+	}
+
+	pthread_join(thread, NULL);
+
+	MOP(t.vcpu, LOGICAL, READ, mem2, max_block, GADDR_V(mem2));
+	TEST_ASSERT(popcount_eq(*(__uint128_t *)mem1, *(__uint128_t *)mem2),
+		    "Must retain number of set bits");
+
+	kvm_vm_free(t.kvm_vm);
+}
+
 static void guest_copy_key_fetch_prot(void)
 {
 	/*
@@ -457,6 +763,24 @@ static void test_errors_key(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
+static void test_errors_cmpxchg_key(void)
+{
+	struct test_default t = test_default_init(guest_copy_key_fetch_prot);
+	int i;
+
+	HOST_SYNC(t.vcpu, STAGE_INITED);
+	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
+
+	for (i = 1; i <= 16; i *= 2) {
+		__uint128_t old = 0;
+
+		CHECK_N_DO(ERR_PROT_MOP, t.vm, ABSOLUTE, WRITE, mem2, i, GADDR_V(mem2),
+			   CMPXCHG_OLD(&old), KEY(2));
+	}
+
+	kvm_vm_free(t.kvm_vm);
+}
+
 static void test_termination(void)
 {
 	struct test_default t = test_default_init(guest_error_key);
@@ -690,6 +1014,38 @@ static void test_errors(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
+static void test_errors_cmpxchg(void)
+{
+	struct test_default t = test_default_init(guest_idle);
+	__uint128_t old;
+	int rv, i, power = 1;
+
+	HOST_SYNC(t.vcpu, STAGE_INITED);
+
+	for (i = 0; i < 32; i++) {
+		if (i == power) {
+			power *= 2;
+			continue;
+		}
+		rv = ERR_MOP(t.vm, ABSOLUTE, WRITE, mem1, i, GADDR_V(mem1),
+			     CMPXCHG_OLD(&old));
+		TEST_ASSERT(rv == -1 && errno == EINVAL,
+			    "ioctl allows bad size for cmpxchg");
+	}
+	for (i = 1; i <= 16; i *= 2) {
+		rv = ERR_MOP(t.vm, ABSOLUTE, WRITE, mem1, i, GADDR((void *)~0xfffUL),
+			     CMPXCHG_OLD(&old));
+		TEST_ASSERT(rv > 0, "ioctl allows bad guest address for cmpxchg");
+	}
+	for (i = 2; i <= 16; i *= 2) {
+		rv = ERR_MOP(t.vm, ABSOLUTE, WRITE, mem1, i, GADDR_V(mem1 + 1),
+			     CMPXCHG_OLD(&old));
+		TEST_ASSERT(rv == -1 && errno == EINVAL,
+			    "ioctl allows bad alignment for cmpxchg");
+	}
+
+	kvm_vm_free(t.kvm_vm);
+}
 
 int main(int argc, char *argv[])
 {
@@ -719,6 +1075,16 @@ int main(int argc, char *argv[])
 			.test = test_copy_key,
 			.requirements_met = extension_cap > 0,
 		},
+		{
+			.name = "cmpxchg with storage keys",
+			.test = test_cmpxchg_key,
+			.requirements_met = extension_cap & 0x2,
+		},
+		{
+			.name = "concurrently cmpxchg with storage keys",
+			.test = test_cmpxchg_key_concurrent,
+			.requirements_met = extension_cap & 0x2,
+		},
 		{
 			.name = "copy with key storage protection override",
 			.test = test_copy_key_storage_prot_override,
@@ -739,6 +1105,16 @@ int main(int argc, char *argv[])
 			.test = test_errors_key,
 			.requirements_met = extension_cap > 0,
 		},
+		{
+			.name = "error checks for cmpxchg with key",
+			.test = test_errors_cmpxchg_key,
+			.requirements_met = extension_cap & 0x2,
+		},
+		{
+			.name = "error checks for cmpxchg",
+			.test = test_errors_cmpxchg,
+			.requirements_met = extension_cap & 0x2,
+		},
 		{
 			.name = "termination",
 			.test = test_termination,
-- 
2.34.1

[PATCH v3 7/9] KVM: s390: selftest: memop: Add bad address test

[Janis Schoetterl-Glausch]

Add test that tries to access, instead of CHECK_ONLY.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 269b181731c8..13eeb50c036b 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -965,7 +965,9 @@ static void _test_errors_common(struct test_info info, enum mop_target target, i
 
 	/* Bad guest address: */
 	rv = ERR_MOP(info, target, WRITE, mem1, size, GADDR((void *)~0xfffUL), CHECK_ONLY);
-	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory access");
+	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address");
+	rv = ERR_MOP(info, target, WRITE, mem1, size, GADDR((void *)~0xfffUL));
+	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address");
 
 	/* Bad host address: */
 	rv = ERR_MOP(info, target, WRITE, 0, size, GADDR_V(mem1));
-- 
2.34.1

[PATCH v3 8/9] KVM: s390: selftest: memop: Fix typo

[Janis Schoetterl-Glausch]

"acceeded" isn't a word, should be "exceeded".

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 13eeb50c036b..a4dc8d93d672 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -926,7 +926,7 @@ static void test_errors_key_fetch_prot_override_enabled(void)
 
 	/*
 	 * vcpu, mismatching keys on fetch,
-	 * fetch protection override does not apply because memory range acceeded
+	 * fetch protection override does not apply because memory range exceeded
 	 */
 	CHECK_N_DO(ERR_PROT_MOP, t.vcpu, LOGICAL, READ, mem2, 2048 + 1, GADDR_V(0), KEY(2));
 	CHECK_N_DO(ERR_PROT_MOP, t.vcpu, LOGICAL, READ, mem2, PAGE_SIZE + 2048 + 1,
-- 
2.34.1

[PATCH v3 9/9] KVM: s390: selftest: memop: Fix wrong address being used in test

[Janis Schoetterl-Glausch]

The guest code sets the key for mem1 only. In order to provoke a
protection exception the test codes needs to address mem1.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index a4dc8d93d672..50a20dc74eb9 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -756,9 +756,9 @@ static void test_errors_key(void)
 
 	/* vm/vcpu, mismatching keys, fetch protection in effect */
 	CHECK_N_DO(ERR_PROT_MOP, t.vcpu, LOGICAL, WRITE, mem1, t.size, GADDR_V(mem1), KEY(2));
-	CHECK_N_DO(ERR_PROT_MOP, t.vcpu, LOGICAL, READ, mem2, t.size, GADDR_V(mem2), KEY(2));
+	CHECK_N_DO(ERR_PROT_MOP, t.vcpu, LOGICAL, READ, mem2, t.size, GADDR_V(mem1), KEY(2));
 	CHECK_N_DO(ERR_PROT_MOP, t.vm, ABSOLUTE, WRITE, mem1, t.size, GADDR_V(mem1), KEY(2));
-	CHECK_N_DO(ERR_PROT_MOP, t.vm, ABSOLUTE, READ, mem2, t.size, GADDR_V(mem2), KEY(2));
+	CHECK_N_DO(ERR_PROT_MOP, t.vm, ABSOLUTE, READ, mem2, t.size, GADDR_V(mem1), KEY(2));
 
 	kvm_vm_free(t.kvm_vm);
 }
-- 
2.34.1

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Bagas Sanjaya]

On 11/18/22 05:17, Janis Schoetterl-Glausch wrote:
> +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
> +In this case, instead of doing an unconditional write, the access occurs only
> +if the target location contains the "size" byte long value pointed to by
> +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two
> +up to and including 16.
> +The value at the target location is written to the location "old_p" points to.
> +If the exchange did not take place because the target value doesn't match the
> +old value KVM_S390_MEMOP_R_NO_XCHG is returned.
> +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
> +has bit 1 (i.e. bit with value 2) set.
>  

Is KVM_S390_MEMOP_F_CMPXCHG supported with conditions (as you implied)?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Heiko Carstens]

On Thu, Nov 17, 2022 at 11:17:50PM +0100, Janis Schoetterl-Glausch wrote:
> User space can use the MEM_OP ioctl to make storage key checked reads
> and writes to the guest, however, it has no way of performing atomic,
> key checked, accesses to the guest.
> Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
> mode. For now, support this mode for absolute accesses only.
> 
> This mode can be use, for example, to set the device-state-change
> indicator and the adapter-local-summary indicator atomically.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  include/uapi/linux/kvm.h |   5 ++
>  arch/s390/kvm/gaccess.h  |   3 ++
>  arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
>  4 files changed, 142 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 0d5d4419139a..1f36be5493e6 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
>  		struct {
>  			__u8 ar;	/* the access register number */
>  			__u8 key;	/* access key, ignored if flag unset */
> +			__u8 pad1[6];	/* ignored */
> +			__u64 old_p;	/* ignored if flag unset */

Just one comment: the suffix "_p" for pointer is quite unusual within
the kernel. This also would be the first of its kind within kvm.h.
Usually there is either no suffix or "_addr".
So for consistency reasons I would suggest to change this to one of
the common variants.

The code itself looks good from my point of view, even though for the
sake of simplicity I would have put the complete sign/zero extended
128 bit old value into the structure, instead of having a pointer to
the value. Imho that would simplify the interface. Also alignment, as
pointed out previously, really doesn't matter for this use case.

But you had already something like that previously and changed it, so
no reason to go back and forth. Not really important.

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Thomas Huth]

On 18/11/2022 11.12, Heiko Carstens wrote:
> On Thu, Nov 17, 2022 at 11:17:50PM +0100, Janis Schoetterl-Glausch wrote:
>> User space can use the MEM_OP ioctl to make storage key checked reads
>> and writes to the guest, however, it has no way of performing atomic,
>> key checked, accesses to the guest.
>> Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
>> mode. For now, support this mode for absolute accesses only.
>>
>> This mode can be use, for example, to set the device-state-change
>> indicator and the adapter-local-summary indicator atomically.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   include/uapi/linux/kvm.h |   5 ++
>>   arch/s390/kvm/gaccess.h  |   3 ++
>>   arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
>>   arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
>>   4 files changed, 142 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 0d5d4419139a..1f36be5493e6 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
>>   		struct {
>>   			__u8 ar;	/* the access register number */
>>   			__u8 key;	/* access key, ignored if flag unset */
>> +			__u8 pad1[6];	/* ignored */
>> +			__u64 old_p;	/* ignored if flag unset */
> 
> Just one comment: the suffix "_p" for pointer is quite unusual within
> the kernel. This also would be the first of its kind within kvm.h.
> Usually there is either no suffix or "_addr".
> So for consistency reasons I would suggest to change this to one of
> the common variants.
> 
> The code itself looks good from my point of view, even though for the
> sake of simplicity I would have put the complete sign/zero extended
> 128 bit old value into the structure, instead of having a pointer to
> the value.

See 
https://lore.kernel.org/kvm/37197cfe-d109-332f-089b-266d7e8e23f8@redhat.com/ 
... it would break the "IOW" definition of the ioctl. It can be done, but 
that confuses tools like valgrind, as far as I know. So I think the idea 
with the pointer is better in this case.

  Thomas

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Heiko Carstens]

On Fri, Nov 18, 2022 at 03:37:26PM +0100, Thomas Huth wrote:
> > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > index 0d5d4419139a..1f36be5493e6 100644
> > > --- a/include/uapi/linux/kvm.h
> > > +++ b/include/uapi/linux/kvm.h
> > > @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
> > >   		struct {
> > >   			__u8 ar;	/* the access register number */
> > >   			__u8 key;	/* access key, ignored if flag unset */
> > > +			__u8 pad1[6];	/* ignored */
> > > +			__u64 old_p;	/* ignored if flag unset */
> > 
> > Just one comment: the suffix "_p" for pointer is quite unusual within
> > the kernel. This also would be the first of its kind within kvm.h.
> > Usually there is either no suffix or "_addr".
> > So for consistency reasons I would suggest to change this to one of
> > the common variants.
> > 
> > The code itself looks good from my point of view, even though for the
> > sake of simplicity I would have put the complete sign/zero extended
> > 128 bit old value into the structure, instead of having a pointer to
> > the value.
> 
> See
> https://lore.kernel.org/kvm/37197cfe-d109-332f-089b-266d7e8e23f8@redhat.com/
> ... it would break the "IOW" definition of the ioctl. It can be done, but
> that confuses tools like valgrind, as far as I know. So I think the idea
> with the pointer is better in this case.

Ah right, I forgot about that. Then let's do it this way.

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch]

On Fri, 2022-11-18 at 11:12 +0100, Heiko Carstens wrote:
> On Thu, Nov 17, 2022 at 11:17:50PM +0100, Janis Schoetterl-Glausch wrote:
> > User space can use the MEM_OP ioctl to make storage key checked reads
> > and writes to the guest, however, it has no way of performing atomic,
> > key checked, accesses to the guest.
> > Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
> > mode. For now, support this mode for absolute accesses only.
> > 
> > This mode can be use, for example, to set the device-state-change
> > indicator and the adapter-local-summary indicator atomically.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >  include/uapi/linux/kvm.h |   5 ++
> >  arch/s390/kvm/gaccess.h  |   3 ++
> >  arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
> >  arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
> >  4 files changed, 142 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 0d5d4419139a..1f36be5493e6 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
> >  		struct {
> >  			__u8 ar;	/* the access register number */
> >  			__u8 key;	/* access key, ignored if flag unset */
> > +			__u8 pad1[6];	/* ignored */
> > +			__u64 old_p;	/* ignored if flag unset */
> 
> Just one comment: the suffix "_p" for pointer is quite unusual within
> the kernel. This also would be the first of its kind within kvm.h.
> Usually there is either no suffix or "_addr".
> So for consistency reasons I would suggest to change this to one of
> the common variants.
> 
Thanks, good point.

[...]

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Janis Schoetterl-Glausch]

On Fri, 2022-11-18 at 08:50 +0700, Bagas Sanjaya wrote:
> On 11/18/22 05:17, Janis Schoetterl-Glausch wrote:
> > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
> > +In this case, instead of doing an unconditional write, the access occurs only
> > +if the target location contains the "size" byte long value pointed to by
> > +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two
> > +up to and including 16.
> > +The value at the target location is written to the location "old_p" points to.
> > +If the exchange did not take place because the target value doesn't match the
> > +old value KVM_S390_MEMOP_R_NO_XCHG is returned.
> > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
> > +has bit 1 (i.e. bit with value 2) set.
> >  
> 
> Is KVM_S390_MEMOP_F_CMPXCHG supported with conditions (as you implied)?
> 
I'm afraid I don't quite understand the question.
It is supported if the capability says it is, i.e. if bit 1 is set.

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Thomas Huth]

On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote:
> Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for
> absolute vm write memops which allows user space to perform (storage key
> checked) cmpxchg operations on guest memory.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
...
>   Supported flags:
>     * ``KVM_S390_MEMOP_F_CHECK_ONLY``
>     * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> +  * ``KVM_S390_MEMOP_F_CMPXCHG``
> +
> +The semantics of the flags common with logical acesses are as for logical
> +accesses.
> +
> +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.

I'd maybe merge this with the last sentence:

For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if 
KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set.

... and speaking of that, I wonder whether it's maybe a good idea to 
introduce some #defines for bit 1 / value 2, to avoid the confusion ?

> +In this case, instead of doing an unconditional write, the access occurs only
> +if the target location contains the "size" byte long value pointed to by
> +"old_p". This is performed as an atomic cmpxchg.

I had to read the first sentence twice to understand it ... maybe it's 
easier to understand if you move the "size" part to the second sentence:

In this case, instead of doing an unconditional write, the access occurs 
only if the target location contains value pointed to by "old_p". This is 
performed as an atomic cmpxchg with the length specified by the "size" 
parameter.

?

> "size" must be a power of two
> +up to and including 16.
> +The value at the target location is written to the location "old_p" points to.

IMHO something like this would be better:

The value at the target location is replaced with the value from the 
location that "old_p" points to.

> +If the exchange did not take place because the target value doesn't match the
> +old value KVM_S390_MEMOP_R_NO_XCHG is returned.
> +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
> +has bit 1 (i.e. bit with value 2) set.

  Thomas

PS: Please take my suggestions with a grain of salt ... I'm not a native 
speaker either.

Re: [PATCH v3 5/9] KVM: s390: selftest: memop: Move testlist into main

[Thomas Huth]

On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote:
> This allows checking if the necessary requirements for a test case are
> met via an arbitrary expression. In particular, it is easy to check if
> certain bits are set in the memop extension capability.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   tools/testing/selftests/kvm/s390x/memop.c | 132 +++++++++++-----------
>   1 file changed, 66 insertions(+), 66 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index 286185a59238..10f34c629cac 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -690,87 +690,87 @@ static void test_errors(void)
>   	kvm_vm_free(t.kvm_vm);
>   }
>   
> -struct testdef {
> -	const char *name;
> -	void (*test)(void);
> -	int extension;
> -} testlist[] = {
> -	{
> -		.name = "simple copy",
> -		.test = test_copy,
> -	},
> -	{
> -		.name = "generic error checks",
> -		.test = test_errors,
> -	},
> -	{
> -		.name = "copy with storage keys",
> -		.test = test_copy_key,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "copy with key storage protection override",
> -		.test = test_copy_key_storage_prot_override,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "copy with key fetch protection",
> -		.test = test_copy_key_fetch_prot,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "copy with key fetch protection override",
> -		.test = test_copy_key_fetch_prot_override,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "error checks with key",
> -		.test = test_errors_key,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "termination",
> -		.test = test_termination,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "error checks with key storage protection override",
> -		.test = test_errors_key_storage_prot_override,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "error checks without key fetch prot override",
> -		.test = test_errors_key_fetch_prot_override_not_enabled,
> -		.extension = 1,
> -	},
> -	{
> -		.name = "error checks with key fetch prot override",
> -		.test = test_errors_key_fetch_prot_override_enabled,
> -		.extension = 1,
> -	},
> -};
>   
>   int main(int argc, char *argv[])
>   {
>   	int extension_cap, idx;
>   
> +	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
>   	TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_MEM_OP));
> +	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
>   
> -	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
> +	struct testdef {
> +		const char *name;
> +		void (*test)(void);
> +		bool requirements_met;
> +	} testlist[] = {
> +		{
> +			.name = "simple copy",
> +			.test = test_copy,
> +			.requirements_met = true,
> +		},
> +		{
> +			.name = "generic error checks",
> +			.test = test_errors,
> +			.requirements_met = true,
> +		},
> +		{
> +			.name = "copy with storage keys",
> +			.test = test_copy_key,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "copy with key storage protection override",
> +			.test = test_copy_key_storage_prot_override,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "copy with key fetch protection",
> +			.test = test_copy_key_fetch_prot,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "copy with key fetch protection override",
> +			.test = test_copy_key_fetch_prot_override,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "error checks with key",
> +			.test = test_errors_key,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "termination",
> +			.test = test_termination,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "error checks with key storage protection override",
> +			.test = test_errors_key_storage_prot_override,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "error checks without key fetch prot override",
> +			.test = test_errors_key_fetch_prot_override_not_enabled,
> +			.requirements_met = extension_cap > 0,
> +		},
> +		{
> +			.name = "error checks with key fetch prot override",
> +			.test = test_errors_key_fetch_prot_override_enabled,
> +			.requirements_met = extension_cap > 0,

I wonder whether it would rather make sense to check for "extension_cap & 1" 
instead of "extension_cap > 0" ?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 5/9] KVM: s390: selftest: memop: Move testlist into main

[Janis Schoetterl-Glausch]

On Tue, 2022-11-22 at 08:52 +0100, Thomas Huth wrote:
> On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote:
> > This allows checking if the necessary requirements for a test case are
> > met via an arbitrary expression. In particular, it is easy to check if
> > certain bits are set in the memop extension capability.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >   tools/testing/selftests/kvm/s390x/memop.c | 132 +++++++++++-----------
> >   1 file changed, 66 insertions(+), 66 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> > index 286185a59238..10f34c629cac 100644
> > --- a/tools/testing/selftests/kvm/s390x/memop.c
> > +++ b/tools/testing/selftests/kvm/s390x/memop.c
> > @@ -690,87 +690,87 @@ static void test_errors(void)
> >   	kvm_vm_free(t.kvm_vm);
> >   }
> >   
[...]
> > 
> > +	} testlist[] = {
> > +		{
> > +			.name = "simple copy",
> > +			.test = test_copy,
> > +			.requirements_met = true,
> > +		},
> > +		{
> > +			.name = "generic error checks",
> > +			.test = test_errors,
> > +			.requirements_met = true,
> > +		},
> > +		{
> > +			.name = "copy with storage keys",
> > +			.test = test_copy_key,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "copy with key storage protection override",
> > +			.test = test_copy_key_storage_prot_override,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "copy with key fetch protection",
> > +			.test = test_copy_key_fetch_prot,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "copy with key fetch protection override",
> > +			.test = test_copy_key_fetch_prot_override,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "error checks with key",
> > +			.test = test_errors_key,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "termination",
> > +			.test = test_termination,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "error checks with key storage protection override",
> > +			.test = test_errors_key_storage_prot_override,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "error checks without key fetch prot override",
> > +			.test = test_errors_key_fetch_prot_override_not_enabled,
> > +			.requirements_met = extension_cap > 0,
> > +		},
> > +		{
> > +			.name = "error checks with key fetch prot override",
> > +			.test = test_errors_key_fetch_prot_override_enabled,
> > +			.requirements_met = extension_cap > 0,
> 
> I wonder whether it would rather make sense to check for "extension_cap & 1" 
> instead of "extension_cap > 0" ?

The cap should always have been a bitmap, but unfortunately I didn't initially
define it as one, the storage key extension must be supported if the cap > 0.
So the test reflects that and may catch an error in the future.
> 
> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
Thanks!

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Janis Schoetterl-Glausch]

On Tue, 2022-11-22 at 08:47 +0100, Thomas Huth wrote:
> On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote:
> > Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for
> > absolute vm write memops which allows user space to perform (storage key
> > checked) cmpxchg operations on guest memory.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> ...
> >   Supported flags:
> >     * ``KVM_S390_MEMOP_F_CHECK_ONLY``
> >     * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> > +  * ``KVM_S390_MEMOP_F_CMPXCHG``
> > +
> > +The semantics of the flags common with logical acesses are as for logical
> > +accesses.
> > +
> > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
> 
> I'd maybe merge this with the last sentence:
> 
> For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if 
> KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set.

Ok.
> 
> ... and speaking of that, I wonder whether it's maybe a good idea to 
> introduce some #defines for bit 1 / value 2, to avoid the confusion ?

Not sure, I don't feel it's too complicated. Where would you define it?
Next to the mem_op struct? KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG?
> 
> > +In this case, instead of doing an unconditional write, the access occurs only
> > +if the target location contains the "size" byte long value pointed to by
> > +"old_p". This is performed as an atomic cmpxchg.
> 
> I had to read the first sentence twice to understand it ... maybe it's 
> easier to understand if you move the "size" part to the second sentence:
> 
> In this case, instead of doing an unconditional write, the access occurs 
> only if the target location contains value pointed to by "old_p". This is 
> performed as an atomic cmpxchg with the length specified by the "size" 
> parameter.
> 
> ?

Ok.
> 
> > "size" must be a power of two
> > +up to and including 16.
> > +The value at the target location is written to the location "old_p" points to.
> 
> IMHO something like this would be better:
> 
> The value at the target location is replaced with the value from the 
> location that "old_p" points to.

I'm trying to say the opposite :).
I went with this:

If the exchange did not take place because the target value doesn't match the
old value, KVM_S390_MEMOP_R_NO_XCHG is returned.
In this case the value "old_addr" points to is replaced by the target value.
> 
> > +If the exchange did not take place because the target value doesn't match the
> > +old value KVM_S390_MEMOP_R_NO_XCHG is returned.
> > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
> > +has bit 1 (i.e. bit with value 2) set.
> 
>   Thomas
> 
> PS: Please take my suggestions with a grain of salt ... I'm not a native 
> speaker either.
> 

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Nico Boehr]

Quoting Janis Schoetterl-Glausch (2022-11-22 14:10:41)
> On Tue, 2022-11-22 at 08:47 +0100, Thomas Huth wrote:
> > On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote:
[...]
> > >   Supported flags:
> > >     * ``KVM_S390_MEMOP_F_CHECK_ONLY``
> > >     * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> > > +  * ``KVM_S390_MEMOP_F_CMPXCHG``
> > > +
> > > +The semantics of the flags common with logical acesses are as for logical
> > > +accesses.
> > > +
> > > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
> > 
> > I'd maybe merge this with the last sentence:
> > 
> > For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if 
> > KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set.
> 
> Ok.
> > 
> > ... and speaking of that, I wonder whether it's maybe a good idea to 
> > introduce some #defines for bit 1 / value 2, to avoid the confusion ?
> 
> Not sure, I don't feel it's too complicated. Where would you define it?
> Next to the mem_op struct? KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG?

I think the define would be a good idea. Location and name sound good to me.

You could also replace the hard-coded 0x3 in kvm_vm_ioctl_check_extension() when you have the define.

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Claudio Imbrenda]

On Thu, 17 Nov 2022 23:17:50 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> User space can use the MEM_OP ioctl to make storage key checked reads
> and writes to the guest, however, it has no way of performing atomic,
> key checked, accesses to the guest.
> Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
> mode. For now, support this mode for absolute accesses only.
> 
> This mode can be use, for example, to set the device-state-change
> indicator and the adapter-local-summary indicator atomically.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  include/uapi/linux/kvm.h |   5 ++
>  arch/s390/kvm/gaccess.h  |   3 ++
>  arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
>  4 files changed, 142 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 0d5d4419139a..1f36be5493e6 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
>  		struct {
>  			__u8 ar;	/* the access register number */
>  			__u8 key;	/* access key, ignored if flag unset */
> +			__u8 pad1[6];	/* ignored */
> +			__u64 old_p;	/* ignored if flag unset */
>  		};
>  		__u32 sida_offset; /* offset into the sida */
>  		__u8 reserved[32]; /* ignored */
> @@ -604,6 +606,9 @@ struct kvm_s390_mem_op {
>  #define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
>  #define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)
>  #define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
> +#define KVM_S390_MEMOP_F_CMPXCHG		(1ULL << 3)
> +/* Non program exception return codes (pgm codes are 16 bit) */
> +#define KVM_S390_MEMOP_R_NO_XCHG		((1 << 16) + 0)

are you planning to have further *_R_* macros in the near future?
if not, remove the + 0
if yes, move the (1 << 16) to a macro, so it becomes something like
(KVM_S390_MEMOP_R_BASE + 0)

(maybe you can find a better/shorter name)

>  
>  /* for KVM_INTERRUPT */
>  struct kvm_interrupt {
> diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> index 9408d6cc8e2c..92a3b9fb31ec 100644
> --- a/arch/s390/kvm/gaccess.h
> +++ b/arch/s390/kvm/gaccess.h
> @@ -206,6 +206,9 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>  int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
>  		      void *data, unsigned long len, enum gacc_mode mode);
>  
> +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> +			       __uint128_t *old, __uint128_t new, u8 access_key);
> +
>  /**
>   * write_guest_with_key - copy data from kernel space to guest space
>   * @vcpu: virtual cpu
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 0243b6e38d36..be042865d8a1 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1161,6 +1161,107 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
>  	return rc;
>  }
>  
> +/**
> + * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
> + * @kvm: Virtual machine instance.
> + * @gpa: Absolute guest address of the location to be changed.
> + * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
> + *       non power of two will result in failure.
> + * @old_p: Pointer to old value. If the location at @gpa contains this value, the
> + *         exchange will succeed. After calling cmpxchg_guest_abs_with_key() *@old
> + *         contains the value at @gpa before the attempt to exchange the value.
> + * @new: The value to place at @gpa.
> + * @access_key: The access key to use for the guest access.
> + *
> + * Atomically exchange the value at @gpa by @new, if it contains *@old.
> + * Honors storage keys.
> + *
> + * Return: * 0: successful exchange
> + *         * 1: exchange unsuccessful
> + *         * a program interruption code indicating the reason cmpxchg could
> + *           not be attempted
> + *         * -EINVAL: address misaligned or len not power of two
> + *         * -EAGAIN: transient failure (len 1 or 2)

please also document -EOPNOTSUPP

> + */
> +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> +			       __uint128_t *old_p, __uint128_t new,
> +			       u8 access_key)
> +{
> +	gfn_t gfn = gpa >> PAGE_SHIFT;
> +	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);

exchange the above two lines (reverse christmas tree)

> +	bool writable;
> +	hva_t hva;
> +	int ret;
> +
> +	if (!IS_ALIGNED(gpa, len))
> +		return -EINVAL;
> +
> +	hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
> +	if (kvm_is_error_hva(hva))
> +		return PGM_ADDRESSING;
> +	/*
> +	 * Check if it's a read-only memslot, even though that cannot occur
> +	 * since those are unsupported.
> +	 * Don't try to actually handle that case.
> +	 */
> +	if (!writable)
> +		return -EOPNOTSUPP;

either you document this, or you return something else (like -EINVAL)

> +
> +	hva += offset_in_page(gpa);
> +	switch (len) {
> +	case 1: {
> +		u8 old;
> +
> +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_p, new, access_key);
> +		ret = ret < 0 ? ret : old != *old_p;
> +		*old_p = old;
> +		break;
> +	}
> +	case 2: {
> +		u16 old;
> +
> +		ret = cmpxchg_user_key((u16 *)hva, &old, *old_p, new, access_key);
> +		ret = ret < 0 ? ret : old != *old_p;
> +		*old_p = old;
> +		break;
> +	}
> +	case 4: {
> +		u32 old;
> +
> +		ret = cmpxchg_user_key((u32 *)hva, &old, *old_p, new, access_key);
> +		ret = ret < 0 ? ret : old != *old_p;
> +		*old_p = old;
> +		break;
> +	}
> +	case 8: {
> +		u64 old;
> +
> +		ret = cmpxchg_user_key((u64 *)hva, &old, *old_p, new, access_key);
> +		ret = ret < 0 ? ret : old != *old_p;
> +		*old_p = old;
> +		break;
> +	}
> +	case 16: {
> +		__uint128_t old;
> +
> +		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_p, new, access_key);
> +		ret = ret < 0 ? ret : old != *old_p;
> +		*old_p = old;
> +		break;

I really dislike repeating the same code 5 times, but I guess there was
no other way?

> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +	mark_page_dirty_in_slot(kvm, slot, gfn);
> +	/*
> +	 * Assume that the fault is caused by protection, either key protection
> +	 * or user page write protection.
> +	 */
> +	if (ret == -EFAULT)
> +		ret = PGM_PROTECTION;
> +	return ret;
> +}
> +
>  /**
>   * guest_translate_address_with_key - translate guest logical into guest absolute address
>   * @vcpu: virtual cpu
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 45d4b8182b07..2410b4044283 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -576,7 +576,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_VCPU_RESETS:
>  	case KVM_CAP_SET_GUEST_DEBUG:
>  	case KVM_CAP_S390_DIAG318:
> -	case KVM_CAP_S390_MEM_OP_EXTENSION:
>  		r = 1;
>  		break;
>  	case KVM_CAP_SET_GUEST_DEBUG2:
> @@ -590,6 +589,14 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_MEM_OP:
>  		r = MEM_OP_MAX_SIZE;
>  		break;
> +	case KVM_CAP_S390_MEM_OP_EXTENSION:
> +		/*
> +		 * Flag bits indicating which extensions are supported.
> +		 * The first extension doesn't use a flag, but pretend it does,
> +		 * this way that can be changed in the future.
> +		 */
> +		r = 0x3;
> +		break;
>  	case KVM_CAP_NR_VCPUS:
>  	case KVM_CAP_MAX_VCPUS:
>  	case KVM_CAP_MAX_VCPU_ID:
> @@ -2714,12 +2721,19 @@ static bool access_key_invalid(u8 access_key)
>  static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>  {
>  	void __user *uaddr = (void __user *)mop->buf;
> +	void __user *old_p = (void __user *)mop->old_p;
> +	union {
> +		__uint128_t quad;
> +		char raw[sizeof(__uint128_t)];
> +	} old = { .quad = 0}, new = { .quad = 0 };
> +	unsigned int off_in_quad = sizeof(__uint128_t) - mop->size;
>  	u64 supported_flags;
>  	void *tmpbuf = NULL;
>  	int r, srcu_idx;
>  
>  	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
> +			  | KVM_S390_MEMOP_F_CHECK_ONLY
> +			  | KVM_S390_MEMOP_F_CMPXCHG;
>  	if (mop->flags & ~supported_flags || !mop->size)
>  		return -EINVAL;
>  	if (mop->size > MEM_OP_MAX_SIZE)
> @@ -2741,6 +2755,15 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>  	} else {
>  		mop->key = 0;
>  	}
> +	if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {

add a quick comment here to explain that this check validates
off_in_quad, and that it does not do a full validation of mop->size,
which will happen in cmpxchg_guest_abs_with_key.

> +		if (mop->size > sizeof(new))
> +			return -EINVAL;
> +		/* off_in_quad has been validated */
> +		if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
> +			return -EFAULT;
> +		if (copy_from_user(&old.raw[off_in_quad], old_p, mop->size))
> +			return -EFAULT;
> +	}
>  	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
>  		tmpbuf = vmalloc(mop->size);
>  		if (!tmpbuf)
> @@ -2771,6 +2794,14 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>  	case KVM_S390_MEMOP_ABSOLUTE_WRITE: {
>  		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
>  			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key);
> +		} else if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {
> +			r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size,
> +						       &old.quad, new.quad, mop->key);
> +			if (r == 1) {
> +				r = KVM_S390_MEMOP_R_NO_XCHG;

I wonder if you could not simplify things by returning directly
KVM_S390_MEMOP_R_NO_XCHG instead of 1

> +				if (copy_to_user(old_p, &old.raw[off_in_quad], mop->size))
> +					r = -EFAULT;
> +			}
>  		} else {
>  			if (copy_from_user(tmpbuf, uaddr, mop->size)) {
>  				r = -EFAULT;

Re: [PATCH v3 2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

[Claudio Imbrenda]

On Thu, 17 Nov 2022 23:17:51 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for
> absolute vm write memops which allows user space to perform (storage key
> checked) cmpxchg operations on guest memory.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index eee9f857a986..204d128f23e0 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -3753,7 +3753,8 @@ The fields in each entry are defined as follows:
>  :Parameters: struct kvm_s390_mem_op (in)
>  :Returns: = 0 on success,
>            < 0 on generic error (e.g. -EFAULT or -ENOMEM),
> -          > 0 if an exception occurred while walking the page tables  
> +          16 bit program exception code if the access causes such an exception
> +          other code > maximum 16 bit value with special meaning

I would write the number explicitly ( > 65535 or > 0xffff )

>  
>  Read or write data from/to the VM's memory.
>  The KVM_CAP_S390_MEM_OP_EXTENSION capability specifies what functionality is
> @@ -3771,6 +3772,8 @@ Parameters are specified via the following structure::
>  		struct {
>  			__u8 ar;	/* the access register number */
>  			__u8 key;	/* access key, ignored if flag unset */
> +			__u8 pad1[6];	/* ignored */
> +			__u64 old_p;	/* ignored if flag unset */
>  		};
>  		__u32 sida_offset; /* offset into the sida */
>  		__u8 reserved[32]; /* ignored */
> @@ -3853,8 +3856,22 @@ Absolute accesses are permitted for non-protected guests only.
>  Supported flags:
>    * ``KVM_S390_MEMOP_F_CHECK_ONLY``
>    * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> +  * ``KVM_S390_MEMOP_F_CMPXCHG``
> +
> +The semantics of the flags common with logical acesses are as for logical
> +accesses.
> +
> +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported.
> +In this case, instead of doing an unconditional write, the access occurs only
> +if the target location contains the "size" byte long value pointed to by
> +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two
> +up to and including 16.
> +The value at the target location is written to the location "old_p" points to.
> +If the exchange did not take place because the target value doesn't match the
> +old value KVM_S390_MEMOP_R_NO_XCHG is returned.
> +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION
> +has bit 1 (i.e. bit with value 2) set.
>  
> -The semantics of the flags are as for logical accesses.
>  
>  SIDA read/write:
>  ^^^^^^^^^^^^^^^^

Re: [PATCH v3 4/9] KVM: s390: selftest: memop: Replace macros by functions

[Claudio Imbrenda]

On Thu, 17 Nov 2022 23:17:53 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Replace the DEFAULT_* test helpers by functions, as they don't
> need the exta flexibility.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  tools/testing/selftests/kvm/s390x/memop.c | 82 +++++++++++------------
>  1 file changed, 39 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index 69869c7e2ab1..286185a59238 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -48,6 +48,8 @@ struct mop_desc {
>  	uint8_t key;
>  };
>  
> +const uint8_t NO_KEY = 0xff;
> +
>  static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
>  {
>  	struct kvm_s390_mem_op ksmo = {
> @@ -85,7 +87,7 @@ static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
>  		ksmo.flags |= KVM_S390_MEMOP_F_INJECT_EXCEPTION;
>  	if (desc->_set_flags)
>  		ksmo.flags = desc->set_flags;
> -	if (desc->f_key) {
> +	if (desc->f_key && desc->key != NO_KEY) {

is this change going to affect the behaviour?
if so, please document it in the patch description

>  		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
>  		ksmo.key = desc->key;
>  	}
> @@ -268,34 +270,28 @@ static void prepare_mem12(void)
>  #define ASSERT_MEM_EQ(p1, p2, size) \
>  	TEST_ASSERT(!memcmp(p1, p2, size), "Memory contents do not match!")
>  
> -#define DEFAULT_WRITE_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
> -({										\
> -	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
> -	enum mop_target __target = (mop_target_p);				\
> -	uint32_t __size = (size);						\
> -										\
> -	prepare_mem12();							\
> -	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
> -			GADDR_V(mem1), ##__VA_ARGS__);				\
> -	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
> -	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size,		\
> -			GADDR_V(mem2), ##__VA_ARGS__);				\
> -	ASSERT_MEM_EQ(mem1, mem2, __size);					\
> -})
> +static void default_write_read(struct test_info copy_cpu, struct test_info mop_cpu,
> +			       enum mop_target mop_target, uint32_t size, uint8_t key)
> +{
> +	prepare_mem12();
> +	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size,
> +		   GADDR_V(mem1), KEY(key));
> +	HOST_SYNC(copy_cpu, STAGE_COPIED);
> +	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
> +		   GADDR_V(mem2), KEY(key));
> +	ASSERT_MEM_EQ(mem1, mem2, size);
> +}
>  
> -#define DEFAULT_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
> -({										\
> -	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
> -	enum mop_target __target = (mop_target_p);				\
> -	uint32_t __size = (size);						\
> -										\
> -	prepare_mem12();							\
> -	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
> -			GADDR_V(mem1));						\
> -	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
> -	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size, ##__VA_ARGS__);\
> -	ASSERT_MEM_EQ(mem1, mem2, __size);					\
> -})
> +static void default_read(struct test_info copy_cpu, struct test_info mop_cpu,
> +			 enum mop_target mop_target, uint32_t size, uint8_t key)
> +{
> +	prepare_mem12();
> +	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size, GADDR_V(mem1));
> +	HOST_SYNC(copy_cpu, STAGE_COPIED);
> +	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
> +		   GADDR_V(mem2), KEY(key));
> +	ASSERT_MEM_EQ(mem1, mem2, size);
> +}
>  
>  static void guest_copy(void)
>  {
> @@ -310,7 +306,7 @@ static void test_copy(void)
>  
>  	HOST_SYNC(t.vcpu, STAGE_INITED);
>  
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size);
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, NO_KEY);
>  
>  	kvm_vm_free(t.kvm_vm);
>  }
> @@ -357,26 +353,26 @@ static void test_copy_key(void)
>  	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
>  
>  	/* vm, no key */
> -	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size);
> +	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, NO_KEY);
>  
>  	/* vm/vcpu, machting key or key 0 */
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(0));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(9));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size, KEY(0));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, t.size, KEY(9));
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 0);
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 9);
> +	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, 0);
> +	default_write_read(t.vcpu, t.vm, ABSOLUTE, t.size, 9);
>  	/*
>  	 * There used to be different code paths for key handling depending on
>  	 * if the region crossed a page boundary.
>  	 * There currently are not, but the more tests the merrier.
>  	 */
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, 1, KEY(0));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, 1, KEY(9));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, 1, KEY(0));
> -	DEFAULT_WRITE_READ(t.vcpu, t.vm, ABSOLUTE, 1, KEY(9));
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, 1, 0);
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, 1, 9);
> +	default_write_read(t.vcpu, t.vm, ABSOLUTE, 1, 0);
> +	default_write_read(t.vcpu, t.vm, ABSOLUTE, 1, 9);
>  
>  	/* vm/vcpu, mismatching keys on read, but no fetch protection */
> -	DEFAULT_READ(t.vcpu, t.vcpu, LOGICAL, t.size, GADDR_V(mem2), KEY(2));
> -	DEFAULT_READ(t.vcpu, t.vm, ABSOLUTE, t.size, GADDR_V(mem1), KEY(2));
> +	default_read(t.vcpu, t.vcpu, LOGICAL, t.size, 2);
> +	default_read(t.vcpu, t.vm, ABSOLUTE, t.size, 2);
>  
>  	kvm_vm_free(t.kvm_vm);
>  }
> @@ -409,7 +405,7 @@ static void test_copy_key_storage_prot_override(void)
>  	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
>  
>  	/* vcpu, mismatching keys, storage protection override in effect */
> -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size, KEY(2));
> +	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, 2);
>  
>  	kvm_vm_free(t.kvm_vm);
>  }
> @@ -422,8 +418,8 @@ static void test_copy_key_fetch_prot(void)
>  	HOST_SYNC(t.vcpu, STAGE_SKEYS_SET);
>  
>  	/* vm/vcpu, matching key, fetch protection in effect */
> -	DEFAULT_READ(t.vcpu, t.vcpu, LOGICAL, t.size, GADDR_V(mem2), KEY(9));
> -	DEFAULT_READ(t.vcpu, t.vm, ABSOLUTE, t.size, GADDR_V(mem2), KEY(9));
> +	default_read(t.vcpu, t.vcpu, LOGICAL, t.size, 9);
> +	default_read(t.vcpu, t.vm, ABSOLUTE, t.size, 9);
>  
>  	kvm_vm_free(t.kvm_vm);
>  }

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch]

On Thu, 2022-12-01 at 17:15 +0100, Claudio Imbrenda wrote:
> On Thu, 17 Nov 2022 23:17:50 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
> > User space can use the MEM_OP ioctl to make storage key checked reads
> > and writes to the guest, however, it has no way of performing atomic,
> > key checked, accesses to the guest.
> > Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
> > mode. For now, support this mode for absolute accesses only.
> > 
> > This mode can be use, for example, to set the device-state-change
> > indicator and the adapter-local-summary indicator atomically.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >  include/uapi/linux/kvm.h |   5 ++
> >  arch/s390/kvm/gaccess.h  |   3 ++
> >  arch/s390/kvm/gaccess.c  | 101 +++++++++++++++++++++++++++++++++++++++
> >  arch/s390/kvm/kvm-s390.c |  35 +++++++++++++-
> >  4 files changed, 142 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 0d5d4419139a..1f36be5493e6 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
> >  		struct {
> >  			__u8 ar;	/* the access register number */
> >  			__u8 key;	/* access key, ignored if flag unset */
> > +			__u8 pad1[6];	/* ignored */
> > +			__u64 old_p;	/* ignored if flag unset */
> >  		};
> >  		__u32 sida_offset; /* offset into the sida */
> >  		__u8 reserved[32]; /* ignored */
> > @@ -604,6 +606,9 @@ struct kvm_s390_mem_op {
> >  #define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
> >  #define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)
> >  #define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
> > +#define KVM_S390_MEMOP_F_CMPXCHG		(1ULL << 3)
> > +/* Non program exception return codes (pgm codes are 16 bit) */
> > +#define KVM_S390_MEMOP_R_NO_XCHG		((1 << 16) + 0)
> 
> are you planning to have further *_R_* macros in the near future?
> if not, remove the + 0

No, we can indeed just add it back if there ever are additional ones.

> if yes, move the (1 << 16) to a macro, so it becomes something like
> (KVM_S390_MEMOP_R_BASE + 0)
> 
> (maybe you can find a better/shorter name)
> 
> >  
> >  /* for KVM_INTERRUPT */
> >  struct kvm_interrupt {
> > diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> > index 9408d6cc8e2c..92a3b9fb31ec 100644
> > --- a/arch/s390/kvm/gaccess.h
> > +++ b/arch/s390/kvm/gaccess.h
> > @@ -206,6 +206,9 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> >  int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> >  		      void *data, unsigned long len, enum gacc_mode mode);
> >  
> > +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> > +			       __uint128_t *old, __uint128_t new, u8 access_key);
> > +
> >  /**
> >   * write_guest_with_key - copy data from kernel space to guest space
> >   * @vcpu: virtual cpu
> > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > index 0243b6e38d36..be042865d8a1 100644
> > --- a/arch/s390/kvm/gaccess.c
> > +++ b/arch/s390/kvm/gaccess.c
> > @@ -1161,6 +1161,107 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> >  	return rc;
> >  }
> >  
> > +/**
> > + * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
> > + * @kvm: Virtual machine instance.
> > + * @gpa: Absolute guest address of the location to be changed.
> > + * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
> > + *       non power of two will result in failure.
> > + * @old_p: Pointer to old value. If the location at @gpa contains this value, the
> > + *         exchange will succeed. After calling cmpxchg_guest_abs_with_key() *@old
> > + *         contains the value at @gpa before the attempt to exchange the value.
> > + * @new: The value to place at @gpa.
> > + * @access_key: The access key to use for the guest access.
> > + *
> > + * Atomically exchange the value at @gpa by @new, if it contains *@old.
> > + * Honors storage keys.
> > + *
> > + * Return: * 0: successful exchange
> > + *         * 1: exchange unsuccessful
> > + *         * a program interruption code indicating the reason cmpxchg could
> > + *           not be attempted
> > + *         * -EINVAL: address misaligned or len not power of two
> > + *         * -EAGAIN: transient failure (len 1 or 2)
> 
> please also document -EOPNOTSUPP

I'd add "* -EOPNOTSUPP: should never occur", then, that ok with you?
> 
> > + */
> > +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> > +			       __uint128_t *old_p, __uint128_t new,
> > +			       u8 access_key)
> > +{
> > +	gfn_t gfn = gpa >> PAGE_SHIFT;
> > +	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
> 
> exchange the above two lines (reverse christmas tree)

Is this a hard requirement? Since there is a dependency.
If I do the initialization further down, the order wouldn't actually change.
> 
> > +	bool writable;
> > +	hva_t hva;
> > +	int ret;
> > +
> > +	if (!IS_ALIGNED(gpa, len))
> > +		return -EINVAL;
> > +
> > +	hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
> > +	if (kvm_is_error_hva(hva))
> > +		return PGM_ADDRESSING;
> > +	/*
> > +	 * Check if it's a read-only memslot, even though that cannot occur
> > +	 * since those are unsupported.
> > +	 * Don't try to actually handle that case.
> > +	 */
> > +	if (!writable)
> > +		return -EOPNOTSUPP;
> 
> either you document this, or you return something else (like -EINVAL)
> 
> > +
> > +	hva += offset_in_page(gpa);
> > +	switch (len) {
> > +	case 1: {
> > +		u8 old;
> > +
> > +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_p, new, access_key);
> > +		ret = ret < 0 ? ret : old != *old_p;
> > +		*old_p = old;
> > +		break;
> > +	}
> > +	case 2: {
> > +		u16 old;
> > +
> > +		ret = cmpxchg_user_key((u16 *)hva, &old, *old_p, new, access_key);
> > +		ret = ret < 0 ? ret : old != *old_p;
> > +		*old_p = old;
> > +		break;
> > +	}
> > +	case 4: {
> > +		u32 old;
> > +
> > +		ret = cmpxchg_user_key((u32 *)hva, &old, *old_p, new, access_key);
> > +		ret = ret < 0 ? ret : old != *old_p;
> > +		*old_p = old;
> > +		break;
> > +	}
> > +	case 8: {
> > +		u64 old;
> > +
> > +		ret = cmpxchg_user_key((u64 *)hva, &old, *old_p, new, access_key);
> > +		ret = ret < 0 ? ret : old != *old_p;
> > +		*old_p = old;
> > +		break;
> > +	}
> > +	case 16: {
> > +		__uint128_t old;
> > +
> > +		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_p, new, access_key);
> > +		ret = ret < 0 ? ret : old != *old_p;
> > +		*old_p = old;
> > +		break;
> 
> I really dislike repeating the same code 5 times, but I guess there was
> no other way?

I could use the function called by cmpxchg_user_key directly, but Heiko won't agree to that.
A macro would work too, of course, not sure if I prefer that tho.
> 
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	mark_page_dirty_in_slot(kvm, slot, gfn);
> > +	/*
> > +	 * Assume that the fault is caused by protection, either key protection
> > +	 * or user page write protection.
> > +	 */
> > +	if (ret == -EFAULT)
> > +		ret = PGM_PROTECTION;
> > +	return ret;
> > +}
> > +
> >  /**
> >   * guest_translate_address_with_key - translate guest logical into guest absolute address
> >   * @vcpu: virtual cpu
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 45d4b8182b07..2410b4044283 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -576,7 +576,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  	case KVM_CAP_S390_VCPU_RESETS:
> >  	case KVM_CAP_SET_GUEST_DEBUG:
> >  	case KVM_CAP_S390_DIAG318:
> > -	case KVM_CAP_S390_MEM_OP_EXTENSION:
> >  		r = 1;
> >  		break;
> >  	case KVM_CAP_SET_GUEST_DEBUG2:
> > @@ -590,6 +589,14 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >  	case KVM_CAP_S390_MEM_OP:
> >  		r = MEM_OP_MAX_SIZE;
> >  		break;
> > +	case KVM_CAP_S390_MEM_OP_EXTENSION:
> > +		/*
> > +		 * Flag bits indicating which extensions are supported.
> > +		 * The first extension doesn't use a flag, but pretend it does,
> > +		 * this way that can be changed in the future.
> > +		 */
> > +		r = 0x3;
> > +		break;
> >  	case KVM_CAP_NR_VCPUS:
> >  	case KVM_CAP_MAX_VCPUS:
> >  	case KVM_CAP_MAX_VCPU_ID:
> > @@ -2714,12 +2721,19 @@ static bool access_key_invalid(u8 access_key)
> >  static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >  {
> >  	void __user *uaddr = (void __user *)mop->buf;
> > +	void __user *old_p = (void __user *)mop->old_p;
> > +	union {
> > +		__uint128_t quad;
> > +		char raw[sizeof(__uint128_t)];
> > +	} old = { .quad = 0}, new = { .quad = 0 };
> > +	unsigned int off_in_quad = sizeof(__uint128_t) - mop->size;
> >  	u64 supported_flags;
> >  	void *tmpbuf = NULL;
> >  	int r, srcu_idx;
> >  
> >  	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> > -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
> > +			  | KVM_S390_MEMOP_F_CHECK_ONLY
> > +			  | KVM_S390_MEMOP_F_CMPXCHG;
> >  	if (mop->flags & ~supported_flags || !mop->size)
> >  		return -EINVAL;
> >  	if (mop->size > MEM_OP_MAX_SIZE)
> > @@ -2741,6 +2755,15 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >  	} else {
> >  		mop->key = 0;
> >  	}
> > +	if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {
> 
> add a quick comment here to explain that this check validates
> off_in_quad, and that it does not do a full validation of mop->size,
> which will happen in cmpxchg_guest_abs_with_key.

Will do.
> 
> > +		if (mop->size > sizeof(new))
> > +			return -EINVAL;
> > +		/* off_in_quad has been validated */
> > +		if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
> > +			return -EFAULT;
> > +		if (copy_from_user(&old.raw[off_in_quad], old_p, mop->size))
> > +			return -EFAULT;
> > +	}
> >  	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
> >  		tmpbuf = vmalloc(mop->size);
> >  		if (!tmpbuf)
> > @@ -2771,6 +2794,14 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >  	case KVM_S390_MEMOP_ABSOLUTE_WRITE: {
> >  		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> >  			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key);
> > +		} else if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) {
> > +			r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size,
> > +						       &old.quad, new.quad, mop->key);
> > +			if (r == 1) {
> > +				r = KVM_S390_MEMOP_R_NO_XCHG;
> 
> I wonder if you could not simplify things by returning directly
> KVM_S390_MEMOP_R_NO_XCHG instead of 1

To me it feels like KVM_S390_MEMOP_R_NO_XCHG is api surface and should be referenced here.
cmpxchg_guest_abs_with_key isn't mem op specific
(of course that's the only thing it is currently used for).
> 
> > +				if (copy_to_user(old_p, &old.raw[off_in_quad], mop->size))
> > +					r = -EFAULT;
> > +			}
> >  		} else {
> >  			if (copy_from_user(tmpbuf, uaddr, mop->size)) {
> >  				r = -EFAULT;
> 

Re: [PATCH v3 4/9] KVM: s390: selftest: memop: Replace macros by functions

[Janis Schoetterl-Glausch]

On Thu, 2022-12-01 at 17:28 +0100, Claudio Imbrenda wrote:
> On Thu, 17 Nov 2022 23:17:53 +0100
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
> > Replace the DEFAULT_* test helpers by functions, as they don't
> > need the exta flexibility.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >  tools/testing/selftests/kvm/s390x/memop.c | 82 +++++++++++------------
> >  1 file changed, 39 insertions(+), 43 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> > index 69869c7e2ab1..286185a59238 100644
> > --- a/tools/testing/selftests/kvm/s390x/memop.c
> > +++ b/tools/testing/selftests/kvm/s390x/memop.c
> > @@ -48,6 +48,8 @@ struct mop_desc {
> >  	uint8_t key;
> >  };
> >  
> > +const uint8_t NO_KEY = 0xff;
> > +
> >  static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
> >  {
> >  	struct kvm_s390_mem_op ksmo = {
> > @@ -85,7 +87,7 @@ static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
> >  		ksmo.flags |= KVM_S390_MEMOP_F_INJECT_EXCEPTION;
> >  	if (desc->_set_flags)
> >  		ksmo.flags = desc->set_flags;
> > -	if (desc->f_key) {
> > +	if (desc->f_key && desc->key != NO_KEY) {
> 
> is this change going to affect the behaviour?
> if so, please document it in the patch description

No, previously the absence of a key in the vararg would denote there not being a key,
now that a function is used there is an explicit no key argument, which is checked
here to see if we use key checking or not
> 
> >  		ksmo.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION;
> >  		ksmo.key = desc->key;
> >  	}
> > @@ -268,34 +270,28 @@ static void prepare_mem12(void)
> >  #define ASSERT_MEM_EQ(p1, p2, size) \
> >  	TEST_ASSERT(!memcmp(p1, p2, size), "Memory contents do not match!")
> >  
> > -#define DEFAULT_WRITE_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
> > -({										\
> > -	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
> > -	enum mop_target __target = (mop_target_p);				\
> > -	uint32_t __size = (size);						\
> > -										\
> > -	prepare_mem12();							\
> > -	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
> > -			GADDR_V(mem1), ##__VA_ARGS__);				\
> > -	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
> > -	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size,		\
> > -			GADDR_V(mem2), ##__VA_ARGS__);				\
> > -	ASSERT_MEM_EQ(mem1, mem2, __size);					\
> > -})
> > +static void default_write_read(struct test_info copy_cpu, struct test_info mop_cpu,
> > +			       enum mop_target mop_target, uint32_t size, uint8_t key)
> > +{
> > +	prepare_mem12();
> > +	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size,
> > +		   GADDR_V(mem1), KEY(key));
> > +	HOST_SYNC(copy_cpu, STAGE_COPIED);
> > +	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
> > +		   GADDR_V(mem2), KEY(key));
> > +	ASSERT_MEM_EQ(mem1, mem2, size);
> > +}
> >  
> > -#define DEFAULT_READ(copy_cpu, mop_cpu, mop_target_p, size, ...)		\
> > -({										\
> > -	struct test_info __copy_cpu = (copy_cpu), __mop_cpu = (mop_cpu);	\
> > -	enum mop_target __target = (mop_target_p);				\
> > -	uint32_t __size = (size);						\
> > -										\
> > -	prepare_mem12();							\
> > -	CHECK_N_DO(MOP, __mop_cpu, __target, WRITE, mem1, __size,		\
> > -			GADDR_V(mem1));						\
> > -	HOST_SYNC(__copy_cpu, STAGE_COPIED);					\
> > -	CHECK_N_DO(MOP, __mop_cpu, __target, READ, mem2, __size, ##__VA_ARGS__);\
> > -	ASSERT_MEM_EQ(mem1, mem2, __size);					\
> > -})
> > +static void default_read(struct test_info copy_cpu, struct test_info mop_cpu,
> > +			 enum mop_target mop_target, uint32_t size, uint8_t key)
> > +{
> > +	prepare_mem12();
> > +	CHECK_N_DO(MOP, mop_cpu, mop_target, WRITE, mem1, size, GADDR_V(mem1));
> > +	HOST_SYNC(copy_cpu, STAGE_COPIED);
> > +	CHECK_N_DO(MOP, mop_cpu, mop_target, READ, mem2, size,
> > +		   GADDR_V(mem2), KEY(key));
> > +	ASSERT_MEM_EQ(mem1, mem2, size);
> > +}
> >  
> >  static void guest_copy(void)
> >  {
> > @@ -310,7 +306,7 @@ static void test_copy(void)
> >  
> >  	HOST_SYNC(t.vcpu, STAGE_INITED);
> >  
> > -	DEFAULT_WRITE_READ(t.vcpu, t.vcpu, LOGICAL, t.size);
> > +	default_write_read(t.vcpu, t.vcpu, LOGICAL, t.size, NO_KEY);
> >  
> >  	kvm_vm_free(t.kvm_vm);
> >  }

[...]

Re: [PATCH v3 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Claudio Imbrenda]

On Thu, 01 Dec 2022 18:44:56 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> > 
> > please also document -EOPNOTSUPP  
> 
> I'd add "* -EOPNOTSUPP: should never occur", then, that ok with you?

no, also explain in which conditions it is returned

something like: 
 * -EOPNOTSUPP: if the memslot is not writable (should never occour)

> >   
> > > + */
> > > +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> > > +			       __uint128_t *old_p, __uint128_t new,
> > > +			       u8 access_key)
> > > +{
> > > +	gfn_t gfn = gpa >> PAGE_SHIFT;
> > > +	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);  
> > 
> > exchange the above two lines (reverse christmas tree)  
> 
> Is this a hard requirement? Since there is a dependency.
> If I do the initialization further down, the order wouldn't actually change.

ahhhhh right, I had missed that

keep it as it is, of course

[...]

> > I really dislike repeating the same code 5 times, but I guess there was
> > no other way?  
> 
> I could use the function called by cmpxchg_user_key directly, but Heiko won't agree to that.
> A macro would work too, of course, not sure if I prefer that tho.

ok so there is no other way, let's keep it as it is

[...]

> To me it feels like KVM_S390_MEMOP_R_NO_XCHG is api surface and should be referenced here.
> cmpxchg_guest_abs_with_key isn't mem op specific
> (of course that's the only thing it is currently used for).

fair enough

> >   
> > > +				if (copy_to_user(old_p, &old.raw[off_in_quad], mop->size))
> > > +					r = -EFAULT;
> > > +			}
> > >  		} else {
> > >  			if (copy_from_user(tmpbuf, uaddr, mop->size)) {
> > >  				r = -EFAULT;  
> >   
>