Re: [PATCH] KVM: nVMX: Fully support of nested VMX preemption timer

[Jan Kiszka <jan.kiszka@web.de>]

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

On 2013-08-25 09:37, Abel Gordon wrote:
> 
> 
>> From: Jan Kiszka <jan.kiszka@web.de>
>> To: "李春奇 <Arthur Chunqi Li>"  <yzt356@gmail.com>,
>> Cc: kvm@vger.kernel.org, gleb@redhat.com, pbonzini@redhat.com
>> Date: 25/08/2013 09:44 AM
>> Subject: Re: [PATCH] KVM: nVMX: Fully support of nested VMX preemption
> timer
>> Sent by: kvm-owner@vger.kernel.org
>>
>> On 2013-08-24 20:44, root wrote:
>>> This patch contains the following two changes:
>>> 1. Fix the bug in nested preemption timer support. If vmexit L2->L0
>>> with some reasons not emulated by L1, preemption timer value should
>>> be save in such exits.
>>> 2. Add support of "Save VMX-preemption timer value" VM-Exit controls
>>> to nVMX.
>>>
>>> With this patch, nested VMX preemption timer features are fully
>>> supported.
>>>
>>> Signed-off-by: Arthur Chunqi Li <yzt356@gmail.com>
>>> ---
> 
>>>
>>> @@ -7578,9 +7579,14 @@ static void prepare_vmcs02(struct kvm_vcpu
>> *vcpu, struct vmcs12 *vmcs12)
>>>        (vmcs_config.pin_based_exec_ctrl |
>>>         vmcs12->pin_based_vm_exec_control));
>>>
>>> -   if (vmcs12->pin_based_vm_exec_control &
> PIN_BASED_VMX_PREEMPTION_TIMER)
>>> -      vmcs_write32(VMX_PREEMPTION_TIMER_VALUE,
>>> -              vmcs12->vmx_preemption_timer_value);
>>> +   if (vmcs12->pin_based_vm_exec_control &
>> PIN_BASED_VMX_PREEMPTION_TIMER) {
>>> +      if (vmcs12->vm_exit_controls &
> VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
>>> +         vmcs12->vmx_preemption_timer_value =
>>> +            vmcs_read32(VMX_PREEMPTION_TIMER_VALUE);
>>> +      else
>>> +         vmcs_write32(VMX_PREEMPTION_TIMER_VALUE,
>>> +               vmcs12->vmx_preemption_timer_value);
>>> +   }
>>
>> This is not correct. We still need to set the vmcs to
>> vmx_preemption_timer_value. The difference is that, on exit from L2,
>> vmx_preemption_timer_value has to be updated according to the saved
>> hardware state. The corresponding code is missing in your patch so far.
> 
> I think something else maybe be missing here: assuming L0 handles exits
> for L2 without involving L1 (e.g. external interrupts or ept violations),
> then, we may spend some cycles in L0 handling these exits. Note L1 is not
> aware of these exits and from L1 perspective L2 was running on the CPU.
> That means that we may need to reduce these cycles spent at
> L0 from the preemtion timer or emulate a preemption timer exit to
> force a transition to L1 instead of resuming L2.

That's precisely what the logic I described should achieve: reload the
value we saved on L2 exit on reentry.

Jan



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