From: Jan Kiszka <jan.kiszka@web.de>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Gleb Natapov <gleb@kernel.org>, kvm <kvm@vger.kernel.org>
Subject: [PATCH v2] KVM: SVM: Fix reading of DR6
Date: Fri, 27 Dec 2013 19:17:07 +0100 [thread overview]
Message-ID: <52BDC423.4060404@web.de> (raw)
In-Reply-To: <20131227172109.GA12552@amt.cnet>
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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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 <jan.kiszka@siemens.com>
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 <jan.kiszka@siemens.com>
---
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))
--
1.8.1.1.298.ge7eed54
next prev parent reply other threads:[~2013-12-27 18:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-19 13:24 [PATCH] KVM: SVM: Fix reading of DR6 Jan Kiszka
2013-12-27 17:21 ` Marcelo Tosatti
2013-12-27 18:17 ` Jan Kiszka [this message]
2013-12-31 15:28 ` [PATCH v2] " Paolo Bonzini
2013-12-31 17:29 ` Jan Kiszka
2014-01-02 8:24 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52BDC423.4060404@web.de \
--to=jan.kiszka@web.de \
--cc=gleb@kernel.org \
--cc=kvm@vger.kernel.org \
--cc=mtosatti@redhat.com \
--cc=pbonzini@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.