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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
