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

[Ewan Hai <ewanhai-oc@zhaoxin.com>]

[-- Attachment #1: Type: text/plain, Size: 5782 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 7653 bytes --]