[PATCH v2 5/8] x86: Don't assume #DB is always caused by singlestep if EFLAGS.TF is set

[Jinoh Kang <jinoh.kang.kr@xxxxxxxxx>]

Today, when a HVM (or PVH) guest triggers a hardware breakpoint while
EFLAGS.TF is set, Xen incorrectly assumes that this is a single stepping
exception and sets X86_DR6_BS in dr6 in addition to X86_DR6_B<n>.

This causes problems with Linux HW breakpoint handler, which ignores
X86_DR6_B<n> bits when X86_DR6_BS is set.  This prevents user-mode
debuggers from recognizing hardware breakpoints if EFLAGS.TF is set.

Fix this by not setting X86_DR6_BS in {vmx,svm}_inject_event, unless the
emulator explicitly signals the single-stepping mode via the
'pending_dbg' field of struct x86_event.

While we're at it, defer setting guest DR6 from vmx_vmexit_handler() to
vmx_inject_event() on Intel hardware.  This gives the monitor a chance
to modify the pending_dbg flags before it is applied to guest DR6.

Fixes: 8b831f4189 ("x86: single step after instruction emulation")
Signed-off-by: Jinoh Kang <jinoh.kang.kr@xxxxxxxxx>
---
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wl@xxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
CC: Kevin Tian <kevin.tian@xxxxxxxxx>
CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
CC: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
CC: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>

v1 -> v2: new patch

The next patch in series adds the explanation for DR6 setting behavior
in the form of comments.  These comments are from Andrew Cooper's patch
"x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to
{svm,vmx}_inject_event()", which I split out because I was unsure about
how to handle authorship.  The comments are reproduced below:

> On AMD hardware, a #DB exception:
>  1) Merges new status bits into %dr6
>  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
>
> Item 1 is done by hardware before a #DB intercepted vmexit, but we
> may end up here from monitor so have to repeat it ourselves.
> Item 2 is done by hardware when injecting a #DB exception.

> On Intel hardware, a #DB exception:
>  1) Merges new status bits into %dr6
>  2) Clears %dr7.gd and MSR_DEBUGCTL.LBR
>
> All actions are left up to the hypervisor to perform.
---
 xen/arch/x86/hvm/svm/svm.c |  8 +++-----
 xen/arch/x86/hvm/vmx/vmx.c | 14 +++-----------
 2 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 038c8d6e7e..6f3e6b3512 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1328,11 +1328,9 @@ static void cf_check svm_inject_event(const struct 
x86_event *event)
     switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
     case X86_EXC_DB:
-        if ( regs->eflags & X86_EFLAGS_TF )
-        {
-            __restore_debug_registers(vmcb, curr);
-            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
-        }
+        __restore_debug_registers(vmcb, curr);
+        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | _event.pending_dbg);
+
         /* fall through */
     case X86_EXC_BP:
         if ( curr->domain->debugger_attached )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9b59374258..4e20fca43e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2022,11 +2022,9 @@ static void cf_check vmx_inject_event(const struct 
x86_event *event)
     switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
     {
     case X86_EXC_DB:
-        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
-        {
-            __restore_debug_registers(curr);
-            write_debugreg(6, read_debugreg(6) | DR_STEP);
-        }
+        __restore_debug_registers(curr);
+        write_debugreg(6, read_debugreg(6) | event->pending_dbg);
+
         if ( !nestedhvm_vcpu_in_guestmode(curr) ||
              !nvmx_intercepts_exception(curr, X86_EXC_DB, _event.error_code) )
         {
@@ -4250,14 +4248,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         switch ( vector )
         {
         case X86_EXC_DB:
-            /*
-             * Updates DR6 where debugger can peek (See 3B 23.2.1,
-             * Table 23-1, "Exit Qualification for Debug Exceptions").
-             */
             __vmread(EXIT_QUALIFICATION, &exit_qualification);
             HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
-            __restore_debug_registers(v);
-            write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
 
             /*
              * Work around SingleStep + STI/MovSS VMEntry failures.
-- 
2.41.0