Skip to contentSkip to conversation

KVM: nVMX: shadow VMCS support, v3

PATCH In review Abel Gordon 2 participants 18 messages Apr 17, 2013 – Apr 18, 2013

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Abel Gordon

author
#0 lore raw

cover KVM: nVMX: shadow VMCS support, v3

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

Changes since v1:
1) Added sync_shadow_vmcs flag used to indicate when the content of VMCS12 must be copied to the shadow vmcs. The flag value is checked during vmx_vcpu_run.
2) Code quality improvements

Changes since v2:
1) Allocate shadow vmcs only once per VCPU on handle_vmxon and re-use the same instance for multiple VMCS12s
2) More code quality improvements

Acknowledgments:

Many thanks to
"Natapov, Gleb" <[email protected]>
"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

author
#1 lore raw

PATCH 1/10 KVM: nVMX: Shadow-vmcs control fields/bits

Add definitions for all the vmcs control fields/bits required to enable vmcs-shadowing

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/include/asm/vmx.h            |    3 +++
 arch/x86/include/uapi/asm/msr-index.h |    2 ++
 2 files changed, 5 insertions(+)
arch/x86/include/asm/vmx.h +3 -0
@@ -65,6 +65,7 @@
 #define SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY    0x00000200
 #define SECONDARY_EXEC_PAUSE_LOOP_EXITING	0x00000400
 #define SECONDARY_EXEC_ENABLE_INVPCID		0x00001000
+#define SECONDARY_EXEC_SHADOW_VMCS              0x00004000
 
 
 #define PIN_BASED_EXT_INTR_MASK                 0x00000001

@@ -150,6 +151,8 @@ enum vmcs_field {
 	EOI_EXIT_BITMAP2_HIGH           = 0x00002021,
 	EOI_EXIT_BITMAP3                = 0x00002022,
 	EOI_EXIT_BITMAP3_HIGH           = 0x00002023,
+	VMREAD_BITMAP                   = 0x00002026,
+	VMWRITE_BITMAP                  = 0x00002028,
 	GUEST_PHYSICAL_ADDRESS          = 0x00002400,
 	GUEST_PHYSICAL_ADDRESS_HIGH     = 0x00002401,
 	VMCS_LINK_POINTER               = 0x00002800,
arch/x86/include/uapi/asm/msr-index.h +2 -0
@@ -522,6 +522,8 @@
 #define VMX_BASIC_MEM_TYPE_WB	6LLU
 #define VMX_BASIC_INOUT		0x0040000000000000LLU
 
+/* MSR_IA32_VMX_MISC bits */
+#define MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS (1ULL << 29)
 /* AMD-V MSRs */
 
 #define MSR_VM_CR                       0xc0010114
author
#2 lore raw

PATCH 2/10 KVM: nVMX: Detect shadow-vmcs capability

In reply to Abel Gordon

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 +24 -1
@@ -87,6 +87,8 @@ module_param(fasteoi, bool, S_IRUGO);
 static bool __read_mostly enable_apicv = 1;
 module_param(enable_apicv, bool, S_IRUGO);
 
+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

@@ -940,6 +942,18 @@ static inline bool cpu_has_vmx_wbinvd_ex
 		SECONDARY_EXEC_WBINVD_EXITING;
 }
 
+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 & MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS))
+		return false;
+
+	return vmcs_config.cpu_based_2nd_exec_ctrl &
+		SECONDARY_EXEC_SHADOW_VMCS;
+}
+
 static inline bool report_flexpriority(void)
 {
 	return flexpriority_enabled;

@@ -2632,7 +2646,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)

@@ -2833,6 +2848,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()) {

@@ -4075,6 +4092,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;
 }
author
#3 lore raw

PATCH 3/10 KVM: nVMX: Introduce vmread and vmwrite bitmaps

In reply to Abel Gordon

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 |   85 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)
arch/x86/kvm/vmx.c +84 -1
@@ -484,6 +484,59 @@ 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,
+	/*
+	 * we do NOT shadow VM_INSTRUCTION_ERROR because it is read
+	 * by L1 only as part of the error path.
+	 * Note the code assumes this logic. If for some reason
+	 * we start shadowing VM_INSTRUCTION_ERROR then we need to
+	 * force a shadow sync in nested_vmx_failValid
+	 */
+	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),

@@ -675,6 +728,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;

@@ -4126,6 +4181,10 @@ 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));
 
+	if (enable_shadow_vmcs) {
+		vmcs_write64(VMREAD_BITMAP, __pa(vmx_vmread_bitmap));
+		vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap));
+	}
 	if (cpu_has_vmx_msr_bitmap())
 		vmcs_write64(MSR_BITMAP, __pa(vmx_msr_bitmap_legacy));
 

@@ -7936,6 +7995,24 @@ static int __init vmx_init(void)
 				(unsigned long *)__get_free_page(GFP_KERNEL);
 	if (!vmx_msr_bitmap_longmode_x2apic)
 		goto out4;
+	vmx_vmread_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
+	if (!vmx_vmread_bitmap)
+		goto out5;
+
+	vmx_vmwrite_bitmap = (unsigned long *)__get_free_page(GFP_KERNEL);
+	if (!vmx_vmwrite_bitmap)
+		goto out6;
+
+	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

@@ -7954,7 +8031,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 out5;
+		goto out7;
 
 #ifdef CONFIG_KEXEC
 	rcu_assign_pointer(crash_vmclear_loaded_vmcss,

@@ -8002,6 +8079,10 @@ static int __init vmx_init(void)
 
 	return 0;
 
+out7:
+	free_page((unsigned long)vmx_vmwrite_bitmap);
+out6:
+	free_page((unsigned long)vmx_vmread_bitmap);
 out5:
 	free_page((unsigned long)vmx_msr_bitmap_longmode_x2apic);
 out4:

@@ -8025,6 +8106,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);
author
#4 lore raw

PATCH 4/10 KVM: nVMX: Refactor handle_vmwrite

In reply to Abel Gordon

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 +28 -24
@@ -5835,6 +5835,33 @@ static inline bool vmcs12_read_any(struc
 	}
 }
 
+
+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 false;
+
+	switch (vmcs_field_type(field)) {
+	case VMCS_FIELD_TYPE_U16:
+		*(u16 *)p = field_value;
+		return true;
+	case VMCS_FIELD_TYPE_U32:
+		*(u32 *)p = field_value;
+		return true;
+	case VMCS_FIELD_TYPE_U64:
+		*(u64 *)p = field_value;
+		return true;
+	case VMCS_FIELD_TYPE_NATURAL_WIDTH:
+		*(natural_width *)p = field_value;
+		return true;
+	default:
+		return false; /* 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.

@@ -5899,8 +5926,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

@@ -5937,28 +5962,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;
author
#5 lore raw

PATCH 5/10 KVM: nVMX: Allocate shadow vmcs

In reply to Abel Gordon

Allocate a shadow vmcs used by the processor to shadow part of the fields stored in the software defined VMCS12 (let L1 access fields without causing exits). Note we keep a shadow vmcs only for the current vmcs12. Once a vmcs12 becomes non-current, its shadow vmcs is released.

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/x86/kvm/vmx.c +12 -0
@@ -355,6 +355,7 @@ struct nested_vmx {
 	/* The host-usable pointer to the above */
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
+	struct vmcs *current_shadow_vmcs;
 
 	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
 	struct list_head vmcs02_pool;

@@ -5517,6 +5518,7 @@ static int handle_vmon(struct kvm_vcpu *
 {
 	struct kvm_segment cs;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vmcs *shadow_vmcs;
 
 	/* The Intel VMX Instruction Reference lists a bunch of bits that
 	 * are prerequisite to running VMXON, most notably cr4.VMXE must be

@@ -5540,6 +5542,16 @@ static int handle_vmon(struct kvm_vcpu *
 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}
+	if (enable_shadow_vmcs) {
+		shadow_vmcs = alloc_vmcs();
+		if (!shadow_vmcs)
+			return -ENOMEM;
+		/* mark vmcs as shadow */
+		shadow_vmcs->revision_id |= (1u << 31);
+		/* init shadow vmcs */
+		vmcs_clear(shadow_vmcs);
+		vmx->nested.current_shadow_vmcs = shadow_vmcs;
+	}
 
 	INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
 	vmx->nested.vmcs02_num = 0;
author
#10 lore raw

PATCH 6/10 KVM: nVMX: Release shadow vmcs

In reply to Abel Gordon

Unmap vmcs12 and release the corresponding shadow vmcs

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
arch/x86/kvm/vmx.c +12 -8
@@ -5592,6 +5592,12 @@ static int nested_vmx_check_permission(s
 	return 1;
 }
 
+static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
+{
+	kunmap(vmx->nested.current_vmcs12_page);
+	nested_release_page(vmx->nested.current_vmcs12_page);
+}
+
 /*
  * Free whatever needs to be freed from vmx->nested when L1 goes down, or
  * just stops using VMX.

@@ -5602,8 +5608,9 @@ static void free_nested(struct vcpu_vmx 
 		return;
 	vmx->nested.vmxon = false;
 	if (vmx->nested.current_vmptr != -1ull) {
-		kunmap(vmx->nested.current_vmcs12_page);
-		nested_release_page(vmx->nested.current_vmcs12_page);
+		nested_release_vmcs12(vmx);
+		if (enable_shadow_vmcs)
+			free_vmcs(vmx->nested.current_shadow_vmcs);
 		vmx->nested.current_vmptr = -1ull;
 		vmx->nested.current_vmcs12 = NULL;
 	}

@@ -5747,8 +5754,7 @@ static int handle_vmclear(struct kvm_vcp
 	}
 
 	if (vmptr == vmx->nested.current_vmptr) {
-		kunmap(vmx->nested.current_vmcs12_page);
-		nested_release_page(vmx->nested.current_vmcs12_page);
+		nested_release_vmcs12(vmx);
 		vmx->nested.current_vmptr = -1ull;
 		vmx->nested.current_vmcs12 = NULL;
 	}

@@ -6030,10 +6036,8 @@ static int handle_vmptrld(struct kvm_vcp
 			skip_emulated_instruction(vcpu);
 			return 1;
 		}
-		if (vmx->nested.current_vmptr != -1ull) {
-			kunmap(vmx->nested.current_vmcs12_page);
-			nested_release_page(vmx->nested.current_vmcs12_page);
-		}
+		if (vmx->nested.current_vmptr != -1ull)
+			nested_release_vmcs12(vmx);
 
 		vmx->nested.current_vmptr = vmptr;
 		vmx->nested.current_vmcs12 = new_vmcs12;
author
#11 lore raw

PATCH 7/10 KVM: nVMX: Copy processor-specific shadow-vmcs to VMCS12

In reply to Abel Gordon

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

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
arch/x86/kvm/vmx.c +35 -0
@@ -713,6 +713,7 @@ static void vmx_get_segment(struct kvm_v
 static bool guest_state_valid(struct kvm_vcpu *vcpu);
 static u32 vmx_segment_access_rights(struct kvm_segment *var);
 static void vmx_sync_pir_to_irr_dummy(struct kvm_vcpu *vcpu);
+static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);

@@ -5880,6 +5881,40 @@ static inline bool vmcs12_write_any(stru
 
 }
 
+static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
+{
+	int i;
+	unsigned long field;
+	u64 field_value;
+	struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
+	unsigned long *fields = (unsigned long *)shadow_read_write_fields;
+	int num_fields = max_shadow_read_write_fields;
+
+	vmcs_load(shadow_vmcs);
+
+	for (i = 0; i < num_fields; i++) {
+		field = fields[i];
+		switch (vmcs_field_type(field)) {
+		case VMCS_FIELD_TYPE_U16:
+			field_value = vmcs_read16(field);
+			break;
+		case VMCS_FIELD_TYPE_U32:
+			field_value = vmcs_read32(field);
+			break;
+		case VMCS_FIELD_TYPE_U64:
+			field_value = vmcs_read64(field);
+			break;
+		case VMCS_FIELD_TYPE_NATURAL_WIDTH:
+			field_value = vmcs_readl(field);
+			break;
+		}
+		vmcs12_write_any(&vmx->vcpu, field, field_value);
+	}
+
+	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.
author
#12 lore raw

PATCH 8/10 KVM: nVMX: Copy VMCS12 to processor-specific shadow vmcs

In reply to Abel Gordon

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 +45 -0
@@ -713,6 +713,7 @@ static void vmx_get_segment(struct kvm_v
 static bool guest_state_valid(struct kvm_vcpu *vcpu);
 static u32 vmx_segment_access_rights(struct kvm_segment *var);
 static void vmx_sync_pir_to_irr_dummy(struct kvm_vcpu *vcpu);
+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);

@@ -5915,6 +5916,50 @@ static void copy_shadow_to_vmcs12(struct
 	vmcs_load(vmx->loaded_vmcs->vmcs);
 }
 
+static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
+{
+	unsigned long *fields[] = {
+		(unsigned long *)shadow_read_write_fields,
+		(unsigned long *)shadow_read_only_fields
+	};
+	int num_lists =  ARRAY_SIZE(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.
author
#13 lore raw

PATCH 9/10 KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs

In reply to Abel Gordon

Synchronize between the VMCS12 software controlled structure and the processor-specific shadow vmcs

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
arch/x86/kvm/vmx.c +32 -0
@@ -356,6 +356,11 @@ struct nested_vmx {
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
 	struct vmcs *current_shadow_vmcs;
+	/*
+	 * Indicates if the shadow vmcs must be updated with the
+	 * data hold by vmcs12
+	 */
+	bool sync_shadow_vmcs;
 
 	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
 	struct list_head vmcs02_pool;

@@ -5596,6 +5601,14 @@ static int nested_vmx_check_permission(s
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
+	if (enable_shadow_vmcs) {
+		if (vmx->nested.current_vmcs12 != NULL) {
+			/* copy to memory all shadowed fields in case
+			   they were modified */
+			copy_shadow_to_vmcs12(vmx);
+			vmx->nested.sync_shadow_vmcs = false;
+		}
+	}
 	kunmap(vmx->nested.current_vmcs12_page);
 	nested_release_page(vmx->nested.current_vmcs12_page);
 }

@@ -5724,6 +5737,10 @@ static void nested_vmx_failValid(struct 
 			    X86_EFLAGS_SF | X86_EFLAGS_OF))
 			| X86_EFLAGS_ZF);
 	get_vmcs12(vcpu)->vm_instruction_error = vm_instruction_error;
+	/*
+	 * We don't need to force a shadow sync because
+	 * VM_INSTRUCTION_ERROR is not shadowed
+	 */
 }
 
 /* Emulate the VMCLEAR instruction */

@@ -6122,6 +6139,9 @@ static int handle_vmptrld(struct kvm_vcp
 		vmx->nested.current_vmptr = vmptr;
 		vmx->nested.current_vmcs12 = new_vmcs12;
 		vmx->nested.current_vmcs12_page = page;
+		if (enable_shadow_vmcs) {
+			vmx->nested.sync_shadow_vmcs = true;
+		}
 	}
 
 	nested_vmx_succeed(vcpu);

@@ -6880,6 +6900,11 @@ static void __noclone vmx_vcpu_run(struc
 	if (vmx->emulation_required)
 		return;
 
+	if (vmx->nested.sync_shadow_vmcs) {
+		copy_vmcs12_to_shadow(vmx);
+		vmx->nested.sync_shadow_vmcs = false;
+	}
+
 	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
 	if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))

@@ -7490,6 +7515,9 @@ static int nested_vmx_run(struct kvm_vcp
 	skip_emulated_instruction(vcpu);
 	vmcs12 = get_vmcs12(vcpu);
 
+	if (enable_shadow_vmcs)
+		copy_shadow_to_vmcs12(vmx);
+
 	/*
 	 * The nested entry process starts with enforcing various prerequisites
 	 * on vmcs12 as required by the Intel SDM, and act appropriately when

@@ -7932,6 +7960,8 @@ static void nested_vmx_vmexit(struct kvm
 		nested_vmx_failValid(vcpu, vmcs_read32(VM_INSTRUCTION_ERROR));
 	} else
 		nested_vmx_succeed(vcpu);
+	if (enable_shadow_vmcs)
+		vmx->nested.sync_shadow_vmcs = true;
 }
 
 /*

@@ -7949,6 +7979,8 @@ static void nested_vmx_entry_failure(str
 	vmcs12->vm_exit_reason = reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
 	vmcs12->exit_qualification = qualification;
 	nested_vmx_succeed(vcpu);
+	if (enable_shadow_vmcs)
+		to_vmx(vcpu)->nested.sync_shadow_vmcs = true;
 }
 
 static int vmx_check_intercept(struct kvm_vcpu *vcpu,
author
#17 lore raw

PATCH 10/10 KVM: nVMX: Enable and disable shadow vmcs functionality

In reply to Abel Gordon

Once L1 loads VMCS12 we enable shadow-vmcs capability and copy all the VMCS12 shadowed fields to the shadow vmcs. When we release the VMCS12, we also disable shadow-vmcs capability.

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
arch/x86/kvm/vmx.c +11 -0
@@ -5601,12 +5601,17 @@ static int nested_vmx_check_permission(s
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
+	u32 exec_control;
 	if (enable_shadow_vmcs) {
 		if (vmx->nested.current_vmcs12 != NULL) {
 			/* copy to memory all shadowed fields in case
 			   they were modified */
 			copy_shadow_to_vmcs12(vmx);
 			vmx->nested.sync_shadow_vmcs = false;
+			exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
+			exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
+			vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+			vmcs_write64(VMCS_LINK_POINTER, -1ull);
 		}
 	}
 	kunmap(vmx->nested.current_vmcs12_page);

@@ -6095,6 +6100,7 @@ static int handle_vmptrld(struct kvm_vcp
 	gva_t gva;
 	gpa_t vmptr;
 	struct x86_exception e;
+	u32 exec_control;
 
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;

@@ -6140,6 +6146,11 @@ static int handle_vmptrld(struct kvm_vcp
 		vmx->nested.current_vmcs12 = new_vmcs12;
 		vmx->nested.current_vmcs12_page = page;
 		if (enable_shadow_vmcs) {
+			exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
+			exec_control |= SECONDARY_EXEC_SHADOW_VMCS;
+			vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+			vmcs_write64(VMCS_LINK_POINTER,
+				     __pa(vmx->nested.current_shadow_vmcs));
 			vmx->nested.sync_shadow_vmcs = true;
 		}
 	}
#6 lore raw

Re: [PATCH 05/10] KVM: nVMX: Allocate shadow vmcs

In reply to Abel Gordon
Quoted 34 lines · Abel Gordon

Allocate a shadow vmcs used by the processor to shadow part of the fields stored in the software defined VMCS12 (let L1 access fields without causing exits). Note we keep a shadow vmcs only for the current vmcs12. Once a vmcs12 becomes non-current, its shadow vmcs is released.

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/x86/kvm/vmx.c +3 -0
@@ -355,6 +355,7 @@ struct nested_vmx {
 	/* The host-usable pointer to the above */
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
+	struct vmcs *current_shadow_vmcs;
 
 	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
 	struct list_head vmcs02_pool;

@@ -5517,6 +5518,7 @@ static int handle_vmon(struct kvm_vcpu *
 {
 	struct kvm_segment cs;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vmcs *shadow_vmcs;
 
 	/* The Intel VMX Instruction Reference lists a bunch of bits that
 	 * are prerequisite to running VMXON, most notably cr4.VMXE must be

@@ -5540,6 +5542,16 @@ static int handle_vmon(struct kvm_vcpu *
 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}
+	if (enable_shadow_vmcs) {

arch/x86/kvm/vmx.c static int handle_vmon(struct kvm_vcpu * · line 5555

Quoted 10 lines
arch/x86/kvm/vmx.c +9 -0
+		shadow_vmcs = alloc_vmcs();
+		if (!shadow_vmcs)
+			return -ENOMEM;
+		/* mark vmcs as shadow */
+		shadow_vmcs->revision_id |= (1u << 31);
+		/* init shadow vmcs */
+		vmcs_clear(shadow_vmcs);
+		vmx->nested.current_shadow_vmcs = shadow_vmcs;
+	}

Guest can ddos host by calling vmxon repeatedly causing host to leak memory. This point to a bug in vmxon implementation. vmxon should call nested_vmx_failInvalid() if (vmx->nested.vmxon).

Quoted 7 lines
arch/x86/kvm/vmx.c +0 -1
 	INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
 	vmx->nested.vmcs02_num = 0;
 
--

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

--
Gleb.

#14 lore raw

Re: [PATCH 09/10] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs

In reply to Abel Gordon
Quoted 49 lines · Abel Gordon

Synchronize between the VMCS12 software controlled structure and the processor-specific shadow vmcs

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
arch/x86/kvm/vmx.c +17 -0
@@ -356,6 +356,11 @@ struct nested_vmx {
 	struct page *current_vmcs12_page;
 	struct vmcs12 *current_vmcs12;
 	struct vmcs *current_shadow_vmcs;
+	/*
+	 * Indicates if the shadow vmcs must be updated with the
+	 * data hold by vmcs12
+	 */
+	bool sync_shadow_vmcs;
 
 	/* vmcs02_list cache of VMCSs recently used to run L2 guests */
 	struct list_head vmcs02_pool;

@@ -5596,6 +5601,14 @@ static int nested_vmx_check_permission(s
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
+	if (enable_shadow_vmcs) {
+		if (vmx->nested.current_vmcs12 != NULL) {
+			/* copy to memory all shadowed fields in case
+			   they were modified */
+			copy_shadow_to_vmcs12(vmx);
+			vmx->nested.sync_shadow_vmcs = false;
+		}
+	}
 	kunmap(vmx->nested.current_vmcs12_page);
 	nested_release_page(vmx->nested.current_vmcs12_page);
 }

@@ -5724,6 +5737,10 @@ static void nested_vmx_failValid(struct 
 			    X86_EFLAGS_SF | X86_EFLAGS_OF))
 			| X86_EFLAGS_ZF);
 	get_vmcs12(vcpu)->vm_instruction_error = vm_instruction_error;
+	/*
+	 * We don't need to force a shadow sync because
+	 * VM_INSTRUCTION_ERROR is not shadowed
+	 */
 }
 
 /* Emulate the VMCLEAR instruction */

arch/x86/kvm/vmx.c static int handle_vmptrld(struct kvm_vcp · line 6144

Quoted 6 lines
arch/x86/kvm/vmx.c +3 -0
@@ -6122,6 +6139,9 @@ static int handle_vmptrld(struct kvm_vcp
 		vmx->nested.current_vmptr = vmptr;
 		vmx->nested.current_vmcs12 = new_vmcs12;
 		vmx->nested.current_vmcs12_page = page;
+		if (enable_shadow_vmcs) {
+			vmx->nested.sync_shadow_vmcs = true;
+		}

No need braces around one statement according to kernel coding style.

Quoted 45 lines
arch/x86/kvm/vmx.c +12 -1
 	}
 
 	nested_vmx_succeed(vcpu);

@@ -6880,6 +6900,11 @@ static void __noclone vmx_vcpu_run(struc
 	if (vmx->emulation_required)
 		return;
 
+	if (vmx->nested.sync_shadow_vmcs) {
+		copy_vmcs12_to_shadow(vmx);
+		vmx->nested.sync_shadow_vmcs = false;
+	}
+
 	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
 	if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))

@@ -7490,6 +7515,9 @@ static int nested_vmx_run(struct kvm_vcp
 	skip_emulated_instruction(vcpu);
 	vmcs12 = get_vmcs12(vcpu);
 
+	if (enable_shadow_vmcs)
+		copy_shadow_to_vmcs12(vmx);
+
 	/*
 	 * The nested entry process starts with enforcing various prerequisites
 	 * on vmcs12 as required by the Intel SDM, and act appropriately when

@@ -7932,6 +7960,8 @@ static void nested_vmx_vmexit(struct kvm
 		nested_vmx_failValid(vcpu, vmcs_read32(VM_INSTRUCTION_ERROR));
 	} else
 		nested_vmx_succeed(vcpu);
+	if (enable_shadow_vmcs)
+		vmx->nested.sync_shadow_vmcs = true;
 }
 
 /*

@@ -7949,6 +7979,8 @@ static void nested_vmx_entry_failure(str
 	vmcs12->vm_exit_reason = reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
 	vmcs12->exit_qualification = qualification;
 	nested_vmx_succeed(vcpu);
+	if (enable_shadow_vmcs)
+		to_vmx(vcpu)->nested.sync_shadow_vmcs = true;
 }
 
 static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 
--
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

--
Gleb.

author
#7 lore raw

Re: [PATCH 05/10] KVM: nVMX: Allocate shadow vmcs

In reply to Gleb Natapov

Gleb Natapov <[email protected]> wrote on 18/04/2013 09:38:43 AM:

Quoted 2 lines · Abel Gordon
Quoted 1 line

Allocate a shadow vmcs used by the processor to shadow part of the

fields

Quoted 1 line
Quoted 1 line

stored in the software defined VMCS12 (let L1 access fields without

causing

Quoted 38 lines
Quoted 1 line

exits). Note we keep a shadow vmcs only for the current vmcs12.

Once a vmcs12

Quoted 36 lines

becomes non-current, its shadow vmcs is released.

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 +8 -0
@@ -355,6 +355,7 @@ struct nested_vmx {
    /* The host-usable pointer to the above */
    struct page *current_vmcs12_page;
    struct vmcs12 *current_vmcs12;
+   struct vmcs *current_shadow_vmcs;
 
    /* vmcs02_list cache of VMCSs recently used to run L2 guests */
    struct list_head vmcs02_pool;

@@ -5517,6 +5518,7 @@ static int handle_vmon(struct kvm_vcpu *
 {
    struct kvm_segment cs;
    struct vcpu_vmx *vmx = to_vmx(vcpu);
+   struct vmcs *shadow_vmcs;
 
    /* The Intel VMX Instruction Reference lists a bunch of bits that
     * are prerequisite to running VMXON, most notably cr4.VMXE must be

@@ -5540,6 +5542,16 @@ static int handle_vmon(struct kvm_vcpu *
       kvm_inject_gp(vcpu, 0);
       return 1;
    }
+   if (enable_shadow_vmcs) {
+      shadow_vmcs = alloc_vmcs();
+      if (!shadow_vmcs)
+         return -ENOMEM;
+      /* mark vmcs as shadow */
+      shadow_vmcs->revision_id |= (1u << 31);
Quoted 8 lines

arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 static int handle_vmon(struct kvm_vcpu * · line 5555

Quoted 5 lines
arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 +4 -0
+      /* init shadow vmcs */
+      vmcs_clear(shadow_vmcs);
+      vmx->nested.current_shadow_vmcs = shadow_vmcs;
+   }

Guest can ddos host by calling vmxon repeatedly causing host to leak memory. This point to a bug in vmxon implementation. vmxon should call nested_vmx_failInvalid() if (vmx->nested.vmxon).

Good point. I just checked the spec (VMXON pseudo-code) to verify the right emulation:
According to the pseudo-code we should:
ELSE VMfail(“VMXON executed in VMX root operation”) which means:

VMfail(ErrorNumber):
IF VMCS pointer is valid
THEN VMfailValid(ErrorNumber);
ELSE VMfailInvalid;
FI;

So, I'll call nested_vmx_failValid if nested.current_vmptr != -1ull Otherwise, I'll call nested_vmx_failInvalid.

author
#15 lore raw

Re: [PATCH 09/10] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs

In reply to Gleb Natapov

Gleb Natapov <[email protected]> wrote on 18/04/2013 09:41:16 AM:

Quoted 48 lines · Abel Gordon
Quoted 47 lines

Synchronize between the VMCS12 software controlled structure and the processor-specific shadow vmcs

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 +17 -0
@@ -356,6 +356,11 @@ struct nested_vmx {
    struct page *current_vmcs12_page;
    struct vmcs12 *current_vmcs12;
    struct vmcs *current_shadow_vmcs;
+   /*
+    * Indicates if the shadow vmcs must be updated with the
+    * data hold by vmcs12
+    */
+   bool sync_shadow_vmcs;
 
    /* vmcs02_list cache of VMCSs recently used to run L2 guests */
    struct list_head vmcs02_pool;

@@ -5596,6 +5601,14 @@ static int nested_vmx_check_permission(s
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
+   if (enable_shadow_vmcs) {
+      if (vmx->nested.current_vmcs12 != NULL) {
+         /* copy to memory all shadowed fields in case
+            they were modified */
+         copy_shadow_to_vmcs12(vmx);
+         vmx->nested.sync_shadow_vmcs = false;
+      }
+   }
    kunmap(vmx->nested.current_vmcs12_page);
    nested_release_page(vmx->nested.current_vmcs12_page);
 }

@@ -5724,6 +5737,10 @@ static void nested_vmx_failValid(struct
              X86_EFLAGS_SF | X86_EFLAGS_OF))
          | X86_EFLAGS_ZF);
    get_vmcs12(vcpu)->vm_instruction_error = vm_instruction_error;
+   /*
+    * We don't need to force a shadow sync because
+    * VM_INSTRUCTION_ERROR is not shadowed
+    */
 }
Quoted 9 lines

arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 static int handle_vmptrld(struct kvm_vcp · line 6144

Quoted 8 lines
arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 +3 -0
 /* Emulate the VMCLEAR instruction */

@@ -6122,6 +6139,9 @@ static int handle_vmptrld(struct kvm_vcp
       vmx->nested.current_vmptr = vmptr;
       vmx->nested.current_vmcs12 = new_vmcs12;
       vmx->nested.current_vmcs12_page = page;
+      if (enable_shadow_vmcs) {
+         vmx->nested.sync_shadow_vmcs = true;
+      }

No need braces around one statement according to kernel coding style.

The last patch introduces code within the braces... I usually prefer to add them in advance so you don't see the "if" statement as modified in future patches. If this is not a common practice, I'll remove the braces here and add them again later. Please confirm.

#16 lore raw

Re: [PATCH 09/10] KVM: nVMX: Synchronize VMCS12 content with the shadow vmcs

Quoted 63 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 18/04/2013 09:41:16 AM:

Quoted 48 lines · Abel Gordon
Quoted 47 lines

Synchronize between the VMCS12 software controlled structure and the processor-specific shadow vmcs

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 +17 -0
@@ -356,6 +356,11 @@ struct nested_vmx {
    struct page *current_vmcs12_page;
    struct vmcs12 *current_vmcs12;
    struct vmcs *current_shadow_vmcs;
+   /*
+    * Indicates if the shadow vmcs must be updated with the
+    * data hold by vmcs12
+    */
+   bool sync_shadow_vmcs;
 
    /* vmcs02_list cache of VMCSs recently used to run L2 guests */
    struct list_head vmcs02_pool;

@@ -5596,6 +5601,14 @@ static int nested_vmx_check_permission(s
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
+   if (enable_shadow_vmcs) {
+      if (vmx->nested.current_vmcs12 != NULL) {
+         /* copy to memory all shadowed fields in case
+            they were modified */
+         copy_shadow_to_vmcs12(vmx);
+         vmx->nested.sync_shadow_vmcs = false;
+      }
+   }
    kunmap(vmx->nested.current_vmcs12_page);
    nested_release_page(vmx->nested.current_vmcs12_page);
 }

@@ -5724,6 +5737,10 @@ static void nested_vmx_failValid(struct
              X86_EFLAGS_SF | X86_EFLAGS_OF))
          | X86_EFLAGS_ZF);
    get_vmcs12(vcpu)->vm_instruction_error = vm_instruction_error;
+   /*
+    * We don't need to force a shadow sync because
+    * VM_INSTRUCTION_ERROR is not shadowed
+    */
 }
Quoted 9 lines

arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 static int handle_vmptrld(struct kvm_vcp · line 6144

Quoted 8 lines
arch/x86/kvm/vmx.c 2013-04-17 19:58:33.000000000 +0300 +3 -0
 /* Emulate the VMCLEAR instruction */

@@ -6122,6 +6139,9 @@ static int handle_vmptrld(struct kvm_vcp
       vmx->nested.current_vmptr = vmptr;
       vmx->nested.current_vmcs12 = new_vmcs12;
       vmx->nested.current_vmcs12_page = page;
+      if (enable_shadow_vmcs) {
+         vmx->nested.sync_shadow_vmcs = true;
+      }

No need braces around one statement according to kernel coding style.

Quoted 4 lines

The last patch introduces code within the braces... I usually prefer to add them in advance so you don't see the "if" statement as modified in future patches. If this is not a common practice, I'll remove the braces here and add them again later. Please confirm.

Ah, I haven't noticed that the later patch adds code here. In this case the braces a welcome.

--
Gleb.

#8 lore raw

Re: [PATCH 05/10] KVM: nVMX: Allocate shadow vmcs

In reply to Abel Gordon
Quoted 68 lines · Abel Gordon

Gleb Natapov <[email protected]> wrote on 18/04/2013 09:38:43 AM:

Quoted 2 lines · Abel Gordon
Quoted 1 line

Allocate a shadow vmcs used by the processor to shadow part of the

fields

Quoted 1 line
Quoted 1 line

stored in the software defined VMCS12 (let L1 access fields without

causing

Quoted 38 lines
Quoted 1 line

exits). Note we keep a shadow vmcs only for the current vmcs12.

Once a vmcs12

Quoted 36 lines

becomes non-current, its shadow vmcs is released.

Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 +8 -0
@@ -355,6 +355,7 @@ struct nested_vmx {
    /* The host-usable pointer to the above */
    struct page *current_vmcs12_page;
    struct vmcs12 *current_vmcs12;
+   struct vmcs *current_shadow_vmcs;
 
    /* vmcs02_list cache of VMCSs recently used to run L2 guests */
    struct list_head vmcs02_pool;

@@ -5517,6 +5518,7 @@ static int handle_vmon(struct kvm_vcpu *
 {
    struct kvm_segment cs;
    struct vcpu_vmx *vmx = to_vmx(vcpu);
+   struct vmcs *shadow_vmcs;
 
    /* The Intel VMX Instruction Reference lists a bunch of bits that
     * are prerequisite to running VMXON, most notably cr4.VMXE must be

@@ -5540,6 +5542,16 @@ static int handle_vmon(struct kvm_vcpu *
       kvm_inject_gp(vcpu, 0);
       return 1;
    }
+   if (enable_shadow_vmcs) {
+      shadow_vmcs = alloc_vmcs();
+      if (!shadow_vmcs)
+         return -ENOMEM;
+      /* mark vmcs as shadow */
+      shadow_vmcs->revision_id |= (1u << 31);
Quoted 8 lines

arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 static int handle_vmon(struct kvm_vcpu * · line 5555

Quoted 5 lines
arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300 +4 -0
+      /* init shadow vmcs */
+      vmcs_clear(shadow_vmcs);
+      vmx->nested.current_shadow_vmcs = shadow_vmcs;
+   }

Guest can ddos host by calling vmxon repeatedly causing host to leak memory. This point to a bug in vmxon implementation. vmxon should call nested_vmx_failInvalid() if (vmx->nested.vmxon).

Good point. I just checked the spec (VMXON pseudo-code) to verify the right emulation:
According to the pseudo-code we should:
ELSE VMfail(“VMXON executed in VMX root operation”) which means:

VMfail(ErrorNumber):
IF VMCS pointer is valid
THEN VMfailValid(ErrorNumber);
ELSE VMfailInvalid;
FI;

Quoted 3 lines

So, I'll call nested_vmx_failValid if nested.current_vmptr != -1ull Otherwise, I'll call nested_vmx_failInvalid.

Just call nested_vmx_failValid(). It does that internally.

--
Gleb.

author
#9 lore raw

Re: [PATCH 05/10] KVM: nVMX: Allocate shadow vmcs

Gleb Natapov <[email protected]> wrote on 18/04/2013 10:11:22 AM:

Quoted 15 lines · Abel Gordon
Quoted 14 lines

Gleb Natapov <[email protected]> wrote on 18/04/2013 09:38:43 AM:

Quoted 2 lines · Abel Gordon
Quoted 1 line

Allocate a shadow vmcs used by the processor to shadow part of the

fields

Quoted 1 line
Quoted 1 line

stored in the software defined VMCS12 (let L1 access fields without

causing

Quoted 5 lines
Quoted 1 line

exits). Note we keep a shadow vmcs only for the current vmcs12.

Once a vmcs12

Quoted 3 lines

becomes non-current, its shadow vmcs is released.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Abel Gordon <[email protected]>
---
 arch/x86/kvm/vmx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

--- .before/arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000

+0300

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines

+++ .after/arch/x86/kvm/vmx.c 2013-04-17 19:58:32.000000000 +0300

arch/x86/kvm/vmx.c +2 -0
@@ -355,6 +355,7 @@ struct nested_vmx {
    /* The host-usable pointer to the above */
    struct page *current_vmcs12_page;
    struct vmcs12 *current_vmcs12;
+   struct vmcs *current_shadow_vmcs;
 
    /* vmcs02_list cache of VMCSs recently used to run L2 guests */
    struct list_head vmcs02_pool;

@@ -5517,6 +5518,7 @@ static int handle_vmon(struct kvm_vcpu *
 {
    struct kvm_segment cs;
    struct vcpu_vmx *vmx = to_vmx(vcpu);
+   struct vmcs *shadow_vmcs;
Quoted 1 line
Quoted 1 line
Quoted 1 line

Review static int handle_vmon(struct kvm_vcpu * · line 5523

Quoted 1 line
(quoted diff) +0 -0
    /* The Intel VMX Instruction Reference lists a bunch of bits

that

Quoted 1 line
Quoted 1 line
Quoted 1 line

Review static int handle_vmon(struct kvm_vcpu * · line 5524

Quoted 1 line
(quoted diff) +0 -0
     * are prerequisite to running VMXON, most notably cr4.VMXE

must be

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
arch/x86/kvm/vmx.c +5 -0
@@ -5540,6 +5542,16 @@ static int handle_vmon(struct kvm_vcpu *
       kvm_inject_gp(vcpu, 0);
       return 1;
    }
+   if (enable_shadow_vmcs) {
+      shadow_vmcs = alloc_vmcs();
+      if (!shadow_vmcs)
+         return -ENOMEM;
+      /* mark vmcs as shadow */
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

arch/x86/kvm/vmx.c static int handle_vmon(struct kvm_vcpu * · line 5555

Quoted 6 lines
arch/x86/kvm/vmx.c +5 -0
+      shadow_vmcs->revision_id |= (1u << 31);
+      /* init shadow vmcs */
+      vmcs_clear(shadow_vmcs);
+      vmx->nested.current_shadow_vmcs = shadow_vmcs;
+   }

Guest can ddos host by calling vmxon repeatedly causing host to leak memory. This point to a bug in vmxon implementation. vmxon should

call

Quoted 14 lines
Quoted 14 lines
Quoted 1 line

nested_vmx_failInvalid() if (vmx->nested.vmxon).

Good point. I just checked the spec (VMXON pseudo-code) to verify the right emulation:
According to the pseudo-code we should:
ELSE VMfail(“VMXON executed in VMX root operation”) which means:

VMfail(ErrorNumber):
IF VMCS pointer is valid
THEN VMfailValid(ErrorNumber);
ELSE VMfailInvalid;
FI;

Quoted 4 lines
Quoted 3 lines

So, I'll call nested_vmx_failValid if nested.current_vmptr != -1ull Otherwise, I'll call nested_vmx_failInvalid.

Just call nested_vmx_failValid(). It does that internally.

Right, forgot that part :)