[PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[Paolo Bonzini]

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

[PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67]

[Paolo Bonzini]

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

Re: [PATCH 1/3] KVM: SVM: fill in kvm_run->debug.arch.dr[67]

[Sasha Levin]

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

[PATCH 2/3] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6

[Paolo Bonzini]

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

[PATCH 3/3] KVM: x86: simplify dr6 accessors in kvm_x86_ops

[Paolo Bonzini]

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

Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[Peter Xu]

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

Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[Paolo Bonzini]

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

Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[Peter Xu]

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

Re: [PATCH 0/3] KVM: x86: cleanup and fixes for debug register accesses

[Paolo Bonzini]

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