Skip to contentSkip to conversation

KVM: VMX: Conditionally reload debug register 6

PATCH Applied Avi Kivity 3 participants 7 messages Sep 1, 2009 – Sep 3, 2009

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Avi Kivity

author
#0 lore raw

PATCH v2 1/1 KVM: VMX: Conditionally reload debug register 6

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Signed-off-by Avi Kivity <[email protected]>

---
v2: trap #DB so we maintain the TF related bits of DR7.

 arch/x86/kvm/vmx.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +9 -5
index 05cd554..d4be978 100644
@@ -540,10 +540,12 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
 	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_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_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
-			eb |= 1u << DB_VECTOR;
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
 			eb |= 1u << BP_VECTOR;
 	}

@@ -3629,7 +3631,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	vmcs_writel(HOST_CR0, read_cr0());
 
-	set_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		set_debugreg(vcpu->arch.dr6, 6);
 
 	asm(
 		/* Store host registers */

@@ -3731,7 +3734,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 				  | (1 << VCPU_EXREG_PDPTR));
 	vcpu->arch.regs_dirty = 0;
 
-	get_debugreg(vcpu->arch.dr6, 6);
+	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)
Signature · 1 line
1.6.4.1
#1 lore raw
Quoted 20 lines · Avi Kivity

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Signed-off-by Avi Kivity <[email protected]>

---
v2: trap #DB so we maintain the TF related bits of DR7.

 arch/x86/kvm/vmx.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +0 -0
index 05cd554..d4be978 100644

arch/x86/kvm/vmx.c static void update_exception_bitmap(struct kvm_vcpu *vcpu) · line 547

Quoted 8 lines
arch/x86/kvm/vmx.c +5 -0
@@ -540,10 +540,12 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
 	eb = (1u << PF_VECTOR) | (1u << UD_VECTOR) | (1u << MC_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 this is safe...

Quoted 18 lines
arch/x86/kvm/vmx.c +2 -4
 	if (vcpu->guest_debug & KVM_GUESTDBG_ENABLE) {
-		if (vcpu->guest_debug &
-		    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
-			eb |= 1u << DB_VECTOR;
 		if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
 			eb |= 1u << BP_VECTOR;
 	}

@@ -3629,7 +3631,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	vmcs_writel(HOST_CR0, read_cr0());
 
-	set_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		set_debugreg(vcpu->arch.dr6, 6);
 
 	asm(
 		/* Store host registers */

arch/x86/kvm/vmx.c static void vmx_vcpu_run(struct kvm_vcpu *vcpu) · line 3741

Quoted 9 lines
arch/x86/kvm/vmx.c +2 -1
@@ -3731,7 +3734,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 				  | (1 << VCPU_EXREG_PDPTR));
 	vcpu->arch.regs_dirty = 0;
 
-	get_debugreg(vcpu->arch.dr6, 6);
+	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)

...I wonder why we cannot drop this save/restore? The guest is not able to access dr6 without causing a trap, thus will never see dr6 as stored in the hardware.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
author
#2 lore raw
Quoted 59 lines · Jan Kiszka
Quoted 20 lines · Avi Kivity

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Signed-off-by Avi Kivity<[email protected]>

---
v2: trap #DB so we maintain the TF related bits of DR7.

  arch/x86/kvm/vmx.c |   14 +++++++++-----
  1 files changed, 9 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +0 -0
index 05cd554..d4be978 100644

arch/x86/kvm/vmx.c static void update_exception_bitmap(struct kvm_vcpu *vcpu) · line 548

Quoted 9 lines
arch/x86/kvm/vmx.c +5 -0
@@ -540,10 +540,12 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
  	eb = (1u<<  PF_VECTOR) | (1u<<  UD_VECTOR) | (1u<<  MC_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 this is safe...

Quoted 25 lines
arch/x86/kvm/vmx.c +0 -3
  	if (vcpu->guest_debug&  KVM_GUESTDBG_ENABLE) {
-		if (vcpu->guest_debug&
-		    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
-			eb |= 1u<<  DB_VECTOR;
  		if (vcpu->guest_debug&  KVM_GUESTDBG_USE_SW_BP)
  			eb |= 1u<<  BP_VECTOR;

  	}
arch/x86/kvm/vmx.c +4 -2
@@ -3629,7 +3631,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
  	 */
  	vmcs_writel(HOST_CR0, read_cr0());
 
-	set_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		set_debugreg(vcpu->arch.dr6, 6);
 
  	asm(
  		/* Store host registers */

@@ -3731,7 +3734,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
  				  | (1<<  VCPU_EXREG_PDPTR));
  	vcpu->arch.regs_dirty = 0;
 
-	get_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		get_debugreg(vcpu->arch.dr6, 6);
Quoted 7 lines

arch/x86/kvm/vmx.c static void vmx_vcpu_run(struct kvm_vcpu *vcpu) · line 3741

Quoted 3 lines
arch/x86/kvm/vmx.c +0 -0
  	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
  	if (vmx->rmode.irq.pending)

...I wonder why we cannot drop this save/restore? The guest is not able to access dr6 without causing a trap, thus will never see dr6 as stored in the hardware.

I think you're right.

btw, something else I've considered was not to do any emulation for 'mov dr' but instead load the debug registers and disable the intercept. So far the only issue I've seen is that we lose support for real mode guest self-debug on intel (pre-unrestricted guest). What do you think of this?

Signature · 1 line
error compiling committee.c: too many arguments to function
#3 lore raw
Quoted 68 lines · Avi Kivity
Quoted 59 lines · Jan Kiszka
Quoted 20 lines · Avi Kivity

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Signed-off-by Avi Kivity<[email protected]>

---
v2: trap #DB so we maintain the TF related bits of DR7.

  arch/x86/kvm/vmx.c |   14 +++++++++-----
  1 files changed, 9 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +0 -0
index 05cd554..d4be978 100644

arch/x86/kvm/vmx.c static void update_exception_bitmap(struct kvm_vcpu *vcpu) · line 548

Quoted 9 lines
arch/x86/kvm/vmx.c +5 -0
@@ -540,10 +540,12 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
  	eb = (1u<<  PF_VECTOR) | (1u<<  UD_VECTOR) | (1u<<  MC_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 this is safe...

Quoted 25 lines
arch/x86/kvm/vmx.c +0 -3
  	if (vcpu->guest_debug&  KVM_GUESTDBG_ENABLE) {
-		if (vcpu->guest_debug&
-		    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))
-			eb |= 1u<<  DB_VECTOR;
  		if (vcpu->guest_debug&  KVM_GUESTDBG_USE_SW_BP)
  			eb |= 1u<<  BP_VECTOR;

  	}
arch/x86/kvm/vmx.c +4 -2
@@ -3629,7 +3631,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
  	 */
  	vmcs_writel(HOST_CR0, read_cr0());
 
-	set_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		set_debugreg(vcpu->arch.dr6, 6);
 
  	asm(
  		/* Store host registers */

@@ -3731,7 +3734,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
  				  | (1<<  VCPU_EXREG_PDPTR));
  	vcpu->arch.regs_dirty = 0;
 
-	get_debugreg(vcpu->arch.dr6, 6);
+	if (vcpu->arch.switch_db_regs)
+		get_debugreg(vcpu->arch.dr6, 6);
Quoted 7 lines

arch/x86/kvm/vmx.c static void vmx_vcpu_run(struct kvm_vcpu *vcpu) · line 3741

Quoted 3 lines
arch/x86/kvm/vmx.c +0 -0
  	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
  	if (vmx->rmode.irq.pending)

...I wonder why we cannot drop this save/restore? The guest is not able to access dr6 without causing a trap, thus will never see dr6 as stored in the hardware.

Quoted 6 lines

I think you're right.

btw, something else I've considered was not to do any emulation for 'mov dr' but instead load the debug registers and disable the intercept. So far the only issue I've seen is that we lose support for real mode guest self-debug on intel (pre-unrestricted guest). What do you think of this?

I think you can't have both: optimized dr save/restore on vmentry/exit and optimized dr access. If you drop on-demand dr register readout/update, you need to deal with this on every vmentry/exit. My feeling is that this would be awfully slower, even slower than what we currently have without your patches.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
author
#4 lore raw
Quoted 11 lines · Jan Kiszka
Quoted 5 lines

btw, something else I've considered was not to do any emulation for 'mov dr' but instead load the debug registers and disable the intercept. So far the only issue I've seen is that we lose support for real mode guest self-debug on intel (pre-unrestricted guest). What do you think of this?

I think you can't have both: optimized dr save/restore on vmentry/exit and optimized dr access. If you drop on-demand dr register readout/update, you need to deal with this on every vmentry/exit. My feeling is that this would be awfully slower, even slower than what we currently have without your patches.

I'll clarify:

In the normal case, we'll have #DB and MOV DR intercepted.

On #DB, update vcpu->dr6.
On MOV DR, disable MOV DR interception, enable dr swap, and let the guest execute; on the next exit, reenable MOV DR interception and disable dr swap.

It's similar to the fpu code where we don't emulate fpu instructions. It doesn't really buy us a lot.

Signature · 1 line
error compiling committee.c: too many arguments to function
Applied
#5 lore raw
In reply to Avi Kivity
Quoted 6 lines · Avi Kivity

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Applied, thanks.

#6 lore raw
Quoted 10 lines · Marcelo Tosatti
Quoted 6 lines · Avi Kivity

Only reload debug register 6 if we're running with the guest's debug registers. Saves around 150 cycles from the guest lightweight exit path.

dr6 contains a couple of bits that are updated on #DB, so intercept that unconditionally and update those bits then.

Applied, thanks.

Will send a cleanup patch based on the discussion with Avi later. It just requires some testing. And I also have a kgdb-safe(r) version of the switch optimization here.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux