From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v2] KVM: SVM: Fix reading of DR6 Date: Tue, 31 Dec 2013 18:29:12 +0100 Message-ID: <52C2FEE8.1060001@web.de> References: <52B2F3AB.9050101@siemens.com> <20131227172109.GA12552@amt.cnet> <52BDC423.4060404@web.de> <52C2E287.2050400@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="eTpWU2BdksrDuDdXht2QIgl3g25CTVrGU" Cc: Marcelo Tosatti , Gleb Natapov , kvm To: Paolo Bonzini Return-path: Received: from mout.web.de ([212.227.17.11]:56051 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755486Ab3LaR3S (ORCPT ); Tue, 31 Dec 2013 12:29:18 -0500 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb102) with ESMTPSA (Nemesis) id 0M6mPA-1VCWYC43vQ-00wSjj for ; Tue, 31 Dec 2013 18:29:17 +0100 In-Reply-To: <52C2E287.2050400@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --eTpWU2BdksrDuDdXht2QIgl3g25CTVrGU Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2013-12-31 16:28, Paolo Bonzini wrote: > Il 27/12/2013 19:17, Jan Kiszka ha scritto: >> >> From: Jan Kiszka >> >> In contrast to VMX, SVM dose not automatically transfer DR6 into the >> VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendo= r >> hook to obtain the current value. And as SVM now picks the DR6 state >> from its VMCB, we also need a set callback in order to write updates o= f >> DR6 back. >> >> Fixes a regression of 020df0794f. >> >> Signed-off-by: Jan Kiszka >> --- >> arch/x86/include/asm/kvm_host.h | 2 ++ >> arch/x86/kvm/svm.c | 18 ++++++++++++++++++ >> arch/x86/kvm/vmx.c | 11 +++++++++++ >> arch/x86/kvm/x86.c | 3 ++- >> 4 files changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kv= m_host.h >> index ae5d783..e73651b 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -699,6 +699,8 @@ struct kvm_x86_ops { >> void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); >> void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); >> void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); >> + u64 (*get_dr6)(struct kvm_vcpu *vcpu); >> + void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value); >> void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value); >> void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg); >> unsigned long (*get_rflags)(struct kvm_vcpu *vcpu); >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index c7168a5..5987414 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1671,6 +1671,22 @@ static void new_asid(struct vcpu_svm *svm, stru= ct svm_cpu_data *sd) >> mark_dirty(svm->vmcb, VMCB_ASID); >> } >> =20 >> +static u64 svm_get_dr6(struct kvm_vcpu *vcpu) >> +{ >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) >> + return vcpu->arch.dr6; >> + else >> + return to_svm(vcpu)->vmcb->save.dr6; >> +} >> + >> +static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value) >> +{ >> + struct vcpu_svm *svm =3D to_svm(vcpu); >> + >> + svm->vmcb->save.dr6 =3D value; >> + mark_dirty(svm->vmcb, VMCB_DR); >=20 > The code here is a bit different from the existing DR7 hooks. Why isn'= t > get_dr7 needed? I cannot find anything in the AMD manuals that suggest= s > a difference between DR6 and DR7. DR7 doesn't change while the guest is running, only when the guest writes to it, and we intercept that. >=20 > Also, set_dr7 is only called when !(vcpu->guest_debug & > KVM_GUESTDBG_USE_HW_BP). I think it makes sense to do the same for all= > the new hooks you need to introduce (get_dr6, set_dr6 and possibly get_= dr7). We could make set_dr6 conditional, but it doesn't make a difference in practice. If guest debugging is enabled, we effectively only use DR6 when leaving the guest, i.e. when the hardware defined DR6. Hmm, SVM is not updating kvm_run::debug.arch.dr6/7... There might be more broken, need to check. But that would not be guest visible then. >=20 > And finally, is it necessary to write to both vcpu->arch.dr6/7 and > svm->vmcb->save.dr6/7, or is it simply okay to do that? arch.dr6/7 holds the guest visible state, save.dr6/7 the effective one. Jan PS: I'll look into a kvm unit test for hardware debugging features these days. Guest-side at least, no idea yet how to implement host-side debugging with unit tests. --eTpWU2BdksrDuDdXht2QIgl3g25CTVrGU Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlLC/usACgkQitSsb3rl5xTejgCfS0sJ+day0yhkMTvnZjgIJCSK lK8An2dqjf9Adyhbz9u6XhvPqAdoCpeR =9kEa -----END PGP SIGNATURE----- --eTpWU2BdksrDuDdXht2QIgl3g25CTVrGU--