On Mon, Sep 26, 2022 at 2:45 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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().
