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

[Jan Kiszka <jan.kiszka@siemens.com>]

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