[PATCH 0/4]: kvm-unit-test: nVMX: Test deferring of error from VM-entry MSR-load area

[PATCH 0/4]: kvm-unit-test: nVMX: Test deferring of error from VM-entry MSR-load area

[Krish Sadhukhan]

Patch# 1: Replaces hard-coded value with instruction length read from VMCS
	  field.
Patch# 2: Adds an extra check to __enter_guest() so that it can distinguish
	  between VM-entry failure due to invalid guest state and that due to
	  invalid VM-entry MSR-load area.
Patch# 3: Verifies that when VM-entry fails due to invalid VM-entry MSR-load
	  area in vmcs12, the error is deferred and caught by hardware when
	  it is done processing higher priority checks such as guest state etc.
	  This patch also verifies that when VM-entry fails due to invalid
	  VM-entry MSR-load area in vmcs12, MSRs that were loaded from  that
	  MSR-load area are rolled back to their original values.
Patch# 4: Replaces hard-coded value with corresponding #define.


[PATCH 1/4] kvm-unit-test: VMX: Replace hard-coded exit instruction length
[PATCH 2/4] kvm-unit-test: nVMX: __enter_guest() needs to also check for
[PATCH 3/4] kvm-unit-test: nVMX: Test deferring of error from VM-entry MSR-load area
[PATCH 4/4] kvm-unit-test: nVMX: Use #defines for exit reason in

 x86/vmx.c       |   3 +-
 x86/vmx_tests.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 136 insertions(+), 6 deletions(-)

Krish Sadhukhan (4):
      VMX: Replace hard-coded exit instruction length
      VMX: __enter_guest() needs to also check for VMX_FAIL_STATE
      nVMX: Test deferring of error from VM-entry MSR-load area
      nVMX: Use #defines for exit reason in advance_guest_state_test()

[PATCH 1/4] kvm-unit-test: VMX: Replace hard-coded exit instruction length

[Krish Sadhukhan]

  ..with value read from EXI_INST_LEN field.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 4aebc3f..7d73ee3 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -87,6 +87,7 @@ static int vmenter_exit_handler(void)
 {
 	u64 guest_rip;
 	ulong reason;
+	u32 insn_len = vmcs_read(EXI_INST_LEN);
 
 	guest_rip = vmcs_read(GUEST_RIP);
 	reason = vmcs_read(EXI_REASON) & 0xff;
@@ -97,7 +98,7 @@ static int vmenter_exit_handler(void)
 			return VMX_TEST_VMEXIT;
 		}
 		regs.rax = 0xFFFF;
-		vmcs_write(GUEST_RIP, guest_rip + 3);
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
 		return VMX_TEST_RESUME;
 	default:
 		report("test vmresume", 0);
@@ -340,7 +341,9 @@ static int test_ctrl_pat_exit_handler(void)
 	u64 guest_rip;
 	ulong reason;
 	u64 guest_pat;
+	u32 insn_len;
 
+	insn_len = vmcs_read(EXI_INST_LEN);
 	guest_rip = vmcs_read(GUEST_RIP);
 	reason = vmcs_read(EXI_REASON) & 0xff;
 	switch (reason) {
@@ -357,7 +360,7 @@ static int test_ctrl_pat_exit_handler(void)
 		else
 			report("Exit load PAT", rdmsr(MSR_IA32_CR_PAT) == ia32_pat);
 		vmcs_write(GUEST_PAT, ia32_pat);
-		vmcs_write(GUEST_RIP, guest_rip + 3);
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
 		return VMX_TEST_RESUME;
 	default:
 		printf("ERROR : Undefined exit reason, reason = %ld.\n", reason);
@@ -407,7 +410,9 @@ static int test_ctrl_efer_exit_handler(void)
 	u64 guest_rip;
 	ulong reason;
 	u64 guest_efer;
+	u32 insn_len;
 
+	insn_len = vmcs_read(EXI_INST_LEN);
 	guest_rip = vmcs_read(GUEST_RIP);
 	reason = vmcs_read(EXI_REASON) & 0xff;
 	switch (reason) {
@@ -426,7 +431,7 @@ static int test_ctrl_efer_exit_handler(void)
 			report("Exit load EFER", rdmsr(MSR_EFER) == (ia32_efer ^ EFER_NX));
 		}
 		vmcs_write(GUEST_PAT, ia32_efer);
-		vmcs_write(GUEST_RIP, guest_rip + 3);
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
 		return VMX_TEST_RESUME;
 	default:
 		printf("ERROR : Undefined exit reason, reason = %ld.\n", reason);
@@ -2076,6 +2081,11 @@ static void disable_rdtscp_main(void)
 static int disable_rdtscp_exit_handler(void)
 {
 	unsigned int reason = vmcs_read(EXI_REASON) & 0xff;
+	u64 guest_rip;
+	u32 insn_len;
+
+	guest_rip = vmcs_read(GUEST_RIP);
+	insn_len = vmcs_read(EXI_INST_LEN);
 
 	switch (reason) {
 	case VMX_VMCALL:
@@ -2086,7 +2096,7 @@ static int disable_rdtscp_exit_handler(void)
 			/* fallthrough */
 		case 1:
 			vmx_inc_test_stage();
-			vmcs_write(GUEST_RIP, vmcs_read(GUEST_RIP) + 3);
+			vmcs_write(GUEST_RIP, guest_rip + insn_len);
 			return VMX_TEST_RESUME;
 		case 2:
 			report("RDPID triggers #UD", false);
-- 
2.20.1

Re: [PATCH 1/4] kvm-unit-test: VMX: Replace hard-coded exit instruction length

[Jim Mattson]

On Mon, Oct 14, 2019 at 5:52 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>   ..with value read from EXI_INST_LEN field.
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 2/4] kvm-unit-test: nVMX: __enter_guest() needs to also check for VMX_FAIL_STATE

[Krish Sadhukhan]

  ..as both VMX_ENTRY_FAILURE and VMX_FAIL_STATE together comprise the
    exit eeason when VM-entry fails due invalid guest state.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 647ab49..4ce0fb5 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -1848,7 +1848,8 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
 	vmx_enter_guest(failure);
 	if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
 	    (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
-	    vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
+	    (vmcs_read(EXI_REASON) & (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))
+	    == (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))) {
 
 		print_vmentry_failure_info(failure);
 		abort();
-- 
2.20.1

Re: [PATCH 2/4] kvm-unit-test: nVMX: __enter_guest() needs to also check for VMX_FAIL_STATE

[Jim Mattson]

On Mon, Oct 14, 2019 at 5:52 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>   ..as both VMX_ENTRY_FAILURE and VMX_FAIL_STATE together comprise the
>     exit eeason when VM-entry fails due invalid guest state.

Nit: s/reason/reason/

> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  x86/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 647ab49..4ce0fb5 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -1848,7 +1848,8 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
>         vmx_enter_guest(failure);
>         if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
>             (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
> -           vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
> +           (vmcs_read(EXI_REASON) & (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))
> +           == (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))) {
This shouldn't be a bitwise comparison. It should just be a value comparison:

vmcs_read(EXI_REASON) == VMX_ENTRY_FAILURE | VMX_FAIL_STATE

>
>                 print_vmentry_failure_info(failure);
>                 abort();
> --
> 2.20.1
>

Re: [PATCH 2/4] kvm-unit-test: nVMX: __enter_guest() needs to also check for VMX_FAIL_STATE

[Sean Christopherson]

On Wed, Oct 16, 2019 at 10:55:55AM -0700, Jim Mattson wrote:
> On Mon, Oct 14, 2019 at 5:52 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
> >
> >   ..as both VMX_ENTRY_FAILURE and VMX_FAIL_STATE together comprise the
> >     exit eeason when VM-entry fails due invalid guest state.
> 
> Nit: s/reason/reason/
> 
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> > ---
> >  x86/vmx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/x86/vmx.c b/x86/vmx.c
> > index 647ab49..4ce0fb5 100644
> > --- a/x86/vmx.c
> > +++ b/x86/vmx.c
> > @@ -1848,7 +1848,8 @@ static void __enter_guest(u8 abort_flag, struct vmentry_failure *failure)
> >         vmx_enter_guest(failure);
> >         if ((abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL && failure->early) ||
> >             (abort_flag & ABORT_ON_INVALID_GUEST_STATE &&
> > -           vmcs_read(EXI_REASON) & VMX_ENTRY_FAILURE)) {
> > +           (vmcs_read(EXI_REASON) & (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))
> > +           == (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))) {
> This shouldn't be a bitwise comparison. It should just be a value comparison:
> 
> vmcs_read(EXI_REASON) == VMX_ENTRY_FAILURE | VMX_FAIL_STATE

Hmm, but then we miss failed VM-Enter if something goes truly haywire,
e.g. KVM signals a failed VM-Enter due to #MC.

I'd do something like this:

	u32 exit_reason;

	...

	vmx_enter_guest(failure);

	if (failure->early) {
		if (abort_flag & ABORT_ON_EARLY_VMENTRY_FAIL)
			goto do_abort;
		return;
	}
	exit_reason = vmcs_read(EXI_REASON);
	if (exit_reason & VMX_ENTRY_FAILURE) {
		if ((abort_flag & ABORT_ON_INVALID_GUEST_STATE) ||
		    exit_reason != (VMX_ENTRY_FAILURE | VMX_FAIL_STATE))
			goto do_abort;
		return;
	}

	launched = 1;
	check_for_guest_termination();
	return;

do_abort:
	print_vmentry_failure_info(failure);
	abort();

[PATCH 3/4] kvm-unit-test: nVMX: Test deferring of error from VM-entry MSR-load area

[Krish Sadhukhan]

According to section "VM Entries" in Intel SDM vol 3C, VM-entry checks are
performed in a certain order. Checks on MSRs that are loaded on VM-entry
from VM-entry MSR-load area, should be done after verifying VMCS controls,
host-state area and guest-state area. As KVM relies on CPU hardware to
perform some of these checks, it defers VM-exit due to invalid VM-entry
MSR-load area to until after CPU hardware completes the earlier checks
and is ready to do VMLAUNCH/VMRESUME.

This patch tests the following VM-entry scenarios:
	i) Valid guest state and valid VM-entry MSR-load area
	ii) Invalid guest state and valid VM-entry MSR-load area
	iii) Invalid guest state and invalid VM-entry MSR-load area
	iv) Valid guest state and invalid VM-entry MSR-load area
	v) Guest MSRs are not loaded from VM-entry MSR-load area
	   when VM-entry fails due to invalid VM-entry MSR-load area.

This patch sets up the invalid guest state by writing a GUEST_PAT value which
is illegal according to section "Checks on Guest Control Registers, Debug
Registers, and MSRs" in Intel SDM vol 3C.

This patch sets up the invalid VM-entry MSR-load area in vmcs12, by creating
an MSR entry which is illegal according to section "Loading MSRs" in Intel
SDM vol 3C.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7d73ee3..d68f0c0 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7282,6 +7282,124 @@ static void test_load_guest_pat(void)
 	test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
 }
 
+static void report_vm_entry_msr_load_test(const char *test, u32 xreason,
+					  u64 xqual, u64 field,
+					  const char * field_name)
+{
+	u32 reason = vmcs_read(EXI_REASON);
+	u64 qual = vmcs_read(EXI_QUALIFICATION);
+	u64 guest_rip;
+	u32 insn_len;
+
+	report("%s, %s %lx", (reason == xreason && qual == xqual), test,
+	    field_name, field);
+
+	guest_rip = vmcs_read(GUEST_RIP);
+	insn_len = vmcs_read(EXI_INST_LEN);
+	if (reason != (VMX_ENTRY_FAILURE | VMX_FAIL_MSR))
+		vmcs_write(GUEST_RIP, guest_rip + insn_len);
+}
+
+static u64 guest_msr_efer;
+
+static void invalid_vm_entry_msr_load_main(void)
+{
+	while (1) {
+		guest_msr_efer = rdmsr(MSR_EFER);
+		if (vmx_get_test_stage() != 2)
+			vmcall();
+		else
+			break;
+	}
+	asm volatile("fnop");
+}
+
+static void vmx_invalid_vm_entry_msr_load_test(void)
+{
+	void *msr_bitmap;
+	u32 ctrl_cpu0;
+	u64 guest_efer = vmcs_read(GUEST_EFER);
+	u64 efer_unmodified;
+	entry_msr_load = alloc_page();
+
+	/*
+	 * Set MSR bitmap so that we don't VM-exit on RDMSR
+	 */
+	msr_bitmap = alloc_page();
+	ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
+	ctrl_cpu0 |= CPU_MSR_BITMAP;
+	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);
+	vmcs_write(MSR_BITMAP, (u64)msr_bitmap);
+
+	vmcs_set_bits(ENT_CONTROLS, ENT_LOAD_PAT);
+	vmx_set_test_stage(1);
+	test_set_guest(invalid_vm_entry_msr_load_main);
+
+	/*
+	 * Valid guest state and valid MSR-load area
+	 */
+	enter_guest();
+	efer_unmodified = guest_msr_efer;
+	report_guest_state_test("Valid guest state, valid MSR-load area",
+				 VMX_VMCALL, 0, "");
+
+	/*
+	 * Invalid guest state and valid MSR-load area.
+	 * We render the guest-state invalid by setting an illegal value
+	 * (according to SDM) in GUEST_PAT.
+	 */
+	vmcs_write(GUEST_PAT, 2);
+	enter_guest_with_invalid_guest_state();
+	report_guest_state_test("Invalid guest state, valid MSR-load area",
+				 VMX_FAIL_STATE | VMX_ENTRY_FAILURE, GUEST_PAT,
+				 "GUEST_PAT");
+
+	/*
+	 * Valid guest state and invalid MSR-load area. In VM-entry MSR-load
+	 * area, we are creating two entries: entry# 1 is valid while entry# 2
+	 * is invalid (as per Intel SDM). This will cause KVM to return an
+	 * Exit Qualification of 2 to us on VM-entry failure.
+	 */
+	vmcs_write(GUEST_PAT, 0);
+	vmcs_write(ENT_MSR_LD_CNT, 2);
+	entry_msr_load[0].index = MSR_EFER;
+	entry_msr_load[0].value = guest_efer ^ EFER_NX;
+	entry_msr_load[1].index = MSR_FS_BASE;
+	vmcs_write(ENTER_MSR_LD_ADDR, (u64) entry_msr_load);
+	enter_guest();
+	report_vm_entry_msr_load_test("Valid guest state, invalid MSR-load area",
+					VMX_ENTRY_FAILURE | VMX_FAIL_MSR, 2,
+					ENTER_MSR_LD_ADDR, "ENTER_MSR_LD_ADDR");
+
+	/*
+	 * Invalid guest state and invalid MSR-load area
+	 */
+	vmcs_write(GUEST_PAT, 2);
+	enter_guest_with_invalid_guest_state();
+	report_guest_state_test("Invalid guest state, invalid MSR-load area",
+				 VMX_FAIL_STATE | VMX_ENTRY_FAILURE, GUEST_PAT,
+				"GUEST_PAT");
+
+	/*
+	 * Let the guest finish execution
+	 */
+	vmcs_clear_bits(ENT_CONTROLS, ENT_LOAD_PAT);
+	vmcs_write(GUEST_PAT, 0);
+	entry_msr_load[0].index = MSR_IA32_TSC;
+	entry_msr_load[0].value = 0x11111;
+	vmcs_write(ENT_MSR_LD_CNT, 1);
+	vmx_set_test_stage(2);
+	enter_guest();
+
+	/*
+	 * Verify that MSR_EFER in guest was not loaded from our invalid
+	 * VM-entry MSR-load area
+	 */
+	report("Test invalid VM-entry MSR-load, actual: 0x%lx, expected: 0x%lx",
+		guest_msr_efer == efer_unmodified, guest_msr_efer,
+		efer_unmodified);
+}
+
 /*
  * Check that the virtual CPU checks the VMX Guest State Area as
  * documented in the Intel SDM.
@@ -9023,6 +9141,7 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_controls_test),
 	TEST(vmx_host_state_area_test),
 	TEST(vmx_guest_state_area_test),
+	TEST(vmx_invalid_vm_entry_msr_load_test),
 	TEST(vmentry_movss_shadow_test),
 	/* APICv tests */
 	TEST(vmx_eoi_bitmap_ioapic_scan_test),
-- 
2.20.1

[PATCH 4/4] kvm-unit-test: nVMX: Use #defines for exit reason in advance_guest_state_test()

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index d68f0c0..759e24a 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -5043,7 +5043,7 @@ static void guest_state_test_main(void)
 static void advance_guest_state_test(void)
 {
 	u32 reason = vmcs_read(EXI_REASON);
-	if (! (reason & 0x80000000)) {
+	if (! (reason & VMX_ENTRY_FAILURE)) {
 		u64 guest_rip = vmcs_read(GUEST_RIP);
 		u32 insn_len = vmcs_read(EXI_INST_LEN);
 		vmcs_write(GUEST_RIP, guest_rip + insn_len);
-- 
2.20.1

Re: [PATCH 4/4] kvm-unit-test: nVMX: Use #defines for exit reason in advance_guest_state_test()

[Jim Mattson]

On Mon, Oct 14, 2019 at 5:52 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  x86/vmx_tests.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index d68f0c0..759e24a 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -5043,7 +5043,7 @@ static void guest_state_test_main(void)
>  static void advance_guest_state_test(void)
>  {
>         u32 reason = vmcs_read(EXI_REASON);
> -       if (! (reason & 0x80000000)) {
> +       if (! (reason & VMX_ENTRY_FAILURE)) {
Nit: Drop the superfluous space while you're here?
>                 u64 guest_rip = vmcs_read(GUEST_RIP);
>                 u32 insn_len = vmcs_read(EXI_INST_LEN);
>                 vmcs_write(GUEST_RIP, guest_rip + insn_len);
> --
> 2.20.1
>
Reviewed-by: Jim Mattson <jmattson@google.com>