Re: [PATCH] KVM: selftests: Use TEST_REQUIRE() in nx_huge_pages_test

[David Matlack <dmatlack@google.com>]

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@linux.dev>
> > > ---
> > >  .../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@google.com>

> 
> --
> 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
> > >