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 <[email protected]>
---
.../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
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 <[email protected]>
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
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 <[email protected]>
>
> 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.
On Fri, Aug 04, 2023, Colton Lewis wrote:
> Oliver Upton <[email protected]> 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 <[email protected]>
>
> > 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'.
Oliver Upton <[email protected]> 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 <[email protected]>
> 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.