[PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[Paolo Bonzini]

Alex Williamson reported that a Windows game does something weird that
makes the guest save and restore debug registers on each context switch.
This cause several hundred thousands vmexits per second, and basically
cuts performance in half when running under KVM.

However, when not running in guest-debug mode, the guest controls the
debug registers and having to take an exit for each DR access is a waste
of time.  We just need one vmexit to load any stale values of DR0-DR6,
and then we can let the guest run freely.  On the next vmexit (whatever
the reason) we will read out whatever changes the guest made to the
debug registers.

Tested with x86/debug.flat on both Intel and AMD, both direct and
nested virtualization.

Changes from RFC: changed get_dr7 callback to sync_dirty_debug_regs,
	new patches 5-7.

Paolo Bonzini (7):
  KVM: vmx: we do rely on loading DR7 on entry
  KVM: x86: change vcpu->arch.switch_db_regs to a bit mask
  KVM: x86: Allow the guest to run with dirty debug registers
  KVM: vmx: Allow the guest to run with dirty debug registers
  KVM: nVMX: Allow nested guests to run with dirty debug registers
  KVM: svm: set/clear all DR intercepts in one swoop
  KVM: svm: Allow the guest to run with dirty debug registers

 arch/x86/include/asm/kvm_host.h |  8 ++++-
 arch/x86/kvm/svm.c              | 68 ++++++++++++++++++++++++++++-------------
 arch/x86/kvm/vmx.c              | 43 ++++++++++++++++++++++++--
 arch/x86/kvm/x86.c              | 20 +++++++++++-
 4 files changed, 114 insertions(+), 25 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] KVM: vmx: we do rely on loading DR7 on entry

[Paolo Bonzini]

Currently, this works even if the bit is not in "min", because the bit is always
set in MSR_IA32_VMX_ENTRY_CTLS.  Mention it for the sake of documentation, and
to avoid surprises if we later switch to MSR_IA32_VMX_TRUE_ENTRY_CTLS.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 53c324f3cc5e..06e4ec877a1c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2864,7 +2864,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 		!(_vmexit_control & VM_EXIT_ACK_INTR_ON_EXIT))
 		_pin_based_exec_control &= ~PIN_BASED_POSTED_INTR;
 
-	min = 0;
+	min = VM_ENTRY_LOAD_DEBUG_CONTROLS;
 	opt = VM_ENTRY_LOAD_IA32_PAT | VM_ENTRY_LOAD_BNDCFGS;
 	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_ENTRY_CTLS,
 				&_vmentry_control) < 0)
-- 
1.8.3.1

[PATCH 2/7] KVM: x86: change vcpu->arch.switch_db_regs to a bit mask

[Paolo Bonzini]

The next patch will add another bit that we can test with the
same "if".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 6 +++++-
 arch/x86/kvm/x86.c              | 4 +++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 85be627ef5de..5ef59d3b6c63 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -337,6 +337,10 @@ struct kvm_pmu {
 	u64 reprogram_pmi;
 };
 
+enum {
+	KVM_DEBUGREG_BP_ENABLED = 1,
+};
+
 struct kvm_vcpu_arch {
 	/*
 	 * rip and regs accesses must go through
@@ -463,7 +467,7 @@ struct kvm_vcpu_arch {
 	struct mtrr_state_type mtrr_state;
 	u32 pat;
 
-	int switch_db_regs;
+	unsigned switch_db_regs;
 	unsigned long db[KVM_NR_DB_REGS];
 	unsigned long dr6;
 	unsigned long dr7;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a45bcac45645..252b47e85c69 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -759,7 +759,9 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
 	else
 		dr7 = vcpu->arch.dr7;
 	kvm_x86_ops->set_dr7(vcpu, dr7);
-	vcpu->arch.switch_db_regs = (dr7 & DR7_BP_EN_MASK);
+	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_BP_ENABLED;
+	if (dr7 & DR7_BP_EN_MASK)
+		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
 }
 
 static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
-- 
1.8.3.1

[PATCH 3/7] KVM: x86: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

When not running in guest-debug mode, the guest controls the debug
registers and having to take an exit for each DR access is a waste
of time.  If the guest gets into a state where each context switch
causes DR to be saved and restored, this can take away as much as 40%
of the execution time from the guest.

After this patch, VMX- and SVM-specific code can set a flag in
switch_db_regs, telling vcpu_enter_guest that on the next exit the debug
registers might be dirty and need to be reloaded (syncing will be taken
care of by a new callback in kvm_x86_ops).  This flag can be set on the
first access to a debug registers, so that multiple accesses to the
debug registers only cause one vmexit.

Note that since the guest will be able to read debug registers and
enable breakpoints in DR7, we need to ensure that they are synchronized
on entry to the guest---including DR6 that was not synced before.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/x86.c              | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5ef59d3b6c63..74eb361eaa8f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -339,6 +339,7 @@ struct kvm_pmu {
 
 enum {
 	KVM_DEBUGREG_BP_ENABLED = 1,
+	KVM_DEBUGREG_WONT_EXIT = 2,
 };
 
 struct kvm_vcpu_arch {
@@ -707,6 +708,7 @@ struct kvm_x86_ops {
 	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);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 252b47e85c69..c48818aa04c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6033,12 +6033,28 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		set_debugreg(vcpu->arch.eff_db[1], 1);
 		set_debugreg(vcpu->arch.eff_db[2], 2);
 		set_debugreg(vcpu->arch.eff_db[3], 3);
+		set_debugreg(vcpu->arch.dr6, 6);
 	}
 
 	trace_kvm_entry(vcpu->vcpu_id);
 	kvm_x86_ops->run(vcpu);
 
 	/*
+	 * Do this here before restoring debug registers on the host.  And
+	 * since we do this before handling the vmexit, a DR access vmexit
+	 * can (a) read the correct value of the debug registers, (b) set
+	 * KVM_DEBUGREG_WONT_EXIT again.
+	 */
+	if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
+		int i;
+
+		WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
+		kvm_x86_ops->sync_dirty_debug_regs(vcpu);
+		for (i = 0; i < KVM_NR_DB_REGS; i++)
+			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
+	}
+
+	/*
 	 * If the guest has used debug registers, at least dr7
 	 * will be disabled while returning to the host.
 	 * If we don't have active breakpoints in the host, we don't
-- 
1.8.3.1

[PATCH 4/7] KVM: vmx: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

When not running in guest-debug mode (i.e. the guest controls the debug
registers, having to take an exit for each DR access is a waste of time.
If the guest gets into a state where each context switch causes DR to be
saved and restored, this can take away as much as 40% of the execution
time from the guest.

If the guest is running with vcpu->arch.db == vcpu->arch.eff_db, we
can let it write freely to the debug registers and reload them on the
next exit.  We still need to exit on the first access, so that the
KVM_DEBUGREG_WONT_EXIT flag is set in switch_db_regs; after that, further
accesses to the debug registers will not cause a vmexit.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 06e4ec877a1c..e377522408b1 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2843,7 +2843,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 		      vmx_capability.ept, vmx_capability.vpid);
 	}
 
-	min = 0;
+	min = VM_EXIT_SAVE_DEBUG_CONTROLS;
 #ifdef CONFIG_X86_64
 	min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
 #endif
@@ -5113,6 +5113,22 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		}
 	}
 
+	if (vcpu->guest_debug == 0) {
+		u32 cpu_based_vm_exec_control;
+
+		cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
+		cpu_based_vm_exec_control &= ~CPU_BASED_MOV_DR_EXITING;
+		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);
+
+		/*
+		 * No more DR vmexits; force a reload of the debug registers
+		 * and reenter on this instruction.  The next vmexit will
+		 * retrieve the full state of the debug registers.
+		 */
+		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
+		return 1;
+	}
+
 	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 	dr = exit_qualification & DEBUG_REG_ACCESS_NUM;
 	reg = DEBUG_REG_ACCESS_REG(exit_qualification);
@@ -5139,6 +5155,24 @@ static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
 {
 }
 
+static void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
+{
+	u32 cpu_based_vm_exec_control;
+
+	get_debugreg(vcpu->arch.db[0], 0);
+	get_debugreg(vcpu->arch.db[1], 1);
+	get_debugreg(vcpu->arch.db[2], 2);
+	get_debugreg(vcpu->arch.db[3], 3);
+	get_debugreg(vcpu->arch.dr6, 6);
+	vcpu->arch.dr7 = vmcs_readl(GUEST_DR7);
+
+	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
+
+	cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
+	cpu_based_vm_exec_control |= CPU_BASED_MOV_DR_EXITING;
+	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);
@@ -8590,6 +8624,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.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,
 	.get_rflags = vmx_get_rflags,
 	.set_rflags = vmx_set_rflags,
-- 
1.8.3.1

[PATCH 5/7] KVM: nVMX: Allow nested guests to run with dirty debug registers

[Paolo Bonzini]

When preparing the VMCS02, the CPU-based execution controls is computed
by vmx_exec_control.  Turn off DR access exits there, too, if the
KVM_DEBUGREG_WONT_EXIT bit is set in switch_db_regs.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e377522408b1..73ced6efc93e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4234,6 +4234,10 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
 static u32 vmx_exec_control(struct vcpu_vmx *vmx)
 {
 	u32 exec_control = vmcs_config.cpu_based_exec_ctrl;
+
+	if (vmx->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)
+		exec_control &= ~CPU_BASED_MOV_DR_EXITING;
+
 	if (!vm_need_tpr_shadow(vmx->vcpu.kvm)) {
 		exec_control &= ~CPU_BASED_TPR_SHADOW;
 #ifdef CONFIG_X86_64
-- 
1.8.3.1

[PATCH 6/7] KVM: svm: set/clear all DR intercepts in one swoop

[Paolo Bonzini]

Unlike other intercepts, debug register intercepts will be modified
in hot paths if the guest OS is bad or otherwise gets tricked into
doing so.

Avoid calling recalc_intercepts 16 times for debug registers.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 64d9bb9590e3..5ed67e9739f6 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -303,20 +303,35 @@ static inline bool is_cr_intercept(struct vcpu_svm *svm, int bit)
 	return vmcb->control.intercept_cr & (1U << bit);
 }
 
-static inline void set_dr_intercept(struct vcpu_svm *svm, int bit)
+static inline void set_dr_intercepts(struct vcpu_svm *svm)
 {
 	struct vmcb *vmcb = get_host_vmcb(svm);
 
-	vmcb->control.intercept_dr |= (1U << bit);
+	vmcb->control.intercept_dr = (1 << INTERCEPT_DR0_READ)
+		| (1 << INTERCEPT_DR1_READ)
+		| (1 << INTERCEPT_DR2_READ)
+		| (1 << INTERCEPT_DR3_READ)
+		| (1 << INTERCEPT_DR4_READ)
+		| (1 << INTERCEPT_DR5_READ)
+		| (1 << INTERCEPT_DR6_READ)
+		| (1 << INTERCEPT_DR7_READ)
+		| (1 << INTERCEPT_DR0_WRITE)
+		| (1 << INTERCEPT_DR1_WRITE)
+		| (1 << INTERCEPT_DR2_WRITE)
+		| (1 << INTERCEPT_DR3_WRITE)
+		| (1 << INTERCEPT_DR4_WRITE)
+		| (1 << INTERCEPT_DR5_WRITE)
+		| (1 << INTERCEPT_DR6_WRITE)
+		| (1 << INTERCEPT_DR7_WRITE);
 
 	recalc_intercepts(svm);
 }
 
-static inline void clr_dr_intercept(struct vcpu_svm *svm, int bit)
+static inline void clr_dr_intercepts(struct vcpu_svm *svm)
 {
 	struct vmcb *vmcb = get_host_vmcb(svm);
 
-	vmcb->control.intercept_dr &= ~(1U << bit);
+	vmcb->control.intercept_dr = 0;
 
 	recalc_intercepts(svm);
 }
@@ -1080,23 +1095,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	set_cr_intercept(svm, INTERCEPT_CR4_WRITE);
 	set_cr_intercept(svm, INTERCEPT_CR8_WRITE);
 
-	set_dr_intercept(svm, INTERCEPT_DR0_READ);
-	set_dr_intercept(svm, INTERCEPT_DR1_READ);
-	set_dr_intercept(svm, INTERCEPT_DR2_READ);
-	set_dr_intercept(svm, INTERCEPT_DR3_READ);
-	set_dr_intercept(svm, INTERCEPT_DR4_READ);
-	set_dr_intercept(svm, INTERCEPT_DR5_READ);
-	set_dr_intercept(svm, INTERCEPT_DR6_READ);
-	set_dr_intercept(svm, INTERCEPT_DR7_READ);
-
-	set_dr_intercept(svm, INTERCEPT_DR0_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR1_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR2_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR3_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR4_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR5_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR6_WRITE);
-	set_dr_intercept(svm, INTERCEPT_DR7_WRITE);
+	set_dr_intercepts(svm);
 
 	set_exception_intercept(svm, PF_VECTOR);
 	set_exception_intercept(svm, UD_VECTOR);
-- 
1.8.3.1

[PATCH 7/7] KVM: svm: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

When not running in guest-debug mode (i.e. the guest controls the debug
registers, having to take an exit for each DR access is a waste of time.
If the guest gets into a state where each context switch causes DR to be
saved and restored, this can take away as much as 40% of the execution
time from the guest.

If the guest is running with vcpu->arch.db == vcpu->arch.eff_db, we
can let it write freely to the debug registers and reload them on the
next exit.  We still need to exit on the first access, so that the
KVM_DEBUGREG_WONT_EXIT flag is set in switch_db_regs; after that, further
accesses to the debug registers will not cause a vmexit.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5ed67e9739f6..ac68edccc57c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1683,6 +1683,21 @@ static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
 	mark_dirty(svm->vmcb, VMCB_DR);
 }
 
+static void svm_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	get_debugreg(vcpu->arch.db[0], 0);
+	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.dr7 = svm->vmcb->save.dr7;
+
+	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
+	set_dr_intercepts(svm);
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -2974,6 +2989,17 @@ static int dr_interception(struct vcpu_svm *svm)
 	unsigned long val;
 	int err;
 
+	if (svm->vcpu.guest_debug == 0) {
+		/*
+		 * No more DR vmexits; force a reload of the debug registers
+		 * and reenter on this instruction.  The next vmexit will
+		 * retrieve the full state of the debug registers.
+		 */
+		clr_dr_intercepts(svm);
+		svm->vcpu.arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
+		return 1;
+	}
+
 	if (!boot_cpu_has(X86_FEATURE_DECODEASSISTS))
 		return emulate_on_interception(svm);
 
@@ -4302,6 +4328,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.get_dr6 = svm_get_dr6,
 	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
+	.sync_dirty_debug_regs = svm_sync_dirty_debug_regs,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
 	.set_rflags = svm_set_rflags,
-- 
1.8.3.1

Re: [PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]

On 2014-03-07 12:42, Paolo Bonzini wrote:
> Alex Williamson reported that a Windows game does something weird that
> makes the guest save and restore debug registers on each context switch.
> This cause several hundred thousands vmexits per second, and basically
> cuts performance in half when running under KVM.
> 
> However, when not running in guest-debug mode, the guest controls the
> debug registers and having to take an exit for each DR access is a waste
> of time.  We just need one vmexit to load any stale values of DR0-DR6,
> and then we can let the guest run freely.  On the next vmexit (whatever
> the reason) we will read out whatever changes the guest made to the
> debug registers.
> 
> Tested with x86/debug.flat on both Intel and AMD, both direct and
> nested virtualization.
> 
> Changes from RFC: changed get_dr7 callback to sync_dirty_debug_regs,
> 	new patches 5-7.

This looks good now to me from KVM perspective. I was just wondering how
the case is handled that the host used debug registers on the thread the
runs a VCPU? What if I set a hw breakpoint on its userspace path e.g.?
What if I debug the kernel side with kgdb?

Jan

> 
> Paolo Bonzini (7):
>   KVM: vmx: we do rely on loading DR7 on entry
>   KVM: x86: change vcpu->arch.switch_db_regs to a bit mask
>   KVM: x86: Allow the guest to run with dirty debug registers
>   KVM: vmx: Allow the guest to run with dirty debug registers
>   KVM: nVMX: Allow nested guests to run with dirty debug registers
>   KVM: svm: set/clear all DR intercepts in one swoop
>   KVM: svm: Allow the guest to run with dirty debug registers
> 
>  arch/x86/include/asm/kvm_host.h |  8 ++++-
>  arch/x86/kvm/svm.c              | 68 ++++++++++++++++++++++++++++-------------
>  arch/x86/kvm/vmx.c              | 43 ++++++++++++++++++++++++--
>  arch/x86/kvm/x86.c              | 20 +++++++++++-
>  4 files changed, 114 insertions(+), 25 deletions(-)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On 2014-03-09 09:11, Jan Kiszka wrote:
> On 2014-03-07 12:42, Paolo Bonzini wrote:
>> Alex Williamson reported that a Windows game does something weird that
>> makes the guest save and restore debug registers on each context switch.
>> This cause several hundred thousands vmexits per second, and basically
>> cuts performance in half when running under KVM.
>>
>> However, when not running in guest-debug mode, the guest controls the
>> debug registers and having to take an exit for each DR access is a waste
>> of time.  We just need one vmexit to load any stale values of DR0-DR6,
>> and then we can let the guest run freely.  On the next vmexit (whatever
>> the reason) we will read out whatever changes the guest made to the
>> debug registers.
>>
>> Tested with x86/debug.flat on both Intel and AMD, both direct and
>> nested virtualization.
>>
>> Changes from RFC: changed get_dr7 callback to sync_dirty_debug_regs,
>> 	new patches 5-7.
> 
> This looks good now to me from KVM perspective. I was just wondering how
> the case is handled that the host used debug registers on the thread the
> runs a VCPU? What if I set a hw breakpoint on its userspace path e.g.?
> What if I debug the kernel side with kgdb?

Ah, this part didn't change, we still switch between host and guest
state on vmentries/exits. All fine!

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[Paolo Bonzini]

Il 09/03/2014 09:15, Jan Kiszka ha scritto:
>> > This looks good now to me from KVM perspective. I was just wondering how
>> > the case is handled that the host used debug registers on the thread the
>> > runs a VCPU? What if I set a hw breakpoint on its userspace path e.g.?
>> > What if I debug the kernel side with kgdb?
> Ah, this part didn't change, we still switch between host and guest
> state on vmentries/exits. All fine!

Yes, that's why sync_dirty_debug_regs is added so early.  Thanks for the 
review.

Paolo

Re: [PATCH 4/7] KVM: vmx: Allow the guest to run with dirty debug registers

[Radim Krčmář]

2014-03-07 12:42+0100, Paolo Bonzini:
> When not running in guest-debug mode (i.e. the guest controls the debug
> registers, having to take an exit for each DR access is a waste of time.
> If the guest gets into a state where each context switch causes DR to be
> saved and restored, this can take away as much as 40% of the execution
> time from the guest.
> 
> If the guest is running with vcpu->arch.db == vcpu->arch.eff_db, we
> can let it write freely to the debug registers and reload them on the
> next exit.  We still need to exit on the first access, so that the
> KVM_DEBUGREG_WONT_EXIT flag is set in switch_db_regs; after that, further
> accesses to the debug registers will not cause a vmexit.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 06e4ec877a1c..e377522408b1 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2843,7 +2843,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>  		      vmx_capability.ept, vmx_capability.vpid);
>  	}
>  
> -	min = 0;
> +	min = VM_EXIT_SAVE_DEBUG_CONTROLS;
>  #ifdef CONFIG_X86_64
>  	min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
>  #endif
> @@ -5113,6 +5113,22 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> +	if (vcpu->guest_debug == 0) {
> +		u32 cpu_based_vm_exec_control;
> +
> +		cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
> +		cpu_based_vm_exec_control &= ~CPU_BASED_MOV_DR_EXITING;
> +		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);

vmcs_clear_bits() covers exactly this use-case.
(Barring the explicit bit-width.)

> +
> +		/*
> +		 * No more DR vmexits; force a reload of the debug registers
> +		 * and reenter on this instruction.  The next vmexit will
> +		 * retrieve the full state of the debug registers.
> +		 */
> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
> +		return 1;
> +	}
> +

We could make the code slighly uglier and move the functional part of
this block before the previous one, so it would do both things in one
exit.  (Exception handler will likely access DR too.)

>  	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>  	dr = exit_qualification & DEBUG_REG_ACCESS_NUM;
>  	reg = DEBUG_REG_ACCESS_REG(exit_qualification);
> @@ -5139,6 +5155,24 @@ static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
>  {
>  }
>  
> +static void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	u32 cpu_based_vm_exec_control;
> +
> +	get_debugreg(vcpu->arch.db[0], 0);
> +	get_debugreg(vcpu->arch.db[1], 1);
> +	get_debugreg(vcpu->arch.db[2], 2);
> +	get_debugreg(vcpu->arch.db[3], 3);
> +	get_debugreg(vcpu->arch.dr6, 6);
> +	vcpu->arch.dr7 = vmcs_readl(GUEST_DR7);
> +
> +	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
> +
> +	cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
> +	cpu_based_vm_exec_control |= CPU_BASED_MOV_DR_EXITING;
> +	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);

(I'll be calling this sneaky later on.)

> +}
> +
>  static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
>  {
>  	vmcs_writel(GUEST_DR7, val);
> @@ -8590,6 +8624,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
>  	.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,
>  	.get_rflags = vmx_get_rflags,
>  	.set_rflags = vmx_set_rflags,
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/7] KVM: x86: Allow the guest to run with dirty debug registers

[Radim Krčmář]

2014-03-07 12:42+0100, Paolo Bonzini:
> When not running in guest-debug mode, the guest controls the debug
> registers and having to take an exit for each DR access is a waste
> of time.  If the guest gets into a state where each context switch
> causes DR to be saved and restored, this can take away as much as 40%
> of the execution time from the guest.
> 
> After this patch, VMX- and SVM-specific code can set a flag in
> switch_db_regs, telling vcpu_enter_guest that on the next exit the debug
> registers might be dirty and need to be reloaded (syncing will be taken
> care of by a new callback in kvm_x86_ops).  This flag can be set on the
> first access to a debug registers, so that multiple accesses to the
> debug registers only cause one vmexit.
> 
> Note that since the guest will be able to read debug registers and
> enable breakpoints in DR7, we need to ensure that they are synchronized
> on entry to the guest---including DR6 that was not synced before.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/x86.c              | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 5ef59d3b6c63..74eb361eaa8f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -339,6 +339,7 @@ struct kvm_pmu {
>  
>  enum {
>  	KVM_DEBUGREG_BP_ENABLED = 1,
> +	KVM_DEBUGREG_WONT_EXIT = 2,
>  };
>  
>  struct kvm_vcpu_arch {
> @@ -707,6 +708,7 @@ struct kvm_x86_ops {
>  	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);
>  	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>  	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 252b47e85c69..c48818aa04c0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6033,12 +6033,28 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		set_debugreg(vcpu->arch.eff_db[1], 1);
>  		set_debugreg(vcpu->arch.eff_db[2], 2);
>  		set_debugreg(vcpu->arch.eff_db[3], 3);
> +		set_debugreg(vcpu->arch.dr6, 6);
>  	}
>  
>  	trace_kvm_entry(vcpu->vcpu_id);
>  	kvm_x86_ops->run(vcpu);
>  
>  	/*
> +	 * Do this here before restoring debug registers on the host.  And
> +	 * since we do this before handling the vmexit, a DR access vmexit
> +	 * can (a) read the correct value of the debug registers, (b) set
> +	 * KVM_DEBUGREG_WONT_EXIT again.

We re-enable intercepts on the next exit for the sake of simplicity?
(Batched accesses make perfect sense, but it seems we don't have to care
 about DRs at all, without guest-debug.)

> +	 */
> +	if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
> +		int i;
> +
> +		WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);

Is this possible? (I presumed that we vmexit before handling ioctls.)

> +		kvm_x86_ops->sync_dirty_debug_regs(vcpu);

Sneaky functionality ... it would be nicer to split 'enable DR
intercepts' into a new kvm op.
I think we want to disable them whenever we are not in guest-debug mode
anyway, so it would be a pair.
And we don't have to modify DR intercepts then, which is probably the
main reason why sync_dirty_debug_regs() does two things.

> +		for (i = 0; i < KVM_NR_DB_REGS; i++)
> +			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
> +	}
> +
> +	/*
>  	 * If the guest has used debug registers, at least dr7
>  	 * will be disabled while returning to the host.
>  	 * If we don't have active breakpoints in the host, we don't
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/7] KVM: x86: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

Il 09/03/2014 19:28, Radim Krčmář ha scritto:
>> >  	/*
>> > +	 * Do this here before restoring debug registers on the host.  And
>> > +	 * since we do this before handling the vmexit, a DR access vmexit
>> > +	 * can (a) read the correct value of the debug registers, (b) set
>> > +	 * KVM_DEBUGREG_WONT_EXIT again.
> We re-enable intercepts on the next exit for the sake of simplicity?
> (Batched accesses make perfect sense, but it seems we don't have to care
>  about DRs at all, without guest-debug.)

It's not just for the sake of simplicity.  Synchronizing debug registers 
on entry does have some cost, and it's required for running without 
debug register intercepts.  You would incur this cost always, since 
no-guest-debug is actually the common case.

We're well into diminishing returns; Alex timed this patch vs. one that 
completely ignores MOV DR exits and (to the surprise of both of us) this 
patch completely restored performance despite still having a few tens of 
thousands MOV DR exits/second.

In the problematic case, each context switch will do a save and restore 
of debug registers; that's in total 13 debug register accesses (read 6 
registers, write DR7 to disable all registers, write 6 registers 
including DR7 which enables breakpoints), all very close in time.  It's 
quite likely that the final DR7 write is very expensive (e.g. it might 
have to flush the pipeline).  Also, this test case must be very heavy on 
context switches, and each context switch already incurs a few exits 
(for example to inject the interrupt that causes it, to read the current 
time).

A different optimization could be to skip the LOAD_DEBUG_CONTROLS 
vm-entry control if DR7 == host DR7 == 0x400 && MOV DR exits are 
enabled.  Not sure it's worthwhile though, and there's also the DEBUGCTL 
MSR to take into account.  Better do these kinds of tweaks if they 
actually show up in the profile: Alex's testcase shows that when they 
do, the impact is massive.

>> +		kvm_x86_ops->sync_dirty_debug_regs(vcpu);
>
> Sneaky functionality ... it would be nicer to split 'enable DR
> intercepts' into a new kvm op.

Why?  "Disable DR intercepts" is already folded into the handler for MOV 
DR exits.

> And we don't have to modify DR intercepts then, which is probably the
> main reason why sync_dirty_debug_regs() does two things.

Another is that indirect calls are relatively expensive and add 
complexity; in this case they would always be used back-to-back.

Paolo

Re: [PATCH 4/7] KVM: vmx: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

Il 09/03/2014 19:26, Radim Krčmář ha scritto:
> > +
> > +		cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
> > +		cpu_based_vm_exec_control &= ~CPU_BASED_MOV_DR_EXITING;
> > +		vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, cpu_based_vm_exec_control);
>
> vmcs_clear_bits() covers exactly this use-case.
> (Barring the explicit bit-width.)

Good idea.

> > +
> > +		/*
> > +		 * No more DR vmexits; force a reload of the debug registers
> > +		 * and reenter on this instruction.  The next vmexit will
> > +		 * retrieve the full state of the debug registers.
> > +		 */
> > +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
> > +		return 1;
> > +	}
> > +
>
> We could make the code slighly uglier and move the functional part of
> this block before the previous one, so it would do both things in one
> exit.

I considered this, but decided that it's unlikely for emulation to be 
faster than hardware---especially on those AMD CPUs that lack decode 
assists (and it's good for VMX and SVM code to look as similar as possible).

> (Exception handler will likely access DR too.)

Which exception handler?

Paolo

Re: [PATCH 3/7] KVM: x86: Allow the guest to run with dirty debug registers

[Radim Krčmář]

2014-03-09 21:07+0100, Paolo Bonzini:
> Il 09/03/2014 19:28, Radim Krčmář ha scritto:
> >>>  	/*
> >>> +	 * Do this here before restoring debug registers on the host.  And
> >>> +	 * since we do this before handling the vmexit, a DR access vmexit
> >>> +	 * can (a) read the correct value of the debug registers, (b) set
> >>> +	 * KVM_DEBUGREG_WONT_EXIT again.
> >We re-enable intercepts on the next exit for the sake of simplicity?
> >(Batched accesses make perfect sense, but it seems we don't have to care
> > about DRs at all, without guest-debug.)
> 
> It's not just for the sake of simplicity.  Synchronizing debug
> registers on entry does have some cost, and it's required for
> running without debug register intercepts.  You would incur this
> cost always, since no-guest-debug is actually the common case.

Good point, it's just this case that accesses them often, most guest
won't use them at all ...

> We're well into diminishing returns; Alex timed this patch vs. one
> that completely ignores MOV DR exits and (to the surprise of both of
> us) this patch completely restored performance despite still having
> a few tens of thousands MOV DR exits/second.
> 
> In the problematic case, each context switch will do a save and
> restore of debug registers; that's in total 13 debug register
> accesses (read 6 registers, write DR7 to disable all registers,
> write 6 registers including DR7 which enables breakpoints), all very
> close in time.  It's quite likely that the final DR7 write is very
> expensive (e.g. it might have to flush the pipeline).  Also, this
> test case must be very heavy on context switches, and each context
> switch already incurs a few exits (for example to inject the
> interrupt that causes it, to read the current time).

We would save just one exit and there are probably enough non-DR exits
even in this case to balance it out.
I've been carried too far away for design.
(Thanks for the details.)

> A different optimization could be to skip the LOAD_DEBUG_CONTROLS
> vm-entry control if DR7 == host DR7 == 0x400 && MOV DR exits are
> enabled.  Not sure it's worthwhile though, and there's also the
> DEBUGCTL MSR to take into account.  Better do these kinds of tweaks
> if they actually show up in the profile: Alex's testcase shows that
> when they do, the impact is massive.

Yeah, we'd have to implement TRUE-VMX-MSR handling just for two
load/stores ...

> >>+		kvm_x86_ops->sync_dirty_debug_regs(vcpu);
> >
> >Sneaky functionality ... it would be nicer to split 'enable DR
> >intercepts' into a new kvm op.
> 
> Why?  "Disable DR intercepts" is already folded into the handler for
> MOV DR exits.

It's not obvious from the name that we also enable intercepts again,
which is more important part, imho.
(I don't like the folded code much, but considering the alternatives,
 it's a good solution.)

> >And we don't have to modify DR intercepts then, which is probably the
> >main reason why sync_dirty_debug_regs() does two things.
> 
> Another is that indirect calls are relatively expensive and add
> complexity; in this case they would always be used back-to-back.

True.
(I could not come up with a good name to this function,
 intercept_debug_registers() does not reveal that we also save them, and
 a longer name would be hard to read without 'and', which looks weird.)

Re: [PATCH 4/7] KVM: vmx: Allow the guest to run with dirty debug registers

[Radim Krčmář]

2014-03-09 21:12+0100, Paolo Bonzini:
> Il 09/03/2014 19:26, Radim Krčmář ha scritto:
> >> +
> >> +		/*
> >> +		 * No more DR vmexits; force a reload of the debug registers
> >> +		 * and reenter on this instruction.  The next vmexit will
> >> +		 * retrieve the full state of the debug registers.
> >> +		 */
> >> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
> >> +		return 1;
> >> +	}
> >> +
> >
> >We could make the code slighly uglier and move the functional part of
> >this block before the previous one, so it would do both things in one
> >exit.
> 
> I considered this, but decided that it's unlikely for emulation to
> be faster than hardware---especially on those AMD CPUs that lack
> decode assists (and it's good for VMX and SVM code to look as
> similar as possible).

I meant to move it before the block that exits if there is the
'exception on access' bit set in cr7, so we wouldn't exit again right
away on the actual access, which is quite likely.
(Exiting without emulation was a great move.)

> >(Exception handler will likely access DR too.)
> 
> Which exception handler?

For #DB. (Pure guesswork, I haven't seen any of them.)

Re: [PATCH 0/7] KVM: x86: Let the guest write to multiple debug registers with one vmexit

[Radim Krčmář]

2014-03-07 12:42+0100, Paolo Bonzini:
> Alex Williamson reported that a Windows game does something weird that
> makes the guest save and restore debug registers on each context switch.
> This cause several hundred thousands vmexits per second, and basically
> cuts performance in half when running under KVM.
> 
> However, when not running in guest-debug mode, the guest controls the
> debug registers and having to take an exit for each DR access is a waste
> of time.  We just need one vmexit to load any stale values of DR0-DR6,
> and then we can let the guest run freely.  On the next vmexit (whatever
> the reason) we will read out whatever changes the guest made to the
> debug registers.
> 
> Tested with x86/debug.flat on both Intel and AMD, both direct and
> nested virtualization.
> 
> Changes from RFC: changed get_dr7 callback to sync_dirty_debug_regs,
> 	new patches 5-7.
> 
> Paolo Bonzini (7):
>   KVM: vmx: we do rely on loading DR7 on entry
>   KVM: x86: change vcpu->arch.switch_db_regs to a bit mask
>   KVM: x86: Allow the guest to run with dirty debug registers
>   KVM: vmx: Allow the guest to run with dirty debug registers
>   KVM: nVMX: Allow nested guests to run with dirty debug registers
>   KVM: svm: set/clear all DR intercepts in one swoop
>   KVM: svm: Allow the guest to run with dirty debug registers

All patches,
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

This series is good even without vmcs_{set,clr}_bits().
(There is enough of them already to warrant a cleanup patch.)

> 
>  arch/x86/include/asm/kvm_host.h |  8 ++++-
>  arch/x86/kvm/svm.c              | 68 ++++++++++++++++++++++++++++-------------
>  arch/x86/kvm/vmx.c              | 43 ++++++++++++++++++++++++--
>  arch/x86/kvm/x86.c              | 20 +++++++++++-
>  4 files changed, 114 insertions(+), 25 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/7] KVM: vmx: Allow the guest to run with dirty debug registers

[Paolo Bonzini]

Il 10/03/2014 13:17, Radim Krčmář ha scritto:
>> > I considered this, but decided that it's unlikely for emulation to
>> > be faster than hardware---especially on those AMD CPUs that lack
>> > decode assists (and it's good for VMX and SVM code to look as
>> > similar as possible).
> I meant to move it before the block that exits if there is the
> 'exception on access' bit set in cr7, so we wouldn't exit again right
> away on the actual access, which is quite likely.
> (Exiting without emulation was a great move.)
>

Ah yes, for the GD bit.  I don't know, it's quite unlikely that anyone 
will use it...  It's a feeble attempt at allowing virtualization of the 
debug registers without VMX.

Paolo