Re: [PATCH]Abstract vmcs feature detect part

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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/