[PATCH 0/9] KVM_SET_GUEST_DEBUG tests and fixes, DR accessors cleanups

[PATCH 0/9] KVM_SET_GUEST_DEBUG tests and fixes, DR accessors cleanups

[Paolo Bonzini]

I am posting all the pending patches as a full series because I found
another issue on AMD, which is easily fixed with the last patch but has
dependencies on the patches to keep DR6 synchronized with vcpu->arch.dr6.

Paolo Bonzini (5):
  KVM: x86: fix DR6 delivery for various cases of #DB injection
  KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6
  KVM: x86: simplify dr6 accessors in kvm_x86_ops
  KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG
  KVM: VMX: pass correct DR6 for GD userspace exit

Peter Xu (4):
  KVM: X86: Declare KVM_CAP_SET_GUEST_DEBUG properly
  KVM: X86: Set RTM for DB_VECTOR too for KVM_EXIT_DEBUG
  KVM: X86: Fix single-step with KVM_SET_GUEST_DEBUG
  KVM: selftests: Add KVM_SET_GUEST_DEBUG test

 arch/powerpc/kvm/powerpc.c                    |   1 +
 arch/s390/kvm/kvm-s390.c                      |   1 +
 arch/x86/include/asm/kvm_host.h               |   2 +-
 arch/x86/kvm/svm/svm.c                        |  11 +-
 arch/x86/kvm/vmx/vmx.c                        |  23 +-
 arch/x86/kvm/x86.c                            |  28 +--
 arch/x86/kvm/x86.h                            |   2 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/include/kvm_util.h  |   2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |   9 +
 .../testing/selftests/kvm/x86_64/debug_regs.c | 202 ++++++++++++++++++
 11 files changed, 243 insertions(+), 39 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/debug_regs.c

-- 
2.18.2

[PATCH 1/9] KVM: X86: Declare KVM_CAP_SET_GUEST_DEBUG properly

[Paolo Bonzini]

From: Peter Xu <peterx@redhat.com>

KVM_CAP_SET_GUEST_DEBUG should be supported for x86 however it's not declared
as supported.  My wild guess is that userspaces like QEMU are using "#ifdef
KVM_CAP_SET_GUEST_DEBUG" to check for the capability instead, but that could be
wrong because the compilation host may not be the runtime host.

The userspace might still want to keep the old "#ifdef" though to not break the
guest debug on old kernels.

Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20200505154750.126300-1-peterx@redhat.com>
[Do the same for PPC and s390. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/powerpc/kvm/powerpc.c | 1 +
 arch/s390/kvm/kvm-s390.c   | 1 +
 arch/x86/kvm/x86.c         | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index e15166b0a16d..ad2f172c26a6 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -521,6 +521,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_IOEVENTFD:
 	case KVM_CAP_DEVICE_CTRL:
 	case KVM_CAP_IMMEDIATE_EXIT:
+	case KVM_CAP_SET_GUEST_DEBUG:
 		r = 1;
 		break;
 	case KVM_CAP_PPC_GUEST_DEBUG_SSTEP:
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 5dcf9ff12828..d05bb040fd42 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -545,6 +545,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_AIS:
 	case KVM_CAP_S390_AIS_MIGRATION:
 	case KVM_CAP_S390_VCPU_RESETS:
+	case KVM_CAP_SET_GUEST_DEBUG:
 		r = 1;
 		break;
 	case KVM_CAP_S390_HPAGE_1M:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8d296e3d0d56..d786c7d27ce5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3372,6 +3372,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_GET_MSR_FEATURES:
 	case KVM_CAP_MSR_PLATFORM_INFO:
 	case KVM_CAP_EXCEPTION_PAYLOAD:
+	case KVM_CAP_SET_GUEST_DEBUG:
 		r = 1;
 		break;
 	case KVM_CAP_SYNC_REGS:
-- 
2.18.2

[PATCH 2/9] KVM: x86: fix DR6 delivery for various cases of #DB injection

[Paolo Bonzini]

Go through kvm_queue_exception_p so that the payload is correctly delivered
through the exit qualification, and add a kvm_update_dr6 call to
kvm_deliver_exception_payload that is needed on AMD.

Reported-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/vmx/vmx.c          |  8 ++------
 arch/x86/kvm/x86.c              | 11 ++++++-----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0dea9f122bb9..8c247bcb037e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1449,6 +1449,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu);
 
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr);
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
+void kvm_queue_exception_p(struct kvm_vcpu *vcpu, unsigned nr, unsigned long payload);
 void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr);
 void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c2c6335a998c..bb5a527e49d9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4677,12 +4677,10 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		dr6 = vmcs_readl(EXIT_QUALIFICATION);
 		if (!(vcpu->guest_debug &
 		      (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))) {
-			vcpu->arch.dr6 &= ~DR_TRAP_BITS;
-			vcpu->arch.dr6 |= dr6 | DR6_RTM;
 			if (is_icebp(intr_info))
 				WARN_ON(!skip_emulated_instruction(vcpu));
 
-			kvm_queue_exception(vcpu, DB_VECTOR);
+			kvm_queue_exception_p(vcpu, DB_VECTOR, dr6);
 			return 1;
 		}
 		kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1;
@@ -4936,9 +4934,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			vcpu->run->exit_reason = KVM_EXIT_DEBUG;
 			return 0;
 		} else {
-			vcpu->arch.dr6 &= ~DR_TRAP_BITS;
-			vcpu->arch.dr6 |= DR6_BD | DR6_RTM;
-			kvm_queue_exception(vcpu, DB_VECTOR);
+			kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BD);
 			return 1;
 		}
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d786c7d27ce5..109115c96897 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -104,6 +104,7 @@ static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
                                     KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
 
 static void update_cr8_intercept(struct kvm_vcpu *vcpu);
+static void kvm_update_dr6(struct kvm_vcpu *vcpu);
 static void process_nmi(struct kvm_vcpu *vcpu);
 static void enter_smm(struct kvm_vcpu *vcpu);
 static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
@@ -473,6 +474,7 @@ void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)
 		 * breakpoint), it is reserved and must be zero in DR6.
 		 */
 		vcpu->arch.dr6 &= ~BIT(12);
+		kvm_update_dr6(vcpu);
 		break;
 	case PF_VECTOR:
 		vcpu->arch.cr2 = payload;
@@ -572,11 +574,12 @@ void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 }
 EXPORT_SYMBOL_GPL(kvm_requeue_exception);
 
-static void kvm_queue_exception_p(struct kvm_vcpu *vcpu, unsigned nr,
-				  unsigned long payload)
+void kvm_queue_exception_p(struct kvm_vcpu *vcpu, unsigned nr,
+			   unsigned long payload)
 {
 	kvm_multiple_exception(vcpu, nr, false, 0, true, payload, false);
 }
+EXPORT_SYMBOL_GPL(kvm_queue_exception_p);
 
 static void kvm_queue_exception_e_p(struct kvm_vcpu *vcpu, unsigned nr,
 				    u32 error_code, unsigned long payload)
@@ -6719,9 +6722,7 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
 					   vcpu->arch.db);
 
 		if (dr6 != 0) {
-			vcpu->arch.dr6 &= ~DR_TRAP_BITS;
-			vcpu->arch.dr6 |= dr6 | DR6_RTM;
-			kvm_queue_exception(vcpu, DB_VECTOR);
+			kvm_queue_exception_p(vcpu, DB_VECTOR, dr6);
 			*r = 1;
 			return true;
 		}
-- 
2.18.2

[PATCH 3/9] KVM: X86: Set RTM for DB_VECTOR too for KVM_EXIT_DEBUG

[Paolo Bonzini]

From: Peter Xu <peterx@redhat.com>

RTM should always been set even with KVM_EXIT_DEBUG on #DB.

Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20200505205000.188252-2-peterx@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bb5a527e49d9..2384a2dbec44 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4683,7 +4683,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 			kvm_queue_exception_p(vcpu, DB_VECTOR, dr6);
 			return 1;
 		}
-		kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1;
+		kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM;
 		kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
 		/* fall through */
 	case BP_VECTOR:
-- 
2.18.2

[PATCH 4/9] KVM: X86: Fix single-step with KVM_SET_GUEST_DEBUG

[Paolo Bonzini]

From: Peter Xu <peterx@redhat.com>

When single-step triggered with KVM_SET_GUEST_DEBUG, we should fill in the pc
value with current linear RIP rather than the cached singlestep address.

Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20200505205000.188252-3-peterx@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 109115c96897..f7628555f036 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6662,7 +6662,7 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
 
 	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
 		kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM;
-		kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
+		kvm_run->debug.arch.pc = kvm_get_linear_rip(vcpu);
 		kvm_run->debug.arch.exception = DB_VECTOR;
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
 		return 0;
-- 
2.18.2

[PATCH 5/9] KVM: selftests: Add KVM_SET_GUEST_DEBUG test

[Paolo Bonzini]

From: Peter Xu <peterx@redhat.com>

Covers fundamental tests for KVM_SET_GUEST_DEBUG. It is very close to the debug
test in kvm-unit-test, but doing it from outside the guest.

Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20200505205000.188252-4-peterx@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../testing/selftests/kvm/include/kvm_util.h  |   2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |   9 +
 .../testing/selftests/kvm/x86_64/debug_regs.c | 180 ++++++++++++++++++
 4 files changed, 192 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/debug_regs.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 712a2ddd2a27..44b6ef513164 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -28,6 +28,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_dirty_log_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
 TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
+TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
 TEST_GEN_PROGS_x86_64 += clear_dirty_log_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
 TEST_GEN_PROGS_x86_64 += dirty_log_test
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index a99b875f50d2..92e184a422ee 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -143,6 +143,8 @@ struct kvm_run *vcpu_state(struct kvm_vm *vm, uint32_t vcpuid);
 void vcpu_run(struct kvm_vm *vm, uint32_t vcpuid);
 int _vcpu_run(struct kvm_vm *vm, uint32_t vcpuid);
 void vcpu_run_complete_io(struct kvm_vm *vm, uint32_t vcpuid);
+void vcpu_set_guest_debug(struct kvm_vm *vm, uint32_t vcpuid,
+			  struct kvm_guest_debug *debug);
 void vcpu_set_mp_state(struct kvm_vm *vm, uint32_t vcpuid,
 		       struct kvm_mp_state *mp_state);
 void vcpu_regs_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_regs *regs);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 8a3523d4434f..9622431069bc 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1201,6 +1201,15 @@ void vcpu_run_complete_io(struct kvm_vm *vm, uint32_t vcpuid)
 		    ret, errno);
 }
 
+void vcpu_set_guest_debug(struct kvm_vm *vm, uint32_t vcpuid,
+			  struct kvm_guest_debug *debug)
+{
+	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
+	int ret = ioctl(vcpu->fd, KVM_SET_GUEST_DEBUG, debug);
+
+	TEST_ASSERT(ret == 0, "KVM_SET_GUEST_DEBUG failed: %d", ret);
+}
+
 /*
  * VM VCPU Set MP State
  *
diff --git a/tools/testing/selftests/kvm/x86_64/debug_regs.c b/tools/testing/selftests/kvm/x86_64/debug_regs.c
new file mode 100644
index 000000000000..077f25d61d1a
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/debug_regs.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KVM guest debug register tests
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#include <stdio.h>
+#include <string.h>
+#include "kvm_util.h"
+#include "processor.h"
+
+#define VCPU_ID 0
+
+/* For testing data access debug BP */
+uint32_t guest_value;
+
+extern unsigned char sw_bp, hw_bp, write_data, ss_start;
+
+static void guest_code(void)
+{
+	/*
+	 * Software BP tests.
+	 *
+	 * NOTE: sw_bp need to be before the cmd here, because int3 is an
+	 * exception rather than a normal trap for KVM_SET_GUEST_DEBUG (we
+	 * capture it using the vcpu exception bitmap).
+	 */
+	asm volatile("sw_bp: int3");
+
+	/* Hardware instruction BP test */
+	asm volatile("hw_bp: nop");
+
+	/* Hardware data BP test */
+	asm volatile("mov $1234,%%rax;\n\t"
+		     "mov %%rax,%0;\n\t write_data:"
+		     : "=m" (guest_value) : : "rax");
+
+	/* Single step test, covers 2 basic instructions and 2 emulated */
+	asm volatile("ss_start: "
+		     "xor %%rax,%%rax\n\t"
+		     "cpuid\n\t"
+		     "movl $0x1a0,%%ecx\n\t"
+		     "rdmsr\n\t"
+		     : : : "rax", "ecx");
+
+	GUEST_DONE();
+}
+
+#define  CLEAR_DEBUG()  memset(&debug, 0, sizeof(debug))
+#define  APPLY_DEBUG()  vcpu_set_guest_debug(vm, VCPU_ID, &debug)
+#define  CAST_TO_RIP(v)  ((unsigned long long)&(v))
+#define  SET_RIP(v)  do {				\
+		vcpu_regs_get(vm, VCPU_ID, &regs);	\
+		regs.rip = (v);				\
+		vcpu_regs_set(vm, VCPU_ID, &regs);	\
+	} while (0)
+#define  MOVE_RIP(v)  SET_RIP(regs.rip + (v));
+
+int main(void)
+{
+	struct kvm_guest_debug debug;
+	unsigned long long target_dr6, target_rip;
+	struct kvm_regs regs;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+	uint64_t cmd;
+	int i;
+	/* Instruction lengths starting at ss_start */
+	int ss_size[4] = {
+		3,		/* xor */
+		2,		/* cpuid */
+		5,		/* mov */
+		2,		/* rdmsr */
+	};
+
+	if (!kvm_check_cap(KVM_CAP_SET_GUEST_DEBUG)) {
+		print_skip("KVM_CAP_SET_GUEST_DEBUG not supported");
+		return 0;
+	}
+
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
+	run = vcpu_state(vm, VCPU_ID);
+
+	/* Test software BPs - int3 */
+	CLEAR_DEBUG();
+	debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+	APPLY_DEBUG();
+	vcpu_run(vm, VCPU_ID);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG &&
+		    run->debug.arch.exception == BP_VECTOR &&
+		    run->debug.arch.pc == CAST_TO_RIP(sw_bp),
+		    "INT3: exit %d exception %d rip 0x%llx (should be 0x%llx)",
+		    run->exit_reason, run->debug.arch.exception,
+		    run->debug.arch.pc, CAST_TO_RIP(sw_bp));
+	MOVE_RIP(1);
+
+	/* Test instruction HW BP over DR[0-3] */
+	for (i = 0; i < 4; i++) {
+		CLEAR_DEBUG();
+		debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
+		debug.arch.debugreg[i] = CAST_TO_RIP(hw_bp);
+		debug.arch.debugreg[7] = 0x400 | (1UL << (2*i+1));
+		APPLY_DEBUG();
+		vcpu_run(vm, VCPU_ID);
+		target_dr6 = 0xffff0ff0 | (1UL << i);
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG &&
+			    run->debug.arch.exception == DB_VECTOR &&
+			    run->debug.arch.pc == CAST_TO_RIP(hw_bp) &&
+			    run->debug.arch.dr6 == target_dr6,
+			    "INS_HW_BP (DR%d): exit %d exception %d rip 0x%llx "
+			    "(should be 0x%llx) dr6 0x%llx (should be 0x%llx)",
+			    i, run->exit_reason, run->debug.arch.exception,
+			    run->debug.arch.pc, CAST_TO_RIP(hw_bp),
+			    run->debug.arch.dr6, target_dr6);
+	}
+	/* Skip "nop" */
+	MOVE_RIP(1);
+
+	/* Test data access HW BP over DR[0-3] */
+	for (i = 0; i < 4; i++) {
+		CLEAR_DEBUG();
+		debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
+		debug.arch.debugreg[i] = CAST_TO_RIP(guest_value);
+		debug.arch.debugreg[7] = 0x00000400 | (1UL << (2*i+1)) |
+		    (0x000d0000UL << (4*i));
+		APPLY_DEBUG();
+		vcpu_run(vm, VCPU_ID);
+		target_dr6 = 0xffff0ff0 | (1UL << i);
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG &&
+			    run->debug.arch.exception == DB_VECTOR &&
+			    run->debug.arch.pc == CAST_TO_RIP(write_data) &&
+			    run->debug.arch.dr6 == target_dr6,
+			    "DATA_HW_BP (DR%d): exit %d exception %d rip 0x%llx "
+			    "(should be 0x%llx) dr6 0x%llx (should be 0x%llx)",
+			    i, run->exit_reason, run->debug.arch.exception,
+			    run->debug.arch.pc, CAST_TO_RIP(write_data),
+			    run->debug.arch.dr6, target_dr6);
+		/* Rollback the 4-bytes "mov" */
+		MOVE_RIP(-7);
+	}
+	/* Skip the 4-bytes "mov" */
+	MOVE_RIP(7);
+
+	/* Test single step */
+	target_rip = CAST_TO_RIP(ss_start);
+	target_dr6 = 0xffff4ff0ULL;
+	vcpu_regs_get(vm, VCPU_ID, &regs);
+	for (i = 0; i < (sizeof(ss_size) / sizeof(ss_size[0])); i++) {
+		target_rip += ss_size[i];
+		CLEAR_DEBUG();
+		debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+		debug.arch.debugreg[7] = 0x00000400;
+		APPLY_DEBUG();
+		vcpu_run(vm, VCPU_ID);
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG &&
+			    run->debug.arch.exception == DB_VECTOR &&
+			    run->debug.arch.pc == target_rip &&
+			    run->debug.arch.dr6 == target_dr6,
+			    "SINGLE_STEP[%d]: exit %d exception %d rip 0x%llx "
+			    "(should be 0x%llx) dr6 0x%llx (should be 0x%llx)",
+			    i, run->exit_reason, run->debug.arch.exception,
+			    run->debug.arch.pc, target_rip, run->debug.arch.dr6,
+			    target_dr6);
+	}
+
+	/* Disable all debug controls, run to the end */
+	CLEAR_DEBUG();
+	APPLY_DEBUG();
+
+	vcpu_run(vm, VCPU_ID);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO, "KVM_EXIT_IO");
+	cmd = get_ucall(vm, VCPU_ID, &uc);
+	TEST_ASSERT(cmd == UCALL_DONE, "UCALL_DONE");
+
+	kvm_vm_free(vm);
+
+	return 0;
+}
-- 
2.18.2

[PATCH 6/9] 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 38f6aeefeb55..5627004e077e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1674,7 +1674,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)
@@ -1693,7 +1693,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;
@@ -1739,6 +1739,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 7/9] 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 8c247bcb037e..93f6f696d059 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1093,7 +1093,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 5627004e077e..f03bffafd9e6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1672,11 +1672,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);
@@ -3932,7 +3927,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 2384a2dbec44..e2b71b0cdfce 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4965,15 +4965,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);
@@ -7736,8 +7727,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 f7628555f036..f4254d716b10 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1050,7 +1050,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);
 }
 
@@ -1129,10 +1130,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

[PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Paolo Bonzini]

On Intel, #DB exceptions transmit the DR6 value via the exit qualification
field of the VMCS, and the exit qualification only contains the description
of the precise event that caused a vmexit.

On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
was to be injected into the guest.  This has two effects when guest debugging
is in use:

* the guest DR6 is clobbered

* the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
than just the last one that happened.

Fortunately, if guest debugging is in use we debug register reads and writes
are always intercepted.  Now that the guest DR6 is always synchronized with
vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
debugging is enabled, and restore the guest value when it is disabled.  This
fixes both problems.

A testcase for the second issue is added in the next patch.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/svm.c |  2 ++
 arch/x86/kvm/x86.c     | 12 ++++++++----
 arch/x86/kvm/x86.h     |  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f03bffafd9e6..29dc7311dbb1 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1750,6 +1750,8 @@ static int db_interception(struct vcpu_svm *svm)
 		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;
+		/* This restores DR6 to all zeros.  */
+		kvm_update_dr6(vcpu);
 		kvm_run->debug.arch.pc =
 			svm->vmcb->save.cs.base + svm->vmcb->save.rip;
 		kvm_run->debug.arch.exception = DB_VECTOR;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f4254d716b10..1b5e0fc346bb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -104,7 +104,6 @@ static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
                                     KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
 
 static void update_cr8_intercept(struct kvm_vcpu *vcpu);
-static void kvm_update_dr6(struct kvm_vcpu *vcpu);
 static void process_nmi(struct kvm_vcpu *vcpu);
 static void enter_smm(struct kvm_vcpu *vcpu);
 static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
@@ -1048,10 +1047,14 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
 	}
 }
 
-static void kvm_update_dr6(struct kvm_vcpu *vcpu)
+void kvm_update_dr6(struct kvm_vcpu *vcpu)
 {
-	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
-	    kvm_x86_ops.set_dr6)
+	if (!kvm_x86_ops.set_dr6)
+		return;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		kvm_x86_ops.set_dr6(vcpu, DR6_FIXED_1 | DR6_RTM);
+	else
 		kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6);
 }
 
@@ -9154,6 +9157,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 		for (i = 0; i < KVM_NR_DB_REGS; i++)
 			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
 	}
+	kvm_update_dr6(vcpu);
 	kvm_update_dr7(vcpu);
 
 	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b968acc0516f..a4c950ad4d60 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -240,6 +240,8 @@ static inline bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu)
 	return is_smm(vcpu) || kvm_x86_ops.apic_init_signal_blocked(vcpu);
 }
 
+void kvm_update_dr6(struct kvm_vcpu *vcpu);
+
 void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
 void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
 
-- 
2.18.2

[PATCH 9/9] KVM: VMX: pass correct DR6 for GD userspace exit

[Paolo Bonzini]

When KVM_EXIT_DEBUG is raised for the disabled-breakpoints case (DR7.GD),
DR6 was incorrectly copied from the value in the VM.  Instead,
DR6.BD should be set in order to catch this case.

On AMD this does not need any special code because the processor triggers
a #DB exception that is intercepted.  However, the testcase would fail
without the previous patch because both DR6.BS and DR6.BD would be set.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c                        |  2 +-
 .../testing/selftests/kvm/x86_64/debug_regs.c | 24 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e2b71b0cdfce..e45cf89c5821 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4927,7 +4927,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		 * guest debugging itself.
 		 */
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
-			vcpu->run->debug.arch.dr6 = vcpu->arch.dr6;
+			vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1;
 			vcpu->run->debug.arch.dr7 = dr7;
 			vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu);
 			vcpu->run->debug.arch.exception = DB_VECTOR;
diff --git a/tools/testing/selftests/kvm/x86_64/debug_regs.c b/tools/testing/selftests/kvm/x86_64/debug_regs.c
index 077f25d61d1a..52796138b1f3 100644
--- a/tools/testing/selftests/kvm/x86_64/debug_regs.c
+++ b/tools/testing/selftests/kvm/x86_64/debug_regs.c
@@ -11,10 +11,13 @@
 
 #define VCPU_ID 0
 
+#define DR6_BD		(1 << 13)
+#define DR7_GD		(1 << 13)
+
 /* For testing data access debug BP */
 uint32_t guest_value;
 
-extern unsigned char sw_bp, hw_bp, write_data, ss_start;
+extern unsigned char sw_bp, hw_bp, write_data, ss_start, bd_start;
 
 static void guest_code(void)
 {
@@ -43,6 +46,8 @@ static void guest_code(void)
 		     "rdmsr\n\t"
 		     : : : "rax", "ecx");
 
+	/* DR6.BD test */
+	asm volatile("bd_start: mov %%dr0, %%rax" : : : "rax");
 	GUEST_DONE();
 }
 
@@ -165,6 +170,23 @@ int main(void)
 			    target_dr6);
 	}
 
+	/* Finally test global disable */
+	CLEAR_DEBUG();
+	debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
+	debug.arch.debugreg[7] = 0x400 | DR7_GD;
+	APPLY_DEBUG();
+	vcpu_run(vm, VCPU_ID);
+	target_dr6 = 0xffff0ff0 | DR6_BD;
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG &&
+		    run->debug.arch.exception == DB_VECTOR &&
+		    run->debug.arch.pc == CAST_TO_RIP(bd_start) &&
+		    run->debug.arch.dr6 == target_dr6,
+			    "DR7.GD: exit %d exception %d rip 0x%llx "
+			    "(should be 0x%llx) dr6 0x%llx (should be 0x%llx)",
+			    run->exit_reason, run->debug.arch.exception,
+			    run->debug.arch.pc, target_rip, run->debug.arch.dr6,
+			    target_dr6);
+
 	/* Disable all debug controls, run to the end */
 	CLEAR_DEBUG();
 	APPLY_DEBUG();
-- 
2.18.2

Re: [PATCH 2/9] KVM: x86: fix DR6 delivery for various cases of #DB injection

[Peter Xu]

On Wed, May 06, 2020 at 07:10:27AM -0400, Paolo Bonzini wrote:
> Go through kvm_queue_exception_p so that the payload is correctly delivered
> through the exit qualification, and add a kvm_update_dr6 call to
> kvm_deliver_exception_payload that is needed on AMD.
> 
> Reported-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 6/9] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6

[Peter Xu]

On Wed, May 06, 2020 at 07:10:31AM -0400, Paolo Bonzini wrote:
> 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>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 7/9] KVM: x86: simplify dr6 accessors in kvm_x86_ops

[Peter Xu]

On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote:
> 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>

(I think this patch and the previous one seem to be the same as the previous
 version.  Anyway...)

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 7/9] KVM: x86: simplify dr6 accessors in kvm_x86_ops

[Paolo Bonzini]

On 06/05/20 18:06, Peter Xu wrote:
> On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote:
>> 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>
> (I think this patch and the previous one seem to be the same as the previous
>  version.  Anyway...)

Yes, I placed them here because they are needed to solve the SVM bugs in
patch 8.  Sorry for not adding your Reviewed-by.

Paolo

> Reviewed-by: Peter Xu <peterx@redhat.com>

Re: [PATCH 9/9] KVM: VMX: pass correct DR6 for GD userspace exit

[Peter Xu]

On Wed, May 06, 2020 at 07:10:34AM -0400, Paolo Bonzini wrote:
> When KVM_EXIT_DEBUG is raised for the disabled-breakpoints case (DR7.GD),
> DR6 was incorrectly copied from the value in the VM.  Instead,
> DR6.BD should be set in order to catch this case.
> 
> On AMD this does not need any special code because the processor triggers
> a #DB exception that is intercepted.  However, the testcase would fail
> without the previous patch because both DR6.BS and DR6.BD would be set.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 7/9] KVM: x86: simplify dr6 accessors in kvm_x86_ops

[Peter Xu]

On Wed, May 06, 2020 at 06:09:23PM +0200, Paolo Bonzini wrote:
> On 06/05/20 18:06, Peter Xu wrote:
> > On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote:
> >> 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>
> > (I think this patch and the previous one seem to be the same as the previous
> >  version.  Anyway...)
> 
> Yes, I placed them here because they are needed to solve the SVM bugs in
> patch 8.  Sorry for not adding your Reviewed-by.

That's not a problem to me. :) Instead I'm more afraid of not noticing some
trivial difference in the patch comparing to the last one.

Thanks,

-- 
Peter Xu

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Peter Xu]

On Wed, May 06, 2020 at 07:10:33AM -0400, Paolo Bonzini wrote:
> On Intel, #DB exceptions transmit the DR6 value via the exit qualification
> field of the VMCS, and the exit qualification only contains the description
> of the precise event that caused a vmexit.
> 
> On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
> was to be injected into the guest.  This has two effects when guest debugging
> is in use:
> 
> * the guest DR6 is clobbered
> 
> * the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
> than just the last one that happened.
> 
> Fortunately, if guest debugging is in use we debug register reads and writes
> are always intercepted.  Now that the guest DR6 is always synchronized with
> vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
> debugging is enabled, and restore the guest value when it is disabled.  This
> fixes both problems.
> 
> A testcase for the second issue is added in the next patch.

Is there supposed to be another test after this one, or the GD test?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/svm.c |  2 ++
>  arch/x86/kvm/x86.c     | 12 ++++++++----
>  arch/x86/kvm/x86.h     |  2 ++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f03bffafd9e6..29dc7311dbb1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1750,6 +1750,8 @@ static int db_interception(struct vcpu_svm *svm)
>  		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;

[1]

> +		/* This restores DR6 to all zeros.  */
> +		kvm_update_dr6(vcpu);

I feel like it won't work as expected for KVM_GUESTDBG_SINGLESTEP, because at
[2] below it'll go to the "else" instead so dr6 seems won't be cleared in that
case.

Another concern I have is that, I mostly read kvm_update_dr6() as "apply the
dr6 memory cache --> VMCB".  I'm worried this might confuse people (at least I
used quite a few minutes to digest...) here because latest data should already
be in the VMCB.

Also, IMHO it would be fine to have invalid dr6 values during
KVM_SET_GUEST_DEBUG.  I'm not sure whether my understanding is correct, but I
see KVM_SET_GUEST_DEBUG needs to override the in-guest debug completely.  If we
worry about dr6 being incorrect after KVM_SET_GUEST_DEBUG is disabled, IMHO we
can reset dr6 in kvm_arch_vcpu_ioctl_set_guest_debug() properly before we
return the debug registers to the guest.

PS. I cannot see above lines [1] in my local tree (which seems to be really a
bugfix...).  I tried to use kvm/queue just in case I missed some patches, but I
still didn't see them.  So am I reading the wrong tree here?

Thanks,

>  		kvm_run->debug.arch.pc =
>  			svm->vmcb->save.cs.base + svm->vmcb->save.rip;
>  		kvm_run->debug.arch.exception = DB_VECTOR;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f4254d716b10..1b5e0fc346bb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -104,7 +104,6 @@ static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
>                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
>  
>  static void update_cr8_intercept(struct kvm_vcpu *vcpu);
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu);
>  static void process_nmi(struct kvm_vcpu *vcpu);
>  static void enter_smm(struct kvm_vcpu *vcpu);
>  static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
> @@ -1048,10 +1047,14 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu)
> +void kvm_update_dr6(struct kvm_vcpu *vcpu)
>  {
> -	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
> -	    kvm_x86_ops.set_dr6)
> +	if (!kvm_x86_ops.set_dr6)
> +		return;
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)

[2]

> +		kvm_x86_ops.set_dr6(vcpu, DR6_FIXED_1 | DR6_RTM);
> +	else
>  		kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6);
>  }
>  
> @@ -9154,6 +9157,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		for (i = 0; i < KVM_NR_DB_REGS; i++)
>  			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
>  	}
> +	kvm_update_dr6(vcpu);
>  	kvm_update_dr7(vcpu);
>  
>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b968acc0516f..a4c950ad4d60 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -240,6 +240,8 @@ static inline bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu)
>  	return is_smm(vcpu) || kvm_x86_ops.apic_init_signal_blocked(vcpu);
>  }
>  
> +void kvm_update_dr6(struct kvm_vcpu *vcpu);
> +
>  void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
>  void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
>  
> -- 
> 2.18.2
> 
> 

-- 
Peter Xu

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Paolo Bonzini]

On 06/05/20 20:15, Peter Xu wrote:
> On Wed, May 06, 2020 at 07:10:33AM -0400, Paolo Bonzini wrote:
>> On Intel, #DB exceptions transmit the DR6 value via the exit qualification
>> field of the VMCS, and the exit qualification only contains the description
>> of the precise event that caused a vmexit.
>>
>> On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
>> was to be injected into the guest.  This has two effects when guest debugging
>> is in use:
>>
>> * the guest DR6 is clobbered
>>
>> * the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
>> than just the last one that happened.
>>
>> Fortunately, if guest debugging is in use we debug register reads and writes
>> are always intercepted.  Now that the guest DR6 is always synchronized with
>> vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
>> debugging is enabled, and restore the guest value when it is disabled.  This
>> fixes both problems.
>>
>> A testcase for the second issue is added in the next patch.
> 
> Is there supposed to be another test after this one, or the GD test?

It's the GD test.
>> +		/* This restores DR6 to all zeros.  */
>> +		kvm_update_dr6(vcpu);
> 
> I feel like it won't work as expected for KVM_GUESTDBG_SINGLESTEP, because at
> [2] below it'll go to the "else" instead so dr6 seems won't be cleared in that
> case.

You're right, I need to cover both cases that trigger #DB.

> Another concern I have is that, I mostly read kvm_update_dr6() as "apply the
> dr6 memory cache --> VMCB".  I'm worried this might confuse people (at least I
> used quite a few minutes to digest...) here because latest data should already
> be in the VMCB.

No, the latest guest register is always in vcpu->arch.dr6.  It's only
because of KVM_DEBUGREG_WONT_EXIT that kvm_update_dr6() needs to pass
vcpu->arch.dr6 to kvm_x86_ops.set_dr6.  Actually this patch could even
check KVM_DEBUGREG_WONT_EXIT instead of vcpu->guest_debug.  I'll take a
look tomorrow.

> Also, IMHO it would be fine to have invalid dr6 values during
> KVM_SET_GUEST_DEBUG.  I'm not sure whether my understanding is correct, but I
> see KVM_SET_GUEST_DEBUG needs to override the in-guest debug completely.

Sort of, userspace can try to juggle host and guest debugging (this is
why you have KVM_GUESTDBG_INJECT_DB and KVM_GUESTDBG_INJECT_BP).

> If we worry about dr6 being incorrect after KVM_SET_GUEST_DEBUG is disabled,
> IMHO we can reset dr6 in kvm_arch_vcpu_ioctl_set_guest_debug() properly before
> we return the debug registers to the guest.
> 
> PS. I cannot see above lines [1] in my local tree (which seems to be really a
> bugfix...).  I tried to use kvm/queue just in case I missed some patches, but I
> still didn't see them.  So am I reading the wrong tree here?

The patch is based on kvm/master, and indeed that line is from a bugfix
that I've posted yesterday ("KVM: SVM: fill in
kvm_run->debug.arch.dr[67]"). I had pushed that one right away, because
it was quite obviously suitable for 5.7.

Paolo

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Peter Xu]

On Wed, May 06, 2020 at 10:07:15PM +0200, Paolo Bonzini wrote:
> On 06/05/20 20:15, Peter Xu wrote:
> > On Wed, May 06, 2020 at 07:10:33AM -0400, Paolo Bonzini wrote:
> >> On Intel, #DB exceptions transmit the DR6 value via the exit qualification
> >> field of the VMCS, and the exit qualification only contains the description
> >> of the precise event that caused a vmexit.
> >>
> >> On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
> >> was to be injected into the guest.  This has two effects when guest debugging
> >> is in use:
> >>
> >> * the guest DR6 is clobbered
> >>
> >> * the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
> >> than just the last one that happened.
> >>
> >> Fortunately, if guest debugging is in use we debug register reads and writes
> >> are always intercepted.  Now that the guest DR6 is always synchronized with
> >> vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
> >> debugging is enabled, and restore the guest value when it is disabled.  This
> >> fixes both problems.
> >>
> >> A testcase for the second issue is added in the next patch.
> > 
> > Is there supposed to be another test after this one, or the GD test?
> 
> It's the GD test.

Oh... so is dr6 going to have some leftover bit set in the GD test if without
this patch for AMD?  Btw, I noticed a small difference on Intel/AMD spec for
this case, e.g., B[0-3] definitions on such leftover bits...

Intel says:

        B0 through B3 (breakpoint condition detected) flags (bits 0 through 3)
        — Indicates (when set) that its associated breakpoint condition was met
        when a debug exception was generated. These flags are set if the
        condition described for each breakpoint by the LENn, and R/Wn flags in
        debug control register DR7 is true. They may or may not be set if the
        breakpoint is not enabled by the Ln or the Gn flags in register
        DR7. Therefore on a #DB, a debug handler should check only those B0-B3
        bits which correspond to an enabled breakpoint.

AMD says:

        Breakpoint-Condition Detected (B3–B0)—Bits 3:0. The processor updates
        these four bits on every debug breakpoint or general-detect
        condition. A bit is set to 1 if the corresponding address- breakpoint
        register detects an enabled breakpoint condition, as specified by the
        DR7 Ln, Gn, R/Wn and LENn controls, and is cleared to 0 otherwise. For
        example, B1 (bit 1) is set to 1 if an address- breakpoint condition is
        detected by DR1.

I'm not sure whether it means AMD B[0-3] bits are more strict on the Intel ones
(if so, then the selftest could be a bit too strict to VMX).

> >> +		/* This restores DR6 to all zeros.  */
> >> +		kvm_update_dr6(vcpu);
> > 
> > I feel like it won't work as expected for KVM_GUESTDBG_SINGLESTEP, because at
> > [2] below it'll go to the "else" instead so dr6 seems won't be cleared in that
> > case.
> 
> You're right, I need to cover both cases that trigger #DB.
> 
> > Another concern I have is that, I mostly read kvm_update_dr6() as "apply the
> > dr6 memory cache --> VMCB".  I'm worried this might confuse people (at least I
> > used quite a few minutes to digest...) here because latest data should already
> > be in the VMCB.
> 
> No, the latest guest register is always in vcpu->arch.dr6.  It's only
> because of KVM_DEBUGREG_WONT_EXIT that kvm_update_dr6() needs to pass
> vcpu->arch.dr6 to kvm_x86_ops.set_dr6.  Actually this patch could even
> check KVM_DEBUGREG_WONT_EXIT instead of vcpu->guest_debug.  I'll take a
> look tomorrow.

OK.

> 
> > Also, IMHO it would be fine to have invalid dr6 values during
> > KVM_SET_GUEST_DEBUG.  I'm not sure whether my understanding is correct, but I
> > see KVM_SET_GUEST_DEBUG needs to override the in-guest debug completely.
> 
> Sort of, userspace can try to juggle host and guest debugging (this is
> why you have KVM_GUESTDBG_INJECT_DB and KVM_GUESTDBG_INJECT_BP).

I see!

> 
> > If we worry about dr6 being incorrect after KVM_SET_GUEST_DEBUG is disabled,
> > IMHO we can reset dr6 in kvm_arch_vcpu_ioctl_set_guest_debug() properly before
> > we return the debug registers to the guest.
> > 
> > PS. I cannot see above lines [1] in my local tree (which seems to be really a
> > bugfix...).  I tried to use kvm/queue just in case I missed some patches, but I
> > still didn't see them.  So am I reading the wrong tree here?
> 
> The patch is based on kvm/master, and indeed that line is from a bugfix
> that I've posted yesterday ("KVM: SVM: fill in
> kvm_run->debug.arch.dr[67]"). I had pushed that one right away, because
> it was quite obviously suitable for 5.7.

Oh that's why it looks very familiar (because I read that patch.. :).  Then it
makes sense now.  Thanks!

-- 
Peter Xu

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Sean Christopherson]

On Wed, May 06, 2020 at 05:13:56PM -0400, Peter Xu wrote:
> Oh... so is dr6 going to have some leftover bit set in the GD test if without
> this patch for AMD?  Btw, I noticed a small difference on Intel/AMD spec for
> this case, e.g., B[0-3] definitions on such leftover bits...
> 
> Intel says:
> 
>         B0 through B3 (breakpoint condition detected) flags (bits 0 through 3)
>         — Indicates (when set) that its associated breakpoint condition was met
>         when a debug exception was generated. These flags are set if the
>         condition described for each breakpoint by the LENn, and R/Wn flags in
>         debug control register DR7 is true. They may or may not be set if the
>         breakpoint is not enabled by the Ln or the Gn flags in register
>         DR7. Therefore on a #DB, a debug handler should check only those B0-B3
>         bits which correspond to an enabled breakpoint.
> 
> AMD says:
> 
>         Breakpoint-Condition Detected (B3–B0)—Bits 3:0. The processor updates
>         these four bits on every debug breakpoint or general-detect
>         condition. A bit is set to 1 if the corresponding address- breakpoint
>         register detects an enabled breakpoint condition, as specified by the
>         DR7 Ln, Gn, R/Wn and LENn controls, and is cleared to 0 otherwise. For
>         example, B1 (bit 1) is set to 1 if an address- breakpoint condition is
>         detected by DR1.
> 
> I'm not sure whether it means AMD B[0-3] bits are more strict on the Intel ones
> (if so, then the selftest could be a bit too strict to VMX).

If the question is "can DR6 bits 3:0 be set on Intel CPUs even if the
associated breakpoint is disabled?", then the answer is yes.  I haven't
looked at the selftest, but if it's checking DR6 then it should ignore
bits corresponding to disabled breakpoints.

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Peter Xu]

On Wed, May 06, 2020 at 02:20:47PM -0700, Sean Christopherson wrote:
> On Wed, May 06, 2020 at 05:13:56PM -0400, Peter Xu wrote:
> > Oh... so is dr6 going to have some leftover bit set in the GD test if without
> > this patch for AMD?  Btw, I noticed a small difference on Intel/AMD spec for
> > this case, e.g., B[0-3] definitions on such leftover bits...
> > 
> > Intel says:
> > 
> >         B0 through B3 (breakpoint condition detected) flags (bits 0 through 3)
> >         — Indicates (when set) that its associated breakpoint condition was met
> >         when a debug exception was generated. These flags are set if the
> >         condition described for each breakpoint by the LENn, and R/Wn flags in
> >         debug control register DR7 is true. They may or may not be set if the
> >         breakpoint is not enabled by the Ln or the Gn flags in register
> >         DR7. Therefore on a #DB, a debug handler should check only those B0-B3
> >         bits which correspond to an enabled breakpoint.
> > 
> > AMD says:
> > 
> >         Breakpoint-Condition Detected (B3–B0)—Bits 3:0. The processor updates
> >         these four bits on every debug breakpoint or general-detect
> >         condition. A bit is set to 1 if the corresponding address- breakpoint
> >         register detects an enabled breakpoint condition, as specified by the
> >         DR7 Ln, Gn, R/Wn and LENn controls, and is cleared to 0 otherwise. For
> >         example, B1 (bit 1) is set to 1 if an address- breakpoint condition is
> >         detected by DR1.
> > 
> > I'm not sure whether it means AMD B[0-3] bits are more strict on the Intel ones
> > (if so, then the selftest could be a bit too strict to VMX).
> 
> If the question is "can DR6 bits 3:0 be set on Intel CPUs even if the
> associated breakpoint is disabled?", then the answer is yes.  I haven't
> looked at the selftest, but if it's checking DR6 then it should ignore
> bits corresponding to disabled breakpoints.

Currently the selftest will also check that the B[0-3] is cleared when specific
BP is disabled.  We can loose that.  The thing is this same test can also run
on AMD hosts, so logically on the other hand we should still check those bits
as cleared to follow the AMD spec (and it never failed for me even on Intel..).

Thanks,

-- 
Peter Xu

Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Sean Christopherson]

On Wed, May 06, 2020 at 07:33:06PM -0400, Peter Xu wrote:
> On Wed, May 06, 2020 at 02:20:47PM -0700, Sean Christopherson wrote:
> > On Wed, May 06, 2020 at 05:13:56PM -0400, Peter Xu wrote:
> > > Oh... so is dr6 going to have some leftover bit set in the GD test if without
> > > this patch for AMD?  Btw, I noticed a small difference on Intel/AMD spec for
> > > this case, e.g., B[0-3] definitions on such leftover bits...
> > > 
> > > Intel says:
> > > 
> > >         B0 through B3 (breakpoint condition detected) flags (bits 0 through 3)
> > >         — Indicates (when set) that its associated breakpoint condition was met
> > >         when a debug exception was generated. These flags are set if the
> > >         condition described for each breakpoint by the LENn, and R/Wn flags in
> > >         debug control register DR7 is true. They may or may not be set if the
> > >         breakpoint is not enabled by the Ln or the Gn flags in register
> > >         DR7. Therefore on a #DB, a debug handler should check only those B0-B3
> > >         bits which correspond to an enabled breakpoint.
> > > 
> > > AMD says:
> > > 
> > >         Breakpoint-Condition Detected (B3–B0)—Bits 3:0. The processor updates
> > >         these four bits on every debug breakpoint or general-detect
> > >         condition. A bit is set to 1 if the corresponding address- breakpoint
> > >         register detects an enabled breakpoint condition, as specified by the
> > >         DR7 Ln, Gn, R/Wn and LENn controls, and is cleared to 0 otherwise. For
> > >         example, B1 (bit 1) is set to 1 if an address- breakpoint condition is
> > >         detected by DR1.
> > > 
> > > I'm not sure whether it means AMD B[0-3] bits are more strict on the Intel ones
> > > (if so, then the selftest could be a bit too strict to VMX).
> > 
> > If the question is "can DR6 bits 3:0 be set on Intel CPUs even if the
> > associated breakpoint is disabled?", then the answer is yes.  I haven't
> > looked at the selftest, but if it's checking DR6 then it should ignore
> > bits corresponding to disabled breakpoints.
> 
> Currently the selftest will also check that the B[0-3] is cleared when specific
> BP is disabled.  We can loose that.  The thing is this same test can also run
> on AMD hosts, so logically on the other hand we should still check those bits
> as cleared to follow the AMD spec (and it never failed for me even on Intel..).

There still has to be an address and type match, e.g. if the DRs are
completely bogus in the selftest then the "cleard to zero" check is still
valid.  And if I'm reading the selftest code correctly, this is indeed the
case as only the DRn being tested is configured.