[PATCH v2] KVM: VMX: Conditionally reload debug register 6

[PATCH v2] KVM: VMX: Conditionally reload debug register 6

[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 <avi@redhat.com>
---
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(-)


diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 05cd554..d4be978 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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)
-- 
1.6.4.1

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Jan Kiszka]

Avi Kivity wrote:
> 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 <avi@redhat.com>
> ---
> 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(-)
> 
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 05cd554..d4be978 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -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...

>  	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)

...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

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

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Avi Kivity]

On 09/01/2009 04:15 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> 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<avi@redhat.com>
>> ---
>> 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(-)
>>
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 05cd554..d4be978 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -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...
>
>    
>>   	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)
>>      
> ...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?

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

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Jan Kiszka]

Avi Kivity wrote:
> On 09/01/2009 04:15 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> 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<avi@redhat.com>
>>> ---
>>> 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(-)
>>>
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 05cd554..d4be978 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -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...
>>
>>    
>>>   	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)
>>>      
>> ...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?

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

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

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Avi Kivity]

On 09/01/2009 04:47 PM, Jan Kiszka wrote:
>> 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.

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

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Marcelo Tosatti]

On Tue, Sep 01, 2009 at 04:06:25PM +0300, Avi Kivity wrote:
> 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.

Re: [PATCH v2] KVM: VMX: Conditionally reload debug register 6

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Tue, Sep 01, 2009 at 04:06:25PM +0300, Avi Kivity wrote:
>> 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

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