On Wed, 2023-01-11 at 10:35 +0100, Janosch Frank wrote:
On 1/10/23 21: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 mode. For now, support this mode for absolute accesses only.
This mode can be use, for example, to set the device-state-change indicator and the adapter-local-summary indicator atomically.
Signed-off-by: Janis Schoetterl-Glausch scgl@linux.ibm.com
include/uapi/linux/kvm.h | 7 +++ arch/s390/kvm/gaccess.h | 3 ++ arch/s390/kvm/gaccess.c | 102 +++++++++++++++++++++++++++++++++++++++ arch/s390/kvm/kvm-s390.c | 41 +++++++++++++++- 4 files changed, 151 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 55155e262646..452f43c1cc34 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 flag unset */@@ -599,6 +601,11 @@ struct kvm_s390_mem_op { #define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0) #define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1) #define KVM_S390_MEMOP_F_SKEY_PROTECTION (1ULL << 2) +#define KVM_S390_MEMOP_F_CMPXCHG (1ULL << 3) +/* flags specifying extension support */
Would that fit behind the bit shifts without getting into the "line too long" territory?
Bit shifts or the next line?
+#define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG 0x2
\n please
Not sure about all that, this is the way it looks right now:
/* types for kvm_s390_mem_op->op */ #define KVM_S390_MEMOP_LOGICAL_READ 0 #define KVM_S390_MEMOP_LOGICAL_WRITE 1 #define KVM_S390_MEMOP_SIDA_READ 2 #define KVM_S390_MEMOP_SIDA_WRITE 3 #define KVM_S390_MEMOP_ABSOLUTE_READ 4 #define KVM_S390_MEMOP_ABSOLUTE_WRITE 5 /* 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)
Seems consistent to me.
+/* Non program exception return codes (pgm codes are 16 bit) */ +#define KVM_S390_MEMOP_R_NO_XCHG (1 << 16) /* for KVM_INTERRUPT */ struct kvm_interrupt { diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h index 9408d6cc8e2c..92a3b9fb31ec 100644 --- a/arch/s390/kvm/gaccess.h +++ b/arch/s390/kvm/gaccess.h @@ -206,6 +206,9 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra, void *data, unsigned long len, enum gacc_mode mode); +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
__uint128_t *old, __uint128_t new, u8 access_key);- /**
- write_guest_with_key - copy data from kernel space to guest space
- @vcpu: virtual cpu
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index 0243b6e38d36..6165e761a637 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -1161,6 +1161,108 @@ 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
contains the value at @gpa before the attempt to exchange the value.
- @new: The value to place at @gpa.
- @access_key: The access key to use for the guest access.
- Atomically exchange the value at @gpa by @new, if it contains *@old.
- Honors storage keys.
- Return: * 0: successful exchange
* 1: exchange unsuccessful
* a program interruption code indicating the reason cmpxchg could
not be attempted1 Access related program interruption code indicating the reason
cmpxchg could not be attempted
< 1 Kernel / input data error codes
I think I'll do it like I said in the email to Thomas, that way it's maximally explicit about the return values one might get.
* -EINVAL: address misaligned or len not power of two
* -EAGAIN: transient failure (len 1 or 2)
* -EOPNOTSUPP: read-only memslot (should never occur)Would PGM_PROTECTED also make sense here instead of EOPNOTSUPP?
I don't think so, if you get EOPNOTSUPP there's a programming error somewhere that needs to be fixed. I wouldn't want to mix that with the totally fine case of a protection exception.
[...]
@@ -2772,12 +2779,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;@@ -2799,6 +2813,21 @@ 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;!mop->size || mop->size > sizeof(new)
Not sure why that would be necessary, but I did write "Operand length of the cmpxchg, required: 1 <= len <= 16", so I'll trust my past self on that.
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;- } if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) { tmpbuf = vmalloc(mop->size); if (!tmpbuf)
@@ -2829,6 +2858,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;Why don't we return KVM_S390_MEMOP_R_NO_XCHG from cmpxchg_guest_abs_with_key instead of aliasing 1 here?
I think it's a bit ugly, since cmpxchg_guest_abs_with_key is an internal function and not memop specific. I don't like returning a MEMOP API constant there.
if (copy_to_user(old_addr, &old.raw[off_in_quad], mop->size))r = -EFAULT;}