| Message ID | 20220926171457.532542-1-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Skip tests that require EPT when it is not available | expand |
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(). > + > + 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.
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().
On Mon, Sep 26, 2022, David Matlack wrote: > On Mon, Sep 26, 2022 at 2:45 PM Sean Christopherson <seanjc@google.com> wrote: > > 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. But you're going to have that problem regardless, e.g. perf_test_setup_nested() already has TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX)); so why not also have the EPT requirement there? IMO, it's far less confusing if the hard requirements are collected together, even if that one location isn't ideal. If someone wants to improve the memstress framework, a hook can be added for probing nested requirements. In other words, IMO this is a shortcoming of the memstress code, not a valid reason to shove the requirement down in common code. > Another option is to do nothing and let the test fail if running on > hosts without EPT. I don't like this solution though Neither do I.
On Mon, Sep 26, 2022 at 3:29 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Sep 26, 2022, David Matlack wrote: > > On Mon, Sep 26, 2022 at 2:45 PM Sean Christopherson <seanjc@google.com> wrote: > > > 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. > > But you're going to have that problem regardless, e.g. perf_test_setup_nested() > already has > > TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX)); > > so why not also have the EPT requirement there? IMO, it's far less confusing if > the hard requirements are collected together, even if that one location isn't ideal. Ack, can do. > > If someone wants to improve the memstress framework, a hook can be added for probing > nested requirements. In other words, IMO this is a shortcoming of the memstress code, > not a valid reason to shove the requirement down in common code. Sorry I forgot to ask this in my previous reply... Why do you prefer to decouple test requirements from the test setup code? There is a maintenance burden to such an approach, so I want to understand the benefit. e.g. I forsee myself having to send patches in the future to add TEST_REQUIRE(kvm_cpu_has_ept()), because someone added a new VMX test and forgot to test with ept=N.
On Mon, Sep 26, 2022, David Matlack wrote: > On Mon, Sep 26, 2022 at 3:29 PM Sean Christopherson <seanjc@google.com> wrote: > > If someone wants to improve the memstress framework, a hook can be added for probing > > nested requirements. In other words, IMO this is a shortcoming of the memstress code, > > not a valid reason to shove the requirement down in common code. > > Sorry I forgot to ask this in my previous reply... Why do you prefer > to decouple test requirements from the test setup code? There is a > maintenance burden to such an approach, so I want to understand the > benefit. e.g. I forsee myself having to send patches in the future to > add TEST_REQUIRE(kvm_cpu_has_ept()), because someone added a new VMX > test and forgot to test with ept=N. I don't necessarily prefer decoupling, what I really dislike is having the TEST_REQUIRE() buried deep down, because it's not clear from the reader whether or not TDP/EPT is truly required, and if it is a hard requirement, it's not easily visible to the reader. The print_skip() output helps, but that obviously requires actually trying to run the test. E.g. I wouldn't object as much if perf_test_setup_nested() looked like this: void perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]) { struct vmx_pages *vmx, *vmx0 = NULL; struct kvm_regs regs; vm_vaddr_t vmx_gva; int vcpu_id; TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX)); TEST_REQUEST(perf_test_setup_ept(vm)); for (vcpu_id = 0; vcpu_id < nr_vcpus; vcpu_id++) { vmx = vcpu_alloc_vmx(vm, &vmx_gva); ... } } I'd still object to some extent, because that obfuscates that the requirement is that KVM supports nested EPT, e.g. one might wonder why EPT setup can fail, and because there's really no need for prepare_eptp() to exist. If/when "struct vmx_page" is a thing[*], then prepare_eptp() goes away and becomes vm_alloc_vmx_page(<pointer to a struct vmx_page>); and so there's not even a real place to shove the TEST_REQUIRE(). And I 100% agree there's a maintenance burden, but it's fairly small and it's only paid once per test, whereas making it even the tiniest bit difficult to understand a test's requirements incurs some amount of cost every time someone reads the code. E.g. the memstress code ends up looking something like this: void perf_test_setup_ept(struct vmx_page *eptp, struct kvm_vm *vm) { uint64_t start, end; vm_alloc_vmx_page(eptp) /* * Identity map the first 4G and the test region with 1G pages so that * KVM can shadow the EPT12 with the maximum huge page size supported * by the backing source. */ nested_identity_map_1g(eptp, vm, 0, 0x100000000ULL); start = align_down(perf_test_args.gpa, PG_SIZE_1G); end = align_up(perf_test_args.gpa + perf_test_args.size, PG_SIZE_1G); nested_identity_map_1g(eptp, vm, start, end - start); } void perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_vcpu *vcpus[]) { struct vmx_pages *vmx; struct vmx_page eptp; struct kvm_regs regs; vm_vaddr_t vmx_gva; int vcpu_id; TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX)); TEST_REQUEST(kvm_cpu_has_ept()); perf_test_setup_ept(eptp, vm); for (vcpu_id = 0; vcpu_id < nr_vcpus; vcpu_id++) { vmx = vcpu_alloc_vmx(vm, &vmx_gva); memcpy(vmx->eptp, &eptp, sizeof(eptp)); /* * Override the vCPU to run perf_test_l1_guest_code() which will * bounce it into L2 before calling perf_test_guest_code(). */ vcpu_regs_get(vcpus[vcpu_id], ®s); regs.rip = (unsigned long) perf_test_l1_guest_code; vcpu_regs_set(vcpus[vcpu_id], ®s); vcpu_args_set(vcpus[vcpu_id], 2, vmx_gva, vcpu_id); } } and at that point, IMO adding a helper to assert/require EPT is contrived and not necessarily a net positive. [*] https://lore.kernel.org/all/YwznLAqRb2i4lHiH@google.com
On 9/26/22 19:14, David Matlack wrote: > Skip selftests that require EPT support in the VM when it is not > available. For example, if running on a machine where kvm_intel.ept=N > since KVM does not offer EPT support to guests if EPT is not supported > on the host. > > This commit causes vmx_dirty_log_test to be skipped instead of failing > on hosts where kvm_intel.ept=N. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > .../selftests/kvm/include/x86_64/vmx.h | 1 + > tools/testing/selftests/kvm/lib/x86_64/vmx.c | 20 +++++++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/x86_64/vmx.h b/tools/testing/selftests/kvm/include/x86_64/vmx.h > index 99fa1410964c..790c6d1ecb34 100644 > --- a/tools/testing/selftests/kvm/include/x86_64/vmx.h > +++ b/tools/testing/selftests/kvm/include/x86_64/vmx.h > @@ -617,6 +617,7 @@ void nested_map_memslot(struct vmx_pages *vmx, struct kvm_vm *vm, > uint32_t memslot); > void nested_identity_map_1g(struct vmx_pages *vmx, struct kvm_vm *vm, > uint64_t addr, uint64_t size); > +bool kvm_vm_has_ept(struct kvm_vm *vm); > void prepare_eptp(struct vmx_pages *vmx, struct kvm_vm *vm, > uint32_t eptp_memslot); > void prepare_virtualize_apic_accesses(struct vmx_pages *vmx, struct kvm_vm *vm); > diff --git a/tools/testing/selftests/kvm/lib/x86_64/vmx.c b/tools/testing/selftests/kvm/lib/x86_64/vmx.c > index 80a568c439b8..d21049c38fc5 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/vmx.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/vmx.c > @@ -5,6 +5,8 @@ > * Copyright (C) 2018, Google LLC. > */ > > +#include <asm/msr-index.h> > + > #include "test_util.h" > #include "kvm_util.h" > #include "processor.h" > @@ -542,9 +544,27 @@ void nested_identity_map_1g(struct vmx_pages *vmx, struct kvm_vm *vm, > __nested_map(vmx, vm, addr, addr, size, PG_LEVEL_1G); > } > > +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"); > + > + 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)); > + > vmx->eptp = (void *)vm_vaddr_alloc_page(vm); > vmx->eptp_hva = addr_gva2hva(vm, (uintptr_t)vmx->eptp); > vmx->eptp_gpa = addr_gva2gpa(vm, (uintptr_t)vmx->eptp); > > base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2 > prerequisite-patch-id: 2e3661ba8856c29b769499bac525b6943d9284b8 > prerequisite-patch-id: 93031262de7b1f7fab1ad31ea5d6ef812c139179 Queued for 6.0, thanks. Paolo
On Tue, Sep 27, 2022 at 4:58 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 9/26/22 19:14, David Matlack wrote: > > Skip selftests that require EPT support in the VM when it is not > > available. For example, if running on a machine where kvm_intel.ept=N > > since KVM does not offer EPT support to guests if EPT is not supported > > on the host. > > > > This commit causes vmx_dirty_log_test to be skipped instead of failing > > on hosts where kvm_intel.ept=N. > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > Queued for 6.0, thanks. I sent a v2 based on the feedback from Sean. Please grab that one instead. Thanks.
diff --git a/tools/testing/selftests/kvm/include/x86_64/vmx.h b/tools/testing/selftests/kvm/include/x86_64/vmx.h index 99fa1410964c..790c6d1ecb34 100644 --- a/tools/testing/selftests/kvm/include/x86_64/vmx.h +++ b/tools/testing/selftests/kvm/include/x86_64/vmx.h @@ -617,6 +617,7 @@ void nested_map_memslot(struct vmx_pages *vmx, struct kvm_vm *vm, uint32_t memslot); void nested_identity_map_1g(struct vmx_pages *vmx, struct kvm_vm *vm, uint64_t addr, uint64_t size); +bool kvm_vm_has_ept(struct kvm_vm *vm); void prepare_eptp(struct vmx_pages *vmx, struct kvm_vm *vm, uint32_t eptp_memslot); void prepare_virtualize_apic_accesses(struct vmx_pages *vmx, struct kvm_vm *vm); diff --git a/tools/testing/selftests/kvm/lib/x86_64/vmx.c b/tools/testing/selftests/kvm/lib/x86_64/vmx.c index 80a568c439b8..d21049c38fc5 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/vmx.c +++ b/tools/testing/selftests/kvm/lib/x86_64/vmx.c @@ -5,6 +5,8 @@ * Copyright (C) 2018, Google LLC. */ +#include <asm/msr-index.h> + #include "test_util.h" #include "kvm_util.h" #include "processor.h" @@ -542,9 +544,27 @@ void nested_identity_map_1g(struct vmx_pages *vmx, struct kvm_vm *vm, __nested_map(vmx, vm, addr, addr, size, PG_LEVEL_1G); } +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"); + + 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)); + vmx->eptp = (void *)vm_vaddr_alloc_page(vm); vmx->eptp_hva = addr_gva2hva(vm, (uintptr_t)vmx->eptp); vmx->eptp_gpa = addr_gva2gpa(vm, (uintptr_t)vmx->eptp);
Skip selftests that require EPT support in the VM when it is not available. For example, if running on a machine where kvm_intel.ept=N since KVM does not offer EPT support to guests if EPT is not supported on the host. This commit causes vmx_dirty_log_test to be skipped instead of failing on hosts where kvm_intel.ept=N. Signed-off-by: David Matlack <dmatlack@google.com> --- .../selftests/kvm/include/x86_64/vmx.h | 1 + tools/testing/selftests/kvm/lib/x86_64/vmx.c | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2 prerequisite-patch-id: 2e3661ba8856c29b769499bac525b6943d9284b8 prerequisite-patch-id: 93031262de7b1f7fab1ad31ea5d6ef812c139179