lkml.org 
[lkml]   [2022]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] KVM: VMX: Enable Notify VM exit
From
On 2/25/2022 7:54 PM, Paolo Bonzini wrote:
> On 2/23/22 07:24, Chenyi Qiang wrote:
>> Nested handling
>> - Nested notify VM exits are not supported yet. Keep the same notify
>>    window control in vmcs02 as vmcs01, so that L1 can't escape the
>>    restriction of notify VM exits through launching L2 VM.
>> - When L2 VM is context invalid, synthesize a nested
>>    EXIT_REASON_TRIPLE_FAULT to L1 so that L1 won't be killed due to L2's
>>    VM_CONTEXT_INVALID happens.
>>
>> Notify VM exit is defined in latest Intel Architecture Instruction Set
>> Extensions Programming Reference, chapter 9.2.
>>
>> TODO: Allow to change the window size (to enable the feature) at runtime,
>> which can make it more flexible to do management.
>
> I only have a couple questions, any changes in response to the question
> I can do myself.
>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 1dfe23963a9e..f306b642c3e1 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2177,6 +2177,9 @@ static void prepare_vmcs02_constant_state(struct
>> vcpu_vmx *vmx)
>>       if (cpu_has_vmx_encls_vmexit())
>>           vmcs_write64(ENCLS_EXITING_BITMAP, INVALID_GPA);
>> +    if (notify_window >= 0)
>> +        vmcs_write32(NOTIFY_WINDOW, notify_window);
>
> Is a value of 0 valid?

Yes, 0 is valid. That's why there is an internal value to ensure even 0
won't cause false positive

> Should it be changed to the recommended value of
> 128000 in hardware_setup()?
>
>> +    case EXIT_REASON_NOTIFY:
>> +        return nested_cpu_has2(vmcs12,
>> +            SECONDARY_EXEC_NOTIFY_VM_EXITING);
>
> This should be "return false" since you don't expose the secondary
> control to L1 (meaning, it will never be set).

Fine with either.

>> +         * L0 will synthensize a nested TRIPLE_FAULT to kill L2 when
>> +         * notify VM exit occurred in L2 and
>> NOTIFY_VM_CONTEXT_INVALID is
>> +         * set in exit qualification. In this case, if notify VM exit
>> +         * occurred incident to delivery of a vectored event, the IDT
>> +         * vectoring info are recorded in VMCS. Drop the pending event
>> +         * in vmcs12, otherwise L1 VMM will exit to userspace with
>> +         * internal error due to delivery event.
>>           */
>> -        vmcs12_save_pending_event(vcpu, vmcs12);
>> +        if (to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_NOTIFY)
>> +            vmcs12_save_pending_event(vcpu, vmcs12);
>
> I would prefer to call out the triple fault here:
>
>                 /*
>                  * Transfer the event that L0 or L1 may have wanted to
> inject into
>                  * L2 to IDT_VECTORING_INFO_FIELD.
>                  *
>                  * Skip this if the exit is due to a
> NOTIFY_VM_CONTEXT_INVALID
>                  * exit; in that case, L0 will synthesize a nested
> TRIPLE_FAULT
>                  * vmexit to kill L2.  No IDT vectoring info is
> recorded for
>                  * triple faults, and __vmx_handle_exit does not expect
> it.
>                  */
>                 if (!(to_vmx(vcpu)->exit_reason.basic ==
> EXIT_REASON_NOTIFY)
>                       && kvm_test_request(KVM_REQ_TRIPLE_FAULT, vcpu))
>                         vmcs12_save_pending_event(vcpu, vmcs12);

looks good to me.

> What do you think?
>
> Paolo
>

\
 
 \ /
  Last update: 2022-02-25 13:49    [W:0.303 / U:1.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site