[v7,00/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

Message

Janis Schoetterl-Glausch Feb. 6, 2023, 4:45 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
operation. For now, support this operation for absolute accesses only.

This operation 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.

v6 -> v7
 * cast to __user pointers in cmpxchg_guest_abs_with_key
 * dropped function for allocating buffer for mem_op
 * use goto if CHECK_ONLY in mem_op refactoring
 * changed assert message in bad address test
 * picked up R-b's (thanks Thomas & Janosch)
 * fix nits, fix typos, improve commit descriptions

v5 -> v6
 * move memop selftest fixes/refactoring to front of series so they can
   be picked independently from the rest
 * use op instead of flag to indicate cmpxchg
 * no longer indicate success of cmpxchg to user space, which can infer
   it by observing a change in the old value instead
 * refactor functions implementing the ioctl
 * adjust documentation (drop R-b)
 * adjust selftest
 * rebase

v4 -> v5
 * refuse cmpxchg if not write (thanks Thomas)
 * minor doc changes (thanks Claudio)
 * picked up R-b's (thanks Thomas & Claudio)
 * memop selftest fixes
 * rebased

v3 -> v4
 * no functional change intended
 * rework documentation a bit
 * name extension cap cmpxchg bit
 * picked up R-b (thanks Thomas)
 * various changes (rename variable, comments, ...) see range-diff below

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 (14):
  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 bad address test
  KVM: s390: selftest: memop: Fix typo
  KVM: s390: selftest: memop: Fix wrong address being used in test
  KVM: s390: selftest: memop: Fix integer literal
  KVM: s390: Move common code of mem_op functions into function
  KVM: s390: Dispatch to implementing function at top level of vm mem_op
  KVM: s390: Refactor absolute vm mem_op function
  KVM: s390: Refactor vcpu mem_op function
  KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
  Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG
  KVM: s390: selftest: memop: Add cmpxchg tests

 Documentation/virt/kvm/api.rst            |  29 +-
 include/uapi/linux/kvm.h                  |   8 +
 arch/s390/kvm/gaccess.h                   |   3 +
 arch/s390/kvm/gaccess.c                   | 109 ++++
 arch/s390/kvm/kvm-s390.c                  | 221 ++++---
 tools/testing/selftests/kvm/s390x/memop.c | 675 +++++++++++++++++-----
 6 files changed, 809 insertions(+), 236 deletions(-)

Range-diff against v6:
 1:  512e1a3e0ae5 !  1:  fb51df0930d8 KVM: s390: selftest: memop: Pass mop_desc via pointer
    @@ Commit message
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
         Reviewed-by: Thomas Huth <[e-mail]>
    +    Reviewed-by: Janosch Frank <[e-mail]>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: struct mop_desc {
 2:  47328ea64f80 !  2:  6f09b06574f0 KVM: s390: selftest: memop: Replace macros by functions
    @@ Commit message
         KVM: s390: selftest: memop: Replace macros by functions
     
         Replace the DEFAULT_* test helpers by functions, as they don't
    -    need the exta flexibility.
    +    need the extra flexibility.
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
         Reviewed-by: Thomas Huth <[e-mail]>
    +    Acked-by: Janosch Frank <[e-mail]>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: struct mop_desc {
 3:  224fe37eeec7 !  3:  108c1af396fe KVM: s390: selftest: memop: Move testlist into main
    @@ Commit message
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
         Reviewed-by: Thomas Huth <[e-mail]>
    +    Reviewed-by: Janosch Frank <[e-mail]>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
 4:  f622d3413cf0 !  4:  4bcad9b4bf9d KVM: s390: selftest: memop: Add bad address test
    @@ Metadata
      ## Commit message ##
         KVM: s390: selftest: memop: Add bad address test
     
    -    Add test that tries to access, instead of CHECK_ONLY.
    +    Add a test that tries a real write to a bad address.
    +    The existing CHECK_ONLY test doesn't cover all paths.
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
         Reviewed-by: Nico Boehr <[e-mail]>
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void _test_errors_common(struc
      	/* 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");
    ++	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address with CHECK_ONLY");
     +	rv = ERR_MOP(info, target, WRITE, mem1, size, GADDR((void *)~0xfffUL));
    -+	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address");
    ++	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address on write");
      
      	/* Bad host address: */
      	rv = ERR_MOP(info, target, WRITE, 0, size, GADDR_V(mem1));
 5:  431f191a8a57 =  5:  7af4c710045c KVM: s390: selftest: memop: Fix typo
 6:  3122187435fb =  6:  2ab1854f3959 KVM: s390: selftest: memop: Fix wrong address being used in test
 7:  401f51f3ef55 !  7:  980644e1ce74 KVM: s390: selftest: memop: Fix integer literal
    @@ Commit message
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
         Fixes: 1bb873495a9e ("KVM: s390: selftests: Add more copy memop tests")
    +    Reviewed-by: Thomas Huth <[e-mail]>
     
      ## tools/testing/selftests/kvm/s390x/memop.c ##
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void guest_copy_key_fetch_prot_override(void)
 8:  185e5cd33df6 !  8:  a9930caadf5e KVM: s390: Move common code of mem_op functions into functions
    @@ Metadata
     Author: Janis Schoetterl-Glausch <[e-mail]>
     
      ## Commit message ##
    -    KVM: s390: Move common code of mem_op functions into functions
    +    KVM: s390: Move common code of mem_op functions into function
     
         The vcpu and vm mem_op ioctl implementations share some functionality.
    -    Move argument checking and buffer allocation into functions and call
    -    them from both implementations.
    -    This allows code reuse in case of additional future mem_op operations.
    +    Move argument checking into a function and call it from both
    +    implementations. This allows code reuse in case of additional future
    +    mem_op operations.
     
         Suggested-by: Janosch Frank <[e-mail]>
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
    @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_handle_pv(struct kvm *kvm, struct
     +		mop->key = 0;
     +	}
     +	return 0;
    -+}
    -+
    -+static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
    -+{
    -+	void *buf;
    -+
    -+	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)
    -+		return NULL;
    -+	buf = vmalloc(mop->size);
    -+	if (!buf)
    -+		return ERR_PTR(-ENOMEM);
    -+	return buf;
      }
      
      static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op(struct kvm *kvm, struct
     -	} else {
     -		mop->key = 0;
     -	}
    --	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    --		tmpbuf = vmalloc(mop->size);
    --		if (!tmpbuf)
    --			return -ENOMEM;
    --	}
    -+	tmpbuf = mem_op_alloc_buf(mop);
    -+	if (IS_ERR(tmpbuf))
    -+		return PTR_ERR(tmpbuf);
    - 
    - 	srcu_idx = srcu_read_lock(&kvm->srcu);
    - 
    + 	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    + 		tmpbuf = vmalloc(mop->size);
    + 		if (!tmpbuf)
     @@ arch/s390/kvm/kvm-s390.c: static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
      {
      	void __user *uaddr = (void __user *)mop->buf;
    @@ arch/s390/kvm/kvm-s390.c: static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu
     -	} else {
     -		mop->key = 0;
     -	}
    --	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    --		tmpbuf = vmalloc(mop->size);
    --		if (!tmpbuf)
    --			return -ENOMEM;
    --	}
    -+	tmpbuf = mem_op_alloc_buf(mop);
    -+	if (IS_ERR(tmpbuf))
    -+		return PTR_ERR(tmpbuf);
    - 
    - 	switch (mop->op) {
    - 	case KVM_S390_MEMOP_LOGICAL_READ:
    + 	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    + 		tmpbuf = vmalloc(mop->size);
    + 		if (!tmpbuf)
 9:  d7c8b94cb351 !  9:  2715c1ceabdb KVM: s390: Dispatch to implementing function at top level of vm mem_op
    @@ Commit message
     
         Suggested-by: Janosch Frank <[e-mail]>
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
    +    Reviewed-by: Thomas Huth <[e-mail]>
     
      ## arch/s390/kvm/kvm-s390.c ##
    -@@ arch/s390/kvm/kvm-s390.c: static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
    - 	return buf;
    +@@ arch/s390/kvm/kvm-s390.c: static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_fla
    + 	return 0;
      }
      
     -static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op(struct kvm *kvm, struct
     -	 */
     -	if (kvm_s390_pv_get_handle(kvm))
     -		return -EINVAL;
    - 	tmpbuf = mem_op_alloc_buf(mop);
    - 	if (IS_ERR(tmpbuf))
    - 		return PTR_ERR(tmpbuf);
    + 	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    + 		tmpbuf = vmalloc(mop->size);
    + 		if (!tmpbuf)
     @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
      		}
      		break;
10:  8acb425a9e93 ! 10:  91f4e887f311 KVM: s390: Refactor absolute vm mem_op function
    @@ Commit message
     
         Suggested-by: Janosch Frank <[e-mail]>
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
    +    Reviewed-by: Thomas Huth <[e-mail]>
     
      ## arch/s390/kvm/kvm-s390.c ##
    -@@ arch/s390/kvm/kvm-s390.c: static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
    +@@ arch/s390/kvm/kvm-s390.c: static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_fla
      static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
      {
      	void __user *uaddr = (void __user *)mop->buf;
    @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, str
     -			}
     -		}
     -		break;
    --	}
    ++	acc_mode = mop->op == KVM_S390_MEMOP_ABSOLUTE_READ ? GACC_FETCH : GACC_STORE;
    ++	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
    ++		r = check_gpa_range(kvm, mop->gaddr, mop->size, acc_mode, mop->key);
    ++		goto out_unlock;
    + 	}
     -	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);
    @@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, str
     -			}
     -			r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
     -						      mop->size, GACC_STORE, mop->key);
    -+	acc_mode = mop->op == KVM_S390_MEMOP_ABSOLUTE_READ ? GACC_FETCH : GACC_STORE;
    -+	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
    -+		r = check_gpa_range(kvm, mop->gaddr, mop->size, acc_mode, mop->key);
    -+	} else if (acc_mode == GACC_FETCH) {
    ++	if (acc_mode == GACC_FETCH) {
     +		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
     +					      mop->size, GACC_FETCH, mop->key);
     +		if (r)
11:  f62dfb06ea55 ! 11:  d16ac0db04c7 KVM: s390: Refactor absolute vcpu mem_op function
    @@ Metadata
     Author: Janis Schoetterl-Glausch <[e-mail]>
     
      ## Commit message ##
    -    KVM: s390: Refactor absolute vcpu mem_op function
    +    KVM: s390: Refactor vcpu mem_op function
     
         Remove code duplication with regards to the CHECK_ONLY flag.
         Decrease the number of indents.
    @@ arch/s390/kvm/kvm-s390.c: static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu
      	int r;
      
     @@ arch/s390/kvm/kvm-s390.c: static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
    - 	if (IS_ERR(tmpbuf))
    - 		return PTR_ERR(tmpbuf);
    + 			return -ENOMEM;
    + 	}
      
     -	switch (mop->op) {
     -	case KVM_S390_MEMOP_LOGICAL_READ:
    @@ arch/s390/kvm/kvm-s390.c: static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu
     +	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
     +		r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
     +				    acc_mode, mop->key);
    -+	} else if (acc_mode == GACC_FETCH) {
    ++		goto out_inject;
    ++	}
    ++	if (acc_mode == GACC_FETCH) {
      		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
      					mop->size, mop->key);
     -		if (r == 0) {
12:  2199327cb484 ! 12:  afec9544eec4 KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
    @@ Commit message
         Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
         op. For now, support this op for absolute accesses only.
     
    -    This op can be use, for example, to set the device-state-change
    +    This op can be used, for example, to set the device-state-change
         indicator and the adapter-local-summary indicator atomically.
     
         Signed-off-by: Janis Schoetterl-Glausch <[e-mail]>
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     + * Honors storage keys.
     + *
     + * Return: * 0: successful exchange
    -+ *         * a program interruption code indicating the reason cmpxchg could
    -+ *           not be attempted
    ++ *         * >0: 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)
     + *         * -EOPNOTSUPP: read-only memslot (should never occur)
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +			       __uint128_t *old_addr, __uint128_t new,
     +			       u8 access_key, bool *success)
     +{
    -+	gfn_t gfn = gpa >> PAGE_SHIFT;
    ++	gfn_t gfn = gpa_to_gfn(gpa);
     +	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
     +	bool writable;
     +	hva_t hva;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		return -EOPNOTSUPP;
     +
     +	hva += offset_in_page(gpa);
    ++	/*
    ++	 * The cmpxchg_user_key macro depends on the type of "old", so we need
    ++	 * a case for each valid length and get some code duplication as long
    ++	 * as we don't introduce a new macro.
    ++	 */
     +	switch (len) {
     +	case 1: {
     +		u8 old;
     +
    -+		ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
    ++		ret = cmpxchg_user_key((u8 __user *)hva, &old, *old_addr, new, access_key);
     +		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +	case 2: {
     +		u16 old;
     +
    -+		ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
    ++		ret = cmpxchg_user_key((u16 __user *)hva, &old, *old_addr, new, access_key);
     +		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +	case 4: {
     +		u32 old;
     +
    -+		ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
    ++		ret = cmpxchg_user_key((u32 __user *)hva, &old, *old_addr, new, access_key);
     +		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +	case 8: {
     +		u64 old;
     +
    -+		ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
    ++		ret = cmpxchg_user_key((u64 __user *)hva, &old, *old_addr, new, access_key);
     +		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +	case 16: {
     +		__uint128_t old;
     +
    -+		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
    ++		ret = cmpxchg_user_key((__uint128_t __user *)hva, &old, *old_addr, new, access_key);
     +		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +	default:
     +		return -EINVAL;
     +	}
    -+	mark_page_dirty_in_slot(kvm, slot, gfn);
    ++	if (*success)
    ++		mark_page_dirty_in_slot(kvm, slot, gfn);
     +	/*
     +	 * Assume that the fault is caused by protection, either key protection
     +	 * or user page write protection.
13:  05d7682a90ba = 13:  2c006a4015c0 Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG
14:  993293dcdf13 = 14:  ac16991fbe27 KVM: s390: selftest: memop: Add cmpxchg tests

base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3
prerequisite-patch-id: 02346a556c2af3340683322ef71816f813599c21
prerequisite-patch-id: 770ae375220957f1db8eb2657bf2fb7868a501af
prerequisite-patch-id: fba46548bd1b60742eed8669a15cadad3c5ec19c
prerequisite-patch-id: 8df365bc587d8768618759d77e3a6806bc36130d
prerequisite-patch-id: 0fc74ea6b8342b1dd69b034be308647d47d31746
prerequisite-patch-id: c5cdc3ce7cdffc18c5e56abfb657c84141fb623a
prerequisite-patch-id: 837715a7a75d9175027343d727c98a398b927c9b
prerequisite-patch-id: 6ccea6ff976ae44347f266d7a9e19e09a2ae9063