* [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses @ 2020-05-04 15:55 Paolo Bonzini 2020-05-04 15:55 ` [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] Paolo Bonzini ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Paolo Bonzini @ 2020-05-04 15:55 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Sean Christopherson The purpose of this series is to get rid of the get_dr6 accessor and, on Intel, of set_dr6 as well. This is done mostly in patch 2, since patch 3 is only the resulting cleanup. Patch 1 is a related bug fix that I found while inspecting the code. A guest debugging selftest is sorely needed if anyone wants to take a look! Paolo Paolo Bonzini (3): KVM: SVM: fill in kvm_run->debug.arch.dr[67] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6 KVM: x86: simplify dr6 accessors in kvm_x86_ops arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/svm/svm.c | 11 ++++------- arch/x86/kvm/vmx/vmx.c | 11 ----------- arch/x86/kvm/x86.c | 8 +++----- 4 files changed, 7 insertions(+), 24 deletions(-) -- 2.18.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] 2020-05-04 15:55 [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Paolo Bonzini @ 2020-05-04 15:55 ` Paolo Bonzini 2020-05-06 23:42 ` Sasha Levin 2020-05-04 15:55 ` [PATCH 2/3] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6 Paolo Bonzini ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2020-05-04 15:55 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Sean Christopherson, stable The corresponding code was added for VMX in commit 42dbaa5a057 ("KVM: x86: Virtualize debug registers, 2008-12-15) but never for AMD. Fix this. Cc: stable@vger.kernel.org Fixes: 42dbaa5a057 ("KVM: x86: Virtualize debug registers") Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/svm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 8447ceb02c74..dbcf4198a9fe 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1732,6 +1732,8 @@ static int db_interception(struct vcpu_svm *svm) if (svm->vcpu.guest_debug & (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) { kvm_run->exit_reason = KVM_EXIT_DEBUG; + kvm_run->debug.arch.dr6 = svm->vmcb->save.dr6; + kvm_run->debug.arch.dr7 = svm->vmcb->save.dr7; kvm_run->debug.arch.pc = svm->vmcb->save.cs.base + svm->vmcb->save.rip; kvm_run->debug.arch.exception = DB_VECTOR; -- 2.18.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] 2020-05-04 15:55 ` [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] Paolo Bonzini @ 2020-05-06 23:42 ` Sasha Levin 0 siblings, 0 replies; 9+ messages in thread From: Sasha Levin @ 2020-05-06 23:42 UTC (permalink / raw) To: Sasha Levin, Paolo Bonzini, linux-kernel, kvm Cc: Sean Christopherson, stable, stable Hi [This is an automated email] This commit has been processed because it contains a "Fixes:" tag fixing commit: 42dbaa5a0577 ("KVM: x86: Virtualize debug registers"). The bot has tested the following trees: v5.6.10, v5.4.38, v4.19.120, v4.14.178, v4.9.221, v4.4.221. v5.6.10: Build OK! v5.4.38: Failed to apply! Possible dependencies: Unable to calculate v4.19.120: Failed to apply! Possible dependencies: Unable to calculate v4.14.178: Failed to apply! Possible dependencies: Unable to calculate v4.9.221: Failed to apply! Possible dependencies: Unable to calculate v4.4.221: Failed to apply! Possible dependencies: Unable to calculate NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks Sasha ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6 2020-05-04 15:55 [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Paolo Bonzini 2020-05-04 15:55 ` [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] Paolo Bonzini @ 2020-05-04 15:55 ` Paolo Bonzini 2020-05-04 15:55 ` [PATCH 3/3] KVM: x86: simplify dr6 accessors in kvm_x86_ops Paolo Bonzini 2020-05-04 18:55 ` [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Peter Xu 3 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2020-05-04 15:55 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Sean Christopherson Ensure that the current value of DR6 is always available in vcpu->arch.dr6, so that the get_dr6 callback can just access vcpu->arch.dr6 and becomes redundant. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/svm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index dbcf4198a9fe..6b65c75b6816 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1654,7 +1654,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd) static u64 svm_get_dr6(struct kvm_vcpu *vcpu) { - return to_svm(vcpu)->vmcb->save.dr6; + return vcpu->arch.dr6; } static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value) @@ -1673,7 +1673,7 @@ static void svm_sync_dirty_debug_regs(struct kvm_vcpu *vcpu) get_debugreg(vcpu->arch.db[1], 1); get_debugreg(vcpu->arch.db[2], 2); get_debugreg(vcpu->arch.db[3], 3); - vcpu->arch.dr6 = svm_get_dr6(vcpu); + vcpu->arch.dr6 = svm->vmcb->save.dr6; vcpu->arch.dr7 = svm->vmcb->save.dr7; vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT; @@ -1719,6 +1719,7 @@ static int db_interception(struct vcpu_svm *svm) if (!(svm->vcpu.guest_debug & (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) && !svm->nmi_singlestep) { + vcpu->arch.dr6 = svm->vmcb->save.dr6; kvm_queue_exception(&svm->vcpu, DB_VECTOR); return 1; } -- 2.18.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] KVM: x86: simplify dr6 accessors in kvm_x86_ops 2020-05-04 15:55 [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Paolo Bonzini 2020-05-04 15:55 ` [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] Paolo Bonzini 2020-05-04 15:55 ` [PATCH 2/3] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6 Paolo Bonzini @ 2020-05-04 15:55 ` Paolo Bonzini 2020-05-04 18:55 ` [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Peter Xu 3 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2020-05-04 15:55 UTC (permalink / raw) To: linux-kernel, kvm; +Cc: Sean Christopherson kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the second argument, and for both SVM and VMX the VMCB value is kept synchronized with vcpu->arch.dr6 on #DB; we can therefore remove the read accessor. For the write accessor we can avoid the retpoline penalty on Intel by accepting a NULL value and just skipping the call in that case. Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/svm/svm.c | 6 ------ arch/x86/kvm/vmx/vmx.c | 11 ----------- arch/x86/kvm/x86.c | 8 +++----- 4 files changed, 3 insertions(+), 23 deletions(-) diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 90840593cd6c..e218d907ba46 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1100,7 +1100,6 @@ 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 (*sync_dirty_debug_regs)(struct kvm_vcpu *vcpu); void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 6b65c75b6816..d8bc1b78a3c6 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1652,11 +1652,6 @@ 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) -{ - return vcpu->arch.dr6; -} - static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value) { struct vcpu_svm *svm = to_svm(vcpu); @@ -3996,7 +3991,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = { .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, .sync_dirty_debug_regs = svm_sync_dirty_debug_regs, diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index fce11f2c4c55..82042b7028a9 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5023,15 +5023,6 @@ static int handle_dr(struct kvm_vcpu *vcpu) return kvm_skip_emulated_instruction(vcpu); } -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_sync_dirty_debug_regs(struct kvm_vcpu *vcpu) { get_debugreg(vcpu->arch.db[0], 0); @@ -7812,8 +7803,6 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = { .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, .sync_dirty_debug_regs = vmx_sync_dirty_debug_regs, .cache_reg = vmx_cache_reg, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 8ec356ac1e6e..0ba83748cd53 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1069,7 +1069,8 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) static void kvm_update_dr6(struct kvm_vcpu *vcpu) { - if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) + if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) && + kvm_x86_ops.set_dr6) kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6); } @@ -1148,10 +1149,7 @@ int kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) case 4: /* fall through */ case 6: - if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) - *val = vcpu->arch.dr6; - else - *val = kvm_x86_ops.get_dr6(vcpu); + *val = vcpu->arch.dr6; break; case 5: /* fall through */ -- 2.18.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses 2020-05-04 15:55 [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Paolo Bonzini ` (2 preceding siblings ...) 2020-05-04 15:55 ` [PATCH 3/3] KVM: x86: simplify dr6 accessors in kvm_x86_ops Paolo Bonzini @ 2020-05-04 18:55 ` Peter Xu 2020-05-04 19:20 ` Paolo Bonzini 3 siblings, 1 reply; 9+ messages in thread From: Peter Xu @ 2020-05-04 18:55 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, Sean Christopherson On Mon, May 04, 2020 at 11:55:55AM -0400, Paolo Bonzini wrote: > The purpose of this series is to get rid of the get_dr6 accessor > and, on Intel, of set_dr6 as well. This is done mostly in patch 2, > since patch 3 is only the resulting cleanup. Patch 1 is a related > bug fix that I found while inspecting the code. Reviewed-by: Peter Xu <peterx@redhat.com> (Btw, the db_interception() change in patch 2 seems to be a real fix to me) > > A guest debugging selftest is sorely needed if anyone wants to take > a look! I have that in my list, but I don't know it's "sorely" needed. :) It was low after I knew the fact that we've got one test in kvm-unit-test, but I can for sure do that earlier. I am wondering whether we still want a test in selftests if there's a similar test in kvm-unit-test already. For this one I guess at least the guest debug test is still missing. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses 2020-05-04 18:55 ` [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Peter Xu @ 2020-05-04 19:20 ` Paolo Bonzini 2020-05-04 23:49 ` Peter Xu 0 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2020-05-04 19:20 UTC (permalink / raw) To: Peter Xu; +Cc: linux-kernel, kvm, Sean Christopherson On 04/05/20 20:55, Peter Xu wrote: > On Mon, May 04, 2020 at 11:55:55AM -0400, Paolo Bonzini wrote: >> The purpose of this series is to get rid of the get_dr6 accessor >> and, on Intel, of set_dr6 as well. This is done mostly in patch 2, >> since patch 3 is only the resulting cleanup. Patch 1 is a related >> bug fix that I found while inspecting the code. > > Reviewed-by: Peter Xu <peterx@redhat.com> > > (Btw, the db_interception() change in patch 2 seems to be a real fix to me) It should be okay because vcpu->arch.dr6 is not used on AMD. However I think a kvm_update_dr6 call is missing in kvm_deliver_exception_payload, and kvm_vcpu_check_breakpoint should use kvm_queue_exception_p. I'll fix all of those. > I have that in my list, but I don't know it's "sorely" needed. :) It was low > after I knew the fact that we've got one test in kvm-unit-test, but I can for > sure do that earlier. > > I am wondering whether we still want a test in selftests if there's a similar > test in kvm-unit-test already. For this one I guess at least the guest debug > test is still missing. The guest debugging test would basically cover the gdbstub case, which is different from kvm-unit-tests. It would run similar tests to kvm-unit-tests, but #DB and #BP exceptions would be replaced by KVM_EXIT_DEBUG, and MOVs to DR would be replaced by KVM_SET_GUEST_DEBUG. It could also cover exception payload support in KVM_GET_VCPU_EVENTS, but that is more complicated because it would require support for exceptions in the selftests. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses 2020-05-04 19:20 ` Paolo Bonzini @ 2020-05-04 23:49 ` Peter Xu 2020-05-05 11:36 ` Paolo Bonzini 0 siblings, 1 reply; 9+ messages in thread From: Peter Xu @ 2020-05-04 23:49 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, Sean Christopherson On Mon, May 04, 2020 at 09:20:05PM +0200, Paolo Bonzini wrote: > On 04/05/20 20:55, Peter Xu wrote: > > On Mon, May 04, 2020 at 11:55:55AM -0400, Paolo Bonzini wrote: > >> The purpose of this series is to get rid of the get_dr6 accessor > >> and, on Intel, of set_dr6 as well. This is done mostly in patch 2, > >> since patch 3 is only the resulting cleanup. Patch 1 is a related > >> bug fix that I found while inspecting the code. > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > > (Btw, the db_interception() change in patch 2 seems to be a real fix to me) > > It should be okay because vcpu->arch.dr6 is not used on AMD. > > However I think a kvm_update_dr6 call is missing in > kvm_deliver_exception_payload, and kvm_vcpu_check_breakpoint should use > kvm_queue_exception_p. Seems correct. Maybe apply the same thing to handle_exception_nmi() and handle_dr()? It's probably not a problem because VMX does not have set_dr6(), however it's still cleaner to avoid clearing DR_TRAP_BITS and set DR6_RTM manually before calling kvm_queue_exception() every time in VMX code. > I'll fix all of those. > > > I have that in my list, but I don't know it's "sorely" needed. :) It was low > > after I knew the fact that we've got one test in kvm-unit-test, but I can for > > sure do that earlier. > > > > I am wondering whether we still want a test in selftests if there's a similar > > test in kvm-unit-test already. For this one I guess at least the guest debug > > test is still missing. > > The guest debugging test would basically cover the gdbstub case, which > is different from kvm-unit-tests. It would run similar tests to > kvm-unit-tests, but #DB and #BP exceptions would be replaced by > KVM_EXIT_DEBUG, and MOVs to DR would be replaced by KVM_SET_GUEST_DEBUG. > > It could also cover exception payload support in KVM_GET_VCPU_EVENTS, > but that is more complicated because it would require support for > exceptions in the selftests. Yep, I guess the in-guest debug test will still need the exception support, though I also guess we don't need that when we have the kvm unit test, and anyway I'll start with the simple (KVM_SET_GUEST_DEBUG). Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses 2020-05-04 23:49 ` Peter Xu @ 2020-05-05 11:36 ` Paolo Bonzini 0 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2020-05-05 11:36 UTC (permalink / raw) To: Peter Xu; +Cc: linux-kernel, kvm, Sean Christopherson On 05/05/20 01:49, Peter Xu wrote: >> The guest debugging test would basically cover the gdbstub case, which >> is different from kvm-unit-tests. It would run similar tests to >> kvm-unit-tests, but #DB and #BP exceptions would be replaced by >> KVM_EXIT_DEBUG, and MOVs to DR would be replaced by KVM_SET_GUEST_DEBUG. >> >> It could also cover exception payload support in KVM_GET_VCPU_EVENTS, >> but that is more complicated because it would require support for >> exceptions in the selftests. > > Yep, I guess the in-guest debug test will still need the exception support, > though I also guess we don't need that when we have the kvm unit test We can still use it in order to test for example live migration with pending debug exceptions (KVM_GET_VCPU_EVENTS), but it's less important indeed. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-06 23:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 15:55 [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Paolo Bonzini 2020-05-04 15:55 ` [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67] Paolo Bonzini 2020-05-06 23:42 ` Sasha Levin 2020-05-04 15:55 ` [PATCH 2/3] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6 Paolo Bonzini 2020-05-04 15:55 ` [PATCH 3/3] KVM: x86: simplify dr6 accessors in kvm_x86_ops Paolo Bonzini 2020-05-04 18:55 ` [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses Peter Xu 2020-05-04 19:20 ` Paolo Bonzini 2020-05-04 23:49 ` Peter Xu 2020-05-05 11:36 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).