Messages in this thread | | | Date | Fri, 25 Feb 2022 20:46:05 +0800 | Subject | Re: [PATCH v3] KVM: VMX: Enable Notify VM exit | From | Xiaoyao Li <> |
| |
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 >
| |