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

[Juergen Gross <juergen.gross@ts.fujitsu.com>]

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