[PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Sasha Levin]

Add a module parameter 'check_inuse' to allow disabling the check of whether
virtualization has already been enabled on the given cpu.

This is needed to deal with broken BIOS which set the SVM/VMX bit by default.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Suggested-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 arch/x86/kvm/svm.c |    5 ++++-
 arch/x86/kvm/vmx.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 475d1c9..5ca76e3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -183,6 +183,9 @@ module_param(npt, int, S_IRUGO);
 static int nested = 1;
 module_param(nested, int, S_IRUGO);
 
+static bool check_inuse = 1;
+module_param(check_inuse, bool, S_IRUGO);
+
 static void svm_flush_tlb(struct kvm_vcpu *vcpu);
 static void svm_complete_interrupts(struct vcpu_svm *svm);
 
@@ -587,7 +590,7 @@ static int svm_hardware_enable(void *garbage)
 	int me = raw_smp_processor_id();
 
 	rdmsrl(MSR_EFER, efer);
-	if (efer & EFER_SVME)
+	if (check_inuse && (efer & EFER_SVME))
 		return -EBUSY;
 
 	if (!has_svm()) {
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f5b49c7..3046b07 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -74,6 +74,9 @@ module_param(vmm_exclusive, bool, S_IRUGO);
 static int __read_mostly yield_on_hlt = 1;
 module_param(yield_on_hlt, bool, S_IRUGO);
 
+static bool check_inuse = 1;
+module_param(check_inuse, bool, S_IRUGO);
+
 /*
  * If nested=1, nested virtualization is supported, i.e., guests may use
  * VMX and be a hypervisor for its own guests. If nested=0, guests may not
@@ -2230,7 +2233,7 @@ static int hardware_enable(void *garbage)
 	u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
 	u64 old, test_bits;
 
-	if (read_cr4() & X86_CR4_VMXE)
+	if (check_inuse && (read_cr4() & X86_CR4_VMXE))
 		return -EBUSY;
 
 	INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu));
-- 
1.7.6

[PATCH 2/2] vmx,svm: Print errors if SVM or VMX were already set

[Sasha Levin]

Instead of exiting quietly, print an error if the VMX or the SVM bits
were already set when loading the module.

Having VMX/SVM bits set means that either there is someone else doing
hardware virtualization, or that the BIOS is buggy and sets it on
by default.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 arch/x86/kvm/svm.c |    5 ++++-
 arch/x86/kvm/vmx.c |    4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5ca76e3..2a1df2e 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -590,8 +590,11 @@ static int svm_hardware_enable(void *garbage)
 	int me = raw_smp_processor_id();
 
 	rdmsrl(MSR_EFER, efer);
-	if (check_inuse && (efer & EFER_SVME))
+	if (check_inuse && (efer & EFER_SVME)) {
+		printk(KERN_ERR "svm_hardware_enable: SVM already set on %d\n",
+		       me);
 		return -EBUSY;
+	}
 
 	if (!has_svm()) {
 		printk(KERN_ERR "svm_hardware_enable: err EOPNOTSUPP on %d\n",
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 3046b07..df69b1d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2233,8 +2233,10 @@ static int hardware_enable(void *garbage)
 	u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
 	u64 old, test_bits;
 
-	if (check_inuse && (read_cr4() & X86_CR4_VMXE))
+	if (check_inuse && (read_cr4() & X86_CR4_VMXE)) {
+		printk(KERN_ERR "hardware_enable: VMX already set\n");
 		return -EBUSY;
+	}
 
 	INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu));
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, old);
-- 
1.7.6

Re: [PATCH 2/2] vmx,svm: Print errors if SVM or VMX were already set

[Alexander Graf]

On 05.07.2011, at 01:09, Sasha Levin wrote:

> Instead of exiting quietly, print an error if the VMX or the SVM bits
> were already set when loading the module.
> 
> Having VMX/SVM bits set means that either there is someone else doing
> hardware virtualization, or that the BIOS is buggy and sets it on
> by default.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> arch/x86/kvm/svm.c |    5 ++++-
> arch/x86/kvm/vmx.c |    4 +++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5ca76e3..2a1df2e 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -590,8 +590,11 @@ static int svm_hardware_enable(void *garbage)
> 	int me = raw_smp_processor_id();
> 
> 	rdmsrl(MSR_EFER, efer);
> -	if (check_inuse && (efer & EFER_SVME))
> +	if (check_inuse && (efer & EFER_SVME)) {
> +		printk(KERN_ERR "svm_hardware_enable: SVM already set on %d\n",

CPU%d

Also I'd rephrase it as "already in use on". Otherwise looks good :)


Alex

RE: [PATCH 2/2] vmx,svm: Print errors if SVM or VMX were already set

[Tian, Kevin]

> From: Sasha Levin
> Sent: Tuesday, July 05, 2011 7:09 AM
> 
> Instead of exiting quietly, print an error if the VMX or the SVM bits
> were already set when loading the module.
> 
> Having VMX/SVM bits set means that either there is someone else doing
> hardware virtualization, or that the BIOS is buggy and sets it on
> by default.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  arch/x86/kvm/svm.c |    5 ++++-
>  arch/x86/kvm/vmx.c |    4 +++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5ca76e3..2a1df2e 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -590,8 +590,11 @@ static int svm_hardware_enable(void *garbage)
>  	int me = raw_smp_processor_id();
> 
>  	rdmsrl(MSR_EFER, efer);
> -	if (check_inuse && (efer & EFER_SVME))
> +	if (check_inuse && (efer & EFER_SVME)) {
> +		printk(KERN_ERR "svm_hardware_enable: SVM already set
> on %d\n",
> +		       me);
>  		return -EBUSY;
> +	}
> 
>  	if (!has_svm()) {
>  		printk(KERN_ERR "svm_hardware_enable: err EOPNOTSUPP
> on %d\n",
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 3046b07..df69b1d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2233,8 +2233,10 @@ static int hardware_enable(void *garbage)
>  	u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
>  	u64 old, test_bits;
> 
> -	if (check_inuse && (read_cr4() & X86_CR4_VMXE))
> +	if (check_inuse && (read_cr4() & X86_CR4_VMXE)) {
> +		printk(KERN_ERR "hardware_enable: VMX already set\n");
>  		return -EBUSY;
> +	}
> 

make the error message consistent between vmx and svm, i.e. adding
cpu id for vmx too.

Thanks
Kevin

RE: [PATCH 2/2] vmx,svm: Print errors if SVM or VMX were already set

[Tian, Kevin]

> From: Alexander Graf
> Sent: Tuesday, July 05, 2011 8:42 AM
> 
> 
> On 05.07.2011, at 01:09, Sasha Levin wrote:
> 
> > Instead of exiting quietly, print an error if the VMX or the SVM bits
> > were already set when loading the module.
> >
> > Having VMX/SVM bits set means that either there is someone else doing
> > hardware virtualization, or that the BIOS is buggy and sets it on
> > by default.
> >
> > Cc: Avi Kivity <avi@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> > arch/x86/kvm/svm.c |    5 ++++-
> > arch/x86/kvm/vmx.c |    4 +++-
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 5ca76e3..2a1df2e 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -590,8 +590,11 @@ static int svm_hardware_enable(void *garbage)
> > 	int me = raw_smp_processor_id();
> >
> > 	rdmsrl(MSR_EFER, efer);
> > -	if (check_inuse && (efer & EFER_SVME))
> > +	if (check_inuse && (efer & EFER_SVME)) {
> > +		printk(KERN_ERR "svm_hardware_enable: SVM already set
> on %d\n",
> 
> CPU%d
> 
> Also I'd rephrase it as "already in use on". Otherwise looks good :)
> 

A more elaborative message sounds better, like the explanation for possible
cause in the commit message. Also an advertisement about "check_inuse"
option is a good thing here in the message. :-)

Thanks
Kevin

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Avi Kivity]

On 07/05/2011 02:09 AM, Sasha Levin wrote:
> Add a module parameter 'check_inuse' to allow disabling the check of whether
> virtualization has already been enabled on the given cpu.
>
> This is needed to deal with broken BIOS which set the SVM/VMX bit by default.

Please split the vmx and svm parts.

What machine was this?  Did you try a BIOS update/complaint to 
manufacturer/etc?  Let's try the proper channels before workarounds.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Sasha Levin]

On Tue, 2011-07-05 at 11:09 +0300, Avi Kivity wrote:
> On 07/05/2011 02:09 AM, Sasha Levin wrote:
> > Add a module parameter 'check_inuse' to allow disabling the check of whether
> > virtualization has already been enabled on the given cpu.
> >
> > This is needed to deal with broken BIOS which set the SVM/VMX bit by default.
> 
> Please split the vmx and svm parts.
> 
> What machine was this?  Did you try a BIOS update/complaint to 
> manufacturer/etc?  Let's try the proper channels before workarounds.
> 

I have no information about the machine.
It was the result of trying to debug an issue reported on IRC where we
found that the SVM flag is being set after boot even before the kvm
module is loaded.

-- 

Sasha.

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Joerg Roedel]

On Tue, Jul 05, 2011 at 11:14:43AM +0300, Sasha Levin wrote:
> I have no information about the machine.  It was the result of trying
> to debug an issue reported on IRC where we found that the SVM flag is
> being set after boot even before the kvm module is loaded.

Have you ruled out that any other third-party hypervisor module was
loaded before KVM? The virtual-box module for example?

	Joerg

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Sasha Levin]

On Tue, 2011-07-05 at 11:11 +0200, Joerg Roedel wrote:
> On Tue, Jul 05, 2011 at 11:14:43AM +0300, Sasha Levin wrote:
> > I have no information about the machine.  It was the result of trying
> > to debug an issue reported on IRC where we found that the SVM flag is
> > being set after boot even before the kvm module is loaded.
> 
> Have you ruled out that any other third-party hypervisor module was
> loaded before KVM? The virtual-box module for example?

No other hypervisor was installed and lsmod output was clean.

-- 

Sasha.

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Avi Kivity]

On 07/05/2011 12:32 PM, Sasha Levin wrote:
> On Tue, 2011-07-05 at 11:11 +0200, Joerg Roedel wrote:
> >  On Tue, Jul 05, 2011 at 11:14:43AM +0300, Sasha Levin wrote:
> >  >  I have no information about the machine.  It was the result of trying
> >  >  to debug an issue reported on IRC where we found that the SVM flag is
> >  >  being set after boot even before the kvm module is loaded.
> >
> >  Have you ruled out that any other third-party hypervisor module was
> >  loaded before KVM? The virtual-box module for example?
>
> No other hypervisor was installed and lsmod output was clean.

This needs to be understood further.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Sasha Levin]

On Tue, 2011-07-05 at 12:37 +0300, Avi Kivity wrote:
> On 07/05/2011 12:32 PM, Sasha Levin wrote:
> > On Tue, 2011-07-05 at 11:11 +0200, Joerg Roedel wrote:
> > >  On Tue, Jul 05, 2011 at 11:14:43AM +0300, Sasha Levin wrote:
> > >  >  I have no information about the machine.  It was the result of trying
> > >  >  to debug an issue reported on IRC where we found that the SVM flag is
> > >  >  being set after boot even before the kvm module is loaded.
> > >
> > >  Have you ruled out that any other third-party hypervisor module was
> > >  loaded before KVM? The virtual-box module for example?
> >
> > No other hypervisor was installed and lsmod output was clean.
> 
> This needs to be understood further.
> 

Please note that I don't have access to the hardware in question, this
was done over IRC.

Here are the steps taken in debugging this issue:

1. Looking at the dmesg ( http://pastebin.com/eM7bDY8r ) we saw that
when trying to load the kvm module, the following error shows up: 'kvm:
enabling virtualization on CPU0 failed'.

2. We went through the lsmod output (unfortunately I don't have the link
as it's gone from my IRC buffer) and didn't see any modules belonging to
other hypervisors.

3. At that point, looking at the code - we figured that a set SVM flag
is the possible culprit since it's the only code path which fails
loading the module with that error message without printing anything
else.

4. Installed msr-tools and injected the msr module so that we could read
msr values from userspace.

5. Ran 'rdmsr 0xc0000080' to read the extended feature register. The
output had bit 12 set - which means that SVM bit was enabled.

6. Ran 'wrmsr 0xc0000080 0xd01' which disabled the SVM bit.

7. kvm module loaded ok.

-- 

Sasha.

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Avi Kivity]

On 07/05/2011 12:56 PM, Sasha Levin wrote:
> Please note that I don't have access to the hardware in question, this
> was done over IRC.
>

I understand that.  Can you get in contact with the reporter again?

> Here are the steps taken in debugging this issue:
>
> 1. Looking at the dmesg ( http://pastebin.com/eM7bDY8r ) we saw that
> when trying to load the kvm module, the following error shows up: 'kvm:
> enabling virtualization on CPU0 failed'.
>
> 2. We went through the lsmod output (unfortunately I don't have the link
> as it's gone from my IRC buffer) and didn't see any modules belonging to
> other hypervisors.
>
> 3. At that point, looking at the code - we figured that a set SVM flag
> is the possible culprit since it's the only code path which fails
> loading the module with that error message without printing anything
> else.
>
> 4. Installed msr-tools and injected the msr module so that we could read
> msr values from userspace.
>
> 5. Ran 'rdmsr 0xc0000080' to read the extended feature register. The
> output had bit 12 set - which means that SVM bit was enabled.
>
> 6. Ran 'wrmsr 0xc0000080 0xd01' which disabled the SVM bit.
>
> 7. kvm module loaded ok.

My questions are:

- was a BIOS update attempted?  at least VMware uses the same check as 
kvm, and probably virtualbox as well, so this problem should have been 
seen before.
- was the vendor contacted?  Not that I think we'll see a lot of good 
from that.
- was this after a reset or cold boot?
- maybe a stealth rootkit is involved?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Sasha Levin]

On Tue, 2011-07-05 at 13:37 +0300, Avi Kivity wrote:
> On 07/05/2011 12:56 PM, Sasha Levin wrote:
> > Please note that I don't have access to the hardware in question, this
> > was done over IRC.
> >
> 
> I understand that.  Can you get in contact with the reporter again?
> 

Hopefully, If he comes back on IRC (or reads these mails :) ).

> > Here are the steps taken in debugging this issue:
> >
> > 1. Looking at the dmesg ( http://pastebin.com/eM7bDY8r ) we saw that
> > when trying to load the kvm module, the following error shows up: 'kvm:
> > enabling virtualization on CPU0 failed'.
> >
> > 2. We went through the lsmod output (unfortunately I don't have the link
> > as it's gone from my IRC buffer) and didn't see any modules belonging to
> > other hypervisors.
> >
> > 3. At that point, looking at the code - we figured that a set SVM flag
> > is the possible culprit since it's the only code path which fails
> > loading the module with that error message without printing anything
> > else.
> >
> > 4. Installed msr-tools and injected the msr module so that we could read
> > msr values from userspace.
> >
> > 5. Ran 'rdmsr 0xc0000080' to read the extended feature register. The
> > output had bit 12 set - which means that SVM bit was enabled.
> >
> > 6. Ran 'wrmsr 0xc0000080 0xd01' which disabled the SVM bit.
> >
> > 7. kvm module loaded ok.
> 
> My questions are:
> 
> - was a BIOS update attempted?  at least VMware uses the same check as 
> kvm, and probably virtualbox as well, so this problem should have been 
> seen before.

We didn't update the BIOS.

virtualbox was installed previously and didn't work properly either -
thats why he tried kvm afaik.

We made sure to remove virtualbox properly and did a reset afterwards.
After removal, no virtualbox modules were loaded at any point.

> - was the vendor contacted?  Not that I think we'll see a lot of good 
> from that.

Nope.

> - was this after a reset or cold boot?

This was a reset, we didn't try a cold boot.

> - maybe a stealth rootkit is involved?
> 

A rootkit that messed up the MSRs or runs a hidden guest sounds like a
possibility too.

Alexander Graf suggested it's a simple case of a BIOS vendor not
implementing specs properly as he has seen a similar case of BIOS only
allowing to start virtualization on the first CPU.

-- 

Sasha.

Re: [PATCH 1/2] vmx,svm: Add module parameter to ignore the 'in use' check

[Avi Kivity]

On 07/05/2011 02:07 PM, Sasha Levin wrote:
> >
> >  - was a BIOS update attempted?  at least VMware uses the same check as
> >  kvm, and probably virtualbox as well, so this problem should have been
> >  seen before.
>
> We didn't update the BIOS.
>
> virtualbox was installed previously and didn't work properly either -
> thats why he tried kvm afaik.

That's a good data point.

> >  - was this after a reset or cold boot?
>
> This was a reset, we didn't try a cold boot.

Unlikely to help, since it was a preexisting problem.

> >  - maybe a stealth rootkit is involved?
> >
>
> A rootkit that messed up the MSRs or runs a hidden guest sounds like a
> possibility too.
>
> Alexander Graf suggested it's a simple case of a BIOS vendor not
> implementing specs properly as he has seen a similar case of BIOS only
> allowing to start virtualization on the first CPU.

I agree.  But let's try a BIOS update first, if possible.

If that fails, please repost the patches, but for svm only, since we 
haven't seen these issues on Intel.

-- 
error compiling committee.c: too many arguments to function