All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests
@ 2022-01-20  0:29 Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 1/7] bitops: Include stdbool.h and stddef.h as necessary Sean Christopherson
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Add single-step #DB + STI/MOVSS blocking regression tests for a related
KVM bug[*].  Before adding the test (last patch), overhaul asm/debugreg.h
and clean up x86's "debug" test.

[*] https://lore.kernel.org/all/20220120000624.655815-1-seanjc@google.com

Sean Christopherson (7):
  bitops: Include stdbool.h and stddef.h as necessary
  x86/debug: Add framework for single-step #DB tests
  x86/debug: Test OUT instead of RDMSR for single-step #DB emulation
    test
  x86/debug: Run single-step #DB tests in usermode (and kernel mode)
  x86: Overhaul definitions for DR6 and DR7 bits
  x86/debug: Add single-step #DB + STI/MOVSS blocking tests
  x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS
    sub-test

 lib/bitops.h           |   3 +
 lib/x86/asm/debugreg.h | 125 ++++++--------
 x86/debug.c            | 384 ++++++++++++++++++++++++++++++++++-------
 x86/emulator.c         |  14 +-
 x86/vmx_tests.c        |  27 +--
 5 files changed, 405 insertions(+), 148 deletions(-)


base-commit: 92a6c9b95ab10eba66bff3ff44476ab0c015b276
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 1/7] bitops: Include stdbool.h and stddef.h as necessary
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 2/7] x86/debug: Add framework for single-step #DB tests Sean Christopherson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Include stdbool.h and stddef.h in bitops.h to pick up the definitions for
"bool" and "size_t" respectively.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/bitops.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/bitops.h b/lib/bitops.h
index 308aa865..81a06a47 100644
--- a/lib/bitops.h
+++ b/lib/bitops.h
@@ -1,6 +1,9 @@
 #ifndef _BITOPS_H_
 #define _BITOPS_H_
 
+#include <stdbool.h>
+#include <stddef.h>
+
 /*
  * Adapted from
  *   include/linux/bitops.h
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 2/7] x86/debug: Add framework for single-step #DB tests
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 1/7] bitops: Include stdbool.h and stddef.h as necessary Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test Sean Christopherson
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Add a framework to the "debug" test for running single-step #DB tests,
future commits will extend the single-step tests to run in usermode and
to verify interaction with STI and MOVSS blocking.

Opportunistically add comments and stop open coding RFLAGS stuff.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/debug.c | 168 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 115 insertions(+), 53 deletions(-)

diff --git a/x86/debug.c b/x86/debug.c
index 0019ebd5..98bdfe36 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -36,14 +36,24 @@ static void handle_db(struct ex_regs *regs)
 	dr6[n] = read_dr6();
 
 	if (dr6[n] & 0x1)
-		regs->rflags |= (1 << 16);
+		regs->rflags |= X86_EFLAGS_RF;
 
 	if (++n >= 10) {
-		regs->rflags &= ~(1 << 8);
+		regs->rflags &= ~X86_EFLAGS_TF;
 		write_dr7(0x00000400);
 	}
 }
 
+static inline bool is_single_step_db(unsigned long dr6_val)
+{
+	return dr6_val == 0xffff4ff0;
+}
+
+static inline bool is_icebp_db(unsigned long dr6_val)
+{
+	return dr6_val == 0xffff0ff0;
+}
+
 extern unsigned char handle_db_save_rip;
 asm("handle_db_save_rip:\n"
    "stc\n"
@@ -64,15 +74,106 @@ static void handle_ud(struct ex_regs *regs)
 	got_ud = 1;
 }
 
+typedef unsigned long (*db_test_fn)(void);
+typedef void (*db_report_fn)(unsigned long);
+
+static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
+{
+	unsigned long start;
+
+	n = 0;
+	write_dr6(0);
+
+	start = test();
+	report_fn(start);
+}
+
+#define run_ss_db_test(name) __run_single_step_db_test(name, report_##name)
+
+static void report_singlestep_basic(unsigned long start)
+{
+	report(n == 3 &&
+	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 1 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 1 + 1,
+	       "Single-step #DB basic test");
+}
+
+static unsigned long singlestep_basic(void)
+{
+	unsigned long start;
+
+	/*
+	 * After being enabled, single-step breakpoints have a one instruction
+	 * delay before the first #DB is generated.
+	 */
+	asm volatile (
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"and $~(1<<8),%%rax\n\t"
+		"1:push %%rax\n\t"
+		"popf\n\t"
+		"lea 1b, %0\n\t"
+		: "=r" (start) : : "rax"
+	);
+	return start;
+}
+
+static void report_singlestep_emulated_instructions(unsigned long start)
+{
+	report(n == 7 &&
+	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 1 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 1 + 3 &&
+	       is_single_step_db(dr6[3]) && db_addr[3] == start + 1 + 3 + 2 &&
+	       is_single_step_db(dr6[4]) && db_addr[4] == start + 1 + 3 + 2 + 5 &&
+	       is_single_step_db(dr6[5]) && db_addr[5] == start + 1 + 3 + 2 + 5 + 2 &&
+	       is_single_step_db(dr6[6]) && db_addr[6] == start + 1 + 3 + 2 + 5 + 2 + 1,
+	       "Single-step #DB on emulated instructions");
+}
+
+static unsigned long singlestep_emulated_instructions(void)
+{
+	unsigned long start;
+
+	/*
+	 * Verify single-step #DB are generated correctly on emulated
+	 * instructions, e.g. CPUID and RDMSR.
+	 */
+	asm volatile (
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"and $~(1<<8),%%rax\n\t"
+		"1:push %%rax\n\t"
+		"xor %%rax,%%rax\n\t"
+		"cpuid\n\t"
+		"movl $0x1a0,%%ecx\n\t"
+		"rdmsr\n\t"
+		"popf\n\t"
+		"lea 1b,%0\n\t"
+		: "=r" (start) : : "rax", "ebx", "ecx", "edx"
+	);
+	return start;
+}
+
 int main(int ac, char **av)
 {
-	unsigned long start;
 	unsigned long cr4;
 
 	handle_exception(DB_VECTOR, handle_db);
 	handle_exception(BP_VECTOR, handle_bp);
 	handle_exception(UD_VECTOR, handle_ud);
 
+	/*
+	 * DR4 is an alias for DR6 (and DR5 aliases DR7) if CR4.DE is NOT set,
+	 * and is reserved if CR4.DE=1 (Debug Extensions enabled).
+	 */
 	got_ud = 0;
 	cr4 = read_cr4();
 	write_cr4(cr4 & ~X86_CR4_DE);
@@ -83,13 +184,21 @@ int main(int ac, char **av)
 	cr4 = read_cr4();
 	write_cr4(cr4 | X86_CR4_DE);
 	read_dr4();
-	report(got_ud, "reading DR4 with CR4.DE == 1");
+	report(got_ud, "DR4 read got #UD with CR4.DE == 1");
 	write_dr6(0);
 
 	extern unsigned char sw_bp;
 	asm volatile("int3; sw_bp:");
 	report(bp_addr == (unsigned long)&sw_bp, "#BP");
 
+	/*
+	 * The CPU sets/clears bits 0-3 (trap bits for DR0-3) on #DB based on
+	 * whether or not the corresponding DR0-3 got a match.  All other bits
+	 * in DR6 are set if and only if their associated breakpoint condition
+	 * is active, and are never cleared by the CPU.  Verify a match on DR0
+	 * is reported correctly, and that DR6.BS is not set when single-step
+	 * breakpoints are disabled, but is left set (if set by software).
+	 */
 	n = 0;
 	extern unsigned char hw_bp1;
 	write_dr0(&hw_bp1);
@@ -108,55 +217,8 @@ int main(int ac, char **av)
 	       db_addr[0] == ((unsigned long)&hw_bp2) && dr6[0] == 0xffff4ff1,
 	       "hw breakpoint (test that dr6.BS is not cleared)");
 
-	n = 0;
-	write_dr6(0);
-	asm volatile(
-		"pushf\n\t"
-		"pop %%rax\n\t"
-		"or $(1<<8),%%rax\n\t"
-		"push %%rax\n\t"
-		"lea (%%rip),%0\n\t"
-		"popf\n\t"
-		"and $~(1<<8),%%rax\n\t"
-		"push %%rax\n\t"
-		"popf\n\t"
-		: "=r" (start) : : "rax");
-	report(n == 3 &&
-	       db_addr[0] == start + 1 + 6 && dr6[0] == 0xffff4ff0 &&
-	       db_addr[1] == start + 1 + 6 + 1 && dr6[1] == 0xffff4ff0 &&
-	       db_addr[2] == start + 1 + 6 + 1 + 1 && dr6[2] == 0xffff4ff0,
-	       "single step");
-
-	/*
-	 * cpuid and rdmsr (among others) trigger VM exits and are then
-	 * emulated. Test that single stepping works on emulated instructions.
-	 */
-	n = 0;
-	write_dr6(0);
-	asm volatile(
-		"pushf\n\t"
-		"pop %%rax\n\t"
-		"or $(1<<8),%%rax\n\t"
-		"push %%rax\n\t"
-		"lea (%%rip),%0\n\t"
-		"popf\n\t"
-		"and $~(1<<8),%%rax\n\t"
-		"push %%rax\n\t"
-		"xor %%rax,%%rax\n\t"
-		"cpuid\n\t"
-		"movl $0x1a0,%%ecx\n\t"
-		"rdmsr\n\t"
-		"popf\n\t"
-		: "=r" (start) : : "rax", "ebx", "ecx", "edx");
-	report(n == 7 &&
-	       db_addr[0] == start + 1 + 6 && dr6[0] == 0xffff4ff0 &&
-	       db_addr[1] == start + 1 + 6 + 1 && dr6[1] == 0xffff4ff0 &&
-	       db_addr[2] == start + 1 + 6 + 1 + 3 && dr6[2] == 0xffff4ff0 &&
-	       db_addr[3] == start + 1 + 6 + 1 + 3 + 2 && dr6[3] == 0xffff4ff0 &&
-	       db_addr[4] == start + 1 + 6 + 1 + 3 + 2 + 5 && dr6[4] == 0xffff4ff0 &&
-	       db_addr[5] == start + 1 + 6 + 1 + 3 + 2 + 5 + 2 && dr6[5] == 0xffff4ff0 &&
-	       db_addr[6] == start + 1 + 6 + 1 + 3 + 2 + 5 + 2 + 1 && dr6[6] == 0xffff4ff0,
-	       "single step emulated instructions");
+	run_ss_db_test(singlestep_basic);
+	run_ss_db_test(singlestep_emulated_instructions);
 
 	n = 0;
 	write_dr1((void *)&value);
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 1/7] bitops: Include stdbool.h and stddef.h as necessary Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 2/7] x86/debug: Add framework for single-step #DB tests Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-24 14:15   ` Paolo Bonzini
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 4/7] x86/debug: Run single-step #DB tests in usermode (and kernel mode) Sean Christopherson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Replace RDMSR with OUT so that testing single-step #DBs on emulated
instructions can be run in userspace (by modifying IOPL).  OUT is also
more interesting in that it is guaranteed to exit to host userspace,
whereas RDMSR will do so if and only if userspace is filtering the target
MSR.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/debug.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/x86/debug.c b/x86/debug.c
index 98bdfe36..4b2fbe97 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -124,14 +124,13 @@ static unsigned long singlestep_basic(void)
 
 static void report_singlestep_emulated_instructions(unsigned long start)
 {
-	report(n == 7 &&
+	report(n == 6 &&
 	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
 	       is_single_step_db(dr6[1]) && db_addr[1] == start + 1 &&
 	       is_single_step_db(dr6[2]) && db_addr[2] == start + 1 + 3 &&
 	       is_single_step_db(dr6[3]) && db_addr[3] == start + 1 + 3 + 2 &&
-	       is_single_step_db(dr6[4]) && db_addr[4] == start + 1 + 3 + 2 + 5 &&
-	       is_single_step_db(dr6[5]) && db_addr[5] == start + 1 + 3 + 2 + 5 + 2 &&
-	       is_single_step_db(dr6[6]) && db_addr[6] == start + 1 + 3 + 2 + 5 + 2 + 1,
+	       is_single_step_db(dr6[4]) && db_addr[4] == start + 1 + 3 + 2 + 2 &&
+	       is_single_step_db(dr6[5]) && db_addr[5] == start + 1 + 3 + 2 + 2 + 1,
 	       "Single-step #DB on emulated instructions");
 }
 
@@ -153,8 +152,7 @@ static unsigned long singlestep_emulated_instructions(void)
 		"1:push %%rax\n\t"
 		"xor %%rax,%%rax\n\t"
 		"cpuid\n\t"
-		"movl $0x1a0,%%ecx\n\t"
-		"rdmsr\n\t"
+		"out %%eax, $0x80\n\t"
 		"popf\n\t"
 		"lea 1b,%0\n\t"
 		: "=r" (start) : : "rax", "ebx", "ecx", "edx"
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 4/7] x86/debug: Run single-step #DB tests in usermode (and kernel mode)
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
                   ` (2 preceding siblings ...)
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 5/7] x86: Overhaul definitions for DR6 and DR7 bits Sean Christopherson
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Run the single-step #DB tests in usermode in addition to running them in
kernel mode, i.e. run at CPL0 and CPL3.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/debug.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/x86/debug.c b/x86/debug.c
index 4b2fbe97..21f1da0b 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -12,6 +12,7 @@
 #include "libcflat.h"
 #include "processor.h"
 #include "desc.h"
+#include "usermode.h"
 
 static volatile unsigned long bp_addr;
 static volatile unsigned long db_addr[10], dr6[10];
@@ -75,28 +76,43 @@ static void handle_ud(struct ex_regs *regs)
 }
 
 typedef unsigned long (*db_test_fn)(void);
-typedef void (*db_report_fn)(unsigned long);
+typedef void (*db_report_fn)(unsigned long, const char *);
 
 static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
 {
 	unsigned long start;
+	bool ign;
 
 	n = 0;
 	write_dr6(0);
 
 	start = test();
-	report_fn(start);
+	report_fn(start, "");
+
+	n = 0;
+	write_dr6(0);
+	/*
+	 * Run the test in usermode.  Use the expected start RIP from the first
+	 * run, the usermode framework doesn't make it easy to get the expected
+	 * RIP out of the test, and it shouldn't change in any case.  Run the
+	 * test with IOPL=3 so that it can use OUT, CLI, STI, etc...
+	 */
+	set_iopl(3);
+	run_in_user((usermode_func)test, GP_VECTOR, 0, 0, 0, 0, &ign);
+	set_iopl(0);
+
+	report_fn(start, "Usermode ");
 }
 
 #define run_ss_db_test(name) __run_single_step_db_test(name, report_##name)
 
-static void report_singlestep_basic(unsigned long start)
+static void report_singlestep_basic(unsigned long start, const char *usermode)
 {
 	report(n == 3 &&
 	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
 	       is_single_step_db(dr6[1]) && db_addr[1] == start + 1 &&
 	       is_single_step_db(dr6[2]) && db_addr[2] == start + 1 + 1,
-	       "Single-step #DB basic test");
+	       "%sSingle-step #DB basic test", usermode);
 }
 
 static unsigned long singlestep_basic(void)
@@ -122,7 +138,8 @@ static unsigned long singlestep_basic(void)
 	return start;
 }
 
-static void report_singlestep_emulated_instructions(unsigned long start)
+static void report_singlestep_emulated_instructions(unsigned long start,
+						    const char *usermode)
 {
 	report(n == 6 &&
 	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
@@ -131,7 +148,7 @@ static void report_singlestep_emulated_instructions(unsigned long start)
 	       is_single_step_db(dr6[3]) && db_addr[3] == start + 1 + 3 + 2 &&
 	       is_single_step_db(dr6[4]) && db_addr[4] == start + 1 + 3 + 2 + 2 &&
 	       is_single_step_db(dr6[5]) && db_addr[5] == start + 1 + 3 + 2 + 2 + 1,
-	       "Single-step #DB on emulated instructions");
+	       "%sSingle-step #DB on emulated instructions", usermode);
 }
 
 static unsigned long singlestep_emulated_instructions(void)
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 5/7] x86: Overhaul definitions for DR6 and DR7 bits
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
                   ` (3 preceding siblings ...)
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 4/7] x86/debug: Run single-step #DB tests in usermode (and kernel mode) Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 6/7] x86/debug: Add single-step #DB + STI/MOVSS blocking tests Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test Sean Christopherson
  6 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Clean up the mess that is debugreg.h to follow the nomenclature used by
the SDM and the kernel (as best as possible).  Use the "new" defines in
various tests.  Opportunistically add a define for VMX's extra flag in
vmcs.GUEST_PENDING_DBG_EXCEPTIONS that is set if any DR0-3 trap matched
and was enabled.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/asm/debugreg.h | 125 ++++++++++++++++++-----------------------
 x86/debug.c            |  28 +++++----
 x86/emulator.c         |  14 +++--
 x86/vmx_tests.c        |  27 +++++----
 4 files changed, 97 insertions(+), 97 deletions(-)

diff --git a/lib/x86/asm/debugreg.h b/lib/x86/asm/debugreg.h
index e86f5a62..a30f9493 100644
--- a/lib/x86/asm/debugreg.h
+++ b/lib/x86/asm/debugreg.h
@@ -2,80 +2,63 @@
 #ifndef _ASMX86_DEBUGREG_H_
 #define _ASMX86_DEBUGREG_H_
 
+#include <bitops.h>
 
-/* Indicate the register numbers for a number of the specific
-   debug registers.  Registers 0-3 contain the addresses we wish to trap on */
-#define DR_FIRSTADDR 0        /* u_debugreg[DR_FIRSTADDR] */
-#define DR_LASTADDR 3         /* u_debugreg[DR_LASTADDR]  */
-
-#define DR_STATUS 6           /* u_debugreg[DR_STATUS]     */
-#define DR_CONTROL 7          /* u_debugreg[DR_CONTROL] */
-
-/* Define a few things for the status register.  We can use this to determine
-   which debugging register was responsible for the trap.  The other bits
-   are either reserved or not of interest to us. */
-
-/* Define reserved bits in DR6 which are always set to 1 */
-#define DR6_RESERVED	(0xFFFF0FF0)
-
-#define DR_TRAP0	(0x1)		/* db0 */
-#define DR_TRAP1	(0x2)		/* db1 */
-#define DR_TRAP2	(0x4)		/* db2 */
-#define DR_TRAP3	(0x8)		/* db3 */
-#define DR_TRAP_BITS	(DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3)
-
-#define DR_STEP		(0x4000)	/* single-step */
-#define DR_SWITCH	(0x8000)	/* task switch */
-
-/* Now define a bunch of things for manipulating the control register.
-   The top two bytes of the control register consist of 4 fields of 4
-   bits - each field corresponds to one of the four debug registers,
-   and indicates what types of access we trap on, and how large the data
-   field is that we are looking at */
-
-#define DR_CONTROL_SHIFT 16 /* Skip this many bits in ctl register */
-#define DR_CONTROL_SIZE 4   /* 4 control bits per register */
-
-#define DR_RW_EXECUTE (0x0)   /* Settings for the access types to trap on */
-#define DR_RW_WRITE (0x1)
-#define DR_RW_READ (0x3)
-
-#define DR_LEN_1 (0x0) /* Settings for data length to trap on */
-#define DR_LEN_2 (0x4)
-#define DR_LEN_4 (0xC)
-#define DR_LEN_8 (0x8)
-
-/* The low byte to the control register determine which registers are
-   enabled.  There are 4 fields of two bits.  One bit is "local", meaning
-   that the processor will reset the bit after a task switch and the other
-   is global meaning that we have to explicitly reset the bit.  With linux,
-   you can use either one, since we explicitly zero the register when we enter
-   kernel mode. */
-
-#define DR_LOCAL_ENABLE_SHIFT 0    /* Extra shift to the local enable bit */
-#define DR_GLOBAL_ENABLE_SHIFT 1   /* Extra shift to the global enable bit */
-#define DR_LOCAL_ENABLE (0x1)      /* Local enable for reg 0 */
-#define DR_GLOBAL_ENABLE (0x2)     /* Global enable for reg 0 */
-#define DR_ENABLE_SIZE 2           /* 2 enable bits per register */
-
-#define DR_LOCAL_ENABLE_MASK (0x55)  /* Set  local bits for all 4 regs */
-#define DR_GLOBAL_ENABLE_MASK (0xAA) /* Set global bits for all 4 regs */
-
-/* The second byte to the control register has a few special things.
-   We can slow the instruction pipeline for instructions coming via the
-   gdt or the ldt if we want to.  I am not sure why this is an advantage */
-
-#ifdef __i386__
-#define DR_CONTROL_RESERVED (0xFC00) /* Reserved by Intel */
-#else
-#define DR_CONTROL_RESERVED (0xFFFFFFFF0000FC00UL) /* Reserved */
-#endif
-
-#define DR_LOCAL_SLOWDOWN (0x100)   /* Local slow the pipeline */
-#define DR_GLOBAL_SLOWDOWN (0x200)  /* Global slow the pipeline */
+/*
+ * DR6_ACTIVE_LOW combines fixed-1 and active-low bits (e.g. RTM), and is also
+ * the init/reset value for DR6.
+ */
+#define DR6_ACTIVE_LOW	0xffff0ff0
+#define DR6_VOLATILE	0x0001e80f
+#define DR6_FIXED_1	(DR6_ACTIVE_LOW & ~DR6_VOLATILE)
+
+#define DR6_TRAP0	BIT(0)		/* DR0 matched */
+#define DR6_TRAP1	BIT(1)		/* DR1 matched */
+#define DR6_TRAP2	BIT(2)		/* DR2 matched */
+#define DR6_TRAP3	BIT(3)		/* DR3 matched */
+#define DR6_TRAP_BITS	(DR6_TRAP0|DR6_TRAP1|DR6_TRAP2|DR6_TRAP3)
+
+#define DR6_BUS_LOCK	BIT(11)		/* Bus lock	    0x800 */
+#define DR6_BD		BIT(13)		/* General Detect  0x2000 */
+#define DR6_BS		BIT(14)		/* Single-Step	   0x4000 */
+#define DR6_BT		BIT(15)		/* Task Switch	   0x8000 */
+#define DR6_RTM		BIT(16)		/* RTM / TSX	  0x10000 */
+
+#define DR7_FIXED_1	0x00000400	/* init/reset value, too */
+#define DR7_VOLATILE	0xffff2bff
+#define DR7_BP_EN_MASK	0x000000ff
+#define DR7_LE		BIT(8)		/* Local Exact	    0x100 */
+#define DR7_GE		BIT(9)		/* Global Exact     0x200 */
+#define DR7_RTM		BIT(11)		/* RTM / TSX	    0x800 */
+#define DR7_GD		BIT(13)		/* General Detect  0x2000 */
 
 /*
- * HW breakpoint additions
+ * Enable bits for DR0-D3.  Bits 0, 2, 4, and 6 are local enable bits (cleared
+ * by the CPU on task switch), bits 1, 3, 5, and 7 are global enable bits
+ * (never cleared by the CPU).
  */
+#define DR7_LOCAL_ENABLE_DRx(x)		(BIT(0) << (x))
+#define DR7_GLOBAL_ENABLE_DRx(x)	(BIT(1) << (x))
+#define DR7_ENABLE_DRx(x) \
+	(DR7_LOCAL_ENABLE_DRx(x) | DR7_GLOBAL_ENABLE_DRx(x))
+
+#define DR7_GLOBAL_ENABLE_DR0	DR7_GLOBAL_ENABLE_DRx(0)
+#define DR7_GLOBAL_ENABLE_DR1	DR7_GLOBAL_ENABLE_DRx(1)
+#define DR7_GLOBAL_ENABLE_DR2	DR7_GLOBAL_ENABLE_DRx(2)
+#define DR7_GLOBAL_ENABLE_DR3	DR7_GLOBAL_ENABLE_DRx(3)
+
+/* Condition/type of the breakpoint for DR0-3. */
+#define DR7_RW_TYPE_DRx(x, rw)	((rw) << (((x) * 4) + 16))
+#define DR7_EXECUTE_DRx(x)	DR7_RW_TYPE_DRx(x, 0)
+#define DR7_WRITE_DRx(x)	DR7_RW_TYPE_DRx(x, 1)
+#define DR7_PORT_IO_DRx(x)	DR7_RW_TYPE_DRx(x, 2)
+#define DR7_DATA_IO_DRx(x)	DR7_RW_TYPE_DRx(x, 3)	/* Read or Write */
+
+/* Length of the breakpoint for DR0-3. */
+#define DR7_LEN_DRx(x, enc)	((enc) << (((x) * 4) + 18))
+#define DR7_LEN_1_DRx(x)	DR7_LEN_DRx(x, 0)
+#define DR7_LEN_2_DRx(x)	DR7_LEN_DRx(x, 1)
+#define DR7_LEN_4_DRx(x)	DR7_LEN_DRx(x, 3)
+#define DR7_LEN_8_DRx(x)	DR7_LEN_DRx(x, 2) /* Out of sequence, undefined for 32-bit CPUs. */
 
 #endif /* _ASMX86_DEBUGREG_H_ */
diff --git a/x86/debug.c b/x86/debug.c
index 21f1da0b..5835a064 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -8,6 +8,7 @@
  *
  * This work is licensed under the terms of the GNU GPL, version 2.
  */
+#include <asm/debugreg.h>
 
 #include "libcflat.h"
 #include "processor.h"
@@ -47,12 +48,12 @@ static void handle_db(struct ex_regs *regs)
 
 static inline bool is_single_step_db(unsigned long dr6_val)
 {
-	return dr6_val == 0xffff4ff0;
+	return dr6_val == (DR6_ACTIVE_LOW | DR6_BS);
 }
 
 static inline bool is_icebp_db(unsigned long dr6_val)
 {
-	return dr6_val == 0xffff0ff0;
+	return dr6_val == DR6_ACTIVE_LOW;
 }
 
 extern unsigned char handle_db_save_rip;
@@ -193,8 +194,9 @@ int main(int ac, char **av)
 	cr4 = read_cr4();
 	write_cr4(cr4 & ~X86_CR4_DE);
 	write_dr4(0);
-	write_dr6(0xffff4ff2);
-	report(read_dr4() == 0xffff4ff2 && !got_ud, "reading DR4 with CR4.DE == 0");
+	write_dr6(DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1);
+	report(read_dr4() == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1) && !got_ud,
+	       "DR4==DR6 with CR4.DE == 0");
 
 	cr4 = read_cr4();
 	write_cr4(cr4 | X86_CR4_DE);
@@ -217,19 +219,21 @@ int main(int ac, char **av)
 	n = 0;
 	extern unsigned char hw_bp1;
 	write_dr0(&hw_bp1);
-	write_dr7(0x00000402);
+	write_dr7(DR7_FIXED_1 | DR7_GLOBAL_ENABLE_DR0);
 	asm volatile("hw_bp1: nop");
 	report(n == 1 &&
-	       db_addr[0] == ((unsigned long)&hw_bp1) && dr6[0] == 0xffff0ff1,
+	       db_addr[0] == ((unsigned long)&hw_bp1) &&
+	       dr6[0] == (DR6_ACTIVE_LOW | DR6_TRAP0),
 	       "hw breakpoint (test that dr6.BS is not set)");
 
 	n = 0;
 	extern unsigned char hw_bp2;
 	write_dr0(&hw_bp2);
-	write_dr6(0x00004002);
+	write_dr6(DR6_BS | DR6_TRAP1);
 	asm volatile("hw_bp2: nop");
 	report(n == 1 &&
-	       db_addr[0] == ((unsigned long)&hw_bp2) && dr6[0] == 0xffff4ff1,
+	       db_addr[0] == ((unsigned long)&hw_bp2) &&
+	       dr6[0] == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP0),
 	       "hw breakpoint (test that dr6.BS is not cleared)");
 
 	run_ss_db_test(singlestep_basic);
@@ -245,7 +249,8 @@ int main(int ac, char **av)
 		"mov %%rax,%0\n\t; hw_wp1:"
 		: "=m" (value) : : "rax");
 	report(n == 1 &&
-	       db_addr[0] == ((unsigned long)&hw_wp1) && dr6[0] == 0xffff4ff2,
+	       db_addr[0] == ((unsigned long)&hw_wp1) &&
+	       dr6[0] == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1),
 	       "hw watchpoint (test that dr6.BS is not cleared)");
 
 	n = 0;
@@ -257,7 +262,8 @@ int main(int ac, char **av)
 		"mov %%rax,%0\n\t; hw_wp2:"
 		: "=m" (value) : : "rax");
 	report(n == 1 &&
-	       db_addr[0] == ((unsigned long)&hw_wp2) && dr6[0] == 0xffff0ff2,
+	       db_addr[0] == ((unsigned long)&hw_wp2) &&
+	       dr6[0] == (DR6_ACTIVE_LOW | DR6_TRAP1),
 	       "hw watchpoint (test that dr6.BS is not set)");
 
 	n = 0;
@@ -265,7 +271,7 @@ int main(int ac, char **av)
 	extern unsigned char sw_icebp;
 	asm volatile(".byte 0xf1; sw_icebp:");
 	report(n == 1 &&
-	       db_addr[0] == (unsigned long)&sw_icebp && dr6[0] == 0xffff0ff0,
+	       db_addr[0] == (unsigned long)&sw_icebp && dr6[0] == DR6_ACTIVE_LOW,
 	       "icebp");
 
 	write_dr7(0x400);
diff --git a/x86/emulator.c b/x86/emulator.c
index 22a518f4..cd78e3cb 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1,3 +1,5 @@
+#include <asm/debugreg.h>
+
 #include "ioram.h"
 #include "vm.h"
 #include "libcflat.h"
@@ -883,12 +885,14 @@ static void test_nop(uint64_t *mem)
 static void test_mov_dr(uint64_t *mem)
 {
 	unsigned long rax;
-	const unsigned long in_rax = 0;
-	bool rtm_support = this_cpu_has(X86_FEATURE_RTM);
-	unsigned long dr6_fixed_1 = rtm_support ? 0xfffe0ff0ul : 0xffff0ff0ul;
+
 	asm(KVM_FEP "movq %0, %%dr6\n\t"
-	    KVM_FEP "movq %%dr6, %0\n\t" : "=a" (rax) : "a" (in_rax));
-	report(rax == dr6_fixed_1, "mov_dr6");
+	    KVM_FEP "movq %%dr6, %0\n\t" : "=a" (rax) : "a" (0));
+
+	if (this_cpu_has(X86_FEATURE_RTM))
+		report(rax == (DR6_ACTIVE_LOW & ~DR6_RTM), "mov_dr6");
+	else
+		report(rax == DR6_ACTIVE_LOW, "mov_dr6");
 }
 
 static void test_push16(uint64_t *mem)
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 3d57ed6c..e67eaea4 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -22,6 +22,13 @@
 #include "delay.h"
 #include "access.h"
 
+/*
+ * vmcs.GUEST_PENDING_DEBUG has the same format as DR6, although some bits that
+ * are legal in DR6 are reserved in vmcs.GUEST_PENDING_DEBUG.  And if any data
+ * or I/O breakpoint matches *and* was enabled, bit 12 is also set.
+ */
+#define PENDING_DBG_TRAP	BIT(12)
+
 #define VPID_CAP_INVVPID_TYPES_SHIFT 40
 
 u64 ia32_pat;
@@ -5080,9 +5087,9 @@ static void vmx_mtf_test(void)
 	enter_guest();
 	report_mtf("OUT", (unsigned long) &test_mtf2);
 	pending_dbg = vmcs_read(GUEST_PENDING_DEBUG);
-	report(pending_dbg & DR_STEP,
+	report(pending_dbg & DR6_BS,
 	       "'pending debug exceptions' field after MTF VM-exit: 0x%lx (expected 0x%lx)",
-	       pending_dbg, (unsigned long) DR_STEP);
+	       pending_dbg, (unsigned long) DR6_BS);
 
 	disable_mtf();
 	disable_tf();
@@ -8931,7 +8938,7 @@ static void vmx_preemption_timer_zero_inject_db(bool intercept_db)
 static void vmx_preemption_timer_zero_set_pending_dbg(u32 exception_bitmap)
 {
 	vmx_preemption_timer_zero_activate_preemption_timer();
-	vmcs_write(GUEST_PENDING_DEBUG, BIT(12) | DR_TRAP1);
+	vmcs_write(GUEST_PENDING_DEBUG, PENDING_DBG_TRAP | DR6_TRAP1);
 	vmcs_write(EXC_BITMAP, exception_bitmap);
 	enter_guest();
 }
@@ -9315,7 +9322,7 @@ static void vmx_db_test(void)
 	 * (b) stale bits in DR6 (DR6.BD, in particular) don't leak into
          *     the exit qualification field for a subsequent #DB exception.
 	 */
-	const u64 starting_dr6 = DR6_RESERVED | BIT(13) | DR_TRAP3 | DR_TRAP1;
+	const u64 starting_dr6 = DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP3 | DR6_TRAP1;
 	extern char post_nop asm(".Lpost_nop");
 	extern char post_movss_nop asm(".Lpost_movss_nop");
 	extern char post_wbinvd asm(".Lpost_wbinvd");
@@ -9339,7 +9346,7 @@ static void vmx_db_test(void)
 	 * standard that L0 has to follow for emulated instructions.
 	 */
 	single_step_guest("Hardware delivered single-step", starting_dr6, 0);
-	check_db_exit(false, false, false, &post_nop, DR_STEP, starting_dr6);
+	check_db_exit(false, false, false, &post_nop, DR6_BS, starting_dr6);
 
 	/*
 	 * Hardware-delivered #DB trap for single-step in MOVSS shadow
@@ -9349,8 +9356,8 @@ static void vmx_db_test(void)
 	 * data breakpoint as well as the single-step trap.
 	 */
 	single_step_guest("Hardware delivered single-step in MOVSS shadow",
-			  starting_dr6, BIT(12) | DR_STEP | DR_TRAP0 );
-	check_db_exit(false, false, false, &post_movss_nop, DR_STEP | DR_TRAP0,
+			  starting_dr6, DR6_BS | PENDING_DBG_TRAP | DR6_TRAP0);
+	check_db_exit(false, false, false, &post_movss_nop, DR6_BS | DR6_TRAP0,
 		      starting_dr6);
 
 	/*
@@ -9360,7 +9367,7 @@ static void vmx_db_test(void)
 	 * modified DR6, but fails miserably.
 	 */
 	single_step_guest("Software synthesized single-step", starting_dr6, 0);
-	check_db_exit(false, false, false, &post_wbinvd, DR_STEP, starting_dr6);
+	check_db_exit(false, false, false, &post_wbinvd, DR6_BS, starting_dr6);
 
 	/*
 	 * L0 synthesized #DB trap for single-step in MOVSS shadow is
@@ -9369,8 +9376,8 @@ static void vmx_db_test(void)
 	 * the exit qualification field for the #DB exception.
 	 */
 	single_step_guest("Software synthesized single-step in MOVSS shadow",
-			  starting_dr6, BIT(12) | DR_STEP | DR_TRAP0);
-	check_db_exit(true, false, true, &post_movss_wbinvd, DR_STEP | DR_TRAP0,
+			  starting_dr6, DR6_BS | PENDING_DBG_TRAP | DR6_TRAP0);
+	check_db_exit(true, false, true, &post_movss_wbinvd, DR6_BS | DR6_TRAP0,
 		      starting_dr6);
 
 	/*
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 6/7] x86/debug: Add single-step #DB + STI/MOVSS blocking tests
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
                   ` (4 preceding siblings ...)
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 5/7] x86: Overhaul definitions for DR6 and DR7 bits Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test Sean Christopherson
  6 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Add a variety of test cases to verify single-step #DB interaction with
STI and MOVSS blocking.  Of particular note are STI blocking and MOVSS
blocking with DR7.GD=1, both of which require manual intervention from
the hypervisor to set vmcs.GUEST_PENDING_DBG_EXCEPTION.BS when
re-injecting an intercepted #DB with STI/MOVSS blocking active.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alexander Graf <graf@amazon.de>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/debug.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)

diff --git a/x86/debug.c b/x86/debug.c
index 5835a064..0165dc68 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -51,6 +51,11 @@ static inline bool is_single_step_db(unsigned long dr6_val)
 	return dr6_val == (DR6_ACTIVE_LOW | DR6_BS);
 }
 
+static inline bool is_general_detect_db(unsigned long dr6_val)
+{
+	return dr6_val == (DR6_ACTIVE_LOW | DR6_BD);
+}
+
 static inline bool is_icebp_db(unsigned long dr6_val)
 {
 	return dr6_val == DR6_ACTIVE_LOW;
@@ -79,6 +84,8 @@ static void handle_ud(struct ex_regs *regs)
 typedef unsigned long (*db_test_fn)(void);
 typedef void (*db_report_fn)(unsigned long, const char *);
 
+static unsigned long singlestep_with_movss_blocking_and_dr7_gd(void);
+
 static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
 {
 	unsigned long start;
@@ -90,8 +97,13 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
 	start = test();
 	report_fn(start, "");
 
+	/* MOV DR #GPs at CPL>0, don't try to run the DR7.GD test in usermode. */
+	if (test == singlestep_with_movss_blocking_and_dr7_gd)
+		return;
+
 	n = 0;
 	write_dr6(0);
+
 	/*
 	 * Run the test in usermode.  Use the expected start RIP from the first
 	 * run, the usermode framework doesn't make it easy to get the expected
@@ -178,6 +190,166 @@ static unsigned long singlestep_emulated_instructions(void)
 	return start;
 }
 
+static void report_singlestep_with_sti_blocking(unsigned long start,
+						const char *usermode)
+{
+	report(n == 4 &&
+	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 6 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 6 + 1 &&
+	       is_single_step_db(dr6[3]) && db_addr[3] == start + 6 + 1 + 1,
+	       "%sSingle-step #DB w/ STI blocking", usermode);
+}
+
+
+static unsigned long singlestep_with_sti_blocking(void)
+{
+	unsigned long start_rip;
+
+	/*
+	 * STI blocking doesn't suppress #DBs, thus the first single-step #DB
+	 * should arrive after the standard one instruction delay.
+	 */
+	asm volatile(
+		"cli\n\t"
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"sti\n\t"
+		"1:and $~(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"lea 1b,%0\n\t"
+		: "=r" (start_rip) : : "rax"
+	);
+	return start_rip;
+}
+
+static void report_singlestep_with_movss_blocking(unsigned long start,
+						  const char *usermode)
+{
+	report(n == 3 &&
+	       is_single_step_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 1 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 1 + 1,
+	       "%sSingle-step #DB w/ MOVSS blocking", usermode);
+}
+
+static unsigned long singlestep_with_movss_blocking(void)
+{
+	unsigned long start_rip;
+
+	/*
+	 * MOVSS blocking suppresses single-step #DBs (and select other #DBs),
+	 * thus the first single-step #DB should occur after MOVSS blocking
+	 * expires, i.e. two instructions after #DBs are enabled in this case.
+	 */ 
+	asm volatile(
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"mov %%ss, %%ax\n\t"
+		"popf\n\t"
+		"mov %%ax, %%ss\n\t"
+		"and $~(1<<8),%%rax\n\t"
+		"1: push %%rax\n\t"
+		"popf\n\t"
+		"lea 1b,%0\n\t"
+		: "=r" (start_rip) : : "rax"
+	);
+	return start_rip;
+}
+
+
+static void report_singlestep_with_movss_blocking_and_icebp(unsigned long start,
+							    const char *usermode)
+{
+	report(n == 4 &&
+	       is_icebp_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 6 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 6 + 1 &&
+	       is_single_step_db(dr6[3]) && db_addr[3] == start + 6 + 1 + 1,
+	       "%sSingle-Step + ICEBP #DB w/ MOVSS blocking", usermode);
+}
+
+static unsigned long singlestep_with_movss_blocking_and_icebp(void)
+{
+	unsigned long start;
+
+	/*
+	 * ICEBP, a.k.a. INT1 or int1icebrk, is an oddball.  It generates a
+	 * trap-like #DB, is intercepted if #DBs are intercepted, and manifests
+	 * as a #DB VM-Exit, but the VM-Exit occurs on the ICEBP itself, i.e.
+	 * it's treated as an instruction intercept.  Verify that ICEBP is
+	 * correctly emulated as a trap-like #DB when intercepted, and that
+	 * MOVSS blocking is handled correctly with respect to single-step
+	 * breakpoints being enabled.
+	 */
+	asm volatile(
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"mov %%ss, %%ax\n\t"
+		"popf\n\t"
+		"mov %%ax, %%ss\n\t"
+		".byte 0xf1;"
+		"1:and $~(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"lea 1b,%0\n\t"
+		: "=r" (start) : : "rax"
+	);
+	return start;
+}
+
+static void report_singlestep_with_movss_blocking_and_dr7_gd(unsigned long start,
+							     const char *ign)
+{
+	report(n == 5 &&
+	       is_general_detect_db(dr6[0]) && db_addr[0] == start &&
+	       is_single_step_db(dr6[1]) && db_addr[1] == start + 3 &&
+	       is_single_step_db(dr6[2]) && db_addr[2] == start + 3 + 6 &&
+	       is_single_step_db(dr6[3]) && db_addr[3] == start + 3 + 6 + 1 &&
+	       is_single_step_db(dr6[4]) && db_addr[4] == start + 3 + 6 + 1 + 1,
+	       "Single-step #DB w/ MOVSS blocking and DR7.GD=1");
+}
+
+static unsigned long singlestep_with_movss_blocking_and_dr7_gd(void)
+{
+	unsigned long start_rip;
+
+	write_dr7(DR7_GD);
+
+	/*
+	 * MOVSS blocking does NOT suppress General Detect #DBs, which have
+	 * fault-like behavior.  Note, DR7.GD is cleared by the CPU upon
+	 * successful delivery of the #DB.  DR6.BD is NOT cleared by the CPU,
+	 * but the MOV DR6 below will be re-executed after handling the
+	 * General Detect #DB.
+	 */
+	asm volatile(
+		"xor %0, %0\n\t"
+		"pushf\n\t"
+		"pop %%rax\n\t"
+		"or $(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"mov %%ss, %%ax\n\t"
+		"popf\n\t"
+		"mov %%ax, %%ss\n\t"
+		"1: mov %0, %%dr6\n\t"
+		"and $~(1<<8),%%rax\n\t"
+		"push %%rax\n\t"
+		"popf\n\t"
+		"lea 1b,%0\n\t"
+		: "=r" (start_rip) : : "rax"
+	);
+	return start_rip;
+}
+
 int main(int ac, char **av)
 {
 	unsigned long cr4;
@@ -238,6 +410,10 @@ int main(int ac, char **av)
 
 	run_ss_db_test(singlestep_basic);
 	run_ss_db_test(singlestep_emulated_instructions);
+	run_ss_db_test(singlestep_with_sti_blocking);
+	run_ss_db_test(singlestep_with_movss_blocking);
+	run_ss_db_test(singlestep_with_movss_blocking_and_icebp);
+	run_ss_db_test(singlestep_with_movss_blocking_and_dr7_gd);
 
 	n = 0;
 	write_dr1((void *)&value);
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test
  2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
                   ` (5 preceding siblings ...)
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 6/7] x86/debug: Add single-step #DB + STI/MOVSS blocking tests Sean Christopherson
@ 2022-01-20  0:29 ` Sean Christopherson
  2022-01-20  0:47   ` Jim Mattson
  6 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-01-20  0:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, Sean Christopherson, David Woodhouse, Alexander Graf

Explicitly set DR6.BS for the sub-test that verifies DR6.BS isn't cleared
when a data breakpoint (a.k.a. H/W watchpoint) #DB occurs.  Relying on
the single-step #DB tests to leave DR6 is all kinds of mean.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/debug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/x86/debug.c b/x86/debug.c
index 0165dc68..4fec241f 100644
--- a/x86/debug.c
+++ b/x86/debug.c
@@ -417,6 +417,7 @@ int main(int ac, char **av)
 
 	n = 0;
 	write_dr1((void *)&value);
+	write_dr6(DR6_BS);
 	write_dr7(0x00d0040a); // 4-byte write
 
 	extern unsigned char hw_wp1;
-- 
2.34.1.703.g22d0c6ccf7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test Sean Christopherson
@ 2022-01-20  0:47   ` Jim Mattson
  0 siblings, 0 replies; 10+ messages in thread
From: Jim Mattson @ 2022-01-20  0:47 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, David Woodhouse, Alexander Graf

On Wed, Jan 19, 2022 at 4:30 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Explicitly set DR6.BS for the sub-test that verifies DR6.BS isn't cleared
> when a data breakpoint (a.k.a. H/W watchpoint) #DB occurs.  Relying on
> the single-step #DB tests to leave DR6 is all kinds of mean.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test
  2022-01-20  0:29 ` [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test Sean Christopherson
@ 2022-01-24 14:15   ` Paolo Bonzini
  0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2022-01-24 14:15 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, David Woodhouse, Alexander Graf

On 1/20/22 01:29, Sean Christopherson wrote:
> @@ -153,8 +152,7 @@ static unsigned long singlestep_emulated_instructions(void)
>   		"1:push %%rax\n\t"
>   		"xor %%rax,%%rax\n\t"
>   		"cpuid\n\t"
> -		"movl $0x1a0,%%ecx\n\t"
> -		"rdmsr\n\t"
> +		"out %%eax, $0x80\n\t"
>   		"popf\n\t"
>   		"lea 1b,%0\n\t"
>   		: "=r" (start) : : "rax", "ebx", "ecx", "edx"

This is a bit more "dangerous" if the tests are run on bare metal. 
Let's replace it with a

	movl $0x3fd, %%edx
	in %%edx, %%al

Queued with this change, thanks.

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-01-24 14:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-20  0:29 [kvm-unit-tests PATCH 0/7] x86/debug: More single-step #DB tests Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 1/7] bitops: Include stdbool.h and stddef.h as necessary Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 2/7] x86/debug: Add framework for single-step #DB tests Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 3/7] x86/debug: Test OUT instead of RDMSR for single-step #DB emulation test Sean Christopherson
2022-01-24 14:15   ` Paolo Bonzini
2022-01-20  0:29 ` [kvm-unit-tests PATCH 4/7] x86/debug: Run single-step #DB tests in usermode (and kernel mode) Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 5/7] x86: Overhaul definitions for DR6 and DR7 bits Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 6/7] x86/debug: Add single-step #DB + STI/MOVSS blocking tests Sean Christopherson
2022-01-20  0:29 ` [kvm-unit-tests PATCH 7/7] x86/debug: Explicitly write DR6 in the H/W watchpoint + DR6.BS sub-test Sean Christopherson
2022-01-20  0:47   ` Jim Mattson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.