[kvm-unit-tests PATCH v2 0/2] x86: nVMX: Fix NMI/INTR-window tests

[kvm-unit-tests PATCH v2 0/2] x86: nVMX: Fix NMI/INTR-window tests

[Nadav Amit]

NMI/INTR-window test in VMX have a couple of bugs. Each of the patches
fixes one. The first version of this patch-set claimed that one of the
bugs might be a silicon issue. However, according to Sean, it is a
just a test and KVM issue, once you read the SDM more carefully.

Cc: Jim Mattson <jmattson@google.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>

Nadav Amit (2):
  x86: nVMX: Use #DB in nmi- and intr-window tests
  x86: nVMX: Set guest as active after NMI/INTR-window tests

 x86/vmx_tests.c | 80 +++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 42 deletions(-)

-- 
2.17.1

[kvm-unit-tests PATCH v2 1/2] x86: nVMX: Use #DB in nmi- and intr-window tests

[Nadav Amit]

According to Intel SDM 26.3.1.5 "Checks on Guest Non-Register State", if
the activity state is HLT, the only events that can be injected are NMI,
MTF and "Those with interruption type hardware exception and vector 1
(debug exception) or vector 18 (machine-check exception)."

Theverify_nmi_window_exit() and verify_intr_window_exit() tests try
to do something that real hardware disallows (i.e., fail the VM-entry)
by injecting #UD in HLT-state.  Inject #DB instead as the injection
should succeed in these tests.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 x86/vmx_tests.c | 72 ++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index d0ce1af..f921286 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7035,21 +7035,21 @@ static void vmx_pending_event_hlt_test(void)
 	vmx_pending_event_test_core(true);
 }
 
-static int vmx_window_test_ud_count;
+static int vmx_window_test_db_count;
 
-static void vmx_window_test_ud_handler(struct ex_regs *regs)
+static void vmx_window_test_db_handler(struct ex_regs *regs)
 {
-	vmx_window_test_ud_count++;
+	vmx_window_test_db_count++;
 }
 
 static void vmx_nmi_window_test_guest(void)
 {
-	handle_exception(UD_VECTOR, vmx_window_test_ud_handler);
+	handle_exception(DB_VECTOR, vmx_window_test_db_handler);
 
 	asm volatile("vmcall\n\t"
 		     "nop\n\t");
 
-	handle_exception(UD_VECTOR, NULL);
+	handle_exception(DB_VECTOR, NULL);
 }
 
 static void verify_nmi_window_exit(u64 rip)
@@ -7068,7 +7068,7 @@ static void verify_nmi_window_exit(u64 rip)
 static void vmx_nmi_window_test(void)
 {
 	u64 nop_addr;
-	void *ud_fault_addr = get_idt_addr(&boot_idt[UD_VECTOR]);
+	void *db_fault_addr = get_idt_addr(&boot_idt[DB_VECTOR]);
 
 	if (!(ctrl_pin_rev.clr & PIN_VIRT_NMI)) {
 		report_skip("CPU does not support the \"Virtual NMIs\" VM-execution control.");
@@ -7080,7 +7080,7 @@ static void vmx_nmi_window_test(void)
 		return;
 	}
 
-	vmx_window_test_ud_count = 0;
+	vmx_window_test_db_count = 0;
 
 	report_prefix_push("NMI-window");
 	test_set_guest(vmx_nmi_window_test_guest);
@@ -7113,27 +7113,27 @@ static void vmx_nmi_window_test(void)
 	/*
 	 * Ask for "NMI-window exiting" (with event injection), and
 	 * expect a VM-exit after the event is injected. (RIP should
-	 * be at the address specified in the IDT entry for #UD.)
+	 * be at the address specified in the IDT entry for #DB.)
 	 */
-	report_prefix_push("active, no blocking, injecting #UD");
+	report_prefix_push("active, no blocking, injecting #DB");
 	vmcs_write(ENT_INTR_INFO,
-		   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION | UD_VECTOR);
+		   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION | DB_VECTOR);
 	enter_guest();
-	verify_nmi_window_exit((u64)ud_fault_addr);
+	verify_nmi_window_exit((u64)db_fault_addr);
 	report_prefix_pop();
 
 	/*
 	 * Ask for "NMI-window exiting" with NMI blocking, and expect
-	 * a VM-exit after the next IRET (i.e. after the #UD handler
+	 * a VM-exit after the next IRET (i.e. after the #DB handler
 	 * returns). So, RIP should be back at one byte past the nop.
 	 */
 	report_prefix_push("active, blocking by NMI");
 	vmcs_write(GUEST_INTR_STATE, GUEST_INTR_STATE_NMI);
 	enter_guest();
 	verify_nmi_window_exit(nop_addr + 1);
-	report("#UD handler executed once (actual %d times)",
-	       vmx_window_test_ud_count == 1,
-	       vmx_window_test_ud_count);
+	report("#DB handler executed once (actual %d times)",
+	       vmx_window_test_db_count == 1,
+	       vmx_window_test_db_count);
 	report_prefix_pop();
 
 	if (!(rdmsr(MSR_IA32_VMX_MISC) & (1 << 6))) {
@@ -7154,15 +7154,15 @@ static void vmx_nmi_window_test(void)
 		 * Ask for "NMI-window exiting" when entering activity
 		 * state HLT (with event injection), and expect a
 		 * VM-exit after the event is injected. (RIP should be
-		 * at the address specified in the IDT entry for #UD.)
+		 * at the address specified in the IDT entry for #DB.)
 		 */
-		report_prefix_push("halted, no blocking, injecting #UD");
+		report_prefix_push("halted, no blocking, injecting #DB");
 		vmcs_write(GUEST_ACTV_STATE, ACTV_HLT);
 		vmcs_write(ENT_INTR_INFO,
 			   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION |
-			   UD_VECTOR);
+			   DB_VECTOR);
 		enter_guest();
-		verify_nmi_window_exit((u64)ud_fault_addr);
+		verify_nmi_window_exit((u64)db_fault_addr);
 		report_prefix_pop();
 	}
 
@@ -7173,7 +7173,7 @@ static void vmx_nmi_window_test(void)
 
 static void vmx_intr_window_test_guest(void)
 {
-	handle_exception(UD_VECTOR, vmx_window_test_ud_handler);
+	handle_exception(DB_VECTOR, vmx_window_test_db_handler);
 
 	/*
 	 * The two consecutive STIs are to ensure that only the first
@@ -7185,7 +7185,7 @@ static void vmx_intr_window_test_guest(void)
 		     "sti\n\t"
 		     "sti\n\t");
 
-	handle_exception(UD_VECTOR, NULL);
+	handle_exception(DB_VECTOR, NULL);
 }
 
 static void verify_intr_window_exit(u64 rip)
@@ -7205,8 +7205,8 @@ static void vmx_intr_window_test(void)
 {
 	u64 vmcall_addr;
 	u64 nop_addr;
-	unsigned int orig_ud_gate_type;
-	void *ud_fault_addr = get_idt_addr(&boot_idt[UD_VECTOR]);
+	unsigned int orig_db_gate_type;
+	void *db_fault_addr = get_idt_addr(&boot_idt[DB_VECTOR]);
 
 	if (!(ctrl_cpu_rev[0].clr & CPU_INTR_WINDOW)) {
 		report_skip("CPU does not support the \"interrupt-window exiting\" VM-execution control.");
@@ -7214,12 +7214,12 @@ static void vmx_intr_window_test(void)
 	}
 
 	/*
-	 * Change the IDT entry for #UD from interrupt gate to trap gate,
+	 * Change the IDT entry for #DB from interrupt gate to trap gate,
 	 * so that it won't clear RFLAGS.IF. We don't want interrupts to
-	 * be disabled after vectoring a #UD.
+	 * be disabled after vectoring a #DB.
 	 */
-	orig_ud_gate_type = boot_idt[UD_VECTOR].type;
-	boot_idt[UD_VECTOR].type = 15;
+	orig_db_gate_type = boot_idt[DB_VECTOR].type;
+	boot_idt[DB_VECTOR].type = 15;
 
 	report_prefix_push("interrupt-window");
 	test_set_guest(vmx_intr_window_test_guest);
@@ -7244,14 +7244,14 @@ static void vmx_intr_window_test(void)
 	 * Ask for "interrupt-window exiting" (with event injection)
 	 * with RFLAGS.IF set and no blocking; expect a VM-exit after
 	 * the event is injected. That is, RIP should should be at the
-	 * address specified in the IDT entry for #UD.
+	 * address specified in the IDT entry for #DB.
 	 */
-	report_prefix_push("active, no blocking, RFLAGS.IF=1, injecting #UD");
+	report_prefix_push("active, no blocking, RFLAGS.IF=1, injecting #DB");
 	vmcs_write(ENT_INTR_INFO,
-		   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION | UD_VECTOR);
+		   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION | DB_VECTOR);
 	vmcall_addr = vmcs_read(GUEST_RIP);
 	enter_guest();
-	verify_intr_window_exit((u64)ud_fault_addr);
+	verify_intr_window_exit((u64)db_fault_addr);
 	report_prefix_pop();
 
 	/*
@@ -7323,19 +7323,19 @@ static void vmx_intr_window_test(void)
 		 * activity state HLT (with event injection), and
 		 * expect a VM-exit after the event is injected. That
 		 * is, RIP should should be at the address specified
-		 * in the IDT entry for #UD.
+		 * in the IDT entry for #DB.
 		 */
-		report_prefix_push("halted, no blocking, injecting #UD");
+		report_prefix_push("halted, no blocking, injecting #DB");
 		vmcs_write(GUEST_ACTV_STATE, ACTV_HLT);
 		vmcs_write(ENT_INTR_INFO,
 			   INTR_INFO_VALID_MASK | INTR_TYPE_HARD_EXCEPTION |
-			   UD_VECTOR);
+			   DB_VECTOR);
 		enter_guest();
-		verify_intr_window_exit((u64)ud_fault_addr);
+		verify_intr_window_exit((u64)db_fault_addr);
 		report_prefix_pop();
 	}
 
-	boot_idt[UD_VECTOR].type = orig_ud_gate_type;
+	boot_idt[DB_VECTOR].type = orig_db_gate_type;
 	vmcs_clear_bits(CPU_EXEC_CTRL0, CPU_INTR_WINDOW);
 	enter_guest();
 	report_prefix_pop();
-- 
2.17.1

[kvm-unit-tests PATCH v2 2/2] x86: nVMX: Set guest as active after NMI/INTR-window tests

[Nadav Amit]

Running tests which are similar to verify_nmi_window_exit() and
verify_intr_window_exit() on bare-metal suggests that real CPUs do not
wake up. It appears, according to Sean, that the activity state should
not change after NMI/INTR-window.

Remove the offending test and set the activity state to "active" after
each test to prevent the whole test-suite from getting stuck.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 x86/vmx_tests.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index f921286..1092fad 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7060,9 +7060,7 @@ static void verify_nmi_window_exit(u64 rip)
 	       exit_reason == VMX_NMI_WINDOW, exit_reason);
 	report("RIP (%#lx) is %#lx", vmcs_read(GUEST_RIP) == rip,
 	       vmcs_read(GUEST_RIP), rip);
-	report("Activity state (%ld) is 'ACTIVE'",
-	       vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE,
-	       vmcs_read(GUEST_ACTV_STATE));
+	vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE);
 }
 
 static void vmx_nmi_window_test(void)
@@ -7196,9 +7194,7 @@ static void verify_intr_window_exit(u64 rip)
 	       exit_reason == VMX_INTR_WINDOW, exit_reason);
 	report("RIP (%#lx) is %#lx", vmcs_read(GUEST_RIP) == rip,
 	       vmcs_read(GUEST_RIP), rip);
-	report("Activity state (%ld) is 'ACTIVE'",
-	       vmcs_read(GUEST_ACTV_STATE) == ACTV_ACTIVE,
-	       vmcs_read(GUEST_ACTV_STATE));
+	vmcs_write(GUEST_ACTV_STATE, ACTV_ACTIVE);
 }
 
 static void vmx_intr_window_test(void)
-- 
2.17.1

Re: [kvm-unit-tests PATCH v2 0/2] x86: nVMX: Fix NMI/INTR-window tests

[Paolo Bonzini]

On 18/05/19 18:02, Nadav Amit wrote:
> NMI/INTR-window test in VMX have a couple of bugs. Each of the patches
> fixes one. The first version of this patch-set claimed that one of the
> bugs might be a silicon issue. However, according to Sean, it is a
> just a test and KVM issue, once you read the SDM more carefully.
> 
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Nadav Amit (2):
>   x86: nVMX: Use #DB in nmi- and intr-window tests
>   x86: nVMX: Set guest as active after NMI/INTR-window tests
> 
>  x86/vmx_tests.c | 80 +++++++++++++++++++++++--------------------------
>  1 file changed, 38 insertions(+), 42 deletions(-)
> 

Queued, thanks.

Paolo