Yang, Sheng wrote:
> All the physical CPUs on the board should support the same VMX feature
> set.
> The hardware_enable() do the per-cpu check now.
> In case of vmx/svm enabling failure, transfer a variable in
> hardware_enable()
> for error report.
>
>
> static void hardware_disable(void *junk)
> @@ -3173,7 +3173,10 @@ int kvm_init_arch(struct kvm_arch_ops *ops,
> struct module *module)
> if (r < 0)
> goto out;
>
> - on_each_cpu(hardware_enable, NULL, 0, 1);
> + on_each_cpu(hardware_enable, &r, 0, 1);
> + if (r < 0)
> + goto out;
> +
>
Looks like the checking and assignment to r are done in parallel; this
is racy.
I suggest adding a new arch op, ->check_processor_compatibility(), and
running it from kvm_main as follows:
for_each_online_cpu(cpu) {
smp_call_function_single(cpu,
kvm_arch_ops->check_processor_compatibility, &r, ...);
if (r < 0)
break;
}
>
> -static void hardware_enable(void *garbage)
> -{
> - int cpu = raw_smp_processor_id();
> - u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
> - u64 old;
> -
> - rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
> - if ((old & (MSR_IA32_FEATURE_CONTROL_LOCKED |
> - MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED))
> - != (MSR_IA32_FEATURE_CONTROL_LOCKED |
> - MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED))
> - /* enable and lock */
> - wrmsrl(MSR_IA32_FEATURE_CONTROL, old |
> - MSR_IA32_FEATURE_CONTROL_LOCKED |
> - MSR_IA32_FEATURE_CONTROL_VMXON_ENABLED);
> - write_cr4(read_cr4() | X86_CR4_VMXE); /* FIXME: not cpu hotplug
> safe */
> - asm volatile (ASM_VMX_VMXON_RAX : : "a"(&phys_addr),
> "m"(phys_addr)
> - : "memory", "cc");
> -}
> -
> -static void hardware_disable(void *garbage)
> -{
> - asm volatile (ASM_VMX_VMXOFF : : : "cc");
> -}
> -
>
Please avoid moving a function and changing it in the same patch. That
makes it very hard to review.
> static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
> u32 msr, u32* result)
> {
> @@ -856,18 +831,61 @@ static __init int setup_vmcs_config(void)
> if (((vmx_msr_high >> 18) & 15) != 6)
> return -1;
>
> - 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;
> + if (vmcs_config.size == 0) {
> + /* called in hardware_setup(), initialization */
> + 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;
> + } else if ((vmcs_config.size != (vmx_msr_high & 0x1fff))
> + || (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)) {
> + /* called in hardware_enable(), check consistence */
> + printk(KERN_ERR "kvm: CPUs feature inconsistence!\n");
> + return -1;
> + }
>
> return 0;
> }
>
This is called from ->hardware_enable(), right? If so, then it is racy.
--
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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
