Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
Yang, Sheng wrote: > I modified the patch as you suggest. > For any failure in this due to hardware error, I replace all the error > return code with -EIO. > > Thanks. > --- > From: Sheng Yang <[EMAIL PROTECTED]> > Date: Tue, 31 Jul 2007 17:23:20 +0800 > Subject: [PATCH] Add cpu consistence check in vmx. > > 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. > > Applied, thanks. -- 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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
I modified the patch as you suggest.
For any failure in this due to hardware error, I replace all the error
return code with -EIO.
Thanks.
---
From: Sheng Yang <[EMAIL PROTECTED]>
Date: Tue, 31 Jul 2007 17:23:20 +0800
Subject: [PATCH] Add cpu consistence check in vmx.
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.
Signed-off-by: Sheng Yang <[EMAIL PROTECTED]>
---
drivers/kvm/kvm.h |1 +
drivers/kvm/kvm_main.c | 10 +
drivers/kvm/svm.c |6 +
drivers/kvm/vmx.c | 51
+++
4 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index f78729c..e4f11b6 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -420,6 +420,7 @@ struct kvm_arch_ops {
int (*disabled_by_bios)(void); /* __init */
void (*hardware_enable)(void *dummy); /* __init */
void (*hardware_disable)(void *dummy);
+ void (*check_processor_compatibility)(void *rtn);
int (*hardware_setup)(void); /* __init */
void (*hardware_unsetup)(void);/* __exit */
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 4fd2074..1811ed4 100644
--- 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_free_0;
+ }
+
on_each_cpu(hardware_enable, NULL, 0, 1);
r = register_cpu_notifier(&kvm_cpu_notifier);
if (r)
@@ -3162,6 +3171,7 @@ out_free_2:
unregister_cpu_notifier(&kvm_cpu_notifier);
out_free_1:
on_each_cpu(hardware_disable, NULL, 0, 1);
+out_free_0:
kvm_arch_ops->hardware_unsetup();
out:
kvm_arch_ops = NULL;
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..fccaf35 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -840,13 +840,13 @@ static __init int adjust_vmx_controls(u32 ctl_min,
u32 ctl_opt,
/* Ensure minimum (required) set of control bits are supported.
*/
if (ctl_min & ~ctl)
- return -1;
+ return -EIO;
*result = ctl;
return 0;
}
-static __init int setup_vmcs_config(void)
+static __init int setup_vmcs_config(struct vmcs_config* vmcs_conf)
{
u32 vmx_msr_low, vmx_msr_high;
u32 min, opt;
@@ -859,7 +859,7 @@ static __init int setup_vmcs_config(void)
opt = 0;
if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS,
&_pin_based_exec_control) < 0)
- return -1;
+ return -EIO;
min = CPU_BASED_HLT_EXITING |
CPU_BASED_CR8_LOAD_EXITING |
@@ -870,7 +870,7 @@ static __init int setup_vmcs_config(void)
opt = 0;
if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PROCBASED_CTLS,
&_cpu_based_exec_control) < 0)
- return -1;
+ return -EIO;
min = 0;
#ifdef CONFIG_X86_64
@@ -879,37 +879,37 @@ static __init int setup_vmcs_config(void)
opt = 0;
if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
&_vmexit_control) < 0)
- return -1;
+ return -EIO;
min = opt = 0;
if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_ENTRY_CTLS,
&_vmentry_control) < 0)
- return -1;
+ return -EIO;
rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
/* IA-32 SDM Vol 3B: VMCS size is never greater than
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
Yang, Sheng wrote: >>> + return -1; >>> >>> >> -1 is -EPERM. We need a real, more suitable, error code here. >> > > How about using -EINVAL? But the comment of it is "Invalid argument"... > I can't find more one here... > > EINVAL usually means a user-supplied argument is incorrect. The best fit I can find is EIO, as it implies a hardware issue. -- 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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
>> +return -1; >> > >-1 is -EPERM. We need a real, more suitable, error code here. How about using -EINVAL? But the comment of it is "Invalid argument"... I can't find more one 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. Good approach. I will do it. >> +void __init check_processor_compat(void *rtn) >> > >Call this vmx_processor_compat() for consistency? Ok. Thanks. Yang, Sheng - 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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
>-Original Message- >From: Avi Kivity [mailto:[EMAIL PROTECTED] >Sent: Tuesday, July 31, 2007 4:10 PM >To: Li, Xin B >Cc: Yang, Sheng; kvm-devel@lists.sourceforge.net >Subject: Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx. > >Li, Xin B wrote: >>> 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? >>> >> >> I think we shouldn't enable it, or the code becomes to complex. >> -xin >> >> > >What do you mean exactly? > >Don't enable cpu hotplug? it's needed for suspend/resume. >Don't enable the cpu if it isn't compatible? I think that's >the way to go. Yes, this is what I meant. >Don't enable feature compatibility checking on hotplug? > >-- >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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
Li, Xin B wrote: >> 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? >> > > I think we shouldn't enable it, or the code becomes to complex. > -xin > > What do you mean exactly? Don't enable cpu hotplug? it's needed for suspend/resume. Don't enable the cpu if it isn't compatible? I think that's the way to go. Don't enable feature compatibility checking on hotplug? -- 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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
> >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? I think we shouldn't enable it, or the code becomes to complex. -xin > >-- >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 > - 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
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
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 tha
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
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).
Thanks
Yang, Sheng
--
From: Sheng Yang <[EMAIL PROTECTED]>
Date: Tue, 31 Jul 2007 10:21:32 +0800
Subject: [PATCH] Add cpu consistence check in vmx.
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.
---
drivers/kvm/kvm.h |1 +
drivers/kvm/kvm_main.c |9 +
drivers/kvm/svm.c |6 ++
drivers/kvm/vmx.c | 36
4 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index f78729c..e4f11b6 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -420,6 +420,7 @@ struct kvm_arch_ops {
int (*disabled_by_bios)(void); /* __init */
void (*hardware_enable)(void *dummy); /* __init */
void (*hardware_disable)(void *dummy);
+ void (*check_processor_compatibility)(void *rtn);
int (*hardware_setup)(void); /* __init */
void (*hardware_unsetup)(void);/* __exit */
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 4fd2074..ae14163 100644
--- 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;
+ }
+
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");
+ return -1;
+ }
return 0;
}
@@ -2412,11 +2424,19 @@ free_vcpu:
return ERR_PTR(err);
}
+void __init check_processor_compat(void *rtn)
+{
+ *(int *)rtn = 0;
+ if (setup_vmcs_config() < 0)
+ *(int *)rtn = -1;
+}
+
static struct kvm_arch_ops
Re: [kvm-devel] [PATCH] Add cpu consistence check in vmx.
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
[kvm-devel] [PATCH] Add cpu consistence check in vmx.
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.
Signed-off-by: Sheng Yang <[EMAIL PROTECTED]>
---
drivers/kvm/kvm_main.c |9 +++--
drivers/kvm/svm.c |3 +-
drivers/kvm/vmx.c | 84
+---
3 files changed, 59 insertions(+), 37 deletions(-)
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index bc11c2d..10443ea 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2974,14 +2974,14 @@ static void decache_vcpus_on_cpu(int cpu)
spin_unlock(&kvm_lock);
}
-static void hardware_enable(void *junk)
+static void hardware_enable(void *rtn)
{
int cpu = raw_smp_processor_id();
if (cpu_isset(cpu, cpus_hardware_enabled))
return;
cpu_set(cpu, cpus_hardware_enabled);
- kvm_arch_ops->hardware_enable(NULL);
+ kvm_arch_ops->hardware_enable(rtn);
}
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;
+
r = register_cpu_notifier(&kvm_cpu_notifier);
if (r)
goto out_free_1;
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 850a1b1..1e76417 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -285,7 +285,7 @@ static void svm_hardware_disable(void *garbage)
}
}
-static void svm_hardware_enable(void *garbage)
+static void svm_hardware_enable(void *rtn)
{
struct svm_cpu_data *svm_data;
@@ -298,6 +298,7 @@ static void svm_hardware_enable(void *garbage)
struct desc_struct *gdt;
int me = raw_smp_processor_id();
+ *(int*)rtn = 0;
if (!has_svm()) {
printk(KERN_ERR "svm_cpu_init: err EOPNOTSUPP on %d\n",
me);
return;
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index e5721a5..8032c7e 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -756,31 +756,6 @@ static __init int vmx_disabled_by_bios(void)
/* locked but not enabled */
}
-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");
-}
-
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)) {
+
