From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: Use thread debug register storage instead of kvm specific data Date: Tue, 01 Sep 2009 15:31:42 +0200 Message-ID: <4A9D223E.5030906@siemens.com> References: <1251798248-13164-1-git-send-email-avi@redhat.com> <4A9CFA90.50202@siemens.com> <4A9D0289.7080201@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" To: Avi Kivity Return-path: Received: from thoth.sbs.de ([192.35.17.2]:15800 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754555AbZIANbz (ORCPT ); Tue, 1 Sep 2009 09:31:55 -0400 In-Reply-To: <4A9D0289.7080201@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi Kivity wrote: > On 09/01/2009 01:42 PM, Jan Kiszka wrote: >>> 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. >> > > IIUC, thread.debugregs should be synced with the hardware registers. > The kernel itself only writes to the debug registers, so we're safe > doing the same. Yep, confirmed. There is only an unsync'ed window between debug trap handling and signal injection, but I cannot imagine then kvm could intercept this path with a guest entry. > >> >>> 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. >> > > Yeah, I thought of it but forgot to implement it. > >>> + 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. >> > > The hardware will clear dr7 on exit. Given this fact, I wonder why you want to reload host dr0..3 on vcpu->arch.switch_db_regs. if (unlikely(test_thread_flag(TIF_DEBUG))) should suffice then, right? Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux