| Message ID | 20240624095806.214525-1-ewanhai-oc@zhaoxin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/i386/kvm: Refine VMX controls setting for backward compatibility | expand |
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; > } >
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
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?
[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
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
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
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; }
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(+)