Date:   Tue, 21 Mar 2023 10:52:14 -0700
In-Reply-To: <20230119141946.585610-2-yu.c.zhang@linux.intel.com>
Mime-Version: 1.0
References: <20230119141946.585610-1-yu.c.zhang@linux.intel.com> <20230119141946.585610-2-yu.c.zhang@linux.intel.com>
Message-ID: <ZBnuzoWuwAPoy0+N@google.com>
Subject: Re: [PATCH v2 2/2] KVM: nVMX: Add helpers to setup VMX control msr configs
From:   Sean Christopherson <seanjc@google.com>
To:     Yu Zhang <yu.c.zhang@linux.intel.com>
Cc:     pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thu, Jan 19, 2023, Yu Zhang wrote:
> nested_vmx_setup_ctls_msrs() is used to set up the various VMX MSR
> controls for nested VMX. But it is a bit lengthy, just add helpers
> to setup the configuration of VMX MSRs.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 129 +++++++++++++++++++++++++-------------
>  1 file changed, 85 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 81dfbffae575..98ed7631e810 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6750,36 +6750,9 @@ static u64 nested_vmx_calc_vmcs_enum_msr(void)
>  	return (u64)max_idx << VMCS_FIELD_INDEX_SHIFT;
>  }
>  
> -/*
> - * nested_vmx_setup_ctls_msrs() sets up variables containing the values to be
> - * returned for the various VMX controls MSRs when nested VMX is enabled.
> - * The same values should also be used to verify that vmcs12 control fields are
> - * valid during nested entry from L1 to L2.
> - * Each of these control msrs has a low and high 32-bit half: A low bit is on
> - * if the corresponding bit in the (32-bit) control field *must* be on, and a
> - * bit in the high half is on if the corresponding bit in the control field
> - * may be on. See also vmx_control_verify().
> - */
> -void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
> +static inline void nested_vmx_setup_pinbased_ctls(struct vmcs_config *vmcs_conf,

No need for the "inline", this isn't performance sensitive code, and odds are very,
very good the compiler will inline the code anyways.

> @@ -6953,13 +6943,16 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
>  		VMX_MISC_ACTIVITY_HLT |
>  		VMX_MISC_ACTIVITY_WAIT_SIPI;
>  	msrs->misc_high = 0;
> +}
>  
> -	/*
> -	 * This MSR reports some information about VMX support. We
> -	 * should return information about the VMX we emulate for the
> -	 * guest, and the VMCS structure we give it - not about the
> -	 * VMX support of the underlying hardware.
> -	 */
> +/*
> + * VMX basic MSR reports some information about VMX support. We should
> + * return information about the VMX we emulate for the guest, and the
> + * VMCS structure we give it - not about the VMX support of the underlying
> + * hardware.
> + */
> +static inline void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
> +{
>  	msrs->basic =
>  		VMCS12_REVISION |
>  		VMX_BASIC_TRUE_CTLS |
> @@ -6968,12 +6961,15 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
>  
>  	if (cpu_has_vmx_basic_inout())
>  		msrs->basic |= VMX_BASIC_INOUT;
> +}
>  
> -	/*
> -	 * These MSRs specify bits which the guest must keep fixed on
> -	 * while L1 is in VMXON mode (in L1's root mode, or running an L2).
> -	 * We picked the standard core2 setting.
> -	 */
> +/*
> + * cr0_fixed & cr4_fixed MSRs specify bits which the guest must keep fixed
> + * on while L1 is in VMXON mode (in L1's root mode, or running an L2).
> + * We picked the standard core2 setting.
> + */

This change is flawed, the comment is specific to the fixed0 MSRs, not to all
flavors of fixed MSRs and thus not to the function as a whole.  As much as I want
to clean up the comments, for this patch I think it's best to leave them alone.

No need to send a v2, I'll fixup when applying.