Single step in HVM domU on Intel machine may see wrong DB6

Single step in HVM domU on Intel machine may see wrong DB6

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

Hi,

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.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

[-- Attachment #2: single-step.patch --]
[-- Type: text/x-patch, Size: 1927 bytes --]

--- xen-4.2.3-testing.orig/xen/include/asm-x86/hvm/hvm.h	2014-02-14 19:05:59.000000000 +0100
+++ xen-4.2.3-testing/xen/include/asm-x86/hvm/hvm.h	2014-02-17 07:43:05.000000000 +0100
@@ -374,7 +374,8 @@ static inline int hvm_do_pmu_interrupt(s
         (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) |\
+	(1 << TRAP_debug))
 
 /*
  * x86 event types. This enumeration is valid for:
--- xen-4.2.3-testing.orig/xen/arch/x86/hvm/vmx/vmcs.c	2014-02-17 07:48:43.000000000 +0100
+++ xen-4.2.3-testing/xen/arch/x86/hvm/vmx/vmcs.c	2014-02-17 10:16:25.000000000 +0100
@@ -168,7 +168,7 @@ static int vmx_init_vmcs_config(void)
            CPU_BASED_RDTSC_EXITING);
     opt = (CPU_BASED_ACTIVATE_MSR_BITMAP |
            CPU_BASED_TPR_SHADOW |
-           CPU_BASED_MONITOR_TRAP_FLAG |
+           /* CPU_BASED_MONITOR_TRAP_FLAG | */
            CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
     _vmx_cpu_based_exec_control = adjust_vmx_controls(
         "CPU-Based Exec Control", min, opt,
--- xen-4.2.3-testing.orig/xen/arch/x86/hvm/vmx/vmx.c	2014-02-18 08:04:23.000000000 +0100
+++ xen-4.2.3-testing/xen/arch/x86/hvm/vmx/vmx.c	2014-02-18 10:45:42.000000000 +0100
@@ -2646,7 +2646,11 @@ void vmx_vmexit_handler(struct cpu_user_
             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;
+            {
+                __restore_debug_registers(v);
+                hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
+                break;
+            }
             domain_pause_for_debugger();
             break;
         case TRAP_int3: 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

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?

> 
> Juergen
>


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Juergen Gross]

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


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

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

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Jan Beulich]

>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> @@ -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);
> +            }

I suppose you need to set DR6.BS after restoring the reigsters?

Also, the change looks rather simple - is that really correct for both
cpu_has_monitor_trap_flag and !cpu_has_monitor_trap_flag cases?

> 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);
>  }
>  
>  /*

That's being done by at least one of its callers (vmx_dr_access())
already, and I think it was purposefully not done in other cases.

Jan

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Jan Beulich wrote on 2014-02-27:
>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> @@ -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); +      
>>      }
> 
> I suppose you need to set DR6.BS after restoring the reigsters?

Right but is not enough. If flag_dr_dirty is set, we need to restore register from hardware. Conversely, restore is from debugreg and set DR6 to exit_qualification.

> 
> Also, the change looks rather simple - is that really correct for both
> cpu_has_monitor_trap_flag and !cpu_has_monitor_trap_flag cases?
> 

Until now, MTF is used for single step by gdb. And with MTF enabled, it should never goto here. So those changes should not impact the MTF. Also, I used gdb to debug guest with using MTF approach and applied this patch. It seems everything is working well.
But still need more testing from Juergen.

>> 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);
>>  }
>>  
>>  /*
> 
> That's being done by at least one of its callers (vmx_dr_access())
> already, and I think it was purposefully not done in other cases.

Yes, the question is why only one caller does this. Per my understanding, after restoring debug register, hypervisor should allow the guest to access them unless the VCPU is scheduled out again and vmx_save_dr is called.
Take the injecting debug_trap to guest as example, hypervisor will restore the debug register to hardware. After returning to guest, we should allow guest to read DR without trap again. But now, it still causes DR access vmexit and hypervisor restores it again. Then it clear the DR exiting bit in this vmexit handler. Are this behavior we expecting?

> 
> Jan


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Jan Beulich]

>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-02-27:
>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> @@ -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); +      
>>>      }
>> 
>> I suppose you need to set DR6.BS after restoring the reigsters?
> 
> Right but is not enough. If flag_dr_dirty is set, we need to restore 
> register from hardware. Conversely, restore is from debugreg and set DR6 to 
> exit_qualification.

After some more thought, I in fact doubt that restoring the debug
registers is in line with the current model: We should simply set
DR6.BS in the in-memory copy when the debug registers aren't
live yet (and it doesn't hurt to always do that). And since DR6
bits generally are sticky, I think exit_qualification actually needs
to be or-ed into the in-memory copy.

And presumably we would be better off if we dropped the
interception of TRAP_debug once we restored the debug
registers.

Jan

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Jan Beulich wrote on 2014-02-27:
>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-02-27:
>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com>
> wrote:
>>>> @@ -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); +
>>>>      }
>>> 
>>> I suppose you need to set DR6.BS after restoring the reigsters?
>> 
>> Right but is not enough. If flag_dr_dirty is set, we need to restore
>> register from hardware. Conversely, restore is from debugreg and set
>> DR6 to exit_qualification.
> 
> After some more thought, I in fact doubt that restoring the debug
> registers is in line with the current model: We should simply set
> DR6.BS in the in-memory copy when the debug registers aren't live yet
> (and it doesn't hurt to always do that). And since DR6 bits generally
> are sticky, I think exit_qualification actually needs to be or-ed into the in-memory copy.

Will guest be confused to see the DR6.BS always set?

> 
> And presumably we would be better off if we dropped the interception
> of TRAP_debug once we restored the debug registers.

Yes, it's unnecessary to trap into hypervisor always. Also, if we set DR6.BS always, I guess there is no need to intercept the TRAP_debug.

> 
> Jan


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Juergen Gross]

On 05.03.2014 03:22, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-02-27:
>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-02-27:
>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>> wrote:
>>>>> @@ -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); +
>>>>>       }
>>>>
>>>> I suppose you need to set DR6.BS after restoring the reigsters?
>>>
>>> Right but is not enough. If flag_dr_dirty is set, we need to restore
>>> register from hardware. Conversely, restore is from debugreg and set
>>> DR6 to exit_qualification.
>>
>> After some more thought, I in fact doubt that restoring the debug
>> registers is in line with the current model: We should simply set
>> DR6.BS in the in-memory copy when the debug registers aren't live yet
>> (and it doesn't hurt to always do that). And since DR6 bits generally
>> are sticky, I think exit_qualification actually needs to be or-ed into the in-memory copy.
>
> Will guest be confused to see the DR6.BS always set?

You can't set DR6.BS unconditionally! This bit should be set only in case
of a debug trap caused by single stepping, of course!

At least our BS2000 domU will crash in case of an unmotivated DR6.BS in debug
trap handling.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Jan Beulich]

>>> On 05.03.14 at 03:22, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-02-27:
>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-02-27:
>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>> wrote:
>>>>> @@ -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); +
>>>>>      }
>>>> 
>>>> I suppose you need to set DR6.BS after restoring the reigsters?
>>> 
>>> Right but is not enough. If flag_dr_dirty is set, we need to restore
>>> register from hardware. Conversely, restore is from debugreg and set
>>> DR6 to exit_qualification.
>> 
>> After some more thought, I in fact doubt that restoring the debug
>> registers is in line with the current model: We should simply set
>> DR6.BS in the in-memory copy when the debug registers aren't live yet
>> (and it doesn't hurt to always do that). And since DR6 bits generally
>> are sticky, I think exit_qualification actually needs to be or-ed into the 
> in-memory copy.
> 
> Will guest be confused to see the DR6.BS always set?

It certainly shouldn't when single stepping. And as long as the guest
clears the bit while handling the single step trap, it won't see it set on
other kinds of #DB. After all that's how hardware behaves.

>> And presumably we would be better off if we dropped the interception
>> of TRAP_debug once we restored the debug registers.
> 
> Yes, it's unnecessary to trap into hypervisor always. Also, if we set DR6.BS 
> always, I guess there is no need to intercept the TRAP_debug.

Oh, perhaps you misunderstood then: I didn't suggest to always
set that flag. What I suggested is that during the intercepted
TRAP_debug we should or exit_qualification (which ought to
have BS set when single stepping with no other breakpoint
enabled in DR7) into the in-memory copy of DR6. Once the
intercept got dropped (as also suggested), hardware would again
take care of setting DR6 correctly.

Jan

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Jan Beulich wrote on 2014-03-05:
>>>> On 05.03.14 at 03:22, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-02-27:
>>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com>
> wrote:
>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>>> wrote:
>>>>>> @@ -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); +
>>>>>>      }
>>>>> 
>>>>> I suppose you need to set DR6.BS after restoring the reigsters?
>>>> 
>>>> Right but is not enough. If flag_dr_dirty is set, we need to
>>>> restore register from hardware. Conversely, restore is from
>>>> debugreg and set
>>>> DR6 to exit_qualification.
>>> 
>>> After some more thought, I in fact doubt that restoring the debug
>>> registers is in line with the current model: We should simply set
>>> DR6.BS in the in-memory copy when the debug registers aren't live
>>> yet (and it doesn't hurt to always do that). And since DR6 bits
>>> generally are sticky, I think exit_qualification actually needs to
>>> be or-ed into the
>> in-memory copy.
>> 
>> Will guest be confused to see the DR6.BS always set?
> 
> It certainly shouldn't when single stepping. And as long as the guest
> clears the bit while handling the single step trap, it won't see it set on other kinds of #DB.
> After all that's how hardware behaves.
> 
>>> And presumably we would be better off if we dropped the
>>> interception of TRAP_debug once we restored the debug registers.
>> 
>> Yes, it's unnecessary to trap into hypervisor always. Also, if we
>> set DR6.BS always, I guess there is no need to intercept the TRAP_debug.
> 
> Oh, perhaps you misunderstood then: I didn't suggest to always set that flag.
> What I suggested is that during the intercepted TRAP_debug we should
> or exit_qualification (which ought to have BS set when single stepping
> with no other breakpoint enabled in DR7) into the in-memory copy of
> DR6. Once the intercept got dropped (as also suggested), hardware
> would again take care of setting DR6 correctly.

Oh, I am sorry, I misunderstand you.
How about the following changes: Intercept the TRAP_debug when schedule out and drop it after restoring VCPU's DR register into hardware.

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b128e81..5784dd1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
     v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
     vmx_update_cpu_exec_control(v);
 
+    /* Trap debug for signle stepping. */
+    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
+    vmx_update_exception_bitmap(v);
+
     v->arch.debugreg[0] = read_debugreg(0);
     v->arch.debugreg[1] = read_debugreg(1);
     v->arch.debugreg[2] = read_debugreg(2);
@@ -388,6 +392,13 @@ static void __restore_debug_registers(struct vcpu *v)
 
     v->arch.hvm_vcpu.flag_dr_dirty = 1;
 
+    /* 
+     * After restore, hardware has the right context.
+     * So no need to trap debug anymore.
+     */
+    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);
+    vmx_update_exception_bitmap(v);
+
     write_debugreg(0, v->arch.debugreg[0]);
     write_debugreg(1, v->arch.debugreg[1]);
     write_debugreg(2, v->arch.debugreg[2]);
@@ -1171,8 +1182,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;
@@ -2689,10 +2698,18 @@ 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();
+            exit_qualification |= 0xffff0ff0;
+            if ( v->domain->debugger_attached )
+            {
+                write_debugreg(6, exit_qualification);
+                domain_pause_for_debugger();
+            }
+            else 
+            {
+                __restore_debug_registers(v);
+                write_debugreg(6, exit_qualification | read_debugreg(6));
+                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:



Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Jan Beulich]

>>> On 07.03.14 at 06:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> How about the following changes: Intercept the TRAP_debug when schedule out 
> and drop it after restoring VCPU's DR register into hardware.

Along those lines at least.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
>      v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
>      vmx_update_cpu_exec_control(v);
>  
> +    /* Trap debug for signle stepping. */
> +    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
> +    vmx_update_exception_bitmap(v);
> +
>      v->arch.debugreg[0] = read_debugreg(0);
>      v->arch.debugreg[1] = read_debugreg(1);
>      v->arch.debugreg[2] = read_debugreg(2);
> @@ -388,6 +392,13 @@ static void __restore_debug_registers(struct vcpu *v)
>  
>      v->arch.hvm_vcpu.flag_dr_dirty = 1;
>  
> +    /* 
> +     * After restore, hardware has the right context.
> +     * So no need to trap debug anymore.
> +     */
> +    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);

&=

> +    vmx_update_exception_bitmap(v);
> +
>      write_debugreg(0, v->arch.debugreg[0]);
>      write_debugreg(1, v->arch.debugreg[1]);
>      write_debugreg(2, v->arch.debugreg[2]);
> @@ -1171,8 +1182,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;

If the intercept now really becomes independent of MTF
availability, then I guess this code should be further cleaned up
("mask" becoming pointless as a separate variable).

> @@ -2689,10 +2698,18 @@ 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();
> +            exit_qualification |= 0xffff0ff0;

Is this really needed?

> +            if ( v->domain->debugger_attached )
> +            {
> +                write_debugreg(6, exit_qualification);
> +                domain_pause_for_debugger();
> +            }
> +            else 
> +            {
> +                __restore_debug_registers(v);
> +                write_debugreg(6, exit_qualification | read_debugreg(6));

I still wonder whether it wouldn't be more efficient to simply or
exit_qualification into v->arch.debugreg[6] before calling
__restore_debug_registers().

Jan

> +                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:
> 
> 
> 
> Best regards,
> Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Jan Beulich wrote on 2014-03-07:
>>>> On 07.03.14 at 06:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> How about the following changes: Intercept the TRAP_debug when schedule
>> out and drop it after restoring VCPU's DR register into hardware.
> 
> Along those lines at least.
> 
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
>>      v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
>>      vmx_update_cpu_exec_control(v);
>> +    /* Trap debug for signle stepping. */
>> +    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
>> +    vmx_update_exception_bitmap(v);
>> +
>>      v->arch.debugreg[0] = read_debugreg(0);
>>      v->arch.debugreg[1] = read_debugreg(1);
>>      v->arch.debugreg[2] = read_debugreg(2);
>> @@ -388,6 +392,13 @@ static void __restore_debug_registers(struct vcpu
>> *v)
>> 
>>      v->arch.hvm_vcpu.flag_dr_dirty = 1;
>> +    /*
>> +     * After restore, hardware has the right context.
>> +     * So no need to trap debug anymore.
>> +     */
>> +    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);
> 
> &=
> 
>> +    vmx_update_exception_bitmap(v);
>> +
>>      write_debugreg(0, v->arch.debugreg[0]); write_debugreg(1,
>>      v->arch.debugreg[1]); write_debugreg(2, v->arch.debugreg[2]); @@
>>      -1171,8 +1182,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;
> 
> If the intercept now really becomes independent of MTF
> availability, then I guess this code should be further cleaned up
> ("mask" becoming pointless as a separate variable).
> 

Yes, I will clean up it when sending the formal patch.

>> @@ -2689,10 +2698,18 @@ 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(); +            exit_qualification |=
>> 0xffff0ff0;
> 
> Is this really needed?

Yes. The reserved bits need to set to 1 in DB6. But it is cleared in exit_qualification.

> 
>> +            if ( v->domain->debugger_attached )
>> +            {
>> +                write_debugreg(6, exit_qualification);
>> +                domain_pause_for_debugger();
>> +            }
>> +            else
>> +            {
>> +                __restore_debug_registers(v);
>> +                write_debugreg(6, exit_qualification |
> read_debugreg(6));
> 
> I still wonder whether it wouldn't be more efficient to simply or
> exit_qualification into v->arch.debugreg[6] before calling
> __restore_debug_registers().
> 

__restore_debug_registers() only copy the v->arch.debugreg[6] into hardware DB6 when flag_dr_dirty is cleared. So as I mentioned before, the hardware DB register will hold the latest value if flag_dr_dirty is set and we should write hardware DB6 directly.

> Jan
> 
>> +                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:
>> 
>> 
>> Best regards,
>> Yang
> 
>


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Jan Beulich]

>>> On 11.03.14 at 03:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-03-07:
>>>>> On 07.03.14 at 06:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> @@ -2689,10 +2698,18 @@ 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(); +            exit_qualification |=
>>> 0xffff0ff0;
>> 
>> Is this really needed?
> 
> Yes. The reserved bits need to set to 1 in DB6. But it is cleared in 
> exit_qualification.

In which case I'd strongly suggest adding a respective #define to
debugreg.h and using it here.

>>> +            if ( v->domain->debugger_attached )
>>> +            {
>>> +                write_debugreg(6, exit_qualification);
>>> +                domain_pause_for_debugger();
>>> +            }
>>> +            else
>>> +            {
>>> +                __restore_debug_registers(v);
>>> +                write_debugreg(6, exit_qualification |
>> read_debugreg(6));
>> 
>> I still wonder whether it wouldn't be more efficient to simply or
>> exit_qualification into v->arch.debugreg[6] before calling
>> __restore_debug_registers().
>> 
> 
> __restore_debug_registers() only copy the v->arch.debugreg[6] into hardware 
> DB6 when flag_dr_dirty is cleared. So as I mentioned before, the hardware DB 
> register will hold the latest value if flag_dr_dirty is set and we should 
> write hardware DB6 directly.

Except that, as per the earlier discussion, the intercept should not
occur when flag_dr_dirty is set, as it ought to have got disabled
when setting the flag while restoring debug registers.

Jan

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Jan Beulich wrote on 2014-03-11:
>>>> On 11.03.14 at 03:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-03-07:
>>>>>> On 07.03.14 at 06:10, "Zhang, Yang Z" <yang.z.zhang@intel.com>
> wrote:
>>>> @@ -2689,10 +2698,18 @@ 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(); +            exit_qualification |=
>>>> 0xffff0ff0;
>>> 
>>> Is this really needed?
>> 
>> Yes. The reserved bits need to set to 1 in DB6. But it is cleared in
>> exit_qualification.
> 
> In which case I'd strongly suggest adding a respective #define to
> debugreg.h and using it here.

OK.

> 
>>>> +            if ( v->domain->debugger_attached )
>>>> +            {
>>>> +                write_debugreg(6, exit_qualification);
>>>> +                domain_pause_for_debugger();
>>>> +            }
>>>> +            else
>>>> +            {
>>>> +                __restore_debug_registers(v);
>>>> +                write_debugreg(6, exit_qualification |
>>> read_debugreg(6));
>>> 
>>> I still wonder whether it wouldn't be more efficient to simply or
>>> exit_qualification into v->arch.debugreg[6] before calling
>>> __restore_debug_registers().
>>> 
>> 
>> __restore_debug_registers() only copy the v->arch.debugreg[6] into
>> hardware DB6 when flag_dr_dirty is cleared. So as I mentioned before,
>> the hardware DB register will hold the latest value if flag_dr_dirty is
>> set and we should write hardware DB6 directly.
> 
> Except that, as per the earlier discussion, the intercept should not
> occur when flag_dr_dirty is set, as it ought to have got disabled
> when setting the flag while restoring debug registers.
> 

Yes, I see your point.

> Jan


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2014-03-11:
> Jan Beulich wrote on 2014-03-07:
>>>>> On 07.03.14 at 06:10, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> How about the following changes: Intercept the TRAP_debug when
>>> schedule out and drop it after restoring VCPU's DR register into hardware.
>> 
>> Along those lines at least.
>> 
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
>>>      v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
>>>      vmx_update_cpu_exec_control(v);
>>> +    /* Trap debug for signle stepping. */
>>> +    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
>>> +    vmx_update_exception_bitmap(v);
>>> +
>>>      v->arch.debugreg[0] = read_debugreg(0);
>>>      v->arch.debugreg[1] = read_debugreg(1);
>>>      v->arch.debugreg[2] = read_debugreg(2); @@ -388,6 +392,13 @@
>>> static void __restore_debug_registers(struct vcpu
>>> *v)
>>> 
>>>      v->arch.hvm_vcpu.flag_dr_dirty = 1;
>>> +    /*
>>> +     * After restore, hardware has the right context.
>>> +     * So no need to trap debug anymore.
>>> +     */
>>> +    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);
>> 
>> &=
>> 
>>> +    vmx_update_exception_bitmap(v);
>>> +
>>>      write_debugreg(0, v->arch.debugreg[0]); write_debugreg(1,
>>>      v->arch.debugreg[1]); write_debugreg(2, v->arch.debugreg[2]); @@
>>>      -1171,8 +1182,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;
>> 
>> If the intercept now really becomes independent of MTF availability,
>> then I guess this code should be further cleaned up ("mask" becoming
>> pointless as a separate variable).
>> 
> 
> Yes, I will clean up it when sending the formal patch.
> 
>>> @@ -2689,10 +2698,18 @@ 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(); +            exit_qualification |=
>>> 0xffff0ff0;
>> 
>> Is this really needed?
> 
> Yes. The reserved bits need to set to 1 in DB6. But it is cleared in
> exit_qualification.
> 
>> 
>>> +            if ( v->domain->debugger_attached )
>>> +            {
>>> +                write_debugreg(6, exit_qualification);
>>> +                domain_pause_for_debugger();
>>> +            }
>>> +            else
>>> +            {
>>> +                __restore_debug_registers(v);
>>> +                write_debugreg(6, exit_qualification |
>> read_debugreg(6));
>> 
>> I still wonder whether it wouldn't be more efficient to simply or
>> exit_qualification into v->arch.debugreg[6] before calling
>> __restore_debug_registers().
>> 
> 
> __restore_debug_registers() only copy the v->arch.debugreg[6] into
> hardware DB6 when flag_dr_dirty is cleared. So as I mentioned before,
> the hardware DB register will hold the latest value if flag_dr_dirty is
> set and we should write hardware DB6 directly.

After more thoughts, in this version patch, we will not intercept the debug trap, so the above condition will never happen. Am I right?

> 
>> Jan
>> 
>>> +                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:
>>> 
>>> Best regards,
>>> Yang
>> 
>> 
> 
> 
> Best regards,
> Yang
>


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Juergen Gross]

Hi folks,

any chance to get this fixed?


Juergen

On 07.03.2014 06:10, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-03-05:
>>>>> On 05.03.14 at 03:22, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-02-27:
>>>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>> wrote:
>>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>>>> wrote:
>>>>>>> @@ -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); +
>>>>>>>       }
>>>>>>
>>>>>> I suppose you need to set DR6.BS after restoring the reigsters?
>>>>>
>>>>> Right but is not enough. If flag_dr_dirty is set, we need to
>>>>> restore register from hardware. Conversely, restore is from
>>>>> debugreg and set
>>>>> DR6 to exit_qualification.
>>>>
>>>> After some more thought, I in fact doubt that restoring the debug
>>>> registers is in line with the current model: We should simply set
>>>> DR6.BS in the in-memory copy when the debug registers aren't live
>>>> yet (and it doesn't hurt to always do that). And since DR6 bits
>>>> generally are sticky, I think exit_qualification actually needs to
>>>> be or-ed into the
>>> in-memory copy.
>>>
>>> Will guest be confused to see the DR6.BS always set?
>>
>> It certainly shouldn't when single stepping. And as long as the guest
>> clears the bit while handling the single step trap, it won't see it set on other kinds of #DB.
>> After all that's how hardware behaves.
>>
>>>> And presumably we would be better off if we dropped the
>>>> interception of TRAP_debug once we restored the debug registers.
>>>
>>> Yes, it's unnecessary to trap into hypervisor always. Also, if we
>>> set DR6.BS always, I guess there is no need to intercept the TRAP_debug.
>>
>> Oh, perhaps you misunderstood then: I didn't suggest to always set that flag.
>> What I suggested is that during the intercepted TRAP_debug we should
>> or exit_qualification (which ought to have BS set when single stepping
>> with no other breakpoint enabled in DR7) into the in-memory copy of
>> DR6. Once the intercept got dropped (as also suggested), hardware
>> would again take care of setting DR6 correctly.
>
> Oh, I am sorry, I misunderstand you.
> How about the following changes: Intercept the TRAP_debug when schedule out and drop it after restoring VCPU's DR register into hardware.
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index b128e81..5784dd1 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
>       v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
>       vmx_update_cpu_exec_control(v);
>
> +    /* Trap debug for signle stepping. */
> +    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
> +    vmx_update_exception_bitmap(v);
> +
>       v->arch.debugreg[0] = read_debugreg(0);
>       v->arch.debugreg[1] = read_debugreg(1);
>       v->arch.debugreg[2] = read_debugreg(2);
> @@ -388,6 +392,13 @@ static void __restore_debug_registers(struct vcpu *v)
>
>       v->arch.hvm_vcpu.flag_dr_dirty = 1;
>
> +    /*
> +     * After restore, hardware has the right context.
> +     * So no need to trap debug anymore.
> +     */
> +    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);
> +    vmx_update_exception_bitmap(v);
> +
>       write_debugreg(0, v->arch.debugreg[0]);
>       write_debugreg(1, v->arch.debugreg[1]);
>       write_debugreg(2, v->arch.debugreg[2]);
> @@ -1171,8 +1182,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;
> @@ -2689,10 +2698,18 @@ 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();
> +            exit_qualification |= 0xffff0ff0;
> +            if ( v->domain->debugger_attached )
> +            {
> +                write_debugreg(6, exit_qualification);
> +                domain_pause_for_debugger();
> +            }
> +            else
> +            {
> +                __restore_debug_registers(v);
> +                write_debugreg(6, exit_qualification | read_debugreg(6));
> +                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:
>
>
>
> Best regards,
> Yang
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>


-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Zhang, Yang Z]

Juergen Gross wrote on 2014-05-06:
> Hi folks,
> 
> any chance to get this fixed?

Hi Juegen, 

I am really sorry that I have forgotten to do this. I am too busy on other task recently. :(
I will provide a patch ASAP.

> 
> 
> Juergen
> 
> On 07.03.2014 06:10, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2014-03-05:
>>>>>> On 05.03.14 at 03:22, "Zhang, Yang Z" <yang.z.zhang@intel.com>
> wrote:
>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>>> wrote:
>>>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z"
>>>>>>>>>> <yang.z.zhang@intel.com>
>>>>> wrote:
>>>>>>>> @@ -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); +
>>>>>>>>       }
>>>>>>> 
>>>>>>> I suppose you need to set DR6.BS after restoring the reigsters?
>>>>>> 
>>>>>> Right but is not enough. If flag_dr_dirty is set, we need to
>>>>>> restore register from hardware. Conversely, restore is from
>>>>>> debugreg and set
>>>>>> DR6 to exit_qualification.
>>>>> 
>>>>> After some more thought, I in fact doubt that restoring the debug
>>>>> registers is in line with the current model: We should simply set
>>>>> DR6.BS in the in-memory copy when the debug registers aren't live
>>>>> yet (and it doesn't hurt to always do that). And since DR6 bits
>>>>> generally are sticky, I think exit_qualification actually needs
>>>>> to be or-ed into the
>>>> in-memory copy.
>>>> 
>>>> Will guest be confused to see the DR6.BS always set?
>>> 
>>> It certainly shouldn't when single stepping. And as long as the guest
>>> clears the bit while handling the single step trap, it won't see it
>>> set on other kinds of #DB. After all that's how hardware behaves.
>>> 
>>>>> And presumably we would be better off if we dropped the
>>>>> interception of TRAP_debug once we restored the debug registers.
>>>> 
>>>> Yes, it's unnecessary to trap into hypervisor always. Also, if we
>>>> set DR6.BS always, I guess there is no need to intercept the TRAP_debug.
>>> 
>>> Oh, perhaps you misunderstood then: I didn't suggest to always set
>>> that flag. What I suggested is that during the intercepted TRAP_debug
>>> we should or exit_qualification (which ought to have BS set when
>>> single stepping with no other breakpoint enabled in DR7) into the
>>> in-memory copy of DR6. Once the intercept got dropped (as also
>>> suggested), hardware would again take care of setting DR6 correctly.
>> 
>> Oh, I am sorry, I misunderstand you. How about the following changes:
>> Intercept the TRAP_debug when schedule out and drop it after restoring
>> VCPU's DR register into hardware.
>> 
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index b128e81..5784dd1 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -372,6 +372,10 @@ static void vmx_save_dr(struct vcpu *v)
>>       v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
>>       vmx_update_cpu_exec_control(v);
>> +    /* Trap debug for signle stepping. */
>> +    v->arch.hvm_vmx.exception_bitmap |= 1 << TRAP_debug;
>> +    vmx_update_exception_bitmap(v);
>> +
>>       v->arch.debugreg[0] = read_debugreg(0);
>>       v->arch.debugreg[1] = read_debugreg(1);
>>       v->arch.debugreg[2] = read_debugreg(2); @@ -388,6 +392,13 @@
>> static void __restore_debug_registers(struct vcpu *v)
>> 
>>       v->arch.hvm_vcpu.flag_dr_dirty = 1;
>> +    /*
>> +     * After restore, hardware has the right context.
>> +     * So no need to trap debug anymore.
>> +     */
>> +    v->arch.hvm_vmx.exception_bitmap |= ~(1 << TRAP_debug);
>> +    vmx_update_exception_bitmap(v);
>> +
>>       write_debugreg(0, v->arch.debugreg[0]); write_debugreg(1,
>>       v->arch.debugreg[1]); write_debugreg(2, v->arch.debugreg[2]); @@
>>       -1171,8 +1182,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; @@ -2689,10
>> +2698,18 @@ 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(); +            exit_qualification |=
>> 0xffff0ff0; +            if ( v->domain->debugger_attached ) +         
>>   { +                write_debugreg(6, exit_qualification); +          
>>      domain_pause_for_debugger(); +            } +            else +   
>>         { +                __restore_debug_registers(v); +             
>>   write_debugreg(6, exit_qualification | read_debugreg(6)); +          
>>      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:
>> 
>> 
>> Best regards,
>> Yang
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>> 
>> 
> 
>


Best regards,
Yang

Re: Single step in HVM domU on Intel machine may see wrong DB6

[Juergen Gross]

On 26.02.2014 06:15, Zhang, Yang Z wrote:
> 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);
>   }
>
>   /*
>

Okay, finally I've got a test machine and could test your patch. The problem
was no longer observed, neither with nor without removing the
CPU_BASED_MOV_DR_EXITING bit in __restore_debug_registers().

I did no test to verify any other functionality regarding debug registers.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html