| Message ID | 20230518091339.1102-3-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Fix some comments | expand |
On Thu, May 18, 2023, Binbin Wu wrote: > msrs_to_save_all is out-dated after commit 2374b7310b66 > (KVM: x86/pmu: Use separate array for defining "PMU MSRs to save"). > > Update the comments to msrs_to_save_base. > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index ceb7c5e9cf9e..ca7cff5252ae 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1432,7 +1432,7 @@ EXPORT_SYMBOL_GPL(kvm_emulate_rdpmc); > * > * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) > * extract the supported MSRs from the related const lists. > - * msrs_to_save is selected from the msrs_to_save_all to reflect the > + * msrs_to_save is selected from the msrs_to_save_base to reflect the A straight conversion isn't correct, msrs_to_save isn't selected from *just* msrs_to_save_base. > * capabilities of the host cpu. This capabilities test skips MSRs that are > * kvm-specific. Those are put in emulated_msrs_all; filtering of emulated_msrs This "kvm-specific" blurb is also stale. > * may depend on host virtualization features rather than host cpu features. > @@ -1535,7 +1535,7 @@ static const u32 emulated_msrs_all[] = { > * by arch/x86/kvm/vmx/nested.c based on CPUID or other MSRs. > * We always support the "true" VMX control MSRs, even if the host > * processor does not, so I am putting these registers here rather > - * than in msrs_to_save_all. > + * than in msrs_to_save_base. And this entire comment is rather weird, e.g. I have no idea what MSRs the part about CPUID and other MSRs is referring to. Rather than do a blind replacement, how about this? -- From: Sean Christopherson <seanjc@google.com> Date: Mon, 5 Jun 2023 12:56:46 -0700 Subject: [PATCH] KVM: x86: Update comments about MSR lists exposed to userspace Refresh comments about msrs_to_save, emulated_msrs, and msr_based_features to remove stale references left behind by commit 2374b7310b66 (KVM: x86/pmu: Use separate array for defining "PMU MSRs to save"), and to better reflect the current reality, e.g. emulated_msrs is no longer just for MSRs that are "kvm-specific". Reported-by: Binbin Wu <binbin.wu@linux.intel.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 5ad55ef71433..c77f72cf6dc8 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1427,15 +1427,14 @@ int kvm_emulate_rdpmc(struct kvm_vcpu *vcpu) EXPORT_SYMBOL_GPL(kvm_emulate_rdpmc); /* - * List of msr numbers which we expose to userspace through KVM_GET_MSRS - * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST. - * - * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) - * extract the supported MSRs from the related const lists. - * msrs_to_save is selected from the msrs_to_save_all to reflect the - * capabilities of the host cpu. This capabilities test skips MSRs that are - * kvm-specific. Those are put in emulated_msrs_all; filtering of emulated_msrs - * may depend on host virtualization features rather than host cpu features. + * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) track + * the set of MSRs that KVM exposes to userspace through KVM_GET_MSRS, + * KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST. msrs_to_save holds MSRs that + * require host support, i.e. should be probed via RDMSR. emulated_msrs holds + * MSRs that emulates without strictly requiring host support. + * msr_based_features holds MSRs that enumerate features, i.e. are effectively + * CPUID leafs. Note, msr_based_features isn't mutually exclusive with + * msrs_to_save and emulated_msrs. */ static const u32 msrs_to_save_base[] = { @@ -1531,11 +1530,11 @@ static const u32 emulated_msrs_all[] = { MSR_IA32_UCODE_REV, /* - * The following list leaves out MSRs whose values are determined - * by arch/x86/kvm/vmx/nested.c based on CPUID or other MSRs. - * We always support the "true" VMX control MSRs, even if the host - * processor does not, so I am putting these registers here rather - * than in msrs_to_save_all. + * KVM always supports the "true" VMX control MSRs, even if the host + * does not. The VMX MSRs as a whole are considered "emulated" as KVM + * doesn't strictly require them to exist in the host (ignoring that + * KVM would refuse to load in the first place if the core set of MSRs + * aren't supported). */ MSR_IA32_VMX_BASIC, MSR_IA32_VMX_TRUE_PINBASED_CTLS, base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7 --
On 6/6/2023 4:00 AM, Sean Christopherson wrote: > On Thu, May 18, 2023, Binbin Wu wrote: >> msrs_to_save_all is out-dated after commit 2374b7310b66 >> (KVM: x86/pmu: Use separate array for defining "PMU MSRs to save"). >> >> Update the comments to msrs_to_save_base. >> >> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> arch/x86/kvm/x86.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index ceb7c5e9cf9e..ca7cff5252ae 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1432,7 +1432,7 @@ EXPORT_SYMBOL_GPL(kvm_emulate_rdpmc); >> * >> * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) >> * extract the supported MSRs from the related const lists. >> - * msrs_to_save is selected from the msrs_to_save_all to reflect the >> + * msrs_to_save is selected from the msrs_to_save_base to reflect the > A straight conversion isn't correct, msrs_to_save isn't selected from *just* > msrs_to_save_base. > >> * capabilities of the host cpu. This capabilities test skips MSRs that are >> * kvm-specific. Those are put in emulated_msrs_all; filtering of emulated_msrs > This "kvm-specific" blurb is also stale. > >> * may depend on host virtualization features rather than host cpu features. >> @@ -1535,7 +1535,7 @@ static const u32 emulated_msrs_all[] = { >> * by arch/x86/kvm/vmx/nested.c based on CPUID or other MSRs. >> * We always support the "true" VMX control MSRs, even if the host >> * processor does not, so I am putting these registers here rather >> - * than in msrs_to_save_all. >> + * than in msrs_to_save_base. > And this entire comment is rather weird, e.g. I have no idea what MSRs the part > about CPUID and other MSRs is referring to. > > Rather than do a blind replacement, how about this? LGTM. Thanks. > > -- > From: Sean Christopherson <seanjc@google.com> > Date: Mon, 5 Jun 2023 12:56:46 -0700 > Subject: [PATCH] KVM: x86: Update comments about MSR lists exposed to > userspace > > Refresh comments about msrs_to_save, emulated_msrs, and msr_based_features > to remove stale references left behind by commit 2374b7310b66 (KVM: > x86/pmu: Use separate array for defining "PMU MSRs to save"), and to > better reflect the current reality, e.g. emulated_msrs is no longer just > for MSRs that are "kvm-specific". > > Reported-by: Binbin Wu <binbin.wu@linux.intel.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 5ad55ef71433..c77f72cf6dc8 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1427,15 +1427,14 @@ int kvm_emulate_rdpmc(struct kvm_vcpu *vcpu) > EXPORT_SYMBOL_GPL(kvm_emulate_rdpmc); > > /* > - * List of msr numbers which we expose to userspace through KVM_GET_MSRS > - * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST. > - * > - * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) > - * extract the supported MSRs from the related const lists. > - * msrs_to_save is selected from the msrs_to_save_all to reflect the > - * capabilities of the host cpu. This capabilities test skips MSRs that are > - * kvm-specific. Those are put in emulated_msrs_all; filtering of emulated_msrs > - * may depend on host virtualization features rather than host cpu features. > + * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) track > + * the set of MSRs that KVM exposes to userspace through KVM_GET_MSRS, > + * KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST. msrs_to_save holds MSRs that > + * require host support, i.e. should be probed via RDMSR. emulated_msrs holds > + * MSRs that emulates without strictly requiring host support. > + * msr_based_features holds MSRs that enumerate features, i.e. are effectively > + * CPUID leafs. Note, msr_based_features isn't mutually exclusive with > + * msrs_to_save and emulated_msrs. > */ > > static const u32 msrs_to_save_base[] = { > @@ -1531,11 +1530,11 @@ static const u32 emulated_msrs_all[] = { > MSR_IA32_UCODE_REV, > > /* > - * The following list leaves out MSRs whose values are determined > - * by arch/x86/kvm/vmx/nested.c based on CPUID or other MSRs. > - * We always support the "true" VMX control MSRs, even if the host > - * processor does not, so I am putting these registers here rather > - * than in msrs_to_save_all. > + * KVM always supports the "true" VMX control MSRs, even if the host > + * does not. The VMX MSRs as a whole are considered "emulated" as KVM > + * doesn't strictly require them to exist in the host (ignoring that > + * KVM would refuse to load in the first place if the core set of MSRs > + * aren't supported). > */ > MSR_IA32_VMX_BASIC, > MSR_IA32_VMX_TRUE_PINBASED_CTLS, > > base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index ceb7c5e9cf9e..ca7cff5252ae 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1432,7 +1432,7 @@ EXPORT_SYMBOL_GPL(kvm_emulate_rdpmc); * * The three MSR lists(msrs_to_save, emulated_msrs, msr_based_features) * extract the supported MSRs from the related const lists. - * msrs_to_save is selected from the msrs_to_save_all to reflect the + * msrs_to_save is selected from the msrs_to_save_base to reflect the * capabilities of the host cpu. This capabilities test skips MSRs that are * kvm-specific. Those are put in emulated_msrs_all; filtering of emulated_msrs * may depend on host virtualization features rather than host cpu features. @@ -1535,7 +1535,7 @@ static const u32 emulated_msrs_all[] = { * by arch/x86/kvm/vmx/nested.c based on CPUID or other MSRs. * We always support the "true" VMX control MSRs, even if the host * processor does not, so I am putting these registers here rather - * than in msrs_to_save_all. + * than in msrs_to_save_base. */ MSR_IA32_VMX_BASIC, MSR_IA32_VMX_TRUE_PINBASED_CTLS,
msrs_to_save_all is out-dated after commit 2374b7310b66 (KVM: x86/pmu: Use separate array for defining "PMU MSRs to save"). Update the comments to msrs_to_save_base. Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> --- arch/x86/kvm/x86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)