On Wed, Sep 28, 2022 at 3:51 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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.
