diff mbox series

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

Message ID 20190508102715.685-2-namit@vmware.com (mailing list archive)
State New, archived
Headers show
Series x86: nVMX: Fix NMI/INTR-window tests | expand

Commit Message

Nadav Amit May 8, 2019, 10:27 a.m. UTC 
From: Nadav Amit <nadav.amit@gmail.com>

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)."

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

Comments

Jim Mattson May 8, 2019, 11:11 p.m. UTC | #1 
From: Nadav Amit <nadav.amit@gmail.com>
Date: Wed, May 8, 2019 at 10:47 AM
To: Paolo Bonzini
Cc: <kvm@vger.kernel.org>, Nadav Amit, Jim Mattson, Sean Christopherson

> From: Nadav Amit <nadav.amit@gmail.com>
>
> 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)."
>
> Two tests, verify_nmi_window_exit() and verify_intr_window_exit(), 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: Jim Mattson <jmattson@google.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Thanks for the fix!

It has always bothered me that there is no easy way to validate a
kvm-unit-test on physical hardware. Do you have a mechanism for doing
so? If so, would you be willing to share?

I don't suppose you have a patch for kvm to fail the VM-entry in this case?
Nadav Amit May 8, 2019, 11:35 p.m. UTC | #2 
> On May 8, 2019, at 4:11 PM, Jim Mattson <jmattson@google.com> wrote:
> 
> From: Nadav Amit <nadav.amit@gmail.com>
> Date: Wed, May 8, 2019 at 10:47 AM
> To: Paolo Bonzini
> Cc: <kvm@vger.kernel.org>, Nadav Amit, Jim Mattson, Sean Christopherson
> 
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> 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)."
>> 
>> Two tests, verify_nmi_window_exit() and verify_intr_window_exit(), 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: Jim Mattson <jmattson@google.com>
>> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> 
> Thanks for the fix!
> 
> It has always bothered me that there is no easy way to validate a
> kvm-unit-test on physical hardware. Do you have a mechanism for doing
> so? If so, would you be willing to share?

I call this mechanism “grub”. ;-)

If you saw my recent kvm-unit-tests patches - they are needed to run
kvm-unit-tests on physical hardware. Once I am done sending the remaining
fixes, I’ll send an RFC that enable test execution on physical hardware
(e.g., by skipping tests that require test devices).

I just hope that this support would convince you, and others, to prefer
(when possible) kvm-unit-tests over the selftest environment.

> I don't suppose you have a patch for kvm to fail the VM-entry in this case?

I am trying to keep my day job.
Paolo Bonzini May 20, 2019, 3:52 p.m. UTC | #3 
On 09/05/19 01:35, Nadav Amit wrote:
> I just hope that this support would convince you, and others, to prefer
> (when possible) kvm-unit-tests over the selftest environment.

kvm-unit-tests are not superseded by selftests; selftests are mostly
meant to test the KVM API.  While they are more easily debuggable than
kvm-unit-tests, the benefit is not big enough to justify the effort of
rewriting everything.

Furthermore, being able to run kvm-unit-tests on bare metal is useful,
even if it's not something that people commonly do, and consistent with
KVM's design of not departing radically for what bare metal does.

Thanks,

Paolo
Nadav Amit May 20, 2019, 4:39 p.m. UTC | #4 
> On May 20, 2019, at 8:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 09/05/19 01:35, Nadav Amit wrote:
>> I just hope that this support would convince you, and others, to prefer
>> (when possible) kvm-unit-tests over the selftest environment.
> 
> kvm-unit-tests are not superseded by selftests; selftests are mostly
> meant to test the KVM API.  While they are more easily debuggable than
> kvm-unit-tests, the benefit is not big enough to justify the effort of
> rewriting everything.
> 
> Furthermore, being able to run kvm-unit-tests on bare metal is useful,
> even if it's not something that people commonly do, and consistent with
> KVM's design of not departing radically for what bare metal does.

I understand.

And just to clarify - my corporate overload deserves the credit for this
work. I just prefer not to shout it too loudly.

I’m sorry for not collecting the patches into a set, but I know that it
would just cause me to resend all of them for individual patch issue.

I still have some more pending patches that I will send after rebasing and
cleanup.
Paolo Bonzini May 20, 2019, 5:21 p.m. UTC | #5 
On 20/05/19 18:39, Nadav Amit wrote:
> I’m sorry for not collecting the patches into a set, but I know that it
> would just cause me to resend all of them for individual patch issue.

All the patches you've sent make sense individually.  Thanks to you (and
the corporate overlord ;)) for doing the work and for sharing it, really.

Paolo
diff mbox series

Patch

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();