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

[PATCH v6 00/14] 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
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.

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 functions
  KVM: s390: Dispatch to implementing function at top level of vm mem_op
  KVM: s390: Refactor absolute vm mem_op function
  KVM: s390: Refactor absolute 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                   | 103 ++++
 arch/s390/kvm/kvm-s390.c                  | 249 ++++----
 tools/testing/selftests/kvm/s390x/memop.c | 675 +++++++++++++++++-----
 6 files changed, 819 insertions(+), 248 deletions(-)

Range-diff against v5:
 3:  94c1165ae24a =  1:  512e1a3e0ae5 KVM: s390: selftest: memop: Pass mop_desc via pointer
 4:  027c87eee0ac =  2:  47328ea64f80 KVM: s390: selftest: memop: Replace macros by functions
 5:  16ac410ecc0f =  3:  224fe37eeec7 KVM: s390: selftest: memop: Move testlist into main
 7:  2d6776733e64 =  4:  f622d3413cf0 KVM: s390: selftest: memop: Add bad address test
 8:  8c49eafd2881 =  5:  431f191a8a57 KVM: s390: selftest: memop: Fix typo
 9:  0af907110b34 =  6:  3122187435fb KVM: s390: selftest: memop: Fix wrong address being used in test
10:  886c80b2bdce =  7:  401f51f3ef55 KVM: s390: selftest: memop: Fix integer literal
 -:  ------------ >  8:  df09794e0794 KVM: s390: Move common code of mem_op functions into functions
 -:  ------------ >  9:  5cbae63357ed KVM: s390: Dispatch to implementing function at top level of vm mem_op
 -:  ------------ > 10:  76ba77b63a26 KVM: s390: Refactor absolute vm mem_op function
 -:  ------------ > 11:  c848e772e22a KVM: s390: Refactor absolute vcpu mem_op function
 1:  6adc166ee141 ! 12:  6ccb200ad85c KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
    @@ Commit message
         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.
    +    op. For now, support this op for absolute accesses only.
     
    -    This mode can be use, for example, to set the device-state-change
    +    This op 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: struct kvm_s390_mem_op {
      			__u8 ar;	/* the access register number */
      			__u8 key;	/* access key, ignored if flag unset */
     +			__u8 pad1[6];	/* ignored */
    -+			__u64 old_addr;	/* ignored if flag unset */
    ++			__u64 old_addr;	/* ignored if cmpxchg flag unset */
      		};
      		__u32 sida_offset; /* offset into the sida */
      		__u8 reserved[32]; /* ignored */
     @@ include/uapi/linux/kvm.h: struct kvm_s390_mem_op {
    + #define KVM_S390_MEMOP_SIDA_WRITE	3
    + #define KVM_S390_MEMOP_ABSOLUTE_READ	4
    + #define KVM_S390_MEMOP_ABSOLUTE_WRITE	5
    ++#define KVM_S390_MEMOP_ABSOLUTE_CMPXCHG	6
    ++
    + /* flags for kvm_s390_mem_op->flags */
      #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)
    -+/* flags specifying extension support */
    -+#define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG 0x2
    -+/* Non program exception return codes (pgm codes are 16 bit) */
    -+#define KVM_S390_MEMOP_R_NO_XCHG		(1 << 16)
      
    ++/* flags specifying extension support via KVM_CAP_S390_MEM_OP_EXTENSION */
    ++#define KVM_S390_MEMOP_EXTENSION_CAP_BASE	(1 << 0)
    ++#define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG	(1 << 1)
    ++
      /* for KVM_INTERRUPT */
      struct kvm_interrupt {
    + 	/* in */
     
      ## arch/s390/kvm/gaccess.h ##
     @@ arch/s390/kvm/gaccess.h: 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);
    ++int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len, __uint128_t *old,
    ++			       __uint128_t new, u8 access_key, bool *success);
     +
      /**
       * 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
     + * @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_addr: 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.
    ++ * @old_addr: Pointer to old value. If the location at @gpa contains this value,
    ++ *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
    ++ *            *@old_addr 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.
    ++ * @success: output value indicating if an exchange occurred.
     + *
     + * 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
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     + */
     +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
     +			       __uint128_t *old_addr, __uint128_t new,
    -+			       u8 access_key)
    ++			       u8 access_key, bool *success)
     +{
     +	gfn_t gfn = gpa >> PAGE_SHIFT;
     +	struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		u8 old;
     +
     +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
    -+		ret = ret < 0 ? ret : old != *old_addr;
    ++		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
     +	}
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		u16 old;
     +
     +		ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
    -+		ret = ret < 0 ? ret : old != *old_addr;
    ++		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
     +	}
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		u32 old;
     +
     +		ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
    -+		ret = ret < 0 ? ret : old != *old_addr;
    ++		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
     +	}
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		u64 old;
     +
     +		ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
    -+		ret = ret < 0 ? ret : old != *old_addr;
    ++		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
     +	}
    @@ arch/s390/kvm/gaccess.c: int access_guest_real(struct kvm_vcpu *vcpu, unsigned l
     +		__uint128_t old;
     +
     +		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
    -+		ret = ret < 0 ? ret : old != *old_addr;
    ++		*success = !ret && old == *old_addr;
     +		*old_addr = old;
     +		break;
     +	}
    @@ arch/s390/kvm/kvm-s390.c: int kvm_vm_ioctl_check_extension(struct kvm *kvm, long
     +	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.
    ++		 * If r > 0, the base extension must also be supported/indicated,
    ++		 * in order to maintain backwards compatibility.
     +		 */
    -+		r = KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG | 1;
    ++		r = KVM_S390_MEMOP_EXTENSION_CAP_BASE |
    ++		    KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG;
     +		break;
      	case KVM_CAP_NR_VCPUS:
      	case KVM_CAP_MAX_VCPUS:
      	case KVM_CAP_MAX_VCPU_ID:
    -@@ arch/s390/kvm/kvm-s390.c: 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;
    +@@ arch/s390/kvm/kvm-s390.c: static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    + 	return r;
    + }
    + 
    ++static int kvm_s390_vm_mem_op_cmpxchg(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    ++{
    ++	void __user *uaddr = (void __user *)mop->buf;
     +	void __user *old_addr = (void __user *)mop->old_addr;
     +	union {
     +		__uint128_t quad;
     +		char raw[sizeof(__uint128_t)];
     +	} old = { .quad = 0}, new = { .quad = 0 };
     +	unsigned int off_in_quad = sizeof(new) - 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)
    -@@ arch/s390/kvm/kvm-s390.c: 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) {
    -+		/*
    -+		 * This validates off_in_quad. Checking that size is a power
    -+		 * of two is not necessary, as cmpxchg_guest_abs_with_key
    -+		 * takes care of that
    -+		 */
    -+		if (mop->size > sizeof(new))
    -+			return -EINVAL;
    -+		if (mop->op != KVM_S390_MEMOP_ABSOLUTE_WRITE)
    -+			return -EINVAL;
    -+		if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
    -+			return -EFAULT;
    -+		if (copy_from_user(&old.raw[off_in_quad], old_addr, mop->size))
    -+			return -EFAULT;
    ++	int r, srcu_idx;
    ++	bool success;
    ++
    ++	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION);
    ++	if (r)
    ++		return r;
    ++	/*
    ++	 * This validates off_in_quad. Checking that size is a power
    ++	 * of two is not necessary, as cmpxchg_guest_abs_with_key
    ++	 * takes care of that
    ++	 */
    ++	if (mop->size > sizeof(new))
    ++		return -EINVAL;
    ++	if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
    ++		return -EFAULT;
    ++	if (copy_from_user(&old.raw[off_in_quad], old_addr, mop->size))
    ++		return -EFAULT;
    ++
    ++	srcu_idx = srcu_read_lock(&kvm->srcu);
    ++
    ++	if (kvm_is_error_gpa(kvm, mop->gaddr)) {
    ++		r = PGM_ADDRESSING;
    ++		goto out_unlock;
     +	}
    - 	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    - 		tmpbuf = vmalloc(mop->size);
    - 		if (!tmpbuf)
    ++
    ++	r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size, &old.quad,
    ++				       new.quad, mop->key, &success);
    ++	if (!success && copy_to_user(old_addr, &old.raw[off_in_quad], mop->size))
    ++		r = -EFAULT;
    ++
    ++out_unlock:
    ++	srcu_read_unlock(&kvm->srcu, srcu_idx);
    ++	return r;
    ++}
    ++
    + 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 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_addr, &old.raw[off_in_quad], mop->size))
    -+					r = -EFAULT;
    -+			}
    - 		} else {
    - 			if (copy_from_user(tmpbuf, uaddr, mop->size)) {
    - 				r = -EFAULT;
    + 	case KVM_S390_MEMOP_ABSOLUTE_READ:
    + 	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
    + 		return kvm_s390_vm_mem_op_abs(kvm, mop);
    ++	case KVM_S390_MEMOP_ABSOLUTE_CMPXCHG:
    ++		return kvm_s390_vm_mem_op_cmpxchg(kvm, mop);
    + 	default:
    + 		return -EINVAL;
    + 	}
 2:  fce9a063ab70 ! 13:  4d983d179903 Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG
    @@ Commit message
         checked) cmpxchg operations on guest memory.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    -    Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
     
      ## Documentation/virt/kvm/api.rst ##
     @@ Documentation/virt/kvm/api.rst: The fields in each entry are defined as follows:
    @@ Documentation/virt/kvm/api.rst: Parameters are specified via the following struc
      		};
      		__u32 sida_offset; /* offset into the sida */
      		__u8 reserved[32]; /* ignored */
    -@@ Documentation/virt/kvm/api.rst: Absolute accesses are permitted for non-protected guests only.
    - Supported flags:
    +@@ Documentation/virt/kvm/api.rst: Possible operations are:
    +   * ``KVM_S390_MEMOP_ABSOLUTE_WRITE``
    +   * ``KVM_S390_MEMOP_SIDA_READ``
    +   * ``KVM_S390_MEMOP_SIDA_WRITE``
    ++  * ``KVM_S390_MEMOP_ABSOLUTE_CMPXCHG``
    + 
    + Logical read/write:
    + ^^^^^^^^^^^^^^^^^^^
    +@@ Documentation/virt/kvm/api.rst: the checks required for storage key protection as one operation (as opposed to
    + user space getting the storage keys, performing the checks, and accessing
    + memory thereafter, which could lead to a delay between check and access).
    + Absolute accesses are permitted for the VM ioctl if KVM_CAP_S390_MEM_OP_EXTENSION
    +-is > 0.
    ++has the KVM_S390_MEMOP_EXTENSION_CAP_BASE bit set.
    + Currently absolute accesses are not permitted for VCPU ioctls.
    + Absolute accesses are permitted for non-protected guests only.
    + 
    +@@ Documentation/virt/kvm/api.rst: Supported flags:
        * ``KVM_S390_MEMOP_F_CHECK_ONLY``
        * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
    -+  * ``KVM_S390_MEMOP_F_CMPXCHG``
    -+
    + 
    +-The semantics of the flags are as for logical accesses.
     +The semantics of the flags common with logical accesses are as for logical
     +accesses.
     +
    -+For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if
    -+KVM_CAP_S390_MEM_OP_EXTENSION has flag KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG set.
    -+In this case, instead of doing an unconditional write, the access occurs
    -+only if the target location contains the value pointed to by "old_addr".
    ++Absolute cmpxchg:
    ++^^^^^^^^^^^^^^^^^
    ++
    ++Perform cmpxchg on absolute guest memory. Intended for use with the
    ++KVM_S390_MEMOP_F_SKEY_PROTECTION flag.
    ++Instead of doing an unconditional write, the access occurs only if the target
    ++location contains the value pointed to by "old_addr".
     +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.
     +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.
    - 
    --The semantics of the flags are as for logical accesses.
    ++old value, the value "old_addr" points to is replaced by the target value.
    ++User space can tell if an exchange took place by checking if this replacement
    ++occurred. The cmpxchg op is permitted for the VM ioctl if
    ++KVM_CAP_S390_MEM_OP_EXTENSION has flag KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG set.
    ++
    ++Supported flags:
    ++  * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
      
      SIDA read/write:
      ^^^^^^^^^^^^^^^^
 6:  214281b6eb96 ! 14:  5250be3dd58b KVM: s390: selftest: memop: Add cmpxchg tests
    @@ tools/testing/selftests/kvm/s390x/memop.c
      
      #include <linux/bits.h>
      
    +@@ tools/testing/selftests/kvm/s390x/memop.c: enum mop_target {
    + enum mop_access_mode {
    + 	READ,
    + 	WRITE,
    ++	CMPXCHG,
    + };
    + 
    + struct mop_desc {
     @@ tools/testing/selftests/kvm/s390x/memop.c: struct mop_desc {
      	enum mop_access_mode mode;
      	void *buf;
      	uint32_t sida_offset;
     +	void *old;
    ++	uint8_t old_value[16];
     +	bool *cmpxchg_success;
      	uint8_t ar;
      	uint8_t key;
      };
    + 
    + const uint8_t NO_KEY = 0xff;
    + 
    +-static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
    ++static struct kvm_s390_mem_op ksmo_from_desc(struct mop_desc *desc)
    + {
    + 	struct kvm_s390_mem_op ksmo = {
    + 		.gaddr = (uintptr_t)desc->gaddr,
     @@ 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;
    - 	}
    -+	if (desc->old) {
    -+		ksmo.flags |= KVM_S390_MEMOP_F_CMPXCHG;
    -+		ksmo.old_addr = (uint64_t)desc->old;
    -+	}
    - 	if (desc->_ar)
    - 		ksmo.ar = desc->ar;
    - 	else
    + 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_READ;
    + 		if (desc->mode == WRITE)
    + 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_WRITE;
    ++		if (desc->mode == CMPXCHG) {
    ++			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_CMPXCHG;
    ++			ksmo.old_addr = (uint64_t)desc->old;
    ++			memcpy(desc->old_value, desc->old, desc->size);
    ++		}
    + 		break;
    + 	case INVALID:
    + 		ksmo.op = -1;
     @@ tools/testing/selftests/kvm/s390x/memop.c: static void print_memop(struct kvm_vcpu *vcpu, const struct kvm_s390_mem_op *ksm
    + 	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
      		printf("ABSOLUTE, WRITE, ");
      		break;
    ++	case KVM_S390_MEMOP_ABSOLUTE_CMPXCHG:
    ++		printf("ABSOLUTE, CMPXCHG, ");
    ++		break;
      	}
     -	printf("gaddr=%llu, size=%u, buf=%llu, ar=%u, key=%u",
     -	       ksmo->gaddr, ksmo->size, ksmo->buf, ksmo->ar, ksmo->key);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void print_memop(struct kvm_vc
      	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");
    +@@ tools/testing/selftests/kvm/s390x/memop.c: static void print_memop(struct kvm_vcpu *vcpu, const struct kvm_s390_mem_op *ksm
      	puts(")");
      }
      
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void print_memop(struct kvm_vc
     +	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;
    ++	if (ksmo->op == KVM_S390_MEMOP_ABSOLUTE_CMPXCHG) {
    ++		if (desc->cmpxchg_success) {
    ++			int diff = memcmp(desc->old_value, desc->old, desc->size);
    ++			*desc->cmpxchg_success = !diff;
    ++		}
     +	}
     +	TEST_ASSERT(!r, __KVM_IOCTL_ERROR("KVM_S390_MEM_OP", r));
      
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void default_read(struct test_
     +			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));
    ++			MOP(test->vm, ABSOLUTE, CMPXCHG, 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");
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void default_read(struct test_
     +			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));
    ++			MOP(test->vm, ABSOLUTE, CMPXCHG, 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");
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +		do {
     +			old = 0;
     +			new = 1;
    -+			MOP(t.vm, ABSOLUTE, WRITE, &new,
    ++			MOP(t.vm, ABSOLUTE, CMPXCHG, &new,
     +			    sizeof(new), GADDR_V(mem1),
     +			    CMPXCHG_OLD(&old),
     +			    CMPXCHG_SUCCESS(&success), KEY(1));
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_copy_key(void)
     +			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),
    ++				MOP(t.vm, ABSOLUTE, CMPXCHG, quad_to_char(&new, size),
     +				    size, GADDR_V(mem2 + offset),
     +				    CMPXCHG_OLD(quad_to_char(&old, size)),
     +				    CMPXCHG_SUCCESS(&success), KEY(1));
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors_key(void)
     +	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));
    ++		ERR_PROT_MOP(t.vm, ABSOLUTE, CMPXCHG, mem2, i, GADDR_V(mem2),
    ++			     CMPXCHG_OLD(&old), KEY(2));
     +	}
     +
     +	kvm_vm_free(t.kvm_vm);
    @@ tools/testing/selftests/kvm/s390x/memop.c: static void test_errors(void)
     +			power *= 2;
     +			continue;
     +		}
    -+		rv = ERR_MOP(t.vm, ABSOLUTE, WRITE, mem1, i, GADDR_V(mem1),
    ++		rv = ERR_MOP(t.vm, ABSOLUTE, CMPXCHG, 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),
    ++		rv = ERR_MOP(t.vm, ABSOLUTE, CMPXCHG, mem1, i, GADDR((void *)~0xfffUL),
     +			     CMPXCHG_OLD(&old));
     +		TEST_ASSERT(rv > 0, "ioctl allows bad guest address for cmpxchg");
    -+		rv = ERR_MOP(t.vm, ABSOLUTE, READ, mem1, i, GADDR_V(mem1),
    -+			     CMPXCHG_OLD(&old));
    -+		TEST_ASSERT(rv == -1 && errno == EINVAL,
    -+			    "ioctl allows read cmpxchg call");
     +	}
     +	for (i = 2; i <= 16; i *= 2) {
    -+		rv = ERR_MOP(t.vm, ABSOLUTE, WRITE, mem1, i, GADDR_V(mem1 + 1),
    ++		rv = ERR_MOP(t.vm, ABSOLUTE, CMPXCHG, mem1, i, GADDR_V(mem1 + 1),
     +			     CMPXCHG_OLD(&old));
     +		TEST_ASSERT(rv == -1 && errno == EINVAL,
     +			    "ioctl allows bad alignment for cmpxchg");
-- 
2.34.1

[PATCH v6 01/14] 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 3fd81e58f40c..9c05d1205114 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

Re: [PATCH v6 01/14] KVM: s390: selftest: memop: Pass mop_desc via pointer

[Janosch Frank]

On 1/25/23 22:25, Janis Schoetterl-Glausch wrote:
> 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>

Reviewed-by: Janosch Frank <frankja@linux.ibm.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 3fd81e58f40c..9c05d1205114 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);					\
>   })

[PATCH v6 02/14] 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>
Reviewed-by: Thomas Huth <thuth@redhat.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 9c05d1205114..df1c726294b2 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

Re: [PATCH v6 02/14] KVM: s390: selftest: memop: Replace macros by functions

[Janosch Frank]

On 1/25/23 22:25, Janis Schoetterl-Glausch wrote:
> Replace the DEFAULT_* test helpers by functions, as they don't
> need the exta flexibility.

s/exta/extra/

But if you want I can fix that up.

The __VA_ARGS__ often don't make it easier to understand therefore I'd 
rather have a function so I'm happy this patch removes a bit of the magic:

Acked-by: Janosch Frank <frankja@linux.ibm.com>

> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.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 9c05d1205114..df1c726294b2 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);
>   }

[PATCH v6 03/14] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 131 +++++++++++-----------
 1 file changed, 66 insertions(+), 65 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index df1c726294b2..bbc191a13760 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -690,85 +690,86 @@ 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;
 
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_MEM_OP));
+	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
 
-	ksft_print_header();
+	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

Re: [PATCH v6 03/14] KVM: s390: selftest: memop: Move testlist into main

[Janosch Frank]

On 1/25/23 22:25, 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

[PATCH v6 04/14] 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 bbc191a13760..5aae27549437 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -641,7 +641,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

Re: [PATCH v6 04/14] KVM: s390: selftest: memop: Add bad address test

[Janosch Frank]

On 1/25/23 22:25, Janis Schoetterl-Glausch wrote:
> Add test that tries to access, instead of CHECK_ONLY.
""
Add a test that tries a real write to a bad address.
A CHECK_ONLY test doesn't cover all paths.
""

At first I thought you were replacing a test.

> 
> 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 bbc191a13760..5aae27549437 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -641,7 +641,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");

"ioctl does not report bad guest memory address on CHECK_ONLY write" ?

> +	rv = ERR_MOP(info, target, WRITE, mem1, size, GADDR((void *)~0xfffUL));
> +	TEST_ASSERT(rv > 0, "ioctl does not report bad guest memory address");

"ioctl does not report bad guest memory address on write" ?

Not really necessary in this case, it just needs to be different from 
the one on top.

>   
>   	/* Bad host address: */
>   	rv = ERR_MOP(info, target, WRITE, 0, size, GADDR_V(mem1));

[PATCH v6 05/14] 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 5aae27549437..1f6008d87518 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -602,7 +602,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 v6 06/14] 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 1f6008d87518..3ec501881c7c 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -450,9 +450,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

[PATCH v6 07/14] KVM: s390: selftest: memop: Fix integer literal

[Janis Schoetterl-Glausch]

The address is a 64 bit value, specifying a 32 bit value can crash the
guest. In this case things worked out with -O2 but not -O0.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Fixes: 1bb873495a9e ("KVM: s390: selftests: Add more copy memop tests")
---
 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 3ec501881c7c..55b856c4d656 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -514,7 +514,7 @@ static void guest_copy_key_fetch_prot_override(void)
 	GUEST_SYNC(STAGE_INITED);
 	set_storage_key_range(0, PAGE_SIZE, 0x18);
 	set_storage_key_range((void *)last_page_addr, PAGE_SIZE, 0x0);
-	asm volatile ("sske %[key],%[addr]\n" :: [addr] "r"(0), [key] "r"(0x18) : "cc");
+	asm volatile ("sske %[key],%[addr]\n" :: [addr] "r"(0L), [key] "r"(0x18) : "cc");
 	GUEST_SYNC(STAGE_SKEYS_SET);
 
 	for (;;) {
-- 
2.34.1

Re: [PATCH v6 07/14] KVM: s390: selftest: memop: Fix integer literal

[Thomas Huth]

On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> The address is a 64 bit value, specifying a 32 bit value can crash the
> guest. In this case things worked out with -O2 but not -O0.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Fixes: 1bb873495a9e ("KVM: s390: selftests: Add more copy memop tests")
> ---
>   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 3ec501881c7c..55b856c4d656 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -514,7 +514,7 @@ static void guest_copy_key_fetch_prot_override(void)
>   	GUEST_SYNC(STAGE_INITED);
>   	set_storage_key_range(0, PAGE_SIZE, 0x18);
>   	set_storage_key_range((void *)last_page_addr, PAGE_SIZE, 0x0);
> -	asm volatile ("sske %[key],%[addr]\n" :: [addr] "r"(0), [key] "r"(0x18) : "cc");
> +	asm volatile ("sske %[key],%[addr]\n" :: [addr] "r"(0L), [key] "r"(0x18) : "cc");
>   	GUEST_SYNC(STAGE_SKEYS_SET);
>   
>   	for (;;) {

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

[PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions

[Janis Schoetterl-Glausch]

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.

Suggested-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 38 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e4890e04b210..e0dfaa195949 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
 	return r;
 }
 
-static bool access_key_invalid(u8 access_key)
+static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
 {
-	return access_key > 0xf;
+	if (mop->flags & ~supported_flags || !mop->size)
+		return -EINVAL;
+	if (mop->size > MEM_OP_MAX_SIZE)
+		return -E2BIG;
+	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
+		if (mop->key > 0xf)
+			return -EINVAL;
+	} else {
+		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)
 {
 	void __user *uaddr = (void __user *)mop->buf;
-	u64 supported_flags;
 	void *tmpbuf = NULL;
 	int r, srcu_idx;
 
-	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
-			  | KVM_S390_MEMOP_F_CHECK_ONLY;
-	if (mop->flags & ~supported_flags || !mop->size)
-		return -EINVAL;
-	if (mop->size > MEM_OP_MAX_SIZE)
-		return -E2BIG;
+	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
+					KVM_S390_MEMOP_F_CHECK_ONLY);
+	if (r)
+		return r;
+
 	/*
 	 * This is technically a heuristic only, if the kvm->lock is not
 	 * taken, it is not guaranteed that the vm is/remains non-protected.
@@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	 */
 	if (kvm_s390_pv_get_handle(kvm))
 		return -EINVAL;
-	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
-		if (access_key_invalid(mop->key))
-			return -EINVAL;
-	} 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);
 
@@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
 {
 	void __user *uaddr = (void __user *)mop->buf;
 	void *tmpbuf = NULL;
-	int r = 0;
-	const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
-				    | KVM_S390_MEMOP_F_CHECK_ONLY
-				    | KVM_S390_MEMOP_F_SKEY_PROTECTION;
+	int r;
 
-	if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
+	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_INJECT_EXCEPTION |
+					KVM_S390_MEMOP_F_CHECK_ONLY |
+					KVM_S390_MEMOP_F_SKEY_PROTECTION);
+	if (r)
+		return r;
+	if (mop->ar >= NUM_ACRS)
 		return -EINVAL;
-	if (mop->size > MEM_OP_MAX_SIZE)
-		return -E2BIG;
 	if (kvm_s390_pv_cpu_is_protected(vcpu))
 		return -EINVAL;
-	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
-		if (access_key_invalid(mop->key))
-			return -EINVAL;
-	} 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:
-- 
2.34.1

Re: [PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions

[Thomas Huth]

On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> 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.
> 
> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
>   1 file changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e4890e04b210..e0dfaa195949 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   	return r;
>   }
>   
> -static bool access_key_invalid(u8 access_key)
> +static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
>   {
> -	return access_key > 0xf;
> +	if (mop->flags & ~supported_flags || !mop->size)
> +		return -EINVAL;
> +	if (mop->size > MEM_OP_MAX_SIZE)
> +		return -E2BIG;
> +	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> +		if (mop->key > 0xf)
> +			return -EINVAL;
> +	} else {
> +		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)
>   {
>   	void __user *uaddr = (void __user *)mop->buf;
> -	u64 supported_flags;
>   	void *tmpbuf = NULL;

You likely can now remove the "= NULL" here, I guess?

>   	int r, srcu_idx;
>   
> -	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
> -	if (mop->flags & ~supported_flags || !mop->size)
> -		return -EINVAL;
> -	if (mop->size > MEM_OP_MAX_SIZE)
> -		return -E2BIG;
> +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
> +					KVM_S390_MEMOP_F_CHECK_ONLY);
> +	if (r)
> +		return r;
> +
>   	/*
>   	 * This is technically a heuristic only, if the kvm->lock is not
>   	 * taken, it is not guaranteed that the vm is/remains non-protected.
> @@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   	 */
>   	if (kvm_s390_pv_get_handle(kvm))
>   		return -EINVAL;
> -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> -		if (access_key_invalid(mop->key))
> -			return -EINVAL;
> -	} 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);
>   
> @@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>   {
>   	void __user *uaddr = (void __user *)mop->buf;
>   	void *tmpbuf = NULL;

... and here, too.

But I have to admit that I'm also not sure whether I like the 
mem_op_alloc_buf() part or not (the mem_op_validate_common() part looks fine 
to me) : mem_op_alloc_buf() is a new function with 11 lines of code, and the 
old spots that allocate memory were only 5 lines of code each, so you now 
increased the LoC count and additionally have to fiddly with IS_ERR and 
PTR_ERR which is always a little bit ugly in my eyes ... IMHO I'd rather 
keep the old code here. But that's just my 0.02 €, if you think it's nicer 
with mem_op_alloc_buf(), I won't insist on keeping the old code.

  Thomas


> -	int r = 0;
> -	const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
> -				    | KVM_S390_MEMOP_F_CHECK_ONLY
> -				    | KVM_S390_MEMOP_F_SKEY_PROTECTION;
> +	int r;
>   
> -	if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
> +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_INJECT_EXCEPTION |
> +					KVM_S390_MEMOP_F_CHECK_ONLY |
> +					KVM_S390_MEMOP_F_SKEY_PROTECTION);
> +	if (r)
> +		return r;
> +	if (mop->ar >= NUM_ACRS)
>   		return -EINVAL;
> -	if (mop->size > MEM_OP_MAX_SIZE)
> -		return -E2BIG;
>   	if (kvm_s390_pv_cpu_is_protected(vcpu))
>   		return -EINVAL;
> -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> -		if (access_key_invalid(mop->key))
> -			return -EINVAL;
> -	} 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:

Re: [PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions

[Janosch Frank]

On 1/26/23 07:48, Thomas Huth wrote:
> On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
>> 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.
>>
>> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>    arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
>>    1 file changed, 42 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index e4890e04b210..e0dfaa195949 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>    	return r;
>>    }
>>    
>> -static bool access_key_invalid(u8 access_key)
>> +static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
>>    {
>> -	return access_key > 0xf;
>> +	if (mop->flags & ~supported_flags || !mop->size)
>> +		return -EINVAL;
>> +	if (mop->size > MEM_OP_MAX_SIZE)
>> +		return -E2BIG;
>> +	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>> +		if (mop->key > 0xf)
>> +			return -EINVAL;
>> +	} else {
>> +		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)
>>    {
>>    	void __user *uaddr = (void __user *)mop->buf;
>> -	u64 supported_flags;
>>    	void *tmpbuf = NULL;
> 
> You likely can now remove the "= NULL" here, I guess?
> 
>>    	int r, srcu_idx;
>>    
>> -	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
>> -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
>> -	if (mop->flags & ~supported_flags || !mop->size)
>> -		return -EINVAL;
>> -	if (mop->size > MEM_OP_MAX_SIZE)
>> -		return -E2BIG;
>> +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
>> +					KVM_S390_MEMOP_F_CHECK_ONLY);
>> +	if (r)
>> +		return r;
>> +
>>    	/*
>>    	 * This is technically a heuristic only, if the kvm->lock is not
>>    	 * taken, it is not guaranteed that the vm is/remains non-protected.
>> @@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>>    	 */
>>    	if (kvm_s390_pv_get_handle(kvm))
>>    		return -EINVAL;
>> -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
>> -		if (access_key_invalid(mop->key))
>> -			return -EINVAL;
>> -	} 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);
>>    
>> @@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>>    {
>>    	void __user *uaddr = (void __user *)mop->buf;
>>    	void *tmpbuf = NULL;
> 
> ... and here, too.
> 
> But I have to admit that I'm also not sure whether I like the
> mem_op_alloc_buf() part or not (the mem_op_validate_common() part looks fine
> to me) : mem_op_alloc_buf() is a new function with 11 lines of code, and the
> old spots that allocate memory were only 5 lines of code each, so you now
> increased the LoC count and additionally have to fiddly with IS_ERR and
> PTR_ERR which is always a little bit ugly in my eyes ... IMHO I'd rather
> keep the old code here. But that's just my 0.02 €, if you think it's nicer
> with mem_op_alloc_buf(), I won't insist on keeping the old code.
> 
>    Thomas
> 

I've done a PoC that has a **buff argument and combines the check with 
the alloc.

@Nina: Any reason why this was split up?

Re: [PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions

[Janis Schoetterl-Glausch]

On Thu, 2023-01-26 at 14:02 +0100, Janosch Frank wrote:
> On 1/26/23 07:48, Thomas Huth wrote:
> > On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> > > 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.
> > > 
> > > Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > > ---
> > >    arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
> > >    1 file changed, 42 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > > index e4890e04b210..e0dfaa195949 100644
> > > --- a/arch/s390/kvm/kvm-s390.c
> > > +++ b/arch/s390/kvm/kvm-s390.c
> > > @@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> > >    	return r;
> > >    }
> > >    
> > > -static bool access_key_invalid(u8 access_key)
> > > +static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
> > >    {
> > > -	return access_key > 0xf;
> > > +	if (mop->flags & ~supported_flags || !mop->size)
> > > +		return -EINVAL;
> > > +	if (mop->size > MEM_OP_MAX_SIZE)
> > > +		return -E2BIG;
> > > +	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> > > +		if (mop->key > 0xf)
> > > +			return -EINVAL;
> > > +	} else {
> > > +		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)
> > >    {
> > >    	void __user *uaddr = (void __user *)mop->buf;
> > > -	u64 supported_flags;
> > >    	void *tmpbuf = NULL;
> > 
> > You likely can now remove the "= NULL" here, I guess?
> > 
> > >    	int r, srcu_idx;
> > >    
> > > -	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> > > -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
> > > -	if (mop->flags & ~supported_flags || !mop->size)
> > > -		return -EINVAL;
> > > -	if (mop->size > MEM_OP_MAX_SIZE)
> > > -		return -E2BIG;
> > > +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
> > > +					KVM_S390_MEMOP_F_CHECK_ONLY);
> > > +	if (r)
> > > +		return r;
> > > +
> > >    	/*
> > >    	 * This is technically a heuristic only, if the kvm->lock is not
> > >    	 * taken, it is not guaranteed that the vm is/remains non-protected.
> > > @@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> > >    	 */
> > >    	if (kvm_s390_pv_get_handle(kvm))
> > >    		return -EINVAL;
> > > -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> > > -		if (access_key_invalid(mop->key))
> > > -			return -EINVAL;
> > > -	} 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);
> > >    
> > > @@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
> > >    {
> > >    	void __user *uaddr = (void __user *)mop->buf;
> > >    	void *tmpbuf = NULL;
> > 
> > ... and here, too.
> > 
> > But I have to admit that I'm also not sure whether I like the
> > mem_op_alloc_buf() part or not (the mem_op_validate_common() part looks fine
> > to me) : mem_op_alloc_buf() is a new function with 11 lines of code, and the
> > old spots that allocate memory were only 5 lines of code each, so you now
> > increased the LoC count and additionally have to fiddly with IS_ERR and
> > PTR_ERR which is always a little bit ugly in my eyes ... IMHO I'd rather
> > keep the old code here. But that's just my 0.02 €, if you think it's nicer
> > with mem_op_alloc_buf(), I won't insist on keeping the old code.
> > 
> >    Thomas
> > 
> 
> I've done a PoC that has a **buff argument and combines the check with 
> the alloc.

I just didn't like that much because it felt like an unspecific memop_do_things function.

Re: [PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions

[Janis Schoetterl-Glausch]

On Thu, 2023-01-26 at 07:48 +0100, Thomas Huth wrote:
> On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> > 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.
> > 
> > Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> >   arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
> >   1 file changed, 42 insertions(+), 38 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index e4890e04b210..e0dfaa195949 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> >   	return r;
> >   }
> >   
> > -static bool access_key_invalid(u8 access_key)
> > +static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
> >   {
> > -	return access_key > 0xf;
> > +	if (mop->flags & ~supported_flags || !mop->size)
> > +		return -EINVAL;
> > +	if (mop->size > MEM_OP_MAX_SIZE)
> > +		return -E2BIG;
> > +	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> > +		if (mop->key > 0xf)
> > +			return -EINVAL;
> > +	} else {
> > +		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)
> >   {
> >   	void __user *uaddr = (void __user *)mop->buf;
> > -	u64 supported_flags;
> >   	void *tmpbuf = NULL;
> 
> You likely can now remove the "= NULL" here, I guess?

Yeah, I thought about it, but wasn't sure if I like moving the line down because of
some people's insistence on reverse christmas tree.
It's entirely arbitrary in a different way, but I like the return value being the last
thing declared.
In the end I forgot to make a decision on it.

> 
> >   	int r, srcu_idx;
> >   
> > -	supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> > -			  | KVM_S390_MEMOP_F_CHECK_ONLY;
> > -	if (mop->flags & ~supported_flags || !mop->size)
> > -		return -EINVAL;
> > -	if (mop->size > MEM_OP_MAX_SIZE)
> > -		return -E2BIG;
> > +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
> > +					KVM_S390_MEMOP_F_CHECK_ONLY);
> > +	if (r)
> > +		return r;
> > +
> >   	/*
> >   	 * This is technically a heuristic only, if the kvm->lock is not
> >   	 * taken, it is not guaranteed that the vm is/remains non-protected.
> > @@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >   	 */
> >   	if (kvm_s390_pv_get_handle(kvm))
> >   		return -EINVAL;
> > -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> > -		if (access_key_invalid(mop->key))
> > -			return -EINVAL;
> > -	} 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);
> >   
> > @@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
> >   {
> >   	void __user *uaddr = (void __user *)mop->buf;
> >   	void *tmpbuf = NULL;
> 
> ... and here, too.
> 
> But I have to admit that I'm also not sure whether I like the 
> mem_op_alloc_buf() part or not (the mem_op_validate_common() part looks fine 
> to me) : mem_op_alloc_buf() is a new function with 11 lines of code, and the 
> old spots that allocate memory were only 5 lines of code each, so you now 
> increased the LoC count and additionally have to fiddly with IS_ERR and 
> PTR_ERR which is always a little bit ugly in my eyes ... IMHO I'd rather 
> keep the old code here. But that's just my 0.02 €, if you think it's nicer 
> with mem_op_alloc_buf(), I won't insist on keeping the old code.

Yeah, that's fair.

> 
>   Thomas
> 
> 
> > -	int r = 0;
> > -	const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
> > -				    | KVM_S390_MEMOP_F_CHECK_ONLY
> > -				    | KVM_S390_MEMOP_F_SKEY_PROTECTION;
> > +	int r;
> >   
> > -	if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
> > +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_INJECT_EXCEPTION |
> > +					KVM_S390_MEMOP_F_CHECK_ONLY |
> > +					KVM_S390_MEMOP_F_SKEY_PROTECTION);
> > +	if (r)
> > +		return r;
> > +	if (mop->ar >= NUM_ACRS)
> >   		return -EINVAL;
> > -	if (mop->size > MEM_OP_MAX_SIZE)
> > -		return -E2BIG;
> >   	if (kvm_s390_pv_cpu_is_protected(vcpu))
> >   		return -EINVAL;
> > -	if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> > -		if (access_key_invalid(mop->key))
> > -			return -EINVAL;
> > -	} 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:
> 

[PATCH v6 09/14] KVM: s390: Dispatch to implementing function at top level of vm mem_op

[Janis Schoetterl-Glausch]

Instead of having one function covering all mem_op operations,
have a function implementing absolute access and dispatch to that
function in its caller, based on the operation code.
This way additional future operations can be implemented by adding an
implementing function without changing existing operations.

Suggested-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e0dfaa195949..588cf70dc81e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2791,7 +2791,7 @@ static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
 	return buf;
 }
 
-static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
+static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 {
 	void __user *uaddr = (void __user *)mop->buf;
 	void *tmpbuf = NULL;
@@ -2802,17 +2802,6 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	if (r)
 		return r;
 
-	/*
-	 * This is technically a heuristic only, if the kvm->lock is not
-	 * taken, it is not guaranteed that the vm is/remains non-protected.
-	 * This is ok from a kernel perspective, wrongdoing is detected
-	 * on the access, -EFAULT is returned and the vm may crash the
-	 * next time it accesses the memory in question.
-	 * There is no sane usecase to do switching and a memop on two
-	 * different CPUs at the same time.
-	 */
-	if (kvm_s390_pv_get_handle(kvm))
-		return -EINVAL;
 	tmpbuf = mem_op_alloc_buf(mop);
 	if (IS_ERR(tmpbuf))
 		return PTR_ERR(tmpbuf);
@@ -2851,8 +2840,6 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 		}
 		break;
 	}
-	default:
-		r = -EINVAL;
 	}
 
 out_unlock:
@@ -2862,6 +2849,29 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	return r;
 }
 
+static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
+{
+	/*
+	 * This is technically a heuristic only, if the kvm->lock is not
+	 * taken, it is not guaranteed that the vm is/remains non-protected.
+	 * This is ok from a kernel perspective, wrongdoing is detected
+	 * on the access, -EFAULT is returned and the vm may crash the
+	 * next time it accesses the memory in question.
+	 * There is no sane usecase to do switching and a memop on two
+	 * different CPUs at the same time.
+	 */
+	if (kvm_s390_pv_get_handle(kvm))
+		return -EINVAL;
+
+	switch (mop->op) {
+	case KVM_S390_MEMOP_ABSOLUTE_READ:
+	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
+		return kvm_s390_vm_mem_op_abs(kvm, mop);
+	default:
+		return -EINVAL;
+	}
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
-- 
2.34.1

Re: [PATCH v6 09/14] KVM: s390: Dispatch to implementing function at top level of vm mem_op

[Thomas Huth]

On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> Instead of having one function covering all mem_op operations,
> have a function implementing absolute access and dispatch to that
> function in its caller, based on the operation code.
> This way additional future operations can be implemented by adding an
> implementing function without changing existing operations.
> 
> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)

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

[PATCH v6 10/14] KVM: s390: Refactor absolute vm mem_op function

[Janis Schoetterl-Glausch]

Remove code duplication with regards to the CHECK_ONLY flag.
Decrease the number of indents.
No functional change indented.

Suggested-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---


Cosmetic only, can be dropped.


 arch/s390/kvm/kvm-s390.c | 43 ++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 588cf70dc81e..cfd09cb43ef6 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2794,6 +2794,7 @@ static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
 static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 {
 	void __user *uaddr = (void __user *)mop->buf;
+	enum gacc_mode acc_mode;
 	void *tmpbuf = NULL;
 	int r, srcu_idx;
 
@@ -2813,33 +2814,23 @@ static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 		goto out_unlock;
 	}
 
-	switch (mop->op) {
-	case KVM_S390_MEMOP_ABSOLUTE_READ: {
-		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
-			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_FETCH, mop->key);
-		} else {
-			r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
-						      mop->size, GACC_FETCH, mop->key);
-			if (r == 0) {
-				if (copy_to_user(uaddr, tmpbuf, mop->size))
-					r = -EFAULT;
-			}
-		}
-		break;
-	}
-	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 (copy_from_user(tmpbuf, uaddr, mop->size)) {
-				r = -EFAULT;
-				break;
-			}
-			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) {
+		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
+					      mop->size, GACC_FETCH, mop->key);
+		if (r)
+			goto out_unlock;
+		if (copy_to_user(uaddr, tmpbuf, mop->size))
+			r = -EFAULT;
+	} else {
+		if (copy_from_user(tmpbuf, uaddr, mop->size)) {
+			r = -EFAULT;
+			goto out_unlock;
 		}
-		break;
-	}
+		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
+					      mop->size, GACC_STORE, mop->key);
 	}
 
 out_unlock:
-- 
2.34.1

Re: [PATCH v6 10/14] KVM: s390: Refactor absolute vm mem_op function

[Thomas Huth]

On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
> Remove code duplication with regards to the CHECK_ONLY flag.
> Decrease the number of indents.
> No functional change indented.
> 
> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> 
> Cosmetic only, can be dropped.

I'm torn between unnecessary-code-churn and 
nice-to-get-rid-of-one-indentation-level here ... anyway, patch looks sane 
to me, so:

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

Re: [PATCH v6 10/14] KVM: s390: Refactor absolute vm mem_op function

[Janosch Frank]

On 1/26/23 13:18, Thomas Huth wrote:
> On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
>> Remove code duplication with regards to the CHECK_ONLY flag.
>> Decrease the number of indents.
>> No functional change indented.
>>
>> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>
>>
>> Cosmetic only, can be dropped.
> 
> I'm torn between unnecessary-code-churn and
> nice-to-get-rid-of-one-indentation-level here ... anyway, patch looks sane
> to me, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

As long as we're not adding to this function in the future then I'm 
okish with leaving it as is.

Re: [PATCH v6 10/14] KVM: s390: Refactor absolute vm mem_op function

[Janosch Frank]

On 1/25/23 22:26, Janis Schoetterl-Glausch wrote:
> Remove code duplication with regards to the CHECK_ONLY flag.
> Decrease the number of indents.
> No functional change indented.
> 
> Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> 
> Cosmetic only, can be dropped.
> 
> 
>   arch/s390/kvm/kvm-s390.c | 43 ++++++++++++++++------------------------
>   1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 588cf70dc81e..cfd09cb43ef6 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2794,6 +2794,7 @@ static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
>   static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   {
>   	void __user *uaddr = (void __user *)mop->buf;
> +	enum gacc_mode acc_mode;
>   	void *tmpbuf = NULL;
>   	int r, srcu_idx;
>   
> @@ -2813,33 +2814,23 @@ static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   		goto out_unlock;
>   	}
>   
> -	switch (mop->op) {
> -	case KVM_S390_MEMOP_ABSOLUTE_READ: {
> -		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> -			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_FETCH, mop->key);
> -		} else {
> -			r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> -						      mop->size, GACC_FETCH, mop->key);
> -			if (r == 0) {
> -				if (copy_to_user(uaddr, tmpbuf, mop->size))
> -					r = -EFAULT;
> -			}
> -		}
> -		break;
> -	}
> -	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 (copy_from_user(tmpbuf, uaddr, mop->size)) {
> -				r = -EFAULT;
> -				break;
> -			}
> -			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;

Would the line be too long if that variable would be initialized where 
it's defined?

> +	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> +		r = check_gpa_range(kvm, mop->gaddr, mop->size, acc_mode, mop->key);

We should early return i.e. goto out_unlock.

IMHO else if, else patterns should either be switches (testing the same 
variable) or kept as short as possible / be avoided.

> +	} else if (acc_mode == GACC_FETCH) {
> +		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> +					      mop->size, GACC_FETCH, mop->key);

I'd guess it's personal taste whether you use GACC_FETCH or access_mode 
but if you don't use it here then we can remove the variable all 
together, no?

> +		if (r)
> +			goto out_unlock;
> +		if (copy_to_user(uaddr, tmpbuf, mop->size))
> +			r = -EFAULT;
> +	} else {
> +		if (copy_from_user(tmpbuf, uaddr, mop->size)) {
> +			r = -EFAULT;
> +			goto out_unlock;
>   		}
> -		break;
> -	}
> +		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> +					      mop->size, GACC_STORE, mop->key);
>   	}
>   
>   out_unlock:

Re: [PATCH v6 10/14] KVM: s390: Refactor absolute vm mem_op function

[Janis Schoetterl-Glausch]

On Fri, 2023-02-03 at 15:48 +0100, Janosch Frank wrote:
> On 1/25/23 22:26, Janis Schoetterl-Glausch wrote:
> > Remove code duplication with regards to the CHECK_ONLY flag.
> > Decrease the number of indents.
> > No functional change indented.
> > 
> > Suggested-by: Janosch Frank <frankja@linux.ibm.com>
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> > 
> > 
> > Cosmetic only, can be dropped.
> > 
> > 
> >   arch/s390/kvm/kvm-s390.c | 43 ++++++++++++++++------------------------
> >   1 file changed, 17 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 588cf70dc81e..cfd09cb43ef6 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2794,6 +2794,7 @@ static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
> >   static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >   {
> >   	void __user *uaddr = (void __user *)mop->buf;
> > +	enum gacc_mode acc_mode;
> >   	void *tmpbuf = NULL;
> >   	int r, srcu_idx;
> >   
> > @@ -2813,33 +2814,23 @@ static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> >   		goto out_unlock;
> >   	}
> >   
> > -	switch (mop->op) {
> > -	case KVM_S390_MEMOP_ABSOLUTE_READ: {
> > -		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> > -			r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_FETCH, mop->key);
> > -		} else {
> > -			r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> > -						      mop->size, GACC_FETCH, mop->key);
> > -			if (r == 0) {
> > -				if (copy_to_user(uaddr, tmpbuf, mop->size))
> > -					r = -EFAULT;
> > -			}
> > -		}
> > -		break;
> > -	}
> > -	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 (copy_from_user(tmpbuf, uaddr, mop->size)) {
> > -				r = -EFAULT;
> > -				break;
> > -			}
> > -			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;
> 
> Would the line be too long if that variable would be initialized where 
> it's defined?

Just fits at 100 columns. Want me to move it?

> 
> > +	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> > +		r = check_gpa_range(kvm, mop->gaddr, mop->size, acc_mode, mop->key);
> 
> We should early return i.e. goto out_unlock.
> 
> IMHO else if, else patterns should either be switches (testing the same 
> variable) or kept as short as possible / be avoided.
> 
> > +	} else if (acc_mode == GACC_FETCH) {
> > +		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> > +					      mop->size, GACC_FETCH, mop->key);
> 
> I'd guess it's personal taste whether you use GACC_FETCH or access_mode 
> but if you don't use it here then we can remove the variable all 
> together, no?

Yeah, I think I did replace it, but then undid it.
Probably just because it is a bit more explicit.
It's used in check_gpa_range, so no, unless you want to dump the expression
directly in there.
> 
> > +		if (r)
> > +			goto out_unlock;
> > +		if (copy_to_user(uaddr, tmpbuf, mop->size))
> > +			r = -EFAULT;
> > +	} else {
> > +		if (copy_from_user(tmpbuf, uaddr, mop->size)) {
> > +			r = -EFAULT;
> > +			goto out_unlock;
> >   		}
> > -		break;
> > -	}
> > +		r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> > +					      mop->size, GACC_STORE, mop->key);
> >   	}
> >   
> >   out_unlock:
> 

[PATCH v6 11/14] KVM: s390: Refactor absolute vcpu mem_op function

[Janis Schoetterl-Glausch]

Remove code duplication with regards to the CHECK_ONLY flag.
Decrease the number of indents.
No functional change indented.

Suggested-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---


Cosmetic only, can be dropped.


 arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index cfd09cb43ef6..4b8b41be7aed 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5262,6 +5262,7 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
 				 struct kvm_s390_mem_op *mop)
 {
 	void __user *uaddr = (void __user *)mop->buf;
+	enum gacc_mode acc_mode;
 	void *tmpbuf = NULL;
 	int r;
 
@@ -5278,38 +5279,33 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
 	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) {
-			r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
-					    GACC_FETCH, mop->key);
-			break;
-		}
+	acc_mode = mop->op == KVM_S390_MEMOP_LOGICAL_READ ? GACC_FETCH : GACC_STORE;
+	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) {
 		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
 					mop->size, mop->key);
-		if (r == 0) {
-			if (copy_to_user(uaddr, tmpbuf, mop->size))
-				r = -EFAULT;
-		}
-		break;
-	case KVM_S390_MEMOP_LOGICAL_WRITE:
-		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
-			r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
-					    GACC_STORE, mop->key);
-			break;
+		if (r)
+			goto out_inject;
+		if (copy_to_user(uaddr, tmpbuf, mop->size)) {
+			r = -EFAULT;
+			goto out_free;
 		}
+	} else {
 		if (copy_from_user(tmpbuf, uaddr, mop->size)) {
 			r = -EFAULT;
-			break;
+			goto out_free;
 		}
 		r = write_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
 					 mop->size, mop->key);
-		break;
 	}
 
+out_inject:
 	if (r > 0 && (mop->flags & KVM_S390_MEMOP_F_INJECT_EXCEPTION) != 0)
 		kvm_s390_inject_prog_irq(vcpu, &vcpu->arch.pgm);
 
+out_free:
 	vfree(tmpbuf);
 	return r;
 }
-- 
2.34.1

[PATCH v6 12/14] 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
op. For now, support this op for absolute accesses only.

This op 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 |   8 +++
 arch/s390/kvm/gaccess.h  |   3 ++
 arch/s390/kvm/gaccess.c  | 103 +++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c |  56 ++++++++++++++++++++-
 4 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 55155e262646..d2f30463c133 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -583,6 +583,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_addr;	/* ignored if cmpxchg flag unset */
 		};
 		__u32 sida_offset; /* offset into the sida */
 		__u8 reserved[32]; /* ignored */
@@ -595,11 +597,17 @@ struct kvm_s390_mem_op {
 #define KVM_S390_MEMOP_SIDA_WRITE	3
 #define KVM_S390_MEMOP_ABSOLUTE_READ	4
 #define KVM_S390_MEMOP_ABSOLUTE_WRITE	5
+#define KVM_S390_MEMOP_ABSOLUTE_CMPXCHG	6
+
 /* flags for kvm_s390_mem_op->flags */
 #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)
 
+/* flags specifying extension support via KVM_CAP_S390_MEM_OP_EXTENSION */
+#define KVM_S390_MEMOP_EXTENSION_CAP_BASE	(1 << 0)
+#define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG	(1 << 1)
+
 /* for KVM_INTERRUPT */
 struct kvm_interrupt {
 	/* in */
diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 9408d6cc8e2c..b320d12aa049 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, bool *success);
+
 /**
  * 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..74cb728a0f24 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1161,6 +1161,109 @@ 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_addr: Pointer to old value. If the location at @gpa contains this value,
+ *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
+ *            *@old_addr 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.
+ * @success: output value indicating if an exchange occurred.
+ *
+ * Atomically exchange the value at @gpa by @new, if it contains *@old.
+ * Honors storage keys.
+ *
+ * Return: * 0: successful exchange
+ *         * 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)
+ */
+int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
+			       __uint128_t *old_addr, __uint128_t new,
+			       u8 access_key, bool *success)
+{
+	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_addr, new, access_key);
+		*success = !ret && old == *old_addr;
+		*old_addr = old;
+		break;
+	}
+	case 2: {
+		u16 old;
+
+		ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
+		*success = !ret && old == *old_addr;
+		*old_addr = old;
+		break;
+	}
+	case 4: {
+		u32 old;
+
+		ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
+		*success = !ret && old == *old_addr;
+		*old_addr = old;
+		break;
+	}
+	case 8: {
+		u64 old;
+
+		ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
+		*success = !ret && old == *old_addr;
+		*old_addr = old;
+		break;
+	}
+	case 16: {
+		__uint128_t old;
+
+		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
+		*success = !ret && old == *old_addr;
+		*old_addr = 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 4b8b41be7aed..86e9734d5782 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -584,7 +584,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:
@@ -598,6 +597,15 @@ 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.
+		 * If r > 0, the base extension must also be supported/indicated,
+		 * in order to maintain backwards compatibility.
+		 */
+		r = KVM_S390_MEMOP_EXTENSION_CAP_BASE |
+		    KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG;
+		break;
 	case KVM_CAP_NR_VCPUS:
 	case KVM_CAP_MAX_VCPUS:
 	case KVM_CAP_MAX_VCPU_ID:
@@ -2840,6 +2848,50 @@ static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	return r;
 }
 
+static int kvm_s390_vm_mem_op_cmpxchg(struct kvm *kvm, struct kvm_s390_mem_op *mop)
+{
+	void __user *uaddr = (void __user *)mop->buf;
+	void __user *old_addr = (void __user *)mop->old_addr;
+	union {
+		__uint128_t quad;
+		char raw[sizeof(__uint128_t)];
+	} old = { .quad = 0}, new = { .quad = 0 };
+	unsigned int off_in_quad = sizeof(new) - mop->size;
+	int r, srcu_idx;
+	bool success;
+
+	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION);
+	if (r)
+		return r;
+	/*
+	 * This validates off_in_quad. Checking that size is a power
+	 * of two is not necessary, as cmpxchg_guest_abs_with_key
+	 * takes care of that
+	 */
+	if (mop->size > sizeof(new))
+		return -EINVAL;
+	if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
+		return -EFAULT;
+	if (copy_from_user(&old.raw[off_in_quad], old_addr, mop->size))
+		return -EFAULT;
+
+	srcu_idx = srcu_read_lock(&kvm->srcu);
+
+	if (kvm_is_error_gpa(kvm, mop->gaddr)) {
+		r = PGM_ADDRESSING;
+		goto out_unlock;
+	}
+
+	r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size, &old.quad,
+				       new.quad, mop->key, &success);
+	if (!success && copy_to_user(old_addr, &old.raw[off_in_quad], mop->size))
+		r = -EFAULT;
+
+out_unlock:
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
+	return r;
+}
+
 static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 {
 	/*
@@ -2858,6 +2910,8 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
 	case KVM_S390_MEMOP_ABSOLUTE_READ:
 	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
 		return kvm_s390_vm_mem_op_abs(kvm, mop);
+	case KVM_S390_MEMOP_ABSOLUTE_CMPXCHG:
+		return kvm_s390_vm_mem_op_cmpxchg(kvm, mop);
 	default:
 		return -EINVAL;
 	}
-- 
2.34.1

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Heiko Carstens]

On Wed, Jan 25, 2023 at 10:26:06PM +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
> op. For now, support this op for absolute accesses only.
> 
> This op 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 |   8 +++
>  arch/s390/kvm/gaccess.h  |   3 ++
>  arch/s390/kvm/gaccess.c  | 103 +++++++++++++++++++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.c |  56 ++++++++++++++++++++-
>  4 files changed, 169 insertions(+), 1 deletion(-)
...
> +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);

FWIW, this and the three others need a __user annotation:

		ret = cmpxchg_user_key((u8 __user *)hva, &old, *old_addr, new, access_key);

Otherwise you end up with sparse warnings (compile with C=1).

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janosch Frank]

On 1/25/23 22:26, 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
> op. For now, support this op for absolute accesses only.
> 
> This op can be use, for example, to set the device-state-change

s/use/used/

> indicator and the adapter-local-summary indicator atomically.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
[...]
> +/**
> + * 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_addr: Pointer to old value. If the location at @gpa contains this value,
> + *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
> + *            *@old_addr 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.
> + * @success: output value indicating if an exchange occurred.
> + *
> + * Atomically exchange the value at @gpa by @new, if it contains *@old.
> + * Honors storage keys.
> + *
> + * Return: * 0: successful exchange
> + *         * a program interruption code indicating the reason cmpxchg could
> + *           not be attempted

Nit:
 >0: a program interruption code...


> + *         * -EINVAL: address misaligned or len not power of two
> + *         * -EAGAIN: transient failure (len 1 or 2)
> + *         * -EOPNOTSUPP: read-only memslot (should never occur)
> + */
> +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> +			       __uint128_t *old_addr, __uint128_t new,
> +			       u8 access_key, bool *success)
> +{
> +	gfn_t gfn = gpa >> PAGE_SHIFT;

  gpa_to_gfn()?

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

Hmm if we don't use a macro to generate these then I'd add an explanation:

cmpxchg_user_key() is a macro that is dependent on the type of "old" so 
there's no deduplication possible without further macros.

> +	switch (len) {
> +	case 1: {
> +		u8 old;
> +
> +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
> +		*success = !ret && old == *old_addr;
> +		*old_addr = old;
> +		break;
> +	}
> +	case 2: {
> +		u16 old;
> +
> +		ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
> +		*success = !ret && old == *old_addr;
> +		*old_addr = old;
> +		break;
> +	}
> +	case 4: {
> +		u32 old;
> +
> +		ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
> +		*success = !ret && old == *old_addr;
> +		*old_addr = old;
> +		break;
> +	}
> +	case 8: {
> +		u64 old;
> +
> +		ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
> +		*success = !ret && old == *old_addr;
> +		*old_addr = old;
> +		break;
> +	}
> +	case 16: {
> +		__uint128_t old;
> +
> +		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
> +		*success = !ret && old == *old_addr;
> +		*old_addr = old;
> +		break;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +	mark_page_dirty_in_slot(kvm, slot, gfn);

Is that needed if we failed the store?

> +	/*
> +	 * 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 4b8b41be7aed..86e9734d5782 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -584,7 +584,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:
> @@ -598,6 +597,15 @@ 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.
> +		 * If r > 0, the base extension must also be supported/indicated,
> +		 * in order to maintain backwards compatibility.
> +		 */
> +		r = KVM_S390_MEMOP_EXTENSION_CAP_BASE |
> +		    KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG;
> +		break;
>   	case KVM_CAP_NR_VCPUS:
>   	case KVM_CAP_MAX_VCPUS:
>   	case KVM_CAP_MAX_VCPU_ID:
> @@ -2840,6 +2848,50 @@ static int kvm_s390_vm_mem_op_abs(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   	return r;
>   }
>   
> +static int kvm_s390_vm_mem_op_cmpxchg(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> +{
> +	void __user *uaddr = (void __user *)mop->buf;
> +	void __user *old_addr = (void __user *)mop->old_addr;
> +	union {
> +		__uint128_t quad;
> +		char raw[sizeof(__uint128_t)];
> +	} old = { .quad = 0}, new = { .quad = 0 };
> +	unsigned int off_in_quad = sizeof(new) - mop->size;
> +	int r, srcu_idx;
> +	bool success;
> +
> +	r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION);
> +	if (r)
> +		return r;
> +	/*
> +	 * This validates off_in_quad. Checking that size is a power
> +	 * of two is not necessary, as cmpxchg_guest_abs_with_key
> +	 * takes care of that
> +	 */
> +	if (mop->size > sizeof(new))
> +		return -EINVAL;
> +	if (copy_from_user(&new.raw[off_in_quad], uaddr, mop->size))
> +		return -EFAULT;
> +	if (copy_from_user(&old.raw[off_in_quad], old_addr, mop->size))
> +		return -EFAULT;
> +
> +	srcu_idx = srcu_read_lock(&kvm->srcu);
> +
> +	if (kvm_is_error_gpa(kvm, mop->gaddr)) {
> +		r = PGM_ADDRESSING;
> +		goto out_unlock;
> +	}
> +
> +	r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size, &old.quad,
> +				       new.quad, mop->key, &success);
> +	if (!success && copy_to_user(old_addr, &old.raw[off_in_quad], mop->size))
> +		r = -EFAULT;
> +
> +out_unlock:
> +	srcu_read_unlock(&kvm->srcu, srcu_idx);
> +	return r;
> +}
> +
>   static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   {
>   	/*
> @@ -2858,6 +2910,8 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>   	case KVM_S390_MEMOP_ABSOLUTE_READ:
>   	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
>   		return kvm_s390_vm_mem_op_abs(kvm, mop);
> +	case KVM_S390_MEMOP_ABSOLUTE_CMPXCHG:
> +		return kvm_s390_vm_mem_op_cmpxchg(kvm, mop);
>   	default:
>   		return -EINVAL;
>   	}

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch]

On Thu, 2023-01-26 at 17:10 +0100, Janosch Frank wrote:
> On 1/25/23 22:26, 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
> > op. For now, support this op for absolute accesses only.
> > 
> > This op can be use, for example, to set the device-state-change
> 
> s/use/used/
> 
> > indicator and the adapter-local-summary indicator atomically.
> > 
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> [...]
> > +/**
> > + * 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_addr: Pointer to old value. If the location at @gpa contains this value,
> > + *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
> > + *            *@old_addr 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.
> > + * @success: output value indicating if an exchange occurred.
> > + *
> > + * Atomically exchange the value at @gpa by @new, if it contains *@old.
> > + * Honors storage keys.
> > + *
> > + * Return: * 0: successful exchange
> > + *         * a program interruption code indicating the reason cmpxchg could
> > + *           not be attempted
> 
> Nit:
>  >0: a program interruption code...
> 
> 
> > + *         * -EINVAL: address misaligned or len not power of two
> > + *         * -EAGAIN: transient failure (len 1 or 2)
> > + *         * -EOPNOTSUPP: read-only memslot (should never occur)
> > + */
> > +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> > +			       __uint128_t *old_addr, __uint128_t new,
> > +			       u8 access_key, bool *success)
> > +{
> > +	gfn_t gfn = gpa >> PAGE_SHIFT;
> 
>   gpa_to_gfn()?

Yes.
> 
> > +	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);
> 
> Hmm if we don't use a macro to generate these then I'd add an explanation:
> 
> cmpxchg_user_key() is a macro that is dependent on the type of "old" so 
> there's no deduplication possible without further macros.

Can do.
Btw. I could move the other two statements out of the switch by using a union of old values,
memcmp and memcpy, but I think that would be less readable.

> 
> > +	switch (len) {
> > +	case 1: {
> > +		u8 old;
> > +
> > +		ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
> > +		*success = !ret && old == *old_addr;
> > +		*old_addr = old;
> > +		break;
> > +	}
> > +	case 2: {
> > +		u16 old;
> > +
> > +		ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
> > +		*success = !ret && old == *old_addr;
> > +		*old_addr = old;
> > +		break;
> > +	}
> > +	case 4: {
> > +		u32 old;
> > +
> > +		ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
> > +		*success = !ret && old == *old_addr;
> > +		*old_addr = old;
> > +		break;
> > +	}
> > +	case 8: {
> > +		u64 old;
> > +
> > +		ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
> > +		*success = !ret && old == *old_addr;
> > +		*old_addr = old;
> > +		break;
> > +	}
> > +	case 16: {
> > +		__uint128_t old;
> > +
> > +		ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
> > +		*success = !ret && old == *old_addr;
> > +		*old_addr = old;
> > +		break;
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	mark_page_dirty_in_slot(kvm, slot, gfn);
> 
> Is that needed if we failed the store?

Indeed it isn't.

[...]

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[kernel test robot]

Hi Janis,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on lwn/docs-next linus/master]
[cannot apply to kvms390/next kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230125212608.1860251-13-scgl%40linux.ibm.com
patch subject: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230128/202301281752.6gSopuWh-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6e6b3d99b9978a70b148b989d46b039feda3a3c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
        git checkout 6e6b3d99b9978a70b148b989d46b039feda3a3c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/s390/kvm/gaccess.c: In function 'cmpxchg_guest_abs_with_key':
>> arch/s390/kvm/gaccess.c:1217:23: error: implicit declaration of function 'cmpxchg_user_key'; did you mean 'copy_to_user_key'? [-Werror=implicit-function-declaration]
    1217 |                 ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
         |                       ^~~~~~~~~~~~~~~~
         |                       copy_to_user_key
   cc1: some warnings being treated as errors


vim +1217 arch/s390/kvm/gaccess.c

  1163	
  1164	/**
  1165	 * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
  1166	 * @kvm: Virtual machine instance.
  1167	 * @gpa: Absolute guest address of the location to be changed.
  1168	 * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
  1169	 *       non power of two will result in failure.
  1170	 * @old_addr: Pointer to old value. If the location at @gpa contains this value,
  1171	 *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
  1172	 *            *@old_addr contains the value at @gpa before the attempt to
  1173	 *            exchange the value.
  1174	 * @new: The value to place at @gpa.
  1175	 * @access_key: The access key to use for the guest access.
  1176	 * @success: output value indicating if an exchange occurred.
  1177	 *
  1178	 * Atomically exchange the value at @gpa by @new, if it contains *@old.
  1179	 * Honors storage keys.
  1180	 *
  1181	 * Return: * 0: successful exchange
  1182	 *         * a program interruption code indicating the reason cmpxchg could
  1183	 *           not be attempted
  1184	 *         * -EINVAL: address misaligned or len not power of two
  1185	 *         * -EAGAIN: transient failure (len 1 or 2)
  1186	 *         * -EOPNOTSUPP: read-only memslot (should never occur)
  1187	 */
  1188	int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
  1189				       __uint128_t *old_addr, __uint128_t new,
  1190				       u8 access_key, bool *success)
  1191	{
  1192		gfn_t gfn = gpa >> PAGE_SHIFT;
  1193		struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
  1194		bool writable;
  1195		hva_t hva;
  1196		int ret;
  1197	
  1198		if (!IS_ALIGNED(gpa, len))
  1199			return -EINVAL;
  1200	
  1201		hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
  1202		if (kvm_is_error_hva(hva))
  1203			return PGM_ADDRESSING;
  1204		/*
  1205		 * Check if it's a read-only memslot, even though that cannot occur
  1206		 * since those are unsupported.
  1207		 * Don't try to actually handle that case.
  1208		 */
  1209		if (!writable)
  1210			return -EOPNOTSUPP;
  1211	
  1212		hva += offset_in_page(gpa);
  1213		switch (len) {
  1214		case 1: {
  1215			u8 old;
  1216	
> 1217			ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
  1218			*success = !ret && old == *old_addr;
  1219			*old_addr = old;
  1220			break;
  1221		}
  1222		case 2: {
  1223			u16 old;
  1224	
  1225			ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
  1226			*success = !ret && old == *old_addr;
  1227			*old_addr = old;
  1228			break;
  1229		}
  1230		case 4: {
  1231			u32 old;
  1232	
  1233			ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
  1234			*success = !ret && old == *old_addr;
  1235			*old_addr = old;
  1236			break;
  1237		}
  1238		case 8: {
  1239			u64 old;
  1240	
  1241			ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
  1242			*success = !ret && old == *old_addr;
  1243			*old_addr = old;
  1244			break;
  1245		}
  1246		case 16: {
  1247			__uint128_t old;
  1248	
  1249			ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
  1250			*success = !ret && old == *old_addr;
  1251			*old_addr = old;
  1252			break;
  1253		}
  1254		default:
  1255			return -EINVAL;
  1256		}
  1257		mark_page_dirty_in_slot(kvm, slot, gfn);
  1258		/*
  1259		 * Assume that the fault is caused by protection, either key protection
  1260		 * or user page write protection.
  1261		 */
  1262		if (ret == -EFAULT)
  1263			ret = PGM_PROTECTION;
  1264		return ret;
  1265	}
  1266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[kernel test robot]

Hi Janis,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on lwn/docs-next linus/master v6.2-rc5 next-20230127]
[cannot apply to kvms390/next kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230125212608.1860251-13-scgl%40linux.ibm.com
patch subject: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
config: s390-randconfig-r022-20230123 (https://download.01.org/0day-ci/archive/20230128/202301282258.RvVJOYVA-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6e6b3d99b9978a70b148b989d46b039feda3a3c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
        git checkout 6e6b3d99b9978a70b148b989d46b039feda3a3c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash arch/s390/kvm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/kvm/gaccess.c:1217:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1217:9: note: did you mean 'copy_to_user_key'?
   arch/s390/include/asm/uaccess.h:51:1: note: 'copy_to_user_key' declared here
   copy_to_user_key(void __user *to, const void *from, unsigned long n, unsigned long key)
   ^
   arch/s390/kvm/gaccess.c:1225:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1233:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1241:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1249:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
                         ^
   12 warnings and 5 errors generated.


vim +/cmpxchg_user_key +1217 arch/s390/kvm/gaccess.c

  1163	
  1164	/**
  1165	 * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
  1166	 * @kvm: Virtual machine instance.
  1167	 * @gpa: Absolute guest address of the location to be changed.
  1168	 * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
  1169	 *       non power of two will result in failure.
  1170	 * @old_addr: Pointer to old value. If the location at @gpa contains this value,
  1171	 *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
  1172	 *            *@old_addr contains the value at @gpa before the attempt to
  1173	 *            exchange the value.
  1174	 * @new: The value to place at @gpa.
  1175	 * @access_key: The access key to use for the guest access.
  1176	 * @success: output value indicating if an exchange occurred.
  1177	 *
  1178	 * Atomically exchange the value at @gpa by @new, if it contains *@old.
  1179	 * Honors storage keys.
  1180	 *
  1181	 * Return: * 0: successful exchange
  1182	 *         * a program interruption code indicating the reason cmpxchg could
  1183	 *           not be attempted
  1184	 *         * -EINVAL: address misaligned or len not power of two
  1185	 *         * -EAGAIN: transient failure (len 1 or 2)
  1186	 *         * -EOPNOTSUPP: read-only memslot (should never occur)
  1187	 */
  1188	int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
  1189				       __uint128_t *old_addr, __uint128_t new,
  1190				       u8 access_key, bool *success)
  1191	{
  1192		gfn_t gfn = gpa >> PAGE_SHIFT;
  1193		struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
  1194		bool writable;
  1195		hva_t hva;
  1196		int ret;
  1197	
  1198		if (!IS_ALIGNED(gpa, len))
  1199			return -EINVAL;
  1200	
  1201		hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
  1202		if (kvm_is_error_hva(hva))
  1203			return PGM_ADDRESSING;
  1204		/*
  1205		 * Check if it's a read-only memslot, even though that cannot occur
  1206		 * since those are unsupported.
  1207		 * Don't try to actually handle that case.
  1208		 */
  1209		if (!writable)
  1210			return -EOPNOTSUPP;
  1211	
  1212		hva += offset_in_page(gpa);
  1213		switch (len) {
  1214		case 1: {
  1215			u8 old;
  1216	
> 1217			ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
  1218			*success = !ret && old == *old_addr;
  1219			*old_addr = old;
  1220			break;
  1221		}
  1222		case 2: {
  1223			u16 old;
  1224	
  1225			ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
  1226			*success = !ret && old == *old_addr;
  1227			*old_addr = old;
  1228			break;
  1229		}
  1230		case 4: {
  1231			u32 old;
  1232	
  1233			ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
  1234			*success = !ret && old == *old_addr;
  1235			*old_addr = old;
  1236			break;
  1237		}
  1238		case 8: {
  1239			u64 old;
  1240	
  1241			ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
  1242			*success = !ret && old == *old_addr;
  1243			*old_addr = old;
  1244			break;
  1245		}
  1246		case 16: {
  1247			__uint128_t old;
  1248	
  1249			ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
  1250			*success = !ret && old == *old_addr;
  1251			*old_addr = old;
  1252			break;
  1253		}
  1254		default:
  1255			return -EINVAL;
  1256		}
  1257		mark_page_dirty_in_slot(kvm, slot, gfn);
  1258		/*
  1259		 * Assume that the fault is caused by protection, either key protection
  1260		 * or user page write protection.
  1261		 */
  1262		if (ret == -EFAULT)
  1263			ret = PGM_PROTECTION;
  1264		return ret;
  1265	}
  1266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[kernel test robot]

Hi Janis,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/queue]
[also build test ERROR on lwn/docs-next linus/master v6.2-rc5 next-20230127]
[cannot apply to kvms390/next kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230125212608.1860251-13-scgl%40linux.ibm.com
patch subject: [PATCH v6 12/14] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
config: s390-randconfig-r044-20230123 (https://download.01.org/0day-ci/archive/20230128/202301282223.YafIuB1E-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6e6b3d99b9978a70b148b989d46b039feda3a3c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Janis-Schoetterl-Glausch/KVM-s390-selftest-memop-Pass-mop_desc-via-pointer/20230128-132603
        git checkout 6e6b3d99b9978a70b148b989d46b039feda3a3c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash arch/s390/kvm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from arch/s390/kvm/gaccess.c:16:
   In file included from arch/s390/kvm/kvm-s390.h:17:
   In file included from include/linux/kvm_host.h:19:
   In file included from include/linux/msi.h:27:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/kvm/gaccess.c:1217:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1217:9: note: did you mean 'copy_to_user_key'?
   arch/s390/include/asm/uaccess.h:51:1: note: 'copy_to_user_key' declared here
   copy_to_user_key(void __user *to, const void *from, unsigned long n, unsigned long key)
   ^
   arch/s390/kvm/gaccess.c:1225:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1233:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1241:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
                         ^
   arch/s390/kvm/gaccess.c:1249:9: error: call to undeclared function 'cmpxchg_user_key'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
                         ^
   12 warnings and 5 errors generated.


vim +/cmpxchg_user_key +1217 arch/s390/kvm/gaccess.c

  1163	
  1164	/**
  1165	 * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
  1166	 * @kvm: Virtual machine instance.
  1167	 * @gpa: Absolute guest address of the location to be changed.
  1168	 * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
  1169	 *       non power of two will result in failure.
  1170	 * @old_addr: Pointer to old value. If the location at @gpa contains this value,
  1171	 *            the exchange will succeed. After calling cmpxchg_guest_abs_with_key()
  1172	 *            *@old_addr contains the value at @gpa before the attempt to
  1173	 *            exchange the value.
  1174	 * @new: The value to place at @gpa.
  1175	 * @access_key: The access key to use for the guest access.
  1176	 * @success: output value indicating if an exchange occurred.
  1177	 *
  1178	 * Atomically exchange the value at @gpa by @new, if it contains *@old.
  1179	 * Honors storage keys.
  1180	 *
  1181	 * Return: * 0: successful exchange
  1182	 *         * a program interruption code indicating the reason cmpxchg could
  1183	 *           not be attempted
  1184	 *         * -EINVAL: address misaligned or len not power of two
  1185	 *         * -EAGAIN: transient failure (len 1 or 2)
  1186	 *         * -EOPNOTSUPP: read-only memslot (should never occur)
  1187	 */
  1188	int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
  1189				       __uint128_t *old_addr, __uint128_t new,
  1190				       u8 access_key, bool *success)
  1191	{
  1192		gfn_t gfn = gpa >> PAGE_SHIFT;
  1193		struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
  1194		bool writable;
  1195		hva_t hva;
  1196		int ret;
  1197	
  1198		if (!IS_ALIGNED(gpa, len))
  1199			return -EINVAL;
  1200	
  1201		hva = gfn_to_hva_memslot_prot(slot, gfn, &writable);
  1202		if (kvm_is_error_hva(hva))
  1203			return PGM_ADDRESSING;
  1204		/*
  1205		 * Check if it's a read-only memslot, even though that cannot occur
  1206		 * since those are unsupported.
  1207		 * Don't try to actually handle that case.
  1208		 */
  1209		if (!writable)
  1210			return -EOPNOTSUPP;
  1211	
  1212		hva += offset_in_page(gpa);
  1213		switch (len) {
  1214		case 1: {
  1215			u8 old;
  1216	
> 1217			ret = cmpxchg_user_key((u8 *)hva, &old, *old_addr, new, access_key);
  1218			*success = !ret && old == *old_addr;
  1219			*old_addr = old;
  1220			break;
  1221		}
  1222		case 2: {
  1223			u16 old;
  1224	
  1225			ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
  1226			*success = !ret && old == *old_addr;
  1227			*old_addr = old;
  1228			break;
  1229		}
  1230		case 4: {
  1231			u32 old;
  1232	
  1233			ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
  1234			*success = !ret && old == *old_addr;
  1235			*old_addr = old;
  1236			break;
  1237		}
  1238		case 8: {
  1239			u64 old;
  1240	
  1241			ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
  1242			*success = !ret && old == *old_addr;
  1243			*old_addr = old;
  1244			break;
  1245		}
  1246		case 16: {
  1247			__uint128_t old;
  1248	
  1249			ret = cmpxchg_user_key((__uint128_t *)hva, &old, *old_addr, new, access_key);
  1250			*success = !ret && old == *old_addr;
  1251			*old_addr = old;
  1252			break;
  1253		}
  1254		default:
  1255			return -EINVAL;
  1256		}
  1257		mark_page_dirty_in_slot(kvm, slot, gfn);
  1258		/*
  1259		 * Assume that the fault is caused by protection, either key protection
  1260		 * or user page write protection.
  1261		 */
  1262		if (ret == -EFAULT)
  1263			ret = PGM_PROTECTION;
  1264		return ret;
  1265	}
  1266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v6 13/14] 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 | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 9807b05a1b57..ce8a50d79232 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -3736,7 +3736,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 > 0xffff 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
@@ -3754,6 +3755,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_addr;	/* ignored if flag unset */
 		};
 		__u32 sida_offset; /* offset into the sida */
 		__u8 reserved[32]; /* ignored */
@@ -3781,6 +3784,7 @@ Possible operations are:
   * ``KVM_S390_MEMOP_ABSOLUTE_WRITE``
   * ``KVM_S390_MEMOP_SIDA_READ``
   * ``KVM_S390_MEMOP_SIDA_WRITE``
+  * ``KVM_S390_MEMOP_ABSOLUTE_CMPXCHG``
 
 Logical read/write:
 ^^^^^^^^^^^^^^^^^^^
@@ -3829,7 +3833,7 @@ the checks required for storage key protection as one operation (as opposed to
 user space getting the storage keys, performing the checks, and accessing
 memory thereafter, which could lead to a delay between check and access).
 Absolute accesses are permitted for the VM ioctl if KVM_CAP_S390_MEM_OP_EXTENSION
-is > 0.
+has the KVM_S390_MEMOP_EXTENSION_CAP_BASE bit set.
 Currently absolute accesses are not permitted for VCPU ioctls.
 Absolute accesses are permitted for non-protected guests only.
 
@@ -3837,7 +3841,26 @@ Supported flags:
   * ``KVM_S390_MEMOP_F_CHECK_ONLY``
   * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
 
-The semantics of the flags are as for logical accesses.
+The semantics of the flags common with logical accesses are as for logical
+accesses.
+
+Absolute cmpxchg:
+^^^^^^^^^^^^^^^^^
+
+Perform cmpxchg on absolute guest memory. Intended for use with the
+KVM_S390_MEMOP_F_SKEY_PROTECTION flag.
+Instead of doing an unconditional write, the access occurs only if the target
+location contains the value pointed to by "old_addr".
+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.
+If the exchange did not take place because the target value doesn't match the
+old value, the value "old_addr" points to is replaced by the target value.
+User space can tell if an exchange took place by checking if this replacement
+occurred. The cmpxchg op is permitted for the VM ioctl if
+KVM_CAP_S390_MEM_OP_EXTENSION has flag KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG set.
+
+Supported flags:
+  * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
 
 SIDA read/write:
 ^^^^^^^^^^^^^^^^
-- 
2.34.1

[PATCH v6 14/14] 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 | 410 +++++++++++++++++++++-
 1 file changed, 395 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 55b856c4d656..dfd97a7c7fe8 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>
 
@@ -26,6 +27,7 @@ enum mop_target {
 enum mop_access_mode {
 	READ,
 	WRITE,
+	CMPXCHG,
 };
 
 struct mop_desc {
@@ -44,13 +46,16 @@ struct mop_desc {
 	enum mop_access_mode mode;
 	void *buf;
 	uint32_t sida_offset;
+	void *old;
+	uint8_t old_value[16];
+	bool *cmpxchg_success;
 	uint8_t ar;
 	uint8_t key;
 };
 
 const uint8_t NO_KEY = 0xff;
 
-static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
+static struct kvm_s390_mem_op ksmo_from_desc(struct mop_desc *desc)
 {
 	struct kvm_s390_mem_op ksmo = {
 		.gaddr = (uintptr_t)desc->gaddr,
@@ -77,6 +82,11 @@ static struct kvm_s390_mem_op ksmo_from_desc(const struct mop_desc *desc)
 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_READ;
 		if (desc->mode == WRITE)
 			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_WRITE;
+		if (desc->mode == CMPXCHG) {
+			ksmo.op = KVM_S390_MEMOP_ABSOLUTE_CMPXCHG;
+			ksmo.old_addr = (uint64_t)desc->old;
+			memcpy(desc->old_value, desc->old, desc->size);
+		}
 		break;
 	case INVALID:
 		ksmo.op = -1;
@@ -135,9 +145,13 @@ static void print_memop(struct kvm_vcpu *vcpu, const struct kvm_s390_mem_op *ksm
 	case KVM_S390_MEMOP_ABSOLUTE_WRITE:
 		printf("ABSOLUTE, WRITE, ");
 		break;
+	case KVM_S390_MEMOP_ABSOLUTE_CMPXCHG:
+		printf("ABSOLUTE, CMPXCHG, ");
+		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_addr=%llx",
+	       ksmo->gaddr, ksmo->size, ksmo->buf, ksmo->ar, ksmo->key,
+	       ksmo->old_addr);
 	if (ksmo->flags & KVM_S390_MEMOP_F_CHECK_ONLY)
 		printf(", CHECK_ONLY");
 	if (ksmo->flags & KVM_S390_MEMOP_F_INJECT_EXCEPTION)
@@ -147,24 +161,31 @@ static void print_memop(struct kvm_vcpu *vcpu, const struct kvm_s390_mem_op *ksm
 	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->op == KVM_S390_MEMOP_ABSOLUTE_CMPXCHG) {
+		if (desc->cmpxchg_success) {
+			int diff = memcmp(desc->old_value, desc->old, desc->size);
+			*desc->cmpxchg_success = !diff;
+		}
+	}
+	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 +208,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 +222,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 +233,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 +266,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 +279,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 +321,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);
+			MOP(test->vm, ABSOLUTE, CMPXCHG, 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]++;
+			MOP(test->vm, ABSOLUTE, CMPXCHG, 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 +443,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_addr, __uint128_t new)
+{
+	bool ret;
+
+	switch (size) {
+	case 4: {
+			uint32_t old = *old_addr;
+
+			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_addr;
+			*old_addr = old;
+			return ret;
+		}
+	case 8: {
+			uint64_t old = *old_addr;
+
+			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_addr;
+			*old_addr = old;
+			return ret;
+		}
+	case 16: {
+			__uint128_t old = *old_addr;
+
+			asm volatile ("cdsg %[old],%[new],%[address]"
+			    : [old] "+d" (old),
+			      [address] "+Q" (*(__uint128_t *)(target))
+			    : [new] "d" (new)
+			    : "cc"
+			);
+			ret = old == *old_addr;
+			*old_addr = 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, CMPXCHG, &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, CMPXCHG, 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 +767,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;
+
+		ERR_PROT_MOP(t.vm, ABSOLUTE, CMPXCHG, 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);
@@ -692,6 +1020,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, CMPXCHG, 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, CMPXCHG, 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, CMPXCHG, 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[])
 {
@@ -720,6 +1080,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,
@@ -740,6 +1110,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