On Wed, 2022-12-14 at 10:19 +0100, Thomas Huth wrote: > On 13/12/2022 17.53, Janis Schoetterl-Glausch wrote: > > User space can use the MEM_OP ioctl to make storage key checked reads > > and writes to the guest, however, it has no way of performing atomic, > > key checked, accesses to the guest. > > Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg > > mode. For now, support this mode for absolute accesses only. > > > > This mode can be use, for example, to set the device-state-change > > indicator and the adapter-local-summary indicator atomically. > > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> > > --- > > include/uapi/linux/kvm.h | 7 +++ > > arch/s390/kvm/gaccess.h | 3 ++ > > arch/s390/kvm/gaccess.c | 102 +++++++++++++++++++++++++++++++++++++++ > > arch/s390/kvm/kvm-s390.c | 39 ++++++++++++++- > > 4 files changed, 149 insertions(+), 2 deletions(-) > > [...] > > > > @@ -2714,12 +2721,19 @@ static bool access_key_invalid(u8 access_key) > > static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop) > > { > > void __user *uaddr = (void __user *)mop->buf; > > + void __user *old_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) > > @@ -2741,6 +2755,19 @@ 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; > > I'd maybe add a check for mop->op == KVM_S390_MEMOP_ABSOLUTE_WRITE here, > since calling the _READ function with the F_CMPXCHG flag set does not make > too much sense. Good point. > > Anyway, patch looks good to me, so with or without that additional check: > Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx> Thanks! > > > + 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; > > + } > > if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) { > > tmpbuf = vmalloc(mop->size); > > if (!tmpbuf) > > @@ -2771,6 +2798,14 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop) > > case KVM_S390_MEMOP_ABSOLUTE_WRITE: { > > if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) { > > r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key); > > + } else if (mop->flags & KVM_S390_MEMOP_F_CMPXCHG) { > > + r = cmpxchg_guest_abs_with_key(kvm, mop->gaddr, mop->size, > > + &old.quad, new.quad, mop->key); > > + if (r == 1) { > > + r = KVM_S390_MEMOP_R_NO_XCHG; > > + if (copy_to_user(old_addr, &old.raw[off_in_quad], mop->size)) > > + r = -EFAULT; > > + } > > } else { > > if (copy_from_user(tmpbuf, uaddr, mop->size)) { > > r = -EFAULT; >