From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LUbCv-0007ZG-I8 for qemu-devel@nongnu.org; Wed, 04 Feb 2009 01:25:57 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LUbCt-0007YJ-7G for qemu-devel@nongnu.org; Wed, 04 Feb 2009 01:25:55 -0500 Received: from [199.232.76.173] (port=59763 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LUbCs-0007YC-Vu for qemu-devel@nongnu.org; Wed, 04 Feb 2009 01:25:55 -0500 Received: from mx2.redhat.com ([66.187.237.31]:40514) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LUbCs-0003Dw-Ai for qemu-devel@nongnu.org; Wed, 04 Feb 2009 01:25:54 -0500 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n146PrIY030431 for ; Wed, 4 Feb 2009 01:25:53 -0500 Date: Wed, 4 Feb 2009 11:56:02 +0530 From: Amit Shah Subject: Re: [Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl Message-ID: <20090204062453.GA6556@amit-x200.pnq.redhat.com> References: <1233249569-16686-1-git-send-email-glommer@redhat.com> <1233249569-16686-2-git-send-email-glommer@redhat.com> <1233249569-16686-3-git-send-email-glommer@redhat.com> <1233249569-16686-4-git-send-email-glommer@redhat.com> <498216B0.9030400@us.ibm.com> <4988B65B.6000308@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4988B65B.6000308@redhat.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Glauber Costa , avi@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 >>> >> >> 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 */