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@xxxxxxxxxx>
> Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> ---
> 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.
