Re: [PATCH] Add cpu consistence check in vmx.

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

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/