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

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

[Paolo Bonzini]

This new version of the patches improves the AMD bugfix where
KVM_EXIT_DEBUG clobbers the guest DR6 and includes stale causes.
The improved fix makes it possible to drop kvm_set_dr6 and
kvm_update_dr6 altogether.

v1->v2: - merge v1 patch 6 with get_dr6 part of v1 patch 7, cover nested SVM
	- new patch "KVM: nSVM: trap #DB and #BP to userspace if guest debugging is on"
	- rewritten patch 8 to get rid of set_dr6 completely

Paolo Bonzini (5):
  KVM: x86: fix DR6 delivery for various cases of #DB injection
  KVM: nSVM: trap #DB and #BP to userspace if guest debugging is on
  KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6
  KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6
  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               |   3 +-
 arch/x86/kvm/svm/nested.c                     |  39 +++-
 arch/x86/kvm/svm/svm.c                        |  36 ++--
 arch/x86/kvm/vmx/vmx.c                        |  23 +-
 arch/x86/kvm/x86.c                            |  27 +--
 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, 281 insertions(+), 63 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/debug_regs.c

-- 
2.18.2

[PATCH v2 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 v2 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>
Reviewed-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 v2 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 v2 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 v2 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 v2 6/9] KVM: nSVM: trap #DB and #BP to userspace if guest debugging is on

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 90a1ca939627..adab5b1c5fe1 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -608,6 +608,11 @@ static int nested_svm_intercept_db(struct vcpu_svm *svm)
 {
 	unsigned long dr6;
 
+	/* Always catch it and pass it to userspace if debugging the guest.  */
+	if (svm->vcpu.guest_debug &
+	    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
+		return NESTED_EXIT_HOST;
+
 	/* if we're not singlestepping, it's not ours */
 	if (!svm->nmi_singlestep)
 		return NESTED_EXIT_DONE;
@@ -682,6 +687,9 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 		if (svm->nested.intercept_exceptions & excp_bits) {
 			if (exit_code == SVM_EXIT_EXCP_BASE + DB_VECTOR)
 				vmexit = nested_svm_intercept_db(svm);
+			else if (exit_code == SVM_EXIT_EXCP_BASE + BP_VECTOR &&
+				 svm->vcpu.guest_debug & KVM_GUESTDBG_USE_SW_BP)
+				vmexit = NESTED_EXIT_HOST;
 			else
 				vmexit = NESTED_EXIT_DONE;
 		}
-- 
2.18.2

Re: [PATCH v2 6/9] KVM: nSVM: trap #DB and #BP to userspace if guest debugging is on

[Peter Xu]

On Thu, May 07, 2020 at 07:50:08AM -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

-- 
Peter Xu

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

[Paolo Bonzini]

kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the
second argument.  Ensure that the VMCB value is synchronized to
vcpu->arch.dr6 on #DB (both "normal" and nested), so that the current
value of DR6 is always available in vcpu->arch.dr6.  The get_dr6 callback
can just access vcpu->arch.dr6 and becomes redundant.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/svm/nested.c       | 23 +++++++++++++++--------
 arch/x86/kvm/svm/svm.c          |  9 ++-------
 arch/x86/kvm/vmx/vmx.c          |  6 ------
 arch/x86/kvm/x86.c              |  5 +----
 5 files changed, 18 insertions(+), 26 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/nested.c b/arch/x86/kvm/svm/nested.c
index adab5b1c5fe1..1a547e3ac0e5 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 
 #include <asm/msr-index.h>
+#include <asm/debugreg.h>
 
 #include "kvm_emulate.h"
 #include "trace.h"
@@ -267,7 +268,7 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
 	svm->vmcb->save.rsp = nested_vmcb->save.rsp;
 	svm->vmcb->save.rip = nested_vmcb->save.rip;
 	svm->vmcb->save.dr7 = nested_vmcb->save.dr7;
-	svm->vmcb->save.dr6 = nested_vmcb->save.dr6;
+	svm->vcpu.arch.dr6  = nested_vmcb->save.dr6;
 	svm->vmcb->save.cpl = nested_vmcb->save.cpl;
 
 	svm->nested.vmcb_msrpm = nested_vmcb->control.msrpm_base_pa & ~0x0fffULL;
@@ -482,7 +483,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	nested_vmcb->save.rsp    = vmcb->save.rsp;
 	nested_vmcb->save.rax    = vmcb->save.rax;
 	nested_vmcb->save.dr7    = vmcb->save.dr7;
-	nested_vmcb->save.dr6    = vmcb->save.dr6;
+	nested_vmcb->save.dr6    = svm->vcpu.arch.dr6;
 	nested_vmcb->save.cpl    = vmcb->save.cpl;
 
 	nested_vmcb->control.int_ctl           = vmcb->control.int_ctl;
@@ -606,7 +607,7 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 /* DB exceptions for our internal use must not cause vmexit */
 static int nested_svm_intercept_db(struct vcpu_svm *svm)
 {
-	unsigned long dr6;
+	unsigned long dr6 = svm->vmcb->save.dr6;
 
 	/* Always catch it and pass it to userspace if debugging the guest.  */
 	if (svm->vcpu.guest_debug &
@@ -615,22 +616,28 @@ static int nested_svm_intercept_db(struct vcpu_svm *svm)
 
 	/* if we're not singlestepping, it's not ours */
 	if (!svm->nmi_singlestep)
-		return NESTED_EXIT_DONE;
+		goto reflected_db;
 
 	/* if it's not a singlestep exception, it's not ours */
-	if (kvm_get_dr(&svm->vcpu, 6, &dr6))
-		return NESTED_EXIT_DONE;
 	if (!(dr6 & DR6_BS))
-		return NESTED_EXIT_DONE;
+		goto reflected_db;
 
 	/* if the guest is singlestepping, it should get the vmexit */
 	if (svm->nmi_singlestep_guest_rflags & X86_EFLAGS_TF) {
 		disable_nmi_singlestep(svm);
-		return NESTED_EXIT_DONE;
+		goto reflected_db;
 	}
 
 	/* it's ours, the nested hypervisor must not see this one */
 	return NESTED_EXIT_HOST;
+
+reflected_db:
+	/*
+	 * Synchronize guest DR6 here just like in db_interception; it will
+	 * be moved into the nested VMCB by nested_svm_vmexit.
+	 */
+	svm->vcpu.arch.dr6 = dr6;
+	return NESTED_EXIT_DONE;
 }
 
 static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 38f6aeefeb55..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 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);
@@ -1693,7 +1688,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 +1734,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;
 	}
@@ -3931,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..6153a47109d3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4965,11 +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)
 {
 }
@@ -7736,7 +7731,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,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f7628555f036..b1b92d904f37 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1129,10 +1129,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 v2 7/9] KVM: SVM: keep DR6 synchronized with vcpu->arch.dr6

[Peter Xu]

On Thu, May 07, 2020 at 07:50:09AM -0400, Paolo Bonzini wrote:
> @@ -267,7 +268,7 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa,
>  	svm->vmcb->save.rsp = nested_vmcb->save.rsp;
>  	svm->vmcb->save.rip = nested_vmcb->save.rip;
>  	svm->vmcb->save.dr7 = nested_vmcb->save.dr7;
> -	svm->vmcb->save.dr6 = nested_vmcb->save.dr6;
> +	svm->vcpu.arch.dr6  = nested_vmcb->save.dr6;

The rest looks very sane to me, but here I failed to figure out how arch.dr6
finally applied to save.dr6.  I saw it is applied in svm_vcpu_run() in the next
patch, but if that's the case then iiuc this commit may break bisection. Thanks,

-- 
Peter Xu

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

[Paolo Bonzini]

On 07/05/20 20:22, Peter Xu wrote:
>> -	svm->vmcb->save.dr6 = nested_vmcb->save.dr6;
>> +	svm->vcpu.arch.dr6  = nested_vmcb->save.dr6;
> The rest looks very sane to me, but here I failed to figure out how arch.dr6
> finally applied to save.dr6.  I saw it is applied in svm_vcpu_run() in the next
> patch, but if that's the case then iiuc this commit may break bisection. Thanks,

You're right, this needs a call to kvm_update_dr6 (which would go away
on the next patch).

Paolo

[PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Paolo Bonzini]

There are two issues with KVM_EXIT_DEBUG on AMD, whose root cause is the
different handling of DR6 on intercepted #DB exceptions on Intel and AMD.

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 (the testcase in the next patch covers
this issue).

This patch fixes both issues by emulating, so to speak, the Intel behavior
on AMD processors.  The important observation is that (after the previous
patches) the VMCB value of DR6 is only ever observable from the guest is
KVM_DEBUGREG_WONT_EXIT is set.  Therefore we can actually set vmcb->save.dr6
to any value we want as long as KVM_DEBUGREG_WONT_EXIT is clear, which it
will be if guest debugging is enabled.

Therefore it is possible to enter the guest with an all-zero DR6,
reconstruct the #DB payload from the DR6 we get at exit time, and let
kvm_deliver_exception_payload move the newly set bits into vcpu->arch.dr6.
Some extra bits may be included in the payload if KVM_DEBUGREG_WONT_EXIT
is set, but this is harmless.

This may not be the most optimized way to deal with this, but it is
simple and, being confined within SVM code, it gets rid of the set_dr6
callback and kvm_update_dr6.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/svm/nested.c       | 14 +++++++++++---
 arch/x86/kvm/svm/svm.c          | 29 +++++++++++++++++++++--------
 arch/x86/kvm/vmx/vmx.c          |  5 -----
 arch/x86/kvm/x86.c              | 12 ------------
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 93f6f696d059..9e8263b1e6fe 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);
-	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);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 1a547e3ac0e5..9a2a62e5afeb 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -633,10 +633,18 @@ static int nested_svm_intercept_db(struct vcpu_svm *svm)
 
 reflected_db:
 	/*
-	 * Synchronize guest DR6 here just like in db_interception; it will
-	 * be moved into the nested VMCB by nested_svm_vmexit.
+	 * Synchronize guest DR6 here just like in kvm_deliver_exception_payload;
+	 * it will be moved into the nested VMCB by nested_svm_vmexit.  Once
+	 * exceptions will be moved to svm_check_nested_events, all this stuff
+	 * will just go away and we could just return NESTED_EXIT_HOST
+	 * unconditionally.  db_interception will queue the exception, which
+	 * will be processed by svm_check_nested_events if a nested vmexit is
+	 * required, and we will just use kvm_deliver_exception_payload to copy
+	 * the payload to DR6 before vmexit.
 	 */
-	svm->vcpu.arch.dr6 = dr6;
+	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
+	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
+	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;
 	return NESTED_EXIT_DONE;
 }
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f03bffafd9e6..a862c768fd54 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1672,12 +1672,14 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
 	mark_dirty(svm->vmcb, VMCB_ASID);
 }
 
-static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
+static void svm_set_dr6(struct vcpu_svm *svm, unsigned long value)
 {
-	struct vcpu_svm *svm = to_svm(vcpu);
+	struct vmcb *vmcb = svm->vmcb;
 
-	svm->vmcb->save.dr6 = value;
-	mark_dirty(svm->vmcb, VMCB_DR);
+	if (unlikely(value != vmcb->save.dr6)) {
+		vmcb->save.dr6 = value;
+		mark_dirty(vmcb, VMCB_DR);
+	}
 }
 
 static void svm_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
@@ -1688,9 +1690,12 @@ 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);
+	/*
+	 * We cannot reset svm->vmcb->save.dr6 to DR6_FIXED_1|DR6_RTM here,
+	 * because db_interception might need it.  We can do it before vmentry.
+	 */
 	vcpu->arch.dr6 = svm->vmcb->save.dr6;
 	vcpu->arch.dr7 = svm->vmcb->save.dr7;
-
 	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
 	set_dr_intercepts(svm);
 }
@@ -1734,8 +1739,8 @@ 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);
+		u32 payload = (svm->vmcb->save.dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
+		kvm_queue_exception_p(&svm->vcpu, DB_VECTOR, payload);
 		return 1;
 	}
 
@@ -3313,6 +3318,15 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	svm->vmcb->save.cr2 = vcpu->arch.cr2;
 
+	/*
+	 * Run with all-zero DR6 unless needed, so that we can get the exact cause
+	 * of a #DB.
+	 */
+	if (unlikely(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
+		svm_set_dr6(svm, vcpu->arch.dr6);
+	else
+		svm_set_dr6(svm, DR6_FIXED_1 | DR6_RTM);
+
 	clgi();
 	kvm_load_guest_xsave_state(vcpu);
 
@@ -3927,7 +3941,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,
-	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
 	.sync_dirty_debug_regs = svm_sync_dirty_debug_regs,
 	.cache_reg = svm_cache_reg,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6153a47109d3..e2b71b0cdfce 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4965,10 +4965,6 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return kvm_skip_emulated_instruction(vcpu);
 }
 
-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);
@@ -7731,7 +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,
-	.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 b1b92d904f37..f780af601c5f 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);
@@ -474,7 +473,6 @@ 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;
@@ -1048,12 +1046,6 @@ 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))
-		kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6);
-}
-
 static void kvm_update_dr7(struct kvm_vcpu *vcpu)
 {
 	unsigned long dr7;
@@ -1093,7 +1085,6 @@ 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) | kvm_dr6_fixed(vcpu);
-		kvm_update_dr6(vcpu);
 		break;
 	case 5:
 		/* fall through */
@@ -4009,7 +4000,6 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db));
 	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = dbgregs->dr6;
-	kvm_update_dr6(vcpu);
 	vcpu->arch.dr7 = dbgregs->dr7;
 	kvm_update_dr7(vcpu);
 
@@ -8418,7 +8408,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
 		kvm_x86_ops.sync_dirty_debug_regs(vcpu);
 		kvm_update_dr0123(vcpu);
-		kvm_update_dr6(vcpu);
 		kvm_update_dr7(vcpu);
 		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
 	}
@@ -9479,7 +9468,6 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
 	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = DR6_INIT;
-	kvm_update_dr6(vcpu);
 	vcpu->arch.dr7 = DR7_FIXED_1;
 	kvm_update_dr7(vcpu);
 
-- 
2.18.2

Re: [PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Peter Xu]

On Thu, May 07, 2020 at 07:50:10AM -0400, Paolo Bonzini wrote:
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 1a547e3ac0e5..9a2a62e5afeb 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -633,10 +633,18 @@ static int nested_svm_intercept_db(struct vcpu_svm *svm)
>  
>  reflected_db:
>  	/*
> -	 * Synchronize guest DR6 here just like in db_interception; it will
> -	 * be moved into the nested VMCB by nested_svm_vmexit.
> +	 * Synchronize guest DR6 here just like in kvm_deliver_exception_payload;
> +	 * it will be moved into the nested VMCB by nested_svm_vmexit.  Once
> +	 * exceptions will be moved to svm_check_nested_events, all this stuff
> +	 * will just go away and we could just return NESTED_EXIT_HOST
> +	 * unconditionally.  db_interception will queue the exception, which
> +	 * will be processed by svm_check_nested_events if a nested vmexit is
> +	 * required, and we will just use kvm_deliver_exception_payload to copy
> +	 * the payload to DR6 before vmexit.
>  	 */
> -	svm->vcpu.arch.dr6 = dr6;
> +	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
> +	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
> +	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;

I failed to figure out what the above calculation is going to do...  E.g., I
think the old "BT|BS|BD" bits in the old arch.dr6 cache will be leftover even
if none of them is set in save.dr6, while we shouldn't?

Thanks,

-- 
Peter Xu

Re: [PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Paolo Bonzini]

On 07/05/20 21:28, Peter Xu wrote:
>> -	svm->vcpu.arch.dr6 = dr6;
>> +	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
>> +	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
>> +	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;
> I failed to figure out what the above calculation is going to do... 

The calculation is merging the cause of the #DB with the guest DR6.
It's basically the same effect as kvm_deliver_exception_payload. The
payload has DR6_RTM flipped compared to DR6, so you have the following
simplfications:

	payload = (dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
	/* This is kvm_deliver_exception_payload: */
        vcpu->arch.dr6 &= ~DR_TRAP_BITS;
        vcpu->arch.dr6 |= DR6_RTM;
	/* copy dr6 bits other than RTM */
        vcpu->arch.dr6 |= payload;
	/* copy flipped RTM bit */
        vcpu->arch.dr6 ^= payload & DR6_RTM;

->

	payload = (dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
	/* clear RTM here, so that we can OR it below */
        vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
	/* copy dr6 bits other than RTM */
        vcpu->arch.dr6 |= payload & ~DR6_RTM;
	/* copy flipped RTM bit */
        vcpu->arch.dr6 |= (payload ^ DR6_RTM) & DR6_RTM;

->

	/* we can drop the double XOR of DR6_RTM */
	dr6 &= ~DR6_FIXED_1;
        vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
        vcpu->arch.dr6 |= dr6 & ~DR6_RTM;
        vcpu->arch.dr6 |= dr6 & DR6_RTM;

->

	/* we can do the two ORs with a single operation */
        vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
        vcpu->arch.dr6 |= dr6 & ~DR6_FIXED_1;

> E.g., I
> think the old "BT|BS|BD" bits in the old arch.dr6 cache will be leftover even
> if none of them is set in save.dr6, while we shouldn't?

Those bits should be kept; this is covered for example by the "hw
breakpoint (test that dr6.BS is not cleared)" testcase in kvm-unit-tests
x86/debug.c.

Thanks,

Paolo

Re: [PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Peter Xu]

On Fri, May 08, 2020 at 12:33:57AM +0200, Paolo Bonzini wrote:
> On 07/05/20 21:28, Peter Xu wrote:
> >> -	svm->vcpu.arch.dr6 = dr6;
> >> +	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
> >> +	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
> >> +	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;
> > I failed to figure out what the above calculation is going to do... 
> 
> The calculation is merging the cause of the #DB with the guest DR6.
> It's basically the same effect as kvm_deliver_exception_payload. The
> payload has DR6_RTM flipped compared to DR6, so you have the following
> simplfications:
> 
> 	payload = (dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
> 	/* This is kvm_deliver_exception_payload: */
>         vcpu->arch.dr6 &= ~DR_TRAP_BITS;
>         vcpu->arch.dr6 |= DR6_RTM;
> 	/* copy dr6 bits other than RTM */
>         vcpu->arch.dr6 |= payload;
> 	/* copy flipped RTM bit */
>         vcpu->arch.dr6 ^= payload & DR6_RTM;
> 
> ->
> 
> 	payload = (dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
> 	/* clear RTM here, so that we can OR it below */
>         vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
> 	/* copy dr6 bits other than RTM */
>         vcpu->arch.dr6 |= payload & ~DR6_RTM;
> 	/* copy flipped RTM bit */
>         vcpu->arch.dr6 |= (payload ^ DR6_RTM) & DR6_RTM;
> 
> ->
> 
> 	/* we can drop the double XOR of DR6_RTM */
> 	dr6 &= ~DR6_FIXED_1;
>         vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
>         vcpu->arch.dr6 |= dr6 & ~DR6_RTM;
>         vcpu->arch.dr6 |= dr6 & DR6_RTM;
> 
> ->
> 
> 	/* we can do the two ORs with a single operation */
>         vcpu->arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
>         vcpu->arch.dr6 |= dr6 & ~DR6_FIXED_1;

Oh that's quite some math. :) Thanks Paolo!

Shall we introduce a helper for both kvm_deliver_exception_payload and here
(e.g. kvm_merge_dr6)?  Also, wondering whether this could be a bit easier to
follow by defining:

/*
 * These bits could be kept being set until the next #DB if not
 * explicitly cleared.
 */
#define  DR6_CARRY_OVER_BITS  (DR6_BT | DR6_BS | DR6_BD)

Then the imho above calculation could also become:

    vcpu->arch.dr6 = (vcpu->arch.dr6 & DR6_CARRY_OVER_BITS) | save.dr6;

What do you think?

> 
> > E.g., I
> > think the old "BT|BS|BD" bits in the old arch.dr6 cache will be leftover even
> > if none of them is set in save.dr6, while we shouldn't?
> 
> Those bits should be kept; this is covered for example by the "hw
> breakpoint (test that dr6.BS is not cleared)" testcase in kvm-unit-tests
> x86/debug.c.

Right.  Thanks!

-- 
Peter Xu

Re: [PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Paolo Bonzini]

On 08/05/20 17:32, Peter Xu wrote:
> On Fri, May 08, 2020 at 12:33:57AM +0200, Paolo Bonzini wrote:
>> On 07/05/20 21:28, Peter Xu wrote:
>>>> -	svm->vcpu.arch.dr6 = dr6;
>>>> +	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
>>>> +	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
>>>> +	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;
>>> I failed to figure out what the above calculation is going to do... 
>>
>> The calculation is merging the cause of the #DB with the guest DR6.
>> It's basically the same effect as kvm_deliver_exception_payload.
> 
> Shall we introduce a helper for both kvm_deliver_exception_payload and here
> (e.g. kvm_merge_dr6)?  Also, wondering whether this could be a bit easier to
> follow by defining:

It would make sense indeed but I plan to get rid of this in 5.9 (so in
about a month), as explained in the comment.

Paolo

Re: [PATCH v2 8/9] KVM: x86, SVM: isolate vcpu->arch.dr6 from vmcb->save.dr6

[Peter Xu]

On Sat, May 09, 2020 at 03:28:44PM +0200, Paolo Bonzini wrote:
> On 08/05/20 17:32, Peter Xu wrote:
> > On Fri, May 08, 2020 at 12:33:57AM +0200, Paolo Bonzini wrote:
> >> On 07/05/20 21:28, Peter Xu wrote:
> >>>> -	svm->vcpu.arch.dr6 = dr6;
> >>>> +	WARN_ON(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT);
> >>>> +	svm->vcpu.arch.dr6 &= ~(DR_TRAP_BITS | DR6_RTM);
> >>>> +	svm->vcpu.arch.dr6 |= dr6 & ~DR6_FIXED_1;
> >>> I failed to figure out what the above calculation is going to do... 
> >>
> >> The calculation is merging the cause of the #DB with the guest DR6.
> >> It's basically the same effect as kvm_deliver_exception_payload.
> > 
> > Shall we introduce a helper for both kvm_deliver_exception_payload and here
> > (e.g. kvm_merge_dr6)?  Also, wondering whether this could be a bit easier to
> > follow by defining:
> 
> It would make sense indeed but I plan to get rid of this in 5.9 (so in
> about a month), as explained in the comment.

OK. I thought it would be easy to change and verify when with the selftests,
however it's definitely ok to work upon it too.  Thanks,

-- 
Peter Xu

[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..8162c58a1234 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 9/9] KVM: VMX: pass correct DR6 for GD userspace exit

[Peter Xu]

On Thu, May 07, 2020 at 07:50:11AM -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>
> ---
>  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;

After a second thought I'm thinking whether it would be okay to have BS set in
that test case.  I just remembered there's a test case in the kvm-unit-test
that checks explicitly against BS leftover as long as dr6 is not cleared
explicitly by the guest code, while the spec seems to have no explicit
description on this case.

Intead of above, I'm thinking whether we should allow the userspace to also
change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
Or to make it simple, maybe we can just check BD bit only?

Thanks,

>  			vcpu->run->debug.arch.dr7 = dr7;
>  			vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu);
>  			vcpu->run->debug.arch.exception = DB_VECTOR;

-- 
Peter Xu

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

[Paolo Bonzini]

On 07/05/20 18:18, Peter Xu wrote:
>>  		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;
> After a second thought I'm thinking whether it would be okay to have BS set in
> that test case.  I just remembered there's a test case in the kvm-unit-test
> that checks explicitly against BS leftover as long as dr6 is not cleared
> explicitly by the guest code, while the spec seems to have no explicit
> description on this case.

Yes, I noticed that test as well.  But I don't like having different
behavior for Intel and AMD, and the Intel behavior is more sensible.
Also...

> Intead of above, I'm thinking whether we should allow the userspace to also
> change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
> iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
> Or to make it simple, maybe we can just check BD bit only?

... I'm afraid that this would be a backwards-incompatible change, and
it would require changes in userspace.  If you look at v2, emulating the
Intel behavior in AMD turns out to be self-contained and relatively
elegant (will be better when we finish cleaning up nested SVM).

Paolo

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

[Peter Xu]

On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote:
> On 07/05/20 18:18, Peter Xu wrote:
> >>  		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;
> > After a second thought I'm thinking whether it would be okay to have BS set in
> > that test case.  I just remembered there's a test case in the kvm-unit-test
> > that checks explicitly against BS leftover as long as dr6 is not cleared
> > explicitly by the guest code, while the spec seems to have no explicit
> > description on this case.
> 
> Yes, I noticed that test as well.  But I don't like having different
> behavior for Intel and AMD, and the Intel behavior is more sensible.
> Also...

Do you mean the AMD behavior is more sensible instead? :)

> 
> > Intead of above, I'm thinking whether we should allow the userspace to also
> > change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
> > iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
> > Or to make it simple, maybe we can just check BD bit only?
> 
> ... I'm afraid that this would be a backwards-incompatible change, and
> it would require changes in userspace.  If you look at v2, emulating the
> Intel behavior in AMD turns out to be self-contained and relatively
> elegant (will be better when we finish cleaning up nested SVM).

I'm still trying to read the other patches (I need some more digest because I'm
even less familiar with nested...).  I agree that it would be good to keep the
same behavior across Intel/AMD.  Actually that also does not violate Intel spec
because the AMD one is stricter.  However I guess then we might also want to
fixup the kvm-unit-test too to aligh with the behaviors on leftover set bits.

Thanks,

-- 
Peter Xu

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

[Paolo Bonzini]

On 07/05/20 18:38, Peter Xu wrote:
> On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote:
>> On 07/05/20 18:18, Peter Xu wrote:
>>>>  		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;
>>> After a second thought I'm thinking whether it would be okay to have BS set in
>>> that test case.  I just remembered there's a test case in the kvm-unit-test
>>> that checks explicitly against BS leftover as long as dr6 is not cleared
>>> explicitly by the guest code, while the spec seems to have no explicit
>>> description on this case.
>>
>> Yes, I noticed that test as well.  But I don't like having different
>> behavior for Intel and AMD, and the Intel behavior is more sensible.
>> Also...
> 
> Do you mean the AMD behavior is more sensible instead? :)

No, I mean within the context of KVM_EXIT_DEBUG: the Intel behavior is
to only include the latest debug exception in kvm_run's DR6 field, while
the AMD behavior would be to include all of them.  This was an
implementation detail (it happens because Intel sets kvm_run's DR6 from
the exit qualification of #DB), but it's more sensible too.

In addition:

* AMD was completely broken until this week, so the behavior of
KVM_EXIT_DEBUG is defined de facto by kvm_intel.ko.  Userspace has not
been required to set DR6 with KVM_SET_GUEST_DEBUG, and since we can
emulate that on AMD, we should.

* we have to fix anyway the fact that on AMD a KVM_EXIT_DEBUG is
clobbering the contents of the guest's DR6

>>> Intead of above, I'm thinking whether we should allow the userspace to also
>>> change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
>>> iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
>>> Or to make it simple, maybe we can just check BD bit only?
>>
>> ... I'm afraid that this would be a backwards-incompatible change, and
>> it would require changes in userspace.  If you look at v2, emulating the
>> Intel behavior in AMD turns out to be self-contained and relatively
>> elegant (will be better when we finish cleaning up nested SVM).
> 
> I'm still trying to read the other patches (I need some more digest because I'm
> even less familiar with nested...).  I agree that it would be good to keep the
> same behavior across Intel/AMD.  Actually that also does not violate Intel spec
> because the AMD one is stricter.

Again, careful---we're talking about KVM_EXIT_DEBUG, not the #DB exception.

Thanks,

Paolo

> However I guess then we might also want to
> fixup the kvm-unit-test too to aligh with the behaviors on leftover set bits.

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

[Peter Xu]

On Thu, May 07, 2020 at 07:42:25PM +0200, Paolo Bonzini wrote:
> On 07/05/20 18:38, Peter Xu wrote:
> > On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote:
> >> On 07/05/20 18:18, Peter Xu wrote:
> >>>>  		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;
> >>> After a second thought I'm thinking whether it would be okay to have BS set in
> >>> that test case.  I just remembered there's a test case in the kvm-unit-test
> >>> that checks explicitly against BS leftover as long as dr6 is not cleared
> >>> explicitly by the guest code, while the spec seems to have no explicit
> >>> description on this case.
> >>
> >> Yes, I noticed that test as well.  But I don't like having different
> >> behavior for Intel and AMD, and the Intel behavior is more sensible.
> >> Also...
> > 
> > Do you mean the AMD behavior is more sensible instead? :)
> 
> No, I mean within the context of KVM_EXIT_DEBUG: the Intel behavior is
> to only include the latest debug exception in kvm_run's DR6 field, while
> the AMD behavior would be to include all of them.  This was an
> implementation detail (it happens because Intel sets kvm_run's DR6 from
> the exit qualification of #DB), but it's more sensible too.
> 
> In addition:
> 
> * AMD was completely broken until this week, so the behavior of
> KVM_EXIT_DEBUG is defined de facto by kvm_intel.ko.  Userspace has not
> been required to set DR6 with KVM_SET_GUEST_DEBUG, and since we can
> emulate that on AMD, we should.
> 
> * we have to fix anyway the fact that on AMD a KVM_EXIT_DEBUG is
> clobbering the contents of the guest's DR6
> 
> >>> Intead of above, I'm thinking whether we should allow the userspace to also
> >>> change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
> >>> iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
> >>> Or to make it simple, maybe we can just check BD bit only?
> >>
> >> ... I'm afraid that this would be a backwards-incompatible change, and
> >> it would require changes in userspace.  If you look at v2, emulating the
> >> Intel behavior in AMD turns out to be self-contained and relatively
> >> elegant (will be better when we finish cleaning up nested SVM).
> > 
> > I'm still trying to read the other patches (I need some more digest because I'm
> > even less familiar with nested...).  I agree that it would be good to keep the
> > same behavior across Intel/AMD.  Actually that also does not violate Intel spec
> > because the AMD one is stricter.
> 
> Again, careful---we're talking about KVM_EXIT_DEBUG, not the #DB exception.

OK I get your point now.  Thanks,

-- 
Peter Xu