* [PATCH] KVM: SVM: Fix reading of DR6 @ 2013-12-19 13:24 Jan Kiszka 2013-12-27 17:21 ` Marcelo Tosatti 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2013-12-19 13:24 UTC (permalink / raw) To: Paolo Bonzini, Gleb Natapov; +Cc: kvm 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: SVM: Fix reading of DR6 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 ` [PATCH v2] " Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: Marcelo Tosatti @ 2013-12-27 17:21 UTC (permalink / raw) To: Jan Kiszka; +Cc: Paolo Bonzini, Gleb Natapov, kvm 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). ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] KVM: SVM: Fix reading of DR6 2013-12-27 17:21 ` Marcelo Tosatti @ 2013-12-27 18:17 ` Jan Kiszka 2013-12-31 15:28 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2013-12-27 18:17 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Paolo Bonzini, Gleb Natapov, kvm 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6 2013-12-27 18:17 ` [PATCH v2] " Jan Kiszka @ 2013-12-31 15:28 ` Paolo Bonzini 2013-12-31 17:29 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2013-12-31 15:28 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Gleb Natapov, kvm Il 27/12/2013 19:17, Jan Kiszka ha scritto: > > 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); 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 suggests a difference between DR6 and DR7. 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). 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? Paolo > +} > + > 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)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6 2013-12-31 15:28 ` Paolo Bonzini @ 2013-12-31 17:29 ` Jan Kiszka 2014-01-02 8:24 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2013-12-31 17:29 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Marcelo Tosatti, Gleb Natapov, kvm [-- Attachment #1: Type: text/plain, Size: 3549 bytes --] On 2013-12-31 16:28, Paolo Bonzini wrote: > Il 27/12/2013 19:17, Jan Kiszka ha scritto: >> >> 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); > > 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 suggests > 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. > > 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. > > 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. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 263 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6 2013-12-31 17:29 ` Jan Kiszka @ 2014-01-02 8:24 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2014-01-02 8:24 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Gleb Natapov, kvm -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 31/12/2013 18:29, Jan Kiszka ha scritto: >> 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. No, it doesn't indeed. It's just a matter of consistency. > 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. > >> >> 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. It would be possible to do it with infrastructure like qtest or autotest. You could speak the gdbserver protocol yourself, or open a pty to gdb. But guest-side is already an improvement over nothing! Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJSxSI0AAoJEBvWZb6bTYbyoiYP/REKgPL/dUwJoSA8AEm7h6Ip wkXlaxBcyBiKlIzfiN3abBXBOZvDy7dSzNfXUZ4bY6DitQIJZYSVc93XfvWYRToY ORC6qZej+P/bD/rNwATpvVhGG6RbRpmihmVgkoX5w5J3GWZ52QREcyoW7mZEhIoR fCMJmNCq2l9vxa5hhXLAuiLYtq7mto6hQpFE8TXG81PyzE+c3WWK1D5vy/2cpv48 qabaVsJs7aj0KV2rVDvyQkO+GSdm+8SDpf4GyJcLo1Y1yYjPW97xC0Ey4ZyEXXWf 37zb3oOoH9NPPXtt1oJhUFCFZ34v1MxW2j0IIGvtUfRPSjphW+T/nQ5y17GG+efQ TbKnSlibNP0L7fX/xa/sytaYcslIJVIKvvq0Wz9a3Ka1OHNufgtBIreUjtAPvSmy TLxTm2w2s0S/mD3yMDK62uAtQMV/FW7qV6WHTHHZzj8VJpgfAAO0j4gkN0PlLB91 XfuR8ANmYoAKm9yafOSlr6Xw4uzHh41ZODMIY9w/pwB6nGB8SMRVBUWAILkyiyoN btJdVCFdmxTlUmszAeHGTkd4VMtOMqva8MmgnV5isVEUy5A6gJZwNSZ8KJCdny+J N+jRLj/j3SUlxGFnsK4FPBlMQzK1DDYS3C2QKd+Z5No42kZg4w2g4n0zQamDi2XR 2lcuexvRePHTZglNkP82 =dDQc -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-02 8:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 ` [PATCH v2] " Jan Kiszka 2013-12-31 15:28 ` Paolo Bonzini 2013-12-31 17:29 ` Jan Kiszka 2014-01-02 8:24 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox