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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Mon, Sep 26, 2022, David Matlack wrote:
> On Mon, Sep 26, 2022 at 3:29 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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], &regs);
		regs.rip = (unsigned long) perf_test_l1_guest_code;
		vcpu_regs_set(vcpus[vcpu_id], &regs);
		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@xxxxxxxxxx