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

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

[EwanHai]

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++) {
+        if (kvm_feature_msrs->indices[i] == MSR_IA32_VMX_PROCBASED_CTLS2) {
+            has_msr_vmx_procbased_ctls2 = true;
+        }
+    }
     return 0;
 }
 
-- 
2.34.1

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

[Ewan Hai]

Sorry for my oversight, I am adding the maintainers who were
missed in the previous email.

On 6/24/24 05:58, EwanHai wrote:
> 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++) {
> +        if (kvm_feature_msrs->indices[i] == MSR_IA32_VMX_PROCBASED_CTLS2) {
> +            has_msr_vmx_procbased_ctls2 = true;
> +        }
> +    }
>       return 0;
>   }
>   

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

[Zhao Liu]

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

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

[Ewan Hai]

On 6/25/24 05:49, Zhao Liu wrote:
>> 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++) {
do I need to send a v4 version patch,to do this fix?
>> +        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
Thanks for your reply. In fact, I've tried to process has_msr_vmx_vmfunc 
in the same
way as has_msr_vmx_procbased_ctls in this patch, but when I tested on 
Linux kernel
4.19.67, I encountered an "error: failed to set MSR 0x491 to 0x***".

This issue is due to Linux kernel commit 27c42a1bb ("KVM: nVMX: Enable 
VMFUNC
for the L1 hypervisor", 2017-08-03) exposing VMFUNC to the QEMU guest 
without
corresponding VMFUNC MSR modification code, leading to an error when 
QEMU attempts
to set the VMFUNC MSR. This bug affects kernels from 4.14 to 5.2, with a 
fix introduced
in 5.3 by Paolo (e8a70bd4e "KVM: nVMX: allow setting the VMFUNC controls 
MSR", 2019-07-02).

So the fix for has_msr_vmx_vmfunc is clearly different from 
has_msr_vmx_procbased_ctls2.
However, due to the different kernel support situations, I have not yet 
come up with a suitable
way to handle the compatibility of has_msr_vmx_procbased_ctls2 across 
different kernel versions.

Therefore, should we consider only fixing has_msr_vmx_procbased_ctls2 
this time and addressing
has_msr_vmx_vmfunc in a future patch when the timing is more appropriate?

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

[Zhao Liu]

[snip]

> > Additionally, has_msr_vmx_vmfunc has the similar compat issue. I think
> > it deserves a fix, too.
> > 
> > -Zhao
> Thanks for your reply. In fact, I've tried to process has_msr_vmx_vmfunc in
> the same
> way as has_msr_vmx_procbased_ctls in this patch, but when I tested on Linux
> kernel
> 4.19.67, I encountered an "error: failed to set MSR 0x491 to 0x***".
> 
> This issue is due to Linux kernel commit 27c42a1bb ("KVM: nVMX: Enable
> VMFUNC
> for the L1 hypervisor", 2017-08-03) exposing VMFUNC to the QEMU guest
> without
> corresponding VMFUNC MSR modification code, leading to an error when QEMU
> attempts
> to set the VMFUNC MSR. This bug affects kernels from 4.14 to 5.2, with a fix
> introduced
> in 5.3 by Paolo (e8a70bd4e "KVM: nVMX: allow setting the VMFUNC controls
> MSR", 2019-07-02).

It looks like this fix was not ported to the 4.19 stable kernel.

> So the fix for has_msr_vmx_vmfunc is clearly different from
> has_msr_vmx_procbased_ctls2.
> However, due to the different kernel support situations, I have not yet come
> up with a suitable
> way to handle the compatibility of has_msr_vmx_procbased_ctls2 across
> different kernel versions.
> 
> Therefore, should we consider only fixing has_msr_vmx_procbased_ctls2 this
> time and addressing
> has_msr_vmx_vmfunc in a future patch when the timing is more appropriate?
> 

I agree this fix should focus on MSR_IA32_VMX_PROCBASED_CTLS2.

But I think at least we need a comment (maybe a TODO) to note the case of
has_msr_vmx_vmfunc in a followup patch.

Let's wait and see what Paolo will say.

-Zhao

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

[Ewan Hai]

Dear Maintainers and Paolo,

I hope this message finds you well. I am writing to inquire about the 
status of
the patch I submitted a month ago. Could you please provide any updates or
addtional comments regarding its review?

Thank you for your time and assistance.

Best regards,
Ewan

On 6/25/24 10:08, Zhao Liu wrote:
>>> Additionally, has_msr_vmx_vmfunc has the similar compat issue. I think
>>> it deserves a fix, too.
>>>
>>> -Zhao
>> Thanks for your reply. In fact, I've tried to process has_msr_vmx_vmfunc in
>> the same
>> way as has_msr_vmx_procbased_ctls in this patch, but when I tested on Linux
>> kernel
>> 4.19.67, I encountered an "error: failed to set MSR 0x491 to 0x***".
>>
>> This issue is due to Linux kernel commit 27c42a1bb ("KVM: nVMX: Enable
>> VMFUNC
>> for the L1 hypervisor", 2017-08-03) exposing VMFUNC to the QEMU guest
>> without
>> corresponding VMFUNC MSR modification code, leading to an error when QEMU
>> attempts
>> to set the VMFUNC MSR. This bug affects kernels from 4.14 to 5.2, with a fix
>> introduced
>> in 5.3 by Paolo (e8a70bd4e "KVM: nVMX: allow setting the VMFUNC controls
>> MSR", 2019-07-02).
> It looks like this fix was not ported to the 4.19 stable kernel.
>
>> So the fix for has_msr_vmx_vmfunc is clearly different from
>> has_msr_vmx_procbased_ctls2.
>> However, due to the different kernel support situations, I have not yet come
>> up with a suitable
>> way to handle the compatibility of has_msr_vmx_procbased_ctls2 across
>> different kernel versions.
>>
>> Therefore, should we consider only fixing has_msr_vmx_procbased_ctls2 this
>> time and addressing
>> has_msr_vmx_vmfunc in a future patch when the timing is more appropriate?
>>
> I agree this fix should focus on MSR_IA32_VMX_PROCBASED_CTLS2.
>
> But I think at least we need a comment (maybe a TODO) to note the case of
> has_msr_vmx_vmfunc in a followup patch.
>
> Let's wait and see what Paolo will say.
>
> -Zhao

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

[EwanHai]

Dear Maintainers and Paolo,

I hope this email finds you well. This is my second follow-up regarding the
patch I submitted for review. I previously sent a reminder on July 23rd, but
I have yet to receive any updates or further comments.

I understand that you have many responsibilities, but I would greatly
appreciate any feedback or status updates on this patch. Your guidance is
essential for moving this forward.

Thank you once again for your time and attention to this matter.

Best regards,
Ewan