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

[Avi Kivity <avi@redhat.com>]

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.

-- 
error compiling committee.c: too many arguments to function