* [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
@ 2023-11-27 3:43 EwanHai
2024-02-19 7:54 ` PING: " Ewan Hai
2024-02-20 8:32 ` Xiaoyao Li
0 siblings, 2 replies; 6+ messages in thread
From: EwanHai @ 2023-11-27 3:43 UTC (permalink / raw)
To: pbonzini, mtosatti, kvm, zhao1.liu; +Cc: qemu-devel, cobechen, 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(<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>
---
In response to the suggestions from ZhaoLiu(zhao1.liu@intel.com),
the following changes have been implemented in v2:
- Adjusted some punctuation in the commit message as per the
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/
---
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 11b8177eff..c8f6c0b531 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2296,6 +2296,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;
@@ -2330,6 +2331,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
^ permalink raw reply related [flat|nested] 6+ messages in thread* PING: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
2023-11-27 3:43 [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility EwanHai
@ 2024-02-19 7:54 ` Ewan Hai
2024-02-20 8:32 ` Xiaoyao Li
1 sibling, 0 replies; 6+ messages in thread
From: Ewan Hai @ 2024-02-19 7:54 UTC (permalink / raw)
To: pbonzini, mtosatti, kvm, zhao1.liu; +Cc: qemu-devel, cobechen, ewanhai
Dear QEMU Community,
Two months have passed since my last submission of the patch aimed at
addressing an issue encountered with kernels prior to Linux kernel 5.3.
When using the latest version of QEMU with '-cpu host', if the host
supports the rdseed instruction but not rdseed exiting, it results in
a QEMU error: "error: failed to set MSR 0x48b to 0x*****".
I understand everyone is busy, and reviewing patches can be quite
resource-intensive. Nevertheless, I am eager to hear if there are any
further comments, suggestions, or advice on what steps I should take next
regarding this patch.
Sincerely,Ewan
On 11/26/23 22:43, 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(<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>
> ---
> In response to the suggestions from ZhaoLiu(zhao1.liu@intel.com),
> the following changes have been implemented in v2:
> - Adjusted some punctuation in the commit message as per the
> 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/
> ---
> 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 11b8177eff..c8f6c0b531 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2296,6 +2296,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;
> @@ -2330,6 +2331,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;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
2023-11-27 3:43 [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility EwanHai
2024-02-19 7:54 ` PING: " Ewan Hai
@ 2024-02-20 8:32 ` Xiaoyao Li
2024-02-20 11:07 ` Ewan Hai
1 sibling, 1 reply; 6+ messages in thread
From: Xiaoyao Li @ 2024-02-20 8:32 UTC (permalink / raw)
To: EwanHai, pbonzini, mtosatti, kvm, zhao1.liu; +Cc: qemu-devel, cobechen, ewanhai
On 11/27/2023 11:43 AM, 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(<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>
> ---
> In response to the suggestions from ZhaoLiu(zhao1.liu@intel.com),
> the following changes have been implemented in v2:
> - Adjusted some punctuation in the commit message as per the
> 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/
> ---
> 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 11b8177eff..c8f6c0b531 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2296,6 +2296,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;
> @@ -2330,6 +2331,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
we can be more accurate, that kernel version 4.17 to 5.2, reports
MSR_IA32_VMX_PROCBASED_CTLS2 in KVM_GET_MSR_FEATURE_INDEX_LIST but not
KVM_GET_MSR_INDEX_LIST.
> + * 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;
> + }
> + }
I'm wondering should we move all the initialization of has_msr_*, that
associated with feature MSRs, to here. e.g., has_msr_arch_capabs,
has_msr_vmx_vmfunc,...
> return 0;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
2024-02-20 8:32 ` Xiaoyao Li
@ 2024-02-20 11:07 ` Ewan Hai
2024-02-23 3:13 ` Ewan Hai
0 siblings, 1 reply; 6+ messages in thread
From: Ewan Hai @ 2024-02-20 11:07 UTC (permalink / raw)
To: Xiaoyao Li, pbonzini, mtosatti, kvm, zhao1.liu
Cc: qemu-devel, cobechen, ewanhai
On 2/20/24 03:32, Xiaoyao Li wrote:
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 11b8177eff..c8f6c0b531 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2296,6 +2296,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;
>> @@ -2330,6 +2331,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
>
> we can be more accurate, that kernel version 4.17 to 5.2, reports
> MSR_IA32_VMX_PROCBASED_CTLS2 in KVM_GET_MSR_FEATURE_INDEX_LIST but not
> KVM_GET_MSR_INDEX_LIST.
>
Yeah, I'll add this more precise comment to the next patch.
>> + * 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;
>> + }
>> + }
>
> I'm wondering should we move all the initialization of has_msr_*, that
> associated with feature MSRs, to here. e.g., has_msr_arch_capabs,
> has_msr_vmx_vmfunc,...
>
I believe this is a more elegant way to fix the issue, which will be
reflected in my next patch.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
2024-02-20 11:07 ` Ewan Hai
@ 2024-02-23 3:13 ` Ewan Hai
2024-06-18 9:17 ` Ewan Hai
0 siblings, 1 reply; 6+ messages in thread
From: Ewan Hai @ 2024-02-23 3:13 UTC (permalink / raw)
To: Xiaoyao Li, pbonzini, mtosatti, kvm, zhao1.liu
Cc: qemu-devel, cobechen, ewanhai
On 2/20/24 06:07, Ewan Hai wrote:
> On 2/20/24 03:32, Xiaoyao Li wrote:
>>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>>> index 11b8177eff..c8f6c0b531 100644
>>> --- a/target/i386/kvm/kvm.c
>>> +++ b/target/i386/kvm/kvm.c
>>> @@ -2296,6 +2296,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;
>>> @@ -2330,6 +2331,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
>>
>> we can be more accurate, that kernel version 4.17 to 5.2, reports
>> MSR_IA32_VMX_PROCBASED_CTLS2 in KVM_GET_MSR_FEATURE_INDEX_LIST but not
>> KVM_GET_MSR_INDEX_LIST.
>>
> Yeah, I'll add this more precise comment to the next patch.
>>> + * 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;
>>> + }
>>> + }
>>
>> I'm wondering should we move all the initialization of has_msr_*, that
>> associated with feature MSRs, to here. e.g., has_msr_arch_capabs,
>> has_msr_vmx_vmfunc,...
>>
> I believe this is a more elegant way to fix the issue, which will be
> reflected in my next patch.
When attempting to move the detection logic for feature MSRs (currently
including VMX_VMFUNC, UCODE_REV, ARCH_CAPABILITIES,
PROCBASED_CTLS2) from kvm_get_supported_msrs to
kvm_get_supported_feature_msrs in the current QEMU,
I encountered an "error: failed to set MSR 0x491 to 0x***" on kernel
4.19.67.
This issue is due to 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 proactively tries 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)).
Therefore, even if we were to move all feature MSRs to
kvm_get_supported_feature_msrs,VMX_VMFUNC could not be moved due to
the need to maintain compatibility with different kernel versions. This
exception makes our move less elegant. Hence, I am wondering whether we
need to move all feature MSRs to kvm_get_supported_feature_msrs. Perhaps
we just need to simply move PROCBASED_CTLS2 to fix the "failed set 0x48b
..."
type of bugs, and add a comment about it?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility
2024-02-23 3:13 ` Ewan Hai
@ 2024-06-18 9:17 ` Ewan Hai
0 siblings, 0 replies; 6+ messages in thread
From: Ewan Hai @ 2024-06-18 9:17 UTC (permalink / raw)
To: Xiaoyao Li, pbonzini, mtosatti, kvm, zhao1.liu
Cc: qemu-devel, cobechen, ewanhai
Dear Xiaoyao and Maintainers,
Are there any new comments regarding this patch?
On 2/22/24 22:13, Ewan Hai wrote:
>
>
> On 2/20/24 06:07, Ewan Hai wrote:
>> On 2/20/24 03:32, Xiaoyao Li wrote:
>>>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>>>> index 11b8177eff..c8f6c0b531 100644
>>>> --- a/target/i386/kvm/kvm.c
>>>> +++ b/target/i386/kvm/kvm.c
>>>> @@ -2296,6 +2296,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;
>>>> @@ -2330,6 +2331,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
>>>
>>> we can be more accurate, that kernel version 4.17 to 5.2, reports
>>> MSR_IA32_VMX_PROCBASED_CTLS2 in KVM_GET_MSR_FEATURE_INDEX_LIST but not
>>> KVM_GET_MSR_INDEX_LIST.
>>>
>> Yeah, I'll add this more precise comment to the next patch.
>>>> + * 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;
>>>> + }
>>>> + }
>>>
>>> I'm wondering should we move all the initialization of has_msr_*, that
>>> associated with feature MSRs, to here. e.g., has_msr_arch_capabs,
>>> has_msr_vmx_vmfunc,...
>>>
>> I believe this is a more elegant way to fix the issue, which will be
>> reflected in my next patch.
> When attempting to move the detection logic for feature MSRs (currently
> including VMX_VMFUNC, UCODE_REV, ARCH_CAPABILITIES,
> PROCBASED_CTLS2) from kvm_get_supported_msrs to
> kvm_get_supported_feature_msrs in the current QEMU,
> I encountered an "error: failed to set MSR 0x491 to 0x***" on kernel
> 4.19.67.
> This issue is due to 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 proactively tries 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)).
>
> Therefore, even if we were to move all feature MSRs to
> kvm_get_supported_feature_msrs,VMX_VMFUNC could not be moved due to
> the need to maintain compatibility with different kernel versions. This
> exception makes our move less elegant. Hence, I am wondering whether we
> need to move all feature MSRs to kvm_get_supported_feature_msrs. Perhaps
> we just need to simply move PROCBASED_CTLS2 to fix the "failed set
> 0x48b ..."
> type of bugs, and add a comment about it?
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-18 9:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-27 3:43 [PATCH v2] target/i386/kvm: Refine VMX controls setting for backward compatibility EwanHai
2024-02-19 7:54 ` PING: " Ewan Hai
2024-02-20 8:32 ` Xiaoyao Li
2024-02-20 11:07 ` Ewan Hai
2024-02-23 3:13 ` Ewan Hai
2024-06-18 9:17 ` Ewan Hai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).