regression: nested: L1 3.15+ fails to load kvm-intel on L0 <3.15

Regardless of that, I wonder whether the below (this version untested) sound acceptable for upstream? At least it would make debugging much simpler. :)

arch/x86/kvm/vmx.c +0 -0

@@ -2953,8 +2953,11 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ct
        ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
 

Quoted 17 lines · Stefan Bader

Regardless of that, I wonder whether the below (this version untested) sound acceptable for upstream? At least it would make debugging much simpler. :)

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

@@ -2953,8 +2953,11 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ct
        ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
 
        /* Ensure minimum (required) set of control bits are supported. */
-       if (ctl_min & ~ctl)
+       if (ctl_min & ~ctl) {
+               printk(KERN_ERR "vmx: msr(%08x) does not match requirements. "
+                               "req=%08x cur=%08x\n", msr, ctl_min, ctl);
                return -EIO;
+       }
 

arch/x86/kvm/vmx.c static __init int adjust_vmx_controls(u32 ctl_min, u32 ct · line 2963

Quoted 2 lines

arch/x86/kvm/vmx.c +0 -0

        *result = ctl;
        return 0;

Yes, this is nice. Maybe -ENODEV.

Also, a minimal patch for Ubuntu would probably be:

(quoted diff) +1 -1

@@ -2850,7 +2851,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 		      vmx_capability.ept, vmx_capability.vpid);
 	}
 
-	min = 0;
+	min = VM_EXIT_SAVE_DEBUG_CONTROLS;
 #ifdef CONFIG_X86_64
 	min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
 #endif

but I don't think it's a good idea to add it to stable kernels.

Paolo

Quoted 8 lines

(quoted diff) +0 -1

@@ -2850,7 +2851,7 @@ static __init int setup_vmcs_config(struct
 vmcs_config *vmcs_conf) vmx_capability.ept,
 vmx_capability.vpid); }
 
-	min = 0; +	min = VM_EXIT_SAVE_DEBUG_CONTROLS; #ifdef

CONFIG_X86_64 min |= VM_EXIT_HOST_ADDR_SPACE_SIZE; #endif

but I don't think it's a good idea to add it to stable kernels.

Why is that? Because it has a risk of causing the module failing to load on L0 where it did work before?

Which would be something I would rather avoid. Generally I think it would be good to have something that can be generally applied. Given the speed that cloud service providers tend to move forward (ok they may not actively push the ability to go nested).

Quoted 11 lines · Stefan Bader

Quoted 8 lines

(quoted diff) +0 -1

@@ -2850,7 +2851,7 @@ static __init int setup_vmcs_config(struct
 vmcs_config *vmcs_conf) vmx_capability.ept,
 vmx_capability.vpid); }
 
-	min = 0; +	min = VM_EXIT_SAVE_DEBUG_CONTROLS; #ifdef

CONFIG_X86_64 min |= VM_EXIT_HOST_ADDR_SPACE_SIZE; #endif

but I don't think it's a good idea to add it to stable kernels.

Why is that? Because it has a risk of causing the module failing to load on L0 where it did work before?

Because if we wanted to make 3.14 nested VMX stable-ish we would need several more, at least these:

KVM: nVMX: fix lifetime issues for vmcs02
KVM: nVMX: clean up nested_release_vmcs12 and code around it
KVM: nVMX: Rework interception of IRQs and NMIs
KVM: nVMX: Do not inject NMI vmexits when L2 has a pending
interrupt
KVM: nVMX: Disable preemption while reading from shadow VMCS

and for 3.13:

KVM: nVMX: Leave VMX mode on clearing of feature control MSR

There are also several L2-crash-L1 bugs too in Nadav Amit's patches.

Basically, nested VMX was never considered stable-worthy. Perhaps that can change soon---but not retroactively.

So I'd rather avoid giving false impressions of the stability of nVMX in 3.14.

Even if we considered nVMX stable, I'd _really_ not want to consider the L1<->L2 boundary a secure one for a longer time.

Quoted 4 lines

Which would be something I would rather avoid. Generally I think it would be good to have something that can be generally applied. Given the speed that cloud service providers tend to move forward (ok they may not actively push the ability to go nested).

And if they did, I'd really not want them to do it with a 3.14 kernel.

Paolo

Quoted 9 lines · Stefan Bader

Regardless of that, I wonder whether the below (this version untested) sound acceptable for upstream? At least it would make debugging much simpler. :)

arch/x86/kvm/vmx.c +0 -0

@@ -2953,8 +2953,11 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ct
        ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
 

arch/x86/kvm/vmx.c static __init int adjust_vmx_controls(u32 ctl_min, u32 ct · line 2963

Quoted 10 lines

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

        /* Ensure minimum (required) set of control bits are supported. */
-       if (ctl_min & ~ctl)
+       if (ctl_min & ~ctl) {
+               printk(KERN_ERR "vmx: msr(%08x) does not match requirements. "
+                               "req=%08x cur=%08x\n", msr, ctl_min, ctl);
                return -EIO;
+       }
 
        *result = ctl;
        return 0;

Yes, this is nice. Maybe -ENODEV.

Also, a minimal patch for Ubuntu would probably be:

(quoted diff) +1 -1

@@ -2850,7 +2851,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 		      vmx_capability.ept, vmx_capability.vpid);
 	}
 
-	min = 0;
+	min = VM_EXIT_SAVE_DEBUG_CONTROLS;
 #ifdef CONFIG_X86_64
 	min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
 #endif

but I don't think it's a good idea to add it to stable kernels.

Paolo

This was added in 3.10. So the range of kernels affected <3.10 back to when nested vmx became somewhat usable. For 3.2 Ben (and obviously us) would be affected. Apart from that, I believe, it is only 3.4 which has an active longterm. At least that change looks safer for stable as it sounds like correcting things and not adding a feature. I was able to cherry-pick that into a 3.2 kernel and then a 3.16 guest successfully can load the kvm-intel module again, of course with the same shortcomings as before.