Re: [PATCH] KVM: selftests: Skip tests that require EPT when it is not available

From: David Matlack <dmatlack@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>,
	kvm list <kvm@vger.kernel.org>
Subject: Re: [PATCH] KVM: selftests: Skip tests that require EPT when it is not available
Date: Mon, 26 Sep 2022 15:18:22 -0700	[thread overview]
Message-ID: <CALzav=d-4a8yPxPUuHNh1884Z4Pe_0ewMwnunGK_jAAvr9L-vw@mail.gmail.com> (raw)
In-Reply-To: <YzIdfkovobW3w/zk@google.com>

On Mon, Sep 26, 2022 at 2:45 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Sep 26, 2022, David Matlack wrote:
> > +bool kvm_vm_has_ept(struct kvm_vm *vm)
> > +{
> > +     struct kvm_vcpu *vcpu;
> > +     uint64_t ctrl;
> > +
> > +     vcpu = list_first_entry(&vm->vcpus, struct kvm_vcpu, list);
> > +     TEST_ASSERT(vcpu, "Cannot determine EPT support without vCPUs.\n");
>
> KVM_GET_MSRS is supported on /dev/kvm for feature MSRs, and is available for
> selftests via kvm_get_feature_msr().

Ack.

>
> > +
> > +     ctrl = vcpu_get_msr(vcpu, MSR_IA32_VMX_TRUE_PROCBASED_CTLS) >> 32;
> > +     if (!(ctrl & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS))
> > +             return false;
> > +
> > +     ctrl = vcpu_get_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2) >> 32;
> > +     return ctrl & SECONDARY_EXEC_ENABLE_EPT;
> > +}
> > +
> >  void prepare_eptp(struct vmx_pages *vmx, struct kvm_vm *vm,
> >                 uint32_t eptp_memslot)
> >  {
> > +     TEST_REQUIRE(kvm_vm_has_ept(vm));
>
> I would much rather this be an assert, i.e. force the test to do TEST_REQUIRE(),
> even if that means duplicate code.  One of the roles of TEST_REQUIRE() is to
> document test requirements.

This gets difficult when you consider dirty_log_perf_test. Users can
use dirty_log_perf_test in nested mode by passing "-n". But
dirty_log_perf_test is an architecture-neutral test, so adding
TEST_REQUIRE() there would require an ifdef, and then gets even more
complicated if we add support for AMD or nested on non-x86
architectures.

One option is to put the TEST_REQUIRE() in the x86-specific
perf_test_setup_ept(), but that is only one level above
prepare_eptp(), so not exactly any better at documenting the
requirement.

Another option is to do nothing and let the test fail if running on
hosts without EPT. I don't like this solution though, because that
means developers and automation would need custom logic to skip
running dirty_log_perf_test with -n depending on the state of the
host. (For context: I typically run all selftests and kvm-unit-tests
against kvm with ept=Y and ept=N.)

At least for vmx_dirty_log_test, the TEST_REQUIRE() could be put in main().