Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c7681cf8-7b99-eb43-0195-d35adb011f21@redhat.com>
Date:   Fri, 25 Feb 2022 12:54:45 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3] KVM: VMX: Enable Notify VM exit
Content-Language: en-US
To:     Chenyi Qiang <chenyi.qiang@intel.com>,
        Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Xiaoyao Li <xiaoyao.li@intel.com>
Cc:     Tao Xu <tao3.xu@intel.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220223062412.22334-1-chenyi.qiang@intel.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <20220223062412.22334-1-chenyi.qiang@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?  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).

> +		 * 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);

What do you think?

Paolo