[PATCH]Abstract vmcs feature detect part

[PATCH]Abstract vmcs feature detect part

[Yang, Sheng]

[-- Attachment #1: Type: text/plain, Size: 6592 bytes --]

This patch changes a method to check cpu capability, so we can set vmcs
more conveniently.

Signed-off-by: Sheng Yang (sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org)
--
Index: kvm/drivers/kvm/vmx.c
===================================================================
--- kvm.orig/drivers/kvm/vmx.c	2007-07-25 16:41:38.000000000 +0800
+++ kvm/drivers/kvm/vmx.c	2007-07-25 17:58:45.000000000 +0800
@@ -47,11 +47,15 @@
 #endif
 #define EFER_SAVE_RESTORE_BITS ((u64)EFER_SCE)
 
-static struct vmcs_descriptor {
+static struct vmcs_config {
 	int size;
 	int order;
 	u32 revision_id;
-} vmcs_descriptor;
+	u32 pin_based_exec_ctrl;
+	u32 cpu_based_exec_ctrl;
+	u32 vmexit_ctrl;
+	u32 vmentry_ctrl;
+} vmcs_config;
 
 #define VMX_SEGMENT_FIELD(seg)					\
 	[VCPU_SREG_##seg] = {                                   \
@@ -782,14 +786,96 @@
 	asm volatile (ASM_VMX_VMXOFF : : : "cc");
 }
 
-static __init void setup_vmcs_descriptor(void)
+static __init u32 adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32
msr)
+{
+    u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
+
+    rdmsr(msr, vmx_msr_low, vmx_msr_high);
+
+    ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
+    ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
+
+    /* Ensure minimum (required) set of control bits are supported. */
+    if (ctl_min & ~ctl) return 0;
+
+    return ctl;
+}
+
+static __init int setup_vmcs_config(void)
 {
 	u32 vmx_msr_low, vmx_msr_high;
+	u32 min, opt;
+	u32 _pin_based_exec_control = 0;
+	u32 _cpu_based_exec_control = 0;
+	u32 _vmexit_control = 0;
+	u32 _vmentry_control = 0;
+
+	min = (PIN_BASED_EXT_INTR_MASK |
+		   PIN_BASED_NMI_EXITING);
+	opt = 0;
+	_pin_based_exec_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_PINBASED_CTLS);
+	if (_pin_based_exec_control == 0) {
+	    return -1;
+	}
+
+	min = (CPU_BASED_HLT_EXITING
+		| CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
+		| CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
+		| CPU_BASED_USE_IO_BITMAPS      /* 20.6.2 */
+		| CPU_BASED_MOV_DR_EXITING
+		| CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
+	      );
+	opt = 0;
+	_cpu_based_exec_control = adjust_vmx_controls(
+		min, opt, MSR_IA32_VMX_PROCBASED_CTLS);
+	if (_cpu_based_exec_control == 0) {
+	    return -1;
+	}
+
+	min = VM_EXIT_HOST_ADDR_SPACE_SIZE;
+	opt = 0;
+	_vmexit_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_EXIT_CTLS);
+	if (_vmexit_control == 0) {
+	    return -1;
+	}
+
+	min = opt = 0;
+	_vmentry_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_ENTRY_CTLS);
+	if (_vmentry_control == 0) {
+	    return -1;
+	}
 
 	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
-	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
-	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
-	vmcs_descriptor.revision_id = vmx_msr_low;
+
+	vmcs_config.size = vmx_msr_high & 0x1fff;
+	vmcs_config.order = get_order(vmcs_config.size);
+	vmcs_config.revision_id = vmx_msr_low;
+
+	vmcs_config.pin_based_exec_ctrl = _pin_based_exec_control;
+	vmcs_config.cpu_based_exec_ctrl = _cpu_based_exec_control;
+	vmcs_config.vmexit_ctrl         = _vmexit_control;
+	vmcs_config.vmentry_ctrl        = _vmentry_control;
+
+	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
+	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE) {
+	    return -1;
+	}
+
+#ifdef __x86_64__
+	/* IA-32 SDM Vol 3B: 64-bit CPUs always have
VMX_BASIC_MSR[48]==0. */
+	if (vmx_msr_high & (1u<<16)) {
+	    return -1;
+	}
+#endif
+
+	/* Require Write-Back (WB) memory type for VMCS accesses. */
+	if (((vmx_msr_high >> 18) & 15) != 6) {
+	    return -1;
+	}
+	return 0;
 }
 
 static struct vmcs *alloc_vmcs_cpu(int cpu)
@@ -798,12 +884,12 @@
 	struct page *pages;
 	struct vmcs *vmcs;
 
-	pages = alloc_pages_node(node, GFP_KERNEL,
vmcs_descriptor.order);
+	pages = alloc_pages_node(node, GFP_KERNEL, vmcs_config.order);
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
-	memset(vmcs, 0, vmcs_descriptor.size);
-	vmcs->revision_id = vmcs_descriptor.revision_id; /* vmcs
revision id */
+	memset(vmcs, 0, vmcs_config.size);
+	vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id
*/
 	return vmcs;
 }
 
@@ -814,7 +900,7 @@
 
 static void free_vmcs(struct vmcs *vmcs)
 {
-	free_pages((unsigned long)vmcs, vmcs_descriptor.order);
+	free_pages((unsigned long)vmcs, vmcs_config.order);
 }
 
 static void free_kvm_area(void)
@@ -847,7 +933,9 @@
 
 static __init int hardware_setup(void)
 {
-	setup_vmcs_descriptor();
+	if (setup_vmcs_config() < 0) {
+	    return -1;
+	}
 	return alloc_kvm_area();
 }
 
@@ -1218,17 +1306,6 @@
 	return 1;
 }
 
-static void vmcs_write32_fixedbits(u32 msr, u32 vmcs_field, u32 val)
-{
-	u32 msr_high, msr_low;
-
-	rdmsr(msr, msr_low, msr_high);
-
-	val &= msr_high;
-	val |= msr_low;
-	vmcs_write32(vmcs_field, val);
-}
-
 static void seg_setup(int seg)
 {
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
@@ -1324,20 +1401,10 @@
 	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
 
 	/* Control */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PINBASED_CTLS,
-			       PIN_BASED_VM_EXEC_CONTROL,
-			       PIN_BASED_EXT_INTR_MASK   /* 20.6.1 */
-			       | PIN_BASED_NMI_EXITING   /* 20.6.1 */
-			);
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PROCBASED_CTLS,
-			       CPU_BASED_VM_EXEC_CONTROL,
-			       CPU_BASED_HLT_EXITING         /* 20.6.2
*/
-			       | CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2
*/
-			       | CPU_BASED_CR8_STORE_EXITING   /* 20.6.2
*/
-			       | CPU_BASED_USE_IO_BITMAPS  /* 20.6.2 */
-			       | CPU_BASED_MOV_DR_EXITING
-			       | CPU_BASED_USE_TSC_OFFSETING   /* 21.3
*/
-			);
+	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL,
+		vmcs_config.pin_based_exec_ctrl);
+	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
+		vmcs_config.cpu_based_exec_ctrl);
 
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0);
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0);
@@ -1401,12 +1468,11 @@
 
 	setup_msrs(vcpu);
 
-	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS, VM_EXIT_CONTROLS,
-		     	       (HOST_IS_64 << 9));  /* 22.2,1, 20.7.1 */
+	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
 
 	/* 22.2.1, 20.8.1 */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_ENTRY_CTLS,
-                               VM_ENTRY_CONTROLS, 0);
+	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
+
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 
 #ifdef CONFIG_X86_64

[-- Attachment #2: vmcs_config.patch --]
[-- Type: application/octet-stream, Size: 6186 bytes --]

Index: kvm/drivers/kvm/vmx.c
===================================================================
--- kvm.orig/drivers/kvm/vmx.c	2007-07-25 16:41:38.000000000 +0800
+++ kvm/drivers/kvm/vmx.c	2007-07-25 17:58:45.000000000 +0800
@@ -47,11 +47,15 @@
 #endif
 #define EFER_SAVE_RESTORE_BITS ((u64)EFER_SCE)
 
-static struct vmcs_descriptor {
+static struct vmcs_config {
 	int size;
 	int order;
 	u32 revision_id;
-} vmcs_descriptor;
+	u32 pin_based_exec_ctrl;
+	u32 cpu_based_exec_ctrl;
+	u32 vmexit_ctrl;
+	u32 vmentry_ctrl;
+} vmcs_config;
 
 #define VMX_SEGMENT_FIELD(seg)					\
 	[VCPU_SREG_##seg] = {                                   \
@@ -782,14 +786,96 @@
 	asm volatile (ASM_VMX_VMXOFF : : : "cc");
 }
 
-static __init void setup_vmcs_descriptor(void)
+static __init u32 adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32 msr)
+{
+    u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
+
+    rdmsr(msr, vmx_msr_low, vmx_msr_high);
+
+    ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
+    ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
+
+    /* Ensure minimum (required) set of control bits are supported. */
+    if (ctl_min & ~ctl) return 0;
+
+    return ctl;
+}
+
+static __init int setup_vmcs_config(void)
 {
 	u32 vmx_msr_low, vmx_msr_high;
+	u32 min, opt;
+	u32 _pin_based_exec_control = 0;
+	u32 _cpu_based_exec_control = 0;
+	u32 _vmexit_control = 0;
+	u32 _vmentry_control = 0;
+
+	min = (PIN_BASED_EXT_INTR_MASK |
+		   PIN_BASED_NMI_EXITING);
+	opt = 0;
+	_pin_based_exec_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_PINBASED_CTLS);
+	if (_pin_based_exec_control == 0) {
+	    return -1;
+	}
+
+	min = (CPU_BASED_HLT_EXITING
+		| CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
+		| CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
+		| CPU_BASED_USE_IO_BITMAPS      /* 20.6.2 */
+		| CPU_BASED_MOV_DR_EXITING
+		| CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
+	      );
+	opt = 0;
+	_cpu_based_exec_control = adjust_vmx_controls(
+		min, opt, MSR_IA32_VMX_PROCBASED_CTLS);
+	if (_cpu_based_exec_control == 0) {
+	    return -1;
+	}
+
+	min = VM_EXIT_HOST_ADDR_SPACE_SIZE;
+	opt = 0;
+	_vmexit_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_EXIT_CTLS);
+	if (_vmexit_control == 0) {
+	    return -1;
+	}
+
+	min = opt = 0;
+	_vmentry_control = adjust_vmx_controls(
+			min, opt, MSR_IA32_VMX_ENTRY_CTLS);
+	if (_vmentry_control == 0) {
+	    return -1;
+	}
 
 	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
-	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
-	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
-	vmcs_descriptor.revision_id = vmx_msr_low;
+
+	vmcs_config.size = vmx_msr_high & 0x1fff;
+	vmcs_config.order = get_order(vmcs_config.size);
+	vmcs_config.revision_id = vmx_msr_low;
+
+	vmcs_config.pin_based_exec_ctrl = _pin_based_exec_control;
+	vmcs_config.cpu_based_exec_ctrl = _cpu_based_exec_control;
+	vmcs_config.vmexit_ctrl         = _vmexit_control;
+	vmcs_config.vmentry_ctrl        = _vmentry_control;
+
+	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
+	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE) {
+	    return -1;
+	}
+
+#ifdef __x86_64__
+	/* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
+	if (vmx_msr_high & (1u<<16)) {
+	    return -1;
+	}
+#endif
+
+	/* Require Write-Back (WB) memory type for VMCS accesses. */
+	if (((vmx_msr_high >> 18) & 15) != 6) {
+	    return -1;
+	}
+	return 0;
 }
 
 static struct vmcs *alloc_vmcs_cpu(int cpu)
@@ -798,12 +884,12 @@
 	struct page *pages;
 	struct vmcs *vmcs;
 
-	pages = alloc_pages_node(node, GFP_KERNEL, vmcs_descriptor.order);
+	pages = alloc_pages_node(node, GFP_KERNEL, vmcs_config.order);
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
-	memset(vmcs, 0, vmcs_descriptor.size);
-	vmcs->revision_id = vmcs_descriptor.revision_id; /* vmcs revision id */
+	memset(vmcs, 0, vmcs_config.size);
+	vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */
 	return vmcs;
 }
 
@@ -814,7 +900,7 @@
 
 static void free_vmcs(struct vmcs *vmcs)
 {
-	free_pages((unsigned long)vmcs, vmcs_descriptor.order);
+	free_pages((unsigned long)vmcs, vmcs_config.order);
 }
 
 static void free_kvm_area(void)
@@ -847,7 +933,9 @@
 
 static __init int hardware_setup(void)
 {
-	setup_vmcs_descriptor();
+	if (setup_vmcs_config() < 0) {
+	    return -1;
+	}
 	return alloc_kvm_area();
 }
 
@@ -1218,17 +1306,6 @@
 	return 1;
 }
 
-static void vmcs_write32_fixedbits(u32 msr, u32 vmcs_field, u32 val)
-{
-	u32 msr_high, msr_low;
-
-	rdmsr(msr, msr_low, msr_high);
-
-	val &= msr_high;
-	val |= msr_low;
-	vmcs_write32(vmcs_field, val);
-}
-
 static void seg_setup(int seg)
 {
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
@@ -1324,20 +1401,10 @@
 	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
 
 	/* Control */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PINBASED_CTLS,
-			       PIN_BASED_VM_EXEC_CONTROL,
-			       PIN_BASED_EXT_INTR_MASK   /* 20.6.1 */
-			       | PIN_BASED_NMI_EXITING   /* 20.6.1 */
-			);
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PROCBASED_CTLS,
-			       CPU_BASED_VM_EXEC_CONTROL,
-			       CPU_BASED_HLT_EXITING         /* 20.6.2 */
-			       | CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
-			       | CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
-			       | CPU_BASED_USE_IO_BITMAPS  /* 20.6.2 */
-			       | CPU_BASED_MOV_DR_EXITING
-			       | CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
-			);
+	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL,
+		vmcs_config.pin_based_exec_ctrl);
+	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
+		vmcs_config.cpu_based_exec_ctrl);
 
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0);
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0);
@@ -1401,12 +1468,11 @@
 
 	setup_msrs(vcpu);
 
-	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS, VM_EXIT_CONTROLS,
-		     	       (HOST_IS_64 << 9));  /* 22.2,1, 20.7.1 */
+	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
 
 	/* 22.2.1, 20.8.1 */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_ENTRY_CTLS,
-                               VM_ENTRY_CONTROLS, 0);
+	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
+
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 
 #ifdef CONFIG_X86_64

[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]Abstract vmcs feature detect part

[Avi Kivity]

Yang, Sheng wrote:
> This patch changes a method to check cpu capability, so we can set vmcs
> more conveniently.
>
>   

Please explain the motivation for the change.  What is more convenient?

>  
> -static __init void setup_vmcs_descriptor(void)
> +static __init u32 adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32
> msr)
> +{
> +    u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
>   

Tabs not spaces for indentation.

If you initialize a variable during definition, put it on its own line.

> +
> +    rdmsr(msr, vmx_msr_low, vmx_msr_high);
> +
> +    ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
> +    ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
> +
> +    /* Ensure minimum (required) set of control bits are supported. */
> +    if (ctl_min & ~ctl) return 0;
>   

No single-line ifs, please.

> +
> +    return ctl;
> +}
> +
>   

This replaces vmcs_write32_fixedbits().  Why?  And if it is necessary,
it should be in a separate patch.

> +static __init int setup_vmcs_config(void)
>  {
>  	u32 vmx_msr_low, vmx_msr_high;
> +	u32 min, opt;
> +	u32 _pin_based_exec_control = 0;
> +	u32 _cpu_based_exec_control = 0;
> +	u32 _vmexit_control = 0;
> +	u32 _vmentry_control = 0;
> +
> +	min = (PIN_BASED_EXT_INTR_MASK |
> +		   PIN_BASED_NMI_EXITING);
>   

While we're limited to 80 columns, there's no reason to stop at 40.

> +	opt = 0;
> +	_pin_based_exec_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_PINBASED_CTLS);
> +	if (_pin_based_exec_control == 0) {
> +	    return -1;
> +	}
>   

Single statement if () bodies should be without braces.

> +
> +	min = (CPU_BASED_HLT_EXITING
> +		| CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
> +		| CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
> +		| CPU_BASED_USE_IO_BITMAPS      /* 20.6.2 */
> +		| CPU_BASED_MOV_DR_EXITING
> +		| CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
> +	      );
> +	opt = 0;
> +	_cpu_based_exec_control = adjust_vmx_controls(
> +		min, opt, MSR_IA32_VMX_PROCBASED_CTLS);
> +	if (_cpu_based_exec_control == 0) {
> +	    return -1;
> +	}
> +
> +	min = VM_EXIT_HOST_ADDR_SPACE_SIZE;
> +	opt = 0;
> +	_vmexit_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_EXIT_CTLS);
> +	if (_vmexit_control == 0) {
> +	    return -1;
> +	}
> +
> +	min = opt = 0;
> +	_vmentry_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_ENTRY_CTLS);
> +	if (_vmentry_control == 0) {
> +	    return -1;
> +	}
>  
>  	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
> -	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
> -	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
> -	vmcs_descriptor.revision_id = vmx_msr_low;
> +
> +	vmcs_config.size = vmx_msr_high & 0x1fff;
> +	vmcs_config.order = get_order(vmcs_config.size);
> +	vmcs_config.revision_id = vmx_msr_low;
> +
> +	vmcs_config.pin_based_exec_ctrl = _pin_based_exec_control;
> +	vmcs_config.cpu_based_exec_ctrl = _cpu_based_exec_control;
> +	vmcs_config.vmexit_ctrl         = _vmexit_control;
> +	vmcs_config.vmentry_ctrl        = _vmentry_control;
> +
> +	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
> +	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE) {
> +	    return -1;
> +	}
> +
> +#ifdef __x86_64__
> +	/* IA-32 SDM Vol 3B: 64-bit CPUs always have
> VMX_BASIC_MSR[48]==0. */
> +	if (vmx_msr_high & (1u<<16)) {
> +	    return -1;
> +	}
> +#endif
> +
> +	/* Require Write-Back (WB) memory type for VMCS accesses. */
> +	if (((vmx_msr_high >> 18) & 15) != 6) {
> +	    return -1;
> +	}
> +	return 0;
>  }
>   

This looks like a rewrite.  Please make patches incremental.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH]Abstract vmcs feature detect part

[Yang, Sheng]

[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]

Sorry for not explain clearly.

This patch replaces vmcs_write32_fixedbits() with adjust_vmx_controls(), and add some relevant fields to vmcs_config for the global using. 

Put vmcs situation in global variable enables us using it to check current vmcs condition and deal with different types of CPU, for we may add some feature which is not supported by all types of CPU. And adjust_vmx_controls() also offers a optional(all filled by 0 now, but will be extended after) feature test . In the future, we can decide how VMCS would be filled by detecting CPU's capability in setup_vmcs_config(). 

I attached modified patch. If you need more information, please let me know.

Thanks
 
Yang, Sheng

-----Original Message-----
From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org] 
Sent: 2007年7月26日 13:20
To: Yang, Sheng
Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: Re: [kvm-devel] [PATCH]Abstract vmcs feature detect part

Yang, Sheng wrote:
> This patch changes a method to check cpu capability, so we can set vmcs
> more conveniently.
>
>   

Please explain the motivation for the change.  What is more convenient?

>  
> -static __init void setup_vmcs_descriptor(void)
> +static __init u32 adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32
> msr)
> +{
> +    u32 vmx_msr_low, vmx_msr_high, ctl = ctl_min | ctl_opt;
>   

Tabs not spaces for indentation.

If you initialize a variable during definition, put it on its own line.

> +
> +    rdmsr(msr, vmx_msr_low, vmx_msr_high);
> +
> +    ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
> +    ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
> +
> +    /* Ensure minimum (required) set of control bits are supported. */
> +    if (ctl_min & ~ctl) return 0;
>   

No single-line ifs, please.

> +
> +    return ctl;
> +}
> +
>   

This replaces vmcs_write32_fixedbits().  Why?  And if it is necessary,
it should be in a separate patch.

> +static __init int setup_vmcs_config(void)
>  {
>  	u32 vmx_msr_low, vmx_msr_high;
> +	u32 min, opt;
> +	u32 _pin_based_exec_control = 0;
> +	u32 _cpu_based_exec_control = 0;
> +	u32 _vmexit_control = 0;
> +	u32 _vmentry_control = 0;
> +
> +	min = (PIN_BASED_EXT_INTR_MASK |
> +		   PIN_BASED_NMI_EXITING);
>   

While we're limited to 80 columns, there's no reason to stop at 40.

> +	opt = 0;
> +	_pin_based_exec_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_PINBASED_CTLS);
> +	if (_pin_based_exec_control == 0) {
> +	    return -1;
> +	}
>   

Single statement if () bodies should be without braces.

> +
> +	min = (CPU_BASED_HLT_EXITING
> +		| CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
> +		| CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
> +		| CPU_BASED_USE_IO_BITMAPS      /* 20.6.2 */
> +		| CPU_BASED_MOV_DR_EXITING
> +		| CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
> +	      );
> +	opt = 0;
> +	_cpu_based_exec_control = adjust_vmx_controls(
> +		min, opt, MSR_IA32_VMX_PROCBASED_CTLS);
> +	if (_cpu_based_exec_control == 0) {
> +	    return -1;
> +	}
> +
> +	min = VM_EXIT_HOST_ADDR_SPACE_SIZE;
> +	opt = 0;
> +	_vmexit_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_EXIT_CTLS);
> +	if (_vmexit_control == 0) {
> +	    return -1;
> +	}
> +
> +	min = opt = 0;
> +	_vmentry_control = adjust_vmx_controls(
> +			min, opt, MSR_IA32_VMX_ENTRY_CTLS);
> +	if (_vmentry_control == 0) {
> +	    return -1;
> +	}
>  
>  	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
> -	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
> -	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
> -	vmcs_descriptor.revision_id = vmx_msr_low;
> +
> +	vmcs_config.size = vmx_msr_high & 0x1fff;
> +	vmcs_config.order = get_order(vmcs_config.size);
> +	vmcs_config.revision_id = vmx_msr_low;
> +
> +	vmcs_config.pin_based_exec_ctrl = _pin_based_exec_control;
> +	vmcs_config.cpu_based_exec_ctrl = _cpu_based_exec_control;
> +	vmcs_config.vmexit_ctrl         = _vmexit_control;
> +	vmcs_config.vmentry_ctrl        = _vmentry_control;
> +
> +	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
> +	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE) {
> +	    return -1;
> +	}
> +
> +#ifdef __x86_64__
> +	/* IA-32 SDM Vol 3B: 64-bit CPUs always have
> VMX_BASIC_MSR[48]==0. */
> +	if (vmx_msr_high & (1u<<16)) {
> +	    return -1;
> +	}
> +#endif
> +
> +	/* Require Write-Back (WB) memory type for VMCS accesses. */
> +	if (((vmx_msr_high >> 18) & 15) != 6) {
> +	    return -1;
> +	}
> +	return 0;
>  }
>   

This looks like a rewrite.  Please make patches incremental.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[-- Attachment #2: vmcs_config.patch --]
[-- Type: application/octet-stream, Size: 6014 bytes --]

Index: kvm/drivers/kvm/vmx.c
===================================================================
--- kvm.orig/drivers/kvm/vmx.c	2007-07-25 16:41:38.000000000 +0800
+++ kvm/drivers/kvm/vmx.c	2007-07-26 13:24:59.000000000 +0800
@@ -47,11 +47,15 @@
 #endif
 #define EFER_SAVE_RESTORE_BITS ((u64)EFER_SCE)
 
-static struct vmcs_descriptor {
+static struct vmcs_config {
 	int size;
 	int order;
 	u32 revision_id;
-} vmcs_descriptor;
+	u32 pin_based_exec_ctrl;
+	u32 cpu_based_exec_ctrl;
+	u32 vmexit_ctrl;
+	u32 vmentry_ctrl;
+} vmcs_config;
 
 #define VMX_SEGMENT_FIELD(seg)					\
 	[VCPU_SREG_##seg] = {                                   \
@@ -782,14 +786,89 @@
 	asm volatile (ASM_VMX_VMXOFF : : : "cc");
 }
 
-static __init void setup_vmcs_descriptor(void)
+static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
+					u32 msr, u32* result)
 {
 	u32 vmx_msr_low, vmx_msr_high;
+	u32 ctl = ctl_min | ctl_opt;
+
+	rdmsr(msr, vmx_msr_low, vmx_msr_high);
+
+	ctl &= vmx_msr_high; /* bit == 0 in high word ==> must be zero */
+	ctl |= vmx_msr_low;  /* bit == 1 in low word  ==> must be one  */
+
+	/* Ensure minimum (required) set of control bits are supported. */
+	if (ctl_min & ~ctl)
+	    return -1;
+
+	*result = ctl;
+	return 0;
+}
+
+static __init int setup_vmcs_config(void)
+{
+	u32 vmx_msr_low, vmx_msr_high;
+	u32 min, opt;
+	u32 _pin_based_exec_control = 0;
+	u32 _cpu_based_exec_control = 0;
+	u32 _vmexit_control = 0;
+	u32 _vmentry_control = 0;
+
+	min = (PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING);
+	opt = 0;
+	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS,
+		    &_pin_based_exec_control) < 0)
+	    return -1;
+
+	min = (CPU_BASED_HLT_EXITING |
+		CPU_BASED_CR8_LOAD_EXITING |
+		CPU_BASED_CR8_STORE_EXITING |
+		CPU_BASED_USE_IO_BITMAPS |
+		CPU_BASED_MOV_DR_EXITING |
+		CPU_BASED_USE_TSC_OFFSETING);
+	opt = 0;
+	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PROCBASED_CTLS,
+		    &_cpu_based_exec_control) < 0)
+	    return -1;
+
+	min = VM_EXIT_HOST_ADDR_SPACE_SIZE;
+	opt = 0;
+	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
+		    &_vmexit_control) < 0)
+	    return -1;
+
+	min = opt = 0;
+	if (adjust_vmx_controls(min, opt,
+		    MSR_IA32_VMX_ENTRY_CTLS,
+		    &_vmentry_control) < 0)
+	    return -1;
 
 	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
-	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
-	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
-	vmcs_descriptor.revision_id = vmx_msr_low;
+
+	vmcs_config.size = vmx_msr_high & 0x1fff;
+	vmcs_config.order = get_order(vmcs_config.size);
+	vmcs_config.revision_id = vmx_msr_low;
+
+	vmcs_config.pin_based_exec_ctrl = _pin_based_exec_control;
+	vmcs_config.cpu_based_exec_ctrl = _cpu_based_exec_control;
+	vmcs_config.vmexit_ctrl         = _vmexit_control;
+	vmcs_config.vmentry_ctrl        = _vmentry_control;
+
+	/* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
+	if ((vmx_msr_high & 0x1fff) > PAGE_SIZE)
+	    return -1;
+
+#ifdef __x86_64__
+	/* IA-32 SDM Vol 3B: 64-bit CPUs always have VMX_BASIC_MSR[48]==0. */
+	if (vmx_msr_high & (1u<<16))
+	    return -1;
+#endif
+
+	/* Require Write-Back (WB) memory type for VMCS accesses. */
+	if (((vmx_msr_high >> 18) & 15) != 6)
+	    return -1;
+
+	return 0;
 }
 
 static struct vmcs *alloc_vmcs_cpu(int cpu)
@@ -798,12 +877,12 @@
 	struct page *pages;
 	struct vmcs *vmcs;
 
-	pages = alloc_pages_node(node, GFP_KERNEL, vmcs_descriptor.order);
+	pages = alloc_pages_node(node, GFP_KERNEL, vmcs_config.order);
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
-	memset(vmcs, 0, vmcs_descriptor.size);
-	vmcs->revision_id = vmcs_descriptor.revision_id; /* vmcs revision id */
+	memset(vmcs, 0, vmcs_config.size);
+	vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */
 	return vmcs;
 }
 
@@ -814,7 +893,7 @@
 
 static void free_vmcs(struct vmcs *vmcs)
 {
-	free_pages((unsigned long)vmcs, vmcs_descriptor.order);
+	free_pages((unsigned long)vmcs, vmcs_config.order);
 }
 
 static void free_kvm_area(void)
@@ -847,7 +926,9 @@
 
 static __init int hardware_setup(void)
 {
-	setup_vmcs_descriptor();
+	if (setup_vmcs_config() < 0) {
+	    return -1;
+	}
 	return alloc_kvm_area();
 }
 
@@ -1218,17 +1299,6 @@
 	return 1;
 }
 
-static void vmcs_write32_fixedbits(u32 msr, u32 vmcs_field, u32 val)
-{
-	u32 msr_high, msr_low;
-
-	rdmsr(msr, msr_low, msr_high);
-
-	val &= msr_high;
-	val |= msr_low;
-	vmcs_write32(vmcs_field, val);
-}
-
 static void seg_setup(int seg)
 {
 	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
@@ -1324,20 +1394,10 @@
 	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
 
 	/* Control */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PINBASED_CTLS,
-			       PIN_BASED_VM_EXEC_CONTROL,
-			       PIN_BASED_EXT_INTR_MASK   /* 20.6.1 */
-			       | PIN_BASED_NMI_EXITING   /* 20.6.1 */
-			);
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PROCBASED_CTLS,
-			       CPU_BASED_VM_EXEC_CONTROL,
-			       CPU_BASED_HLT_EXITING         /* 20.6.2 */
-			       | CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */
-			       | CPU_BASED_CR8_STORE_EXITING   /* 20.6.2 */
-			       | CPU_BASED_USE_IO_BITMAPS  /* 20.6.2 */
-			       | CPU_BASED_MOV_DR_EXITING
-			       | CPU_BASED_USE_TSC_OFFSETING   /* 21.3 */
-			);
+	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL,
+		vmcs_config.pin_based_exec_ctrl);
+	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL,
+		vmcs_config.cpu_based_exec_ctrl);
 
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, 0);
 	vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, 0);
@@ -1401,12 +1461,11 @@
 
 	setup_msrs(vcpu);
 
-	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS, VM_EXIT_CONTROLS,
-		     	       (HOST_IS_64 << 9));  /* 22.2,1, 20.7.1 */
+	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
 
 	/* 22.2.1, 20.8.1 */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_ENTRY_CTLS,
-                               VM_ENTRY_CONTROLS, 0);
+	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
+
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 
 #ifdef CONFIG_X86_64

[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]Abstract vmcs feature detect part

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

Yang, Sheng wrote:
> Sorry for not explain clearly.
>
> This patch replaces vmcs_write32_fixedbits() with adjust_vmx_controls(),

It still doesn't say why, but I can now see it: this is code from Xen. I
have no objection to copying Xen code, but it needs to be documented in
the changelog, and there needs to be a good reason as to why you're
replacing existing code rather than improving it. So far I don't see
this reason.

>  and add some relevant fields to vmcs_config for the global using. 
>
> Put vmcs situation in global variable enables us using it to check current vmcs condition and deal with different types of CPU, for we may add some feature which is not supported by all types of CPU. And adjust_vmx_controls() also offers a optional(all filled by 0 now, but will be extended after) feature test . In the future, we can decide how VMCS would be filled by detecting CPU's capability in setup_vmcs_config(). 
>
> I attached modified patch. If you need more information, please let me know.
>
>   

Please either use git send-email or paste patches as well as attaching
them so others can review too.

> Index: kvm/drivers/kvm/vmx.c
> ===================================================================
> --- kvm.orig/drivers/kvm/vmx.c 2007-07-25 16:41:38.000000000 +0800
> +++ kvm/drivers/kvm/vmx.c 2007-07-26 13:24:59.000000000 +0800
> @@ -47,11 +47,15 @@
> #endif
> #define EFER_SAVE_RESTORE_BITS ((u64)EFER_SCE)
>
> -static struct vmcs_descriptor {
> +static struct vmcs_config {

Why the name change? Just for Xen compatibility? If that's the only
reason, it's insufficent.


-- 
error compiling committee.c: too many arguments to function



[-- Attachment #2: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH]Abstract vmcs feature detect part

[Li, Xin B]

>> This patch replaces vmcs_write32_fixedbits() with 
>adjust_vmx_controls(),
>
>It still doesn't say why, but I can now see it: this is code 
>from Xen. I have no objection to copying Xen code, but it needs to be
documented in
>the changelog, and there needs to be a good reason as to why you're
>replacing existing code rather than improving it. So far I don't see
>this reason

We want to enable MSR BITMAP and VTPR for VMX guest, which depend on
this, I think you're already aware this.
And another reason is, all physical CPUs on the system should support
the _same_ VMX features set, or obviously it's weird that one CPU
support Intel VTPR feature while some other not. This is a rare case,
but basically we should detect it when KVM is being initialized, and
this patch also helps.

>
>>
>> -static struct vmcs_descriptor {
>> +static struct vmcs_config {
>
>Why the name change? Just for Xen compatibility? If that's the only
>reason, it's insufficent.
>

If you agree the above, this becomes reasonable.
-Xin

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH]Abstract vmcs feature detect part

[Li, Xin B]

>>>
>>> -static struct vmcs_descriptor {
>>> +static struct vmcs_config {
>>
>>Why the name change? Just for Xen compatibility? If that's the only
>>reason, it's insufficent.
>>
>
>If you agree the above, this becomes reasonable.
>-Xin
>

I'm not a English native speaker, maybe descriptor is a good name too.
-xin


>---------------------------------------------------------------
>----------
>This SF.net email is sponsored by: Splunk Inc.
>Still grepping through log files to find problems?  Stop.
>Now Search log events and configuration files using AJAX and a browser.
>Download your FREE copy of Splunk now >>  http://get.splunk.com/
>_______________________________________________
>kvm-devel mailing list
>kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/kvm-devel
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH]Abstract vmcs feature detect part

[Avi Kivity]

Li, Xin B wrote:
>>>> -static struct vmcs_descriptor {
>>>> +static struct vmcs_config {
>>>>         
>>> Why the name change? Just for Xen compatibility? If that's the only
>>> reason, it's insufficent.
>>>
>>>       
>> If you agree the above, this becomes reasonable.
>> -Xin
>>
>>     
>
> I'm not a English native speaker, maybe descriptor is a 

No, vmcs_config is a better name.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH]Abstract vmcs feature detect part

[Avi Kivity]

Li, Xin B wrote:
>>> This patch replaces vmcs_write32_fixedbits() with 
>>>       
>> adjust_vmx_controls(),
>>
>> It still doesn't say why, but I can now see it: this is code 
>>     
> >from Xen. I have no objection to copying Xen code, but it needs to be
> documented in
>   
>> the changelog, and there needs to be a good reason as to why you're
>> replacing existing code rather than improving it. So far I don't see
>> this reason
>>     
>
> We want to enable MSR BITMAP and VTPR for VMX guest, which depend on
> this, I think you're already aware this.
> And another reason is, all physical CPUs on the system should support
> the _same_ VMX features set, or obviously it's weird that one CPU
> support Intel VTPR feature while some other not. This is a rare case,
> but basically we should detect it when KVM is being initialized, and
> this patch also helps.
>
>   

Sure.  Please update the changelog to state this.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: [PATCH]Abstract vmcs feature detect part

[Yang, Sheng]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

Do you mean the description of patch?

Thanks
Yang, Sheng
-----Original Message-----
From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org] 
Sent: 2007年7月26日 17:50
To: Li, Xin B
Cc: Yang, Sheng; kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: Re: [kvm-devel] [PATCH]Abstract vmcs feature detect part

Sure.  Please update the changelog to state this.

-- 
error compiling committee.c: too many arguments to function


[-- Attachment #2: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel