[PATCH v2 0/3] KVM: Fix some comments

[PATCH v2 0/3] KVM: Fix some comments

[Binbin Wu]

Fix comments for KVM_ENABLE_CAP.
Update msrs_to_save_all to msrs_to_save_base in comments.
Fix a typo in x86/mmu.rst

---
Changelog:
v1 --> v2: 
Add changelog in patch 1 to explain the modification, and drop the tools/ change.
Split the change of x86/mmu.rst to a separate patch.

Binbin Wu (3):
  KVM: Fix comment for KVM_ENABLE_CAP
  KVM: x86: Fix comments that refer to the out-dated msrs_to_save_all
  KVM: Documentation: Fix a typo in Documentation/virt/kvm/x86/mmu.rst

 Documentation/virt/kvm/x86/mmu.rst | 2 +-
 arch/x86/kvm/x86.c                 | 4 ++--
 include/uapi/linux/kvm.h           | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)


base-commit: f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6
-- 
2.25.1

[PATCH v2 1/3] KVM: Fix comment for KVM_ENABLE_CAP

[Binbin Wu]

Fix comment for vcpu ioctl version of KVM_ENABLE_CAP.

KVM provides ioctl KVM_ENABLE_CAP to allow userspace to enable an
extension which is not enabled by default. For vcpu ioctl version,
it is available with the capability KVM_CAP_ENABLE_CAP. For vm ioctl
version, it is available with the capability KVM_CAP_ENABLE_CAP_VM.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 include/uapi/linux/kvm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 737318b1c1d9..bddf2871db8f 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1613,7 +1613,7 @@ struct kvm_s390_ucas_mapping {
 #define KVM_GET_DEBUGREGS         _IOR(KVMIO,  0xa1, struct kvm_debugregs)
 #define KVM_SET_DEBUGREGS         _IOW(KVMIO,  0xa2, struct kvm_debugregs)
 /*
- * vcpu version available with KVM_ENABLE_CAP
+ * vcpu version available with KVM_CAP_ENABLE_CAP
  * vm version available with KVM_CAP_ENABLE_CAP_VM
  */
 #define KVM_ENABLE_CAP            _IOW(KVMIO,  0xa3, struct kvm_enable_cap)
-- 
2.25.1

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

[Binbin Wu]

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
  * 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,
-- 
2.25.1

[PATCH v2 3/3] KVM: Documentation: Fix a typo in Documentation/virt/kvm/x86/mmu.rst

[Binbin Wu]

L1 CR4.LA57 should be '0' instead of '1' when shadowing 5-level NPT
for 4-level NPT L1 guest.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 Documentation/virt/kvm/x86/mmu.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/x86/mmu.rst b/Documentation/virt/kvm/x86/mmu.rst
index 8364afa228ec..26f62034b6f3 100644
--- a/Documentation/virt/kvm/x86/mmu.rst
+++ b/Documentation/virt/kvm/x86/mmu.rst
@@ -205,7 +205,7 @@ Shadow pages contain the following information:
   role.passthrough:
     The page is not backed by a guest page table, but its first entry
     points to one.  This is set if NPT uses 5-level page tables (host
-    CR4.LA57=1) and is shadowing L1's 4-level NPT (L1 CR4.LA57=1).
+    CR4.LA57=1) and is shadowing L1's 4-level NPT (L1 CR4.LA57=0).
   gfn:
     Either the guest page table containing the translations shadowed by this
     page, or the base page frame for linear translations.  See role.direct.
-- 
2.25.1

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

[Sean Christopherson]

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

Re: [PATCH v2 0/3] KVM: Fix some comments

[Sean Christopherson]

On Thu, 18 May 2023 17:13:36 +0800, Binbin Wu wrote:
> Fix comments for KVM_ENABLE_CAP.
> Update msrs_to_save_all to msrs_to_save_base in comments.
> Fix a typo in x86/mmu.rst
> 

Applied [1/3] to kvm-x86 generic, and [3/3] to kvm-x86 misc.  I'll post a
separate patch for [2/3], thanks!

[1/3] KVM: Fix comment for KVM_ENABLE_CAP
      https://github.com/kvm-x86/linux/commit/22725266bdf9

[3/3] KVM: Documentation: Fix a typo in Documentation/virt/kvm/x86/mmu.rst
      https://github.com/kvm-x86/linux/commit/06b66e050095

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

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

[Binbin Wu]

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

Re: [PATCH v2 1/3] KVM: Fix comment for KVM_ENABLE_CAP

[Binbin Wu]

On 6/6/2023 10:23 PM, Xu Yilun wrote:
> On 2023-05-18 at 17:13:37 +0800, Binbin Wu wrote:
>> Fix comment for vcpu ioctl version of KVM_ENABLE_CAP.
>>
>> KVM provides ioctl KVM_ENABLE_CAP to allow userspace to enable an
>> extension which is not enabled by default. For vcpu ioctl version,
>> it is available with the capability KVM_CAP_ENABLE_CAP. For vm ioctl
>> version, it is available with the capability KVM_CAP_ENABLE_CAP_VM.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>>   include/uapi/linux/kvm.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 737318b1c1d9..bddf2871db8f 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
> tools/include/uapi/linux/kvm.h also needs the change?
Sean suggested "never update KVM's uapi headers in tools/ in KVM's tree"
So I dropped the change to tools/include/uapi/linux/kvm.h

You can refer to the following links for detials:
https://lore.kernel.org/kvm/ZGVGkpvWQqLX2BrV@google.com/
https://lore.kernel.org/all/Y8bZ%2FJ98V5i3wG%2Fv@google.com/

>
> Thanks,
> Yilun
>
>> @@ -1613,7 +1613,7 @@ struct kvm_s390_ucas_mapping {
>>   #define KVM_GET_DEBUGREGS         _IOR(KVMIO,  0xa1, struct kvm_debugregs)
>>   #define KVM_SET_DEBUGREGS         _IOW(KVMIO,  0xa2, struct kvm_debugregs)
>>   /*
>> - * vcpu version available with KVM_ENABLE_CAP
>> + * vcpu version available with KVM_CAP_ENABLE_CAP
>>    * vm version available with KVM_CAP_ENABLE_CAP_VM
>>    */
>>   #define KVM_ENABLE_CAP            _IOW(KVMIO,  0xa3, struct kvm_enable_cap)
>> -- 
>> 2.25.1
>>

Re: [PATCH v2 1/3] KVM: Fix comment for KVM_ENABLE_CAP

[Xu Yilun]

On 2023-05-18 at 17:13:37 +0800, Binbin Wu wrote:
> Fix comment for vcpu ioctl version of KVM_ENABLE_CAP.
> 
> KVM provides ioctl KVM_ENABLE_CAP to allow userspace to enable an
> extension which is not enabled by default. For vcpu ioctl version,
> it is available with the capability KVM_CAP_ENABLE_CAP. For vm ioctl
> version, it is available with the capability KVM_CAP_ENABLE_CAP_VM.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
>  include/uapi/linux/kvm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 737318b1c1d9..bddf2871db8f 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h

tools/include/uapi/linux/kvm.h also needs the change?

Thanks,
Yilun

> @@ -1613,7 +1613,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_GET_DEBUGREGS         _IOR(KVMIO,  0xa1, struct kvm_debugregs)
>  #define KVM_SET_DEBUGREGS         _IOW(KVMIO,  0xa2, struct kvm_debugregs)
>  /*
> - * vcpu version available with KVM_ENABLE_CAP
> + * vcpu version available with KVM_CAP_ENABLE_CAP
>   * vm version available with KVM_CAP_ENABLE_CAP_VM
>   */
>  #define KVM_ENABLE_CAP            _IOW(KVMIO,  0xa3, struct kvm_enable_cap)
> -- 
> 2.25.1
>