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-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@xxxxxxxxx>
> wrote:
>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>> On 27.02.14 at 02:31, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>
>>> wrote:
>>>>>> Jan Beulich wrote on 2014-02-27:
>>>>>>>>>> On 26.02.14 at 06:15, "Zhang, Yang Z"
>>>>>>>>>> <yang.z.zhang@xxxxxxxxx>
>>>>> 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@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel
>> 
>> 
> 
>


Best regards,
Yang



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