Re: [Xen-devel] Single step in HVM domU on Intel machine may see wrong DB6

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Juergen Gross wrote on 2014-02-21:
> On 21.02.2014 02:26, Zhang, Yang Z wrote:
>> Juergen Gross wrote on 2014-02-20:
>>> Hi,
>> 
>> Hi, Juergen
>> 
>>> 
>>> I think I've found a bug in debug trap handling in the Xen hypervisor
>>> in case of a HVM domu using single stepping:
>>> 
>>> Debug registers are restored on vcpu switch only if db7 has any debug
>>> events activated or if the debug registers are marked to be used by
>>> the domU. This leads to problems if the domU uses single stepping and
>>> vcpu switch occurs between the single step trap and reading of db6 in
>>> the guest. db6 contents (single step indicator) are lost in this case.
>>> 
>>> Jan suggested to intercept the debug trap in the hypervisor and mark
>>> the debug registers to be used by the domU to enable saving and
>>> restoring the debug registers in case of a context switch. I used the
>>> attached patch (applies to Xen 4.2.3) to verify this solution and it
>>> worked (without the patch a test was able to reproduce the bug once in
>>> about 3 hours, with the patch the test ran for more than 12 hours
>>> without problem).
>>> 
>>> Obviously the patch isn't the final one, as I deactivated the "monitor
>>> trap flag" feature to avoid any strange dependencies. Jan wanted
>>> someone from the VMX folks to put together a proper fix to avoid
>>> overlooking some corner case.
>>> 
>> 
>> Thanks for reporting this issue.
>> Actually, I don't know the scenario that you saw this issue. Are you using
> single step inside guest? Or running gdb to debug VM remotely?
> 
> Single step inside guest:
> 
> 1. Guest sets TF flag in status loaded by IRET and does IRET
> 2. Debug trap in guest occurs, physical DB6 holds single step indicator
> 3. vcpu scheduling event occurs, debug registers are NOT saved as not marked
>     being dirty and DB7 has no debug events configured
> 4. when guest vcpu is scheduled again, DB6 has lost single step indicator

How about the following patch. It is not tested because I don't have the 
environment.
After setting trap_debug in guest exception bitmap, the vmexit for trap_debug 
is not only used by gdb, but also will used by guest itself. In case of such 
vmexit, we need to restore the debug register and inject a trap exception into 
guest.

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b128e81..113a313 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1171,8 +1171,6 @@ void vmx_update_debug_state(struct vcpu *v)
     unsigned long mask;
 
     mask = 1u << TRAP_int3;
-    if ( !cpu_has_monitor_trap_flag )
-        mask |= 1u << TRAP_debug;
 
     if ( v->arch.hvm_vcpu.debug_state_latch )
         v->arch.hvm_vmx.exception_bitmap |= mask;
@@ -2690,9 +2688,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             __vmread(EXIT_QUALIFICATION, &exit_qualification);
             HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
             write_debugreg(6, exit_qualification | 0xffff0ff0);
-            if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
-                goto exit_and_crash;
-            domain_pause_for_debugger();
+            if ( v->domain->debugger_attached )
+                domain_pause_for_debugger();
+            else 
+            {
+                __restore_debug_registers(v);
+                hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
+            }
             break;
         case TRAP_int3: 
         {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index dcc3483..0d76d8c 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -378,7 +378,8 @@ static inline int hvm_event_pending(struct vcpu *v)
         (cpu_has_xsave ? X86_CR4_OSXSAVE : 0))))
 
 /* These exceptions must always be intercepted. */
-#define HVM_TRAP_MASK ((1U << TRAP_machine_check) | (1U << TRAP_invalid_op))
+#define HVM_TRAP_MASK ((1U << TRAP_machine_check) | (1U << TRAP_invalid_op) |\
+                       (1U << TRAP_debug))
 
 /*
  * x86 event types. This enumeration is valid for:


BTW: I also think we should clear the CPU_BASED_MOV_DR_EXITING bit in 
__restore_debug_registers(). After restore the debug register, we should not 
trap any DR access unless the VCPU is scheduled out again. Not sure whether I 
am wrong.

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b128e81..56a3140 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -394,6 +394,8 @@ static void __restore_debug_registers(struct vcpu *v)
     write_debugreg(3, v->arch.debugreg[3]);
     write_debugreg(6, v->arch.debugreg[6]);
     /* DR7 is loaded from the VMCS. */
+    v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MOV_DR_EXITING;
+    vmx_update_cpu_exec_control(v);
 }
 
 /*

> 
> 
> Juergen
>


Best regards,
Yang



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel