diff mbox series

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

Message ID 20230804004226.1984505-3-seanjc@google.com
State New
Headers
Series KVM: selftests: ioctl() macro cleanups |

Commit Message

Sean Christopherson Aug. 4, 2023, 12:42 a.m. UTC 
  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(-)

Comments

Oliver Upton Aug. 4, 2023, 4:46 p.m. UTC | #1 
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.
Sean Christopherson Aug. 4, 2023, 5:27 p.m. UTC | #2 
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.
Sean Christopherson Aug. 4, 2023, 6:33 p.m. UTC | #3 
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'.
diff mbox series

Patch

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",