[PATCH 0/4] KVM: selftests: ioctl() macro cleanups

[PATCH 0/4] KVM: selftests: ioctl() macro cleanups

[Sean Christopherson]

Do some minor housekeeping on the ioctl() macros, and then teach them
to detect and report when an ioctl() unexpectedly fails because KVM has
killed and/or bugged the VM.

Note, I'm 50/50 on whether or not the ARM patch is worthwhile, though I
spent a stupid amount of time on it (don't ask), so darn it I'm at least
posting it.

Oh, and the To: will probably show up funky, but I'd like to take this
through kvm-x86/selftests, not the ARM tree.

Thanks!

Sean Christopherson (4):
  KVM: selftests: Drop the single-underscore ioctl() helpers
  KVM: selftests: Add helper macros for ioctl()s that return file
    descriptors
  KVM: selftests: Use asserting kvm_ioctl() macros when getting ARM page
    sizes
  KVM: selftests: Add logic to detect if ioctl() failed because VM was
    killed

 .../selftests/kvm/include/kvm_util_base.h     | 101 ++++++++++++------
 .../selftests/kvm/lib/aarch64/processor.c     |  18 ++--
 tools/testing/selftests/kvm/lib/kvm_util.c    |  17 +--
 3 files changed, 84 insertions(+), 52 deletions(-)


base-commit: 240f736891887939571854bd6d734b6c9291f22e
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH 1/4] KVM: selftests: Drop the single-underscore ioctl() helpers

[Sean Christopherson]

Drop _kvm_ioctl(), _vm_ioctl(), and _vcpu_ioctl(), as they are no longer
used by anything other than the no-underscores variants (and may have
never been used directly).  The single-underscore variants were never
intended to be a "feature", they were a stopgap of sorts to ease the
conversion to pretty printing ioctl() names when reporting errors.

Opportunistically add a comment explaining when to use __KVM_IOCTL_ERROR()
versus KVM_IOCTL_ERROR().  The single-underscore macros were subtly
ensuring that the name of the ioctl() was printed on error, i.e. it's all
too easy to overlook the fact that using __KVM_IOCTL_ERROR() is
intentional.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     | 42 +++++++++----------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 07732a157ccd..90b7739ca204 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -227,6 +227,13 @@ static inline bool kvm_has_cap(long cap)
 #define __KVM_SYSCALL_ERROR(_name, _ret) \
 	"%s failed, rc: %i errno: %i (%s)", (_name), (_ret), errno, strerror(errno)
 
+/*
+ * Use the "inner", double-underscore macro when reporting errors from within
+ * other macros so that the name of ioctl() and not its literal numeric value
+ * is printed on error.  The "outer" macro is strongly preferred when reporting
+ * errors "directly", i.e. without an additional layer of macros, as it reduces
+ * the probability of passing in the wrong string.
+ */
 #define __KVM_IOCTL_ERROR(_name, _ret)	__KVM_SYSCALL_ERROR(_name, _ret)
 #define KVM_IOCTL_ERROR(_ioctl, _ret) __KVM_IOCTL_ERROR(#_ioctl, _ret)
 
@@ -239,17 +246,13 @@ static inline bool kvm_has_cap(long cap)
 #define __kvm_ioctl(kvm_fd, cmd, arg)				\
 	kvm_do_ioctl(kvm_fd, cmd, arg)
 
-
-#define _kvm_ioctl(kvm_fd, cmd, name, arg)			\
+#define kvm_ioctl(kvm_fd, cmd, arg)				\
 ({								\
 	int ret = __kvm_ioctl(kvm_fd, cmd, arg);		\
 								\
-	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));	\
+	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
 })
 
-#define kvm_ioctl(kvm_fd, cmd, arg) \
-	_kvm_ioctl(kvm_fd, cmd, #cmd, arg)
-
 static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
 
 #define __vm_ioctl(vm, cmd, arg)				\
@@ -258,16 +261,12 @@ static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
 	kvm_do_ioctl((vm)->fd, cmd, arg);			\
 })
 
-#define _vm_ioctl(vm, cmd, name, arg)				\
-({								\
-	int ret = __vm_ioctl(vm, cmd, arg);			\
-								\
-	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));	\
-})
-
 #define vm_ioctl(vm, cmd, arg)					\
-	_vm_ioctl(vm, cmd, #cmd, arg)
-
+({								\
+	int ret = __vm_ioctl(vm, cmd, arg);			\
+								\
+	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
+})
 
 static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 
@@ -277,15 +276,12 @@ static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 	kvm_do_ioctl((vcpu)->fd, cmd, arg);			\
 })
 
-#define _vcpu_ioctl(vcpu, cmd, name, arg)			\
-({								\
-	int ret = __vcpu_ioctl(vcpu, cmd, arg);			\
-								\
-	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));	\
-})
-
 #define vcpu_ioctl(vcpu, cmd, arg)				\
-	_vcpu_ioctl(vcpu, cmd, #cmd, arg)
+({								\
+	int ret = __vcpu_ioctl(vcpu, cmd, arg);			\
+								\
+	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
+})
 
 /*
  * Looks up and returns the value corresponding to the capability
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Sean Christopherson]

Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
descriptors, i.e. deduplicate code for asserting success on ioctls() for
which a positive return value, not just zero, is considered success.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     | 39 +++++++++++++------
 tools/testing/selftests/kvm/lib/kvm_util.c    | 17 ++++----
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 90b7739ca204..b35b0bd23683 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -253,6 +253,14 @@ static inline bool kvm_has_cap(long cap)
 	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
 })
 
+#define kvm_fd_ioctl(kvm_fd, cmd, arg)				\
+({								\
+	int fd = __kvm_ioctl(kvm_fd, cmd, arg);			\
+								\
+	TEST_ASSERT(fd >= 0, __KVM_IOCTL_ERROR(#cmd, fd));	\
+	fd;							\
+})
+
 static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
 
 #define __vm_ioctl(vm, cmd, arg)				\
@@ -268,6 +276,14 @@ static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
 	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
 })
 
+#define vm_fd_ioctl(vm, cmd, arg)				\
+({								\
+	int fd = __vm_ioctl(vm, cmd, arg);			\
+								\
+	TEST_ASSERT(fd >= 0, __KVM_IOCTL_ERROR(#cmd, fd));	\
+	fd;							\
+})
+
 static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 
 #define __vcpu_ioctl(vcpu, cmd, arg)				\
@@ -283,16 +299,21 @@ static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
 })
 
+#define vcpu_fd_ioctl(vcpu, cmd, arg)				\
+({								\
+	int fd = __vcpu_ioctl(vcpu, cmd, arg);			\
+								\
+	TEST_ASSERT(fd >= 0, __KVM_IOCTL_ERROR(#cmd, fd));	\
+	fd;							\
+})
+
 /*
  * Looks up and returns the value corresponding to the capability
  * (KVM_CAP_*) given by cap.
  */
 static inline int vm_check_cap(struct kvm_vm *vm, long cap)
 {
-	int ret =  __vm_ioctl(vm, KVM_CHECK_EXTENSION, (void *)cap);
-
-	TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
-	return ret;
+	return vm_fd_ioctl(vm, KVM_CHECK_EXTENSION, (void *)cap);
 }
 
 static inline int __vm_enable_cap(struct kvm_vm *vm, uint32_t cap, uint64_t arg0)
@@ -348,10 +369,7 @@ static inline uint32_t kvm_vm_reset_dirty_ring(struct kvm_vm *vm)
 
 static inline int vm_get_stats_fd(struct kvm_vm *vm)
 {
-	int fd = __vm_ioctl(vm, KVM_GET_STATS_FD, NULL);
-
-	TEST_ASSERT(fd >= 0, KVM_IOCTL_ERROR(KVM_GET_STATS_FD, fd));
-	return fd;
+	return vm_fd_ioctl(vm, KVM_GET_STATS_FD, NULL);
 }
 
 static inline void read_stats_header(int stats_fd, struct kvm_stats_header *header)
@@ -558,10 +576,7 @@ static inline void vcpu_nested_state_set(struct kvm_vcpu *vcpu,
 #endif
 static inline int vcpu_get_stats_fd(struct kvm_vcpu *vcpu)
 {
-	int fd = __vcpu_ioctl(vcpu, KVM_GET_STATS_FD, NULL);
-
-	TEST_ASSERT(fd >= 0, KVM_IOCTL_ERROR(KVM_GET_STATS_FD, fd));
-	return fd;
+	return vcpu_fd_ioctl(vcpu, KVM_GET_STATS_FD, NULL);
 }
 
 int __kvm_has_device_attr(int dev_fd, uint32_t group, uint64_t attr);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 7a8af1821f5d..557de1d26f10 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -117,8 +117,12 @@ unsigned int kvm_check_cap(long cap)
 	int kvm_fd;
 
 	kvm_fd = open_kvm_dev_path_or_exit();
-	ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, (void *)cap);
-	TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
+
+	/*
+	 * KVM_CHECK_EXTENSION doesn't return a file descriptor, but the
+	 * semantics are the same: a negative value is considered a failure.
+	 */
+	ret = kvm_fd_ioctl(kvm_fd, KVM_CHECK_EXTENSION, (void *)cap);
 
 	close(kvm_fd);
 
@@ -136,12 +140,10 @@ void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size)
 
 static void vm_open(struct kvm_vm *vm)
 {
-	vm->kvm_fd = _open_kvm_dev_path_or_exit(O_RDWR);
-
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_IMMEDIATE_EXIT));
 
-	vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, (void *)vm->type);
-	TEST_ASSERT(vm->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm->fd));
+	vm->kvm_fd = _open_kvm_dev_path_or_exit(O_RDWR);
+	vm->fd = kvm_fd_ioctl(vm->kvm_fd, KVM_CREATE_VM, (void *)vm->type);
 }
 
 const char *vm_guest_mode_string(uint32_t i)
@@ -1226,8 +1228,7 @@ struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
 
 	vcpu->vm = vm;
 	vcpu->id = vcpu_id;
-	vcpu->fd = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)(unsigned long)vcpu_id);
-	TEST_ASSERT(vcpu->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, vcpu->fd));
+	vcpu->fd = vm_fd_ioctl(vm, KVM_CREATE_VCPU, (void *)(unsigned long)vcpu_id);
 
 	TEST_ASSERT(vcpu_mmap_sz() >= sizeof(*vcpu->run), "vcpu mmap size "
 		"smaller than expected, vcpu_mmap_sz: %i expected_min: %zi",
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH 3/4] KVM: selftests: Use asserting kvm_ioctl() macros when getting ARM page sizes

[Sean Christopherson]

Use kvm_ioctl() instead of open coding equivalent ioctl()+TEST_ASSERT()
calls when getting the support page sizes on ARM.  The macro usage is a
little funky since the "kvm_fd" parameter implies an actual /dev/kvm fd,
but on the other hand the code is invoking KVM ioctl()s.

Alternatively, the core utilities could expose a vm_open()+vm_close()
pair so that the ARM code could create a dummy, on-stack VM+vCPU pair and
use {vm,vcpu}_ioctl() as appropriate.  But the odds of something breaking
due to oddball, partial usage of kvm_vm and kvm_vcpu structures is much
higher than realizing meaningful benefit from using {vm,vcpu}_ioctl().

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/lib/aarch64/processor.c      | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 3a0259e25335..afec1a30916f 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -496,7 +496,7 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
 				      bool *ps4k, bool *ps16k, bool *ps64k)
 {
 	struct kvm_vcpu_init preferred_init;
-	int kvm_fd, vm_fd, vcpu_fd, err;
+	int kvm_fd, vm_fd, vcpu_fd;
 	uint64_t val;
 	struct kvm_one_reg reg = {
 		.id	= KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1),
@@ -504,19 +504,13 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
 	};
 
 	kvm_fd = open_kvm_dev_path_or_exit();
-	vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, (void *)(unsigned long)ipa);
-	TEST_ASSERT(vm_fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm_fd));
+	vm_fd = kvm_fd_ioctl(kvm_fd, KVM_CREATE_VM, (void *)(unsigned long)ipa);
+	vcpu_fd = kvm_fd_ioctl(vm_fd, KVM_CREATE_VCPU, (void *)0ul);
 
-	vcpu_fd = ioctl(vm_fd, KVM_CREATE_VCPU, 0);
-	TEST_ASSERT(vcpu_fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, vcpu_fd));
+	kvm_ioctl(vm_fd, KVM_ARM_PREFERRED_TARGET, &preferred_init);
+	kvm_ioctl(vcpu_fd, KVM_ARM_VCPU_INIT, &preferred_init);
 
-	err = ioctl(vm_fd, KVM_ARM_PREFERRED_TARGET, &preferred_init);
-	TEST_ASSERT(err == 0, KVM_IOCTL_ERROR(KVM_ARM_PREFERRED_TARGET, err));
-	err = ioctl(vcpu_fd, KVM_ARM_VCPU_INIT, &preferred_init);
-	TEST_ASSERT(err == 0, KVM_IOCTL_ERROR(KVM_ARM_VCPU_INIT, err));
-
-	err = ioctl(vcpu_fd, KVM_GET_ONE_REG, &reg);
-	TEST_ASSERT(err == 0, KVM_IOCTL_ERROR(KVM_GET_ONE_REG, vcpu_fd));
+	kvm_ioctl(vcpu_fd, KVM_GET_ONE_REG, &reg);
 
 	*ps4k = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN4), val) != 0xf;
 	*ps64k = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64MMFR0_TGRAN64), val) == 0;
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH 4/4] KVM: selftests: Add logic to detect if ioctl() failed because VM was killed

[Sean Christopherson]

Add yet another macro to the VM/vCPU ioctl() framework to detect when an
ioctl() failed because KVM killed/bugged the VM, i.e. when there was
nothing wrong with the ioctl() itself.  If KVM kills a VM, e.g. by way of
a failed KVM_BUG_ON(), all subsequent VM and vCPU ioctl()s will fail with
-EIO, which can be quite misleading and ultimately waste user/developer
time.

Use KVM_CHECK_EXTENSION on KVM_CAP_USER_MEMORY to detect if the VM is
dead and/or bug, as KVM doesn't provide a dedicated ioctl().  Using a
heuristic is obviously less than ideal, but practically speaking the logic
is bulletproof barring a KVM change, and any such change would arguably
break userspace, e.g. if KVM returns something other than -EIO.

Without the detection, tearing down a bugged VM yields a cryptic failure
when deleting memslots:

  ==== Test Assertion Failure ====
  lib/kvm_util.c:689: !ret
  pid=45131 tid=45131 errno=5 - Input/output error
     1	0x00000000004036c3: __vm_mem_region_delete at kvm_util.c:689
     2	0x00000000004042f0: kvm_vm_free at kvm_util.c:724 (discriminator 12)
     3	0x0000000000402929: race_sync_regs at sync_regs_test.c:193
     4	0x0000000000401cab: main at sync_regs_test.c:334 (discriminator 6)
     5	0x0000000000416f13: __libc_start_call_main at libc-start.o:?
     6	0x000000000041855f: __libc_start_main_impl at ??:?
     7	0x0000000000401d40: _start at ??:?
  KVM_SET_USER_MEMORY_REGION failed, rc: -1 errno: 5 (Input/output error)

Which morphs into a more pointed error message with the detection:

  ==== Test Assertion Failure ====
  lib/kvm_util.c:689: false
  pid=80347 tid=80347 errno=5 - Input/output error
     1	0x00000000004039ab: __vm_mem_region_delete at kvm_util.c:689 (discriminator 5)
     2	0x0000000000404660: kvm_vm_free at kvm_util.c:724 (discriminator 12)
     3	0x0000000000402ac9: race_sync_regs at sync_regs_test.c:193
     4	0x0000000000401cb7: main at sync_regs_test.c:334 (discriminator 6)
     5	0x0000000000418263: __libc_start_call_main at libc-start.o:?
     6	0x00000000004198af: __libc_start_main_impl at ??:?
     7	0x0000000000401d90: _start at ??:?
  KVM killed/bugged the VM, check the kernel log for clues

Suggested-by: Michal Luczaj <mhal@rbox.co>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     | 34 ++++++++++++++++---
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index b35b0bd23683..49ad681d2161 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -269,18 +269,44 @@ static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
 	kvm_do_ioctl((vm)->fd, cmd, arg);			\
 })
 
+/*
+ * Assert that a VM or vCPU ioctl() succeeded, with extra magic to detect if
+ * the ioctl() failed because KVM killed/bugged the VM.  To detect a dead VM,
+ * probe KVM_CAP_USER_MEMORY, which (a) has been supported by KVM since before
+ * selftests existed and (b) should never outright fail, i.e. is supposed to
+ * return 0 or 1.  If KVM kills a VM, KVM returns -EIO for all ioctl()s for the
+ * VM and its vCPUs, including KVM_CHECK_EXTENSION.
+ */
+#define TEST_ASSERT_VM_VCPU_IOCTL(cond, name, ret, vm)					\
+do {											\
+	int __errno = errno;								\
+											\
+	static_assert_is_vm(vm);							\
+											\
+	if (cond)									\
+		break;									\
+											\
+	if (errno == EIO &&								\
+	    __vm_ioctl(vm, KVM_CHECK_EXTENSION, (void *)KVM_CAP_USER_MEMORY) < 0) {	\
+		TEST_ASSERT(errno == EIO, "KVM killed the VM, should return -EIO");	\
+		TEST_FAIL("KVM killed/bugged the VM, check the kernel log for clues");	\
+	}										\
+	errno = __errno;								\
+	TEST_ASSERT(cond, __KVM_IOCTL_ERROR(name, ret));				\
+} while (0)
+
 #define vm_ioctl(vm, cmd, arg)					\
 ({								\
 	int ret = __vm_ioctl(vm, cmd, arg);			\
 								\
-	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm);		\
 })
 
 #define vm_fd_ioctl(vm, cmd, arg)				\
 ({								\
 	int fd = __vm_ioctl(vm, cmd, arg);			\
 								\
-	TEST_ASSERT(fd >= 0, __KVM_IOCTL_ERROR(#cmd, fd));	\
+	TEST_ASSERT_VM_VCPU_IOCTL(fd >= 0, #cmd, fd, vm);	\
 	fd;							\
 })
 
@@ -296,14 +322,14 @@ static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 ({								\
 	int ret = __vcpu_ioctl(vcpu, cmd, arg);			\
 								\
-	TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(#cmd, ret));	\
+	TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, (vcpu)->vm);	\
 })
 
 #define vcpu_fd_ioctl(vcpu, cmd, arg)				\
 ({								\
 	int fd = __vcpu_ioctl(vcpu, cmd, arg);			\
 								\
-	TEST_ASSERT(fd >= 0, __KVM_IOCTL_ERROR(#cmd, fd));	\
+	TEST_ASSERT_VM_VCPU_IOCTL(fd >= 0, #cmd, fd, (vcpu)->vm);\
 	fd;							\
 })
 
-- 
2.41.0.585.gd2178a4bd4-goog

Re: [PATCH 3/4] KVM: selftests: Use asserting kvm_ioctl() macros when getting ARM page sizes

[Michal Luczaj]

On 8/4/23 02:42, Sean Christopherson wrote:
> Use kvm_ioctl() instead of open coding equivalent ioctl()+TEST_ASSERT()
> calls when getting the support page sizes on ARM.  The macro usage is a
> little funky since the "kvm_fd" parameter implies an actual /dev/kvm fd,
> but on the other hand the code is invoking KVM ioctl()s.
> 
> Alternatively, the core utilities could expose a vm_open()+vm_close()
> pair so that the ARM code could create a dummy, on-stack VM+vCPU pair and
> use {vm,vcpu}_ioctl() as appropriate.  But the odds of something breaking
> due to oddball, partial usage of kvm_vm and kvm_vcpu structures is much
> higher than realizing meaningful benefit from using {vm,vcpu}_ioctl().

Since you're doing the cleanup, does mmio_warning_test qualify for the
same (funky usage ahead)?

-       kvm = open("/dev/kvm", O_RDWR);
-       TEST_ASSERT(kvm != -1, "failed to open /dev/kvm");
-       kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, NULL);
-       TEST_ASSERT(kvmvm > 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, kvmvm));
-       kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
-       TEST_ASSERT(kvmcpu != -1, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, kvmcpu));
+       kvm = open_path_or_exit(KVM_DEV_PATH, O_RDWR);
+       kvmvm = kvm_fd_ioctl(kvm, KVM_CREATE_VM, NULL);
+       kvmcpu = kvm_fd_ioctl(kvmvm, KVM_CREATE_VCPU, NULL);

Side note, just in case this wasn't your intention: no kvm@ in cc.

thanks,
Michal

Re: [PATCH 3/4] KVM: selftests: Use asserting kvm_ioctl() macros when getting ARM page sizes

[Sean Christopherson]

On Fri, Aug 04, 2023, Michal Luczaj wrote:
> On 8/4/23 02:42, Sean Christopherson wrote:
> > Use kvm_ioctl() instead of open coding equivalent ioctl()+TEST_ASSERT()
> > calls when getting the support page sizes on ARM.  The macro usage is a
> > little funky since the "kvm_fd" parameter implies an actual /dev/kvm fd,
> > but on the other hand the code is invoking KVM ioctl()s.
> > 
> > Alternatively, the core utilities could expose a vm_open()+vm_close()
> > pair so that the ARM code could create a dummy, on-stack VM+vCPU pair and
> > use {vm,vcpu}_ioctl() as appropriate.  But the odds of something breaking
> > due to oddball, partial usage of kvm_vm and kvm_vcpu structures is much
> > higher than realizing meaningful benefit from using {vm,vcpu}_ioctl().
> 
> Since you're doing the cleanup, does mmio_warning_test qualify for the
> same (funky usage ahead)?

Hmm, I'm heavily leaning towards deleting that test entirely.  It's almost
literally a copy+paste of the most basic syzkaller test, e.g. spawn a vCPU with
no backing memory and watch it die a horrible death.  Unless I'm missing something,
the test is complete overkill too, e.g. I highly doubt the original KVM bug required
userspace to fork() and whatnot, but syzkaller spawns threads for everything and
so that got copied into the selftest.

And this stuff is just silly:

	TEST_REQUIRE(host_cpu_is_intel);

	TEST_REQUIRE(!vm_is_unrestricted_guest(NULL));

because crashing the VM doesn't require Intel, nor does it require !URG, those
just happened to be the conditions for the bug.

As much as I like having explicit testcases, adding a new selftest for every bug
that syzkaller finds is neither realistic nor productive.  In other words, I think
we should treat syzkaller as being part of KVM's test infrastructure.

I'll send a patch to nuke the test.
 
> -       kvm = open("/dev/kvm", O_RDWR);
> -       TEST_ASSERT(kvm != -1, "failed to open /dev/kvm");
> -       kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, NULL);
> -       TEST_ASSERT(kvmvm > 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, kvmvm));
> -       kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
> -       TEST_ASSERT(kvmcpu != -1, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, kvmcpu));
> +       kvm = open_path_or_exit(KVM_DEV_PATH, O_RDWR);
> +       kvmvm = kvm_fd_ioctl(kvm, KVM_CREATE_VM, NULL);
> +       kvmcpu = kvm_fd_ioctl(kvmvm, KVM_CREATE_VCPU, NULL);
> 
> Side note, just in case this wasn't your intention: no kvm@ in cc.

Wasn't intentional, I was moving too fast at the end of the day and missed that
KVM wasn't Cc'd.  Grr.

Re: [PATCH 0/4] KVM: selftests: ioctl() macro cleanups

[Sean Christopherson]

+KVM (good job me)

On Thu, Aug 03, 2023, Sean Christopherson wrote:
> Do some minor housekeeping on the ioctl() macros, and then teach them
> to detect and report when an ioctl() unexpectedly fails because KVM has
> killed and/or bugged the VM.
> 
> Note, I'm 50/50 on whether or not the ARM patch is worthwhile, though I
> spent a stupid amount of time on it (don't ask), so darn it I'm at least
> posting it.
> 
> Oh, and the To: will probably show up funky, but I'd like to take this
> through kvm-x86/selftests, not the ARM tree.
> 
> Thanks!
> 
> Sean Christopherson (4):
>   KVM: selftests: Drop the single-underscore ioctl() helpers
>   KVM: selftests: Add helper macros for ioctl()s that return file
>     descriptors
>   KVM: selftests: Use asserting kvm_ioctl() macros when getting ARM page
>     sizes
>   KVM: selftests: Add logic to detect if ioctl() failed because VM was
>     killed
> 
>  .../selftests/kvm/include/kvm_util_base.h     | 101 ++++++++++++------
>  .../selftests/kvm/lib/aarch64/processor.c     |  18 ++--
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  17 +--
>  3 files changed, 84 insertions(+), 52 deletions(-)
> 
> 
> base-commit: 240f736891887939571854bd6d734b6c9291f22e
> -- 
> 2.41.0.585.gd2178a4bd4-goog
> 

Re: [PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Oliver Upton]

Hi Sean,

On Thu, Aug 03, 2023 at 05:42:24PM -0700, Sean Christopherson wrote:
> Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
> descriptors, i.e. deduplicate code for asserting success on ioctls() for
> which a positive return value, not just zero, is considered success.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

I appreciate the desire to eliminate duplicate code, but I think the
naming just muddies the waters. TBH, when I first read the diff w/o the
changelog, I thought you were describing the input fd (i.e. 'kvm_fd',
'vm_fd', 'vcpu_fd'). I don't think explicitly spelling out the condition
each time (i.e. ret >= 0) is all that difficult.

[...]

>  /*
>   * Looks up and returns the value corresponding to the capability
>   * (KVM_CAP_*) given by cap.
>   */
>  static inline int vm_check_cap(struct kvm_vm *vm, long cap)
>  {
> -	int ret =  __vm_ioctl(vm, KVM_CHECK_EXTENSION, (void *)cap);
> -
> -	TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
> -	return ret;
> +	return vm_fd_ioctl(vm, KVM_CHECK_EXTENSION, (void *)cap);
>  }

Though the same error condition, this isn't returning an fd.

-- 
Thanks,
Oliver

Re: [PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Sean Christopherson]

On Fri, Aug 04, 2023, Oliver Upton wrote:
> Hi Sean,
> 
> On Thu, Aug 03, 2023 at 05:42:24PM -0700, Sean Christopherson wrote:
> > Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
> > descriptors, i.e. deduplicate code for asserting success on ioctls() for
> > which a positive return value, not just zero, is considered success.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> I appreciate the desire to eliminate duplicate code, but I think the
> naming just muddies the waters. TBH, when I first read the diff w/o the
> changelog, I thought you were describing the input fd (i.e. 'kvm_fd',
> 'vm_fd', 'vcpu_fd'). I don't think explicitly spelling out the condition
> each time (i.e. ret >= 0) is all that difficult.

Yeah, but it's not just a desire to dedup code, I also am trying to funnel as
many "ioctl() succeeded" asserts as possible into common code so that they naturally
benefit from things like patch 4 (detecting dead/bugged VMs).

I agree the naming sucks.

Re: [PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Colton Lewis]

Oliver Upton <oliver.upton@linux.dev> writes:

> Hi Sean,

> On Thu, Aug 03, 2023 at 05:42:24PM -0700, Sean Christopherson wrote:
>> Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
>> descriptors, i.e. deduplicate code for asserting success on ioctls() for
>> which a positive return value, not just zero, is considered success.

>> Signed-off-by: Sean Christopherson <seanjc@google.com>

> I appreciate the desire to eliminate duplicate code, but I think the
> naming just muddies the waters. TBH, when I first read the diff w/o the
> changelog, I thought you were describing the input fd (i.e. 'kvm_fd',
> 'vm_fd', 'vcpu_fd'). I don't think explicitly spelling out the condition
> each time (i.e. ret >= 0) is all that difficult.

Couldn't ret >= 0 be the assert condition for everything? Don't see why
there needs to be different helpers to check == 0 and >= 0.

Unless I'm missing something, error returns are only ever negative.

Re: [PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Sean Christopherson]

On Fri, Aug 04, 2023, Colton Lewis wrote:
> Oliver Upton <oliver.upton@linux.dev> writes:
> 
> > Hi Sean,
> 
> > On Thu, Aug 03, 2023 at 05:42:24PM -0700, Sean Christopherson wrote:
> > > Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
> > > descriptors, i.e. deduplicate code for asserting success on ioctls() for
> > > which a positive return value, not just zero, is considered success.
> 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> > I appreciate the desire to eliminate duplicate code, but I think the
> > naming just muddies the waters. TBH, when I first read the diff w/o the
> > changelog, I thought you were describing the input fd (i.e. 'kvm_fd',
> > 'vm_fd', 'vcpu_fd'). I don't think explicitly spelling out the condition
> > each time (i.e. ret >= 0) is all that difficult.
> 
> Couldn't ret >= 0 be the assert condition for everything? Don't see why
> there needs to be different helpers to check == 0 and >= 0.
> 
> Unless I'm missing something, error returns are only ever negative.

Using "ret >= 0" would work in the sense that the tests wouldn't fail, but it
would degrade our test coverage, e.g. selftests wouldn't detect KVM bugs where
an ioctl() unexpectedly returns a non-zero, positive value.

The other wrinkle is that selftests need to actually consume the return value for
ioctl()s that return a positive value, i.e. the fd (or whatever it is) needs to
propagated up the stack.  I.e. all of the generic ioctl() macros would need to
"return" the value, which I really don't want to do because that would (re)open
the gates for having helpers that return an int, even though the only possible
return value is '0'.

Re: [PATCH 2/4] KVM: selftests: Add helper macros for ioctl()s that return file descriptors

[Sean Christopherson]

On Fri, Aug 04, 2023, Sean Christopherson wrote:
> On Fri, Aug 04, 2023, Oliver Upton wrote:
> > Hi Sean,
> > 
> > On Thu, Aug 03, 2023 at 05:42:24PM -0700, Sean Christopherson wrote:
> > > Add KVM, VM, and vCPU scoped helpers for ioctl()s that return file
> > > descriptors, i.e. deduplicate code for asserting success on ioctls() for
> > > which a positive return value, not just zero, is considered success.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > 
> > I appreciate the desire to eliminate duplicate code, but I think the
> > naming just muddies the waters. TBH, when I first read the diff w/o the
> > changelog, I thought you were describing the input fd (i.e. 'kvm_fd',
> > 'vm_fd', 'vcpu_fd'). I don't think explicitly spelling out the condition
> > each time (i.e. ret >= 0) is all that difficult.
> 
> Yeah, but it's not just a desire to dedup code, I also am trying to funnel as
> many "ioctl() succeeded" asserts as possible into common code so that they naturally
> benefit from things like patch 4 (detecting dead/bugged VMs).
> 
> I agree the naming sucks.

The best naming scheme I can come up with is to be super literal:

	kvm_ioctl_non_negative
	vm_ioctl_non_negative
	vcpu_ioctl_non_negative

If that's still too kludgy and no one has a better idea, I'll just scrap this
patch.