* [PATCH] KVM: selftests: Print a message if /dev/kvm is missing
@ 2021-05-07 19:05 David Matlack
2021-05-07 20:14 ` Andrew Jones
0 siblings, 1 reply; 5+ messages in thread
From: David Matlack @ 2021-05-07 19:05 UTC (permalink / raw)
To: kvm; +Cc: Paolo Bonzini, Andrew Jones, Aaron Lewis, David Matlack
If a KVM selftest is run on a machine without /dev/kvm, it will exit
silently. Make it easy to tell what's happening by printing an error
message.
Opportunistically consolidate all codepaths that open /dev/kvm into a
single function so they all print the same message.
This slightly changes the semantics of vm_is_unrestricted_guest() by
changing a TEST_ASSERT() to exit(KSFT_SKIP). However
vm_is_unrestricted_guest() is only called in one place
(x86_64/mmio_warning_test.c) and that is to determine if the test should
be skipped or not.
Signed-off-by: David Matlack <dmatlack@google.com>
---
.../testing/selftests/kvm/include/kvm_util.h | 1 +
tools/testing/selftests/kvm/lib/kvm_util.c | 35 +++++++++++--------
.../selftests/kvm/lib/x86_64/processor.c | 16 +++------
.../kvm/x86_64/get_msr_index_features.c | 8 ++---
4 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index a8f022794ce3..84982eb02b29 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -77,6 +77,7 @@ struct vm_guest_mode_params {
};
extern const struct vm_guest_mode_params vm_guest_mode_params[];
+int open_kvm_dev_path_or_exit(void);
int kvm_check_cap(long cap);
int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id,
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index fc83f6c5902d..bb7dc65d7fb5 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -31,6 +31,23 @@ static void *align(void *x, size_t size)
return (void *) (((size_t) x + mask) & ~mask);
}
+/* Open KVM_DEV_PATH if available, otherwise exit the entire program.
+ *
+ * Return:
+ * The opened file descriptor of /dev/kvm.
+ */
+int open_kvm_dev_path_or_exit(void) {
+ int fd;
+
+ fd = open(KVM_DEV_PATH, O_RDONLY);
+ if (fd < 0) {
+ print_skip("%s not available", KVM_DEV_PATH);
+ exit(KSFT_SKIP);
+ }
+
+ return fd;
+}
+
/*
* Capability
*
@@ -52,10 +69,7 @@ int kvm_check_cap(long cap)
int ret;
int kvm_fd;
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
-
+ kvm_fd = open_kvm_dev_path_or_exit();
ret = ioctl(kvm_fd, KVM_CHECK_EXTENSION, cap);
TEST_ASSERT(ret != -1, "KVM_CHECK_EXTENSION IOCTL failed,\n"
" rc: %i errno: %i", ret, errno);
@@ -128,9 +142,7 @@ void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size)
static void vm_open(struct kvm_vm *vm, int perm)
{
- vm->kvm_fd = open(KVM_DEV_PATH, perm);
- if (vm->kvm_fd < 0)
- exit(KSFT_SKIP);
+ vm->kvm_fd = open_kvm_dev_path_or_exit();
if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
print_skip("immediate_exit not available");
@@ -925,9 +937,7 @@ static int vcpu_mmap_sz(void)
{
int dev_fd, ret;
- dev_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (dev_fd < 0)
- exit(KSFT_SKIP);
+ dev_fd = open_kvm_dev_path_or_exit();
ret = ioctl(dev_fd, KVM_GET_VCPU_MMAP_SIZE, NULL);
TEST_ASSERT(ret >= sizeof(struct kvm_run),
@@ -2015,10 +2025,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
if (vm == NULL) {
/* Ensure that the KVM vendor-specific module is loaded. */
- f = fopen(KVM_DEV_PATH, "r");
- TEST_ASSERT(f != NULL, "Error in opening KVM dev file: %d",
- errno);
- fclose(f);
+ close(open_kvm_dev_path_or_exit());
}
f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index a8906e60a108..efe235044421 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -657,9 +657,7 @@ struct kvm_cpuid2 *kvm_get_supported_cpuid(void)
return cpuid;
cpuid = allocate_kvm_cpuid2();
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_CPUID failed %d %d\n",
@@ -691,9 +689,7 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index)
buffer.header.nmsrs = 1;
buffer.entry.index = msr_index;
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
@@ -986,9 +982,7 @@ struct kvm_msr_list *kvm_get_msr_index_list(void)
struct kvm_msr_list *list;
int nmsrs, r, kvm_fd;
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
nmsrs = kvm_get_num_msrs_fd(kvm_fd);
list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
@@ -1312,9 +1306,7 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void)
return cpuid;
cpuid = allocate_kvm_cpuid2();
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_HV_CPUID failed %d %d\n",
diff --git a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
index cb953df4d7d0..8aed0db1331d 100644
--- a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
+++ b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
@@ -37,9 +37,7 @@ static void test_get_msr_index(void)
int old_res, res, kvm_fd, r;
struct kvm_msr_list *list;
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
old_res = kvm_num_index_msrs(kvm_fd, 0);
TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
@@ -101,9 +99,7 @@ static void test_get_msr_feature(void)
int res, old_res, i, kvm_fd;
struct kvm_msr_list *feature_list;
- kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
- if (kvm_fd < 0)
- exit(KSFT_SKIP);
+ kvm_fd = open_kvm_dev_path_or_exit();
old_res = kvm_num_feature_msrs(kvm_fd, 0);
TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
--
2.31.1.607.g51e8a6a459-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Print a message if /dev/kvm is missing
2021-05-07 19:05 [PATCH] KVM: selftests: Print a message if /dev/kvm is missing David Matlack
@ 2021-05-07 20:14 ` Andrew Jones
2021-05-07 20:51 ` David Matlack
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Jones @ 2021-05-07 20:14 UTC (permalink / raw)
To: David Matlack; +Cc: kvm, Paolo Bonzini, Aaron Lewis
On Fri, May 07, 2021 at 07:05:59PM +0000, David Matlack wrote:
> If a KVM selftest is run on a machine without /dev/kvm, it will exit
> silently. Make it easy to tell what's happening by printing an error
> message.
>
> Opportunistically consolidate all codepaths that open /dev/kvm into a
> single function so they all print the same message.
>
> This slightly changes the semantics of vm_is_unrestricted_guest() by
> changing a TEST_ASSERT() to exit(KSFT_SKIP). However
> vm_is_unrestricted_guest() is only called in one place
> (x86_64/mmio_warning_test.c) and that is to determine if the test should
> be skipped or not.
>
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
> .../testing/selftests/kvm/include/kvm_util.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 35 +++++++++++--------
> .../selftests/kvm/lib/x86_64/processor.c | 16 +++------
> .../kvm/x86_64/get_msr_index_features.c | 8 ++---
> 4 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index a8f022794ce3..84982eb02b29 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -77,6 +77,7 @@ struct vm_guest_mode_params {
> };
> extern const struct vm_guest_mode_params vm_guest_mode_params[];
>
> +int open_kvm_dev_path_or_exit(void);
> int kvm_check_cap(long cap);
> int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
> int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id,
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index fc83f6c5902d..bb7dc65d7fb5 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -31,6 +31,23 @@ static void *align(void *x, size_t size)
> return (void *) (((size_t) x + mask) & ~mask);
> }
>
> +/* Open KVM_DEV_PATH if available, otherwise exit the entire program.
> + *
> + * Return:
> + * The opened file descriptor of /dev/kvm.
> + */
> +int open_kvm_dev_path_or_exit(void) {
> + int fd;
> +
> + fd = open(KVM_DEV_PATH, O_RDONLY);
> + if (fd < 0) {
> + print_skip("%s not available", KVM_DEV_PATH);
> + exit(KSFT_SKIP);
> + }
> +
> + return fd;
> +}
Style issues in the function above '{' and 2 spaces vs. 1 tab.
> +
> /*
> * Capability
> *
> @@ -52,10 +69,7 @@ int kvm_check_cap(long cap)
> int ret;
> int kvm_fd;
>
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> -
> + kvm_fd = open_kvm_dev_path_or_exit();
> ret = ioctl(kvm_fd, KVM_CHECK_EXTENSION, cap);
> TEST_ASSERT(ret != -1, "KVM_CHECK_EXTENSION IOCTL failed,\n"
> " rc: %i errno: %i", ret, errno);
> @@ -128,9 +142,7 @@ void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size)
>
> static void vm_open(struct kvm_vm *vm, int perm)
> {
> - vm->kvm_fd = open(KVM_DEV_PATH, perm);
I don't think we should change this one, otherwise the user provided
perms are ignored.
> - if (vm->kvm_fd < 0)
> - exit(KSFT_SKIP);
> + vm->kvm_fd = open_kvm_dev_path_or_exit();
>
> if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
> print_skip("immediate_exit not available");
> @@ -925,9 +937,7 @@ static int vcpu_mmap_sz(void)
> {
> int dev_fd, ret;
>
> - dev_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (dev_fd < 0)
> - exit(KSFT_SKIP);
> + dev_fd = open_kvm_dev_path_or_exit();
spaces vs. tab here
>
> ret = ioctl(dev_fd, KVM_GET_VCPU_MMAP_SIZE, NULL);
> TEST_ASSERT(ret >= sizeof(struct kvm_run),
> @@ -2015,10 +2025,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>
> if (vm == NULL) {
> /* Ensure that the KVM vendor-specific module is loaded. */
> - f = fopen(KVM_DEV_PATH, "r");
> - TEST_ASSERT(f != NULL, "Error in opening KVM dev file: %d",
> - errno);
> - fclose(f);
> + close(open_kvm_dev_path_or_exit());
spaces
> }
>
> f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index a8906e60a108..efe235044421 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -657,9 +657,7 @@ struct kvm_cpuid2 *kvm_get_supported_cpuid(void)
> return cpuid;
>
> cpuid = allocate_kvm_cpuid2();
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
> TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_CPUID failed %d %d\n",
> @@ -691,9 +689,7 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index)
>
> buffer.header.nmsrs = 1;
> buffer.entry.index = msr_index;
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
> TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
> @@ -986,9 +982,7 @@ struct kvm_msr_list *kvm_get_msr_index_list(void)
> struct kvm_msr_list *list;
> int nmsrs, r, kvm_fd;
>
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> nmsrs = kvm_get_num_msrs_fd(kvm_fd);
> list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
> @@ -1312,9 +1306,7 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void)
> return cpuid;
>
> cpuid = allocate_kvm_cpuid2();
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
> TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_HV_CPUID failed %d %d\n",
> diff --git a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> index cb953df4d7d0..8aed0db1331d 100644
> --- a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> @@ -37,9 +37,7 @@ static void test_get_msr_index(void)
> int old_res, res, kvm_fd, r;
> struct kvm_msr_list *list;
>
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> old_res = kvm_num_index_msrs(kvm_fd, 0);
> TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
> @@ -101,9 +99,7 @@ static void test_get_msr_feature(void)
> int res, old_res, i, kvm_fd;
> struct kvm_msr_list *feature_list;
>
> - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> - if (kvm_fd < 0)
> - exit(KSFT_SKIP);
> + kvm_fd = open_kvm_dev_path_or_exit();
>
> old_res = kvm_num_feature_msrs(kvm_fd, 0);
> TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
> --
> 2.31.1.607.g51e8a6a459-goog
>
Thanks,
drew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Print a message if /dev/kvm is missing
2021-05-07 20:14 ` Andrew Jones
@ 2021-05-07 20:51 ` David Matlack
2021-05-10 6:32 ` Andrew Jones
0 siblings, 1 reply; 5+ messages in thread
From: David Matlack @ 2021-05-07 20:51 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm list, Paolo Bonzini, Aaron Lewis
On Fri, May 7, 2021 at 1:14 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, May 07, 2021 at 07:05:59PM +0000, David Matlack wrote:
> > If a KVM selftest is run on a machine without /dev/kvm, it will exit
> > silently. Make it easy to tell what's happening by printing an error
> > message.
> >
> > Opportunistically consolidate all codepaths that open /dev/kvm into a
> > single function so they all print the same message.
> >
> > This slightly changes the semantics of vm_is_unrestricted_guest() by
> > changing a TEST_ASSERT() to exit(KSFT_SKIP). However
> > vm_is_unrestricted_guest() is only called in one place
> > (x86_64/mmio_warning_test.c) and that is to determine if the test should
> > be skipped or not.
> >
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > ---
> > .../testing/selftests/kvm/include/kvm_util.h | 1 +
> > tools/testing/selftests/kvm/lib/kvm_util.c | 35 +++++++++++--------
> > .../selftests/kvm/lib/x86_64/processor.c | 16 +++------
> > .../kvm/x86_64/get_msr_index_features.c | 8 ++---
> > 4 files changed, 28 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index a8f022794ce3..84982eb02b29 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -77,6 +77,7 @@ struct vm_guest_mode_params {
> > };
> > extern const struct vm_guest_mode_params vm_guest_mode_params[];
> >
> > +int open_kvm_dev_path_or_exit(void);
> > int kvm_check_cap(long cap);
> > int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
> > int vcpu_enable_cap(struct kvm_vm *vm, uint32_t vcpu_id,
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index fc83f6c5902d..bb7dc65d7fb5 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -31,6 +31,23 @@ static void *align(void *x, size_t size)
> > return (void *) (((size_t) x + mask) & ~mask);
> > }
> >
> > +/* Open KVM_DEV_PATH if available, otherwise exit the entire program.
> > + *
> > + * Return:
> > + * The opened file descriptor of /dev/kvm.
> > + */
> > +int open_kvm_dev_path_or_exit(void) {
> > + int fd;
> > +
> > + fd = open(KVM_DEV_PATH, O_RDONLY);
> > + if (fd < 0) {
> > + print_skip("%s not available", KVM_DEV_PATH);
> > + exit(KSFT_SKIP);
> > + }
> > +
> > + return fd;
> > +}
>
> Style issues in the function above '{' and 2 spaces vs. 1 tab.
Well that's sure embarrassing! Thanks for catching.
>
> > +
> > /*
> > * Capability
> > *
> > @@ -52,10 +69,7 @@ int kvm_check_cap(long cap)
> > int ret;
> > int kvm_fd;
> >
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > -
> > + kvm_fd = open_kvm_dev_path_or_exit();
> > ret = ioctl(kvm_fd, KVM_CHECK_EXTENSION, cap);
> > TEST_ASSERT(ret != -1, "KVM_CHECK_EXTENSION IOCTL failed,\n"
> > " rc: %i errno: %i", ret, errno);
> > @@ -128,9 +142,7 @@ void vm_enable_dirty_ring(struct kvm_vm *vm, uint32_t ring_size)
> >
> > static void vm_open(struct kvm_vm *vm, int perm)
> > {
> > - vm->kvm_fd = open(KVM_DEV_PATH, perm);
>
> I don't think we should change this one, otherwise the user provided
> perms are ignored.
Good catch. I don't see any reason to exclude this case, but we do need
to pass `perm` down to open_kvm_dev_path_or_exit().
>
> > - if (vm->kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + vm->kvm_fd = open_kvm_dev_path_or_exit();
> >
> > if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
> > print_skip("immediate_exit not available");
> > @@ -925,9 +937,7 @@ static int vcpu_mmap_sz(void)
> > {
> > int dev_fd, ret;
> >
> > - dev_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (dev_fd < 0)
> > - exit(KSFT_SKIP);
> > + dev_fd = open_kvm_dev_path_or_exit();
>
> spaces vs. tab here
>
> >
> > ret = ioctl(dev_fd, KVM_GET_VCPU_MMAP_SIZE, NULL);
> > TEST_ASSERT(ret >= sizeof(struct kvm_run),
> > @@ -2015,10 +2025,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> >
> > if (vm == NULL) {
> > /* Ensure that the KVM vendor-specific module is loaded. */
> > - f = fopen(KVM_DEV_PATH, "r");
> > - TEST_ASSERT(f != NULL, "Error in opening KVM dev file: %d",
> > - errno);
> > - fclose(f);
> > + close(open_kvm_dev_path_or_exit());
>
> spaces
>
> > }
> >
> > f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index a8906e60a108..efe235044421 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -657,9 +657,7 @@ struct kvm_cpuid2 *kvm_get_supported_cpuid(void)
> > return cpuid;
> >
> > cpuid = allocate_kvm_cpuid2();
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
> > TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_CPUID failed %d %d\n",
> > @@ -691,9 +689,7 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index)
> >
> > buffer.header.nmsrs = 1;
> > buffer.entry.index = msr_index;
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > r = ioctl(kvm_fd, KVM_GET_MSRS, &buffer.header);
> > TEST_ASSERT(r == 1, "KVM_GET_MSRS IOCTL failed,\n"
> > @@ -986,9 +982,7 @@ struct kvm_msr_list *kvm_get_msr_index_list(void)
> > struct kvm_msr_list *list;
> > int nmsrs, r, kvm_fd;
> >
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > nmsrs = kvm_get_num_msrs_fd(kvm_fd);
> > list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
> > @@ -1312,9 +1306,7 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void)
> > return cpuid;
> >
> > cpuid = allocate_kvm_cpuid2();
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > ret = ioctl(kvm_fd, KVM_GET_SUPPORTED_HV_CPUID, cpuid);
> > TEST_ASSERT(ret == 0, "KVM_GET_SUPPORTED_HV_CPUID failed %d %d\n",
> > diff --git a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> > index cb953df4d7d0..8aed0db1331d 100644
> > --- a/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> > +++ b/tools/testing/selftests/kvm/x86_64/get_msr_index_features.c
> > @@ -37,9 +37,7 @@ static void test_get_msr_index(void)
> > int old_res, res, kvm_fd, r;
> > struct kvm_msr_list *list;
> >
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > old_res = kvm_num_index_msrs(kvm_fd, 0);
> > TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
> > @@ -101,9 +99,7 @@ static void test_get_msr_feature(void)
> > int res, old_res, i, kvm_fd;
> > struct kvm_msr_list *feature_list;
> >
> > - kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
> > - if (kvm_fd < 0)
> > - exit(KSFT_SKIP);
> > + kvm_fd = open_kvm_dev_path_or_exit();
> >
> > old_res = kvm_num_feature_msrs(kvm_fd, 0);
> > TEST_ASSERT(old_res != 0, "Expecting nmsrs to be > 0");
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Print a message if /dev/kvm is missing
2021-05-07 20:51 ` David Matlack
@ 2021-05-10 6:32 ` Andrew Jones
2021-05-10 17:23 ` David Matlack
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Jones @ 2021-05-10 6:32 UTC (permalink / raw)
To: David Matlack; +Cc: kvm list, Paolo Bonzini, Aaron Lewis
On Fri, May 07, 2021 at 01:51:45PM -0700, David Matlack wrote:
> > > static void vm_open(struct kvm_vm *vm, int perm)
> > > {
> > > - vm->kvm_fd = open(KVM_DEV_PATH, perm);
> >
> > I don't think we should change this one, otherwise the user provided
> > perms are ignored.
>
> Good catch. I don't see any reason to exclude this case, but we do need
> to pass `perm` down to open_kvm_dev_path_or_exit().
>
I've reviewed v2 and gave it an r-b, since I don't have overly strong
opinion about this, but I actually liked that open_kvm_dev_path_or_exit()
didn't take any arguments. To handle this case I would have either left
it open coded, like it was, or created something like
int _open_kvm_dev_path_or_exit(int flags)
{
int fd = open(KVM_DEV_PATH, flags);
if (fd < 0)
...exit skip...
return fd;
}
int open_kvm_dev_path_or_exit(void)
{
return _open_kvm_dev_path_or_exit(O_RDONLY);
}
Thanks,
drew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Print a message if /dev/kvm is missing
2021-05-10 6:32 ` Andrew Jones
@ 2021-05-10 17:23 ` David Matlack
0 siblings, 0 replies; 5+ messages in thread
From: David Matlack @ 2021-05-10 17:23 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm list, Paolo Bonzini, Aaron Lewis
On Sun, May 9, 2021 at 11:32 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, May 07, 2021 at 01:51:45PM -0700, David Matlack wrote:
> > > > static void vm_open(struct kvm_vm *vm, int perm)
> > > > {
> > > > - vm->kvm_fd = open(KVM_DEV_PATH, perm);
> > >
> > > I don't think we should change this one, otherwise the user provided
> > > perms are ignored.
> >
> > Good catch. I don't see any reason to exclude this case, but we do need
> > to pass `perm` down to open_kvm_dev_path_or_exit().
> >
>
> I've reviewed v2 and gave it an r-b, since I don't have overly strong
> opinion about this, but I actually liked that open_kvm_dev_path_or_exit()
> didn't take any arguments. To handle this case I would have either left
> it open coded, like it was, or created something like
>
> int _open_kvm_dev_path_or_exit(int flags)
> {
> int fd = open(KVM_DEV_PATH, flags);
> if (fd < 0)
> ...exit skip...
> return fd;
> }
>
> int open_kvm_dev_path_or_exit(void)
> {
> return _open_kvm_dev_path_or_exit(O_RDONLY);
> }
I agree so long as hiding O_RDONLY does not decrease code readability.
I could not find any place in KVM where the R/W permissions on
/dev/kvm played a role, so I'm inclined to agree it's better to hide
the permissions.
I'll send another version with this change along with the comment fix.
Thanks.
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-10 17:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 19:05 [PATCH] KVM: selftests: Print a message if /dev/kvm is missing David Matlack
2021-05-07 20:14 ` Andrew Jones
2021-05-07 20:51 ` David Matlack
2021-05-10 6:32 ` Andrew Jones
2021-05-10 17:23 ` David Matlack
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).