[PATCH 2/6] KVM: x86: Optimize debug register switching

[PATCH 2/6] KVM: x86: Optimize debug register switching

[Jan Kiszka]

Based on Avi's suggestion: Do not save the host debug registers on guest
entry as they are already present in the thread state. Moreover, only
restore them if the current host thread is being debugged. But as KGDB
accesses the debug register directly, we have to fall back to existing
pattern in that case.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arch/x86/kvm/x86.c |   48 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 891234b..036a2c5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -37,6 +37,7 @@
 #include <linux/iommu.h>
 #include <linux/intel-iommu.h>
 #include <linux/cpufreq.h>
+#include <linux/kgdb.h>
 #include <trace/events/kvm.h>
 #undef TRACE_INCLUDE_FILE
 #define CREATE_TRACE_POINTS
@@ -3627,14 +3628,21 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	kvm_guest_enter();
 
-	get_debugreg(vcpu->arch.host_dr6, 6);
-	get_debugreg(vcpu->arch.host_dr7, 7);
+	/*
+	 * kgdb accesses the debug registers directly, so we have to save them
+	 * and restore those values on return from the guest.
+	 */
+	if (unlikely(kgdb_in_use())) {
+		if (unlikely(vcpu->arch.switch_db_regs)) {
+			get_debugreg(vcpu->arch.host_db[0], 0);
+			get_debugreg(vcpu->arch.host_db[1], 1);
+			get_debugreg(vcpu->arch.host_db[2], 2);
+			get_debugreg(vcpu->arch.host_db[3], 3);
+		}
+		get_debugreg(vcpu->arch.host_dr6, 6);
+		get_debugreg(vcpu->arch.host_dr7, 7);
+	}
 	if (unlikely(vcpu->arch.switch_db_regs)) {
-		get_debugreg(vcpu->arch.host_db[0], 0);
-		get_debugreg(vcpu->arch.host_db[1], 1);
-		get_debugreg(vcpu->arch.host_db[2], 2);
-		get_debugreg(vcpu->arch.host_db[3], 3);
-
 		set_debugreg(0, 7);
 		set_debugreg(vcpu->arch.eff_db[0], 0);
 		set_debugreg(vcpu->arch.eff_db[1], 1);
@@ -3645,15 +3653,25 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	trace_kvm_entry(vcpu->vcpu_id);
 	kvm_x86_ops->run(vcpu);
 
-	if (unlikely(vcpu->arch.switch_db_regs)) {
-		set_debugreg(0, 7);
-		set_debugreg(vcpu->arch.host_db[0], 0);
-		set_debugreg(vcpu->arch.host_db[1], 1);
-		set_debugreg(vcpu->arch.host_db[2], 2);
-		set_debugreg(vcpu->arch.host_db[3], 3);
+	if (unlikely(kgdb_in_use())) {
+		if (unlikely(vcpu->arch.switch_db_regs)) {
+			set_debugreg(vcpu->arch.host_db[0], 0);
+			set_debugreg(vcpu->arch.host_db[1], 1);
+			set_debugreg(vcpu->arch.host_db[2], 2);
+			set_debugreg(vcpu->arch.host_db[3], 3);
+		}
+		set_debugreg(vcpu->arch.host_dr6, 6);
+		set_debugreg(vcpu->arch.host_dr7, 7);
+	} else if (unlikely(test_thread_flag(TIF_DEBUG))) {
+		if (unlikely(vcpu->arch.switch_db_regs)) {
+			set_debugreg(current->thread.debugreg0, 0);
+			set_debugreg(current->thread.debugreg1, 1);
+			set_debugreg(current->thread.debugreg2, 2);
+			set_debugreg(current->thread.debugreg3, 3);
+		}
+		set_debugreg(current->thread.debugreg6, 6);
+		set_debugreg(current->thread.debugreg7, 7);
 	}
-	set_debugreg(vcpu->arch.host_dr6, 6);
-	set_debugreg(vcpu->arch.host_dr7, 7);
 
 	set_bit(KVM_REQ_KICK, &vcpu->requests);
 	local_irq_enable();

[PATCH 0/6] Debug register emulation fixes and optimizations

[Jan Kiszka]

This series realizes the debug register switch optimization Avi
suggested, but now in a KGDB-safe way. It furthermore cleans up the DR6
optimization as discussed earlier. And it improves the debug register
emulation for both VMX and SVM.

Note that the series applies on top of kvm.git next.

Find this series also at git://git.kiszka.org/linux-kvm.git queues/debugregs

Jan Kiszka (6):
      KGDB: Add kgdb_in_use
      KVM: x86: Optimize debug register switching
      KVM: VMX: Clean up DR6 emulation
      KVM: VMX: Fix emulation of DR4 and DR5
      KVM: SVM: Enable full MOV DR emulation
      KVM: SVM: Trap all debug register accesses

 arch/x86/include/asm/kvm_host.h |    5 +-
 arch/x86/kvm/svm.c              |   86 ++++++++++++++++++++++++--------------
 arch/x86/kvm/vmx.c              |   55 ++++++++++++++----------
 arch/x86/kvm/x86.c              |   67 ++++++++++++++++--------------
 include/linux/kgdb.h            |   17 ++++++++
 5 files changed, 140 insertions(+), 90 deletions(-)

[PATCH 3/6] KVM: VMX: Clean up DR6 emulation

[Jan Kiszka]

As we trap all debug register accesses, we do not need to switch real
DR6 at all. Clean up update_exception_bitmap at this chance, too.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arch/x86/kvm/vmx.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d3213ac..7012680 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -536,18 +536,14 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
 {
 	u32 eb;
 
-	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_VECTOR);
+	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_VECTOR) |
+	     (1u << DB_VECTOR);
 	if (!vcpu->fpu_active)
 		eb |= 1u << NM_VECTOR;
-	/*
-	 * Unconditionally intercept #DB so we can maintain dr6 without
-	 * reading it every exit.
-	 */
-	eb |= 1u << DB_VECTOR;
-	if (vcpu->guest_debug & KVM_GUESTDBG_ENABLE) {
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
-			eb |= 1u << BP_VECTOR;
-	}
+	if ((vcpu->guest_debug &
+	     (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)) ==
+	    (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP))
+		eb |= 1u << BP_VECTOR;
 	if (to_vmx(vcpu)->rmode.vm86_active)
 		eb = ~0;
 	if (enable_ept)
@@ -3627,9 +3623,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	vmcs_writel(HOST_CR0, read_cr0());
 
-	if (vcpu->arch.switch_db_regs)
-		set_debugreg(vcpu->arch.dr6, 6);
-
 	asm(
 		/* Store host registers */
 		"push %%"R"dx; push %%"R"bp;"
@@ -3730,9 +3723,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 				  | (1 << VCPU_EXREG_PDPTR));
 	vcpu->arch.regs_dirty = 0;
 
-	if (vcpu->arch.switch_db_regs)
-		get_debugreg(vcpu->arch.dr6, 6);
-
 	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
 	if (vmx->rmode.irq.pending)
 		fixup_rmode_irq(vmx);

[PATCH 1/6] KGDB: Add kgdb_in_use

[Jan Kiszka]

Allow other kernel subsystems to check for kgdb debug sessions. This will
first be used by kvm to switch to conservative debug register saving/restoring
on x86 if kgdb is in use.

CC: Jason Wessel <jason.wessel@windriver.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 include/linux/kgdb.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 6adcc29..aebc768 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -72,6 +72,23 @@ struct uart_port;
  */
 void kgdb_breakpoint(void);
 
+/**
+ *	kgdb_in_use - Check if kgdb is currently in use
+ *
+ *	This function allows other kernel subsystems to check if the kernel
+ *	is currently being debugged via kgdb.  It returns false if not or if
+ *	kgdb support was not built into the kernel.
+ *
+ */
+static inline int kgdb_in_use(void)
+{
+#ifdef CONFIG_KGDB
+	return kgdb_connected;
+#else
+	return 0;
+#endif
+}
+
 extern int kgdb_connected;
 
 extern atomic_t			kgdb_setting_breakpoint;

[PATCH 6/6] KVM: SVM: Trap all debug register accesses

[Jan Kiszka]

To enable proper debug register emulation under all conditions, trap
access to all DR0..7. This may be optimized later on.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arch/x86/kvm/svm.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4305969..52cbe11 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -551,13 +551,19 @@ static void init_vmcb(struct vcpu_svm *svm)
 	control->intercept_dr_read = 	INTERCEPT_DR0_MASK |
 					INTERCEPT_DR1_MASK |
 					INTERCEPT_DR2_MASK |
-					INTERCEPT_DR3_MASK;
+					INTERCEPT_DR3_MASK |
+					INTERCEPT_DR4_MASK |
+					INTERCEPT_DR5_MASK |
+					INTERCEPT_DR6_MASK |
+					INTERCEPT_DR7_MASK;
 
 	control->intercept_dr_write = 	INTERCEPT_DR0_MASK |
 					INTERCEPT_DR1_MASK |
 					INTERCEPT_DR2_MASK |
 					INTERCEPT_DR3_MASK |
+					INTERCEPT_DR4_MASK |
 					INTERCEPT_DR5_MASK |
+					INTERCEPT_DR6_MASK |
 					INTERCEPT_DR7_MASK;
 
 	control->intercept_exceptions = (1 << PF_VECTOR) |
@@ -2285,11 +2291,17 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_READ_DR1]			= emulate_on_interception,
 	[SVM_EXIT_READ_DR2]			= emulate_on_interception,
 	[SVM_EXIT_READ_DR3]			= emulate_on_interception,
+	[SVM_EXIT_READ_DR4]			= emulate_on_interception,
+	[SVM_EXIT_READ_DR5]			= emulate_on_interception,
+	[SVM_EXIT_READ_DR6]			= emulate_on_interception,
+	[SVM_EXIT_READ_DR7]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR0]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR1]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR2]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR3]			= emulate_on_interception,
+	[SVM_EXIT_WRITE_DR4]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR5]			= emulate_on_interception,
+	[SVM_EXIT_WRITE_DR6]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR7]			= emulate_on_interception,
 	[SVM_EXIT_EXCP_BASE + DB_VECTOR]	= db_interception,
 	[SVM_EXIT_EXCP_BASE + BP_VECTOR]	= bp_interception,

[PATCH 5/6] KVM: SVM: Enable full MOV DR emulation

[Jan Kiszka]

Enhance MOV DR instruction emulation used by SVM so that it properly
injects faults and handles DR4/5 correctly.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arch/x86/include/asm/kvm_host.h |    5 +--
 arch/x86/kvm/svm.c              |   72 ++++++++++++++++++++++-----------------
 arch/x86/kvm/x86.c              |   19 +---------
 3 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6046e6f..38eef27 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -498,9 +498,8 @@ struct kvm_x86_ops {
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct descriptor_table *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct descriptor_table *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct descriptor_table *dt);
-	unsigned long (*get_dr)(struct kvm_vcpu *vcpu, int dr);
-	void (*set_dr)(struct kvm_vcpu *vcpu, int dr, unsigned long value,
-		       int *exception);
+	int (*get_dr)(struct kvm_vcpu *vcpu, int dr, unsigned long *dest);
+	int (*set_dr)(struct kvm_vcpu *vcpu, int dr, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
 	void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2df9b45..4305969 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1106,76 +1106,86 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *svm_data)
 	svm->vmcb->control.asid = svm_data->next_asid++;
 }
 
-static unsigned long svm_get_dr(struct kvm_vcpu *vcpu, int dr)
+static int svm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *dest)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
-	unsigned long val;
 
 	switch (dr) {
 	case 0 ... 3:
-		val = vcpu->arch.db[dr];
+		*dest = vcpu->arch.db[dr];
 		break;
+	case 4:
+		if (vcpu->arch.cr4 & X86_CR4_DE) {
+			kvm_queue_exception(vcpu, UD_VECTOR);
+			return -1;
+		}
+		/* fall through */
 	case 6:
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
-			val = vcpu->arch.dr6;
+			*dest = vcpu->arch.dr6;
 		else
-			val = svm->vmcb->save.dr6;
+			*dest = svm->vmcb->save.dr6;
 		break;
+	case 5:
+		if (vcpu->arch.cr4 & X86_CR4_DE) {
+			kvm_queue_exception(vcpu, UD_VECTOR);
+			return -1;
+		}
+		/* fall through */
 	case 7:
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
-			val = vcpu->arch.dr7;
+			*dest = vcpu->arch.dr7;
 		else
-			val = svm->vmcb->save.dr7;
+			*dest = svm->vmcb->save.dr7;
 		break;
-	default:
-		val = 0;
 	}
 
-	return val;
+	return 0;
 }
 
-static void svm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long value,
-		       int *exception)
+static int svm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	*exception = 0;
-
 	switch (dr) {
 	case 0 ... 3:
 		vcpu->arch.db[dr] = value;
 		if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
 			vcpu->arch.eff_db[dr] = value;
-		return;
-	case 4 ... 5:
-		if (vcpu->arch.cr4 & X86_CR4_DE)
-			*exception = UD_VECTOR;
-		return;
+		break;
+	case 4:
+		if (vcpu->arch.cr4 & X86_CR4_DE) {
+			kvm_queue_exception(vcpu, UD_VECTOR);
+			return -1;
+		}
+		/* fall through */
 	case 6:
 		if (value & 0xffffffff00000000ULL) {
-			*exception = GP_VECTOR;
-			return;
+			kvm_inject_gp(vcpu, 0);
+			return -1;
 		}
 		vcpu->arch.dr6 = (value & DR6_VOLATILE) | DR6_FIXED_1;
-		return;
+		break;
+	case 5:
+		if (vcpu->arch.cr4 & X86_CR4_DE) {
+			kvm_queue_exception(vcpu, UD_VECTOR);
+			return -1;
+		}
+		/* fall through */
 	case 7:
 		if (value & 0xffffffff00000000ULL) {
-			*exception = GP_VECTOR;
-			return;
+			kvm_inject_gp(vcpu, 0);
+			return -1;
 		}
 		vcpu->arch.dr7 = (value & DR7_VOLATILE) | DR7_FIXED_1;
 		if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
 			svm->vmcb->save.dr7 = vcpu->arch.dr7;
 			vcpu->arch.switch_db_regs = (value & DR7_BP_EN_MASK);
 		}
-		return;
-	default:
-		/* FIXME: Possible case? */
-		printk(KERN_DEBUG "%s: unexpected dr %u\n",
-		       __func__, dr);
-		*exception = UD_VECTOR;
-		return;
+		break;
 	}
+
+	return 0;
 }
 
 static int pf_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 036a2c5..2ca7a1d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2695,29 +2695,14 @@ int emulate_clts(struct kvm_vcpu *vcpu)
 
 int emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr, unsigned long *dest)
 {
-	struct kvm_vcpu *vcpu = ctxt->vcpu;
-
-	switch (dr) {
-	case 0 ... 3:
-		*dest = kvm_x86_ops->get_dr(vcpu, dr);
-		return X86EMUL_CONTINUE;
-	default:
-		pr_unimpl(vcpu, "%s: unexpected dr %u\n", __func__, dr);
-		return X86EMUL_UNHANDLEABLE;
-	}
+	return kvm_x86_ops->get_dr(ctxt->vcpu, dr, dest);
 }
 
 int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr, unsigned long value)
 {
 	unsigned long mask = (ctxt->mode == X86EMUL_MODE_PROT64) ? ~0ULL : ~0U;
-	int exception;
 
-	kvm_x86_ops->set_dr(ctxt->vcpu, dr, value & mask, &exception);
-	if (exception) {
-		/* FIXME: better handling */
-		return X86EMUL_UNHANDLEABLE;
-	}
-	return X86EMUL_CONTINUE;
+	return kvm_x86_ops->set_dr(ctxt->vcpu, dr, value & mask);
 }
 
 void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)

[PATCH 4/6] KVM: VMX: Fix emulation of DR4 and DR5

[Jan Kiszka]

Make sure DR4 and DR5 are aliased to DR6 and DR7, respectively, if
CR4.DE is not set.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arch/x86/kvm/vmx.c |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7012680..d34aea5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2963,14 +2963,24 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		case 0 ... 3:
 			val = vcpu->arch.db[dr];
 			break;
+		case 4:
+			if (vcpu->arch.cr4 & X86_CR4_DE) {
+				kvm_queue_exception(vcpu, UD_VECTOR);
+				goto skip_instr;
+			}
+			/* fall through */
 		case 6:
 			val = vcpu->arch.dr6;
 			break;
-		case 7:
+		case 5:
+			if (vcpu->arch.cr4 & X86_CR4_DE) {
+				kvm_queue_exception(vcpu, UD_VECTOR);
+				goto skip_instr;
+			}
+			/* fall through */
+		default: /* 7 */
 			val = vcpu->arch.dr7;
 			break;
-		default:
-			val = 0;
 		}
 		kvm_register_write(vcpu, reg, val);
 	} else {
@@ -2981,10 +2991,12 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
 				vcpu->arch.eff_db[dr] = val;
 			break;
-		case 4 ... 5:
-			if (vcpu->arch.cr4 & X86_CR4_DE)
+		case 4:
+			if (vcpu->arch.cr4 & X86_CR4_DE) {
 				kvm_queue_exception(vcpu, UD_VECTOR);
-			break;
+				break;
+			}
+			/* fall through */
 		case 6:
 			if (val & 0xffffffff00000000ULL) {
 				kvm_queue_exception(vcpu, GP_VECTOR);
@@ -2992,7 +3004,13 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			}
 			vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
 			break;
-		case 7:
+		case 5:
+			if (vcpu->arch.cr4 & X86_CR4_DE) {
+				kvm_queue_exception(vcpu, UD_VECTOR);
+				break;
+			}
+			/* fall through */
+		default: /* 7 */
 			if (val & 0xffffffff00000000ULL) {
 				kvm_queue_exception(vcpu, GP_VECTOR);
 				break;
@@ -3006,6 +3024,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 			break;
 		}
 	}
+skip_instr:
 	skip_emulated_instruction(vcpu);
 	return 1;
 }

Re: [PATCH 2/6] KVM: x86: Optimize debug register switching

[Avi Kivity]

On 09/04/2009 03:51 PM, Jan Kiszka wrote:
> Based on Avi's suggestion: Do not save the host debug registers on guest
> entry as they are already present in the thread state. Moreover, only
> restore them if the current host thread is being debugged. But as KGDB
> accesses the debug register directly, we have to fall back to existing
> pattern in that case.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
>   arch/x86/kvm/x86.c |   48 +++++++++++++++++++++++++++++++++---------------
>   1 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 891234b..036a2c5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -37,6 +37,7 @@
>   #include<linux/iommu.h>
>   #include<linux/intel-iommu.h>
>   #include<linux/cpufreq.h>
> +#include<linux/kgdb.h>
>   #include<trace/events/kvm.h>
>   #undef TRACE_INCLUDE_FILE
>   #define CREATE_TRACE_POINTS
> @@ -3627,14 +3628,21 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>
>   	kvm_guest_enter();
>
> -	get_debugreg(vcpu->arch.host_dr6, 6);
> -	get_debugreg(vcpu->arch.host_dr7, 7);
> +	/*
> +	 * kgdb accesses the debug registers directly, so we have to save them
> +	 * and restore those values on return from the guest.
> +	 */
> +	if (unlikely(kgdb_in_use())) {
> +		if (unlikely(vcpu->arch.switch_db_regs)) {
> +			get_debugreg(vcpu->arch.host_db[0], 0);
> +			get_debugreg(vcpu->arch.host_db[1], 1);
> +			get_debugreg(vcpu->arch.host_db[2], 2);
> +			get_debugreg(vcpu->arch.host_db[3], 3);
> +		}
> +		get_debugreg(vcpu->arch.host_dr6, 6);
> +		get_debugreg(vcpu->arch.host_dr7, 7);
> +	}
>   	if (unlikely(vcpu->arch.switch_db_regs)) {
> -		get_debugreg(vcpu->arch.host_db[0], 0);
> -		get_debugreg(vcpu->arch.host_db[1], 1);
> -		get_debugreg(vcpu->arch.host_db[2], 2);
> -		get_debugreg(vcpu->arch.host_db[3], 3);
> -
>   		set_debugreg(0, 7);
>   		set_debugreg(vcpu->arch.eff_db[0], 0);
>   		set_debugreg(vcpu->arch.eff_db[1], 1);
> @@ -3645,15 +3653,25 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>   	trace_kvm_entry(vcpu->vcpu_id);
>   	kvm_x86_ops->run(vcpu);
>
> -	if (unlikely(vcpu->arch.switch_db_regs)) {
> -		set_debugreg(0, 7);
> -		set_debugreg(vcpu->arch.host_db[0], 0);
> -		set_debugreg(vcpu->arch.host_db[1], 1);
> -		set_debugreg(vcpu->arch.host_db[2], 2);
> -		set_debugreg(vcpu->arch.host_db[3], 3);
> +	if (unlikely(kgdb_in_use())) {
> +		if (unlikely(vcpu->arch.switch_db_regs)) {
> +			set_debugreg(vcpu->arch.host_db[0], 0);
> +			set_debugreg(vcpu->arch.host_db[1], 1);
> +			set_debugreg(vcpu->arch.host_db[2], 2);
> +			set_debugreg(vcpu->arch.host_db[3], 3);
> +		}
> +		set_debugreg(vcpu->arch.host_dr6, 6);
> +		set_debugreg(vcpu->arch.host_dr7, 7);
> +	} else if (unlikely(test_thread_flag(TIF_DEBUG))) {
> +		if (unlikely(vcpu->arch.switch_db_regs)) {
> +			set_debugreg(current->thread.debugreg0, 0);
> +			set_debugreg(current->thread.debugreg1, 1);
> +			set_debugreg(current->thread.debugreg2, 2);
> +			set_debugreg(current->thread.debugreg3, 3);
> +		}
> +		set_debugreg(current->thread.debugreg6, 6);
> +		set_debugreg(current->thread.debugreg7, 7);
>   	}
> -	set_debugreg(vcpu->arch.host_dr6, 6);
> -	set_debugreg(vcpu->arch.host_dr7, 7);
>
>    

Please, let's have just save/nosave, not different kinds of save/restore.

But really, this looks very hacky.  It's better to have kgdb integrate 
more closely with the kernel debug register support instead of kvm 
juggling between the two.

Something like

   struct debug_registers thread_info::debugregs
   extern struct debug_registers global_debug_registers;

and a function that loads a mix of the debug registers from the 
thread-local and global settings.  The context switch path can call that 
function as well as kvm.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/6] KVM: x86: Optimize debug register switching

[Jan Kiszka]

Avi Kivity wrote:
> On 09/04/2009 03:51 PM, Jan Kiszka wrote:
>> Based on Avi's suggestion: Do not save the host debug registers on guest
>> entry as they are already present in the thread state. Moreover, only
>> restore them if the current host thread is being debugged. But as KGDB
>> accesses the debug register directly, we have to fall back to existing
>> pattern in that case.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>>   arch/x86/kvm/x86.c |   48 +++++++++++++++++++++++++++++++++---------------
>>   1 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 891234b..036a2c5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -37,6 +37,7 @@
>>   #include<linux/iommu.h>
>>   #include<linux/intel-iommu.h>
>>   #include<linux/cpufreq.h>
>> +#include<linux/kgdb.h>
>>   #include<trace/events/kvm.h>
>>   #undef TRACE_INCLUDE_FILE
>>   #define CREATE_TRACE_POINTS
>> @@ -3627,14 +3628,21 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>
>>   	kvm_guest_enter();
>>
>> -	get_debugreg(vcpu->arch.host_dr6, 6);
>> -	get_debugreg(vcpu->arch.host_dr7, 7);
>> +	/*
>> +	 * kgdb accesses the debug registers directly, so we have to save them
>> +	 * and restore those values on return from the guest.
>> +	 */
>> +	if (unlikely(kgdb_in_use())) {
>> +		if (unlikely(vcpu->arch.switch_db_regs)) {
>> +			get_debugreg(vcpu->arch.host_db[0], 0);
>> +			get_debugreg(vcpu->arch.host_db[1], 1);
>> +			get_debugreg(vcpu->arch.host_db[2], 2);
>> +			get_debugreg(vcpu->arch.host_db[3], 3);
>> +		}
>> +		get_debugreg(vcpu->arch.host_dr6, 6);
>> +		get_debugreg(vcpu->arch.host_dr7, 7);
>> +	}
>>   	if (unlikely(vcpu->arch.switch_db_regs)) {
>> -		get_debugreg(vcpu->arch.host_db[0], 0);
>> -		get_debugreg(vcpu->arch.host_db[1], 1);
>> -		get_debugreg(vcpu->arch.host_db[2], 2);
>> -		get_debugreg(vcpu->arch.host_db[3], 3);
>> -
>>   		set_debugreg(0, 7);
>>   		set_debugreg(vcpu->arch.eff_db[0], 0);
>>   		set_debugreg(vcpu->arch.eff_db[1], 1);
>> @@ -3645,15 +3653,25 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   	trace_kvm_entry(vcpu->vcpu_id);
>>   	kvm_x86_ops->run(vcpu);
>>
>> -	if (unlikely(vcpu->arch.switch_db_regs)) {
>> -		set_debugreg(0, 7);
>> -		set_debugreg(vcpu->arch.host_db[0], 0);
>> -		set_debugreg(vcpu->arch.host_db[1], 1);
>> -		set_debugreg(vcpu->arch.host_db[2], 2);
>> -		set_debugreg(vcpu->arch.host_db[3], 3);
>> +	if (unlikely(kgdb_in_use())) {
>> +		if (unlikely(vcpu->arch.switch_db_regs)) {
>> +			set_debugreg(vcpu->arch.host_db[0], 0);
>> +			set_debugreg(vcpu->arch.host_db[1], 1);
>> +			set_debugreg(vcpu->arch.host_db[2], 2);
>> +			set_debugreg(vcpu->arch.host_db[3], 3);
>> +		}
>> +		set_debugreg(vcpu->arch.host_dr6, 6);
>> +		set_debugreg(vcpu->arch.host_dr7, 7);
>> +	} else if (unlikely(test_thread_flag(TIF_DEBUG))) {
>> +		if (unlikely(vcpu->arch.switch_db_regs)) {
>> +			set_debugreg(current->thread.debugreg0, 0);
>> +			set_debugreg(current->thread.debugreg1, 1);
>> +			set_debugreg(current->thread.debugreg2, 2);
>> +			set_debugreg(current->thread.debugreg3, 3);
>> +		}
>> +		set_debugreg(current->thread.debugreg6, 6);
>> +		set_debugreg(current->thread.debugreg7, 7);
>>   	}
>> -	set_debugreg(vcpu->arch.host_dr6, 6);
>> -	set_debugreg(vcpu->arch.host_dr7, 7);
>>
>>    
> 
> Please, let's have just save/nosave, not different kinds of save/restore.
> 
> But really, this looks very hacky.  It's better to have kgdb integrate 
> more closely with the kernel debug register support instead of kvm 
> juggling between the two.
> 
> Something like
> 
>    struct debug_registers thread_info::debugregs
>    extern struct debug_registers global_debug_registers;
> 
> and a function that loads a mix of the debug registers from the 
> thread-local and global settings.  The context switch path can call that 
> function as well as kvm.

For sure this approach is not nice. /me just wanted to avoid the area of
the "generic hw-breakpoint API". K. Prassad and Frederic Weisbecker
already try to push this abstraction for a fairly long time now.

OK, will go around and poll for status and perspectives of this effort.
Maybe throwing our requirements into the ring also helps accelerating it
a bit.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux