[PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[Yu Zhang]

nested_vmx_setup_ctls_msrs() initializes the vmcs_conf.nested,
which stores the global VMX MSR configurations when nested is
supported, regardless of any particular CPUID settings for one
VM.

Commit 6defc591846d ("KVM: nVMX: include conditional controls
in /dev/kvm KVM_GET_MSRS") added the some feature flags for
secondary proc-based controls, so that those features can be
available in KVM_GET_MSRS. Yet this commit did not remove the
obsolete comments in nested_vmx_setup_ctls_msrs().

Just fix the comments, and no functional change intended.

Fixes: 6defc591846d ("KVM: nVMX: include conditional controls in /dev/kvm KVM_GET_MSRS")
Reported-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 arch/x86/kvm/vmx/nested.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b6f4411b613e..76cca5d5aa6b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6854,11 +6854,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 	msrs->procbased_ctls_low &=
 		~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING);
 
-	/*
-	 * secondary cpu-based controls.  Do not include those that
-	 * depend on CPUID bits, they are added later by
-	 * vmx_vcpu_after_set_cpuid.
-	 */
+	/* secondary cpu-based controls */
 	msrs->secondary_ctls_low = 0;
 
 	msrs->secondary_ctls_high = vmcs_conf->cpu_based_2nd_exec_ctrl;
-- 
2.17.1

Re: [PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[Sean Christopherson]

On Thu, Dec 15, 2022, Yu Zhang wrote:
> nested_vmx_setup_ctls_msrs() initializes the vmcs_conf.nested,
> which stores the global VMX MSR configurations when nested is
> supported, regardless of any particular CPUID settings for one
> VM.
> 
> Commit 6defc591846d ("KVM: nVMX: include conditional controls
> in /dev/kvm KVM_GET_MSRS") added the some feature flags for
> secondary proc-based controls, so that those features can be
> available in KVM_GET_MSRS. Yet this commit did not remove the
> obsolete comments in nested_vmx_setup_ctls_msrs().
> 
> Just fix the comments, and no functional change intended.
> 
> Fixes: 6defc591846d ("KVM: nVMX: include conditional controls in /dev/kvm KVM_GET_MSRS")
> Reported-by: Sean Christopherson <seanjc@google.com>

I appreciate the nod, but you found this, not me :-)

> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b6f4411b613e..76cca5d5aa6b 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6854,11 +6854,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
>  	msrs->procbased_ctls_low &=
>  		~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING);
>  
> -	/*
> -	 * secondary cpu-based controls.  Do not include those that
> -	 * depend on CPUID bits, they are added later by
> -	 * vmx_vcpu_after_set_cpuid.
> -	 */
> +	/* secondary cpu-based controls */

Eh, just drop the comment.  Pretty obvious this is for secondary execution controls.

>  	msrs->secondary_ctls_low = 0;
>  
>  	msrs->secondary_ctls_high = vmcs_conf->cpu_based_2nd_exec_ctrl;
> -- 
> 2.17.1
> 

Re: [PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[Yu Zhang]

> 
> Eh, just drop the comment.  Pretty obvious this is for secondary execution controls.
Thanks Sean. Well, I agree it is obvious.

This line was kept because there are comments for other groups of
control fields(e.g., exit/entry/pin-based/cpu-based controls etc.)
in nested_vmx_setup_ctls_msrs(). If we do not keep the one for secondary
cpu-based controls, we may just delete other comments as well. But
is that really necessary? 

B.R.
Yu

Re: [PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[Sean Christopherson]

On Fri, Dec 16, 2022, Yu Zhang wrote:
> > 
> > Eh, just drop the comment.  Pretty obvious this is for secondary execution controls.
> Thanks Sean. Well, I agree it is obvious.
> 
> This line was kept because there are comments for other groups of
> control fields(e.g., exit/entry/pin-based/cpu-based controls etc.)
> in nested_vmx_setup_ctls_msrs(). If we do not keep the one for secondary
> cpu-based controls, we may just delete other comments as well. But
> is that really necessary? 

Adding a patch to delete the various one-line comments is probably unnecessary
churn.  The comments are kinda sorta helpful, but only because the function is a
giant and thus a bit hard to follow.  A better solution than comments would be to
add helpers for each collection ("secondary_ctls" is a bit of a lie because it
handle VPID, EPT, VMFUNC, etc..., but whatever), e.g.

	nested_vmx_setup_pinbased_ctls(msrs);
	nested_vmx_setup_exit_ctls(msrs);
	nested_vmx_setup_entry_ctls(msrs);
	nested_vmx_setup_cpubased_ctls(msrs);
	nested_vmx_setup_secondary_ctls(msrs);
	nested_vmx_setup_misc_data(msrs);

Re: [PATCH] KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs().

[Yu Zhang]

On Fri, Dec 16, 2022 at 04:49:55PM +0000, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Yu Zhang wrote:
> > > 
> > > Eh, just drop the comment.  Pretty obvious this is for secondary execution controls.
> > Thanks Sean. Well, I agree it is obvious.
> > 
> > This line was kept because there are comments for other groups of
> > control fields(e.g., exit/entry/pin-based/cpu-based controls etc.)
> > in nested_vmx_setup_ctls_msrs(). If we do not keep the one for secondary
> > cpu-based controls, we may just delete other comments as well. But
> > is that really necessary? 
> 
> Adding a patch to delete the various one-line comments is probably unnecessary
> churn.  The comments are kinda sorta helpful, but only because the function is a
> giant and thus a bit hard to follow.  A better solution than comments would be to
> add helpers for each collection ("secondary_ctls" is a bit of a lie because it
> handle VPID, EPT, VMFUNC, etc..., but whatever), e.g.

Good point. The "secondary_ctls" may be inaccurate, but I do not
have a better name in mind either...

> 
> 	nested_vmx_setup_pinbased_ctls(msrs);
> 	nested_vmx_setup_exit_ctls(msrs);
> 	nested_vmx_setup_entry_ctls(msrs);
> 	nested_vmx_setup_cpubased_ctls(msrs);
> 	nested_vmx_setup_secondary_ctls(msrs);
Adding nested_vmx_setup_secondary_ctls() will impact
1> your previous patch to expose ENABLE_USR_WAIT_PAUSE control
https://lore.kernel.org/lkml/20221213062306.667649-2-seanjc@google.com/
2> my previous patch to simplify the setting of secondary proc-
based control:
https://www.spinics.net/lists/kernel/msg4582141.html
How about we combine our previous patches and the new ones together
in next version?

One more questionable comment for nested_vmx_setup_ctls_msrs() is:

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b6f4411b613e..58b491f13ed7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6744,8 +6744,6 @@ static u64 nested_vmx_calc_vmcs_enum_msr(void)
 /*
  * 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


> 	nested_vmx_setup_misc_data(msrs);
As to the misc data msr, do we really need a seperate function for it?
If yes, then what about the vmx basic msr, the ones for fixed bits in
CR0/4? 

B.R.
Yu