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

[Sean Christopherson <seanjc@google.com>]

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>