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

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

[Oliver Upton]

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));
+	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

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

[David Matlack]

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.

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
> 

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

[Oliver Upton]

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.

Can't folks just update their kernel? :-)

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

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

[David Matlack]

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

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

[Sean Christopherson]

On Mon, Aug 15, 2022, David Matlack wrote:
> 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.

...

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

Yeah, I'm somewhat ambivalent as well.  All things considered, I think it makes
sense to skip the entire test.  Like Oliver said, without first-class support,
this will become a mess.  And I'm not convinced that adding first-class support
is a good idea, as that will inevitably lead to tests ballooning to include a big
pile of subtests, a la KUT's VMX test.  I would much rather steer selftests in a
"one test per binary" direction; IMO it's easier to do filtering via scripts, and
it also minimizes the chances of creating subtle dependencies between (sub)tests.

So, pushed to branch `for_paolo/6.1` at:

    https://github.com/sean-jc/linux.git

but with a rewritten shortlog+changelog to capture this conversation.  And that's
a good lesson for the future as well: when piggybacking a patch, making functional
changes as the "opportunistic cleanup" is rarely the right thing to do.

This is what I ended up with, holler if anything about it bothers you.

    KVM: selftests: Require DISABLE_NX_HUGE_PAGES cap for NX hugepage test
    
    Require KVM_CAP_VM_DISABLE_NX_HUGE_PAGES for the entire NX hugepage test
    instead of skipping the "disable" subtest if the capability isn't
    supported by the host kernel.  While the "enable" subtest does provide
    value when the capability isn't supported, silently providing only half
    the promised coveraged is undesirable, i.e. it's better to skip the test
    so that the user knows something.
    
    Alternatively, the test could print something to alert the user instead
    of silently skipping the subtest, but that would encourage other tests
    to follow suit, and it's not clear that it's desirable to take selftests
    in that direction.  And if selftests do head down the path of skipping
    subtests, such behavior needs first-class support in the framework.
    
    Opportunistically convert other test preconditions to TEST_REQUIRE().
    
    Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
    Reviewed-by: David Matlack <dmatlack@google.com>
    Link: https://lore.kernel.org/r/20220812175301.3915004-1-oliver.upton@linux.dev
    [sean: rewrote changelog to capture discussion about skipping the test]
    Signed-off-by: Sean Christopherson <seanjc@google.com>