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