nVMX: Nested VMX, v10

From: Nadav Har'El
Sent: Tuesday, May 17, 2011 3:45 AM

This patch allows a guest to use the VMXON and VMXOFF instructions, and emulates them accordingly. Basically this amounts to checking some prerequisites, and then remembering whether the guest has enabled or disabled VMX operation.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |  110
++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 2 deletions(-)

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

@@ -130,6 +130,15 @@ struct shared_msr_entry {
 	u64 mask;
 };
 
+/*
+ * The nested_vmx structure is part of vcpu_vmx, and holds information
+we need
+ * for correct emulation of VMX (i.e., nested VMX) on this vcpu.
+ */
+struct nested_vmx {
+	/* Has the level1 guest done vmxon? */
+	bool vmxon;
+};
+
 struct vcpu_vmx {
 	struct kvm_vcpu       vcpu;
 	struct list_head      local_vcpus_link;

@@ -184,6 +193,9 @@ struct vcpu_vmx {
 	u32 exit_reason;
 
 	bool rdtscp_enabled;
+
+	/* Support for a guest hypervisor (nested VMX) */
+	struct nested_vmx nested;
 };
 
 enum segment_cache_field {

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

+ * region. Consequently, VMCLEAR and VMPTRLD also do not verify that
+the their
+ * argument is different from the VMXON pointer (which the spec says they

do).
+ */

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

+static int handle_vmon(struct kvm_vcpu *vcpu) {
+	struct kvm_segment cs;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	/* The Intel VMX Instruction Reference lists a bunch of bits that
+	 * are prerequisite to running VMXON, most notably cr4.VMXE must be
+	 * set to 1 (see vmx_set_cr4() for when we allow the guest to set this).
+	 * Otherwise, we should fail with #UD. We test these now:
+	 */
+	if (!kvm_read_cr4_bits(vcpu, X86_CR4_VMXE) ||
+	    !kvm_read_cr0_bits(vcpu, X86_CR0_PE) ||
+	    (vmx_get_rflags(vcpu) & X86_EFLAGS_VM)) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 1;
+	}
+

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

+
+	vmx->nested.vmxon = true;
+
+	skip_emulated_instruction(vcpu);
+	return 1;
+}
+
+/*
+ * Intel's VMX Instruction Reference specifies a common set of
+prerequisites
+ * for running VMX instructions (except VMXON, whose prerequisites are
+ * slightly different). It also specifies what exception to inject otherwise.
+ */
+static int nested_vmx_check_permission(struct kvm_vcpu *vcpu) {
+	struct kvm_segment cs;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	if (!vmx->nested.vmxon) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;
+	}
+
+	vmx_get_segment(vcpu, &cs, VCPU_SREG_CS);
+	if ((vmx_get_rflags(vcpu) & X86_EFLAGS_VM) ||
+	    (is_long_mode(vcpu) && !cs.l)) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;
+	}
+
+	if (vmx_get_cpl(vcpu)) {
+		kvm_inject_gp(vcpu, 0);
+		return 0;
+	}
+
+	return 1;
+}
+
+/*
+ * Free whatever needs to be freed from vmx->nested when L1 goes down,
+or
+ * just stops using VMX.
+ */

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

+	free_nested(to_vmx(vcpu));
+	skip_emulated_instruction(vcpu);
+	return 1;
+}
+
+/*
  * The exit handlers return 1 if the exit was handled fully and guest execution
  * may resume.  Otherwise they set the kvm_run parameter to indicate

what needs
* to be done to userspace and return 0.

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

@@ -3917,8 +4022,8 @@ static int (*kvm_vmx_exit_handlers[])(st
 	[EXIT_REASON_VMREAD]                  = handle_vmx_insn,
 	[EXIT_REASON_VMRESUME]                = handle_vmx_insn,
 	[EXIT_REASON_VMWRITE]                 = handle_vmx_insn,
-	[EXIT_REASON_VMOFF]                   = handle_vmx_insn,
-	[EXIT_REASON_VMON]                    = handle_vmx_insn,
+	[EXIT_REASON_VMOFF]                   = handle_vmoff,
+	[EXIT_REASON_VMON]                    = handle_vmon,
 	[EXIT_REASON_TPR_BELOW_THRESHOLD]     =
 handle_tpr_below_threshold,
 	[EXIT_REASON_APIC_ACCESS]             = handle_apic_access,

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

 	[EXIT_REASON_WBINVD]                  = handle_wbinvd,

@@ -4329,6 +4434,7 @@ static void vmx_free_vcpu(struct kvm_vcp
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	free_vpid(vmx);
+	free_nested(vmx);
 	vmx_free_vmcs(vcpu);
 	kfree(vmx->guest_msrs);
 	kvm_vcpu_uninit(vcpu);
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

From: Nadav Har'El
Sent: Tuesday, May 17, 2011 3:48 AM

We saw in a previous patch that L1 controls its L2 guest with a vcms12. L0 needs to create a real VMCS for running L2. We call that "vmcs02". A later patch will contain the code, prepare_vmcs02(), for filling the vmcs02 fields. This patch only contains code for allocating vmcs02.

In this version, prepare_vmcs02() sets *all* of vmcs02's fields each time we enter from L1 to L2, so keeping just one vmcs02 for the vcpu is enough: It can be reused even when L1 runs multiple L2 guests. However, in future versions we'll probably want to add an optimization where vmcs02 fields that rarely change will not be set each time. For that, we may want to keep around several vmcs02s of L2 guests that have recently run, so that potentially we could run these L2s again more quickly because less vmwrites to vmcs02 will be needed.

This patch adds to each vcpu a vmcs02 pool, vmx->nested.vmcs02_pool, which remembers the vmcs02s last used to run up to VMCS02_POOL_SIZE L2s. As explained above, in the current version we choose VMCS02_POOL_SIZE=1, I.e., one vmcs02 is allocated (and loaded onto the processor), and it is reused to enter any L2 guest. In the future, when prepare_vmcs02() is optimized not to set all fields every time, VMCS02_POOL_SIZE should be increased.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |  139
+++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

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

@@ -117,6 +117,7 @@ static int ple_window = KVM_VMX_DEFAULT_
 module_param(ple_window, int, S_IRUGO);
 
 #define NR_AUTOLOAD_MSRS 1
+#define VMCS02_POOL_SIZE 1
 
 struct vmcs {
 	u32 revision_id;

@@ -166,6 +167,30 @@ struct __packed vmcs12 {
 #define VMCS12_SIZE 0x1000
 
 /*
+ * When we temporarily switch a vcpu's VMCS (e.g., stop using an L1's VMCS
+ * while we use L2's VMCS), and we wish to save the previous VMCS, we must

also
+ * remember on which CPU it was last loaded (vcpu->cpu), so when we return to
+ * using this VMCS we'll know if we're now running on a different CPU and need
+ * to clear the VMCS on the old CPU, and load it on the new one. Additionally,
+ * we need to remember whether this VMCS was launched (vmx->launched), so when
+ * we return to it we know if to VMLAUNCH or to VMRESUME it (we cannot deduce
+ * this from other state, because it's possible that this VMCS had once been
+ * launched, but has since been cleared after a CPU switch).
+ */

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

+	struct saved_vmcs vmcs02;
+};
+
+/*
  * The nested_vmx structure is part of vcpu_vmx, and holds information we

need
* for correct emulation of VMX (i.e., nested VMX) on this vcpu. */

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

@@ -178,6 +203,10 @@ struct nested_vmx {
 	/* The host-usable pointer to the above */
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
+
+	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
+	struct list_head vmcs02_pool;
+	int vmcs02_num;
 };
 
 struct vcpu_vmx {

@@ -4200,6 +4229,111 @@ static int handle_invalid_op(struct kvm_
 }
 
 /*
+ * To run an L2 guest, we need a vmcs02 based the L1-specified vmcs12.
+ * We could reuse a single VMCS for all the L2 guests, but we also want the
+ * option to allocate a separate vmcs02 for each separate loaded vmcs12 -

this
+ * allows keeping them loaded on the processor, and in the future will allow
+ * optimizations where prepare_vmcs02 doesn't need to set all the fields on
+ * every entry if they never change.
+ * So we keep, in vmx->nested.vmcs02_pool, a cache of size VMCS02_POOL_SIZE
+ * (>=0) with a vmcs02 for each recently loaded vmcs12s, most recent first.
+ *
+ * The following functions allocate and free a vmcs02 in this pool.
+ */

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

+
+static void __nested_free_saved_vmcs(void *arg)
+{
+	struct saved_vmcs *saved_vmcs = arg;
+
+	vmcs_clear(saved_vmcs->vmcs);
+	if (per_cpu(current_vmcs, saved_vmcs->cpu) == saved_vmcs->vmcs)
+		per_cpu(current_vmcs, saved_vmcs->cpu) = NULL;
+}
+
+/*
+ * Free a VMCS, but before that VMCLEAR it on the CPU where it was last

loaded
+ * (the necessary information is in the saved_vmcs structure).
+ * See also vcpu_clear() (with different parameters and side-effects)
+ */

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

+static void nested_free_saved_vmcs(struct vcpu_vmx *vmx,
+		struct saved_vmcs *saved_vmcs)
+{
+	if (saved_vmcs->cpu != -1)
+		smp_call_function_single(saved_vmcs->cpu,
+				__nested_free_saved_vmcs, saved_vmcs, 1);
+
+	free_vmcs(saved_vmcs->vmcs);
+}
+
+/* Free and remove from pool a vmcs02 saved for a vmcs12 (if there is one) */
+static void nested_free_vmcs02(struct vcpu_vmx *vmx, gpa_t vmptr)
+{
+	struct vmcs02_list *item;
+	list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
+		if (item->vmcs12_addr == vmptr) {
+			nested_free_saved_vmcs(vmx, &item->vmcs02);
+			list_del(&item->list);
+			kfree(item);
+			vmx->nested.vmcs02_num--;
+			return;
+		}
+}
+
+/*
+ * Free all VMCSs saved for this vcpu, except the actual vmx->vmcs.
+ * These include the VMCSs in vmcs02_pool (except the one currently used,
+ * if running L2), and saved_vmcs01 when running L2.
+ */
+static void nested_free_all_saved_vmcss(struct vcpu_vmx *vmx)
+{
+	struct vmcs02_list *item, *n;
+	list_for_each_entry_safe(item, n, &vmx->nested.vmcs02_pool, list) {
+		if (vmx->vmcs != item->vmcs02.vmcs)
+			nested_free_saved_vmcs(vmx, &item->vmcs02);
+		list_del(&item->list);
+		kfree(item);
+	}
+	vmx->nested.vmcs02_num = 0;
+}
+
+/* Get a vmcs02 for the current vmcs12. */
+static struct saved_vmcs *nested_get_current_vmcs02(struct vcpu_vmx

*vmx)

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

+{
+	struct vmcs02_list *item;
+	list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
+		if (item->vmcs12_addr == vmx->nested.current_vmptr) {
+			list_move(&item->list, &vmx->nested.vmcs02_pool);
+			return &item->vmcs02;
+		}
+
+	if (vmx->nested.vmcs02_num >= max(VMCS02_POOL_SIZE, 1)) {
+		/* Recycle the least recently used VMCS. */
+		item = list_entry(vmx->nested.vmcs02_pool.prev,
+			struct vmcs02_list, list);
+		item->vmcs12_addr = vmx->nested.current_vmptr;
+		list_move(&item->list, &vmx->nested.vmcs02_pool);
+		return &item->vmcs02;
+	}
+
+	/* Create a new vmcs02 */
+	item = (struct vmcs02_list *)
+		kmalloc(sizeof(struct vmcs02_list), GFP_KERNEL);
+	if (!item)
+		return NULL;
+	item->vmcs02.vmcs = alloc_vmcs();
+	if (!item->vmcs02.vmcs) {
+		kfree(item);
+		return NULL;
+	}
+	item->vmcs12_addr = vmx->nested.current_vmptr;
+	item->vmcs02.cpu = -1;
+	item->vmcs02.launched = 0;
+	list_add(&(item->list), &(vmx->nested.vmcs02_pool));
+	vmx->nested.vmcs02_num++;
+	return &item->vmcs02;
+}
+
+/*
  * Emulate the VMXON instruction.
  * Currently, we just remember that VMX is active, and do not save or even
  * inspect the argument to VMXON (the so-called "VMXON pointer") because

we

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

@@ -4235,6 +4369,9 @@ static int handle_vmon(struct kvm_vcpu *
 		return 1;
 	}
 
+	INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
+	vmx->nested.vmcs02_num = 0;
+
 	vmx->nested.vmxon = true;
 
 	skip_emulated_instruction(vcpu);

@@ -4286,6 +4423,8 @@ static void free_nested(struct vcpu_vmx
 		vmx->nested.current_vmptr = -1ull;
 		vmx->nested.current_vmcs12 = NULL;
 	}
+
+	nested_free_all_saved_vmcss(vmx);
 }
 
 /* Emulate the VMXOFF instruction */
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Tian, Kevin
Sent: Friday, May 20, 2011 4:05 PM

Quoted 8 lines

From: Nadav Har'El
Sent: Tuesday, May 17, 2011 3:48 AM

We saw in a previous patch that L1 controls its L2 guest with a vcms12. L0 needs to create a real VMCS for running L2. We call that "vmcs02". A later patch will contain the code, prepare_vmcs02(), for filling the vmcs02 fields. This patch only contains code for allocating vmcs02.

Quoted 5 lines

In this version, prepare_vmcs02() sets *all* of vmcs02's fields each time we enter from L1 to L2, so keeping just one vmcs02 for the vcpu is enough: It can be reused even when L1 runs multiple L2 guests. However, in future versions we'll probably want to add an optimization where vmcs02 fields that rarely change will not be set each time. For that, we may want to keep around

several

Quoted 2 lines

vmcs02s of L2 guests that have recently run, so that potentially we could run these L2s again more quickly because less vmwrites to vmcs02 will be

needed.

That would be a neat enhancement and should have an obvious improvement. Possibly we can maintain the vmcs02 pool along with L1 VMCLEAR ops, which is similar to the hardware behavior regarding to cleared and launched state.

Quoted 3 lines

This patch adds to each vcpu a vmcs02 pool, vmx->nested.vmcs02_pool, which remembers the vmcs02s last used to run up to VMCS02_POOL_SIZE

L2s.

Quoted 23 lines

As explained above, in the current version we choose VMCS02_POOL_SIZE=1, I.e., one vmcs02 is allocated (and loaded onto the processor), and it is reused to enter any L2 guest. In the future, when prepare_vmcs02() is optimized not to set all fields every time, VMCS02_POOL_SIZE should be increased.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |  139
+++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

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

@@ -117,6 +117,7 @@ static int ple_window = KVM_VMX_DEFAULT_
 module_param(ple_window, int, S_IRUGO);
 
 #define NR_AUTOLOAD_MSRS 1
+#define VMCS02_POOL_SIZE 1
 
 struct vmcs {
 	u32 revision_id;

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 170

Quoted 4 lines

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

@@ -166,6 +167,30 @@ struct __packed vmcs12 {
 #define VMCS12_SIZE 0x1000
 
 /*
+ * When we temporarily switch a vcpu's VMCS (e.g., stop using an L1's

VMCS

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 171

Quoted 1 line

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

+ * while we use L2's VMCS), and we wish to save the previous VMCS, we

must

Quoted 2 lines

also
+ * remember on which CPU it was last loaded (vcpu->cpu), so when we

return

Quoted 4 lines

to
+ * using this VMCS we'll know if we're now running on a different CPU and need
+ * to clear the VMCS on the old CPU, and load it on the new one.

Additionally,

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 184

Quoted 12 lines

+ * we need to remember whether this VMCS was launched (vmx->launched), so when
+ * we return to it we know if to VMLAUNCH or to VMRESUME it (we cannot deduce
+ * this from other state, because it's possible that this VMCS had once been
+ * launched, but has since been cleared after a CPU switch).
+ */

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

+struct saved_vmcs {
+	struct vmcs *vmcs;
+	int cpu;
+	int launched;
+};

"saved" looks a bit misleading here. It's simply a list of all active vmcs02 tracked
by kvm, isn't it?

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 189

Quoted 6 lines

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

+
+/* Used to remember the last vmcs02 used for some recently used vmcs12s

*/

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

+struct vmcs02_list {
+	struct list_head list;
+	gpa_t vmcs12_addr;

uniform the name 'vmptr' as nested_vmx strucure:
/* The guest-physical address of the current VMCS L1 keeps for L2 */

	gpa_t current_vmptr;
	/* The host-usable pointer to the above */
	struct page *current_vmcs12_page;
	struct vmcs12 *current_vmcs12;

you should keep consistent meaning for vmcs12, which means the arch-neutral state interpreted by KVM only.

Quoted 53 lines

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

+	struct saved_vmcs vmcs02;
+};
+
+/*
  * The nested_vmx structure is part of vcpu_vmx, and holds information we

need
* for correct emulation of VMX (i.e., nested VMX) on this vcpu. */

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

@@ -178,6 +203,10 @@ struct nested_vmx {
 	/* The host-usable pointer to the above */
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
+
+	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
+	struct list_head vmcs02_pool;
+	int vmcs02_num;
 };
 
 struct vcpu_vmx {

@@ -4200,6 +4229,111 @@ static int handle_invalid_op(struct kvm_
 }
 
 /*
+ * To run an L2 guest, we need a vmcs02 based the L1-specified vmcs12.
+ * We could reuse a single VMCS for all the L2 guests, but we also want the
+ * option to allocate a separate vmcs02 for each separate loaded vmcs12 -

this
+ * allows keeping them loaded on the processor, and in the future will allow
+ * optimizations where prepare_vmcs02 doesn't need to set all the fields on
+ * every entry if they never change.
+ * So we keep, in vmx->nested.vmcs02_pool, a cache of size VMCS02_POOL_SIZE
+ * (>=0) with a vmcs02 for each recently loaded vmcs12s, most recent first.
+ *
+ * The following functions allocate and free a vmcs02 in this pool.
+ */

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

+
+static void __nested_free_saved_vmcs(void *arg)
+{
+	struct saved_vmcs *saved_vmcs = arg;
+
+	vmcs_clear(saved_vmcs->vmcs);
+	if (per_cpu(current_vmcs, saved_vmcs->cpu) == saved_vmcs->vmcs)
+		per_cpu(current_vmcs, saved_vmcs->cpu) = NULL;
+}
+
+/*
+ * Free a VMCS, but before that VMCLEAR it on the CPU where it was last

loaded
+ * (the necessary information is in the saved_vmcs structure).
+ * See also vcpu_clear() (with different parameters and side-effects)
+ */

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

+static void nested_free_saved_vmcs(struct vcpu_vmx *vmx,

arch/x86/kvm/vmx.c static int handle_invalid_op(struct kvm_

Quoted 10 lines

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

+		struct saved_vmcs *saved_vmcs)
+{
+	if (saved_vmcs->cpu != -1)
+		smp_call_function_single(saved_vmcs->cpu,
+				__nested_free_saved_vmcs, saved_vmcs, 1);
+
+	free_vmcs(saved_vmcs->vmcs);
+}
+
+/* Free and remove from pool a vmcs02 saved for a vmcs12 (if there is one)

*/

Quoted 40 lines

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

+static void nested_free_vmcs02(struct vcpu_vmx *vmx, gpa_t vmptr)
+{
+	struct vmcs02_list *item;
+	list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
+		if (item->vmcs12_addr == vmptr) {
+			nested_free_saved_vmcs(vmx, &item->vmcs02);
+			list_del(&item->list);
+			kfree(item);
+			vmx->nested.vmcs02_num--;
+			return;
+		}
+}
+
+/*
+ * Free all VMCSs saved for this vcpu, except the actual vmx->vmcs.
+ * These include the VMCSs in vmcs02_pool (except the one currently used,
+ * if running L2), and saved_vmcs01 when running L2.
+ */
+static void nested_free_all_saved_vmcss(struct vcpu_vmx *vmx)
+{
+	struct vmcs02_list *item, *n;
+	list_for_each_entry_safe(item, n, &vmx->nested.vmcs02_pool, list) {
+		if (vmx->vmcs != item->vmcs02.vmcs)
+			nested_free_saved_vmcs(vmx, &item->vmcs02);
+		list_del(&item->list);
+		kfree(item);
+	}
+	vmx->nested.vmcs02_num = 0;
+}
+
+/* Get a vmcs02 for the current vmcs12. */
+static struct saved_vmcs *nested_get_current_vmcs02(struct vcpu_vmx

*vmx)

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

+{
+	struct vmcs02_list *item;
+	list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
+		if (item->vmcs12_addr == vmx->nested.current_vmptr) {
+			list_move(&item->list, &vmx->nested.vmcs02_pool);
+			return &item->vmcs02;
+		}

arch/x86/kvm/vmx.c static int handle_invalid_op(struct kvm_ · line 4315

Quoted 8 lines

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

+
+	if (vmx->nested.vmcs02_num >= max(VMCS02_POOL_SIZE, 1)) {
+		/* Recycle the least recently used VMCS. */
+		item = list_entry(vmx->nested.vmcs02_pool.prev,
+			struct vmcs02_list, list);
+		item->vmcs12_addr = vmx->nested.current_vmptr;
+		list_move(&item->list, &vmx->nested.vmcs02_pool);
+		return &item->vmcs02;

Quoted 14 lines

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

+	}
+
+	/* Create a new vmcs02 */
+	item = (struct vmcs02_list *)
+		kmalloc(sizeof(struct vmcs02_list), GFP_KERNEL);
+	if (!item)
+		return NULL;
+	item->vmcs02.vmcs = alloc_vmcs();
+	if (!item->vmcs02.vmcs) {
+		kfree(item);
+		return NULL;
+	}
+	item->vmcs12_addr = vmx->nested.current_vmptr;
+	item->vmcs02.cpu = -1;

arch/x86/kvm/vmx.c static int handle_invalid_op(struct kvm_

Quoted 10 lines

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

+	item->vmcs02.launched = 0;
+	list_add(&(item->list), &(vmx->nested.vmcs02_pool));
+	vmx->nested.vmcs02_num++;
+	return &item->vmcs02;
+}
+
+/*
  * Emulate the VMXON instruction.
  * Currently, we just remember that VMX is active, and do not save or even
  * inspect the argument to VMXON (the so-called "VMXON pointer")

because

Quoted 21 lines

we

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

@@ -4235,6 +4369,9 @@ static int handle_vmon(struct kvm_vcpu *
 		return 1;
 	}
 
+	INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
+	vmx->nested.vmcs02_num = 0;
+
 	vmx->nested.vmxon = true;
 
 	skip_emulated_instruction(vcpu);

@@ -4286,6 +4423,8 @@ static void free_nested(struct vcpu_vmx
 		vmx->nested.current_vmptr = -1ull;
 		vmx->nested.current_vmcs12 = NULL;
 	}
+
+	nested_free_all_saved_vmcss(vmx);
 }
 
 /* Emulate the VMXOFF instruction */
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

btw, shouldn't you clear recycled VMCS and reset 'cpu' and 'launched' fields?

Have you tried SMP L2 guest?

From: Nadav Har'El [mailto:[email protected]]
Sent: Saturday, May 21, 2011 4:32 AM

On Fri, May 20, 2011, Tian, Kevin wrote about "RE: [PATCH 07/31] nVMX:
Introduce vmcs02: VMCS used to run L2":

Quoted 1 line

btw, shouldn't you clear recycled VMCS and reset 'cpu' and 'launched' fields?

Well, I believe the answer is "no": As far as I understood, a host is allowed to take a VMCS that has been used once to launch a certain guest, and then modify all the VMCS's fields to define a completely different guest, and then VMRESUME it, without doing the regular VMCLEAR/VMLAUNCH, even though it's
"a different guest". Is there something wrong in my assumption? Does VMX keep
anything constant between successive VMRESUMEs?

Quoted 2 lines

Have you tried SMP L2 guest?

It "sort of" works, but it *does* appear to still have a bug which I didn't yet have the time to hunt... In one case, for example, an 8-vcpu L2 on an 8-vcpu L1 seemed to work well (e.g., doing parallel make) for about a minute, and then hung with some sort of page fault in the kernel.

Here the vmcs02 being overridden may have been run on another processor before but is not vmclear-ed yet. When you resume this vmcs02 with new content on a separate processor, the risk of corruption exists.

From: Nadav Har'El [mailto:[email protected]]
Sent: Sunday, May 22, 2011 3:23 PM

Hi,

On Sun, May 22, 2011, Tian, Kevin wrote about "RE: [PATCH 07/31] nVMX:
Introduce vmcs02: VMCS used to run L2":

Quoted 1 line

Here the vmcs02 being overridden may have been run on another processor

before

Quoted 1 line

but is not vmclear-ed yet. When you resume this vmcs02 with new content on

a

Quoted 1 line

separate processor, the risk of corruption exists.

I still believe that my current code is correct (in this area). I'll try to explain it here and would be grateful if you could point to me the error (if there is one) in my logic:

Nested_vmx_run() is our function which is switches from running L1 to L2 (patch 18).

This function starts by calling nested_get_current_vmcs02(), which gets us *some* vmcs to use for vmcs02. This may be a fresh new VMCS, or a "recycled"
VMCS, some VMCS we've previously used to run some, potentially different L2 guest on some, potentially different, CPU. nested_get_current_vmcs02() returns a "saved_vmcs" structure, which not only contains a VMCS, but also remembers on which (if any) cpu it is currently loaded (and whether it was VMLAUNCHed once on that cpu).

The next thing that Nested_vmx_run() now does is to set up in the vcpu object the vmcs, cpu and launched fields according to what was returned above.

Now it calls vmx_vcpu_load(). This standard KVM function checks if we're now running on a different CPU from the vcpu->cpu, and if it a different one, is uses vcpu_clear() to VMCLEAR the vmcs on the CPU where it was last loaded (using an IPI). Only after it vmclears the VMCS on the old CPU, it can finally load the VMCS on the new CPU.

Only now Nested_vmx_run() can call prepare_vmcs02, which starts VMWRITEing
to this VMCS, and finally returns.

P.S. Seeing that you're from Intel, maybe you can help me with a pointer:
I found what appears to be a small error in the SDM - who can I report it to?

Possibly we can maintain the vmcs02 pool along with L1 VMCLEAR ops, which is similar to the hardware behavior regarding to cleared and launched state.

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 184

Quoted 5 lines

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

+struct saved_vmcs {
+	struct vmcs *vmcs;
+	int cpu;
+	int launched;
+};

"saved" looks a bit misleading here. It's simply a list of all active vmcs02 tracked by kvm, isn't it?

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 189

Quoted 5 lines

+/* Used to remember the last vmcs02 used for some recently used vmcs12s */

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

+struct vmcs02_list {
+	struct list_head list;
+	gpa_t vmcs12_addr;

uniform the name 'vmptr' as nested_vmx strucure:

From: Nadav Har'El [mailto:[email protected]]
Sent: Sunday, May 22, 2011 4:30 PM

Hi,

On Fri, May 20, 2011, Tian, Kevin wrote about "RE: [PATCH 07/31] nVMX:
Introduce vmcs02: VMCS used to run L2":

Quoted 2 lines

Possibly we can maintain the vmcs02 pool along with L1 VMCLEAR ops, which is similar to the hardware behavior regarding to cleared and launched state.

If you set VMCS02_POOL_SIZE to a large size, and L1, like typical hypervisors, only keeps around a few VMCSs (and VMCLEARs the ones it will not use again), then we'll only have a few vmcs02: handle_vmclear() removes from the pool the vmcs02 that L1 explicitly told us it won't need again.

Quoted 8 lines

arch/x86/kvm/vmx.c struct __packed vmcs12 { · line 184

Quoted 6 lines

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

+struct saved_vmcs {
+	struct vmcs *vmcs;
+	int cpu;
+	int launched;
+};

"saved" looks a bit misleading here. It's simply a list of all active vmcs02

tracked

Quoted 1 line

by kvm, isn't it?

I have rewritten this part of the code, based on Avi's and Marcelo's requests, and the new name for this structure is "loaded_vmcs", i.e., a structure describing where a VMCS was loaded.

In VMX, before we bring down a CPU we must VMCLEAR all VMCSs loaded on it because (at least in theory) the processor might not have written all of its content back to memory. Since a patch from June 26, 2008, this is done using a per-cpu "vcpus_on_cpu" linked list of vcpus loaded on each CPU.

The problem is that with nested VMX, we no longer have the concept of a vcpu being loaded on a cpu: A vcpu has multiple VMCSs (one for L1, a pool for L2s), and each of those may be have been last loaded on a different cpu.

Our solution is to hold, in addition to vcpus_on_cpu, a second linked list saved_vmcss_on_cpu, which holds the current list of "saved" VMCSs, i.e., VMCSs which are loaded on this CPU but are not the vmx->vmcs of any of the vcpus. These saved VMCSs include L1's VMCS while L2 is running (saved_vmcs01), and L2 VMCSs not currently used - because L1 is running or because the vmcs02_pool contains more than one entry.

When we will switch between L1's and L2's VMCSs, they need to be moved between vcpus_on_cpu and saved_vmcs_on_cpu lists and vice versa. A new function, nested_maintain_per_cpu_lists(), takes care of that.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |   67 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

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

@@ -181,6 +181,7 @@ struct saved_vmcs {
 	struct vmcs *vmcs;
 	int cpu;
 	int launched;
+	struct list_head local_saved_vmcss_link; /* see saved_vmcss_on_cpu */
 };
 
 /* Used to remember the last vmcs02 used for some recently used vmcs12s */

@@ -315,7 +316,20 @@ static int vmx_set_tss_addr(struct kvm *
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
+/*
+ * We maintain a per-CPU linked-list vcpus_on_cpu, holding for each CPU a list
+ * of vcpus whose VMCS are loaded on that CPU. This is needed when a CPU is
+ * brought down, and we need to VMCLEAR all VMCSs loaded on it.
+ *
+ * With nested VMX, we have additional VMCSs which are not the current
+ * vmx->vmcs of any vcpu, but may also be loaded on some CPU: While L2 is
+ * running, L1's VMCS is loaded but not the VMCS of any vcpu; While L1 is
+ * running, a previously used L2 VMCS might still be around and loaded on some
+ * CPU, somes even more than one such L2 VMCSs is kept (see VMCS02_POOL_SIZE).
+ * The list of these additional VMCSs is kept on cpu saved_vmcss_on_cpu.
+ */
 static DEFINE_PER_CPU(struct list_head, vcpus_on_cpu);
+static DEFINE_PER_CPU(struct list_head, saved_vmcss_on_cpu);
 static DEFINE_PER_CPU(struct desc_ptr, host_gdt);
 
 static unsigned long *vmx_io_bitmap_a;

@@ -1818,6 +1832,7 @@ static int hardware_enable(void *garbage
 		return -EBUSY;
 
 	INIT_LIST_HEAD(&per_cpu(vcpus_on_cpu, cpu));
+	INIT_LIST_HEAD(&per_cpu(saved_vmcss_on_cpu, cpu));
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
 
 	test_bits = FEATURE_CONTROL_LOCKED;

@@ -1860,10 +1875,13 @@ static void kvm_cpu_vmxoff(void)
 	asm volatile (__ex(ASM_VMX_VMXOFF) : : : "cc");
 }
 
+static void vmclear_local_saved_vmcss(void);
+
 static void hardware_disable(void *garbage)
 {
 	if (vmm_exclusive) {
 		vmclear_local_vcpus();
+		vmclear_local_saved_vmcss();
 		kvm_cpu_vmxoff();
 	}
 	write_cr4(read_cr4() & ~X86_CR4_VMXE);

@@ -4248,6 +4266,8 @@ static void __nested_free_saved_vmcs(voi
 	vmcs_clear(saved_vmcs->vmcs);
 	if (per_cpu(current_vmcs, saved_vmcs->cpu) == saved_vmcs->vmcs)
 		per_cpu(current_vmcs, saved_vmcs->cpu) = NULL;
+	list_del(&saved_vmcs->local_saved_vmcss_link);
+	saved_vmcs->cpu = -1;
 }
 
 /*

@@ -4265,6 +4285,21 @@ static void nested_free_saved_vmcs(struc
 	free_vmcs(saved_vmcs->vmcs);
 }
 
+/*
+ * VMCLEAR all the currently unused (not vmx->vmcs on any vcpu) saved_vmcss
+ * which were loaded on the current CPU. See also vmclear_load_vcpus(), which
+ * does the same for VMCS currently used in vcpus.
+ */
+static void vmclear_local_saved_vmcss(void)
+{
+	int cpu = raw_smp_processor_id();
+	struct saved_vmcs *v, *n;
+
+	list_for_each_entry_safe(v, n, &per_cpu(saved_vmcss_on_cpu, cpu),
+				 local_saved_vmcss_link)
+		__nested_free_saved_vmcs(v);
+}
+
 /* Free and remove from pool a vmcs02 saved for a vmcs12 (if there is one) */
 static void nested_free_vmcs02(struct vcpu_vmx *vmx, gpa_t vmptr)
 {

@@ -5143,6 +5178,38 @@ static void vmx_set_supported_cpuid(u32 
 {
 }
 
+/*
+ * Maintain the vcpus_on_cpu and saved_vmcss_on_cpu lists of vcpus and
+ * inactive saved_vmcss on nested entry (L1->L2) or nested exit (L2->L1).
+ *
+ * nested_maintain_per_cpu_lists should be called after the VMCS was switched
+ * to the new one, with parameters giving both the new on (after the entry
+ * or exit) and the old one, in that order.
+ */
+static void nested_maintain_per_cpu_lists(struct vcpu_vmx *vmx,
+		struct saved_vmcs *new_vmcs,
+		struct saved_vmcs *old_vmcs)

Quoted 13 lines

+/*
+ * Maintain the vcpus_on_cpu and saved_vmcss_on_cpu lists of vcpus and
+ * inactive saved_vmcss on nested entry (L1->L2) or nested exit (L2->L1).
+ *
+ * nested_maintain_per_cpu_lists should be called after the VMCS was switched
+ * to the new one, with parameters giving both the new on (after the entry
+ * or exit) and the old one, in that order.
+ */
+static void nested_maintain_per_cpu_lists(struct vcpu_vmx *vmx,
+ struct saved_vmcs *new_vmcs,
+ struct saved_vmcs *old_vmcs)
+{
+ /*

Quoted 8 lines

+ * When a vcpus's old vmcs is saved, we need to drop it from
+ * vcpus_on_cpu and put it on saved_vmcss_on_cpu.
+ */
+ if (old_vmcs->cpu != -1) {
+ list_del(&vmx->local_vcpus_link);
+ list_add(&old_vmcs->local_saved_vmcss_link,
+ &per_cpu(saved_vmcss_on_cpu, old_vmcs->cpu));
+ }

This new handling of vmcs could be simplified (local_vcpus_link must be manipulated with interrupts disabled, BTW).

What about having a per-CPU VMCS list instead of per-CPU vcpu list? "local_vmcs_link" list node could be in "struct saved_vmcs" (and a current_saved_vmcs pointer in "struct vcpu_vmx").

vmx_vcpu_load would then add to this list at

         if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
                 per_cpu(current_vmcs, cpu) = vmx->vmcs;
                 vmcs_load(vmx->vmcs);
         }

Quoted 1 line

(local_vcpus_link must be >manipulated with interrupts disabled, BTW).

Quoted 1 line

What about having a per-CPU VMCS list instead of per-CPU vcpu list?

Perhaps even easier (avoids duplication):

struct raw_vmcs {
    u32 revision_id;
    u32 abort;
    char data[0];
};

struct vmcs {
    struct raw_vmcs *raw_vmcs;
    struct list_head local_vmcs_link;
};

struct vcpu_vmx {
    ...
    struct vmcs *vmcs;  /* often points at l1_vmcs */
    struct vmcs l1_vmcs;
    ...
};

static DEFINE_PER_CPU(struct list_head, vmcss_on_cpu);

On Tue, May 17, 2011, Avi Kivity wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 1 line

static DEFINE_PER_CPU(struct list_head, vmcss_on_cpu);

This is an interesting suggestion. My initial plan was to do something similar to this, and I agree it could have been nicer code, but I had to change it after bumping into too many obstacles.

For example, currently, vmclear_local_vcpus() not only VMCLEARs the vmcss, it also sets vmx->vcpu.cpu = -1, xmv->launched=0 for the vcpus holding these VMCSs. If we had only a list of VMCSs, how can we mark the vcpus as being not currently loaded (cpu=-1)?

Quoted 4 lines

For example, currently, vmclear_local_vcpus() not only VMCLEARs the vmcss, it also sets vmx->vcpu.cpu = -1, xmv->launched=0 for the vcpus holding these VMCSs. If we had only a list of VMCSs, how can we mark the vcpus as being not currently loaded (cpu=-1)?

Do it in vcpu_clear, its just an optimization not necessary in vmclear_local_vcpus path.

Quoted 15 lines

Quoted 2 lines

What about having a per-CPU VMCS list instead of per-CPU vcpu list?

Perhaps even easier (avoids duplication):

struct raw_vmcs {
     u32 revision_id;
     u32 abort;
     char data[0];
};

struct vmcs {
     struct raw_vmcs *raw_vmcs;
     struct list_head local_vmcs_link;
};

Quoted 8 lines

struct vcpu_vmx {
     ...
     struct vmcs *vmcs;  /* often points at l1_vmcs */
     struct vmcs l1_vmcs;
     ...
};

static DEFINE_PER_CPU(struct list_head, vmcss_on_cpu);

This is an interesting suggestion. My initial plan was to do something similar to this, and I agree it could have been nicer code, but I had to change it after bumping into too many obstacles.

For example, currently, vmclear_local_vcpus() not only VMCLEARs the vmcss, it also sets vmx->vcpu.cpu = -1, xmv->launched=0 for the vcpus holding these VMCSs. If we had only a list of VMCSs, how can we mark the vcpus as being not currently loaded (cpu=-1)?

Quoted 4 lines

VMCSs. If we had only a list of VMCSs, how can we mark the vcpus as being not
currently loaded (cpu=-1)?

->launched and ->cpu simply move into struct vmcs.

On Tue, May 17, 2011, Avi Kivity wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 6 lines

Quoted 4 lines

VMCSs. If we had only a list of VMCSs, how can we mark the vcpus as being not
currently loaded (cpu=-1)?

->launched and ->cpu simply move into struct vmcs.

As I explained in the sister thread (this discussion is becoming a tree ;-)) this is what I planned to do, until it dawned on me that I can't, because "cpu" isn't part of vmx (where the vmcs and launched sit in the standard KVM), but rather part of vcpu... When I gave up trying to "solve" these interdependencies and avoiding modifying half of KVM, I came up with the current solution.

Maybe I'm missing something - I'd be happy if we do find a solution that simplifies this code.

Quoted 2 lines

this is what I planned to do, until it dawned on me that I can't, because "cpu" isn't part of vmx (where the vmcs and launched sit in the standard KVM), but

...
vcpu->cpu remains there. There is a new ->cpu field on struct vmcs, just as saved_vmcs has in the current patches, to note the cpu which the VMCS was last loaded.

As mentioned there is no need to set "vcpu->cpu = -1" in __vcpu_clear, the IPI handler, that can be done in vcpu_clear.

On Tue, May 17, 2011, Marcelo Tosatti wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 6 lines

Quoted 2 lines

this is what I planned to do, until it dawned on me that I can't, because "cpu" isn't part of vmx (where the vmcs and launched sit in the standard KVM), but

...
vcpu->cpu remains there. There is a new ->cpu field on struct vmcs, just as saved_vmcs has in the current patches, to note the cpu which the VMCS was last loaded.

So we'll have two fields, vmx.vcpu.cpu and vmx.vmcs.cpu, which are supposed to always contain the same value. Are you fine with that?

Quoted 2 lines

So we'll have two fields, vmx.vcpu.cpu and vmx.vmcs.cpu, which are supposed to always contain the same value. Are you fine with that?

Yes. Avi?

On Tue, May 17, 2011, Marcelo Tosatti wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 4 lines · Nadav Har'El

Quoted 2 lines

So we'll have two fields, vmx.vcpu.cpu and vmx.vmcs.cpu, which are supposed to always contain the same value. Are you fine with that?

Quoted 1 line

Yes. Avi?

Oops, it's even worse than I said, because if the new vmclear_local_vmcss clears the vmcs currently used on some vcpu, it will update vmcs.cpu on that vcpu to -1, but will *not* update vmx.vcpu.cpu, which remain its old value, and potentially cause problems when it is used (e.g., in x86.c) instead of vmx.vmcs.cpu.

I did a quick audit and it seems fine. If it isn't, we'll fix it when we see the problem.

On Wed, May 18, 2011, Avi Kivity wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 2 lines

I did a quick audit and it seems fine. If it isn't, we'll fix it when we see the problem.

Ok, then, I'm working on the code with the new approach.

My fear was that some CPU 7 is taken down, but vcpu.cpu remains 7 (not set to -1). If cpu 7 nevers comes up again, it's not a problem because when we run the same vcpu again on a different cpu, it's not 7 so we do what needs to be done on CPU switch. But, what if CPU 7 does come up again later, and we find ourselves running again on CPU 7, but it's not the same CPU 7 and we don't know it? Is this case at all possible?

On Tue, May 17, 2011, Marcelo Tosatti wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 4 lines · Nadav Har'El

Quoted 2 lines

So we'll have two fields, vmx.vcpu.cpu and vmx.vmcs.cpu, which are supposed to always contain the same value. Are you fine with that?

Quoted 1 line

Yes. Avi?

Oops, it's even worse than I said, because if the new vmclear_local_vmcss clears the vmcs currently used on some vcpu, it will update vmcs.cpu on that vcpu to -1, but will *not* update vmx.vcpu.cpu, which remain its old value, and potentially cause problems when it is used (e.g., in x86.c) instead of vmx.vmcs.cpu.

Note this is not just about the code being nicer, but simplicity is crucial, the code is tricky enough with one linked list.

Hi Avi and Marcelo, here is a the new first patch to the nvmx patch set, which overhauls the handling of vmcss on cpus, as you asked.

As you guessed, the nested entry and exit code becomes much simpler and cleaner, with the whole VMCS switching code on entry, for example, reduced to:

	cpu = get_cpu();
	vmx->loaded_vmcs = vmcs02;
	vmx_vcpu_put(vcpu);
	vmx_vcpu_load(vcpu, cpu);
	vcpu->cpu = cpu;
	put_cpu();

You can apply this patch separately from the rest of the patch set, if you wish. I'm sending just this one, like you asked - and can send the rest of the patches when you ask me to.

Subject: [PATCH 01/31] nVMX: Keep list of loaded VMCSs, instead of vcpus.

In VMX, before we bring down a CPU we must VMCLEAR all VMCSs loaded on it because (at least in theory) the processor might not have written all of its content back to memory. Since a patch from June 26, 2008, this is done using a per-cpu "vcpus_on_cpu" linked list of vcpus loaded on each CPU.

The problem is that with nested VMX, we no longer have the concept of a vcpu being loaded on a cpu: A vcpu has multiple VMCSs (one for L1, a pool for L2s), and each of those may be have been last loaded on a different cpu.

So instead of linking the vcpus, we link the VMCSs, using a new structure loaded_vmcs. This structure contains the VMCS, and the information pertaining to its loading on a specific cpu (namely, the cpu number, and whether it was already launched on this cpu once). In nested we will also use the same structure to hold L2 VMCSs, and vmx->loaded_vmcs is a pointer to the currently active VMCS.

arch/x86/kvm/x86.c +8 -8

@@ -971,22 +992,22 @@ static void vmx_vcpu_load(struct kvm_vcp
 
  	if (!vmm_exclusive)
  		kvm_cpu_vmxon(phys_addr);
-	else if (vcpu->cpu != cpu)
-		vcpu_clear(vmx);
+	else if (vmx->loaded_vmcs->cpu != cpu)
+		loaded_vmcs_clear(vmx->loaded_vmcs);
 
-	if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
-		per_cpu(current_vmcs, cpu) = vmx->vmcs;
-		vmcs_load(vmx->vmcs);
+	if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
+		per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
+		vmcs_load(vmx->loaded_vmcs->vmcs);
  	}
 
-	if (vcpu->cpu != cpu) {
+	if (vmx->loaded_vmcs->cpu != cpu) {
  		struct desc_ptr *gdt =&__get_cpu_var(host_gdt);
  		unsigned long sysenter_esp;
 
  		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
  		local_irq_disable();
-		list_add(&vmx->local_vcpus_link,
-			&per_cpu(vcpus_on_cpu, cpu));
+		list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link,
+			&per_cpu(loaded_vmcss_on_cpu, cpu));
  		local_irq_enable();
 
  		/*

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

@@ -4344,11 +4369,13 @@ static struct kvm_vcpu *vmx_create_vcpu(
  		goto uninit_vcpu;
  	}
 
-	vmx->vmcs = alloc_vmcs();

(regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed)

Quoted 3 lines

(regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed)

To be fair this wasn't addressed for almost two years now.

On Mon, May 23, 2011, Gleb Natapov wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 5 lines · Avi Kivity

Quoted 3 lines

(regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed)

To be fair this wasn't addressed for almost two years now.

Gleb, I assume by "this" you meant the idt-vectoring information issue, not Kevin's comments (which I only saw a couple of days ago)?

Joerg, is

     if (unlikely(cpu != vcpu->cpu)) {
         svm->asid_generation = 0;
         mark_all_dirty(svm->vmcb);
     }

susceptible to cpu offline/online?

Quoted 7 lines · Avi Kivity

Joerg, is

      if (unlikely(cpu != vcpu->cpu)) {
          svm->asid_generation = 0;
          mark_all_dirty(svm->vmcb);
      }

Quoted 1 line

susceptible to cpu offline/online?

I don't think so. This should be safe for cpu offline/online as long as the cpu-number value is not reused for another physical cpu. But that should be the case afaik.

Quoted 11 lines · Joerg

Quoted 8 lines · Avi Kivity

Joerg, is

      if (unlikely(cpu != vcpu->cpu)) {
          svm->asid_generation = 0;
          mark_all_dirty(svm->vmcb);
      }

susceptible to cpu offline/online?

Quoted 4 lines

I don't think so. This should be safe for cpu offline/online as long as the cpu-number value is not reused for another physical cpu. But that should be the case afaik.

Why not? offline/online does reuse cpu numbers AFAIK (and it must, if you have a fully populated machine and offline/online just one cpu).

Probably we should make the local_vcpu_list from vmx generic, use it from svm and fix it this way.

On Tue, May 24, 2011, Roedel, Joerg wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 2 lines

Probably we should make the local_vcpu_list from vmx generic, use it from svm and fix it this way.

The point is, local_vcpu_list is now gone, replaced by a loaded_vmcss_on_cpu, and vcpu->cpu is not set to -1 for any vcpu when a CPU is offlined - also in VMX...

Quoted 7 lines · Nadav Har'El

On Tue, May 24, 2011, Roedel, Joerg wrote about "Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 2 lines

Probably we should make the local_vcpu_list from vmx generic, use it from svm and fix it this way.

The point is, local_vcpu_list is now gone, replaced by a loaded_vmcss_on_cpu, and vcpu->cpu is not set to -1 for any vcpu when a CPU is offlined - also in VMX...

loaded_vmcss_on_cpu sound similar, probably this can be generalized.

Is
this code already upstream or is this changed with your nVMX patch-set?

loaded_vmcss_on_cpu sound similar, probably this can be generalized.

Is this code already upstream or is this changed with your nVMX patch-set?

Review

Quoted 9 lines

(quoted diff) +2 -1

 	if (need_emulate_wbinvd(vcpu)) {
 		if (kvm_x86_ops->has_wbinvd_exit())
 			cpumask_set_cpu(cpu, vcpu->arch.wbinvd_dirty_mask);
-		else if (vcpu->cpu != -1&&  vcpu->cpu != cpu)
+		else if (vcpu->cpu != -1&&  vcpu->cpu != cpu
+				&&  cpu_online(vcpu->cpu))
 			smp_call_function_single(vcpu->cpu,
 					wbinvd_ipi, NULL, 1);
 	}

Is this a necessary part of this patch? Or an semi-related bugfix?

I think that it can't actually trigger before this patch due to luck. svm doesn't clear vcpu->cpu on cpu offline, but on the other hand it ->has_wbinvd_exit().

Quoted 12 lines

(quoted diff) +3 -2

+	if (vmx->loaded_vmcs->cpu != cpu) {
 		struct desc_ptr *gdt =&__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
 		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		local_irq_disable();
-		list_add(&vmx->local_vcpus_link,
-			&per_cpu(vcpus_on_cpu, cpu));
+		list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link,
+			&per_cpu(loaded_vmcss_on_cpu, cpu));
 		local_irq_enable();

Review static void vmx_vcpu_load(struct kvm_vcp · line 1024

Quoted 7 lines

(quoted diff) +1 -0

 		/*

@@ -999,13 +1020,15 @@ static void vmx_vcpu_load(struct kvm_vcp
 		rdmsrl(MSR_IA32_SYSENTER_ESP, sysenter_esp);
 		vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 
 		*/
 	}
+	vmx->loaded_vmcs->cpu = cpu;

This should be within the if () block.

Review static void vmx_vcpu_load(struct kvm_vcp · line 1027

Quoted 3 lines

(quoted diff) +3 -0

+	vmcs_init(vmx->loaded_vmcs->vmcs);
+	vmx->loaded_vmcs->cpu = -1;
+	vmx->loaded_vmcs->launched = 0;

Perhaps a loaded_vmcs_init() to encapsulate initialization of these three fields, you'll probably reuse it later.

Please repost separately after the fix, I'd like to apply it before the rest of the series.

(regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed)

From: Nadav Har'El
Sent: Tuesday, May 24, 2011 2:51 AM

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	vmcs_init(vmx->loaded_vmcs->vmcs);
+	vmx->loaded_vmcs->cpu = -1;
+	vmx->loaded_vmcs->launched = 0;

Perhaps a loaded_vmcs_init() to encapsulate initialization of these three fields, you'll probably reuse it later.

It's good you pointed this out, because it made me suddenly realise that I forgot to VMCLEAR the new vmcs02's I allocate. In practice it never made a difference, but better safe than sorry.

I had to restructure some of the code a bit to be able to properly use this new function (in 3 places - __loaded_vmcs_clear, nested_get_current_vmcs02, vmx_create_cpu).

Quoted 2 lines

Please repost separately after the fix, I'd like to apply it before the rest of the series.

I am adding a new version of this patch at the end of this mail.

Quoted 2 lines

(regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed)

I replied to his comments. Done some of the things he asked, and asked for more info on why/where he believes the current code is incorrect where I didn't understand what problems he pointed to, and am now waiting for him to reply.

------- 8< ------ 8< ---------- 8< ---------- 8< ----------- 8< -----------

Subject: [PATCH 01/31] nVMX: Keep list of loaded VMCSs, instead of vcpus.

In VMX, before we bring down a CPU we must VMCLEAR all VMCSs loaded on it because (at least in theory) the processor might not have written all of its content back to memory. Since a patch from June 26, 2008, this is done using a per-cpu "vcpus_on_cpu" linked list of vcpus loaded on each CPU.

The problem is that with nested VMX, we no longer have the concept of a vcpu being loaded on a cpu: A vcpu has multiple VMCSs (one for L1, a pool for L2s), and each of those may be have been last loaded on a different cpu.

So instead of linking the vcpus, we link the VMCSs, using a new structure loaded_vmcs. This structure contains the VMCS, and the information pertaining
to its loading on a specific cpu (namely, the cpu number, and whether it was already launched on this cpu once). In nested we will also use the same structure to hold L2 VMCSs, and vmx->loaded_vmcs is a pointer to the currently active VMCS.

Signed-off-by Nadav Har'El <[email protected]>

---
 arch/x86/kvm/vmx.c |  150 ++++++++++++++++++++++++-------------------
 1 file changed, 86 insertions(+), 64 deletions(-)

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

@@ -116,6 +116,18 @@ struct vmcs {
 	char data[0];
 };
 
+/*
+ * Track a VMCS that may be loaded on a certain CPU. If it is (cpu!=-1), also
+ * remember whether it was VMLAUNCHed, and maintain a linked list of all

VMCSs
+ * loaded on this CPU (so we can clear them if the CPU goes down).
+ */

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

+struct loaded_vmcs {
+	struct vmcs *vmcs;
+	int cpu;
+	int launched;
+	struct list_head loaded_vmcss_on_cpu_link;
+};
+
 struct shared_msr_entry {
 	unsigned index;
 	u64 data;

@@ -124,9 +136,7 @@ struct shared_msr_entry {
 
 struct vcpu_vmx {
 	struct kvm_vcpu       vcpu;
-	struct list_head      local_vcpus_link;
 	unsigned long         host_rsp;
-	int                   launched;
 	u8                    fail;
 	u8                    cpl;
 	bool                  nmi_known_unmasked;

@@ -140,7 +150,14 @@ struct vcpu_vmx {
 	u64 		      msr_host_kernel_gs_base;
 	u64 		      msr_guest_kernel_gs_base;
 #endif
-	struct vmcs          *vmcs;
+	/*
+	 * loaded_vmcs points to the VMCS currently used in this vcpu. For a
+	 * non-nested (L1) guest, it always points to vmcs01. For a nested
+	 * guest (L2), it points to a different VMCS.
+	 */
+	struct loaded_vmcs    vmcs01;
+	struct loaded_vmcs   *loaded_vmcs;
+	bool                  __launched; /* temporary, used in

vmx_vcpu_run */

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

 	struct msr_autoload {
 		unsigned nr;
 		struct vmx_msr_entry guest[NR_AUTOLOAD_MSRS];

@@ -200,7 +217,11 @@ static int vmx_set_tss_addr(struct kvm *
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
-static DEFINE_PER_CPU(struct list_head, vcpus_on_cpu);
+/*
+ * We maintain a per-CPU linked-list of VMCS loaded on that CPU. This is

needed
+ * when a CPU is brought down, and we need to VMCLEAR all VMCSs loaded on it.
+ */

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

+static DEFINE_PER_CPU(struct list_head, loaded_vmcss_on_cpu);
 static DEFINE_PER_CPU(struct desc_ptr, host_gdt);
 
 static unsigned long *vmx_io_bitmap_a;

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

 static void vmcs_load(struct vmcs *vmcs)
 {
 	u64 phys_addr = __pa(vmcs);

@@ -514,25 +542,24 @@ static void vmcs_load(struct vmcs *vmcs)
 		       vmcs, phys_addr);
 }
 
-static void __vcpu_clear(void *arg)
+static void __loaded_vmcs_clear(void *arg)

arch/x86/kvm/vmx.c +27 -23

+	if (per_cpu(current_vmcs, cpu) == loaded_vmcs->vmcs)
 		per_cpu(current_vmcs, cpu) = NULL;
-	list_del(&vmx->local_vcpus_link);
-	vmx->vcpu.cpu = -1;
-	vmx->launched = 0;
+	list_del(&loaded_vmcs->loaded_vmcss_on_cpu_link);
+	loaded_vmcs_init(loaded_vmcs);
 }
 
-static void vcpu_clear(struct vcpu_vmx *vmx)
+static void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs)
 {
-	if (vmx->vcpu.cpu == -1)
-		return;
-	smp_call_function_single(vmx->vcpu.cpu, __vcpu_clear, vmx, 1);
+	if (loaded_vmcs->cpu != -1)
+		smp_call_function_single(
+			loaded_vmcs->cpu, __loaded_vmcs_clear, loaded_vmcs, 1);
 }
 
 static inline void vpid_sync_vcpu_single(struct vcpu_vmx *vmx)

@@ -971,22 +998,22 @@ static void vmx_vcpu_load(struct kvm_vcp
 
 	if (!vmm_exclusive)
 		kvm_cpu_vmxon(phys_addr);
-	else if (vcpu->cpu != cpu)
-		vcpu_clear(vmx);
+	else if (vmx->loaded_vmcs->cpu != cpu)
+		loaded_vmcs_clear(vmx->loaded_vmcs);
 
-	if (per_cpu(current_vmcs, cpu) != vmx->vmcs) {
-		per_cpu(current_vmcs, cpu) = vmx->vmcs;
-		vmcs_load(vmx->vmcs);
+	if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
+		per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
+		vmcs_load(vmx->loaded_vmcs->vmcs);
 	}
 
-	if (vcpu->cpu != cpu) {
+	if (vmx->loaded_vmcs->cpu != cpu) {
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
 		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		local_irq_disable();
-		list_add(&vmx->local_vcpus_link,
-			 &per_cpu(vcpus_on_cpu, cpu));
+		list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link,
+			 &per_cpu(loaded_vmcss_on_cpu, cpu));
 		local_irq_enable();
 
 		/*

@@ -998,6 +1025,7 @@ static void vmx_vcpu_load(struct kvm_vcp
 
 		rdmsrl(MSR_IA32_SYSENTER_ESP, sysenter_esp);
 		vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */
+		vmx->loaded_vmcs->cpu = cpu;
 	}
 }
 

@@ -1005,7 +1033,8 @@ static void vmx_vcpu_put(struct kvm_vcpu
 {
 	__vmx_load_host_state(to_vmx(vcpu));
 	if (!vmm_exclusive) {
-		__vcpu_clear(to_vmx(vcpu));
+		__loaded_vmcs_clear(to_vmx(vcpu)->loaded_vmcs);
+		vcpu->cpu = -1;
 		kvm_cpu_vmxoff();
 	}
 }

@@ -1469,7 +1498,7 @@ static int hardware_enable(void *garbage
 	if (read_cr4() & X86_CR4_VMXE)
 		return -EBUSY;
 
-	INIT_LIST_HEAD(&per_cpu(vcpus_on_cpu, cpu));
+	INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu));
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
 
 	test_bits = FEATURE_CONTROL_LOCKED;

@@ -1493,14 +1522,14 @@ static int hardware_enable(void *garbage
 	return 0;
 }
 
-static void vmclear_local_vcpus(void)
+static void vmclear_local_loaded_vmcss(void)
 {
 	int cpu = raw_smp_processor_id();
-	struct vcpu_vmx *vmx, *n;
+	struct loaded_vmcs *v, *n;
 
-	list_for_each_entry_safe(vmx, n, &per_cpu(vcpus_on_cpu, cpu),
-				 local_vcpus_link)
-		__vcpu_clear(vmx);
+	list_for_each_entry_safe(v, n, &per_cpu(loaded_vmcss_on_cpu, cpu),
+				 loaded_vmcss_on_cpu_link)
+		__loaded_vmcs_clear(v);
 }
 
 

@@ -1515,7 +1544,7 @@ static void kvm_cpu_vmxoff(void)
 static void hardware_disable(void *garbage)
 {
 	if (vmm_exclusive) {
-		vmclear_local_vcpus();
+		vmclear_local_loaded_vmcss();
 		kvm_cpu_vmxoff();
 	}
 	write_cr4(read_cr4() & ~X86_CR4_VMXE);

@@ -1696,6 +1725,18 @@ static void free_vmcs(struct vmcs *vmcs)
 	free_pages((unsigned long)vmcs, vmcs_config.order);
 }
 
+/*
+ * Free a VMCS, but before that VMCLEAR it on the CPU where it was last

loaded
+ */

arch/x86/kvm/vmx.c static void vmcs_clear(struct vmcs *vmcs · line 531

Quoted 7 lines

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

+static inline void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs)
+{
+	vmcs_clear(loaded_vmcs->vmcs);
+	loaded_vmcs->cpu = -1;
+	loaded_vmcs->launched = 0;
+}
+

call it vmcs_init instead since you now remove original vmcs_init invocation, which more reflect the necessity of adding VMCLEAR for a new vmcs?

Quoted 6 lines

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

+static void __loaded_vmcs_clear(void *arg)
 {
-	struct vcpu_vmx *vmx = arg;
+	struct loaded_vmcs *loaded_vmcs = arg;
 	int cpu = raw_smp_processor_id();

arch/x86/kvm/vmx.c static void vmcs_load(struct vmcs *vmcs) · line 551

Quoted 5 lines

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

-	if (vmx->vcpu.cpu == cpu)
-		vmcs_clear(vmx->vmcs);
-	if (per_cpu(current_vmcs, cpu) == vmx->vmcs)
+	if (loaded_vmcs->cpu != cpu)
+		return; /* cpu migration can race with cpu offline */

what do you mean by "cpu migration" here? why does 'cpu offline' matter here regarding to the cpu change?

Quoted 7 lines

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

+static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
+{
+	if (!loaded_vmcs->vmcs)
+		return;
+	loaded_vmcs_clear(loaded_vmcs);
+	free_vmcs(loaded_vmcs->vmcs);
+	loaded_vmcs->vmcs = NULL;

Quoted 1 line

+}

prefer to not do cleanup work through loaded_vmcs since it's just a pointer to a loaded vmcs structure. Though you can carefully arrange the nested vmcs cleanup happening before it, it's not very clean and a bit error prone simply from this function itself. It's clearer to directly cleanup vmcs01, and if you want an assertion could be added to make sure loaded_vmcs doesn't point to any stale vmcs02 structure after nested cleanup step.

From: Nadav Har'El [mailto:[email protected]]
Sent: Tuesday, May 24, 2011 3:57 PM

On Tue, May 24, 2011, Tian, Kevin wrote about "RE: [PATCH 08/31] nVMX: Fix local_vcpus_link handling":

Quoted 10 lines

arch/x86/kvm/vmx.c static void vmcs_clear(struct vmcs *vmcs · line 531

Quoted 7 lines

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

+static inline void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs)
+{
+	vmcs_clear(loaded_vmcs->vmcs);
+	loaded_vmcs->cpu = -1;
+	loaded_vmcs->launched = 0;
+}
+

call it vmcs_init instead since you now remove original vmcs_init invocation, which more reflect the necessity of adding VMCLEAR for a new vmcs?

The best name for this function, I think, would have been loaded_vmcs_clear, because this function isn't necessarily used to "init" - it's also called to VMCLEAR an old vmcs (and flush its content back to memory) - in that sense it is definitely not a "vmcs_init".

Unfortunately, I already have a whole chain of functions with this name :( the existing loaded_vmcs_clear() does an IPI to the CPU which has this VMCS loaded, and causes it to run __loaded_vmcs_clear(), which in turn calls the above loaded_vmcs_init(). I wish I could call all three functions loaded_vmcs_clear(), but of course I can't. If anyone reading this has a good suggestion on how to name these three functions, please let me know.

Quoted 7 lines

Quoted 7 lines

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

+static void __loaded_vmcs_clear(void *arg)
 {
-	struct vcpu_vmx *vmx = arg;
+	struct loaded_vmcs *loaded_vmcs = arg;
 	int cpu = raw_smp_processor_id();

Quoted 8 lines

arch/x86/kvm/vmx.c static void vmcs_load(struct vmcs *vmcs) · line 551

Quoted 5 lines

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

-	if (vmx->vcpu.cpu == cpu)
-		vmcs_clear(vmx->vmcs);
-	if (per_cpu(current_vmcs, cpu) == vmx->vmcs)
+	if (loaded_vmcs->cpu != cpu)
+		return; /* cpu migration can race with cpu offline */

what do you mean by "cpu migration" here? why does 'cpu offline' matter here regarding to the cpu change?

__loaded_vmcs_clear() is typically called in one of two cases: "cpu migration" means that a guest that used to run on one CPU, and had its VMCS loaded there,
suddenly needs to run on a different CPU, so we need to clear the VMCS on the old CPU. "cpu offline" means that we want to take a certain CPU offline, and before we do that we should VMCLEAR all VMCSs which were loaded on it.

The (vmx->cpu.cpu != cpu) case in __loaded_vmcs_clear should ideally never
happen: In the cpu offline path, we only call it for the loaded_vmcss which we know for sure are loaded on the current cpu. In the cpu migration path, loaded_vmcs_clear runs __loaded_vmcs_clear on the right CPU, which ensures that
equality.

But, there can be a race condition (this was actually explained to me a while back by Avi - I never seen this happening in practice): Imagine that cpu migration calls loaded_vmcs_clear, which tells the old cpu (via IPI) to VMCLEAR this vmcs. But before that old CPU gets a chance to act on that IPI, a decision is made to take it offline, and all loaded_vmcs loaded on it (including the one in question) are cleared. When that CPU acts on this IPI, it notices that vmx->cpu.cpu==-1, i.e., != cpu, so it doesn't need to do anything (in the new version of the code, I made this more explicit, by returning immediately in this case).

At least this is the theory. As I said, I didn't see this problem in practice (unsurprising, since I never offlined any CPU). Maybe Avi or someone else can comment more about this (vmx->cpu.cpu == cpu) check, which existed before my patch - in __vcpu_clear().

Quoted 16 lines

arch/x86/kvm/vmx.c static void free_vmcs(struct vmcs *vmcs) · line 1738

Quoted 9 lines

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

+static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
+{
+	if (!loaded_vmcs->vmcs)
+		return;
+	loaded_vmcs_clear(loaded_vmcs);
+	free_vmcs(loaded_vmcs->vmcs);
+	loaded_vmcs->vmcs = NULL;
+}

prefer to not do cleanup work through loaded_vmcs since it's just a pointer to a loaded vmcs structure. Though you can carefully arrange the nested vmcs cleanup happening before it, it's not very clean and a bit error prone simply from this function itself. It's clearer to directly cleanup vmcs01, and if you want an assertion could be added to make sure loaded_vmcs doesn't point to any stale vmcs02 structure after nested cleanup step.

I'm afraid I didn't understand what you meant here... Basically, this free_loaded_vmcs() is just a shortcut for loaded_vmcs_clear() and free_vmcs(), as doing both is needed in 3 places: nested_free_vmcs02, nested_free_all_saved_vmcss, vmx_free_vcpu. The same function is needed for both vmcs01 and vmcs02 VMCSs - in both cases when we don't need them any
more we need to VMCLEAR them and then free the VMCS memory. Note that this
function does *not* free the loaded_vmcs structure itself.

What's wrong with this?
Would you prefer that I remove this function and explictly call loaded_vmcs_clear() and then free_vmcs() in all three places?