[PATCH 0/4] KVM: selftests: Fix and clean up emulator_error_test

[PATCH 0/4] KVM: selftests: Fix and clean up emulator_error_test

[David Matlack]

Miscellaneous fixes and cleanups to emulator_error_test. The reason I
started looking at this test is because it fails when TDP is disabled,
which pollutes my test results when I am testing a new series for
upstream.

David Matlack (4):
  KVM: selftests: Use MMIO to trigger emulation in emulator_error_test
  KVM: selftests: Delete dead ucall code from emulator_error_test
  KVM: selftests: Skip emulator_error_test if
    KVM_CAP_EXIT_ON_EMULATION_FAILURE not available
  KVM: selftests: Explicitly require instructions bytes in
    emulator_error_test

 .../kvm/x86_64/emulator_error_test.c          | 130 +++++-------------
 1 file changed, 35 insertions(+), 95 deletions(-)


base-commit: 69604fe76e58c9d195e48b41d019b07fc27ce9d7
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 1/4] KVM: selftests: Use MMIO to trigger emulation in emulator_error_test

[David Matlack]

Use MMIO to force KVM to emulate the flds isntruction in
emulator_error_test, rather than relying on KVM_CAP_SMALLER_MAXPHYADDR.

KVM_CAP_SMALLER_MAXPHYADDR is not enabled by default when TDP is
enabled. So developers that run all selftests against KVM in its default
configuration do not get test coverage of
KVM_CAP_EXIT_ON_EMULATION_FAILURE.

When TDP is disabled, KVM_CAP_SMALLER_MAXPHYADDR is enabled by default,
but emulator_error_test actually fails because KVM does not need to
emulate flds.  i.e. The test fails to induce and emulation failure.

Fixes: 39bbcc3a4e39 ("selftests: kvm: Allows userspace to handle emulation errors.")
Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../kvm/x86_64/emulator_error_test.c          | 36 ++++++-------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
index 236e11755ba6..2dff57991d31 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
@@ -11,18 +11,12 @@
 #include "kvm_util.h"
 #include "vmx.h"
 
-#define MAXPHYADDR 36
-
-#define MEM_REGION_GVA	0x0000123456789000
-#define MEM_REGION_GPA	0x0000000700000000
-#define MEM_REGION_SLOT	10
-#define MEM_REGION_SIZE PAGE_SIZE
+#define MMIO_REGION_GPA	0x700000000
+#define MMIO_REGION_GVA	0x700000000
 
 static void guest_code(void)
 {
-	__asm__ __volatile__("flds (%[addr])"
-			     :: [addr]"r"(MEM_REGION_GVA));
-
+	__asm__ __volatile__("flds (%[addr])" :: [addr]"r"(MMIO_REGION_GVA));
 	GUEST_DONE();
 }
 
@@ -152,34 +146,24 @@ int main(int argc, char *argv[])
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
-	uint64_t gpa, pte;
-	uint64_t *hva;
 	int rc;
 
 	/* Tell stdout not to buffer its content */
 	setbuf(stdout, NULL);
 
-	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
-
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 
-	vcpu_set_cpuid_maxphyaddr(vcpu, MAXPHYADDR);
-
 	rc = kvm_check_cap(KVM_CAP_EXIT_ON_EMULATION_FAILURE);
 	TEST_ASSERT(rc, "KVM_CAP_EXIT_ON_EMULATION_FAILURE is unavailable");
 	vm_enable_cap(vm, KVM_CAP_EXIT_ON_EMULATION_FAILURE, 1);
 
-	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
-				    MEM_REGION_GPA, MEM_REGION_SLOT,
-				    MEM_REGION_SIZE / PAGE_SIZE, 0);
-	gpa = vm_phy_pages_alloc(vm, MEM_REGION_SIZE / PAGE_SIZE,
-				 MEM_REGION_GPA, MEM_REGION_SLOT);
-	TEST_ASSERT(gpa == MEM_REGION_GPA, "Failed vm_phy_pages_alloc\n");
-	virt_map(vm, MEM_REGION_GVA, MEM_REGION_GPA, 1);
-	hva = addr_gpa2hva(vm, MEM_REGION_GPA);
-	memset(hva, 0, PAGE_SIZE);
-	pte = vm_get_page_table_entry(vm, vcpu, MEM_REGION_GVA);
-	vm_set_page_table_entry(vm, vcpu, MEM_REGION_GVA, pte | (1ull << 36));
+	/*
+	 * Create a virtual mapping so the guest can access MMIO_REGION_GPA.
+	 * MMIO_REGION_GPA is not mapped by a memslot so KVM will treat the
+	 * access as MMIO and attempt to emulate the flds instruction, which
+	 * will then generate an emulation error.
+	 */
+	virt_map(vm, MMIO_REGION_GVA, MMIO_REGION_GPA, 1);
 
 	vcpu_run(vcpu);
 	process_exit_on_emulation_error(vcpu);
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 2/4] KVM: selftests: Delete dead ucall code from emulator_error_test

[David Matlack]

Delete a bunch of code related to ucall handling from
emulator_error_test. The only thing emulator_error_test needs to check
is that the vCPU exits with UCALL_DONE after the second vcpu_run().

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../kvm/x86_64/emulator_error_test.c          | 46 +------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
index 2dff57991d31..52ff1eb772e9 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
@@ -84,28 +84,11 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
 	}
 }
 
-static void do_guest_assert(struct ucall *uc)
-{
-	REPORT_GUEST_ASSERT(*uc);
-}
-
-static void check_for_guest_assert(struct kvm_vcpu *vcpu)
-{
-	struct ucall uc;
-
-	if (vcpu->run->exit_reason == KVM_EXIT_IO &&
-	    get_ucall(vcpu, &uc) == UCALL_ABORT) {
-		do_guest_assert(&uc);
-	}
-}
-
 static void process_ucall_done(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
 	struct ucall uc;
 
-	check_for_guest_assert(vcpu);
-
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s)",
 		    run->exit_reason,
@@ -116,32 +99,6 @@ static void process_ucall_done(struct kvm_vcpu *vcpu)
 		    uc.cmd, UCALL_DONE);
 }
 
-static uint64_t process_ucall(struct kvm_vcpu *vcpu)
-{
-	struct kvm_run *run = vcpu->run;
-	struct ucall uc;
-
-	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
-		    "Unexpected exit reason: %u (%s)",
-		    run->exit_reason,
-		    exit_reason_str(run->exit_reason));
-
-	switch (get_ucall(vcpu, &uc)) {
-	case UCALL_SYNC:
-		break;
-	case UCALL_ABORT:
-		do_guest_assert(&uc);
-		break;
-	case UCALL_DONE:
-		process_ucall_done(vcpu);
-		break;
-	default:
-		TEST_ASSERT(false, "Unexpected ucall");
-	}
-
-	return uc.cmd;
-}
-
 int main(int argc, char *argv[])
 {
 	struct kvm_vcpu *vcpu;
@@ -168,8 +125,7 @@ int main(int argc, char *argv[])
 	vcpu_run(vcpu);
 	process_exit_on_emulation_error(vcpu);
 	vcpu_run(vcpu);
-
-	TEST_ASSERT(process_ucall(vcpu) == UCALL_DONE, "Expected UCALL_DONE");
+	process_ucall_done(vcpu);
 
 	kvm_vm_free(vm);
 
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 3/4] KVM: selftests: Skip emulator_error_test if KVM_CAP_EXIT_ON_EMULATION_FAILURE not available

[David Matlack]

Skip emulator_error_test if KVM_CAP_EXIT_ON_EMULATION_FAILURE is not
available rather than failing. This makes emulator_error_test skip on
older kernels and also deletes a net 2 lines of code from the test.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/x86_64/emulator_error_test.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
index 52ff1eb772e9..4b06c9eefe7d 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
@@ -103,15 +103,13 @@ int main(int argc, char *argv[])
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
-	int rc;
 
 	/* Tell stdout not to buffer its content */
 	setbuf(stdout, NULL);
 
-	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_EXIT_ON_EMULATION_FAILURE));
 
-	rc = kvm_check_cap(KVM_CAP_EXIT_ON_EMULATION_FAILURE);
-	TEST_ASSERT(rc, "KVM_CAP_EXIT_ON_EMULATION_FAILURE is unavailable");
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	vm_enable_cap(vm, KVM_CAP_EXIT_ON_EMULATION_FAILURE, 1);
 
 	/*
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH 4/4] KVM: selftests: Explicitly require instructions bytes in emulator_error_test

[David Matlack]

Explicitly require instruction bytes to be available in
run->emulation_failure by asserting that they are present. Note that
the test already requires the instruction bytes to be present because
that's the only way the test will advance the RIP past the flds and get
to GUEST_DONE().

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../kvm/x86_64/emulator_error_test.c          | 50 ++++++++++---------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
index 4b06c9eefe7d..37ecd880a7c1 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
@@ -58,30 +58,32 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
 		    "Unexpected suberror: %u",
 		    run->emulation_failure.suberror);
 
-	if (run->emulation_failure.ndata >= 1) {
-		flags = run->emulation_failure.flags;
-		if ((flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES) &&
-		    run->emulation_failure.ndata >= 3) {
-			insn_size = run->emulation_failure.insn_size;
-			insn_bytes = run->emulation_failure.insn_bytes;
-
-			TEST_ASSERT(insn_size <= 15 && insn_size > 0,
-				    "Unexpected instruction size: %u",
-				    insn_size);
-
-			TEST_ASSERT(is_flds(insn_bytes, insn_size),
-				    "Unexpected instruction.  Expected 'flds' (0xd9 /0)");
-
-			/*
-			 * If is_flds() succeeded then the instruction bytes
-			 * contained an flds instruction that is 2-bytes in
-			 * length (ie: no prefix, no SIB, no displacement).
-			 */
-			vcpu_regs_get(vcpu, &regs);
-			regs.rip += 2;
-			vcpu_regs_set(vcpu, &regs);
-		}
-	}
+	TEST_ASSERT(run->emulation_failure.ndata >= 3,
+		    "Unexpected emulation_failure.ndata: %d",
+		    run->emulation_failure.ndata);
+
+	flags = run->emulation_failure.flags;
+	TEST_ASSERT(flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
+		    "Missing instruction bytes in emulation_failure.");
+
+	insn_size = run->emulation_failure.insn_size;
+	insn_bytes = run->emulation_failure.insn_bytes;
+
+	TEST_ASSERT(insn_size <= 15 && insn_size > 0,
+		    "Unexpected instruction size: %u",
+		    insn_size);
+
+	TEST_ASSERT(is_flds(insn_bytes, insn_size),
+		    "Unexpected instruction.  Expected 'flds' (0xd9 /0)");
+
+	/*
+	 * If is_flds() succeeded then the instruction bytes contained an flds
+	 * instruction that is 2-bytes in length (ie: no prefix, no SIB, no
+	 * displacement).
+	 */
+	vcpu_regs_get(vcpu, &regs);
+	regs.rip += 2;
+	vcpu_regs_set(vcpu, &regs);
 }
 
 static void process_ucall_done(struct kvm_vcpu *vcpu)
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH 0/4] KVM: selftests: Fix and clean up emulator_error_test

[Sean Christopherson]

On Thu, Sep 29, 2022, David Matlack wrote:
> Miscellaneous fixes and cleanups to emulator_error_test. The reason I
> started looking at this test is because it fails when TDP is disabled,
> which pollutes my test results when I am testing a new series for
> upstream.

This series defeats the (not well documented) purpose of emulator_error_test.  The
test exists specifically to verify that KVM emulates in response to EPT violations
when "allow_smaller_maxphyaddr && guest.MAXPHYADDR < host.MAXPHADDR".

That said, the test could use some upgrades:

  1. Verify that a well-emulated instruction takes #PF(RSVD)
  2. Verify that FLDS takes #PF(RSVD) when EPT is disabled

Re: [PATCH 0/4] KVM: selftests: Fix and clean up emulator_error_test

[David Matlack]

On Mon, Oct 3, 2022 at 4:31 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 29, 2022, David Matlack wrote:
> > Miscellaneous fixes and cleanups to emulator_error_test. The reason I
> > started looking at this test is because it fails when TDP is disabled,
> > which pollutes my test results when I am testing a new series for
> > upstream.
>
> This series defeats the (not well documented) purpose of emulator_error_test.  The
> test exists specifically to verify that KVM emulates in response to EPT violations
> when "allow_smaller_maxphyaddr && guest.MAXPHYADDR < host.MAXPHADDR".

I thought that might have been the case before I sent this series, but
I could not (and still can't) find any evidence to support it. The
commit message and comment at the top of the test all indicate this is
a test for KVM_CAP_EXIT_ON_EMULATION_FAILURE. What did I miss?

Either way, I think we want both types of tests.  In v2 how about I split out:

 (1) exit_on_emulation_failure_test.c: Test that emulation failures
exit to userspace when the cap is enabled, i.e. what
emulator_error_test does by the end of this series.
 (2) smaller_maxphyaddr_emulation_test.c: Test the interaction of
allow_smaller_maxphyaddr and instruction emulation. i.e. what
emulator_error_does at the start of this series plus your suggestions.

The main benefit of splitting out (1) is to test
KVM_CAP_EXIT_ON_EMULATION_FAILURE independent of
allow_smaller_maxphyaddr, since allow_smaller_maxphyaddr is not
enabled by default.


>
> That said, the test could use some upgrades:
>
>   1. Verify that a well-emulated instruction takes #PF(RSVD)
>   2. Verify that FLDS takes #PF(RSVD) when EPT is disabled

Re: [PATCH 0/4] KVM: selftests: Fix and clean up emulator_error_test

[Sean Christopherson]

On Tue, Oct 04, 2022, David Matlack wrote:
> On Mon, Oct 3, 2022 at 4:31 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Sep 29, 2022, David Matlack wrote:
> > > Miscellaneous fixes and cleanups to emulator_error_test. The reason I
> > > started looking at this test is because it fails when TDP is disabled,
> > > which pollutes my test results when I am testing a new series for
> > > upstream.
> >
> > This series defeats the (not well documented) purpose of emulator_error_test.  The
> > test exists specifically to verify that KVM emulates in response to EPT violations
> > when "allow_smaller_maxphyaddr && guest.MAXPHYADDR < host.MAXPHADDR".
> 
> I thought that might have been the case before I sent this series, but
> I could not (and still can't) find any evidence to support it. The
> commit message and comment at the top of the test all indicate this is
> a test for KVM_CAP_EXIT_ON_EMULATION_FAILURE. What did I miss?

Being in the general vicinity when the test was written?  :-)

> Either way, I think we want both types of tests.  In v2 how about I split out:
> 
>  (1) exit_on_emulation_failure_test.c: Test that emulation failures
> exit to userspace when the cap is enabled, i.e. what
> emulator_error_test does by the end of this series.
>  (2) smaller_maxphyaddr_emulation_test.c: Test the interaction of
> allow_smaller_maxphyaddr and instruction emulation. i.e. what
> emulator_error_does at the start of this series plus your suggestions.
> 
> The main benefit of splitting out (1) is to test
> KVM_CAP_EXIT_ON_EMULATION_FAILURE independent of
> allow_smaller_maxphyaddr, since allow_smaller_maxphyaddr is not
> enabled by default.

Works for me.