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

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

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.

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

Jan

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