mbox series

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

Message ID 20221117221758.66326-1-scgl@linux.ibm.com
Headers show
Series KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg | expand

Message

Janis Schoetterl-Glausch Nov. 17, 2022, 10:17 p.m. UTC
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

Comments

Heiko Carstens Nov. 18, 2022, 10:12 a.m. UTC | #1
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.
Thomas Huth Nov. 18, 2022, 2:37 p.m. UTC | #2
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
Heiko Carstens Nov. 18, 2022, 3:15 p.m. UTC | #3
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.
Janis Schoetterl-Glausch Nov. 21, 2022, 5:41 p.m. UTC | #4
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.

[...]