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

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

Jan Kiszka wrote:
> Avi Kivity wrote:
>> Instead of saving the debug registers from the processor to a kvm data
>> structure, rely in the debug registers stored in the thread structure.
>> This allows us not to save dr6 and dr7.
>>
>> Reduces lightweight vmexit cost by 350 cycles, or 11 percent.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |    3 ---
>>  arch/x86/kvm/x86.c              |   22 +++++++---------------
>>  2 files changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 6046e6f..45226f0 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -362,9 +362,6 @@ struct kvm_vcpu_arch {
>>  	u32 pat;
>>  
>>  	int switch_db_regs;
>> -	unsigned long host_db[KVM_NR_DB_REGS];
>> -	unsigned long host_dr6;
>> -	unsigned long host_dr7;
>>  	unsigned long db[KVM_NR_DB_REGS];
>>  	unsigned long dr6;
>>  	unsigned long dr7;
>> 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.
> 
>>  		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.
> 
>> +		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.

Looks like VMX loads 0x400 unconditionally into dr7 on vmexit, thus all
hw debugging is initially disabled, I must have missed this while
reworking the dr switch code. So we can safely load dr0..3 without
clearing dr7 first, and we can skip restoring of dr6 and dr7 unless the
current process (the hypervisor) is debugged itself.

> 
>>  
>>  	set_bit(KVM_REQ_KICK, &vcpu->requests);
>>  	local_irq_enable();
> 
> Jan
> 

Recent Intel CPUs seem to provide control over dr7/debugctl msr
saving/restoring (bit 2 in vmexit and vmentry control). Allready thought
about if this may further help to reduce the common case (ie. no host
and guest drX usage)?

Jan

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