| Message ID | 20221215100558.1202615-1-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Remove outdated comments in nested_vmx_setup_ctls_msrs(). | expand |
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 >
> > 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
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);
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
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;
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(-)