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

[Amit Shah <amit.shah@redhat.com>]

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 */