Re: [PATCH 4/7] KVM: x86: Consolidate KVM_GUESTDBG_SINGLESTEP check into the kvm_inject_emulated_db()

[Hou Wenlong]

On Thu, Dec 11, 2025 at 09:19:39AM -0800, Sean Christopherson wrote:
> On Thu, Dec 11, 2025, Hou Wenlong wrote:
> > On Fri, Dec 05, 2025 at 09:58:04AM -0800, Sean Christopherson wrote:
> > > But I think the WARN will be subject to false positives.  KVM doesn't emulate data
> > > #DBs, but it does emulate code #DBs, and fault-like code #DBs can be coincident
> > > with trap-like single-step #DBs.  Ah, but kvm_vcpu_check_code_breakpoint() doesn't
> > > account for RFLAGS.TF.  That should probably be addressed in this series, especially
> > > since it's consolidating KVM_GUESTDBG_SINGLESTEP handling.
> >
> > Sorry, I didn't follow it, how fault-like code #DBs can be coincident
> > with trap-like single-step #DBs, could you provide an example?
> 
> Ya, here's a KUT testcase that applies on top of
> https://lore.kernel.org/all/20251126191736.907963-1-seanjc@xxxxxxxxxx.
>
Thanks for your testcase; it really changed my perspective.
 
> ---
>  x86/debug.c | 43 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/x86/debug.c b/x86/debug.c
> index 8177575c..313d854e 100644
> --- a/x86/debug.c
> +++ b/x86/debug.c
> @@ -92,6 +92,7 @@ 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 unsigned long singlestep_with_code_db(void);
>  static unsigned long singlestep_with_sti_hlt(void);
>  
>  static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
> @@ -106,11 +107,12 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn)
>  	report_fn(start, "");
>  
>  	/*
> -	 * MOV DR #GPs at CPL>0, don't try to run the DR7.GD test in usermode.
> -	 * Likewise for HLT.
> +	 * MOV DR #GPs at CPL>0, don't try to run the DR7.GD or code #DB tests
> +	 * in usermode. Likewise for HLT.
>  	 */
> -	if (test == singlestep_with_movss_blocking_and_dr7_gd
> -	    || test == singlestep_with_sti_hlt)
> +	if (test == singlestep_with_movss_blocking_and_dr7_gd ||
> +	    test == singlestep_with_code_db ||
> +	    test == singlestep_with_sti_hlt)
>  		return;
>  
>  	n = 0;
> @@ -163,6 +165,38 @@ static noinline unsigned long singlestep_basic(void)
>  	return start;
>  }
>  
> +static void report_singlestep_with_code_db(unsigned long start, const char *usermode)
> +{
> +	report(n == 3 &&
> +	       dr6[0] == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP2) && 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 + code #DB test", usermode);
> +}
> +
> +static noinline unsigned long singlestep_with_code_db(void)
> +{
> +	unsigned long start;
> +
> +	asm volatile (
> +		"lea 1f(%%rip), %0\n\t"
> +		"mov %0, %%dr2\n\t"
> +		"mov $" xstr(DR7_FIXED_1 | DR7_EXECUTE_DRx(2) | DR7_GLOBAL_ENABLE_DR2) ", %0\n\t"
> +		"mov %0, %%dr7\n\t"
> +		"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"
In my previous understanding, I thought there would be two #DBs
generated at the instruction boundary. First, the single-step trap #DB
would be handled, and then, when resuming to start the new instruction,
it would check for the code breakpoint and generate a code fault #DB.
However, it turns out that the check for the code breakpoint happened
before the instruction boundary. I also see in the kernel hardware
breakpoint handler that it notes that code breakpoints and single-step
can be detected together. Is this due to instruction prefetch?

If we want to emulate the hardware behavior in the emulator, does that
mean we need to check for code breakpoints in kvm_vcpu_do_single_step()
and set the DR_TRAP_BITS along with the DR6_BS bit?

Thanks!

> +		"1:push %%rax\n\t"
> +		"popf\n\t"
> +		"lea 1b(%%rip), %0\n\t"
> +		: "=r" (start) : : "rax"
> +	);
> +	return start;
> +}
> +
>  static void report_singlestep_emulated_instructions(unsigned long start,
>  						    const char *usermode)
>  {
> @@ -517,6 +551,7 @@ int main(int ac, char **av)
>  	       n, db_addr[0], dr6[0]);
>  
>  	run_ss_db_test(singlestep_basic);
> +	run_ss_db_test(singlestep_with_code_db);
>  	run_ss_db_test(singlestep_emulated_instructions);
>  	run_ss_db_test(singlestep_with_sti_blocking);
>  	run_ss_db_test(singlestep_with_movss_blocking);
> 
> base-commit: 23071a886edbe303fb964c5c386750b0b458dbfb
> --