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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Zhao,

since I found last email contains non-plain-text content, andkvm@xxxxxxxxxxxxxxx
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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
  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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
  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.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux