KVM: nVMX: shadow VMCS support, v1

Add new counters to measure how many vmread/vmwrite/vmlaunch/vmresume/vmclear instructions were trapped and emulated by L0

Signed-off-by Abel Gordon <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    6 ++++++
 arch/x86/kvm/vmx.c              |    7 +++++++
 arch/x86/kvm/x86.c              |    6 ++++++
 3 files changed, 19 insertions(+)

arch/x86/include/asm/kvm_host.h +6 -0

@@ -619,6 +619,12 @@ struct kvm_vcpu_stat {
 	u32 hypercalls;
 	u32 irq_injections;
 	u32 nmi_injections;
+	u32 nvmx_vmreads;
+	u32 nvmx_vmwrites;
+	u32 nvmx_vmptrlds;
+	u32 nvmx_vmlaunchs;
+	u32 nvmx_vmresumes;
+	u32 nvmx_vmclears;
 };
 
 struct x86_instruction_info;

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

@@ -5545,6 +5545,7 @@ static int handle_vmclear(struct kvm_vcp
 	struct vmcs12 *vmcs12;
 	struct page *page;
 	struct x86_exception e;
+	++vcpu->stat.nvmx_vmclears;
 
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;

@@ -5601,12 +5602,14 @@ static int nested_vmx_run(struct kvm_vcp
 /* Emulate the VMLAUNCH instruction */
 static int handle_vmlaunch(struct kvm_vcpu *vcpu)
 {
+	++vcpu->stat.nvmx_vmlaunchs;
 	return nested_vmx_run(vcpu, true);
 }
 
 /* Emulate the VMRESUME instruction */
 static int handle_vmresume(struct kvm_vcpu *vcpu)
 {
+	++vcpu->stat.nvmx_vmresumes;
 
 	return nested_vmx_run(vcpu, false);
 }

@@ -5689,6 +5692,7 @@ static int handle_vmread(struct kvm_vcpu
 	u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
 	gva_t gva = 0;
 
+	++vcpu->stat.nvmx_vmreads;
 	if (!nested_vmx_check_permission(vcpu) ||
 	    !nested_vmx_check_vmcs12(vcpu))
 		return 1;

@@ -5741,6 +5745,8 @@ static int handle_vmwrite(struct kvm_vcp
 	u64 field_value = 0;
 	struct x86_exception e;
 
+	++vcpu->stat.nvmx_vmwrites;
+
 	if (!nested_vmx_check_permission(vcpu) ||
 	    !nested_vmx_check_vmcs12(vcpu))
 		return 1;

@@ -5807,6 +5813,7 @@ static int handle_vmptrld(struct kvm_vcp
 	gva_t gva;
 	gpa_t vmptr;
 	struct x86_exception e;
+	++vcpu->stat.nvmx_vmptrlds;
 
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;

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

@@ -145,6 +145,12 @@ struct kvm_stats_debugfs_item debugfs_en
 	{ "insn_emulation_fail", VCPU_STAT(insn_emulation_fail) },
 	{ "irq_injections", VCPU_STAT(irq_injections) },
 	{ "nmi_injections", VCPU_STAT(nmi_injections) },
+	{ "nvmx_vmreads", VCPU_STAT(nvmx_vmreads) },
+	{ "nvmx_vmwrites", VCPU_STAT(nvmx_vmwrites) },
+	{ "nvmx_vmptrlds", VCPU_STAT(nvmx_vmptrlds) },
+	{ "nvmx_vmlaunchs", VCPU_STAT(nvmx_vmlaunchs) },
+	{ "nvmx_vmresumes", VCPU_STAT(nvmx_vmresumes) },
+	{ "nvmx_vmclears", VCPU_STAT(nvmx_vmclears) },
 	{ "mmu_shadow_zapped", VM_STAT(mmu_shadow_zapped) },
 	{ "mmu_pte_write", VM_STAT(mmu_pte_write) },
 	{ "mmu_pte_updated", VM_STAT(mmu_pte_updated) },
 
--

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

stat counters are deprecated in favor of trace points. Adding kvmnested trace system is very welcome though.

Gleb Natapov <[email protected]> wrote on 08/04/2013 01:27:28 PM:

Quoted 2 lines

stat counters are deprecated in favor of trace points. Adding kvmnested trace system is very welcome though.

So, should I keep or remove this patch ?

If a kvmnested trace system is added then I'll add shadow-vmcs related events

Add logic required to detect if shadow-vmcs is supported by the processor. Introduce a new kernel module parameter to specify if L0 should use shadow vmcs (or not) to run L1.

Signed-off-by Abel Gordon <[email protected]>

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

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

@@ -86,6 +86,8 @@ module_param(fasteoi, bool, S_IRUGO);
 
 static bool __read_mostly enable_apicv_reg_vid;
 
+static bool __read_mostly enable_shadow_vmcs = 1;
+module_param_named(enable_shadow_vmcs, enable_shadow_vmcs, bool, S_IRUGO);
 /*
  * If nested=1, nested virtualization is supported, i.e., guests may use
  * VMX and be a hypervisor for its own guests. If nested=0, guests may not

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

+		return false;
+
+	return vmcs_config.cpu_based_2nd_exec_ctrl &
+		SECONDARY_EXEC_SHADOW_VMCS;
+}
+
 static inline bool report_flexpriority(void)
 {
 	return flexpriority_enabled;

@@ -2582,7 +2596,8 @@ static __init int setup_vmcs_config(stru
 			SECONDARY_EXEC_RDTSCP |
 			SECONDARY_EXEC_ENABLE_INVPCID |
 			SECONDARY_EXEC_APIC_REGISTER_VIRT |
-			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
+			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
+			SECONDARY_EXEC_SHADOW_VMCS;
 		if (adjust_vmx_controls(min2, opt2,
 					MSR_IA32_VMX_PROCBASED_CTLS2,
 					&_cpu_based_2nd_exec_control) < 0)

@@ -2771,6 +2786,8 @@ static __init int hardware_setup(void)
 
 	if (!cpu_has_vmx_vpid())
 		enable_vpid = 0;
+	if (!cpu_has_vmx_shadow_vmcs())
+		enable_shadow_vmcs = 0;
 
 	if (!cpu_has_vmx_ept() ||
 	    !cpu_has_vmx_ept_4levels()) {

@@ -3982,6 +3999,12 @@ static u32 vmx_secondary_exec_control(st
 		exec_control &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
 				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
 	exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
+	/* SECONDARY_EXEC_SHADOW_VMCS is enabled when L1 executes VMPTRLD
+	   (handle_vmptrld).
+	   We can NOT enable shadow_vmcs here because we don't have yet
+	   a current VMCS12
+	*/
+	exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
 	return exec_control;
 }
 
 
--

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

arch/x86/kvm/vmx.c static inline bool cpu_has_vmx_wbinvd_ex · line 905

Quoted 6 lines

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

+static inline bool cpu_has_vmx_shadow_vmcs(void)
+{
+   u64 vmx_msr;
+   rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
+   /* check if the cpu supports writing r/o exit information fields */
+   if (!(vmx_msr & (1u << 29)))

Define please.

Prepare vmread and vmwrite bitmaps according to a pre-specified list of fields. These lists are intended to specifiy most frequent accessed fields so we can minimize the number of fields that are copied from/to the software controlled VMCS12 format to/from to processor-specific shadow vmcs. The lists were built measuring the VMCS fields access rate after L2 Ubuntu 12.04 booted when it was running on top of L1 KVM, also Ubuntu 12.04. Note that during boot there were additional fields which were frequently modified but they were not added to these lists because after boot these fields were not longer accessed by L1.

Signed-off-by Abel Gordon <[email protected]>

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

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

@@ -453,6 +453,51 @@ static inline struct vcpu_vmx *to_vmx(st
 #define FIELD64(number, name)	[number] = VMCS12_OFFSET(name), \
 				[number##_HIGH] = VMCS12_OFFSET(name)+4
 
+
+static const unsigned long shadow_read_only_fields[] = {
+	VM_EXIT_REASON,
+	VM_EXIT_INTR_INFO,
+	VM_EXIT_INSTRUCTION_LEN,
+	IDT_VECTORING_INFO_FIELD,
+	IDT_VECTORING_ERROR_CODE,
+	VM_EXIT_INTR_ERROR_CODE,
+	EXIT_QUALIFICATION,
+	GUEST_LINEAR_ADDRESS,
+	GUEST_PHYSICAL_ADDRESS
+};
+static const int max_shadow_read_only_fields = ARRAY_SIZE(shadow_read_only_fields);
+
+static const unsigned long shadow_read_write_fields[] = {
+	GUEST_RIP,
+	GUEST_RSP,
+	GUEST_CR0,
+	GUEST_CR3,
+	GUEST_CR4,
+	GUEST_INTERRUPTIBILITY_INFO,
+	GUEST_RFLAGS,
+	GUEST_CS_SELECTOR,
+	GUEST_CS_AR_BYTES,
+	GUEST_CS_LIMIT,
+	GUEST_CS_BASE,
+	GUEST_ES_BASE,
+	CR0_GUEST_HOST_MASK,
+	CR0_READ_SHADOW,
+	CR4_READ_SHADOW,
+	TSC_OFFSET,
+	EXCEPTION_BITMAP,
+	CPU_BASED_VM_EXEC_CONTROL,
+	VM_ENTRY_EXCEPTION_ERROR_CODE,
+	VM_ENTRY_INTR_INFO_FIELD,
+	VM_ENTRY_INSTRUCTION_LEN,
+	VM_ENTRY_EXCEPTION_ERROR_CODE,
+	HOST_FS_BASE,
+	HOST_GS_BASE,
+	HOST_FS_SELECTOR,
+	HOST_GS_SELECTOR
+};
+static const int max_shadow_read_write_fields =
+	ARRAY_SIZE(shadow_read_write_fields);
+
 static const unsigned short vmcs_field_to_offset_table[] = {
 	FIELD(VIRTUAL_PROCESSOR_ID, virtual_processor_id),
 	FIELD(GUEST_ES_SELECTOR, guest_es_selector),

@@ -642,6 +687,8 @@ static unsigned long *vmx_msr_bitmap_leg
 static unsigned long *vmx_msr_bitmap_longmode;
 static unsigned long *vmx_msr_bitmap_legacy_x2apic;
 static unsigned long *vmx_msr_bitmap_longmode_x2apic;
+static unsigned long *vmx_vmread_bitmap;
+static unsigned long *vmx_vmwrite_bitmap;
 
 static bool cpu_has_load_ia32_efer;
 static bool cpu_has_load_perf_global_ctrl;

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

 	if (cpu_has_vmx_msr_bitmap())
 		vmcs_write64(MSR_BITMAP, __pa(vmx_msr_bitmap_legacy));
 

@@ -7764,6 +7813,24 @@ static int __init vmx_init(void)
 				(unsigned long *)__get_free_page(GFP_KERNEL);

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

+	memset(vmx_vmread_bitmap, 0xff, PAGE_SIZE);
+	memset(vmx_vmwrite_bitmap, 0xff, PAGE_SIZE);
+	/* shadowed read/write fields */
+	for (i = 0; i < max_shadow_read_write_fields; i++) {
+		clear_bit(shadow_read_write_fields[i], vmx_vmwrite_bitmap);
+		clear_bit(shadow_read_write_fields[i], vmx_vmread_bitmap);
+	}
+	/* shadowed read only fields */
+	for (i = 0; i < max_shadow_read_only_fields; i++)
+		clear_bit(shadow_read_only_fields[i], vmx_vmread_bitmap);
 
 	/*
 	 * Allow direct access to the PC debug port (it is often used for I/O

@@ -7782,7 +7849,7 @@ static int __init vmx_init(void)
 	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
 		     __alignof__(struct vcpu_vmx), THIS_MODULE);
 	if (r)
-		goto out3;
+		goto out6;
 
 #ifdef CONFIG_KEXEC
 	rcu_assign_pointer(crash_vmclear_loaded_vmcss,

@@ -7830,6 +7897,10 @@ static int __init vmx_init(void)
 
 	return 0;
 
+out6:
+	free_page((unsigned long)vmx_vmwrite_bitmap);
+out5:
+	free_page((unsigned long)vmx_vmread_bitmap);
 out4:
 	free_page((unsigned long)vmx_msr_bitmap_longmode);
 out3:

@@ -7851,6 +7922,8 @@ static void __exit vmx_exit(void)
 	free_page((unsigned long)vmx_msr_bitmap_longmode);
 	free_page((unsigned long)vmx_io_bitmap_b);
 	free_page((unsigned long)vmx_io_bitmap_a);
+	free_page((unsigned long)vmx_vmwrite_bitmap);
+	free_page((unsigned long)vmx_vmread_bitmap);
 
 #ifdef CONFIG_KEXEC
 	rcu_assign_pointer(crash_vmclear_loaded_vmcss, NULL);
 
--

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

arch/x86/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vm · line 4084

Quoted 8 lines

static bool cpu_has_load_ia32_efer;
static bool cpu_has_load_perf_global_ctrl;

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

@@ -4033,6 +4080,8 @@ static int vmx_vcpu_setup(struct vcpu_vm
    vmcs_write64(IO_BITMAP_A, __pa(vmx_io_bitmap_a));
    vmcs_write64(IO_BITMAP_B, __pa(vmx_io_bitmap_b));
 
+   vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
+   vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap));

Why are you doing it without checking that shadow vmcs is supported and enabled?

Refactor existent code so we re-use vmcs12_write_any to copy fields from the shadow vmcs specified by the link pointer (used by the processor, implementation-specific) to the VMCS12 software format used by L0 to hold the fields in L1 memory address space.

Signed-off-by Abel Gordon <[email protected]>

---
 arch/x86/kvm/vmx.c |   52 +++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

arch/x86/kvm/vmx.c +21 -25

+
+	switch (vmcs_field_type(field)) {
+	case VMCS_FIELD_TYPE_U16:
+		*(u16 *)p = field_value;
+		return 1;
+	case VMCS_FIELD_TYPE_U32:
+		*(u32 *)p = field_value;
+		return 1;
+	case VMCS_FIELD_TYPE_U64:
+		*(u64 *)p = field_value;
+		return 1;
+	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
+		*(natural_width *)p = field_value;
+		return 1;
+	default:
+		return 0; /* can never happen. */
+	}
+
+}
+
 /*
  * VMX instructions which assume a current vmcs12 (i.e., that VMPTRLD was
  * used before) all generate the same failure when it is missing.

@@ -5806,8 +5833,6 @@ static int handle_vmwrite(struct kvm_vcp
 	gva_t gva;
 	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
 	u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	char *p;
-	short offset;
 	/* The value to write might be 32 or 64 bits, depending on L1's long
 	 * mode, and eventually we need to write that into a field of several
 	 * possible lengths. The code below first zero-extends the value to 64

@@ -5846,28 +5871,7 @@ static int handle_vmwrite(struct kvm_vcp
 		return 1;
 	}
 
-	offset = vmcs_field_to_offset(field);
-	if (offset < 0) {
-		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
-		skip_emulated_instruction(vcpu);
-		return 1;
-	}
-	p = ((char *) get_vmcs12(vcpu)) + offset;
-
-	switch (vmcs_field_type(field)) {
-	case VMCS_FIELD_TYPE_U16:
-		*(u16 *)p = field_value;
-		break;
-	case VMCS_FIELD_TYPE_U32:
-		*(u32 *)p = field_value;
-		break;
-	case VMCS_FIELD_TYPE_U64:
-		*(u64 *)p = field_value;
-		break;
-	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
-		*(natural_width *)p = field_value;
-		break;
-	default:
+	if (!vmcs12_write_any(vcpu, field, field_value)) {
 		nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
 		skip_emulated_instruction(vcpu);
 		return 1;
 
--

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

arch/x86/kvm/vmx.c static inline bool vmcs12_read_any(struc · line 5750

Quoted 6 lines

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

+static inline bool vmcs12_write_any(struct kvm_vcpu *vcpu,
+                unsigned long field, u64 field_value){
+   short offset = vmcs_field_to_offset(field);
+   char *p = ((char *) get_vmcs12(vcpu)) + offset;
+   if (offset < 0)
+      return 0;

The function returns bool, so use true/false please.

Introduce a function used to copy fields from the software controlled VMCS12 to the processor-specific shadow vmcs

Signed-off-by Abel Gordon <[email protected]>

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

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

@@ -672,6 +672,7 @@ static void vmx_get_segment(struct kvm_v
 			    struct kvm_segment *var, int seg);
 static bool guest_state_valid(struct kvm_vcpu *vcpu);
 static u32 vmx_segment_access_rights(struct kvm_segment *var);
+static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx);
 static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);

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

+	unsigned long *fields[] = {
+		(unsigned long *)shadow_read_write_fields,
+		(unsigned long *)shadow_read_only_fields
+	};
+	int max_fields[] = {
+		max_shadow_read_write_fields,
+		max_shadow_read_only_fields
+	};
+	int i, q;
+	unsigned long field;
+	u64 field_value = 0;
+	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
+
+	vmcs_load(shadow_vmcs);
+
+	for (q = 0; q < num_lists; q++) {
+		for (i = 0; i < max_fields[q]; i++) {
+			field = fields[q][i];
+			vmcs12_read_any(&vmx->vcpu, field, &field_value);
+
+			switch (vmcs_field_type(field)) {
+			case VMCS_FIELD_TYPE_U16:
+				vmcs_write16(field, (u16)field_value);
+				break;
+			case VMCS_FIELD_TYPE_U32:
+				vmcs_write32(field, (u32)field_value);
+				break;
+			case VMCS_FIELD_TYPE_U64:
+				vmcs_write64(field, (u64)field_value);
+				break;
+			case VMCS_FIELD_TYPE_NATURAL_WIDTH:
+				vmcs_writel(field, (long)field_value);
+				break;
+			}
+		}
+	}
+
+	vmcs_clear(shadow_vmcs);
+	vmcs_load(vmx->loaded_vmcs->vmcs);
+}
+
 /*
  * VMX instructions which assume a current vmcs12 (i.e., that VMPTRLD was
  * used before) all generate the same failure when it is missing.
 
--

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

arch/x86/kvm/vmx.c static void copy_shadow_to_vmcs12(struct · line 5819

Quoted 4 lines

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

+static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
+{
+   int num_lists = 2;

int num_lists = ARRAY_SIZE(fields)

On Sun, Mar 10, 2013, Abel Gordon wrote about "[PATCH 10/11] KVM:
nVMX: Synchronize VMCS12 content with the shadow vmcs":

Review

Quoted 3 lines

    nested_vmx_vmexit(vcpu);

(quoted diff) +2 -0

+   if (enable_shadow_vmcs)
+      copy_vmcs12_to_shadow(to_vmx(vcpu));

I was curious why your patch adds this call to copy_vmcs12_to_shadow

every nested_vmx_vmexit (3 times), instead of making this call inside nested_vmx_vmexit(), say right after prepare_vmcs12(). Until I saw:

Review

Quoted 6 lines

       nested_vmx_vmexit(vcpu);
       vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT;
       vmcs12->vm_exit_intr_info = 0;

(quoted diff) +2 -0

+      if (enable_shadow_vmcs)
+         copy_vmcs12_to_shadow(to_vmx(vcpu));

where you need to copy this exit-reason override as well...

I wonder if there isn't a less ugly and repetitive way to do this :(

"Nadav Har'El" <[email protected]> wrote on 11/03/2013 12:43:35 AM:

Quoted 7 lines

On Sun, Mar 10, 2013, Abel Gordon wrote about "[PATCH 10/11] KVM:
nVMX: Synchronize VMCS12 content with the shadow vmcs":

Review

Quoted 3 lines

    nested_vmx_vmexit(vcpu);

(quoted diff) +2 -0

+   if (enable_shadow_vmcs)
+      copy_vmcs12_to_shadow(to_vmx(vcpu));

I was curious why your patch adds this call to copy_vmcs12_to_shadow

after

Quoted 2 lines

every nested_vmx_vmexit (3 times), instead of making this call inside nested_vmx_vmexit(), say right after prepare_vmcs12(). Until I saw:

Because nested code sometimes modifies vmcs fileds "after" nested_vmx_vmexit (see below). I was afraid nested logic may be changed in the future and some field may become out-of-sync.

If we do have to call copy_vmcs12_to_shadow explicitly, then, it will be more difficult to miss some field.

Quoted 1 line

"Nadav Har'El" <[email protected]> wrote on 11/03/2013 12:43:35

Quoted 13 lines

Quoted 9 lines

On Sun, Mar 10, 2013, Abel Gordon wrote about "[PATCH 10/11] KVM:
nVMX: Synchronize VMCS12 content with the shadow vmcs":

Review

Quoted 3 lines

    nested_vmx_vmexit(vcpu);

(quoted diff) +2 -0

+   if (enable_shadow_vmcs)
+      copy_vmcs12_to_shadow(to_vmx(vcpu));

I was curious why your patch adds this call to copy_vmcs12_to_shadow

after

Quoted 2 lines

every nested_vmx_vmexit (3 times), instead of making this call inside nested_vmx_vmexit(), say right after prepare_vmcs12(). Until I saw:

Quoted 5 lines

Because nested code sometimes modifies vmcs fileds "after" nested_vmx_vmexit (see below). I was afraid nested logic may be changed in the future and some field may become out-of-sync.

If we do have to call copy_vmcs12_to_shadow explicitly, then, it will

Quoted 2 lines

more difficult to miss some field.

I think the patch already miss some fields. What if nested_vmx_run() fails and calls nested_vmx_entry_failure(). nested_vmx_entry_failure() sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but where do we copy them back to shadow before going back to L1?

May be we need to introduce vmcs12 accessors to track what is changes and if something need to be copied to shadow before going back to L1.

Gleb Natapov <[email protected]> wrote on 09/04/2013 04:14:35 PM:

Quoted 2 lines · Abel Gordon

Quoted 1 line

"Nadav Har'El" <[email protected]> wrote on 11/03/2013 12:43:35

AM:

Quoted 14 lines

Quoted 13 lines

Quoted 9 lines

On Sun, Mar 10, 2013, Abel Gordon wrote about "[PATCH 10/11] KVM:
nVMX: Synchronize VMCS12 content with the shadow vmcs":

Review

Quoted 3 lines

    nested_vmx_vmexit(vcpu);

(quoted diff) +2 -0

+   if (enable_shadow_vmcs)
+      copy_vmcs12_to_shadow(to_vmx(vcpu));

I was curious why your patch adds this call to copy_vmcs12_to_shadow

after

Quoted 2 lines

every nested_vmx_vmexit (3 times), instead of making this call inside nested_vmx_vmexit(), say right after prepare_vmcs12(). Until I saw:

Quoted 5 lines

Quoted 5 lines

Because nested code sometimes modifies vmcs fileds "after" nested_vmx_vmexit (see below). I was afraid nested logic may be changed in the future and some field may become out-of-sync.

If we do have to call copy_vmcs12_to_shadow explicitly, then, it will

be

Quoted 8 lines

Quoted 2 lines

more difficult to miss some field.

I think the patch already miss some fields. What if nested_vmx_run() fails and calls nested_vmx_entry_failure(). nested_vmx_entry_failure() sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but where do we copy them back to shadow before going back to L1?

Good catch! :)

Note that the entry path is easy to handle because we copy the fields as part of nested_vmx_entry. This is not like exit path where KVM(L0) code can modify fields after nested_vmx_vmexit is called.

So here, we could simple call copy_vmcs12_to_shadow if the entry fails (as part of nested_vmx_entry_failure or nested_vmx). We could optimize the code by updating these specific fields directly, but I don't think we really need to optimize code that is part of the error path.

Quoted 2 lines

May be we need to introduce vmcs12 accessors to track what is changes and if something need to be copied to shadow before going back to L1.

That means we will need to modify all the lines of code that uses "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write function. Inside these inline functions we could access the shadow vmcs directly. However, to access the shadow vmcs we need to vmptrld first and this will force
unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01) each time the code accesses a vmcs12 field. Alternatively, if we want to avoid unnecessary vmptrlds each time we access vmcs12 we could simple set a flag that indicates when a shadow field was changed. In this case, we will need to find all the places to check the flag and copy the fields, considering both success and error paths.

Finally, I am afraid that these directions will introduce new issues, will force us to modify too many lines and they may create a merge/rebase mess...

Maybe we should simple fix nested_vmx_entry_failure (as well as the other fixes you suggested in other patches) and apply the code. Do you agree ?

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 09/04/2013 04:14:35 PM:

Quoted 2 lines

I think the patch already miss some fields. What if nested_vmx_run() fails and calls nested_vmx_entry_failure(). nested_vmx_entry_failure

Quoted 1 line

Quoted 1 line

sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but where

Quoted 8 lines

Quoted 1 line

we copy them back to shadow before going back to L1?

Good catch! :)

Note that the entry path is easy to handle because we copy the fields as part of nested_vmx_entry. This is not like exit path where KVM(L0) code can modify fields after nested_vmx_vmexit is called.

Quoted 5 lines

So here, we could simple call copy_vmcs12_to_shadow if the entry fails (as part of nested_vmx_entry_failure or nested_vmx). We could optimize the code by updating these specific fields directly, but I don't think we really need to optimize code that is part of the error path.

We needn't.

Quoted 5 lines

Quoted 2 lines

May be we need to introduce vmcs12 accessors to track what is changes and if something need to be copied to shadow before going back to L1.

That means we will need to modify all the lines of code that uses "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write

Quoted 2 lines

Inside these inline functions we could access the shadow vmcs directly. However, to access the shadow vmcs we need to vmptrld first and this

Quoted 2 lines

force
unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01) each

Quoted 3 lines

the code accesses a vmcs12 field. Alternatively, if we want to avoid unnecessary vmptrlds each time we access vmcs12 we could simple set a flag that indicates when a shadow field was changed. In this case, we

Quoted 2 lines

need to find all the places to check the flag and copy the fields, considering both success and error paths.

That's not how I see it. nested_vmcs_write() will set a request bit in vcpu (this is the flag you mention above). The bit will be checked during a guest entry and vmcs12 will be synced to shadow at this point. Later we can track what fields were written and sync only them.

Quoted 2 lines

Finally, I am afraid that these directions will introduce new issues, will force us to modify too many lines and they may create a

Quoted 1 line

mess...

Conflicts should be trivial and I do not expect many iterations for the patch series. I like the approach you take to use vmcs shadowing and most comment are nitpicks. I promise to review the next version as soon as it is posted :)

Quoted 1 line

Quoted 4 lines

Maybe we should simple fix nested_vmx_entry_failure (as well as the other fixes you suggested in other patches) and apply the code. Do you agree ?

The approach is error prone. Even if we will fix all bugs in the current code each new nested vmx modification will have to be reviewed with

in mind and sometimes it is hard to see global picture just from a patch. It is better to hide shadow details from occasional nested vmx

Are you expecting EPT patches to make it right for instance?

Gleb Natapov <[email protected]> wrote on 11/04/2013 09:54:11 AM:

Quoted 6 lines · Abel Gordon

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 09/04/2013 04:14:35 PM:

Quoted 2 lines

I think the patch already miss some fields. What if nested_vmx_run() fails and calls nested_vmx_entry_failure(). nested_vmx_entry_failure

()

Quoted 1 line

Quoted 1 line

Quoted 1 line

sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but where

do

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

we copy them back to shadow before going back to L1?

Good catch! :)

Note that the entry path is easy to handle because we copy the fields as part of nested_vmx_entry. This is not like exit path where KVM(L0) code can modify fields after nested_vmx_vmexit is called.

Quoted 12 lines

Quoted 5 lines

So here, we could simple call copy_vmcs12_to_shadow if the entry fails (as part of nested_vmx_entry_failure or nested_vmx). We could optimize the code by updating these specific fields directly, but I don't think we really need to optimize code that is part of the error path.

We needn't.

Quoted 5 lines

Quoted 2 lines

May be we need to introduce vmcs12 accessors to track what is changes and if something need to be copied to shadow before going back to L1.

That means we will need to modify all the lines of code that uses "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write

function.

Quoted 2 lines

Quoted 2 lines

Inside these inline functions we could access the shadow vmcs directly. However, to access the shadow vmcs we need to vmptrld first and this

will

Quoted 2 lines

Quoted 2 lines

force
unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01) each

time

Quoted 3 lines

Quoted 3 lines

the code accesses a vmcs12 field. Alternatively, if we want to avoid unnecessary vmptrlds each time we access vmcs12 we could simple set a flag that indicates when a shadow field was changed. In this case, we

will

Quoted 7 lines

Quoted 2 lines

need to find all the places to check the flag and copy the fields, considering both success and error paths.

That's not how I see it. nested_vmcs_write() will set a request bit in vcpu (this is the flag you mention above). The bit will be checked during a guest entry and vmcs12 will be synced to shadow at this point. Later we can track what fields were written and sync only them.

Quoted 2 lines

Quoted 2 lines

Finally, I am afraid that these directions will introduce new issues, will force us to modify too many lines and they may create a

merge/rebase

Quoted 11 lines

Quoted 1 line

mess...

Conflicts should be trivial and I do not expect many iterations for the patch series. I like the approach you take to use vmcs shadowing and most comment are nitpicks. I promise to review the next version as soon as it is posted :)

Quoted 5 lines

Maybe we should simple fix nested_vmx_entry_failure (as well as the other fixes you suggested in other patches) and apply the code. Do you agree ?

Quoted 2 lines

The approach is error prone. Even if we will fix all bugs in the current code each new nested vmx modification will have to be reviewed with

shadowing

Quoted 2 lines

in mind and sometimes it is hard to see global picture just from a patch. It is better to hide shadow details from occasional nested vmx

hacker.

But how ? I suggested 2 alternatives with pros/cons.

Do you have any other alternative in mid ?
Three more alternatives:
(3) set vmcs12 to null once the shadow is synched (so inappropriate accesses
due to buggy code will access a null pointer and crash kvm).
(4) like -1-, but using the accessors and WARN/PANIC.
(5) change nested_vmx_vmexit so it can update additional fields passed by the caller. Additional vmcs12 fields should NOT be updated directly (as it is today). They should be updated only when calling nested_vmx_vmexit.

Quoted 1 line

Are you expecting EPT patches to make it right for instance?

Yep, I actually added shadow vmcs also to a KVM version that includes nEPT patches Nadav Har'El shared. It required adding 1 call to copy_vmcs12_to_shadow after a nested exit related to which updates vmcs12 fields.

From: Gleb Natapov <[email protected]>
To: Abel Gordon/Haifa/IBM@IBMIL,
Cc: [email protected], [email protected], [email protected], "Nadav Har'El" <[email protected]>, [email protected]
Date: 12/04/2013 01:31 PM
Subject: Re: [PATCH 10/11] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs
Sent by: [email protected]

Quoted 9 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 11/04/2013 09:54:11 AM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Gleb Natapov <[email protected]> wrote on 09/04/2013 04:14:35 PM:

Quoted 1 line

I think the patch already miss some fields. What if

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

fails and calls nested_vmx_entry_failure().

Quoted 2 lines

()

Quoted 1 line

Quoted 1 line

Quoted 1 line

sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but

Quoted 6 lines

do

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

we copy them back to shadow before going back to L1?

Good catch! :)

Note that the entry path is easy to handle because we copy the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

as part of nested_vmx_entry. This is not like exit path where KVM(L0) code can modify fields after nested_vmx_vmexit is called.

So here, we could simple call copy_vmcs12_to_shadow if the entry

Quoted 1 line

Quoted 1 line

Quoted 1 line

(as part of nested_vmx_entry_failure or nested_vmx). We could

Quoted 1 line

Quoted 1 line

Quoted 1 line

the code by updating these specific fields directly, but I don't

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

we really need to optimize code that is part of the error path.

We needn't.

Quoted 1 line

Quoted 1 line

May be we need to introduce vmcs12 accessors to track what is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and if something need to be copied to shadow before going back to

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

That means we will need to modify all the lines of code that uses "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write

function.

Quoted 1 line

Quoted 1 line

Inside these inline functions we could access the shadow vmcs

Quoted 1 line

Quoted 1 line

Quoted 1 line

However, to access the shadow vmcs we need to vmptrld first and

Quoted 3 lines

will

Quoted 2 lines

Quoted 2 lines

force
unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01)

Quoted 2 lines

time

Quoted 1 line

Quoted 1 line

the code accesses a vmcs12 field. Alternatively, if we want to

Quoted 1 line

Quoted 1 line

Quoted 1 line

unnecessary vmptrlds each time we access vmcs12 we could simple set

Quoted 1 line

Quoted 1 line

Quoted 1 line

flag that indicates when a shadow field was changed. In this case,

Quoted 4 lines

will

Quoted 3 lines

Quoted 2 lines

need to find all the places to check the flag and copy the fields, considering both success and error paths.

That's not how I see it. nested_vmcs_write() will set a request bit

Quoted 1 line

Quoted 1 line

vcpu (this is the flag you mention above). The bit will be checked

Quoted 1 line

Quoted 1 line

a guest entry and vmcs12 will be synced to shadow at this point.

Quoted 2 lines

Quoted 2 lines

we can track what fields were written and sync only them.

XXX

Quoted 1 line

Quoted 1 line

Quoted 1 line

Finally, I am afraid that these directions will introduce new

Quoted 4 lines

Quoted 1 line

Quoted 1 line

will force us to modify too many lines and they may create a

merge/rebase

Quoted 2 lines

Quoted 1 line

mess...

Conflicts should be trivial and I do not expect many iterations for

Quoted 1 line

Quoted 1 line

patch series. I like the approach you take to use vmcs shadowing and

Quoted 1 line

Quoted 1 line

comment are nitpicks. I promise to review the next version as soon

Quoted 8 lines

Quoted 8 lines

it is posted :)

Quoted 5 lines

Maybe we should simple fix nested_vmx_entry_failure (as well as the other fixes you suggested in other patches) and apply the code. Do you agree ?

The approach is error prone. Even if we will fix all bugs in the

Quoted 7 lines

Quoted 1 line

code each new nested vmx modification will have to be reviewed with

shadowing

Quoted 2 lines

in mind and sometimes it is hard to see global picture just from a patch. It is better to hide shadow details from occasional nested vmx

hacker.

But how ? I suggested 2 alternatives with pros/cons.

I outlined how above (search for XXX). Use accessors, set request bit, sync during guest entry if request bit is set.

[email protected] wrote on 12/04/2013 01:31:17 PM:

Quoted 10 lines

From: Gleb Natapov <[email protected]>
To: Abel Gordon/Haifa/IBM@IBMIL,
Cc: [email protected], [email protected], [email protected], "Nadav Har'El" <[email protected]>, [email protected]
Date: 12/04/2013 01:31 PM
Subject: Re: [PATCH 10/11] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs
Sent by: [email protected]

Quoted 10 lines

Quoted 9 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 11/04/2013 09:54:11 AM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Gleb Natapov <[email protected]> wrote on 09/04/2013 04:14:35 PM:

Quoted 1 line

I think the patch already miss some fields. What if

nested_vmx_run()

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

fails and calls nested_vmx_entry_failure().

nested_vmx_entry_failure

Quoted 2 lines

Quoted 2 lines

()

Quoted 1 line

Quoted 1 line

Quoted 1 line

sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but

where

Quoted 6 lines

Quoted 6 lines

do

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

we copy them back to shadow before going back to L1?

Good catch! :)

Note that the entry path is easy to handle because we copy the

fields

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

as part of nested_vmx_entry. This is not like exit path where KVM(L0) code can modify fields after nested_vmx_vmexit is called.

So here, we could simple call copy_vmcs12_to_shadow if the entry

fails

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

(as part of nested_vmx_entry_failure or nested_vmx). We could

optimize

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the code by updating these specific fields directly, but I don't

think

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

we really need to optimize code that is part of the error path.

We needn't.

Quoted 1 line

Quoted 1 line

May be we need to introduce vmcs12 accessors to track what is

changes

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and if something need to be copied to shadow before going back to

L1.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

That means we will need to modify all the lines of code that uses "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write

function.

Quoted 1 line

Quoted 1 line

Inside these inline functions we could access the shadow vmcs

directly.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

However, to access the shadow vmcs we need to vmptrld first and

this

Quoted 3 lines

Quoted 3 lines

will

Quoted 2 lines

Quoted 2 lines

force
unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01)

each

Quoted 2 lines

Quoted 2 lines

time

Quoted 1 line

Quoted 1 line

the code accesses a vmcs12 field. Alternatively, if we want to

avoid

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

unnecessary vmptrlds each time we access vmcs12 we could simple set

a

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

flag that indicates when a shadow field was changed. In this case,

we

Quoted 4 lines

Quoted 4 lines

will

Quoted 3 lines

Quoted 2 lines

need to find all the places to check the flag and copy the fields, considering both success and error paths.

That's not how I see it. nested_vmcs_write() will set a request bit

in

Quoted 1 line

Quoted 1 line

Quoted 1 line

vcpu (this is the flag you mention above). The bit will be checked

during

Quoted 1 line

Quoted 1 line

Quoted 1 line

a guest entry and vmcs12 will be synced to shadow at this point.

Later

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

we can track what fields were written and sync only them.

XXX

Quoted 1 line

Quoted 1 line

Quoted 1 line

Finally, I am afraid that these directions will introduce new

issues,

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

Quoted 1 line

will force us to modify too many lines and they may create a

merge/rebase

Quoted 2 lines

Quoted 1 line

mess...

Conflicts should be trivial and I do not expect many iterations for

the

Quoted 1 line

Quoted 1 line

Quoted 1 line

patch series. I like the approach you take to use vmcs shadowing and

most

Quoted 1 line

Quoted 1 line

Quoted 1 line

comment are nitpicks. I promise to review the next version as soon

as

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

it is posted :)

Quoted 5 lines

Maybe we should simple fix nested_vmx_entry_failure (as well as the other fixes you suggested in other patches) and apply the code. Do you agree ?

The approach is error prone. Even if we will fix all bugs in the

current

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

code each new nested vmx modification will have to be reviewed with

shadowing

Quoted 2 lines

in mind and sometimes it is hard to see global picture just from a patch. It is better to hide shadow details from occasional nested vmx

hacker.

But how ? I suggested 2 alternatives with pros/cons.

I outlined how above (search for XXX). Use accessors, set request bit, sync during guest entry if request bit is set.

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are not concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once you apply this patch, I'll continue and send you v2 patches for shadow vmcs.

Do you agree ?

Quoted 4 lines

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

Quoted 2 lines

Do you agree ?

Yes.

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset the flag and call copy_vmcs12_to_shadow (if required).

What do you think ?

Quoted 26 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset the flag and call copy_vmcs12_to_shadow (if required).

Quoted 1 line

What do you think ?

Good idea! With accessors we can do further optimization by copying only things that changed, but it will be premature optimization at this point.

Quoted 27 lines · Gleb Natapov

Quoted 26 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset the flag and call copy_vmcs12_to_shadow (if required).

Quoted 5 lines

Quoted 1 line

What do you think ?

Good idea! With accessors we can do further optimization by copying only things that changed, but it will be premature optimization at this point.

Actually this is good idea only if we know for sure that VMX emulation changes vmcs12 only during guest entry/exit. Is this the case? I think so.

Quoted 9 lines · Gleb Natapov

Quoted 8 lines · Abel Gordon

Quoted 7 lines

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Ok, so then you prefer to add the inline functions to read/write to

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

vmcs12
fields, (to set the request bit if shadowed field changed) and you

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

vmcs.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 1 line

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset

the flag and

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by copying

Quoted 6 lines

Quoted 3 lines

things that changed, but it will be premature optimization at this point.

Actually this is good idea only if we know for sure that VMX emulation changes vmcs12 only during guest entry/exit. Is this the case? I think so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->L1 transitions. What comes to my mind: L0 emulates some change that L1 does not trap, e.g. CRx accesses. Or what do you mean?

Quoted 33 lines · Gleb Natapov

Quoted 32 lines · Gleb Natapov

Quoted 20 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 5 lines · Abel Gordon

Quoted 4 lines

Ok, so then you prefer to add the inline functions to read/write to the vmcs12
fields, (to set the request bit if shadowed field changed) and you are

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for shadow

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Quoted 7 lines

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset the flag and call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by copying only things that changed, but it will be premature optimization at this point.

Quoted 3 lines

Actually this is good idea only if we know for sure that VMX emulation changes vmcs12 only during guest entry/exit. Is this the case? I think so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->L1 transitions. What comes to my mind: L0 emulates some change that L1 does not trap, e.g. CRx accesses. Or what do you mean?

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Gleb Natapov

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Ok, so then you prefer to add the inline functions to read/

write to the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

vmcs12
fields, (to set the request bit if shadowed field changed) and

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset

the flag and

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by copying

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

things that changed, but it will be premature optimization at this point.

Actually this is good idea only if we know for sure that VMX

Quoted 1 line

Quoted 1 line

changes vmcs12 only during guest entry/exit. Is this the case? I

Quoted 4 lines

Quoted 1 line

so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->L1 transitions. What comes to my mind: L0 emulates some change that L1

Quoted 2 lines

not trap, e.g. CRx accesses. Or what do you mean?

If vmcs12 is changed by L0 while L2 is running this is OK. If L0 changes shadowed vmcs12 field while L1 is running this is not OK. So for instance if field XXX is R/W but we allow only read to be shadowed then write emulation in L0 has to sync new value back to shadow before going back to L1.

Gleb Natapov <[email protected]> wrote on 14/04/2013 01:34:52 PM:

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Gleb Natapov

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Ok, so then you prefer to add the inline functions to read/

write to the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

vmcs12
fields, (to set the request bit if shadowed field changed) and

you are

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors.

Once

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for

shadow

Quoted 14 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding the accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset

the flag and

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by copying

only

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

things that changed, but it will be premature optimization at this point.

Actually this is good idea only if we know for sure that VMX

emulation

Quoted 1 line

Quoted 1 line

Quoted 1 line

changes vmcs12 only during guest entry/exit. Is this the case? I

think

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->L1 transitions. What comes to my mind: L0 emulates some change that L1

does

Quoted 7 lines

Quoted 2 lines

not trap, e.g. CRx accesses. Or what do you mean?

If vmcs12 is changed by L0 while L2 is running this is OK. If L0 changes shadowed vmcs12 field while L1 is running this is not OK. So for instance if field XXX is R/W but we allow only read to be shadowed then write emulation in L0 has to sync new value back to shadow before going back to L1.

Exactly.

While L1 runs (L1 root mode), L0 does NOT change VMCS12 (unless L1 executes vmwrite).
VMCS12 fields are changed once L1 launches/resumes L2 and there is a L2 exit.

L0 can change VMCS12 while it handles a L2 exit directly which is not forwarded to L1. But that's OK because L1 will eventually see the change once we switch to L1 due to other exit that L0 let L1 handle.

L0 should NOT change VMCS12 fields if L1 is running and L1 didn't execute any vmlaunch, vmresume or vmwrite instruction.

Quoted 10 lines

Gleb Natapov <[email protected]> wrote on 14/04/2013 01:34:52 PM:

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Gleb Natapov

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Ok, so then you prefer to add the inline functions to read/

write to the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

vmcs12
fields, (to set the request bit if shadowed field changed)

Quoted 5 lines

you are

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the

Quoted 10 lines

Once

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for

shadow

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset

the flag and

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by

Quoted 2 lines

only

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

things that changed, but it will be premature optimization at

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

point.

Actually this is good idea only if we know for sure that VMX

emulation

Quoted 1 line

Quoted 1 line

Quoted 1 line

changes vmcs12 only during guest entry/exit. Is this the case? I

think

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->

Quoted 5 lines

Quoted 1 line

Quoted 1 line

transitions. What comes to my mind: L0 emulates some change that L1

does

Quoted 3 lines

Quoted 2 lines

not trap, e.g. CRx accesses. Or what do you mean?

If vmcs12 is changed by L0 while L2 is running this is OK. If L0

Quoted 2 lines

Quoted 2 lines

shadowed vmcs12 field while L1 is running this is not OK. So for instance if field XXX is R/W but we allow only read to be shadowed

Quoted 1 line

Quoted 1 line

write emulation in L0 has to sync new value back to shadow before

Quoted 11 lines

Quoted 1 line

back to L1.

Exactly.

While L1 runs (L1 root mode), L0 does NOT change VMCS12 (unless L1 executes vmwrite).
VMCS12 fields are changed once L1 launches/resumes L2 and there is a L2 exit.

L0 can change VMCS12 while it handles a L2 exit directly which is not forwarded to L1. But that's OK because L1 will eventually see the

Quoted 5 lines

once we switch to L1 due to other exit that L0 let L1 handle.

L0 should NOT change VMCS12 fields if L1 is running and L1 didn't execute any vmlaunch, vmresume or vmwrite instruction.

The question is: is there a case like I described above when we shadow only reads from R/W field and handle vmwrites in L0?

Gleb Natapov <[email protected]> wrote on 14/04/2013 02:16:02 PM:

Quoted 11 lines · Abel Gordon

Quoted 10 lines

Gleb Natapov <[email protected]> wrote on 14/04/2013 01:34:52 PM:

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Gleb Natapov

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Gleb Natapov <[email protected]> wrote on 12/04/2013 01:48:04 PM:

Quoted 3 lines · Abel Gordon

Quoted 2 lines

Ok, so then you prefer to add the inline functions to read/

write to the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

vmcs12
fields, (to set the request bit if shadowed field changed)

and

Quoted 5 lines

Quoted 5 lines

you are

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

not

Quoted 3 lines

Quoted 3 lines

concerned
about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the

accessors.

Quoted 10 lines

Quoted 10 lines

Once

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

you

Quoted 1 line

Quoted 1 line

apply this patch, I'll continue and send you v2 patches for

shadow

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

vmcs.

Quoted 3 lines

Quoted 2 lines

Do you agree ?

Yes.

Looking again at the code it seems like we could avoid adding

the

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

accessors.
We could just set a flag in nested_vmx_vmexit and nested_vmx_entry_failure. Then, in vmx_vcpu_run we check/reset

the flag and

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

call copy_vmcs12_to_shadow (if required).

What do you think ?

Good idea! With accessors we can do further optimization by

copying

Quoted 2 lines

Quoted 2 lines

only

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

things that changed, but it will be premature optimization at

this

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

point.

Actually this is good idea only if we know for sure that VMX

emulation

Quoted 1 line

Quoted 1 line

Quoted 1 line

changes vmcs12 only during guest entry/exit. Is this the case? I

think

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

so.

Some vmcs12 fields that are exposed to L1 are changed outside L2<->

L1

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

transitions. What comes to my mind: L0 emulates some change that L1

does

Quoted 3 lines

Quoted 2 lines

not trap, e.g. CRx accesses. Or what do you mean?

If vmcs12 is changed by L0 while L2 is running this is OK. If L0

changes

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

shadowed vmcs12 field while L1 is running this is not OK. So for instance if field XXX is R/W but we allow only read to be shadowed

then

Quoted 1 line

Quoted 1 line

Quoted 1 line

write emulation in L0 has to sync new value back to shadow before

going

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

back to L1.

Exactly.

While L1 runs (L1 root mode), L0 does NOT change VMCS12 (unless L1 executes vmwrite).
VMCS12 fields are changed once L1 launches/resumes L2 and there is a L2 exit.

L0 can change VMCS12 while it handles a L2 exit directly which is not forwarded to L1. But that's OK because L1 will eventually see the

change

Quoted 7 lines

Quoted 5 lines

once we switch to L1 due to other exit that L0 let L1 handle.

L0 should NOT change VMCS12 fields if L1 is running and L1 didn't execute any vmlaunch, vmresume or vmwrite instruction.

The question is: is there a case like I described above when we shadow only reads from R/W field and handle vmwrites in L0?

No, there is no such thing with the patches I submitted. The shadow VMCS patches assumes that only R/O fields can be shadowed for read and not for write (the shadow_read_only_fields array). R/O fields can't be written by L1 thus L0 will trap vmwrite to any r/o field and emulate a fail (VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT).

Writable fields are shadowed for both read and write (the shadow_read_write_fields array).

There are no writable fields shadowed for write and not for read. There are no writable fields shadowed for read and not for write

This series of patches implements shadow-vmcs capability for nested VMX.

Shadow-vmcs - background and overview:

In Intel VMX, vmread and vmwrite privileged instructions are used by the hypervisor to read and modify the guest and host specifications (VMCS). In a nested virtualization environment, L1 executes multiple vmread and vmwrite instruction to handle a single L2 exit. Each vmread and vmwrite executed by L1 traps (cause an exit) to the L0 hypervisor (KVM). L0 emulates the instruction behaviour and resumes L1 execution.

Removing the need to trap and emulate these special instructions reduces the number of exits and improves nested virtualization performance. As it was first evaluated in [1], exit-less vmread and vmwrite can reduce nested virtualization overhead up-to 40%.

Intel introduced a new feature to their processors called shadow-vmcs. Using shadow-vmcs, L0 can configure the processor to let L1 running in guest-mode access VMCS12 fields using vmread and vmwrite instructions but without causing an exit to L0. The VMCS12 fields' data is stored in a shadow-vmcs controlled by L0.

Shadow-vmcs - design considerations:

A shadow-vmcs is processor-dependent and must be accessed by L0 or L1 using vmread and vmwrite instructions. With nested virtualization we aim to abstract the hardware from the L1 hypervisor. Thus, to avoid hardware dependencies we prefered to keep the software defined VMCS12 format as part of L1 address space and hold the processor-specific shadow-vmcs format only in L0 address space. In other words, the shadow-vmcs is used by L0 as an accelerator but the format and content is never exposed to L1 directly. L0 syncs the content of the processor-specific shadow vmcs with the content of the software-controlled VMCS12 format.

We could have been kept the processor-specific shadow-vmcs format in L1 address space to avoid using the software defined VMCS12 format, however, this type of design/implementation would have been created hardware dependencies and would complicate other capabilities (e.g. Live Migration of L1).

Acknowledgments:

Many thanks to
"Xu, Dongxiao" <[email protected]>
"Nakajima, Jun" <[email protected]>
"Har'El, Nadav" <[email protected]>

for the insightful discussions, comments and reviews.

These patches were easily created and maintained using
Patchouli -- patch creator
http://patchouli.sourceforge.net/

[1] "The Turtles Project: Design and Implementation of Nested Virtualization",
http://www.usenix.org/events/osdi10/tech/full_papers/Ben-Yehuda.pdf

--
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

By the way do you have some performance results, how does it improve