From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/6] KVM: x86: Optimize debug register switching Date: Sun, 06 Sep 2009 11:10:03 +0300 Message-ID: <4AA36E5B.2050106@redhat.com> References: <20090904125119.18939.89733.stgit@mchn012c.ww002.siemens.net> <20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54338 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752227AbZIFIKH (ORCPT ); Sun, 6 Sep 2009 04:10:07 -0400 In-Reply-To: <20090904125119.18939.56127.stgit@mchn012c.ww002.siemens.net> Sender: kvm-owner@vger.kernel.org List-ID: 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 > --- > > 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 > #include > #include > +#include > #include > #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