[PATCH 0/2] [RFC] Add support for nested SVM (userspace)

[PATCH 0/2] [RFC] Add support for nested SVM (userspace)

[Alexander Graf]

This is the userspace part of the nested SVM series. It enables building
the nested SVM code as external module and passes through the SVM CPUID bit
to the VM.

Alexander Graf (2):
  Add SVM CPUID feature define for backwards compatibility
  Allow the SVM CPUID bit in a VM

 kernel/x86/external-module-compat.h |    4 ++++
 qemu/qemu-kvm-x86.c                 |    3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

[PATCH 1/2] Add SVM CPUID feature define for backwards compatibility

[Alexander Graf]

Add the SVM capability to the external module compat header, so we can use it
when the kernel does not know about this define.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 kernel/x86/external-module-compat.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/x86/external-module-compat.h b/kernel/x86/external-module-compat.h
index c53e0d6..c4def59 100644
--- a/kernel/x86/external-module-compat.h
+++ b/kernel/x86/external-module-compat.h
@@ -201,6 +201,10 @@ static inline void preempt_notifier_sys_exit(void) {}
 #define X86_FEATURE_NX (1*32+20)
 #endif
 
+#ifndef X86_FEATURE_SVM
+#define X86_FEATURE_SVM (6*32+2)
+#endif
+
 #undef true
 #define true 1
 #undef false
-- 
1.5.6

[PATCH 2/2] Allow the SVM CPUID bit in a VM

[Alexander Graf]

Usually the qemu-kvm-bridge removes the SVM capability flag. Since KVM now
supports nested SVM, this is no longer necessary.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 qemu/qemu-kvm-x86.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index 5daedd1..cfc533f 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -489,9 +489,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
 	// nx
 	if ((h_edx & 0x00100000) == 0)
 	    e->edx &= ~0x00100000u;
-	// svm
-	if (e->ecx & 4)
-	    e->ecx &= ~4u;
     }
     // sysenter isn't supported on compatibility mode on AMD.  and syscall
     // isn't supported in compatibility mode on Intel.  so advertise the
-- 
1.5.6

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Avi Kivity]

Alexander Graf wrote:
> Usually the qemu-kvm-bridge removes the SVM capability flag. Since KVM now
> supports nested SVM, this is no longer necessary.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  qemu/qemu-kvm-x86.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
> index 5daedd1..cfc533f 100644
> --- a/qemu/qemu-kvm-x86.c
> +++ b/qemu/qemu-kvm-x86.c
> @@ -489,9 +489,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
>  	// nx
>  	if ((h_edx & 0x00100000) == 0)
>  	    e->edx &= ~0x00100000u;
> -	// svm
> -	if (e->ecx & 4)
> -	    e->ecx &= ~4u;
>      }
>   

Needs to be predicated on a KVM_CAP_ test, so we don't enable this on 
older kvms.

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

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Alexander Graf]

On Sep 1, 2008, at 4:00 PM, Avi Kivity wrote:

> Alexander Graf wrote:
>> Usually the qemu-kvm-bridge removes the SVM capability flag. Since  
>> KVM now
>> supports nested SVM, this is no longer necessary.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> qemu/qemu-kvm-x86.c |    3 ---
>> 1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>> index 5daedd1..cfc533f 100644
>> --- a/qemu/qemu-kvm-x86.c
>> +++ b/qemu/qemu-kvm-x86.c
>> @@ -489,9 +489,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry  
>> *e, uint32_t function,
>> 	// nx
>> 	if ((h_edx & 0x00100000) == 0)
>> 	    e->edx &= ~0x00100000u;
>> -	// svm
>> -	if (e->ecx & 4)
>> -	    e->ecx &= ~4u;
>>     }
>>
>
> Needs to be predicated on a KVM_CAP_ test, so we don't enable this  
> on older kvms.

Is this a real problem? We don't allow setting SVME on older kernels  
anyways.

Alex

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Alexander Graf]

On Sep 2, 2008, at 7:42 PM, Alexander Graf wrote:

>
> On Sep 1, 2008, at 4:00 PM, Avi Kivity wrote:
>
>> Alexander Graf wrote:
>>> Usually the qemu-kvm-bridge removes the SVM capability flag. Since  
>>> KVM now
>>> supports nested SVM, this is no longer necessary.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> qemu/qemu-kvm-x86.c |    3 ---
>>> 1 files changed, 0 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>>> index 5daedd1..cfc533f 100644
>>> --- a/qemu/qemu-kvm-x86.c
>>> +++ b/qemu/qemu-kvm-x86.c
>>> @@ -489,9 +489,6 @@ static void do_cpuid_ent(struct  
>>> kvm_cpuid_entry *e, uint32_t function,
>>> 	// nx
>>> 	if ((h_edx & 0x00100000) == 0)
>>> 	    e->edx &= ~0x00100000u;
>>> -	// svm
>>> -	if (e->ecx & 4)
>>> -	    e->ecx &= ~4u;
>>>    }
>>>
>>
>> Needs to be predicated on a KVM_CAP_ test, so we don't enable this  
>> on older kvms.
>
> Is this a real problem? We don't allow setting SVME on older kernels  
> anyways.

On a sidenote I'm really not fond of the userspace CPUID capability  
restrictions in the first place. Shouldn't this all be determined in  
the kernel module? It doesn't make since IMHO to restrict NX usage  
from userspace. The kernel module knows best what it's capable of.

Alex

>
>
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Itamar Heim]

Userspace may know better, for example when you want to allow live migration
between multiple hosts and want to calculate lowest common denominator
between them.

-----Original Message-----
From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf
Of Alexander Graf
Sent: Tuesday, September 02, 2008 20:54 PM
To: Alexander Graf
Cc: Avi Kivity; kvm@vger.kernel.org; joro@8bytes.org; anthony@codemonkey.ws
Subject: Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM


On Sep 2, 2008, at 7:42 PM, Alexander Graf wrote:

>
> On Sep 1, 2008, at 4:00 PM, Avi Kivity wrote:
>
>> Alexander Graf wrote:
>>> Usually the qemu-kvm-bridge removes the SVM capability flag. Since  
>>> KVM now
>>> supports nested SVM, this is no longer necessary.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> qemu/qemu-kvm-x86.c |    3 ---
>>> 1 files changed, 0 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>>> index 5daedd1..cfc533f 100644
>>> --- a/qemu/qemu-kvm-x86.c
>>> +++ b/qemu/qemu-kvm-x86.c
>>> @@ -489,9 +489,6 @@ static void do_cpuid_ent(struct  
>>> kvm_cpuid_entry *e, uint32_t function,
>>> 	// nx
>>> 	if ((h_edx & 0x00100000) == 0)
>>> 	    e->edx &= ~0x00100000u;
>>> -	// svm
>>> -	if (e->ecx & 4)
>>> -	    e->ecx &= ~4u;
>>>    }
>>>
>>
>> Needs to be predicated on a KVM_CAP_ test, so we don't enable this  
>> on older kvms.
>
> Is this a real problem? We don't allow setting SVME on older kernels  
> anyways.

On a sidenote I'm really not fond of the userspace CPUID capability  
restrictions in the first place. Shouldn't this all be determined in  
the kernel module? It doesn't make since IMHO to restrict NX usage  
from userspace. The kernel module knows best what it's capable of.

Alex

>
>
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Gerd Hoffmann]

  Hi,

> On a sidenote I'm really not fond of the userspace CPUID capability
> restrictions in the first place. Shouldn't this all be determined in the
> kernel module? It doesn't make since IMHO to restrict NX usage from
> userspace. The kernel module knows best what it's capable of.

I think cpuid handling will be a quite interesting when it comes to
merging qemu and kvm.  Doing it completely in the kernel isn't going to
work.  There are several reasons why you might want to change the cpuid
presented to the guest.

First, you might want to mask out cpuid bits not supported by all
machines in your pool, for migration compatibility.

Second, you might want to present a specific CPU to the guest (like qemu
-cpu pentium).  At least as close as possible, of course there are some
restrictions.  I think even if you mask out the sse cpuid bit sse
instructions will actually work.  Also having a virtual intel cpu on amd
and visa versa probably isn't going to work unless we'll fall back to
the emulator for the missing instructions.

Third, there is the hypervisor cpuid range (0x40000000+).  That one
needs to be configurable by userspace as well.

cheers,
  Gerd

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Alexander Graf]

On Sep 3, 2008, at 9:55 AM, Gerd Hoffmann wrote:

>  Hi,
>
>> On a sidenote I'm really not fond of the userspace CPUID capability
>> restrictions in the first place. Shouldn't this all be determined  
>> in the
>> kernel module? It doesn't make since IMHO to restrict NX usage from
>> userspace. The kernel module knows best what it's capable of.
>
> I think cpuid handling will be a quite interesting when it comes to
> merging qemu and kvm.  Doing it completely in the kernel isn't going  
> to
> work.  There are several reasons why you might want to change the  
> cpuid
> presented to the guest.
>
> First, you might want to mask out cpuid bits not supported by all
> machines in your pool, for migration compatibility.
>
> Second, you might want to present a specific CPU to the guest (like  
> qemu
> -cpu pentium).  At least as close as possible, of course there are  
> some
> restrictions.  I think even if you mask out the sse cpuid bit sse
> instructions will actually work.  Also having a virtual intel cpu on  
> amd
> and visa versa probably isn't going to work unless we'll fall back to
> the emulator for the missing instructions.
>
> Third, there is the hypervisor cpuid range (0x40000000+).  That one
> needs to be configurable by userspace as well.

I guess I didn't express myself correctly, sorry for that :-). I think  
that kvm kernel module capabilities should be masked out in the kernel  
module, while "other" stuff, like migration masking or additional  
CPUIDs (do these work atm?) should of course reside in the userspace.

The code I was referring to was the removal of the NX bit and the SVM  
bit. These are definitely kernel capabilities. If the kernel module  
doesn't implement these, it should just mask them out instead of  
having userspace figure out what's there and what isn't.

I hope I didn't completely confuse everybody now :-). On a sidenote:  
We really need to switch to the newer cpuid kernel<->userspace  
interface. The way it currently is, CPUID 4 is severely broken.

Alex

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Avi Kivity]

Alexander Graf wrote:
>>
>> Needs to be predicated on a KVM_CAP_ test, so we don't enable this on 
>> older kvms.
>
> Is this a real problem? We don't allow setting SVME on older kernels 
> anyways.

Actually the kernel exports its supported cpuid bits via 
KVM_GET_SUPPORTED_CPUID.

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

Re: [PATCH 2/2] Allow the SVM CPUID bit in a VM

[Avi Kivity]

Alexander Graf wrote:
>
> I guess I didn't express myself correctly, sorry for that :-). I think 
> that kvm kernel module capabilities should be masked out in the kernel 
> module, while "other" stuff, like migration masking or additional 
> CPUIDs (do these work atm?) should of course reside in the userspace.

But then userspace needs to know when capabilities userspace actually sees.

>
> The code I was referring to was the removal of the NX bit and the SVM 
> bit. These are definitely kernel capabilities. If the kernel module 
> doesn't implement these, it should just mask them out instead of 
> having userspace figure out what's there and what isn't.

The flow I have in mind is:

- kernel exports which cpuid bits it supports (global information; not 
tied to a guest; this way it can be used to calculate g-c-d)
- userspace merges this information with its own ideas
- userspace tells the kernel what cpuid bits to present to the guest 
(lying is allowed)

This gives the maximum flexibility and visibility.

NX and SVM are masked for backward compatibility with older userspace; 
we could probably remove this masking now.
>
> I hope I didn't completely confuse everybody now :-). On a sidenote: 
> We really need to switch to the newer cpuid kernel<->userspace 
> interface. The way it currently is, CPUID 4 is severely broken.

There is a new interface in place (quite old by now), unfortunately it 
isn't used by qemu.

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