Re: [PATCH] KVM: Use thread debug register storage instead of kvm specific data

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> On 09/01/2009 01:42 PM, Jan Kiszka wrote:
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 891234b..9e3acbd 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -3627,14 +3627,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>
>>>   	kvm_guest_enter();
>>>
>>> -	get_debugreg(vcpu->arch.host_dr6, 6);
>>> -	get_debugreg(vcpu->arch.host_dr7, 7);
>>>   	if (unlikely(vcpu->arch.switch_db_regs)) {
>>> -		get_debugreg(vcpu->arch.host_db[0], 0);
>>> -		get_debugreg(vcpu->arch.host_db[1], 1);
>>> -		get_debugreg(vcpu->arch.host_db[2], 2);
>>> -		get_debugreg(vcpu->arch.host_db[3], 3);
>>> -
>>>      
>> I'm fine with this for the common case, but we should switch to the safe
>> pattern on test_thread_flag(TIF_DEBUG), ie. if someone debugs qemu using
>> hardware break/watchpoints.
>>    
> 
> IIUC, thread.debugregs should be synced with the hardware registers.  
> The kernel itself only writes to the debug registers, so we're safe 
> doing the same.

Yep, confirmed. There is only an unsync'ed window between debug trap
handling and signal injection, but I cannot imagine then kvm could
intercept this path with a guest entry.

> 
>>    
>>>   		set_debugreg(0, 7);
>>>   		set_debugreg(vcpu->arch.eff_db[0], 0);
>>>   		set_debugreg(vcpu->arch.eff_db[1], 1);
>>> @@ -3645,15 +3638,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>   	trace_kvm_entry(vcpu->vcpu_id);
>>>   	kvm_x86_ops->run(vcpu);
>>>
>>> -	if (unlikely(vcpu->arch.switch_db_regs)) {
>>> -		set_debugreg(0, 7);
>>> -		set_debugreg(vcpu->arch.host_db[0], 0);
>>> -		set_debugreg(vcpu->arch.host_db[1], 1);
>>> -		set_debugreg(vcpu->arch.host_db[2], 2);
>>> -		set_debugreg(vcpu->arch.host_db[3], 3);
>>> +	if (unlikely(vcpu->arch.switch_db_regs || test_thread_flag(TIF_DEBUG))) {
>>>      
>> IIRC, you must clear dr7 before loading dr0..3 to avoid spurious effects.
>>    
> 
> Yeah, I thought of it but forgot to implement it.
> 
>>> +		set_debugreg(current->thread.debugreg0, 0);
>>> +		set_debugreg(current->thread.debugreg1, 1);
>>> +		set_debugreg(current->thread.debugreg2, 2);
>>> +		set_debugreg(current->thread.debugreg3, 3);
>>> +		set_debugreg(current->thread.debugreg6, 6);
>>> +		set_debugreg(current->thread.debugreg7, 7);
>>>   	}
>>> -	set_debugreg(vcpu->arch.host_dr6, 6);
>>> -	set_debugreg(vcpu->arch.host_dr7, 7);
>>>      
>> Unless I miss something ATM, moving this into the if-clause should cause
>> troubles if the guest leaves dr7 armed behind. But I need to refresh my
>> memory on this.
>>    
> 
> The hardware will clear dr7 on exit.

Given this fact, I wonder why you want to reload host dr0..3 on
vcpu->arch.switch_db_regs. if (unlikely(test_thread_flag(TIF_DEBUG)))
should suffice then, right?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux