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 <[email protected]>
---
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(-)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d5d4419139a..f106db1af5ee 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
struct {
__u8 ar; /* the access register number */
__u8 key; /* access key, ignored if flag unset */
+ __u8 pad1[6]; /* ignored */
+ __u64 old_addr; /* ignored if flag unset */
};
__u32 sida_offset; /* offset into the sida */
__u8 reserved[32]; /* ignored */
@@ -604,6 +606,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 */
+#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)
/* 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 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)
+{
+ 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);
+ ret = ret < 0 ? ret : old != *old_addr;
+ *old_addr = old;
+ break;
+ }
+ case 2: {
+ u16 old;
+
+ ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
+ ret = ret < 0 ? ret : old != *old_addr;
+ *old_addr = old;
+ break;
+ }
+ case 4: {
+ u32 old;
+
+ ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
+ ret = ret < 0 ? ret : old != *old_addr;
+ *old_addr = old;
+ break;
+ }
+ case 8: {
+ u64 old;
+
+ ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
+ ret = ret < 0 ? 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);
+ ret = ret < 0 ? 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 45d4b8182b07..47bcf2cb4345 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -576,7 +576,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:
@@ -590,6 +589,14 @@ 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.
+ * The first extension doesn't use a flag, but pretend it does,
+ * this way that can be changed in the future.
+ */
+ r = KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG | 1;
+ break;
case KVM_CAP_NR_VCPUS:
case KVM_CAP_MAX_VCPUS:
case KVM_CAP_MAX_VCPU_ID:
@@ -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;
+ 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;
--
2.34.1
Hi Janis,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on 739ad2e4e15b585a0eaf98b7bdee62b2dd9588c9]
url: https://github.com/intel-lab-lkp/linux/commits/Janis-Schoetterl-Glausch/KVM-s390-Extend-MEM_OP-ioctl-by-storage-key-checked-cmpxchg/20221214-005540
base: 739ad2e4e15b585a0eaf98b7bdee62b2dd9588c9
patch link: https://lore.kernel.org/r/20221213165405.2953539-2-scgl%40linux.ibm.com
patch subject: [PATCH v4 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
config: s390-randconfig-r004-20221213
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/4e0991bd47ba30c7588e042da7a84d84b9f84056
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Janis-Schoetterl-Glausch/KVM-s390-Extend-MEM_OP-ioctl-by-storage-key-checked-cmpxchg/20221214-005540
git checkout 4e0991bd47ba30c7588e042da7a84d84b9f84056
# 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 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
s390-linux-ld: arch/s390/kvm/gaccess.o: in function `__cmpxchg_user_key':
>> arch/s390/include/asm/uaccess.h:410: undefined reference to `__ashlti3'
>> s390-linux-ld: arch/s390/include/asm/uaccess.h:411: undefined reference to `__ashlti3'
s390-linux-ld: arch/s390/include/asm/uaccess.h:458: undefined reference to `__ashlti3'
s390-linux-ld: arch/s390/include/asm/uaccess.h:459: undefined reference to `__ashlti3'
vim +410 arch/s390/include/asm/uaccess.h
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 396
4148575abe1e14 Heiko Carstens 2022-11-02 397 static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
4148575abe1e14 Heiko Carstens 2022-11-02 398 __uint128_t old, __uint128_t new,
4148575abe1e14 Heiko Carstens 2022-11-02 399 unsigned long key, int size)
4148575abe1e14 Heiko Carstens 2022-11-02 400 {
4148575abe1e14 Heiko Carstens 2022-11-02 401 int rc = 0;
4148575abe1e14 Heiko Carstens 2022-11-02 402
4148575abe1e14 Heiko Carstens 2022-11-02 403 switch (size) {
4148575abe1e14 Heiko Carstens 2022-11-02 404 case 1: {
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 405 unsigned int prev, shift, mask, _old, _new;
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 406 unsigned long count;
4148575abe1e14 Heiko Carstens 2022-11-02 407
4148575abe1e14 Heiko Carstens 2022-11-02 408 shift = (3 ^ (address & 3)) << 3;
4148575abe1e14 Heiko Carstens 2022-11-02 409 address ^= address & 3;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @410 _old = (old & 0xff) << shift;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @411 _new = (new & 0xff) << shift;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 412 mask = ~(0xff << shift);
4148575abe1e14 Heiko Carstens 2022-11-02 413 asm volatile(
4148575abe1e14 Heiko Carstens 2022-11-02 414 " spka 0(%[key])\n"
4148575abe1e14 Heiko Carstens 2022-11-02 415 " sacf 256\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 416 " llill %[count],%[max_loops]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 417 "0: l %[prev],%[address]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 418 "1: nr %[prev],%[mask]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 419 " xilf %[mask],0xffffffff\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 420 " or %[new],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 421 " or %[prev],%[tmp]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 422 "2: lr %[tmp],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 423 "3: cs %[prev],%[new],%[address]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 424 "4: jnl 5f\n"
4148575abe1e14 Heiko Carstens 2022-11-02 425 " xr %[tmp],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 426 " xr %[new],%[tmp]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 427 " nr %[tmp],%[mask]\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 428 " jnz 5f\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 429 " brct %[count],2b\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 430 "5: sacf 768\n"
4148575abe1e14 Heiko Carstens 2022-11-02 431 " spka %[default_key]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 432 EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 433 EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 434 EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 435 EX_TABLE_UA_LOAD_REG(4b, 5b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 436 : [rc] "+&d" (rc),
4148575abe1e14 Heiko Carstens 2022-11-02 437 [prev] "=&d" (prev),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 438 [address] "+Q" (*(int *)address),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 439 [tmp] "+&d" (_old),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 440 [new] "+&d" (_new),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 441 [mask] "+&d" (mask),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 442 [count] "=a" (count)
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 443 : [key] "%[count]" (key << 4),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 444 [default_key] "J" (PAGE_DEFAULT_KEY),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 445 [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
4148575abe1e14 Heiko Carstens 2022-11-02 446 : "memory", "cc");
4148575abe1e14 Heiko Carstens 2022-11-02 447 *(unsigned char *)uval = prev >> shift;
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 448 if (!count)
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 449 rc = -EAGAIN;
4148575abe1e14 Heiko Carstens 2022-11-02 450 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 451 }
4148575abe1e14 Heiko Carstens 2022-11-02 452 case 2: {
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 453 unsigned int prev, shift, mask, _old, _new;
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 454 unsigned long count;
4148575abe1e14 Heiko Carstens 2022-11-02 455
4148575abe1e14 Heiko Carstens 2022-11-02 456 shift = (2 ^ (address & 2)) << 3;
4148575abe1e14 Heiko Carstens 2022-11-02 457 address ^= address & 2;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 458 _old = (old & 0xffff) << shift;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 459 _new = (new & 0xffff) << shift;
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 460 mask = ~(0xffff << shift);
4148575abe1e14 Heiko Carstens 2022-11-02 461 asm volatile(
4148575abe1e14 Heiko Carstens 2022-11-02 462 " spka 0(%[key])\n"
4148575abe1e14 Heiko Carstens 2022-11-02 463 " sacf 256\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 464 " llill %[count],%[max_loops]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 465 "0: l %[prev],%[address]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 466 "1: nr %[prev],%[mask]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 467 " xilf %[mask],0xffffffff\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 468 " or %[new],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 469 " or %[prev],%[tmp]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 470 "2: lr %[tmp],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 471 "3: cs %[prev],%[new],%[address]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 472 "4: jnl 5f\n"
4148575abe1e14 Heiko Carstens 2022-11-02 473 " xr %[tmp],%[prev]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 474 " xr %[new],%[tmp]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 475 " nr %[tmp],%[mask]\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 476 " jnz 5f\n"
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 477 " brct %[count],2b\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 478 "5: sacf 768\n"
4148575abe1e14 Heiko Carstens 2022-11-02 479 " spka %[default_key]\n"
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 480 EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 481 EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 482 EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 483 EX_TABLE_UA_LOAD_REG(4b, 5b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 484 : [rc] "+&d" (rc),
4148575abe1e14 Heiko Carstens 2022-11-02 485 [prev] "=&d" (prev),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 486 [address] "+Q" (*(int *)address),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 487 [tmp] "+&d" (_old),
51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 488 [new] "+&d" (_new),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 489 [mask] "+&d" (mask),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 490 [count] "=a" (count)
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 491 : [key] "%[count]" (key << 4),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 492 [default_key] "J" (PAGE_DEFAULT_KEY),
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 493 [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
4148575abe1e14 Heiko Carstens 2022-11-02 494 : "memory", "cc");
4148575abe1e14 Heiko Carstens 2022-11-02 495 *(unsigned short *)uval = prev >> shift;
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 496 if (!count)
739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 497 rc = -EAGAIN;
4148575abe1e14 Heiko Carstens 2022-11-02 498 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 499 }
4148575abe1e14 Heiko Carstens 2022-11-02 500 case 4: {
4148575abe1e14 Heiko Carstens 2022-11-02 501 unsigned int prev = old;
4148575abe1e14 Heiko Carstens 2022-11-02 502
4148575abe1e14 Heiko Carstens 2022-11-02 503 asm volatile(
4148575abe1e14 Heiko Carstens 2022-11-02 504 " spka 0(%[key])\n"
4148575abe1e14 Heiko Carstens 2022-11-02 505 " sacf 256\n"
4148575abe1e14 Heiko Carstens 2022-11-02 506 "0: cs %[prev],%[new],%[address]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 507 "1: sacf 768\n"
4148575abe1e14 Heiko Carstens 2022-11-02 508 " spka %[default_key]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 509 EX_TABLE_UA_LOAD_REG(0b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 510 EX_TABLE_UA_LOAD_REG(1b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 511 : [rc] "+&d" (rc),
4148575abe1e14 Heiko Carstens 2022-11-02 512 [prev] "+&d" (prev),
4148575abe1e14 Heiko Carstens 2022-11-02 513 [address] "+Q" (*(int *)address)
4148575abe1e14 Heiko Carstens 2022-11-02 514 : [new] "d" ((unsigned int)new),
4148575abe1e14 Heiko Carstens 2022-11-02 515 [key] "a" (key << 4),
4148575abe1e14 Heiko Carstens 2022-11-02 516 [default_key] "J" (PAGE_DEFAULT_KEY)
4148575abe1e14 Heiko Carstens 2022-11-02 517 : "memory", "cc");
4148575abe1e14 Heiko Carstens 2022-11-02 518 *(unsigned int *)uval = prev;
4148575abe1e14 Heiko Carstens 2022-11-02 519 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 520 }
4148575abe1e14 Heiko Carstens 2022-11-02 521 case 8: {
4148575abe1e14 Heiko Carstens 2022-11-02 522 unsigned long prev = old;
4148575abe1e14 Heiko Carstens 2022-11-02 523
4148575abe1e14 Heiko Carstens 2022-11-02 524 asm volatile(
4148575abe1e14 Heiko Carstens 2022-11-02 525 " spka 0(%[key])\n"
4148575abe1e14 Heiko Carstens 2022-11-02 526 " sacf 256\n"
4148575abe1e14 Heiko Carstens 2022-11-02 527 "0: csg %[prev],%[new],%[address]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 528 "1: sacf 768\n"
4148575abe1e14 Heiko Carstens 2022-11-02 529 " spka %[default_key]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 530 EX_TABLE_UA_LOAD_REG(0b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 531 EX_TABLE_UA_LOAD_REG(1b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 532 : [rc] "+&d" (rc),
4148575abe1e14 Heiko Carstens 2022-11-02 533 [prev] "+&d" (prev),
4148575abe1e14 Heiko Carstens 2022-11-02 534 [address] "+QS" (*(long *)address)
4148575abe1e14 Heiko Carstens 2022-11-02 535 : [new] "d" ((unsigned long)new),
4148575abe1e14 Heiko Carstens 2022-11-02 536 [key] "a" (key << 4),
4148575abe1e14 Heiko Carstens 2022-11-02 537 [default_key] "J" (PAGE_DEFAULT_KEY)
4148575abe1e14 Heiko Carstens 2022-11-02 538 : "memory", "cc");
4148575abe1e14 Heiko Carstens 2022-11-02 539 *(unsigned long *)uval = prev;
4148575abe1e14 Heiko Carstens 2022-11-02 540 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 541 }
4148575abe1e14 Heiko Carstens 2022-11-02 542 case 16: {
4148575abe1e14 Heiko Carstens 2022-11-02 543 __uint128_t prev = old;
4148575abe1e14 Heiko Carstens 2022-11-02 544
4148575abe1e14 Heiko Carstens 2022-11-02 545 asm volatile(
4148575abe1e14 Heiko Carstens 2022-11-02 546 " spka 0(%[key])\n"
4148575abe1e14 Heiko Carstens 2022-11-02 547 " sacf 256\n"
4148575abe1e14 Heiko Carstens 2022-11-02 548 "0: cdsg %[prev],%[new],%[address]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 549 "1: sacf 768\n"
4148575abe1e14 Heiko Carstens 2022-11-02 550 " spka %[default_key]\n"
4148575abe1e14 Heiko Carstens 2022-11-02 551 EX_TABLE_UA_LOAD_REGPAIR(0b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 552 EX_TABLE_UA_LOAD_REGPAIR(1b, 1b, %[rc], %[prev])
4148575abe1e14 Heiko Carstens 2022-11-02 553 : [rc] "+&d" (rc),
4148575abe1e14 Heiko Carstens 2022-11-02 554 [prev] "+&d" (prev),
4148575abe1e14 Heiko Carstens 2022-11-02 555 [address] "+QS" (*(__int128_t *)address)
4148575abe1e14 Heiko Carstens 2022-11-02 556 : [new] "d" (new),
4148575abe1e14 Heiko Carstens 2022-11-02 557 [key] "a" (key << 4),
4148575abe1e14 Heiko Carstens 2022-11-02 558 [default_key] "J" (PAGE_DEFAULT_KEY)
4148575abe1e14 Heiko Carstens 2022-11-02 559 : "memory", "cc");
4148575abe1e14 Heiko Carstens 2022-11-02 560 *(__uint128_t *)uval = prev;
4148575abe1e14 Heiko Carstens 2022-11-02 561 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 562 }
4148575abe1e14 Heiko Carstens 2022-11-02 563 }
4148575abe1e14 Heiko Carstens 2022-11-02 564 __cmpxchg_user_key_called_with_bad_pointer();
4148575abe1e14 Heiko Carstens 2022-11-02 565 return rc;
4148575abe1e14 Heiko Carstens 2022-11-02 566 }
4148575abe1e14 Heiko Carstens 2022-11-02 567
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> ---
> 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(-)
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 0d5d4419139a..f106db1af5ee 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -588,6 +588,8 @@ struct kvm_s390_mem_op {
> struct {
> __u8 ar; /* the access register number */
> __u8 key; /* access key, ignored if flag unset */
> + __u8 pad1[6]; /* ignored */
> + __u64 old_addr; /* ignored if flag unset */
> };
> __u32 sida_offset; /* offset into the sida */
> __u8 reserved[32]; /* ignored */
> @@ -604,6 +606,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 */
> +#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)
>
> /* 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 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)
> +{
> + 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);
> + ret = ret < 0 ? ret : old != *old_addr;
> + *old_addr = old;
> + break;
> + }
> + case 2: {
> + u16 old;
> +
> + ret = cmpxchg_user_key((u16 *)hva, &old, *old_addr, new, access_key);
> + ret = ret < 0 ? ret : old != *old_addr;
> + *old_addr = old;
> + break;
> + }
> + case 4: {
> + u32 old;
> +
> + ret = cmpxchg_user_key((u32 *)hva, &old, *old_addr, new, access_key);
> + ret = ret < 0 ? ret : old != *old_addr;
> + *old_addr = old;
> + break;
> + }
> + case 8: {
> + u64 old;
> +
> + ret = cmpxchg_user_key((u64 *)hva, &old, *old_addr, new, access_key);
> + ret = ret < 0 ? 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);
> + ret = ret < 0 ? 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 45d4b8182b07..47bcf2cb4345 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -576,7 +576,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:
> @@ -590,6 +589,14 @@ 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.
> + * The first extension doesn't use a flag, but pretend it does,
> + * this way that can be changed in the future.
> + */
> + r = KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG | 1;
> + break;
> case KVM_CAP_NR_VCPUS:
> case KVM_CAP_MAX_VCPUS:
> case KVM_CAP_MAX_VCPU_ID:
> @@ -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.
Anyway, patch looks good to me, so with or without that additional check:
Reviewed-by: Thomas Huth <[email protected]>
> + 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;
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 <[email protected]>
> > ---
> > 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 <[email protected]>
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;
>
On Wed, 2022-12-14 at 10:23 +0800, kernel test robot wrote:
> Hi Janis,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on 739ad2e4e15b585a0eaf98b7bdee62b2dd9588c9]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Janis-Schoetterl-Glausch/KVM-s390-Extend-MEM_OP-ioctl-by-storage-key-checked-cmpxchg/20221214-005540
> base: 739ad2e4e15b585a0eaf98b7bdee62b2dd9588c9
> patch link: https://lore.kernel.org/r/20221213165405.2953539-2-scgl%40linux.ibm.com
> patch subject: [PATCH v4 1/9] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg
> config: s390-randconfig-r004-20221213
> 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/4e0991bd47ba30c7588e042da7a84d84b9f84056
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Janis-Schoetterl-Glausch/KVM-s390-Extend-MEM_OP-ioctl-by-storage-key-checked-cmpxchg/20221214-005540
> git checkout 4e0991bd47ba30c7588e042da7a84d84b9f84056
> # 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 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> s390-linux-ld: arch/s390/kvm/gaccess.o: in function `__cmpxchg_user_key':
> > > arch/s390/include/asm/uaccess.h:410: undefined reference to `__ashlti3'
> > > s390-linux-ld: arch/s390/include/asm/uaccess.h:411: undefined reference to `__ashlti3'
> s390-linux-ld: arch/s390/include/asm/uaccess.h:458: undefined reference to `__ashlti3'
> s390-linux-ld: arch/s390/include/asm/uaccess.h:459: undefined reference to `__ashlti3'
>
>
> vim +410 arch/s390/include/asm/uaccess.h
>
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 396
> 4148575abe1e14 Heiko Carstens 2022-11-02 397 static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
> 4148575abe1e14 Heiko Carstens 2022-11-02 398 __uint128_t old, __uint128_t new,
> 4148575abe1e14 Heiko Carstens 2022-11-02 399 unsigned long key, int size)
> 4148575abe1e14 Heiko Carstens 2022-11-02 400 {
> 4148575abe1e14 Heiko Carstens 2022-11-02 401 int rc = 0;
> 4148575abe1e14 Heiko Carstens 2022-11-02 402
> 4148575abe1e14 Heiko Carstens 2022-11-02 403 switch (size) {
> 4148575abe1e14 Heiko Carstens 2022-11-02 404 case 1: {
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 405 unsigned int prev, shift, mask, _old, _new;
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 406 unsigned long count;
> 4148575abe1e14 Heiko Carstens 2022-11-02 407
> 4148575abe1e14 Heiko Carstens 2022-11-02 408 shift = (3 ^ (address & 3)) << 3;
> 4148575abe1e14 Heiko Carstens 2022-11-02 409 address ^= address & 3;
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @410 _old = (old & 0xff) << shift;
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @411 _new = (new & 0xff) << shift;
Not sure what it is in this config that causes gcc to emit this symbol instead of a shift instruction, but casting old/new to 32 bit fixes
the error.
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 412 mask = ~(0xff << shift);
> 4148575abe1e14 Heiko Carstens 2022-11-02 413 asm volatile(
> 4148575abe1e14 Heiko Carstens 2022-11-02 414 " spka 0(%[key])\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 415 " sacf 256\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 416 " llill %[count],%[max_loops]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 417 "0: l %[prev],%[address]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 418 "1: nr %[prev],%[mask]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 419 " xilf %[mask],0xffffffff\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 420 " or %[new],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 421 " or %[prev],%[tmp]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 422 "2: lr %[tmp],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 423 "3: cs %[prev],%[new],%[address]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 424 "4: jnl 5f\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 425 " xr %[tmp],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 426 " xr %[new],%[tmp]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 427 " nr %[tmp],%[mask]\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 428 " jnz 5f\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 429 " brct %[count],2b\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 430 "5: sacf 768\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 431 " spka %[default_key]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 432 EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 433 EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 434 EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 435 EX_TABLE_UA_LOAD_REG(4b, 5b, %[rc], %[prev])
> 4148575abe1e14 Heiko Carstens 2022-11-02 436 : [rc] "+&d" (rc),
> 4148575abe1e14 Heiko Carstens 2022-11-02 437 [prev] "=&d" (prev),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 438 [address] "+Q" (*(int *)address),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 439 [tmp] "+&d" (_old),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 440 [new] "+&d" (_new),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 441 [mask] "+&d" (mask),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 442 [count] "=a" (count)
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 443 : [key] "%[count]" (key << 4),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 444 [default_key] "J" (PAGE_DEFAULT_KEY),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 445 [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
> 4148575abe1e14 Heiko Carstens 2022-11-02 446 : "memory", "cc");
> 4148575abe1e14 Heiko Carstens 2022-11-02 447 *(unsigned char *)uval = prev >> shift;
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 448 if (!count)
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 449 rc = -EAGAIN;
> 4148575abe1e14 Heiko Carstens 2022-11-02 450 return rc;
> 4148575abe1e14 Heiko Carstens 2022-11-02 451 }
> 4148575abe1e14 Heiko Carstens 2022-11-02 452 case 2: {
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 453 unsigned int prev, shift, mask, _old, _new;
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 454 unsigned long count;
> 4148575abe1e14 Heiko Carstens 2022-11-02 455
> 4148575abe1e14 Heiko Carstens 2022-11-02 456 shift = (2 ^ (address & 2)) << 3;
> 4148575abe1e14 Heiko Carstens 2022-11-02 457 address ^= address & 2;
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 458 _old = (old & 0xffff) << shift;
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 459 _new = (new & 0xffff) << shift;
Same here.
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 460 mask = ~(0xffff << shift);
> 4148575abe1e14 Heiko Carstens 2022-11-02 461 asm volatile(
> 4148575abe1e14 Heiko Carstens 2022-11-02 462 " spka 0(%[key])\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 463 " sacf 256\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 464 " llill %[count],%[max_loops]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 465 "0: l %[prev],%[address]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 466 "1: nr %[prev],%[mask]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 467 " xilf %[mask],0xffffffff\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 468 " or %[new],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 469 " or %[prev],%[tmp]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 470 "2: lr %[tmp],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 471 "3: cs %[prev],%[new],%[address]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 472 "4: jnl 5f\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 473 " xr %[tmp],%[prev]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 474 " xr %[new],%[tmp]\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 475 " nr %[tmp],%[mask]\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 476 " jnz 5f\n"
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 477 " brct %[count],2b\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 478 "5: sacf 768\n"
> 4148575abe1e14 Heiko Carstens 2022-11-02 479 " spka %[default_key]\n"
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 480 EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 481 EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 482 EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 483 EX_TABLE_UA_LOAD_REG(4b, 5b, %[rc], %[prev])
> 4148575abe1e14 Heiko Carstens 2022-11-02 484 : [rc] "+&d" (rc),
> 4148575abe1e14 Heiko Carstens 2022-11-02 485 [prev] "=&d" (prev),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 486 [address] "+Q" (*(int *)address),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 487 [tmp] "+&d" (_old),
> 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 488 [new] "+&d" (_new),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 489 [mask] "+&d" (mask),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 490 [count] "=a" (count)
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 491 : [key] "%[count]" (key << 4),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 492 [default_key] "J" (PAGE_DEFAULT_KEY),
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 493 [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
> 4148575abe1e14 Heiko Carstens 2022-11-02 494 : "memory", "cc");
> 4148575abe1e14 Heiko Carstens 2022-11-02 495 *(unsigned short *)uval = prev >> shift;
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 496 if (!count)
> 739ad2e4e15b58 Janis Schoetterl-Glausch 2022-11-17 497 rc = -EAGAIN;
> 4148575abe1e14 Heiko Carstens 2022-11-02 498 return rc;
[...]
On Mon, Dec 19, 2022 at 10:24:33PM +0100, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-12-14 at 10:23 +0800, kernel test robot wrote:
> > Hi Janis,
> >
> > Thank you for the patch! Yet something to improve:
> > All errors (new ones prefixed by >>):
> >
> > s390-linux-ld: arch/s390/kvm/gaccess.o: in function `__cmpxchg_user_key':
> > > > arch/s390/include/asm/uaccess.h:410: undefined reference to `__ashlti3'
> > > > s390-linux-ld: arch/s390/include/asm/uaccess.h:411: undefined reference to `__ashlti3'
> > s390-linux-ld: arch/s390/include/asm/uaccess.h:458: undefined reference to `__ashlti3'
> > s390-linux-ld: arch/s390/include/asm/uaccess.h:459: undefined reference to `__ashlti3'
> > 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @410 _old = (old & 0xff) << shift;
> > 51098f0eb22e2f Janis Schoetterl-Glausch 2022-11-16 @411 _new = (new & 0xff) << shift;
>
> Not sure what it is in this config that causes gcc to emit this
> symbol instead of a shift instruction, but casting old/new to 32 bit
> fixes the error.
Right.. now we have the same fun with 128 bit arithmetics that we had
with 64 bit arithmetics on 32 bit. I really missed that :)
Fixed the way you proposed it:
https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=cmpxchg_user_key&id=b33d59fb37ddcb6ee65d4fa23cc3d58793d13c5b