[Qemu-devel] [PATCH 0/4] Improve cpuid x86 code.

[Qemu-devel] [PATCH 0/4] Improve cpuid x86 code.

[Glauber Costa]

Hey,

This is a resend of the cpuid patches, following Anthony's last
suggestion. It proved to lead to a much better code IMHO.

I'm also using the existing host_cpuid function to mask out features,
but now relying on kvm's provided ioctl. This seem to provide the expected
behavior.

[Qemu-devel] [PATCH 1/4] convert cpuid registration to KVM_SET_CPUID2

[Glauber Costa]

Use KVM_SET_CPUID2 instead of KVM_SET_CPUID. This
will allow us to do registration of cpuid leaves without
multiple calls to the kernel in the future.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2412ae4..729568c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -36,8 +36,8 @@
 int kvm_arch_init_vcpu(CPUState *env)
 {
     struct {
-        struct kvm_cpuid cpuid;
-        struct kvm_cpuid_entry entries[100];
+        struct kvm_cpuid2 cpuid;
+        struct kvm_cpuid_entry2 entries[100];
     } __attribute__((packed)) cpuid_data;
     uint32_t limit, i, cpuid_i;
     uint32_t eax, ebx, ecx, edx;
@@ -48,7 +48,7 @@ int kvm_arch_init_vcpu(CPUState *env)
     limit = eax;
 
     for (i = 0; i <= limit; i++) {
-        struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+        struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;
@@ -62,7 +62,7 @@ int kvm_arch_init_vcpu(CPUState *env)
     limit = eax;
 
     for (i = 0x80000000; i <= limit; i++) {
-        struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+        struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;
@@ -74,7 +74,7 @@ int kvm_arch_init_vcpu(CPUState *env)
 
     cpuid_data.cpuid.nent = cpuid_i;
 
-    return kvm_vcpu_ioctl(env, KVM_SET_CPUID, &cpuid_data);
+    return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
 }
 
 static int kvm_has_msr_star(CPUState *env)
-- 
1.5.6.5

[Qemu-devel] [PATCH 2/4] Factor out common code in filling cpuid code

[Glauber Costa]

Use kvm_fill_cpuid to query qemu for cpuid data. There are two exactly equal
instances of this code, and the future introduction of kvm paravirt features
will make it three.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 729568c..64b24db 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -33,22 +33,19 @@
     do { } while (0)
 #endif
 
-int kvm_arch_init_vcpu(CPUState *env)
-{
-    struct {
+typedef struct KVMCpuid {
         struct kvm_cpuid2 cpuid;
         struct kvm_cpuid_entry2 entries[100];
-    } __attribute__((packed)) cpuid_data;
-    uint32_t limit, i, cpuid_i;
-    uint32_t eax, ebx, ecx, edx;
+} KVMCpuid;
 
-    cpuid_i = 0;
-
-    cpu_x86_cpuid(env, 0, &eax, &ebx, &ecx, &edx);
-    limit = eax;
+static void kvm_fill_cpuid(CPUState *env, KVMCpuid *cpuid_data,
+                               uint32_t start, uint32_t limit)
+{
+    int i;
+    uint32_t eax, ebx, ecx, edx;
 
-    for (i = 0; i <= limit; i++) {
-        struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
+    for (i = start; i <= limit; i++) {
+        struct kvm_cpuid_entry2 *c = &cpuid_data->entries[cpuid_data->cpuid.nent++];
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;
@@ -57,22 +54,25 @@ int kvm_arch_init_vcpu(CPUState *env)
         c->ecx = ecx;
         c->edx = edx;
     }
+}
 
-    cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
+int kvm_arch_init_vcpu(CPUState *env)
+{
+    KVMCpuid cpuid_data;
+    uint32_t limit;
+    uint32_t eax, ebx, ecx, edx;
+
+    cpuid_data.cpuid.nent = 0;
+
+    cpu_x86_cpuid(env, 0, &eax, &ebx, &ecx, &edx);
     limit = eax;
 
-    for (i = 0x80000000; i <= limit; i++) {
-        struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
+    kvm_fill_cpuid(env, &cpuid_data, 0, limit);
 
-        cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
-        c->function = i;
-        c->eax = eax;
-        c->ebx = ebx;
-        c->ecx = ecx;
-        c->edx = edx;
-    }
+    cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
+    limit = eax;
 
-    cpuid_data.cpuid.nent = cpuid_i;
+    kvm_fill_cpuid(env, &cpuid_data, 0x80000000, limit);
 
     return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
 }
-- 
1.5.6.5

[Qemu-devel] [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Glauber Costa]

KVM has a (so far unused) ioctl to inform userspace
about supported cpuid bits in the current host.

The lack of this kind of checking can lead to bugs in which
a cpuid bit is exposed but not supported by the host kernel
(an example is fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=481274)

The current host_cpuid code is replaced by this general check.

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

diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..1410002 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1370,37 +1370,33 @@ static void breakpoint_handler(CPUState *env)
 }
 #endif /* !CONFIG_USER_ONLY */
 
-static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
+static void host_cpuid(CPUState *env, uint32_t function, uint32_t *eax, uint32_t *ebx,
                        uint32_t *ecx, uint32_t *edx)
 {
 #if defined(CONFIG_KVM)
-    uint32_t vec[4];
+    static struct {
+        struct kvm_cpuid2 cpuid;
+        struct kvm_cpuid_entry2 entries[100];
+    } __attribute__((packed)) cpuid_base;
+    int i;
 
-#ifdef __x86_64__
-    asm volatile("cpuid"
-		 : "=a"(vec[0]), "=b"(vec[1]),
-		   "=c"(vec[2]), "=d"(vec[3])
-		 : "0"(function) : "cc");
-#else
-    asm volatile("pusha \n\t"
-		 "cpuid \n\t"
-		 "mov %%eax, 0(%1) \n\t"
-		 "mov %%ebx, 4(%1) \n\t"
-		 "mov %%ecx, 8(%1) \n\t"
-		 "mov %%edx, 12(%1) \n\t"
-		 "popa"
-		 : : "a"(function), "S"(vec)
-		 : "memory", "cc");
-#endif
+    if (cpuid_base.cpuid.nent == 0) {
+        cpuid_base.cpuid.nent = 100;
+        kvm_ioctl(env->kvm_state, KVM_GET_SUPPORTED_CPUID, &cpuid_base);
+    }
 
-    if (eax)
-	*eax = vec[0];
-    if (ebx)
-	*ebx = vec[1];
-    if (ecx)
-	*ecx = vec[2];
-    if (edx)
-	*edx = vec[3];
+    for (i = 0; i < cpuid_base.cpuid.nent; i++) {
+        if (cpuid_base.entries[i].function == function) {
+            if (eax)
+                *eax = cpuid_base.entries[i].eax;
+            if (ebx)
+                *ebx = cpuid_base.entries[i].ebx;
+            if (ecx)
+                *ecx = cpuid_base.entries[i].ecx;
+            if (edx)
+                *edx = cpuid_base.entries[i].edx;
+        }
+    }
 #endif
 }
 
@@ -1429,7 +1425,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
          * actuall cpu, and say goodbye to migration between different vendors
          * is you use compatibility mode. */
         if (kvm_enabled())
-            host_cpuid(0, NULL, ebx, ecx, edx);
+            host_cpuid(env, 0, NULL, ebx, ecx, edx);
         break;
     case 1:
         *eax = env->cpuid_version;
@@ -1437,9 +1433,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
         *ecx = env->cpuid_ext_features;
         *edx = env->cpuid_features;
 
-        /* "Hypervisor present" bit required for Microsoft SVVP */
-        if (kvm_enabled())
+        if (kvm_enabled()) {
+            uint32_t h_eax;
+            uint32_t h_ecx = ~0U;
+            uint32_t h_edx = ~0U;
+
+            host_cpuid(env, 1, &h_eax, NULL, &h_ecx, &h_edx);
+
+            *ecx &= h_ecx;
+            *edx &= h_edx;
+            /* "Hypervisor present" bit required for Microsoft SVVP */
             *ecx |= (1 << 31);
+        }
         break;
     case 2:
         /* cache info: needed for Pentium Pro compatibility */
@@ -1519,28 +1524,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
         *edx = env->cpuid_ext2_features;
 
         if (kvm_enabled()) {
-            uint32_t h_eax, h_edx;
-
-            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 */)
-                *edx &= ~0x20000000;
-            /* syscall */
-            if ((h_edx & 0x00000800) == 0)
-                *edx &= ~0x00000800;
-            /* nx */
-            if ((h_edx & 0x00100000) == 0)
-                *edx &= ~0x00100000;
+            uint32_t h_eax;
+            uint32_t h_ecx = ~0U;
+            uint32_t h_edx = ~0U;
 
-            /* disable CPU features that KVM cannot support */
+            host_cpuid(env, 0x80000001, &h_eax, NULL, &h_ecx, &h_edx);
 
-            /* svm */
-            *ecx &= ~4UL;
-            /* 3dnow */
-            *edx &= ~0xc0000000;
+            *ecx &= h_ecx;
+            *edx &= h_edx;
         }
         break;
     case 0x80000002:
-- 
1.5.6.5

[Qemu-devel] [PATCH 4/4] kvm pv features PART I

[Glauber Costa]

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

diff --git a/kvm.h b/kvm.h
index efce145..49a2653 100644
--- a/kvm.h
+++ b/kvm.h
@@ -17,6 +17,8 @@
 #include "config.h"
 
 #ifdef CONFIG_KVM
+#include <linux/kvm.h>
+#include <linux/kvm_para.h>
 extern int kvm_allowed;
 
 #define kvm_enabled() (kvm_allowed)
@@ -76,4 +78,7 @@ int kvm_arch_init(KVMState *s, int smp_cpus);
 
 int kvm_arch_init_vcpu(CPUState *env);
 
+/* x86 specific */
+uint32_t kvm_get_para_features(CPUState *env);
+
 #endif
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 1410002..6bcd94d 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1405,7 +1405,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
                    uint32_t *ecx, uint32_t *edx)
 {
     /* test if maximum index reached */
-    if (index & 0x80000000) {
+    if (index & 0x40000000) {
+        /* Those are KVM-specific, dodge them without messing with
+         * exposed cpuid_level */
+    } else if (index & 0x80000000) {
         if (index > env->cpuid_xlevel)
             index = env->cpuid_level;
     } else {
@@ -1511,6 +1514,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
         *ecx = 0;
         *edx = 0;
         break;
+    case 0x40000000:
+        if (kvm_enabled()) {
+            /* KVM CPUID signature */
+            *eax = 0;
+            *ebx = 0x4b4d564b; /* KVMKVMK */
+            *ecx = 0x564b4d56; /* VMKV */
+            *edx = 0x0000004d; /* M */
+        }
+        break;
+    case 0x40000001:
+        if (kvm_enabled()) {
+            *eax = kvm_get_para_features(env);
+        }
+        break;
     case 0x80000000:
         *eax = env->cpuid_xlevel;
         *ebx = env->cpuid_vendor1;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 64b24db..c9f0272 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -56,6 +56,36 @@ static void kvm_fill_cpuid(CPUState *env, KVMCpuid *cpuid_data,
     }
 }
 
+static struct kvm_para_features {
+    int cap;
+    int feature;
+} para_features[] = {
+#ifdef KVM_CAP_CLOCKSOURCE
+       { KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
+#endif
+#ifdef KVM_CAP_NOP_IO_DELAY
+       { KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
+#endif
+#ifdef KVM_CAP_PV_MMU
+       { KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
+#endif
+#ifdef KVM_CAP_CR3_CACHE
+       { KVM_CAP_CR3_CACHE, KVM_FEATURE_CR3_CACHE },
+#endif
+};
+
+uint32_t kvm_get_para_features(CPUState *env)
+ {
+    uint32_t i, features = 0;
+
+    for (i = 0; i < ARRAY_SIZE(para_features); i++) {
+        if (kvm_ioctl(env->kvm_state, KVM_CHECK_EXTENSION, para_features[i].cap))
+            features |= (1 << para_features[i].feature);
+    }
+
+    return features;
+}
+
 int kvm_arch_init_vcpu(CPUState *env)
 {
     KVMCpuid cpuid_data;
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH 4/4] kvm pv features PART I

[Glauber Costa]

On Thu, Jan 29, 2009 at 3:19 PM, Glauber Costa <glommer@redhat.com> wrote:
> Signed-off-by: Glauber Costa <glommer@redhat.com>

Shame on you, what a great changelog entry!

This is where temporary becomes definitive by accident ;-)

Anthony, let me know if you are okay with this patch, and if so, I'll
provide a more
meaningful changelog (that would be easy).

> ---
>  kvm.h                |    5 +++++
>  target-i386/helper.c |   19 ++++++++++++++++++-
>  target-i386/kvm.c    |   30 ++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+), 1 deletions(-)
>
> diff --git a/kvm.h b/kvm.h
> index efce145..49a2653 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -17,6 +17,8 @@
>  #include "config.h"
>
>  #ifdef CONFIG_KVM
> +#include <linux/kvm.h>
> +#include <linux/kvm_para.h>
>  extern int kvm_allowed;
>
>  #define kvm_enabled() (kvm_allowed)
> @@ -76,4 +78,7 @@ int kvm_arch_init(KVMState *s, int smp_cpus);
>
>  int kvm_arch_init_vcpu(CPUState *env);
>
> +/* x86 specific */
> +uint32_t kvm_get_para_features(CPUState *env);
> +
>  #endif
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 1410002..6bcd94d 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1405,7 +1405,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>                    uint32_t *ecx, uint32_t *edx)
>  {
>     /* test if maximum index reached */
> -    if (index & 0x80000000) {
> +    if (index & 0x40000000) {
> +        /* Those are KVM-specific, dodge them without messing with
> +         * exposed cpuid_level */
> +    } else if (index & 0x80000000) {
>         if (index > env->cpuid_xlevel)
>             index = env->cpuid_level;
>     } else {
> @@ -1511,6 +1514,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>         *ecx = 0;
>         *edx = 0;
>         break;
> +    case 0x40000000:
> +        if (kvm_enabled()) {
> +            /* KVM CPUID signature */
> +            *eax = 0;
> +            *ebx = 0x4b4d564b; /* KVMKVMK */
> +            *ecx = 0x564b4d56; /* VMKV */
> +            *edx = 0x0000004d; /* M */
> +        }
> +        break;
> +    case 0x40000001:
> +        if (kvm_enabled()) {
> +            *eax = kvm_get_para_features(env);
> +        }
> +        break;
>     case 0x80000000:
>         *eax = env->cpuid_xlevel;
>         *ebx = env->cpuid_vendor1;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 64b24db..c9f0272 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -56,6 +56,36 @@ static void kvm_fill_cpuid(CPUState *env, KVMCpuid *cpuid_data,
>     }
>  }
>
> +static struct kvm_para_features {
> +    int cap;
> +    int feature;
> +} para_features[] = {
> +#ifdef KVM_CAP_CLOCKSOURCE
> +       { KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
> +#endif
> +#ifdef KVM_CAP_NOP_IO_DELAY
> +       { KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
> +#endif
> +#ifdef KVM_CAP_PV_MMU
> +       { KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
> +#endif
> +#ifdef KVM_CAP_CR3_CACHE
> +       { KVM_CAP_CR3_CACHE, KVM_FEATURE_CR3_CACHE },
> +#endif
> +};
> +
> +uint32_t kvm_get_para_features(CPUState *env)
> + {
> +    uint32_t i, features = 0;
> +
> +    for (i = 0; i < ARRAY_SIZE(para_features); i++) {
> +        if (kvm_ioctl(env->kvm_state, KVM_CHECK_EXTENSION, para_features[i].cap))
> +            features |= (1 << para_features[i].feature);
> +    }
> +
> +    return features;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *env)
>  {
>     KVMCpuid cpuid_data;
> --
> 1.5.6.5
>
>
>
>



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

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

[Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Anthony Liguori]

Glauber Costa wrote:
> KVM has a (so far unused) ioctl to inform userspace
> about supported cpuid bits in the current host.
>
> The lack of this kind of checking can lead to bugs in which
> a cpuid bit is exposed but not supported by the host kernel
> (an example is fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>
> The current host_cpuid code is replaced by this general check.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
>   

So what do we do about live migration?  Does KVM mask a set of bits 
unconditionally?  Before, we were masking unconditionally which should 
insulate migration reasonable well.

Regards,

Anthony Liguori

> ---
>  target-i386/helper.c |   89 ++++++++++++++++++++++---------------------------
>  1 files changed, 40 insertions(+), 49 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..1410002 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1370,37 +1370,33 @@ static void breakpoint_handler(CPUState *env)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>
> -static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
> +static void host_cpuid(CPUState *env, uint32_t function, uint32_t *eax, uint32_t *ebx,
>                         uint32_t *ecx, uint32_t *edx)
>  {
>  #if defined(CONFIG_KVM)
> -    uint32_t vec[4];
> +    static struct {
> +        struct kvm_cpuid2 cpuid;
> +        struct kvm_cpuid_entry2 entries[100];
> +    } __attribute__((packed)) cpuid_base;
> +    int i;
>
> -#ifdef __x86_64__
> -    asm volatile("cpuid"
> -		 : "=a"(vec[0]), "=b"(vec[1]),
> -		   "=c"(vec[2]), "=d"(vec[3])
> -		 : "0"(function) : "cc");
> -#else
> -    asm volatile("pusha \n\t"
> -		 "cpuid \n\t"
> -		 "mov %%eax, 0(%1) \n\t"
> -		 "mov %%ebx, 4(%1) \n\t"
> -		 "mov %%ecx, 8(%1) \n\t"
> -		 "mov %%edx, 12(%1) \n\t"
> -		 "popa"
> -		 : : "a"(function), "S"(vec)
> -		 : "memory", "cc");
> -#endif
> +    if (cpuid_base.cpuid.nent == 0) {
> +        cpuid_base.cpuid.nent = 100;
> +        kvm_ioctl(env->kvm_state, KVM_GET_SUPPORTED_CPUID, &cpuid_base);
> +    }
>
> -    if (eax)
> -	*eax = vec[0];
> -    if (ebx)
> -	*ebx = vec[1];
> -    if (ecx)
> -	*ecx = vec[2];
> -    if (edx)
> -	*edx = vec[3];
> +    for (i = 0; i < cpuid_base.cpuid.nent; i++) {
> +        if (cpuid_base.entries[i].function == function) {
> +            if (eax)
> +                *eax = cpuid_base.entries[i].eax;
> +            if (ebx)
> +                *ebx = cpuid_base.entries[i].ebx;
> +            if (ecx)
> +                *ecx = cpuid_base.entries[i].ecx;
> +            if (edx)
> +                *edx = cpuid_base.entries[i].edx;
> +        }
> +    }
>  #endif
>  }
>
> @@ -1429,7 +1425,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>           * actuall cpu, and say goodbye to migration between different vendors
>           * is you use compatibility mode. */
>          if (kvm_enabled())
> -            host_cpuid(0, NULL, ebx, ecx, edx);
> +            host_cpuid(env, 0, NULL, ebx, ecx, edx);
>          break;
>      case 1:
>          *eax = env->cpuid_version;
> @@ -1437,9 +1433,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>          *ecx = env->cpuid_ext_features;
>          *edx = env->cpuid_features;
>
> -        /* "Hypervisor present" bit required for Microsoft SVVP */
> -        if (kvm_enabled())
> +        if (kvm_enabled()) {
> +            uint32_t h_eax;
> +            uint32_t h_ecx = ~0U;
> +            uint32_t h_edx = ~0U;
> +
> +            host_cpuid(env, 1, &h_eax, NULL, &h_ecx, &h_edx);
> +
> +            *ecx &= h_ecx;
> +            *edx &= h_edx;
> +            /* "Hypervisor present" bit required for Microsoft SVVP */
>              *ecx |= (1 << 31);
> +        }
>          break;
>      case 2:
>          /* cache info: needed for Pentium Pro compatibility */
> @@ -1519,28 +1524,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>          *edx = env->cpuid_ext2_features;
>
>          if (kvm_enabled()) {
> -            uint32_t h_eax, h_edx;
> -
> -            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 */)
> -                *edx &= ~0x20000000;
> -            /* syscall */
> -            if ((h_edx & 0x00000800) == 0)
> -                *edx &= ~0x00000800;
> -            /* nx */
> -            if ((h_edx & 0x00100000) == 0)
> -                *edx &= ~0x00100000;
> +            uint32_t h_eax;
> +            uint32_t h_ecx = ~0U;
> +            uint32_t h_edx = ~0U;
>
> -            /* disable CPU features that KVM cannot support */
> +            host_cpuid(env, 0x80000001, &h_eax, NULL, &h_ecx, &h_edx);
>
> -            /* svm */
> -            *ecx &= ~4UL;
> -            /* 3dnow */
> -            *edx &= ~0xc0000000;
> +            *ecx &= h_ecx;
> +            *edx &= h_edx;
>          }
>          break;
>      case 0x80000002:
>   

Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Glauber Costa]

On Thu, Jan 29, 2009 at 6:50 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Glauber Costa wrote:
>>
>> KVM has a (so far unused) ioctl to inform userspace
>> about supported cpuid bits in the current host.
>>
>> The lack of this kind of checking can lead to bugs in which
>> a cpuid bit is exposed but not supported by the host kernel
>> (an example is fedora bug
>> https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>>
>> The current host_cpuid code is replaced by this general check.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>
>
> So what do we do about live migration?  Does KVM mask a set of bits
> unconditionally?  Before, we were masking unconditionally which should
> insulate migration reasonable well.
>

Just to be clear:

Are you raising the flag against the lack of unconditional mask only,
or about it + querying kvm.ko for supported bits?

I think we need to query it anyway, because calling cpuid will only
give you information about your cpu, and querying the kernel will give
you this, plus information about what the kernel supports (think of
the 32 bit kernel on 64 bit hardware case), plus whatever else kvm
wants to mask for whatever reason.

About migration, I was under the impression that the solution we were
going towards was based on defining a kvm machine type that would
contain a common set of features among the hosts involved. If this is
the case, I believe we don't need to unconditionally mask out
anything, since the features we expose in the machine type won't even
have the problematic features.

If this is not the case, I'm fine with redoing this patch with
querying kvm.ko + unconditional mask of potentially problematic
features.

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

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

Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Avi Kivity]

Anthony Liguori wrote:
> Glauber Costa wrote:
>> KVM has a (so far unused) ioctl to inform userspace
>> about supported cpuid bits in the current host.
>>
>> The lack of this kind of checking can lead to bugs in which
>> a cpuid bit is exposed but not supported by the host kernel
>> (an example is fedora bug 
>> https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>>
>> The current host_cpuid code is replaced by this general check.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>   
>
> So what do we do about live migration?  

We need a way to completely specify cpuid from the command line.  The 
management utility would figure out what is supported (driven by both 
cpu capabilities and installed kvm capabilities) and fgure out a 
greatest common denominator, or something.

> Does KVM mask a set of bits unconditionally?  

The kernel doesn't touch cpuid (except nx due to an historical mistake).

> Before, we were masking unconditionally which should insulate 
> migration reasonable well.

That suits migration, but not other uses of qemu.

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

Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Amit Shah]

On (Tue) Feb 03 2009 [23:25:47], Avi Kivity wrote:
> Anthony Liguori wrote:
>> Glauber Costa wrote:
>>> KVM has a (so far unused) ioctl to inform userspace
>>> about supported cpuid bits in the current host.
>>>
>>> The lack of this kind of checking can lead to bugs in which
>>> a cpuid bit is exposed but not supported by the host kernel
>>> (an example is fedora bug  
>>> https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>>>
>>> The current host_cpuid code is replaced by this general check.
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>>   
>>
>> So what do we do about live migration?  
>
> We need a way to completely specify cpuid from the command line.  The  
> management utility would figure out what is supported (driven by both  
> cpu capabilities and installed kvm capabilities) and fgure out a  
> greatest common denominator, or something.

I had proposed this a few days ago, pasted below:

http://thread.gmane.org/gmane.comp.emulators.qemu/37134/focus=37306

---

What I'm thinking of is moving all the CPU definitions out from helper.c
into $(PREFIX)/usr/share/qemu/cpu-defs/... with one file holding one
definition, like
$ cat core2duo.def
model= 15
family = 16
stepping = 11
level = 10
.
.
.

Adding a new def then just becomes a matter of submitting a text file
for inclusion and doesn't need any recompilation.

Also, there are many details not captured in a CPU def, like the cache
sizes and types. They're hard-coded for all the CPUs in the cpuid helper
function. So we could have those values too incorporated in the CPU def
file an fall back to the defaults when not available.

When we do get some kind of a machine FDT, we can specify the cpu type
by either pointing to one of these files or by constructing a new CPU
type by specifying individual values.

---

I have a patch that adds some of this functionality to the command line.
It needs cleaning up, but shows the general idea that I'm progressing
in. It takes as input a file from which to read the cpuid definitions.
The cpuid defs could be straight from a host or from a management
application that has computed the "greatest common denominator":


diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
index 944e386..98dc4d0 100644
--- a/qemu/target-i386/cpu.h
+++ b/qemu/target-i386/cpu.h
@@ -643,6 +643,12 @@ typedef struct CPUX86State {
     int mp_state;
 } CPUX86State;
 
+struct KMScpuid {
+    uint32_t function, eax, ebx, ecx, edx;
+};
+extern struct KMScpuid *kms_cpuids;
+extern uint32_t kms_leaves;
+
 CPUX86State *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void cpu_x86_close(CPUX86State *s);
diff --git a/qemu/target-i386/helper.c b/qemu/target-i386/helper.c
index cda0390..33aed77 100644
--- a/qemu/target-i386/helper.c
+++ b/qemu/target-i386/helper.c
@@ -264,8 +264,15 @@ static x86_def_t x86_defs[] = {
         .xlevel = 0x8000000A,
         .model_id = "Intel(R) Atom(TM) CPU N270   @ 1.60GHz",
     },
+    {
+        .name = "migrate-safe",
+    },
 };
 
+u_int32_t kms_leaves;
+struct KMScpuid *kms_cpuids;
+static char kms_file_path[100];
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 {
     unsigned int i;
@@ -338,6 +345,8 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
                         val);
+            } else if (!strncmp(featurestr, "cpuids", 6)) {
+                pstrcpy(kms_file_path, sizeof(kms_file_path), val);
             } else {
                 fprintf(stderr, "unrecognized feature %s\n", featurestr);
                 goto error;
@@ -356,6 +365,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     x86_cpu_def->ext_features &= ~minus_ext_features;
     x86_cpu_def->ext2_features &= ~minus_ext2_features;
     x86_cpu_def->ext3_features &= ~minus_ext3_features;
+
     free(s);
     return 0;
 
@@ -372,12 +382,74 @@ void x86_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
         (*cpu_fprintf)(f, "x86 %16s\n", x86_defs[i].name);
 }
 
+static int get_cpuids(x86_def_t *def)
+{
+    int leaves, i, r;
+    struct KMScpuid cpuid_line;
+    FILE *kms_fp;
+
+    kms_fp = fopen(kms_file_path, "r");
+    if (!kms_fp) {
+        fprintf(stderr, "Can't find cpuid file\n");
+        return -1;
+    }
+    fscanf(kms_fp, "%08x %08x %08x %08x %08x\n",
+           &cpuid_line.function, &cpuid_line.eax, &cpuid_line.ebx,
+           &cpuid_line.ecx, &cpuid_line.edx);
+
+    def->level = cpuid_line.eax;
+    leaves = cpuid_line.eax + 1;
+    r = fseek(kms_fp, 45 * (leaves - 1), SEEK_CUR);
+    if (r < 0) {
+        fprintf(stderr, "Error reading cpuid input, %s\n", strerror(errno));
+        return -1;
+    }
+    fscanf(kms_fp, "%08x %08x %08x %08x %08x\n",
+           &cpuid_line.function, &cpuid_line.eax, &cpuid_line.ebx,
+           &cpuid_line.ecx, &cpuid_line.edx);
+
+    if (cpuid_line.function == 0x80000000) {
+        leaves += cpuid_line.eax - 0x80000000 + 1;
+        def->xlevel = cpuid_line.eax;
+    }
+    kms_cpuids = malloc(sizeof(struct KMScpuid)*leaves);
+    if (!kms_cpuids) {
+        fprintf(stderr, "Not enough memory allocating leaves\n");
+        return -1;
+    }
+    rewind(kms_fp);
+    kms_leaves = leaves;
+    for (i = 0; i < leaves; i++) {
+        r = fscanf(kms_fp, "%08x %08x %08x %08x %08x\n",
+                   &kms_cpuids[i].function,
+                   &kms_cpuids[i].eax, &kms_cpuids[i].ebx,
+                   &kms_cpuids[i].ecx, &kms_cpuids[i].edx);
+        if (r == EOF)
+            break;
+    }
+    fclose(kms_fp);
+    return 0;
+}
+
 static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
 
     if (cpu_x86_find_by_name(def, cpu_model) < 0)
         return -1;
+    if (!strncmp(cpu_model, "migrate-safe", 12)) {
+        if (get_cpuids(def) < 0)
+            return -1;
+
+        def->vendor1 = kms_cpuids[0].ebx;
+        def->vendor2 = kms_cpuids[0].edx;
+        def->vendor3 = kms_cpuids[0].ecx;
+
+        def->features = kms_cpuids[1].edx;
+        def->ext_features = kms_cpuids[1].ecx;
+        def->ext2_features = kms_cpuids[def->level + 2].edx;
+        def->ext3_features = kms_cpuids[def->level + 2].ecx;
+    }
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -388,19 +460,25 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
         env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
     }
     env->cpuid_level = def->level;
-    if (def->family > 0x0f)
-        env->cpuid_version = 0xf00 | ((def->family - 0x0f) << 20);
-    else
-        env->cpuid_version = def->family << 8;
-    env->cpuid_version |= ((def->model & 0xf) << 4) | ((def->model >> 4) << 16);
-    env->cpuid_version |= def->stepping;
+    if (!kms_leaves) {
+        if (def->family > 0x0f)
+            env->cpuid_version = 0xf00 | ((def->family - 0x0f) << 20);
+        else
+            env->cpuid_version = def->family << 8;
+        env->cpuid_version |=
+            ((def->model & 0xf) << 4) | ((def->model >> 4) << 16);
+        env->cpuid_version |= def->stepping;
+    } else
+        env->cpuid_version = kms_cpuids[1].eax;
+
     env->cpuid_features = def->features;
     env->pat = 0x0007040600070406ULL;
     env->cpuid_ext_features = def->ext_features;
     env->cpuid_ext2_features = def->ext2_features;
     env->cpuid_xlevel = def->xlevel;
     env->cpuid_ext3_features = def->ext3_features;
-    {
+
+    if (!kms_leaves) {
         const char *model_id = def->model_id;
         int c, len, i;
 
@@ -416,6 +494,16 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
                 c = (uint8_t)model_id[i];
             env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
         }
+    } else {
+        uint32_t i;
+
+        /* Leaves 0x80000002 - 0x80000004 store the model information */
+        for (i = 0; i < 3; i++) {
+            env->cpuid_model[i * 4 + 0] = kms_cpuids[def->level + 3 + i].eax;
+            env->cpuid_model[i * 4 + 1] = kms_cpuids[def->level + 3 + i].ebx;
+            env->cpuid_model[i * 4 + 2] = kms_cpuids[def->level + 3 + i].ecx;
+            env->cpuid_model[i * 4 + 3] = kms_cpuids[def->level + 3 + i].edx;
+        }
     }
     return 0;
 }
@@ -1383,6 +1471,21 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 #if defined(CONFIG_KVM) || defined(USE_KVM)
     uint32_t vec[4];
 
+    if (kms_leaves) {
+        /* We're using a migrate-safe CPU type */
+        int i;
+
+        for (i = 0; i <= function && i < kms_leaves; i++) {
+            if (kms_cpuids[i].function == function) {
+                vec[0] = kms_cpuids[i].eax;
+                vec[1] = kms_cpuids[i].ebx;
+                vec[2] = kms_cpuids[i].ecx;
+                vec[3] = kms_cpuids[i].edx;
+                break;
+            }
+        }
+    } else {
+
 #ifdef __x86_64__
     asm volatile("cpuid"
 		 : "=a"(vec[0]), "=b"(vec[1]),
@@ -1399,6 +1502,7 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 		 : : "a"(function), "S"(vec)
 		 : "memory", "cc");
 #endif
+    }
 
     if (eax)
 	*eax = vec[0];
@@ -1435,7 +1539,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
          * isn't supported in compatibility mode on Intel.  so advertise the
          * actuall cpu, and say goodbye to migration between different vendors
          * is you use compatibility mode. */
-        if (kvm_enabled())
+        if (!kms_leaves && kvm_enabled())
             host_cpuid(0, NULL, ebx, ecx, edx);
         break;
     case 1:
@@ -1528,19 +1632,21 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
         if (kvm_enabled()) {
             uint32_t h_eax, h_edx;
 
-            host_cpuid(0x80000001, &h_eax, NULL, NULL, &h_edx);
+            if (!kms_leaves) {
+                host_cpuid(0x80000001, &h_eax, NULL, NULL, &h_edx);
 
-            /* disable CPU features that the host does not support */
+                /* disable CPU features that the host does not support */
 
-            /* long mode */
-            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
-                *edx &= ~0x20000000;
-            /* syscall */
-            if ((h_edx & 0x00000800) == 0)
-                *edx &= ~0x00000800;
-            /* nx */
-            if ((h_edx & 0x00100000) == 0)
-                *edx &= ~0x00100000;
+                /* long mode */
+                if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
+                    *edx &= ~0x20000000;
+                /* syscall */
+                if ((h_edx & 0x00000800) == 0)
+                    *edx &= ~0x00000800;
+                /* nx */
+                if ((h_edx & 0x00100000) == 0)
+                    *edx &= ~0x00100000;
+            }
 
             /* disable CPU features that KVM cannot support */
 

Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Avi Kivity]

Anthony Liguori wrote:
> Glauber Costa wrote:
>> KVM has a (so far unused) ioctl to inform userspace
>> about supported cpuid bits in the current host.
>>
>> The lack of this kind of checking can lead to bugs in which
>> a cpuid bit is exposed but not supported by the host kernel
>> (an example is fedora bug 
>> https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>>
>> The current host_cpuid code is replaced by this general check.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>   
>
> So what do we do about live migration?  

We need a way to completely specify cpuid from the command line.  The 
management utility would figure out what is supported (driven by both 
cpu capabilities and installed kvm capabilities) and fgure out a 
greatest common denominator, or something.

> Does KVM mask a set of bits unconditionally?  

The kernel doesn't touch cpuid (except nx due to an historical mistake).

> Before, we were masking unconditionally which should insulate 
> migration reasonable well.

That suits migration, but not other uses of qemu.

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

Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Glauber Costa wrote:
>>> KVM has a (so far unused) ioctl to inform userspace
>>> about supported cpuid bits in the current host.
>>>
>>> The lack of this kind of checking can lead to bugs in which
>>> a cpuid bit is exposed but not supported by the host kernel
>>> (an example is fedora bug 
>>> https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>>>
>>> The current host_cpuid code is replaced by this general check.
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>>   
>>
>> So what do we do about live migration?  
>
> We need a way to completely specify cpuid from the command line.  The 
> management utility would figure out what is supported (driven by both 
> cpu capabilities and installed kvm capabilities) and fgure out a 
> greatest common denominator, or something.

What is the default case?  Pass everything through or pick a reasonable 
small, constant subset?  That is, is -cpuid a union or an intersection.

Regards,

Anthony Liguori

>> Does KVM mask a set of bits unconditionally?  
>
> The kernel doesn't touch cpuid (except nx due to an historical mistake).
>
>> Before, we were masking unconditionally which should insulate 
>> migration reasonable well.
>
> That suits migration, but not other uses of qemu.
>

Re: [Qemu-devel] [PATCH 0/4] Improve cpuid x86 code.

[Amit Shah]

On (Thu) Jan 29 2009 [12:19:25], Glauber Costa wrote:
> Hey,
> 
> This is a resend of the cpuid patches, following Anthony's last
> suggestion. It proved to lead to a much better code IMHO.
> 
> I'm also using the existing host_cpuid function to mask out features,
> but now relying on kvm's provided ioctl. This seem to provide the expected
> behavior.

I had submitted patches previously to convert to get_cpuid2. If Anthony
has them in his queue, we're going to have clashes. My patches add full
support for cupid functions 4, 0xb and 0xd that's missing currently.

I also have a patch ready for function 2; guess I'll send them after
this stuff is in.