kvm: nVMX: Relax guest IA32_FEATURE_CONTROL constraints

Commit 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL") broke the KVM_SET_MSRS ABI by instituting new constraints on the data values that kvm would accept for the guest MSR, IA32_FEATURE_CONTROL. Perhaps these constraints should have been opt-in via a new KVM capability, but they were applied indiscriminately, breaking at least one existing hypervisor.

Relax the constraints to allow either or both of FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX and FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX to be set when nVMX is enabled. This change is sufficient to fix the aforementioned breakage.

Fixes 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL")

Signed-off-by Jim Mattson <[email protected]>

---
arch/x86/kvm/vmx/vmx.c | 4 +++-

1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/vmx.c +3 -1

index 04a8212704c17..9f46023451810 100644
@@ -7097,10 +7097,12 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 
 	if (nested_vmx_allowed(vcpu))
 		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
+			FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
 			FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
 	else
 		to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
-			~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
+			~(FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
+			  FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX);
 
 	if (nested_vmx_allowed(vcpu)) {
 		nested_vmx_cr_fixed1_bits_update(vcpu);

Signature · 1 line

2.24.0.432.g9d3f5f5b63-goog

Quoted 17 lines · Jim Mattson

Commit 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL") broke the KVM_SET_MSRS ABI by instituting new constraints on the data values that kvm would accept for the guest MSR, IA32_FEATURE_CONTROL. Perhaps these constraints should have been opt-in via a new KVM capability, but they were applied indiscriminately, breaking at least one existing hypervisor.

Relax the constraints to allow either or both of FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX and FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX to be set when nVMX is enabled. This change is sufficient to fix the aforementioned breakage.

Quoted 2 lines

Fixes 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL")

Signed-off-by Jim Mattson <[email protected]>

I suggest to also add a comment in code to clarify why we allow setting FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX even though we expose a vCPU that doesn’t support Intel TXT.
(I think the compatibility to existing workloads that sets this blindly on boot is a legit reason. Just recommend documenting it.)

In addition, if the nested hypervisor which relies on this is public, please also mention it in commit message for reference.

Reviewed-by Liran Alon <[email protected]>

Quoted 25 lines

---
arch/x86/kvm/vmx/vmx.c | 4 +++-

1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/vmx.c +3 -2

index 04a8212704c17..9f46023451810 100644
@@ -7097,10 +7097,12 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 
      if (nested_vmx_allowed(vcpu))
              to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
+                     FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
                      FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
      else
              to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
-                     ~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
+                     ~(FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
+                       FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX);
 
      if (nested_vmx_allowed(vcpu)) {
              nested_vmx_cr_fixed1_bits_update(vcpu);
--

2.24.0.432.g9d3f5f5b63-goog

Quoted 23 lines · Liran Alon

Quoted 11 lines · Jim Mattson

Commit 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL") broke the KVM_SET_MSRS ABI by instituting new constraints on the data values that kvm would accept for the guest MSR, IA32_FEATURE_CONTROL. Perhaps these constraints should have been opt-in via a new KVM capability, but they were applied indiscriminately, breaking at least one existing hypervisor.

Quoted 7 lines

Relax the constraints to allow either or both of FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX and FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX to be set when nVMX is enabled. This change is sufficient to fix the aforementioned breakage.

Fixes 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL")

Signed-off-by Jim Mattson <[email protected]>

I suggest to also add a comment in code to clarify why we allow setting FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX even though we expose a vCPU that doesn’t support Intel TXT.
(I think the compatibility to existing workloads that sets this blindly on boot is a legit reason. Just recommend documenting it.)

Quoted 1 line

In addition, if the nested hypervisor which relies on this is public, please also mention it in commit message for reference.

It's not an L1 hypervisor that's the problem. It's Google's L0 hypervisor. We've been incorrectly reporting IA32_FEATURE_CONTROL as 7 to nested guests for years, and now we have thousands of running VMs with the bogus value. I've thought about just changing it to 5 on the fly (on real hardware, one could almost blame it on SMM, but the MSR is *locked*, after all).

Quoted 29 lines · Jim Mattson

Quoted 23 lines · Liran Alon

Quoted 11 lines · Jim Mattson

Commit 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL") broke the KVM_SET_MSRS ABI by instituting new constraints on the data values that kvm would accept for the guest MSR, IA32_FEATURE_CONTROL. Perhaps these constraints should have been opt-in via a new KVM capability, but they were applied indiscriminately, breaking at least one existing hypervisor.

Quoted 7 lines

Relax the constraints to allow either or both of FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX and FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX to be set when nVMX is enabled. This change is sufficient to fix the aforementioned breakage.

Fixes 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL")

Signed-off-by Jim Mattson <[email protected]>

I suggest to also add a comment in code to clarify why we allow setting FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX even though we expose a vCPU that doesn’t support Intel TXT.
(I think the compatibility to existing workloads that sets this blindly on boot is a legit reason. Just recommend documenting it.)

Quoted 8 lines

Quoted 1 line

In addition, if the nested hypervisor which relies on this is public, please also mention it in commit message for reference.

It's not an L1 hypervisor that's the problem. It's Google's L0 hypervisor. We've been incorrectly reporting IA32_FEATURE_CONTROL as 7 to nested guests for years, and now we have thousands of running VMs with the bogus value. I've thought about just changing it to 5 on the fly (on real hardware, one could almost blame it on SMM, but the MSR is *locked*, after all).

If I understand correctly, you are talking about the case a VM is already running and you will perform Live-Migration on it to a new host with a new kernel that it’s KVM don’t allow to set FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX.

If we haven’t encountered yet some guest workload that blindly sets this bit in IA32_FEATURE_CONTROL, then it should be sufficient for you to modify vmx_set_msr() to allow setting this bit in case msr_info->host_initiated (i.e. During restore of VM state on destination) but disallow it when WRMSR is initiated from guest. The behaviour of whether vmx_set_msr() allows host to set this MSR to unsupported can even be gated by an opt-in KVM cap that will be set by Google’s userspace VMM.

That way, you won’t change change guest runtime semantics (it’s original locked MSR value will be preserved during Live-Migration), and you will disallow newly provisioned guests from setting IA32_FEATURE_CONTROL to an unsupported value.

What do you think?

-Liran

I would call that an opt-out cap, rather than an opt-in cap. That is, we'd like to opt-out from the ABI change. I was going to go ahead and do it, but it looks like Paolo has accepted the change as it is.

Quoted 3 lines · Jim Mattson

I would call that an opt-out cap, rather than an opt-in cap. That is, we'd like to opt-out from the ABI change. I was going to go ahead and do it, but it looks like Paolo has accepted the change as it is.

Yes, your experience has proved that guests are not annoyed by the discrepancy between VMX_INSIDE_SMX and the actual lack of SMX, so it's pointless to complicate the code.

Quoted 8 lines

I suggest to also add a comment in code to clarify why we allow setting FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX even though we expose a vCPU that doesn’t support Intel TXT.
(I think the compatibility to existing workloads that sets this blindly on boot is a legit reason. Just recommend documenting it.)

In addition, if the nested hypervisor which relies on this is public, please also mention it in commit message for reference.

It's not an L1 hypervisor that's the problem. It's Google's L0 hypervisor. We've been incorrectly reporting IA32_FEATURE_CONTROL as 7 to nested guests for years, and now we have thousands of running VMs with the bogus value. I've thought about just changing it to 5 on the fly (on real hardware, one could almost blame it on SMM, but the MSR is *locked*, after all).