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

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

Posted by EwanHai 7 months, 1 week ago

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(e.g., linux-4.19.90) 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 maintains 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>
---
 target/i386/kvm/kvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index af101fcdf6..6299284de4 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2343,6 +2343,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;
@@ -2377,6 +2378,11 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
         return ret;
     }
 
+    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] target/i386/kvm: Refine VMX controls setting for backward compatibility

Posted by Zhao Liu 6 months, 1 week ago

On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
> Date: Mon, 25 Sep 2023 03:14:53 -0400
> From: EwanHai <ewanhai-oc@zhaoxin.com>
> Subject: [PATCH] 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

s/didn’t/didn't/

> kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is

For this old kernel, it's better to add the brief lifecycle note (e.g.,
lts, EOL) to illustrate the value of considering such compatibility
fixes.

> in `kvm_feature_msrs`—obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),

s/—obtained/-obtained/

> but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).

s/—obtained/-obtained/

> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
> on `kvm_msr_list` alone, even though KVM maintains 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>
> ---
>  target/i386/kvm/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index af101fcdf6..6299284de4 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2343,6 +2343,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;
> @@ -2377,6 +2378,11 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>          return ret;
>      }

It's worth adding a comment here to indicate that this is a
compatibility fix.

-Zhao

>  
> +    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] target/i386/kvm: Refine VMX controls setting for backward compatibility

Posted by Ewan Hai 6 months, 1 week ago

On 10/25/23 23:20, Zhao Liu wrote:
> On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
>> Date: Mon, 25 Sep 2023 03:14:53 -0400
>> From: EwanHai<ewanhai-oc@zhaoxin.com>
>> Subject: [PATCH] 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
> s/didn’t/didn't/

I'll fix it.

>
>> kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is
> For this old kernel, it's better to add the brief lifecycle note (e.g.,
> lts, EOL) to illustrate the value of considering such compatibility
> fixes.

I've checked the linux-stable repo, found that
MSR_IA32_VMX_PROCBASED_CTLS2 is not included in kvm regular msr list
until linux-5.3, and in linux-4.19.x(EOL:Dec,2024), there is also no
MSR_IA32_VMX_PROCBASED_CTLS2 in kvm regular msr list.

So maybe this is an important compatibility fix for kernel < 5.3.

>> in `kvm_feature_msrs`—obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> s/—obtained/-obtained/

I'll fix it.

>
>> but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> s/—obtained/-obtained/

I'll fix it.

>
>> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
>> on `kvm_msr_list` alone, even though KVM maintains 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>
>> ---
>>   target/i386/kvm/kvm.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index af101fcdf6..6299284de4 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2343,6 +2343,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;
>> @@ -2377,6 +2378,11 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>>           return ret;
>>       }
> It's worth adding a comment here to indicate that this is a
> compatibility fix.
>
> -Zhao
>
>>   
>> +    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
>>
Plan to use patch bellow, any more suggestion?

>  From a3006fcec3615d98ac1eb252a61952d44aa5029b Mon Sep 17 00:00:00 2001
> From: EwanHai<ewanhai-oc@zhaoxin.com>
> Date: Mon, 25 Sep 2023 02:11:59 -0400
> Subject: [PATCH] target/i386/kvm: Refine VMX controls setting for backward
>   compatibility
>
> 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(<5.3) 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 maintains 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>
> ---
>   target/i386/kvm/kvm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index af101fcdf6..3cf95f8579 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2343,6 +2343,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;
> @@ -2377,6 +2378,19 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>           return ret;
>       }
>
> +    /*
> +     * Compatibility fix:
> +     * Older Linux kernels(<5.3) include the MSR_IA32_VMX_PROCBASED_CTLS2
> +     * only in feature msr list, but not in regular msr list. This lead to
> +     * an issue in older kernel versions where QEMU, through the regular
> +     * MSR list check, assumes the kernel doesn't maintain this msr,
> +     * 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] target/i386/kvm: Refine VMX controls setting for backward compatibility

Posted by Ewan Hai 6 months, 1 week ago

Hi Zhao,

since I found last email contains non-plain-text content, andkvm@vger.kernel.org
rejected to receive my mail, so just re-send last mail here, to follow the rule of qemu
/kvm community.

On 10/25/23 23:20, Zhao Liu wrote:
> On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
>> Date: Mon, 25 Sep 2023 03:14:53 -0400
>> From: EwanHai<ewanhai-oc@zhaoxin.com>
>> Subject: [PATCH] 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
> s/didn’t/didn't/

I'll fix it.

>> kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is
> For this old kernel, it's better to add the brief lifecycle note (e.g.,
> lts, EOL) to illustrate the value of considering such compatibility
> fixes.

I've checked the linux-stable repo, found that
MSR_IA32_VMX_PROCBASED_CTLS2 is not included in kvm regular msr list
until linux-5.3, and in linux-4.19.x(EOL:Dec,2024), there is also no
MSR_IA32_VMX_PROCBASED_CTLS2 in kvm regular msr list.

So maybe this is an important compatibility fix for kernel < 5.3.

>> in `kvm_feature_msrs`—obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> s/—obtained/-obtained/

I'll fix it.

>> but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> s/—obtained/-obtained/

I'll fix it.

>> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
>> on `kvm_msr_list` alone, even though KVM maintains 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>
>> ---
>>   target/i386/kvm/kvm.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index af101fcdf6..6299284de4 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2343,6 +2343,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;
>> @@ -2377,6 +2378,11 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>>           return ret;
>>       }
> It's worth adding a comment here to indicate that this is a
> compatibility fix.
>
> -Zhao
>
>>   
>> +    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
>>
Plan to use patch bellow, any more suggestion?

>  From a3006fcec3615d98ac1eb252a61952d44aa5029b Mon Sep 17 00:00:00 2001
> From: EwanHai<ewanhai-oc@zhaoxin.com>
> Date: Mon, 25 Sep 2023 02:11:59 -0400
> Subject: [PATCH] target/i386/kvm: Refine VMX controls setting for backward
>   compatibility
>
> 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(<5.3) 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 maintains 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>
> ---
>   target/i386/kvm/kvm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index af101fcdf6..3cf95f8579 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2343,6 +2343,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;
> @@ -2377,6 +2378,19 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
>           return ret;
>       }
>
> +    /*
> +     * Compatibility fix:
> +     * Older Linux kernels(<5.3) include the MSR_IA32_VMX_PROCBASED_CTLS2
> +     * only in feature msr list, but not in regular msr list. This lead to
> +     * an issue in older kernel versions where QEMU, through the regular
> +     * MSR list check, assumes the kernel doesn't maintain this msr,
> +     * 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

Best regards.

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

Posted by Zhao Liu 5 months, 1 week ago

Hi Ewan,

Look good to me. No other comments.

Regards,
Zhao

On Fri, Oct 27, 2023 at 02:08:57AM -0400, Ewan Hai wrote:
> Date: Fri, 27 Oct 2023 02:08:57 -0400
> From: Ewan Hai <ewanhai-oc@zhaoxin.com>
> Subject: Re: [PATCH] target/i386/kvm: Refine VMX controls setting for
>  backward compatibility
> 
> Hi Zhao,
> 
> since I found last email contains non-plain-text content, andkvm@vger.kernel.org
> rejected to receive my mail, so just re-send last mail here, to follow the rule of qemu
> /kvm community.
> 
> On 10/25/23 23:20, Zhao Liu wrote:
> > On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
> > > Date: Mon, 25 Sep 2023 03:14:53 -0400
> > > From: EwanHai<ewanhai-oc@zhaoxin.com>
> > > Subject: [PATCH] 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
> > s/didn’t/didn't/
> 
> I'll fix it.
> 
> > > kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is
> > For this old kernel, it's better to add the brief lifecycle note (e.g.,
> > lts, EOL) to illustrate the value of considering such compatibility
> > fixes.
> 
> I've checked the linux-stable repo, found that
> MSR_IA32_VMX_PROCBASED_CTLS2 is not included in kvm regular msr list
> until linux-5.3, and in linux-4.19.x(EOL:Dec,2024), there is also no
> MSR_IA32_VMX_PROCBASED_CTLS2 in kvm regular msr list.
> 
> So maybe this is an important compatibility fix for kernel < 5.3.
> 
> > > in `kvm_feature_msrs`—obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> > s/—obtained/-obtained/
> 
> I'll fix it.
> 
> > > but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> > s/—obtained/-obtained/
> 
> I'll fix it.
> 
> > > As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
> > > on `kvm_msr_list` alone, even though KVM maintains 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>
> > > ---
> > >   target/i386/kvm/kvm.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > > index af101fcdf6..6299284de4 100644
> > > --- a/target/i386/kvm/kvm.c
> > > +++ b/target/i386/kvm/kvm.c
> > > @@ -2343,6 +2343,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;
> > > @@ -2377,6 +2378,11 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
> > >           return ret;
> > >       }
> > It's worth adding a comment here to indicate that this is a
> > compatibility fix.
> > 
> > -Zhao
> > 
> > > +    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
> > > 
> Plan to use patch bellow, any more suggestion?
> 
> >  From a3006fcec3615d98ac1eb252a61952d44aa5029b Mon Sep 17 00:00:00 2001
> > From: EwanHai<ewanhai-oc@zhaoxin.com>
> > Date: Mon, 25 Sep 2023 02:11:59 -0400
> > Subject: [PATCH] target/i386/kvm: Refine VMX controls setting for backward
> >   compatibility
> > 
> > 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(<5.3) 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 maintains 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>
> > ---
> >   target/i386/kvm/kvm.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index af101fcdf6..3cf95f8579 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -2343,6 +2343,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;
> > @@ -2377,6 +2378,19 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
> >           return ret;
> >       }
> > 
> > +    /*
> > +     * Compatibility fix:
> > +     * Older Linux kernels(<5.3) include the MSR_IA32_VMX_PROCBASED_CTLS2
> > +     * only in feature msr list, but not in regular msr list. This lead to
> > +     * an issue in older kernel versions where QEMU, through the regular
> > +     * MSR list check, assumes the kernel doesn't maintain this msr,
> > +     * 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
> 
> Best regards.
>

PING: VMX controls setting patch for backward compatibility

Posted by Ewan Hai 5 months, 1 week ago

Hi Zhao Liu and QEMU/KVM Community,

I hope this email finds you well. I am writing to follow up on the
conversation we had a month ago regarding my patch submission for
refining the VMX controls setting for backward compatibility on
QEMU-KVM.

On October 27, I responded to the feedback and suggestions provided
by Zhao Liu, making necessary corrections and improvements to the
patch. However, since then, I haven't received any further responses
or reviews.

I understand that everyone is busy, and I appreciate the time and
effort that goes into reviewing these submissions. I just wanted to
check if there are any updates, additional feedback, or steps I should
take next. I am more than willing to make further changes if needed.

Please let me know if there is anything else required from my side for
this patch to move forward. Thank you for your time and attention. I
look forward to hearing from you.

Best regards,
Ewan Hai

On 10/27/23 02:08, Ewan Hai wrote:
> Hi Zhao,
>
> since I found last email contains non-plain-text content, 
> andkvm@vger.kernel.org
> rejected to receive my mail, so just re-send last mail here, to follow 
> the rule of qemu
> /kvm community.
>
> On 10/25/23 23:20, Zhao Liu wrote:
>> On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
>>> Date: Mon, 25 Sep 2023 03:14:53 -0400
>>> From: EwanHai<ewanhai-oc@zhaoxin.com>
>>> Subject: [PATCH] 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
>> s/didn’t/didn't/
>
> I'll fix it.
>
>>> kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is
>> For this old kernel, it's better to add the brief lifecycle note (e.g.,
>> lts, EOL) to illustrate the value of considering such compatibility
>> fixes.
>
> I've checked the linux-stable repo, found that
> MSR_IA32_VMX_PROCBASED_CTLS2 is not included in kvm regular msr list
> until linux-5.3, and in linux-4.19.x(EOL:Dec,2024), there is also no
> MSR_IA32_VMX_PROCBASED_CTLS2 in kvm regular msr list.
>
> So maybe this is an important compatibility fix for kernel < 5.3.
>
>>> in `kvm_feature_msrs`—obtained by 
>>> ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
>> s/—obtained/-obtained/
>
> I'll fix it.
>
>>> but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
>> s/—obtained/-obtained/
>
> I'll fix it.
>
>>> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
>>> on `kvm_msr_list` alone, even though KVM maintains 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>
>>> ---
>>>   target/i386/kvm/kvm.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>>> index af101fcdf6..6299284de4 100644
>>> --- a/target/i386/kvm/kvm.c
>>> +++ b/target/i386/kvm/kvm.c
>>> @@ -2343,6 +2343,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;
>>> @@ -2377,6 +2378,11 @@ static int 
>>> kvm_get_supported_feature_msrs(KVMState *s)
>>>           return ret;
>>>       }
>> It's worth adding a comment here to indicate that this is a
>> compatibility fix.
>>
>> -Zhao
>>
>>>   +    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
>>>
> Plan to use patch bellow, any more suggestion?
>
>>  From a3006fcec3615d98ac1eb252a61952d44aa5029b Mon Sep 17 00:00:00 2001
>> From: EwanHai<ewanhai-oc@zhaoxin.com>
>> Date: Mon, 25 Sep 2023 02:11:59 -0400
>> Subject: [PATCH] target/i386/kvm: Refine VMX controls setting for 
>> backward
>>   compatibility
>>
>> 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(<5.3) 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 maintains 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>
>> ---
>>   target/i386/kvm/kvm.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index af101fcdf6..3cf95f8579 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2343,6 +2343,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;
>> @@ -2377,6 +2378,19 @@ static int 
>> kvm_get_supported_feature_msrs(KVMState *s)
>>           return ret;
>>       }
>>
>> +    /*
>> +     * Compatibility fix:
>> +     * Older Linux kernels(<5.3) include the 
>> MSR_IA32_VMX_PROCBASED_CTLS2
>> +     * only in feature msr list, but not in regular msr list. This 
>> lead to
>> +     * an issue in older kernel versions where QEMU, through the 
>> regular
>> +     * MSR list check, assumes the kernel doesn't maintain this msr,
>> +     * 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
>
> Best regards.
>

Re: PING: VMX controls setting patch for backward compatibility

Posted by Zhao Liu 5 months, 1 week ago

Hi Ewan,

On Thu, Nov 23, 2023 at 10:01:42PM -0500, Ewan Hai wrote:
> Date: Thu, 23 Nov 2023 22:01:42 -0500
> From: Ewan Hai <ewanhai-oc@zhaoxin.com>
> Subject: PING: VMX controls setting patch for backward compatibility
> 
> Hi Zhao Liu and QEMU/KVM Community,
> 
> I hope this email finds you well. I am writing to follow up on the
> conversation we had a month ago regarding my patch submission for
> refining the VMX controls setting for backward compatibility on
> QEMU-KVM.
> 
> On October 27, I responded to the feedback and suggestions provided
> by Zhao Liu, making necessary corrections and improvements to the
> patch. However, since then, I haven't received any further responses
> or reviews.
> 
> I understand that everyone is busy, and I appreciate the time and
> effort that goes into reviewing these submissions. I just wanted to
> check if there are any updates, additional feedback, or steps I should
> take next. I am more than willing to make further changes if needed.
> 
> Please let me know if there is anything else required from my side for
> this patch to move forward. Thank you for your time and attention. I
> look forward to hearing from you.

I think you could refresh a new version and wait for more reviews.

Regards,
Zhao

> 
> Best regards,
> Ewan Hai
> 
> On 10/27/23 02:08, Ewan Hai wrote:
> > Hi Zhao,
> > 
> > since I found last email contains non-plain-text content,
> > andkvm@vger.kernel.org
> > rejected to receive my mail, so just re-send last mail here, to follow
> > the rule of qemu
> > /kvm community.
> > 
> > On 10/25/23 23:20, Zhao Liu wrote:
> > > On Mon, Sep 25, 2023 at 03:14:53AM -0400, EwanHai wrote:
> > > > Date: Mon, 25 Sep 2023 03:14:53 -0400
> > > > From: EwanHai<ewanhai-oc@zhaoxin.com>
> > > > Subject: [PATCH] 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
> > > s/didn’t/didn't/
> > 
> > I'll fix it.
> > 
> > > > kernels(e.g., linux-4.19.90) where `MSR_IA32_VMX_PROCBASED_CTLS2` is
> > > For this old kernel, it's better to add the brief lifecycle note (e.g.,
> > > lts, EOL) to illustrate the value of considering such compatibility
> > > fixes.
> > 
> > I've checked the linux-stable repo, found that
> > MSR_IA32_VMX_PROCBASED_CTLS2 is not included in kvm regular msr list
> > until linux-5.3, and in linux-4.19.x(EOL:Dec,2024), there is also no
> > MSR_IA32_VMX_PROCBASED_CTLS2 in kvm regular msr list.
> > 
> > So maybe this is an important compatibility fix for kernel < 5.3.
> > 
> > > > in `kvm_feature_msrs`—obtained by
> > > > ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> > > s/—obtained/-obtained/
> > 
> > I'll fix it.
> > 
> > > > but not in `kvm_msr_list`—obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> > > s/—obtained/-obtained/
> > 
> > I'll fix it.
> > 
> > > > As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
> > > > on `kvm_msr_list` alone, even though KVM maintains 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>
> > > > ---
> > > >   target/i386/kvm/kvm.c | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > > > index af101fcdf6..6299284de4 100644
> > > > --- a/target/i386/kvm/kvm.c
> > > > +++ b/target/i386/kvm/kvm.c
> > > > @@ -2343,6 +2343,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;
> > > > @@ -2377,6 +2378,11 @@ static int
> > > > kvm_get_supported_feature_msrs(KVMState *s)
> > > >           return ret;
> > > >       }
> > > It's worth adding a comment here to indicate that this is a
> > > compatibility fix.
> > > 
> > > -Zhao
> > > 
> > > >   +    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
> > > > 
> > Plan to use patch bellow, any more suggestion?
> > 
> > >  From a3006fcec3615d98ac1eb252a61952d44aa5029b Mon Sep 17 00:00:00 2001
> > > From: EwanHai<ewanhai-oc@zhaoxin.com>
> > > Date: Mon, 25 Sep 2023 02:11:59 -0400
> > > Subject: [PATCH] target/i386/kvm: Refine VMX controls setting for
> > > backward
> > >   compatibility
> > > 
> > > 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(<5.3) 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 maintains 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>
> > > ---
> > >   target/i386/kvm/kvm.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > > index af101fcdf6..3cf95f8579 100644
> > > --- a/target/i386/kvm/kvm.c
> > > +++ b/target/i386/kvm/kvm.c
> > > @@ -2343,6 +2343,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;
> > > @@ -2377,6 +2378,19 @@ static int
> > > kvm_get_supported_feature_msrs(KVMState *s)
> > >           return ret;
> > >       }
> > > 
> > > +    /*
> > > +     * Compatibility fix:
> > > +     * Older Linux kernels(<5.3) include the
> > > MSR_IA32_VMX_PROCBASED_CTLS2
> > > +     * only in feature msr list, but not in regular msr list. This
> > > lead to
> > > +     * an issue in older kernel versions where QEMU, through the
> > > regular
> > > +     * MSR list check, assumes the kernel doesn't maintain this msr,
> > > +     * 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
> > 
> > Best regards.
> >

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

Posted by EwanHai 6 months, 3 weeks ago

Hello Pbonzini and QEMU community,

I submitted a patch titled "target/i386/kvm: Refine VMX controls setting for 
backward compatibility" on the 25th of September 2023. I noticed that it 
hasn't received any replies yet. Here's a link to the patch on lore.kernel.org: 
https://lore.kernel.org/all/20230925071453.14908-1-ewanhai-oc@zhaoxin.com/.

I've double-checked to ensure I CC'd the relevant maintainers and addressed 
previous review comments, if any. I understand that everyone is busy and some 
patches might get overlooked, especially in less-maintained areas.

I kindly request feedback or a review for my submission. If there are any 
issues or changes needed, please let me know.

Thank you for your time and consideration.

Best regards,
Ewan