Re: [PATCH 2/6] KVM: x86: Optimize debug register switching

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

Avi Kivity wrote:
> On 09/04/2009 03:51 PM, Jan Kiszka wrote:
>> Based on Avi's suggestion: Do not save the host debug registers on guest
>> entry as they are already present in the thread state. Moreover, only
>> restore them if the current host thread is being debugged. But as KGDB
>> accesses the debug register directly, we have to fall back to existing
>> pattern in that case.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>>   arch/x86/kvm/x86.c |   48 +++++++++++++++++++++++++++++++++---------------
>>   1 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 891234b..036a2c5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -37,6 +37,7 @@
>>   #include<linux/iommu.h>
>>   #include<linux/intel-iommu.h>
>>   #include<linux/cpufreq.h>
>> +#include<linux/kgdb.h>
>>   #include<trace/events/kvm.h>
>>   #undef TRACE_INCLUDE_FILE
>>   #define CREATE_TRACE_POINTS
>> @@ -3627,14 +3628,21 @@ 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);
>> +	/*
>> +	 * kgdb accesses the debug registers directly, so we have to save them
>> +	 * and restore those values on return from the guest.
>> +	 */
>> +	if (unlikely(kgdb_in_use())) {
>> +		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);
>> +		}
>> +		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);
>> -
>>   		set_debugreg(0, 7);
>>   		set_debugreg(vcpu->arch.eff_db[0], 0);
>>   		set_debugreg(vcpu->arch.eff_db[1], 1);
>> @@ -3645,15 +3653,25 @@ 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(kgdb_in_use())) {
>> +		if (unlikely(vcpu->arch.switch_db_regs)) {
>> +			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);
>> +		}
>> +		set_debugreg(vcpu->arch.host_dr6, 6);
>> +		set_debugreg(vcpu->arch.host_dr7, 7);
>> +	} else if (unlikely(test_thread_flag(TIF_DEBUG))) {
>> +		if (unlikely(vcpu->arch.switch_db_regs)) {
>> +			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);
>>
>>    
> 
> Please, let's have just save/nosave, not different kinds of save/restore.
> 
> But really, this looks very hacky.  It's better to have kgdb integrate 
> more closely with the kernel debug register support instead of kvm 
> juggling between the two.
> 
> Something like
> 
>    struct debug_registers thread_info::debugregs
>    extern struct debug_registers global_debug_registers;
> 
> and a function that loads a mix of the debug registers from the 
> thread-local and global settings.  The context switch path can call that 
> function as well as kvm.

For sure this approach is not nice. /me just wanted to avoid the area of
the "generic hw-breakpoint API". K. Prassad and Frederic Weisbecker
already try to push this abstraction for a fairly long time now.

OK, will go around and poll for status and perspectives of this effort.
Maybe throwing our requirements into the ring also helps accelerating it
a bit.

Jan

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