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

Message ID	20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net (mailing list archive)
State	New, archived
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n84CrihV005025 for <patchwork-kvm@patchwork.kernel.org>; Fri, 4 Sep 2009 12:53:45 GMT From: Jan Kiszka <jan.kiszka@siemens.com> Subject: [PATCH 2/6] KVM: x86: Optimize debug register switching To: Avi Kivity <avi@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com> Cc: kvm@vger.kernel.org, Jan Kiszka <jan.kiszka@siemens.com> Date: Fri, 04 Sep 2009 14:51:19 +0200 Message-ID: <20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net> In-Reply-To: <20090904125119.18939.89733.stgit@mchn012c.ww002.siemens.net> References: <20090904125119.18939.89733.stgit@mchn012c.ww002.siemens.net> User-Agent: StGIT/0.14.3 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: kvm-owner@vger.kernel.org Precedence: bulk

Message ID

20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net (mailing list archive)

State

New, archived

Headers

From: Jan Kiszka <jan.kiszka@siemens.com>
Subject: [PATCH 2/6] KVM: x86: Optimize debug register switching
To: Avi Kivity <avi@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org, Jan Kiszka <jan.kiszka@siemens.com>
Date: Fri, 04 Sep 2009 14:51:19 +0200
Message-ID: <20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net>
In-Reply-To: <20090904125119.18939.89733.stgit@mchn012c.ww002.siemens.net>
References: <20090904125119.18939.89733.stgit@mchn012c.ww002.siemens.net>
User-Agent: StGIT/0.14.3
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Commit Message

Jan Kiszka Sept. 4, 2009, 12:51 p.m. UTC

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(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Avi Kivity Sept. 6, 2009, 8:10 a.m. UTC | #1

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.

Jan Kiszka Sept. 7, 2009, 1:47 p.m. UTC | #2

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

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);
 
 	set_bit(KVM_REQ_KICK, &vcpu->requests);
 	local_irq_enable();

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

Commit Message

Comments

Patch