Re: [PATCH v2 2/3] KVM: x86: Fix comments that refer to the out-dated msrs_to_save_all

[Sean Christopherson <seanjc@google.com>]

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
--