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

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

Yang, Sheng wrote:
> Thank you for point out my fault.
>
> Here is a modified version which is clearer. And I have tested it with
> version d9feefe(for the latest git repository broken).
>   

I recommend building kvm.git, not the external module.  kvm.git is not 
broken at the moment.

> All the physical CPUs on the board should support the same VMX feature
> set.
> Add check_processor_compatibility to kvm_arch_ops for the consistence
> check.
>   

> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -3095,6 +3095,7 @@ int kvm_init_arch(struct kvm_arch_ops *ops,
> unsigned int vcpu_size,
>  		  struct module *module)
>  {
>  	int r;
> +	int cpu;
>  
>  	if (kvm_arch_ops) {
>  		printk(KERN_ERR "kvm: already loaded the other
> module\n");
> @@ -3116,6 +3117,14 @@ int kvm_init_arch(struct kvm_arch_ops *ops,
> unsigned int vcpu_size,
>  	if (r < 0)
>  		goto out;
>  
> +	for_each_online_cpu(cpu) {
> +		smp_call_function_single(cpu,
> +
> kvm_arch_ops->check_processor_compatibility,
> +				&r, 0, 1);
> +		if (r < 0)
> +			goto out;
>   

You need to call ->hardware_unsetup() in case of an error here.

> +	}
> +
>  	on_each_cpu(hardware_enable, NULL, 0, 1);
>  	r = register_cpu_notifier(&kvm_cpu_notifier);
>  	if (r)
> diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
> index 5277084..827bc27 100644
> --- a/drivers/kvm/svm.c
> +++ b/drivers/kvm/svm.c
> @@ -1798,11 +1798,17 @@ svm_patch_hypercall(struct kvm_vcpu *vcpu,
> unsigned char *hypercall)
>  	hypercall[3] = 0xc3;
>  }
>  
> +static void svm_check_processor_compat(void *rtn)
> +{
> +	*(int *)rtn = 0;
> +}
> +
>  static struct kvm_arch_ops svm_arch_ops = {
>  	.cpu_has_kvm_support = has_svm,
>  	.disabled_by_bios = is_disabled,
>  	.hardware_setup = svm_hardware_setup,
>  	.hardware_unsetup = svm_hardware_unsetup,
> +	.check_processor_compatibility = svm_check_processor_compat,
>  	.hardware_enable = svm_hardware_enable,
>  	.hardware_disable = svm_hardware_disable,
>  
> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
> index 6e23600..41a4986 100644
> --- a/drivers/kvm/vmx.c
> +++ b/drivers/kvm/vmx.c
> @@ -902,14 +902,26 @@ 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 check_processor_compat(), check consistence */
> +		printk(KERN_ERR "kvm: CPUs feature inconsistence!\n");
>   

Spelling: "CPUs" -> "CPU%d", "inconsistence" -> "inconsistency".

> +		return -1;
>   

-1 is -EPERM.  We need a real, more suitable, error code here.

Also, having a single function either construct vmcs_config or verify, 
depending on whether it is first called or not, it is a bit ugly.  A 
check_... function shouldn't actually set up global variables.  How 
about the following:

- setup_vmcs_config() takes a vmcs_config parameter instead of using a 
global.
- it is called once by vmx_hardware_setup() with the global config
- vmx_check_processor_compat() calls setup_vmcs_config() to set up a 
local variable, and then calls vmx_verify_config() to compare the two 
configurations.  Perhaps we can use memcmp() for the comparison.
> +	}
>  
>  	return 0;
>  }
> @@ -2412,11 +2424,19 @@ free_vcpu:
>  	return ERR_PTR(err);
>  }
>  
> +void __init check_processor_compat(void *rtn)
>   

Call this vmx_processor_compat() for consistency?


btw, what about cpu hotplug?  we need to deal with that too.  Do we 
error out and refuse to enable the cpu if it isn't compatible enough?

-- 
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/