[Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Glauber Costa]

If the kernel is not 64-bit capable (even if the host
machine is) do not expose the lm bit in guest cpuid.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 target-i386/helper.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..997e4e1 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -24,6 +24,7 @@
 #include <inttypes.h>
 #include <signal.h>
 #include <assert.h>
+#include <sys/utsname.h>
 
 #include "cpu.h"
 #include "exec-all.h"
@@ -1520,13 +1521,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
 
         if (kvm_enabled()) {
             uint32_t h_eax, h_edx;
+            struct utsname utsname;
+
+            uname(&utsname);
 
             host_cpuid(0x80000001, &h_eax, NULL, NULL, &h_edx);
 
             /* disable CPU features that the host does not support */
 
             /* long mode */
-            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
+            if ((h_edx & 0x20000000) == 0  || strcmp(utsname.machine, "x86_64"))
                 *edx &= ~0x20000000;
             /* syscall */
             if ((h_edx & 0x00000800) == 0)
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Alexander Graf]

On 03.02.2009, at 16:04, Glauber Costa wrote:

> If the kernel is not 64-bit capable (even if the host
> machine is) do not expose the lm bit in guest cpuid.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
> target-i386/helper.c |    6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..997e4e1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -24,6 +24,7 @@
> #include <inttypes.h>
> #include <signal.h>
> #include <assert.h>
> +#include <sys/utsname.h>
>
> #include "cpu.h"
> #include "exec-all.h"
> @@ -1520,13 +1521,16 @@ void cpu_x86_cpuid(CPUX86State *env,  
> uint32_t index,
>
>         if (kvm_enabled()) {
>             uint32_t h_eax, h_edx;
> +            struct utsname utsname;
> +
> +            uname(&utsname);

This doesn't really compile on win32, does it?

Why not check for sizeof(long) == 4? That way you'd know that you're  
running 32-bit code which can only handle 32-bit guests anyway, right?  
And if you're running 64-bit userspace code, you know that the kernel  
is 64-bit aware.

Alex

>
>
>             host_cpuid(0x80000001, &h_eax, NULL, NULL, &h_edx);
>
>             /* disable CPU features that the host does not support */
>
>             /* long mode */
> -            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel  
> */)
> +            if ((h_edx & 0x20000000) == 0  ||  
> strcmp(utsname.machine, "x86_64"))
>                 *edx &= ~0x20000000;
>             /* syscall */
>             if ((h_edx & 0x00000800) == 0)
> -- 
> 1.5.6.5
>
>
>

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Glauber Costa]

On Tue, Feb 3, 2009 at 1:44 PM, Alexander Graf <agraf@suse.de> wrote:
> This doesn't really compile on win32, does it?
This is kvm code, and as we don't have kvm for win32 hosts, possibly
does not matter.
>
> Why not check for sizeof(long) == 4? That way you'd know that you're running
> 32-bit code which can only handle 32-bit guests anyway, right? And if you're
> running 64-bit userspace code, you know that the kernel is 64-bit aware.

How about qemu/kvm compiled in a 32-bit environment, thus, sizeof long == 4,
running in a x86_64 capable kernel?

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Anthony Liguori]

Glauber Costa wrote:
> On Tue, Feb 3, 2009 at 1:44 PM, Alexander Graf <agraf@suse.de> wrote:
>   
>> This doesn't really compile on win32, does it?
>>     
> This is kvm code, and as we don't have kvm for win32 hosts, possibly
> does not matter.
>   

Would be better to use a kvm accessor to wrap this functionality.  
kvm_is_lmkernel() or something.  Makes things more obviously portable on 
another platform.

Regards,

Anthony Liguori

>> Why not check for sizeof(long) == 4? That way you'd know that you're running
>> 32-bit code which can only handle 32-bit guests anyway, right? And if you're
>> running 64-bit userspace code, you know that the kernel is 64-bit aware.
>>     
>
> How about qemu/kvm compiled in a 32-bit environment, thus, sizeof long == 4,
> running in a x86_64 capable kernel?
>
>   

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Avi Kivity]

Glauber Costa wrote:
> If the kernel is not 64-bit capable (even if the host
> machine is) do not expose the lm bit in guest cpuid.
>
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..997e4e1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -24,6 +24,7 @@
>  #include <inttypes.h>
>  #include <signal.h>
>  #include <assert.h>
> +#include <sys/utsname.h>
>  
>  #include "cpu.h"
>  #include "exec-all.h"
> @@ -1520,13 +1521,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>  
>          if (kvm_enabled()) {
>              uint32_t h_eax, h_edx;
> +            struct utsname utsname;
> +
> +            uname(&utsname);
>  

It would be better to use KVM_GET_SUPPORTED_CPUID for this (and 
similar).  uname() can lie, and in theory kvm can support x86_64 on i386 
kernels (although patches to implement this would be rejected on 
cost/benefit and aesthetic concerns).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Avi Kivity]

Glauber Costa wrote:
> If the kernel is not 64-bit capable (even if the host
> machine is) do not expose the lm bit in guest cpuid.
>
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..997e4e1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -24,6 +24,7 @@
>  #include <inttypes.h>
>  #include <signal.h>
>  #include <assert.h>
> +#include <sys/utsname.h>
>  
>  #include "cpu.h"
>  #include "exec-all.h"
> @@ -1520,13 +1521,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>  
>          if (kvm_enabled()) {
>              uint32_t h_eax, h_edx;
> +            struct utsname utsname;
> +
> +            uname(&utsname);
>  

It would be better to use KVM_GET_SUPPORTED_CPUID for this (and 
similar).  uname() can lie, and in theory kvm can support x86_64 on i386 
kernels (although patches to implement this would be rejected on 
cost/benefit and aesthetic concerns).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Glauber Costa]

> It would be better to use KVM_GET_SUPPORTED_CPUID for this (and similar).
>  uname() can lie, and in theory kvm can support x86_64 on i386 kernels
> (although patches to implement this would be rejected on cost/benefit and
> aesthetic concerns).

I sent a patch that implements this last week, but anthony seemed to
be more inclined
towards this approach.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Anthony Liguori]

Glauber Costa wrote:
>> It would be better to use KVM_GET_SUPPORTED_CPUID for this (and similar).
>>  uname() can lie, and in theory kvm can support x86_64 on i386 kernels
>> (although patches to implement this would be rejected on cost/benefit and
>> aesthetic concerns).
>>     
>
> I sent a patch that implements this last week, but anthony seemed to
> be more inclined
> towards this approach.
>   

All I said was that if we used KVM_GET_SUPPORTED_CPUID, we had to do 
something about save/restore since it can mask arbitrary things.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Glauber Costa]

On Tue, Feb 3, 2009 at 7:14 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Glauber Costa wrote:
>>>
>>> It would be better to use KVM_GET_SUPPORTED_CPUID for this (and similar).
>>>  uname() can lie, and in theory kvm can support x86_64 on i386 kernels
>>> (although patches to implement this would be rejected on cost/benefit and
>>> aesthetic concerns).
>>>
>>
>> I sent a patch that implements this last week, but anthony seemed to
>> be more inclined
>> towards this approach.
>>
>
> All I said was that if we used KVM_GET_SUPPORTED_CPUID, we had to do
> something about save/restore since it can mask arbitrary things.
>
Then I misread you.

Who's the subject of your sentence, that can mask arbitrary things?
kvm.ko in its ioctl,
or save/restore?

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Anthony Liguori]

Glauber Costa wrote:
> On Tue, Feb 3, 2009 at 7:14 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
>>
>> All I said was that if we used KVM_GET_SUPPORTED_CPUID, we had to do
>> something about save/restore since it can mask arbitrary things.
>>
>>     
> Then I misread you.
>
> Who's the subject of your sentence, that can mask arbitrary things?
> kvm.ko in its ioctl,
> or save/restore?
>   

Poor wording on my part.  Your previous patch called 
KVM_GET_SUPPORTED_CPUID which would return the host CPUID bits that KVM 
supports.  QEMU would then use that set to mask out features for the guest.

But the set of bits KVM returns differs on differing platforms.  This 
means that you're now enabling features that were not previously 
enabled.  This isn't a problem, but I'd like to see live migration 
handle this gracefully either by saving off the CPUID bits for the guest 
and failing appropriately when trying to enable CPUID bits on a platform 
that doesn't support it, or an explicit definition of CPUID capabilities.

I don't necessarily have a problem with enabling the most features by 
default, I just want migration to fail gracefully.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Avi Kivity]

Anthony Liguori wrote: 
>
> All I said was that if we used KVM_GET_SUPPORTED_CPUID, we had to do 
> something about save/restore since it can mask arbitrary things.
>

Right, we need to transfer the cpuid bits to the migration target.

btw, we need to read them out of the kernel, since the cpuid instruction 
accesses hidden processor state (the results of cpuid don't depend 
simply on input eax and ecx).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Avi Kivity]

Glauber Costa wrote:
> If the kernel is not 64-bit capable (even if the host
> machine is) do not expose the lm bit in guest cpuid.
>
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..997e4e1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -24,6 +24,7 @@
>  #include <inttypes.h>
>  #include <signal.h>
>  #include <assert.h>
> +#include <sys/utsname.h>
>  
>  #include "cpu.h"
>  #include "exec-all.h"
> @@ -1520,13 +1521,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>  
>          if (kvm_enabled()) {
>              uint32_t h_eax, h_edx;
> +            struct utsname utsname;
> +
> +            uname(&utsname);
>  

It would be better to use KVM_GET_SUPPORTED_CPUID for this (and 
similar).  uname() can lie, and in theory kvm can support x86_64 on i386 
kernels (although patches to implement this would be rejected on 
cost/benefit and aesthetic concerns).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] [PATCH] don't expose lm bit if kernel is not 64-bit capable.

[Glauber Costa]

If the kernel is not 64-bit capable (even if the host
machine is) do not expose the lm bit in guest cpuid.

This patch do the verification inside the host_cpuid
function, for clarity. This way we can encapsulate all
forms of masking in there, and replace with other approaches
if they seem suitable in the future.

Cons are we're calling a syscall multiple times, but this
is hardly the hot path of anything.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 target-i386/helper.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..327d8fc 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -24,6 +24,7 @@
 #include <inttypes.h>
 #include <signal.h>
 #include <assert.h>
+#include <sys/utsname.h>
 
 #include "cpu.h"
 #include "exec-all.h"
@@ -1375,6 +1376,7 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 {
 #if defined(CONFIG_KVM)
     uint32_t vec[4];
+    struct utsname utsname;
 
 #ifdef __x86_64__
     asm volatile("cpuid"
@@ -1399,11 +1401,16 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 	*ebx = vec[1];
     if (ecx)
 	*ecx = vec[2];
-    if (edx)
-	*edx = vec[3];
+    if (edx) {
+        uname(&utsname);
+        if (strcmp(utsname.machine, "x86_64"))
+            vec[3] &= ~0x20000000;
+    	*edx = vec[3];
+    }
 #endif
 }
 
+
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx)
@@ -1526,7 +1533,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
             /* disable CPU features that the host does not support */
 
             /* long mode */
-            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
+            if ((h_edx & 0x20000000) == 0) 
                 *edx &= ~0x20000000;
             /* syscall */
             if ((h_edx & 0x00000800) == 0)
-- 
1.5.6.5