From patchwork Fri Dec 27 18:17:07 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kiszka X-Patchwork-Id: 3412361 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 46B99C02DC for ; Fri, 27 Dec 2013 18:17:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B77B620144 for ; Fri, 27 Dec 2013 18:17:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EDFC9200D6 for ; Fri, 27 Dec 2013 18:17:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754456Ab3L0SRM (ORCPT ); Fri, 27 Dec 2013 13:17:12 -0500 Received: from mout.web.de ([212.227.15.4]:63992 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754152Ab3L0SRL (ORCPT ); Fri, 27 Dec 2013 13:17:11 -0500 Received: from mchn199C.mchp.siemens.de ([178.11.213.8]) by smtp.web.de (mrweb103) with ESMTPSA (Nemesis) id 0LhNs4-1VA83x0Lp2-00maOc for ; Fri, 27 Dec 2013 19:17:09 +0100 Message-ID: <52BDC423.4060404@web.de> Date: Fri, 27 Dec 2013 19:17:07 +0100 From: Jan Kiszka User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 To: Marcelo Tosatti CC: Paolo Bonzini , Gleb Natapov , kvm Subject: [PATCH v2] KVM: SVM: Fix reading of DR6 References: <52B2F3AB.9050101@siemens.com> <20131227172109.GA12552@amt.cnet> In-Reply-To: <20131227172109.GA12552@amt.cnet> X-Enigmail-Version: 1.6 X-Provags-ID: V03:K0:d2fGCBOKEuuZsy+ID6ANjddSrjvxDtE02UZE1GZSVUknNOyNzNL afWZfS17xMtqOCVgmdtyHR/m8icSYsOZCtW2+uMwQoZW2pfQTo1J/X1Swf6ixMhbU86uAb1 BkkCT1TeHEkS7cre02lxixgiafI3zvLg1QqmlJOzxx900ktw3hOFhHilQKs/xe9pF82fgqd eeoN6CcMM1a91coHOmBYw== Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2013-12-27 18:21, Marcelo Tosatti wrote: > On Thu, Dec 19, 2013 at 02:24:59PM +0100, Jan Kiszka wrote: >> 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 vendor >> hook to obtain the current value. >> >> Fixes a regression of 020df0794f. >> >> Signed-off-by: Jan Kiszka >> --- >> >> Should go to stable as well. >> >> arch/x86/include/asm/kvm_host.h | 1 + >> arch/x86/kvm/svm.c | 9 +++++++++ >> arch/x86/kvm/vmx.c | 6 ++++++ >> arch/x86/kvm/x86.c | 2 +- >> 4 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index ae5d783..f115f46 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -699,6 +699,7 @@ 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_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..48fa63e 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1671,6 +1671,14 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd) >> mark_dirty(svm->vmcb, VMCB_ASID); >> } >> >> +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_dr7(struct kvm_vcpu *vcpu, unsigned long value) >> { >> struct vcpu_svm *svm = to_svm(vcpu); >> @@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = { >> .set_idt = svm_set_idt, >> .get_gdt = svm_get_gdt, >> .set_gdt = svm_set_gdt, >> + .get_dr6 = svm_get_dr6, >> .set_dr7 = svm_set_dr7, >> .cache_reg = svm_cache_reg, >> .get_rflags = svm_get_rflags, >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index ee3bf54..c5c7e62 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu) >> return 1; >> } >> >> +static u64 vmx_get_dr6(struct kvm_vcpu *vcpu) >> +{ >> + return vcpu->arch.dr6; >> +} >> + >> static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val) >> { >> vmcs_writel(GUEST_DR7, val); >> @@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = { >> .set_idt = vmx_set_idt, >> .get_gdt = vmx_get_gdt, >> .set_gdt = vmx_set_gdt, >> + .get_dr6 = vmx_get_dr6, >> .set_dr7 = vmx_set_dr7, >> .cache_reg = vmx_cache_reg, >> .get_rflags = vmx_get_rflags, >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 1dc0359..8fe227c 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) >> return 1; >> /* fall through */ >> case 6: >> - *val = vcpu->arch.dr6; >> + *val = kvm_x86_ops->get_dr6(vcpu); >> break; >> case 5: >> if (kvm_read_cr4_bits(vcpu, X86_CR4_DE)) >> -- >> 1.8.1.1.298.ge7eed54 > > It allows > > kvm_set_dr(a) > val = kvm_get_dr() > > to have 'val' different than a. > > Is this OK ? (its certainly counter intuitive). No, it's not ok. We also need to sync the guest-visible state to the VMCB on updates. ----8<---- 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 vendor 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 of 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/kvm_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, struct svm_cpu_data *sd) mark_dirty(svm->vmcb, VMCB_ASID); } +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 = to_svm(vcpu); + + svm->vmcb->save.dr6 = value; + mark_dirty(svm->vmcb, VMCB_DR); +} + static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value) { struct vcpu_svm *svm = to_svm(vcpu); @@ -4286,6 +4302,8 @@ static struct kvm_x86_ops svm_x86_ops = { .set_idt = svm_set_idt, .get_gdt = svm_get_gdt, .set_gdt = svm_set_gdt, + .get_dr6 = svm_get_dr6, + .set_dr6 = svm_set_dr6, .set_dr7 = svm_set_dr7, .cache_reg = svm_cache_reg, .get_rflags = svm_get_rflags, diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ee3bf54..1d9b0ec 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -5153,6 +5153,15 @@ static int handle_dr(struct kvm_vcpu *vcpu) return 1; } +static u64 vmx_get_dr6(struct kvm_vcpu *vcpu) +{ + return vcpu->arch.dr6; +} + +static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val) +{ +} + static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val) { vmcs_writel(GUEST_DR7, val); @@ -8573,6 +8582,8 @@ static struct kvm_x86_ops vmx_x86_ops = { .set_idt = vmx_set_idt, .get_gdt = vmx_get_gdt, .set_gdt = vmx_set_gdt, + .get_dr6 = vmx_get_dr6, + .set_dr6 = vmx_set_dr6, .set_dr7 = vmx_set_dr7, .cache_reg = vmx_cache_reg, .get_rflags = vmx_get_rflags, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1dc0359..be000ec 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -747,6 +747,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) if (val & 0xffffffff00000000ULL) return -1; /* #GP */ vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1; + kvm_x86_ops->set_dr6(vcpu, vcpu->arch.dr6); break; case 5: if (kvm_read_cr4_bits(vcpu, X86_CR4_DE)) @@ -788,7 +789,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) return 1; /* fall through */ case 6: - *val = vcpu->arch.dr6; + *val = kvm_x86_ops->get_dr6(vcpu); break; case 5: if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))