| Message ID | 20220928184853.1681781-3-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Fix nx_huge_pages_test when TDP is disabled | expand |
On Wed, Sep 28, 2022, David Matlack wrote: > @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...) > puts(", skipping test"); > } > > +bool get_module_param_bool(const char *module_name, const char *param) > +{ > + const int path_size = 1024; > + char path[path_size]; > + char value; > + FILE *f; > + int r; > + > + r = snprintf(path, path_size, "/sys/module/%s/parameters/%s", > + module_name, param); > + TEST_ASSERT(r < path_size, > + "Failed to construct sysfs path in %d bytes.", path_size); > + > + f = fopen(path, "r"); Any particular reason for using fopen()? Oh, because that's what the existing code does. More below. > + TEST_ASSERT(f, "fopen(%s) failed", path); I don't actually care myself, but for consistency this should probably be a skip condition. The easiest thing would be to use open_path_or_exit(). At that point, assuming read() instead of fread() does the right thin, that seems like the easiest solution. > + TEST_FAIL("Unrecognized value: %c", value); Maybe be slightly more verbose? E.g. TEST_FAIL("Unrecognized value '%c' for boolean module param", value); > +} > + > bool thp_configured(void) > { > int ret; > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index 2e6e61bbe81b..522d3e2009fb 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm) > /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */ > bool vm_is_unrestricted_guest(struct kvm_vm *vm) > { > - char val = 'N'; > - size_t count; > - FILE *f; > - > /* Ensure that a KVM vendor-specific module is loaded. */ > if (vm == NULL) > close(open_kvm_dev_path_or_exit()); > > - f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r"); > - if (f) { > - count = fread(&val, sizeof(char), 1, f); > - TEST_ASSERT(count == 1, "Unable to read from param file."); > - fclose(f); > - } > - > - return val == 'Y'; > + return get_module_param_bool("kvm_intel", "unrestricted_guest"); Since there are only three possible modules, what about providing wrappers to handle "kvm", "kvm_amd", and "kvm_intel"? I'm guessing we'll end up with wrappers for each param we care about, but one fewer strings to get right would be nice.
On Wed, Sep 28, 2022 at 3:51 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Sep 28, 2022, David Matlack wrote: > > @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...) > > puts(", skipping test"); > > } > > > > +bool get_module_param_bool(const char *module_name, const char *param) > > +{ > > + const int path_size = 1024; (Unrelated to your review but I noticed 1024 is overkill. e.g. "/sys/module/kvm_intel/parameters/error_on_inconsistent_vmcs_config" is only 67 characters. I will reduce this in v3.) > > + char path[path_size]; > > + char value; > > + FILE *f; > > + int r; > > + > > + r = snprintf(path, path_size, "/sys/module/%s/parameters/%s", > > + module_name, param); > > + TEST_ASSERT(r < path_size, > > + "Failed to construct sysfs path in %d bytes.", path_size); > > + > > + f = fopen(path, "r"); > > Any particular reason for using fopen()? Oh, because that's what the existing > code does. More below. Heh, yes. I write C code to do file I/O about once every 2 years so I always need to reference existing code :) > > > + TEST_ASSERT(f, "fopen(%s) failed", path); > > I don't actually care myself, but for consistency this should probably be a > skip condition. The easiest thing would be to use open_path_or_exit(). > > At that point, assuming read() instead of fread() does the right thin, that seems > like the easiest solution. Fine by me! > > > + TEST_FAIL("Unrecognized value: %c", value); > > Maybe be slightly more verbose? E.g. > > TEST_FAIL("Unrecognized value '%c' for boolean module param", value); Will do. > > > +} > > + > > bool thp_configured(void) > > { > > int ret; > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > index 2e6e61bbe81b..522d3e2009fb 100644 > > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm) > > /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */ > > bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > { > > - char val = 'N'; > > - size_t count; > > - FILE *f; > > - > > /* Ensure that a KVM vendor-specific module is loaded. */ > > if (vm == NULL) > > close(open_kvm_dev_path_or_exit()); > > > > - f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r"); > > - if (f) { > > - count = fread(&val, sizeof(char), 1, f); > > - TEST_ASSERT(count == 1, "Unable to read from param file."); > > - fclose(f); > > - } > > - > > - return val == 'Y'; > > + return get_module_param_bool("kvm_intel", "unrestricted_guest"); > > Since there are only three possible modules, what about providing wrappers to > handle "kvm", "kvm_amd", and "kvm_intel"? I'm guessing we'll end up with wrappers > for each param we care about, but one fewer strings to get right would be nice. Will do.
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h index befc754ce9b3..4f119fd84ae5 100644 --- a/tools/testing/selftests/kvm/include/test_util.h +++ b/tools/testing/selftests/kvm/include/test_util.h @@ -108,6 +108,7 @@ struct vm_mem_backing_src_alias { #define MIN_RUN_DELAY_NS 200000UL +bool get_module_param_bool(const char *module_name, const char *param); bool thp_configured(void); size_t get_trans_hugepagesz(void); size_t get_def_hugetlb_pagesz(void); diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c index 6d23878bbfe1..479e482d3202 100644 --- a/tools/testing/selftests/kvm/lib/test_util.c +++ b/tools/testing/selftests/kvm/lib/test_util.c @@ -9,6 +9,7 @@ #include <ctype.h> #include <limits.h> #include <stdlib.h> +#include <stdio.h> #include <time.h> #include <sys/stat.h> #include <sys/syscall.h> @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...) puts(", skipping test"); } +bool get_module_param_bool(const char *module_name, const char *param) +{ + const int path_size = 1024; + char path[path_size]; + char value; + FILE *f; + int r; + + r = snprintf(path, path_size, "/sys/module/%s/parameters/%s", + module_name, param); + TEST_ASSERT(r < path_size, + "Failed to construct sysfs path in %d bytes.", path_size); + + f = fopen(path, "r"); + TEST_ASSERT(f, "fopen(%s) failed", path); + + r = fread(&value, 1, 1, f); + TEST_ASSERT(r == 1, "fread(%s) failed", path); + + r = fclose(f); + TEST_ASSERT(!r, "fclose(%s) failed", path); + + if (value == 'Y') + return true; + else if (value == 'N') + return false; + + TEST_FAIL("Unrecognized value: %c", value); +} + bool thp_configured(void) { int ret; diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 2e6e61bbe81b..522d3e2009fb 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm) /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */ bool vm_is_unrestricted_guest(struct kvm_vm *vm) { - char val = 'N'; - size_t count; - FILE *f; - /* Ensure that a KVM vendor-specific module is loaded. */ if (vm == NULL) close(open_kvm_dev_path_or_exit()); - f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r"); - if (f) { - count = fread(&val, sizeof(char), 1, f); - TEST_ASSERT(count == 1, "Unable to read from param file."); - fclose(f); - } - - return val == 'Y'; + return get_module_param_bool("kvm_intel", "unrestricted_guest"); }
Add a common helper function for reading boolean module parameters and use it in vm_is_unrestricted_guest() to check the value of kvm_intel.unrestricted_guest. Note that vm_is_unrestricted_guest() will now fail with a TEST_ASSERT() if called on AMD instead of just returning false. However this should not cause any functional change since all of the callers of vm_is_unrestricted_guest() first check is_intel_cpu(). No functional change intended. Signed-off-by: David Matlack <dmatlack@google.com> --- .../testing/selftests/kvm/include/test_util.h | 1 + tools/testing/selftests/kvm/lib/test_util.c | 31 +++++++++++++++++++ .../selftests/kvm/lib/x86_64/processor.c | 13 +------- 3 files changed, 33 insertions(+), 12 deletions(-)