Re: [PATCH v3] target/i386/kvm: Refine VMX controls setting for backward compatibility

[Zhao Liu <zhao1.liu@intel.com>]

On Mon, Jun 24, 2024 at 05:58:06AM -0400, EwanHai wrote:
> Date: Mon, 24 Jun 2024 05:58:06 -0400
> From: EwanHai <ewanhai-oc@zhaoxin.com>
> Subject: [PATCH v3] target/i386/kvm: Refine VMX controls setting for
>  backward compatibility
> X-Mailer: git-send-email 2.34.1
> 
> Commit 4a910e1 ("target/i386: do not set unsupported VMX secondary
> execution controls") implemented a workaround for hosts that have
> specific CPUID features but do not support the corresponding VMX
> controls, e.g., hosts support RDSEED but do not support RDSEED-Exiting.
> 
> In detail, commit 4a910e1 introduced a flag `has_msr_vmx_procbased_clts2`.
> If KVM has `MSR_IA32_VMX_PROCBASED_CTLS2` in its msr list, QEMU would
> use KVM's settings, avoiding any modifications to this MSR.
> 
> However, this commit (4a910e1) didn't account for cases in older Linux
> kernels(4.17~5.2) where `MSR_IA32_VMX_PROCBASED_CTLS2` is in
> `kvm_feature_msrs`-obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> but not in `kvm_msr_list`-obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
> on `kvm_msr_list` alone, even though KVM does maintain the value of
> this MSR.
> 
> This patch supplements the above logic, ensuring that
> `has_msr_vmx_procbased_clts2` is correctly set by checking both MSR
> lists, thus maintaining compatibility with older kernels.
> 
> Signed-off-by: EwanHai <ewanhai-oc@zhaoxin.com>
> ---
> Changes in v3:
> - Use a more precise version range in the comment, specifically "4.17~5.2"
> instead of "<5.3".
> 
> Changes in v2:
> - Adjusted some punctuation in the commit message as per suggestions.
> - Added comments to the newly added code to indicate that it is a compatibility fix.
> 
> v1 link:
> https://lore.kernel.org/all/20230925071453.14908-1-ewanhai-oc@zhaoxin.com/
> 
> v2 link:
> https://lore.kernel.org/all/20231127034326.257596-1-ewanhai-oc@zhaoxin.com/
> ---
>  target/i386/kvm/kvm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 7ad8072748..a7c6c5b2d0 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2386,6 +2386,7 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
>  static int kvm_get_supported_feature_msrs(KVMState *s)
>  {
>      int ret = 0;
> +    int i;
>  
>      if (kvm_feature_msrs != NULL) {
>          return 0;
> @@ -2420,6 +2421,20 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>          return ret;
>      }
>  
> +   /*
> +    * Compatibility fix:
> +    * Older Linux kernels (4.17~5.2) report MSR_IA32_VMX_PROCBASED_CTLS2
> +    * in KVM_GET_MSR_FEATURE_INDEX_LIST but not in KVM_GET_MSR_INDEX_LIST.
> +    * This leads to an issue in older kernel versions where QEMU,
> +    * through the KVM_GET_MSR_INDEX_LIST check, assumes the kernel
> +    * doesn't maintain MSR_IA32_VMX_PROCBASED_CTLS2, resulting in
> +    * incorrect settings by QEMU for this MSR.
> +    */
> +    for (i = 0; i < kvm_feature_msrs->nmsrs; i++) {

nit: `i` could be declared here,

for (int i = 0; i < kvm_feature_msrs->nmsrs; i++) {

> +        if (kvm_feature_msrs->indices[i] == MSR_IA32_VMX_PROCBASED_CTLS2) {
> +            has_msr_vmx_procbased_ctls2 = true;
> +        }
> +    }
>      return 0;
>  }
>  
> -- 
> 2.34.1
>

Since the minimum KVM version supported for i386 is v4.5 (docs/system/
target-i386.rst), this fix makes sense, so for this patch,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Additionally, has_msr_vmx_vmfunc has the similar compat issue. I think
it deserves a fix, too.

-Zhao