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

[PATCH v2 0/8] 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 wheneveer I am testing a new series for
upstream.

v2:
 - Split emulator_error_test into 2 separate tests to ensure continued
   test coverage of KVM emulation in response to EPT violations when
   "allow_smaller_maxphyaddr && guest.MAXPHYADDR < host.MAXPHADDR".  [Sean]
 - Test that flds generates #PF(RSVD) when TDP is disabled [Sean]

v1: https://lore.kernel.org/kvm/20220929204708.2548375-1-dmatlack@google.com/

David Matlack (8):
  KVM: selftests: Rename emulator_error_test to
    smaller_maxphyaddr_emulation_test
  KVM: selftests: Explicitly require instructions bytes
  KVM: selftests: Delete dead ucall code
  KVM: selftests: Move flds instruction emulation failure handling to
    header
  KVM: x86/mmu: Use BIT{,_ULL}() for PFERR masks
  KVM: selftests: Copy KVM PFERR masks into selftests
  KVM: selftests: Expect #PF(RSVD) when TDP is disabled
  KVM: selftest: Add a test for KVM_CAP_EXIT_ON_EMULATION_FAILURE

 arch/x86/include/asm/kvm_host.h               |  20 +-
 tools/testing/selftests/kvm/.gitignore        |   3 +-
 tools/testing/selftests/kvm/Makefile          |   3 +-
 .../selftests/kvm/include/x86_64/processor.h  |  31 +++
 .../kvm/x86_64/emulator_error_test.c          | 193 ------------------
 .../x86_64/exit_on_emulation_failure_test.c   |  42 ++++
 .../selftests/kvm/x86_64/flds_emulation.h     |  67 ++++++
 .../smaller_maxphyaddr_emulation_test.c       | 118 +++++++++++
 8 files changed, 272 insertions(+), 205 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/x86_64/emulator_error_test.c
 create mode 100644 tools/testing/selftests/kvm/x86_64/exit_on_emulation_failure_test.c
 create mode 100644 tools/testing/selftests/kvm/x86_64/flds_emulation.h
 create mode 100644 tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c


base-commit: 458e98746fa852d744d34b5a8d0b1673959efc2f
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 1/8] KVM: selftests: Rename emulator_error_test to smaller_maxphyaddr_emulation_test

[David Matlack]

Rename emulator_error_test to smaller_maxphyaddr_emulation_test and
update the comment at the top of the file to document that this is
explicitly a test to validate that KVM emulates instructions in response
to an EPT violation when emulating a smaller MAXPHYADDR.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/.gitignore                         | 2 +-
 tools/testing/selftests/kvm/Makefile                           | 2 +-
 ...ulator_error_test.c => smaller_maxphyaddr_emulation_test.c} | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)
 rename tools/testing/selftests/kvm/x86_64/{emulator_error_test.c => smaller_maxphyaddr_emulation_test.c} (97%)

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index d625a3f83780..c484ff164000 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -16,7 +16,6 @@
 /x86_64/cr4_cpuid_sync_test
 /x86_64/debug_regs
 /x86_64/evmcs_test
-/x86_64/emulator_error_test
 /x86_64/fix_hypercall_test
 /x86_64/get_msr_index_features
 /x86_64/kvm_clock_test
@@ -34,6 +33,7 @@
 /x86_64/set_boot_cpu_id
 /x86_64/set_sregs_test
 /x86_64/sev_migrate_tests
+/x86_64/smaller_maxphyaddr_emulation_test
 /x86_64/smm_test
 /x86_64/state_test
 /x86_64/svm_vmcall_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 6448cb9f710f..90c19e1753f7 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -81,7 +81,6 @@ TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
-TEST_GEN_PROGS_x86_64 += x86_64/emulator_error_test
 TEST_GEN_PROGS_x86_64 += x86_64/fix_hypercall_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
@@ -95,6 +94,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test
 TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test
 TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
 TEST_GEN_PROGS_x86_64 += x86_64/set_sregs_test
+TEST_GEN_PROGS_x86_64 += x86_64/smaller_maxphyaddr_emulation_test
 TEST_GEN_PROGS_x86_64 += x86_64/smm_test
 TEST_GEN_PROGS_x86_64 += x86_64/state_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
similarity index 97%
rename from tools/testing/selftests/kvm/x86_64/emulator_error_test.c
rename to tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index 236e11755ba6..6ed996988a5a 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -2,7 +2,8 @@
 /*
  * Copyright (C) 2020, Google LLC.
  *
- * Tests for KVM_CAP_EXIT_ON_EMULATION_FAILURE capability.
+ * Test that KVM emulates instructions in response to EPT violations when
+ * allow_smaller_maxphyaddr is enabled and guest.MAXPHYADDR < host.MAXPHYADDR.
  */
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 1/8] KVM: selftests: Rename emulator_error_test to smaller_maxphyaddr_emulation_test

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> Rename emulator_error_test to smaller_maxphyaddr_emulation_test and
> update the comment at the top of the file to document that this is
> explicitly a test to validate that KVM emulates instructions in response
> to an EPT violation when emulating a smaller MAXPHYADDR.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v2 2/8] KVM: selftests: Explicitly require instructions bytes

[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>
---
 .../smaller_maxphyaddr_emulation_test.c       | 47 +++++++++----------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index 6ed996988a5a..c5353ad0e06d 100644
--- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -65,30 +65,29 @@ 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);
-		}
-	}
+	flags = run->emulation_failure.flags;
+	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
+		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
+		    "run->emulation_failure is missing instruction bytes");
+
+	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 do_guest_assert(struct ucall *uc)
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 2/8] KVM: selftests: Explicitly require instructions bytes

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> 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>
> ---
>  .../smaller_maxphyaddr_emulation_test.c       | 47 +++++++++----------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> index 6ed996988a5a..c5353ad0e06d 100644
> --- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> @@ -65,30 +65,29 @@ 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);
> -		}
> -	}
> +	flags = run->emulation_failure.flags;
> +	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
> +		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
> +		    "run->emulation_failure is missing instruction bytes");
> +
> +	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);

Unnecessary newline, insn_size fits comfortably on the line above.

> +
> +	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;

This whole sequence is silly.  Assert that size > 0 but < 15, then assert that
it's >= 2, then skip exactly two bytes and effective "assert" that it's '2.

And while I appreciate the ModR/M decoding, IMO it does more harm than good.  If
someone can follow the ModR/M decoding, they can figure out a hardcode opcode.
E.g. IMO this is much simpler and will be easier to debug.

#define FLDS_MEM_EAX ".byte 0xd9, 0x00"

static void guest_code(void)
{
	__asm__ __volatile__(FLDS_MEM_EAX
			     :: "a"(MEM_REGION_GVA));

	GUEST_DONE();
}

static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
{
	struct kvm_run *run = vcpu->run;
	struct kvm_regs regs;
	uint8_t *insn_bytes;
	uint64_t flags;

	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
		    "Unexpected exit reason: %u (%s)",
		    run->exit_reason,
		    exit_reason_str(run->exit_reason));

	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
		    "Unexpected suberror: %u",
		    run->emulation_failure.suberror);

	flags = run->emulation_failure.flags;
	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
		    "run->emulation_failure is missing instruction bytes");

	TEST_ASSERT(run->emulation_failure.insn_size == 2,
		    "Expected a 2-byte opcode for 'flds', got %d bytes",
		    run->emulation_failure.insn_size);

	insn_bytes = run->emulation_failure.insn_bytes;
	TEST_ASSERT(insn_bytes[0] == 0xd9 && insn_bytes[1] == 0,
		    "Expected 'flds [eax]', opcode '0xd9 0x00', got opcode 0x%x 0x%x\n",
		    insn_bytes[0], insn_bytes[1]);

	vcpu_regs_get(vcpu, &regs);
	regs.rip += 2;
	vcpu_regs_set(vcpu, &regs);
}

[PATCH v2 3/8] KVM: selftests: Delete dead ucall code

[David Matlack]

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

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../smaller_maxphyaddr_emulation_test.c       | 54 +------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index c5353ad0e06d..d6e71549ca08 100644
--- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -90,64 +90,15 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
 	vcpu_regs_set(vcpu, &regs);
 }
 
-static void do_guest_assert(struct ucall *uc)
-{
-	REPORT_GUEST_ASSERT(*uc);
-}
-
-static void check_for_guest_assert(struct kvm_vcpu *vcpu)
+static void assert_ucall_done(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,
-		    exit_reason_str(run->exit_reason));
-
 	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_DONE,
 		    "Unexpected ucall command: %lu, expected UCALL_DONE (%d)",
 		    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;
@@ -184,8 +135,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");
+	assert_ucall_done(vcpu);
 
 	kvm_vm_free(vm);
 
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 3/8] KVM: selftests: Delete dead ucall code

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> Delete a bunch of code related to ucall handling from
> smaller_maxphyaddr_emulation_test. The only thing
> smaller_maxphyaddr_emulation_test needs to check is that the vCPU exits
> with UCALL_DONE after the second vcpu_run().
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  .../smaller_maxphyaddr_emulation_test.c       | 54 +------------------
>  1 file changed, 2 insertions(+), 52 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> index c5353ad0e06d..d6e71549ca08 100644
> --- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> @@ -90,64 +90,15 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
>  	vcpu_regs_set(vcpu, &regs);
>  }
>  
> -static void do_guest_assert(struct ucall *uc)
> -{
> -	REPORT_GUEST_ASSERT(*uc);
> -}
> -
> -static void check_for_guest_assert(struct kvm_vcpu *vcpu)
> +static void assert_ucall_done(struct kvm_vcpu *vcpu)

I vote to delete this helper too, it's used exactly once and doesn't exactly make
the code more readable.

>  	TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_DONE,
>  		    "Unexpected ucall command: %lu, expected UCALL_DONE (%d)",
>  		    uc.cmd, UCALL_DONE);

I believe the warning is due to gcc resolving the VA args inputs to test_assert()
before the call to get_ucall().  One thought:

  uint64_t cmd = get_ucall(vcpu, NULL);

  TEST_ASSERT(cmd == UCALL_DONE, ...)

In file included from x86_64/smaller_maxphyaddr_emulation_test.c:11:
include/test_util.h: In function ‘assert_ucall_done’:
include/test_util.h:54:9: warning: ‘uc.cmd’ is used uninitialized [-Wuninitialized]
   54 |         test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
      |         ^~~~~~~~~~~
x86_64/smaller_maxphyaddr_emulation_test.c:69:22: note: ‘uc’ declared here
   69 |         struct ucall uc;
      |                      ^~

Re: [PATCH v2 3/8] KVM: selftests: Delete dead ucall code

[David Matlack]

On Thu, Oct 27, 2022 at 4:44 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Oct 18, 2022, David Matlack wrote:
> > Delete a bunch of code related to ucall handling from
> > smaller_maxphyaddr_emulation_test. The only thing
> > smaller_maxphyaddr_emulation_test needs to check is that the vCPU exits
> > with UCALL_DONE after the second vcpu_run().
> >
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > ---
> >  .../smaller_maxphyaddr_emulation_test.c       | 54 +------------------
> >  1 file changed, 2 insertions(+), 52 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> > index c5353ad0e06d..d6e71549ca08 100644
> > --- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> > @@ -90,64 +90,15 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
> >       vcpu_regs_set(vcpu, &regs);
> >  }
> >
> > -static void do_guest_assert(struct ucall *uc)
> > -{
> > -     REPORT_GUEST_ASSERT(*uc);
> > -}
> > -
> > -static void check_for_guest_assert(struct kvm_vcpu *vcpu)
> > +static void assert_ucall_done(struct kvm_vcpu *vcpu)
>
> I vote to delete this helper too, it's used exactly once and doesn't exactly make
> the code more readable.
>
> >       TEST_ASSERT(get_ucall(vcpu, &uc) == UCALL_DONE,
> >                   "Unexpected ucall command: %lu, expected UCALL_DONE (%d)",
> >                   uc.cmd, UCALL_DONE);
>
> I believe the warning is due to gcc resolving the VA args inputs to test_assert()
> before the call to get_ucall().  One thought:
>
>   uint64_t cmd = get_ucall(vcpu, NULL);
>
>   TEST_ASSERT(cmd == UCALL_DONE, ...)
>

I think you're right. And only gcc complains, which is how I missed
it. We can kill 2 birds (gcc warning + delete helper) with:

  ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);

[PATCH v2 4/8] KVM: selftests: Move flds instruction emulation failure handling to header

[David Matlack]

Move the flds instruction emulation failure handling code to a header
so it can be re-used in an upcoming test.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |  9 +++
 .../selftests/kvm/x86_64/flds_emulation.h     | 67 +++++++++++++++++
 .../smaller_maxphyaddr_emulation_test.c       | 72 ++-----------------
 3 files changed, 81 insertions(+), 67 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/flds_emulation.h

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e8ca0d8a6a7e..6762bc315464 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -882,4 +882,13 @@ void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 #define XSTATE_XTILE_DATA_MASK		(1ULL << XSTATE_XTILE_DATA_BIT)
 #define XFEATURE_XTILE_MASK		(XSTATE_XTILE_CFG_MASK | \
 					XSTATE_XTILE_DATA_MASK)
+
+/*
+ * Accessors to get R/M, REG, and Mod bits described in the SDM vol 2,
+ * figure 2-2 "Table Interpretation of ModR/M Byte (C8H)".
+ */
+#define GET_RM(insn_byte) ((insn_byte) & 0x7)
+#define GET_REG(insn_byte) (((insn_byte) & 0x38) >> 3)
+#define GET_MOD(insn_byte) (((insn_byte) & 0xc) >> 6)
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/x86_64/flds_emulation.h b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
new file mode 100644
index 000000000000..55bb9272f4d0
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef SELFTEST_KVM_FLDS_EMULATION_H
+#define SELFTEST_KVM_FLDS_EMULATION_H
+
+#include "processor.h"
+#include "kvm_util.h"
+
+/*
+ * flds is an instruction that the KVM instruction emulator is known not to
+ * support. This can be used in guest code along with a mechanism to force
+ * KVM to emulate the instruction (e.g. by providing an MMIO address) to
+ * exercise emulation failures.
+ */
+static inline void flds(uint64_t address)
+{
+	__asm__ __volatile__("flds (%0)" :: "r"(address));
+}
+
+static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	uint8_t *insn_bytes;
+	uint8_t insn_size;
+	uint64_t flags;
+
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
+		    "Unexpected exit reason: %u (%s)",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
+
+	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
+		    "Unexpected suberror: %u",
+		    run->emulation_failure.suberror);
+
+	flags = run->emulation_failure.flags;
+	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
+		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
+		    "run->emulation_failure is missing instruction bytes");
+
+	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(insn_size >= 2 &&
+		    insn_bytes[0] == 0xd9 &&
+		    GET_REG(insn_bytes[1]) == 0x0 &&
+		    GET_MOD(insn_bytes[1]) == 0x0 &&
+		    /* Ensure there is no SIB byte. */
+		    GET_RM(insn_bytes[1]) != 0x4 &&
+		    /* Ensure there is no displacement byte. */
+		    GET_RM(insn_bytes[1]) != 0x5,
+		    "Unexpected instruction.  Expected 'flds' (0xd9 /0)");
+}
+
+static inline void skip_flds_instruction(struct kvm_vcpu *vcpu)
+{
+	struct kvm_regs regs;
+
+	vcpu_regs_get(vcpu, &regs);
+	regs.rip += 2;
+	vcpu_regs_set(vcpu, &regs);
+}
+
+#endif /* !SELFTEST_KVM_FLDS_EMULATION_H */
diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index d6e71549ca08..91a85a00b692 100644
--- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -8,6 +8,8 @@
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
 
+#include "flds_emulation.h"
+
 #include "test_util.h"
 #include "kvm_util.h"
 #include "vmx.h"
@@ -21,75 +23,10 @@
 
 static void guest_code(void)
 {
-	__asm__ __volatile__("flds (%[addr])"
-			     :: [addr]"r"(MEM_REGION_GVA));
-
+	flds(MEM_REGION_GVA);
 	GUEST_DONE();
 }
 
-/*
- * Accessors to get R/M, REG, and Mod bits described in the SDM vol 2,
- * figure 2-2 "Table Interpretation of ModR/M Byte (C8H)".
- */
-#define GET_RM(insn_byte) (insn_byte & 0x7)
-#define GET_REG(insn_byte) ((insn_byte & 0x38) >> 3)
-#define GET_MOD(insn_byte) ((insn_byte & 0xc) >> 6)
-
-/* Ensure we are dealing with a simple 2-byte flds instruction. */
-static bool is_flds(uint8_t *insn_bytes, uint8_t insn_size)
-{
-	return insn_size >= 2 &&
-	       insn_bytes[0] == 0xd9 &&
-	       GET_REG(insn_bytes[1]) == 0x0 &&
-	       GET_MOD(insn_bytes[1]) == 0x0 &&
-	       /* Ensure there is no SIB byte. */
-	       GET_RM(insn_bytes[1]) != 0x4 &&
-	       /* Ensure there is no displacement byte. */
-	       GET_RM(insn_bytes[1]) != 0x5;
-}
-
-static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
-{
-	struct kvm_run *run = vcpu->run;
-	struct kvm_regs regs;
-	uint8_t *insn_bytes;
-	uint8_t insn_size;
-	uint64_t flags;
-
-	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
-		    "Unexpected exit reason: %u (%s)",
-		    run->exit_reason,
-		    exit_reason_str(run->exit_reason));
-
-	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
-		    "Unexpected suberror: %u",
-		    run->emulation_failure.suberror);
-
-	flags = run->emulation_failure.flags;
-	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
-		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
-		    "run->emulation_failure is missing instruction bytes");
-
-	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 assert_ucall_done(struct kvm_vcpu *vcpu)
 {
 	struct ucall uc;
@@ -133,7 +70,8 @@ int main(int argc, char *argv[])
 	vm_set_page_table_entry(vm, vcpu, MEM_REGION_GVA, pte | (1ull << 36));
 
 	vcpu_run(vcpu);
-	process_exit_on_emulation_error(vcpu);
+	assert_exit_for_flds_emulation_failure(vcpu);
+	skip_flds_instruction(vcpu);
 	vcpu_run(vcpu);
 	assert_ucall_done(vcpu);
 
-- 
2.38.0.413.g74048e4d9e-goog

[PATCH v2 5/8] KVM: x86/mmu: Use BIT{,_ULL}() for PFERR masks

[David Matlack]

Use the preferred BIT() and BIT_ULL() to construct the PFERR masks
rather than open-coding the bit shifting.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/include/asm/kvm_host.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index aa381ab69a19..1d7605b079bf 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -253,16 +253,16 @@ enum x86_intercept_stage;
 #define PFERR_GUEST_PAGE_BIT 33
 #define PFERR_IMPLICIT_ACCESS_BIT 48
 
-#define PFERR_PRESENT_MASK (1U << PFERR_PRESENT_BIT)
-#define PFERR_WRITE_MASK (1U << PFERR_WRITE_BIT)
-#define PFERR_USER_MASK (1U << PFERR_USER_BIT)
-#define PFERR_RSVD_MASK (1U << PFERR_RSVD_BIT)
-#define PFERR_FETCH_MASK (1U << PFERR_FETCH_BIT)
-#define PFERR_PK_MASK (1U << PFERR_PK_BIT)
-#define PFERR_SGX_MASK (1U << PFERR_SGX_BIT)
-#define PFERR_GUEST_FINAL_MASK (1ULL << PFERR_GUEST_FINAL_BIT)
-#define PFERR_GUEST_PAGE_MASK (1ULL << PFERR_GUEST_PAGE_BIT)
-#define PFERR_IMPLICIT_ACCESS (1ULL << PFERR_IMPLICIT_ACCESS_BIT)
+#define PFERR_PRESENT_MASK	BIT(PFERR_PRESENT_BIT)
+#define PFERR_WRITE_MASK	BIT(PFERR_WRITE_BIT)
+#define PFERR_USER_MASK		BIT(PFERR_USER_BIT)
+#define PFERR_RSVD_MASK		BIT(PFERR_RSVD_BIT)
+#define PFERR_FETCH_MASK	BIT(PFERR_FETCH_BIT)
+#define PFERR_PK_MASK		BIT(PFERR_PK_BIT)
+#define PFERR_SGX_MASK		BIT(PFERR_SGX_BIT)
+#define PFERR_GUEST_FINAL_MASK	BIT_ULL(PFERR_GUEST_FINAL_BIT)
+#define PFERR_GUEST_PAGE_MASK	BIT_ULL(PFERR_GUEST_PAGE_BIT)
+#define PFERR_IMPLICIT_ACCESS	BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
 
 #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK |	\
 				 PFERR_WRITE_MASK |		\
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 5/8] KVM: x86/mmu: Use BIT{,_ULL}() for PFERR masks

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> Use the preferred BIT() and BIT_ULL() to construct the PFERR masks
> rather than open-coding the bit shifting.
> 
> No functional change intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v2 6/8] KVM: selftests: Copy KVM PFERR masks into selftests

[David Matlack]

Copy KVM's macros for pafe fault error masks into processor.h so they
can be used in selftests.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 6762bc315464..c3ae21237af3 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -891,4 +891,26 @@ void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 #define GET_REG(insn_byte) (((insn_byte) & 0x38) >> 3)
 #define GET_MOD(insn_byte) (((insn_byte) & 0xc) >> 6)
 
+#define PFERR_PRESENT_BIT 0
+#define PFERR_WRITE_BIT 1
+#define PFERR_USER_BIT 2
+#define PFERR_RSVD_BIT 3
+#define PFERR_FETCH_BIT 4
+#define PFERR_PK_BIT 5
+#define PFERR_SGX_BIT 15
+#define PFERR_GUEST_FINAL_BIT 32
+#define PFERR_GUEST_PAGE_BIT 33
+#define PFERR_IMPLICIT_ACCESS_BIT 48
+
+#define PFERR_PRESENT_MASK	BIT(PFERR_PRESENT_BIT)
+#define PFERR_WRITE_MASK	BIT(PFERR_WRITE_BIT)
+#define PFERR_USER_MASK		BIT(PFERR_USER_BIT)
+#define PFERR_RSVD_MASK		BIT(PFERR_RSVD_BIT)
+#define PFERR_FETCH_MASK	BIT(PFERR_FETCH_BIT)
+#define PFERR_PK_MASK		BIT(PFERR_PK_BIT)
+#define PFERR_SGX_MASK		BIT(PFERR_SGX_BIT)
+#define PFERR_GUEST_FINAL_MASK	BIT_ULL(PFERR_GUEST_FINAL_BIT)
+#define PFERR_GUEST_PAGE_MASK	BIT_ULL(PFERR_GUEST_PAGE_BIT)
+#define PFERR_IMPLICIT_ACCESS	BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 6/8] KVM: selftests: Copy KVM PFERR masks into selftests

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> Copy KVM's macros for pafe fault error masks into processor.h so they

s/pafe/page

> can be used in selftests.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>

[PATCH v2 7/8] KVM: selftests: Expect #PF(RSVD) when TDP is disabled

[David Matlack]

Change smaller_maxphyaddr_emulation_test to expect a #PF(RSVD), rather
than an emulation failure, when TDP is disabled. KVM only needs to
emulate instructions to emulate a smaller guest.MAXPHYADDR when TDP is
enabled.

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

diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index 91a85a00b692..afa9e0b3dd8a 100644
--- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -21,6 +21,12 @@
 #define MEM_REGION_SLOT	10
 #define MEM_REGION_SIZE PAGE_SIZE
 
+static void guest_page_fault_handler(struct ex_regs *regs)
+{
+	GUEST_ASSERT(regs->error_code & PFERR_RSVD_MASK);
+	GUEST_SYNC(PF_VECTOR);
+}
+
 static void guest_code(void)
 {
 	flds(MEM_REGION_GVA);
@@ -36,6 +42,23 @@ static void assert_ucall_done(struct kvm_vcpu *vcpu)
 		    uc.cmd, UCALL_DONE);
 }
 
+static void assert_reserved_page_fault(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	case UCALL_SYNC:
+		TEST_ASSERT(uc.args[1] == PF_VECTOR,
+			    "Unexpected UCALL_SYNC: %lu", uc.args[1]);
+		break;
+	default:
+		TEST_FAIL("Unrecognized ucall: %lu\n", uc.cmd);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	struct kvm_vcpu *vcpu;
@@ -50,6 +73,9 @@ int main(int argc, char *argv[])
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vcpu);
+	vm_install_exception_handler(vm, PF_VECTOR, guest_page_fault_handler);
 
 	vcpu_set_cpuid_maxphyaddr(vcpu, MAXPHYADDR);
 
@@ -70,10 +96,21 @@ int main(int argc, char *argv[])
 	vm_set_page_table_entry(vm, vcpu, MEM_REGION_GVA, pte | (1ull << 36));
 
 	vcpu_run(vcpu);
-	assert_exit_for_flds_emulation_failure(vcpu);
-	skip_flds_instruction(vcpu);
-	vcpu_run(vcpu);
-	assert_ucall_done(vcpu);
+
+	/*
+	 * When TDP is enabled, KVM must emulate the flds instruction, which
+	 * results in an emulation failure out to userspace. Otherwise, no
+	 * instruction emulation is required so check that the instruction
+	 * generates #PF(RSVD).
+	 */
+	if (kvm_is_tdp_enabled()) {
+		assert_exit_for_flds_emulation_failure(vcpu);
+		skip_flds_instruction(vcpu);
+		vcpu_run(vcpu);
+		assert_ucall_done(vcpu);
+	} else {
+		assert_reserved_page_fault(vcpu);
+	}
 
 	kvm_vm_free(vm);
 
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 7/8] KVM: selftests: Expect #PF(RSVD) when TDP is disabled

[Sean Christopherson]

On Tue, Oct 18, 2022, David Matlack wrote:
> @@ -50,6 +73,9 @@ int main(int argc, char *argv[])
>  	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
>  
>  	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	vm_init_descriptor_tables(vm);
> +	vcpu_init_descriptor_tables(vcpu);
> +	vm_install_exception_handler(vm, PF_VECTOR, guest_page_fault_handler);

Instead of installing an exception handler,

	u8 vector = kvm_asm_safe(KVM_ASM_SAFE(FLDS_MEM_EAX),
				 "a"(MEM_REGION_GVA));

then the guest/test can provide more precise information if a #PF doesn't occur.

Re: [PATCH v2 7/8] KVM: selftests: Expect #PF(RSVD) when TDP is disabled

[David Matlack]

On Thu, Oct 27, 2022 at 5:31 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Oct 18, 2022, David Matlack wrote:
> > @@ -50,6 +73,9 @@ int main(int argc, char *argv[])
> >       TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
> >
> >       vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > +     vm_init_descriptor_tables(vm);
> > +     vcpu_init_descriptor_tables(vcpu);
> > +     vm_install_exception_handler(vm, PF_VECTOR, guest_page_fault_handler);
>
> Instead of installing an exception handler,
>
>         u8 vector = kvm_asm_safe(KVM_ASM_SAFE(FLDS_MEM_EAX),
>                                  "a"(MEM_REGION_GVA));
>
> then the guest/test can provide more precise information if a #PF doesn't occur.

I gave this a shot and realized this would prevent checking that it is
a reserved #PF, since kvm_asm_safe() only returns the vector.

It's probably more important to have more precise testing rather than
more precise failure reporting. But what did you have in mind
specifically? Maybe there's another way.

Re: [PATCH v2 7/8] KVM: selftests: Expect #PF(RSVD) when TDP is disabled

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

On Fri, Oct 28, 2022, David Matlack wrote:
> On Thu, Oct 27, 2022 at 5:31 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Oct 18, 2022, David Matlack wrote:
> > > @@ -50,6 +73,9 @@ int main(int argc, char *argv[])
> > >       TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
> > >
> > >       vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > > +     vm_init_descriptor_tables(vm);
> > > +     vcpu_init_descriptor_tables(vcpu);
> > > +     vm_install_exception_handler(vm, PF_VECTOR, guest_page_fault_handler);
> >
> > Instead of installing an exception handler,
> >
> >         u8 vector = kvm_asm_safe(KVM_ASM_SAFE(FLDS_MEM_EAX),
> >                                  "a"(MEM_REGION_GVA));
> >
> > then the guest/test can provide more precise information if a #PF doesn't occur.
> 
> I gave this a shot and realized this would prevent checking that it is
> a reserved #PF, since kvm_asm_safe() only returns the vector.
> 
> It's probably more important to have more precise testing rather than
> more precise failure reporting. But what did you have in mind
> specifically? Maybe there's another way.

I didn't have anything in mind, I just overlooked the error code.  That said, this
is a good excuse to expand KVM_ASM_SAFE() to provide the error code.  There's
already a clobbered scratch register (two, actually) that can be used to propagate
the error code from the exception handler back to the asm blob, so it's easy to
squeeze into the existing framework.

Patches attached.

[-- Attachment #2: 0001-KVM-selftests-Avoid-JMP-in-non-faulting-path-of-KVM_.patch --]
[-- Type: text/x-diff, Size: 3766 bytes --]

From 4379b8a511b64e21a93a30b33a4c770eecd22672 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Mon, 31 Oct 2022 08:44:15 -0700
Subject: [PATCH 1/2] KVM: selftests: Avoid JMP in non-faulting path of
 KVM_ASM_SAFE()

Clear R9 in the non-faulting path of KVM_ASM_SAFE() and fall through to
to a common load of "vector" to effectively load "vector" with '0' to
reduce the code footprint of the asm blob, to reduce the runtime overhead
of the non-faulting path (when "vector" is stored in a register), and so
that additional output constraints that are valid if and only if a fault
occur are loaded even in the non-faulting case.

A future patch will add a 64-bit output for the error code, and if its
output is not explicitly loaded with _something_, the user of the asm
blob can end up technically consuming uninitialized data.  Using a
common path to load the output constraints will allow using an existing
scratch register, e.g. r10, to hold the error code in the faulting path,
while also guaranteeing the error code is initialized with deterministic
data in the non-faulting patch (r10 is loaded with the RIP of
to-be-executed instruction).

Consuming the error code when a fault doesn't occur would obviously be a
test bug, but there's no guarantee the compiler will detect uninitialized
consumption.  And conversely, it's theoretically possible that the
compiler might throw a false positive on uninitialized data, e.g. if the
compiler can't determine that the non-faulting path won't touch the error
code.

Alternatively, the error code could be explicitly loaded in the
non-faulting path, but loading a 64-bit memory|register output operand
with an explicitl value requires a sign-extended "MOV imm32, r/m64",
which isn't exactly straightforward and has a largish code footprint.
And loading the error code with what is effectively garbage (from a
scratch register) avoids having to choose an arbitrary value for the
non-faulting case.

Opportunistically remove a rogue asterisk in the block comment.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e8ca0d8a6a7e..fd9778d1de3d 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -764,7 +764,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
  * for recursive faults when accessing memory in the handler.  The downside to
  * using registers is that it restricts what registers can be used by the actual
  * instruction.  But, selftests are 64-bit only, making register* pressure a
- * minor concern.  Use r9-r11 as they are volatile, i.e. don't need* to be saved
+ * minor concern.  Use r9-r11 as they are volatile, i.e. don't need to be saved
  * by the callee, and except for r11 are not implicit parameters to any
  * instructions.  Ideally, fixup would use r8-r10 and thus avoid implicit
  * parameters entirely, but Hyper-V's hypercall ABI uses r8 and testing Hyper-V
@@ -786,11 +786,9 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 	"lea 1f(%%rip), %%r10\n\t"				\
 	"lea 2f(%%rip), %%r11\n\t"				\
 	"1: " insn "\n\t"					\
-	"movb $0, %[vector]\n\t"				\
-	"jmp 3f\n\t"						\
+	"xor %%r9, %%r9\n\t"					\
 	"2:\n\t"						\
-	"mov  %%r9b, %[vector]\n\t"				\
-	"3:\n\t"
+	"mov  %%r9b, %[vector]\n\t"
 
 #define KVM_ASM_SAFE_OUTPUTS(v)	[vector] "=qm"(v)
 #define KVM_ASM_SAFE_CLOBBERS	"r9", "r10", "r11"

base-commit: e18d6152ff0f41b7f01f9817372022df04e0d354
-- 
2.38.1.273.g43a17bfeac-goog


[-- Attachment #3: 0002-KVM-selftests-Provide-error-code-as-a-KVM_ASM_SAFE-o.patch --]
[-- Type: text/x-diff, Size: 5030 bytes --]

From a5d5a821524ece6796e05fb1b8c69c9ddb98d312 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Mon, 31 Oct 2022 09:21:28 -0700
Subject: [PATCH 2/2] KVM: selftests: Provide error code as a KVM_ASM_SAFE()
 output

Provide the error code on a fault in KVM_ASM_SAFE(), e.g. to allow tests
to assert that #PF generates the correct error code without needing to
manually install a #PF handler.  Use r10 as the scratch register for the
error code, as it's already clobbered by the asm blob (loaded with the
RIP of the to-be-executed instruction).  Deliberately load the output
"error_code" even in the non-faulting path so that error_code is always
initialized with deterministic data (the aforementioned RIP), i.e to
ensure a selftest won't end up with uninitialized consumption regardless
of how KVM_ASM_SAFE() is used.

Don't clear r10 in the non-faulting case and instead load error code with
the RIP (see above).  The error code is valid if and only if an exception
occurs, and '0' isn't necessarily a better "invalid" value, e.g. '0'
could result in false passes for a buggy test.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 39 +++++++++++++------
 .../selftests/kvm/lib/x86_64/processor.c      |  1 +
 .../selftests/kvm/x86_64/hyperv_features.c    |  3 +-
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index fd9778d1de3d..4c6e0e60609b 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -780,6 +780,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
  *
  * REGISTER OUTPUTS:
  * r9  = exception vector (non-zero)
+ * r10 = error code
  */
 #define KVM_ASM_SAFE(insn)					\
 	"mov $" __stringify(KVM_EXCEPTION_MAGIC) ", %%r9\n\t"	\
@@ -788,29 +789,43 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector,
 	"1: " insn "\n\t"					\
 	"xor %%r9, %%r9\n\t"					\
 	"2:\n\t"						\
-	"mov  %%r9b, %[vector]\n\t"
+	"mov  %%r9b, %[vector]\n\t"				\
+	"mov  %%r10, %[error_code]\n\t"
 
-#define KVM_ASM_SAFE_OUTPUTS(v)	[vector] "=qm"(v)
+#define KVM_ASM_SAFE_OUTPUTS(v, ec)	[vector] "=qm"(v), [error_code] "=rm"(ec)
 #define KVM_ASM_SAFE_CLOBBERS	"r9", "r10", "r11"
 
-#define kvm_asm_safe(insn, inputs...)			\
-({							\
-	uint8_t vector;					\
-							\
-	asm volatile(KVM_ASM_SAFE(insn)			\
-		     : KVM_ASM_SAFE_OUTPUTS(vector)	\
-		     : inputs				\
-		     : KVM_ASM_SAFE_CLOBBERS);		\
-	vector;						\
+#define kvm_asm_safe(insn, inputs...)					\
+({									\
+	uint64_t ign_error_code;					\
+	uint8_t vector;							\
+									\
+	asm volatile(KVM_ASM_SAFE(insn)					\
+		     : KVM_ASM_SAFE_OUTPUTS(vector, ign_error_code)	\
+		     : inputs						\
+		     : KVM_ASM_SAFE_CLOBBERS);				\
+	vector;								\
+})
+
+#define kvm_asm_safe_ec(insn, error_code, inputs...)			\
+({									\
+	uint8_t vector;							\
+									\
+	asm volatile(KVM_ASM_SAFE(insn)					\
+		     : KVM_ASM_SAFE_OUTPUTS(vector, error_code)		\
+		     : inputs						\
+		     : KVM_ASM_SAFE_CLOBBERS);				\
+	vector;								\
 })
 
 static inline uint8_t rdmsr_safe(uint32_t msr, uint64_t *val)
 {
+	uint64_t error_code;
 	uint8_t vector;
 	uint32_t a, d;
 
 	asm volatile(KVM_ASM_SAFE("rdmsr")
-		     : "=a"(a), "=d"(d), KVM_ASM_SAFE_OUTPUTS(vector)
+		     : "=a"(a), "=d"(d), KVM_ASM_SAFE_OUTPUTS(vector, error_code)
 		     : "c"(msr)
 		     : KVM_ASM_SAFE_CLOBBERS);
 
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 39c4409ef56a..fc6c724e0d24 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1116,6 +1116,7 @@ static bool kvm_fixup_exception(struct ex_regs *regs)
 
 	regs->rip = regs->r11;
 	regs->r9 = regs->vector;
+	regs->r10 = regs->error_code;
 	return true;
 }
 
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
index 05b32e550a80..2b6d455acf8a 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
@@ -18,6 +18,7 @@
 static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
 				vm_vaddr_t output_address, uint64_t *hv_status)
 {
+	uint64_t error_code;
 	uint8_t vector;
 
 	/* Note both the hypercall and the "asm safe" clobber r9-r11. */
@@ -25,7 +26,7 @@ static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
 		     KVM_ASM_SAFE("vmcall")
 		     : "=a" (*hv_status),
 		       "+c" (control), "+d" (input_address),
-		       KVM_ASM_SAFE_OUTPUTS(vector)
+		       KVM_ASM_SAFE_OUTPUTS(vector, error_code)
 		     : [output_address] "r"(output_address),
 		       "a" (-EFAULT)
 		     : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v2 8/8] KVM: selftest: Add a test for KVM_CAP_EXIT_ON_EMULATION_FAILURE

[David Matlack]

Add a selftest to exercise the KVM_CAP_EXIT_ON_EMULATION_FAILURE
capability.

This capability is also exercised through
smaller_maxphyaddr_emulation_test, but that test requires
allow_smaller_maxphyaddr=Y, which is off by default on Intel when ept=Y
and unconditionally disabled on AMD when npt=Y. This new test ensures we
exercise KVM_CAP_EXIT_ON_EMULATION_FAILURE independet of
allow_smaller_maxphyaddr.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |  1 +
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../x86_64/exit_on_emulation_failure_test.c   | 42 +++++++++++++++++++
 3 files changed, 44 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/exit_on_emulation_failure_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index c484ff164000..22807badd510 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -16,6 +16,7 @@
 /x86_64/cr4_cpuid_sync_test
 /x86_64/debug_regs
 /x86_64/evmcs_test
+/x86_64/exit_on_emulation_failure_test
 /x86_64/fix_hypercall_test
 /x86_64/get_msr_index_features
 /x86_64/kvm_clock_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 90c19e1753f7..55b6f4efa57c 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -81,6 +81,7 @@ TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
+TEST_GEN_PROGS_x86_64 += x86_64/exit_on_emulation_failure_test
 TEST_GEN_PROGS_x86_64 += x86_64/fix_hypercall_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
diff --git a/tools/testing/selftests/kvm/x86_64/exit_on_emulation_failure_test.c b/tools/testing/selftests/kvm/x86_64/exit_on_emulation_failure_test.c
new file mode 100644
index 000000000000..8e98ad3259de
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/exit_on_emulation_failure_test.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022, Google LLC.
+ *
+ * Test for KVM_CAP_EXIT_ON_EMULATION_FAILURE.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+
+#include "flds_emulation.h"
+
+#include "test_util.h"
+
+#define MMIO_GPA	0x700000000
+#define MMIO_GVA	MMIO_GPA
+
+static void guest_code(void)
+{
+	/* Execute flds with an MMIO address to force KVM to emulate it. */
+	flds(MMIO_GVA);
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_EXIT_ON_EMULATION_FAILURE));
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	vm_enable_cap(vm, KVM_CAP_EXIT_ON_EMULATION_FAILURE, 1);
+	virt_map(vm, MMIO_GVA, MMIO_GPA, 1);
+
+	vcpu_run(vcpu);
+	assert_exit_for_flds_emulation_failure(vcpu);
+
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH v2 8/8] KVM: selftest: Add a test for KVM_CAP_EXIT_ON_EMULATION_FAILURE

[Sean Christopherson]

s/selftest/selftests

On Tue, Oct 18, 2022, David Matlack wrote:
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index c484ff164000..22807badd510 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -16,6 +16,7 @@
>  /x86_64/cr4_cpuid_sync_test
>  /x86_64/debug_regs
>  /x86_64/evmcs_test
> +/x86_64/exit_on_emulation_failure_test
>  /x86_64/fix_hypercall_test
>  /x86_64/get_msr_index_features
>  /x86_64/kvm_clock_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 90c19e1753f7..55b6f4efa57c 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -81,6 +81,7 @@ TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
>  TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
>  TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
>  TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
> +TEST_GEN_PROGS_x86_64 += x86_64/exit_on_emulation_failure_test

We really should figure out an automation solution, both for .gitignore (easier)
and for selecting which binaries to compile.  The makefile especially is getting
unwieldy.