On Fri, Aug 12, 2022 at 07:16:49PM +0000, Oliver Upton wrote:
> On Fri, Aug 12, 2022 at 11:04:25AM -0700, David Matlack wrote:
> > On Fri, Aug 12, 2022 at 05:53:01PM +0000, Oliver Upton wrote:
> > > Avoid boilerplate for checking test preconditions by using
> > > TEST_REQUIRE(). While at it, add a precondition for
> > > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on
> > > older kernels.
> > >
> > > Signed-off-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> > > ---
> > > .../selftests/kvm/x86_64/nx_huge_pages_test.c | 24 +++++--------------
> > > 1 file changed, 6 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> > > index cc6421716400..e19933ea34ca 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> > > @@ -118,13 +118,6 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages,
> > > vm = vm_create(1);
> > >
> > > if (disable_nx_huge_pages) {
> > > - /*
> > > - * Cannot run the test without NX huge pages if the kernel
> > > - * does not support it.
> > > - */
> > > - if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES))
> > > - return;
> > > -
> > > r = __vm_disable_nx_huge_pages(vm);
> > > if (reboot_permissions) {
> > > TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions");
> > > @@ -248,18 +241,13 @@ int main(int argc, char **argv)
> > > }
> > > }
> > >
> > > - if (token != MAGIC_TOKEN) {
> > > - print_skip("This test must be run with the magic token %d.\n"
> > > - "This is done by nx_huge_pages_test.sh, which\n"
> > > - "also handles environment setup for the test.",
> > > - MAGIC_TOKEN);
> > > - exit(KSFT_SKIP);
> > > - }
> > > + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES));
> >
> > This cap is only needed for run_test(..., true, ...) below so I don't think we should require it for the entire test.
>
> It has always seemed that the test preconditions are a way to pretty-print
> a failure/skip instead of having some random ioctl fail deeper in the
> test.
>
> If we really see value in adding predicates for individual test cases
> then IMO it deserves first-class support in our framework. Otherwise
> the next test that comes along is bound to open-code the same thing.
Fair point.
>
> Can't folks just update their kernel? :-)
Consider my suggestion optional. If anyone is backporting this test to
their kernel they'll also probably backport
KVM_CAP_VM_DISABLE_NX_HUGE_PAGES ;). So I don't think there will be a
huge benefit of making the test more flexible.
That aside,
Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
>
> --
> Thanks,
> Oliver
>
> > That being said, it still might be good to inform the user that the test is being skipped. So perhaps something like this:
> >
> > ...
> > run_test(reclaim_period_ms, false, reboot_permissions);
> >
> > if (kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES))
> > run_test(reclaim_period_ms, true, reboot_permissions);
> > else
> > print_skip("KVM_CAP_VM_DISABLE_NX_HUGE_PAGES not supported");
> > ...
> >
> > > + TEST_REQUIRE(reclaim_period_ms > 0);
> > >
> > > - if (!reclaim_period_ms) {
> > > - print_skip("The NX reclaim period must be specified and non-zero");
> > > - exit(KSFT_SKIP);
> > > - }
> > > + __TEST_REQUIRE(token == MAGIC_TOKEN,
> > > + "This test must be run with the magic token %d.\n"
> > > + "This is done by nx_huge_pages_test.sh, which\n"
> > > + "also handles environment setup for the test.");
> > >
> > > run_test(reclaim_period_ms, false, reboot_permissions);
> > > run_test(reclaim_period_ms, true, reboot_permissions);
> > >
> > > base-commit: 93472b79715378a2386598d6632c654a2223267b
> > > --
> > > 2.37.1.595.g718a3a8f04-goog
> > >
