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

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

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

On 2013-08-25 10:18, Abel Gordon wrote:
> 
> 
> kvm-owner@vger.kernel.org wrote on 25/08/2013 10:54:13 AM:
> 
>> From: Jan Kiszka <jan.kiszka@web.de>
>> To: Abel Gordon/Haifa/IBM@IBMIL,
>> Cc: gleb@redhat.com, kvm <kvm@vger.kernel.org>, pbonzini@redhat.com,
>> "李春奇 <Arthur Chunqi Li>"  <yzt356@gmail.com>
>> Date: 25/08/2013 10:54 AM
>> Subject: Re: [PATCH] KVM: nVMX: Fully support of nested VMX preemption
> timer
>> Sent by: kvm-owner@vger.kernel.org
>>
>> On 2013-08-25 09:50, Abel Gordon wrote:
>>>
>>>
>>> kvm-owner@vger.kernel.org wrote on 25/08/2013 10:43:12 AM:
>>>
>>>> From: Jan Kiszka <jan.kiszka@web.de>
>>>> To: Abel Gordon/Haifa/IBM@IBMIL,
>>>> Cc: gleb@redhat.com, kvm@vger.kernel.org, kvm-owner@vger.kernel.org,
>>>> pbonzini@redhat.com, "李春奇 <Arthur Chunqi Li>" <yzt356@gmail.com>
>>>> Date: 25/08/2013 10:43 AM
>>>> Subject: Re: [PATCH] KVM: nVMX: Fully support of nested VMX preemption
>>> timer
>>>> Sent by: kvm-owner@vger.kernel.org
>>>>
>>>> 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.
>>>
>>> But don't you think we should also reduce the cycles spent at L0 from
> the
>>> preemption timer ? I mean, if we spent X cycles at L0 handling a L2
> exit
>>> which was not forwarded to L1, then, before we resume L2,
>>> the preemption timer should be: (previous_value_on_exit - X).
>>> If (previous_value_on_exit - X) < 0, then we should force ("emulate") a
>>> preemption timer exit between L2 and L1.
>>
>> We ask the hardware to save the value of the preemption on L2 exit. This
>> value will be exposed to L1 (if it asked for saving as well) and/or be
>> written back to the hardware on L2 reenty (unless L1 had a chance to run
>> and modified it). So the time spent in L0 is implicitly subtracted.
> 
> I think you are suggesting the following, please correct me if I am wrong.
> 1) L1 resumes L2 with preemption timer enabled
> 2) L0 emulates the resume/launch
> 3) L2 runs for Y cycles until an external interrupt occurs (Y < preemption
> timer specified by L1)
> 4) L0 saved the preemption timer (original value - Y)
> 5) L0 spends X cycles handling the external interrupt
> 6) L0 resumes L2 with preemption timer = original value - Y
> 
> Note that in this case "X is ignored".

Yes, but see my other reply.

> 
> I was suggesting to do the following:
> 6) If original value - Y - X > 0 then
>  L0 resumes L2 with preemption timer = original value - Y - X
> else
>  L0 emulates a L2->L1 preemption timer exit (resumes L1)

Almost . 6) should be:
If exit to L1 occurred after last L2, set X to 0. Then load MAX(original
value - Y - X, 0).

The hardware will trigger the exit for us.

Jan



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