[PATCH] fix cpuid function 4

[PATCH] fix cpuid function 4

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Hi,

Currently CPUID function 4 is broken. This function's values rely on the
value of ECX.
To solve the issue cleanly, there is already a new API for cpuid
settings, which is not used yet.
Using the current interface, the function 4 can be easily passed
through, by giving multiple function 4 outputs and increasing the
index-identifier on the fly. This does not break compatibility.

This fix is really important for Mac OS X, as it requires cache
information. Please also see my previous patches for Mac OS X (or rather
core duo target) compatibility.

Regards,

Alex

[-- Attachment #2: kvm-cpuid.patch --]
[-- Type: text/x-patch, Size: 3108 bytes --]

diff --git a/kernel/x86.c b/kernel/x86.c
index b55c177..73312e9 100644
--- a/kernel/x86.c
+++ b/kernel/x86.c
@@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 				    struct kvm_cpuid *cpuid,
 				    struct kvm_cpuid_entry __user *entries)
 {
-	int r, i;
+	int r, i, n = 0;
 	struct kvm_cpuid_entry *cpuid_entries;
 
 	r = -E2BIG;
@@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
 		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
 		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
-		vcpu->arch.cpuid_entries[i].index = 0;
-		vcpu->arch.cpuid_entries[i].flags = 0;
+                switch(vcpu->arch.cpuid_entries[i].function) {
+                    case 4:
+                        vcpu->arch.cpuid_entries[i].index = n;
+                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+                        n++;
+                        break;
+                    default:
+                        vcpu->arch.cpuid_entries[i].index = 0;
+                        vcpu->arch.cpuid_entries[i].flags = 0;
+                        break;
+                }
 		vcpu->arch.cpuid_entries[i].padding[0] = 0;
 		vcpu->arch.cpuid_entries[i].padding[1] = 0;
 		vcpu->arch.cpuid_entries[i].padding[2] = 0;
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index d86fec3..8e5d754 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -461,10 +461,11 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 }
 
 
-static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
+static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function, uint32_t index,
 			 CPUState *env)
 {
     env->regs[R_EAX] = function;
+    env->regs[R_ECX] = index;
     qemu_kvm_cpuid_on_env(env);
     e->function = function;
     e->eax = env->regs[R_EAX];
@@ -515,7 +516,7 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
 #endif
     int cpuid_nent = 0;
     CPUState copy;
-    uint32_t i, limit;
+    uint32_t i, j, limit;
 
     copy = *cenv;
 
@@ -540,16 +541,27 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
     qemu_kvm_cpuid_on_env(&copy);
     limit = copy.regs[R_EAX];
 
-    for (i = 0; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+    for (i = 0; i <= limit; ++i) {
+        switch(i) {
+            case 4:
+                for(j = 0; ; j++) {
+	            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, j, &copy);
+                    if(!copy.regs[R_EAX]) break;
+                }
+                break;
+            default:
+	        do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
+                break;
+        }
+    }
 
     copy.regs[R_EAX] = 0x80000000;
     qemu_kvm_cpuid_on_env(&copy);
     limit = copy.regs[R_EAX];
 
     for (i = 0x80000000; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
 
     kvm_setup_cpuid(kvm_context, cenv->cpu_index, cpuid_nent, cpuid_ent);
     return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] fix cpuid function 4

[Dan Kenigsberg]

On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
> Hi,
> 
> Currently CPUID function 4 is broken. This function's values rely on the
> value of ECX.
> To solve the issue cleanly, there is already a new API for cpuid
> settings, which is not used yet.
> Using the current interface, the function 4 can be easily passed
> through, by giving multiple function 4 outputs and increasing the
> index-identifier on the fly. This does not break compatibility.
> 
> This fix is really important for Mac OS X, as it requires cache
> information. Please also see my previous patches for Mac OS X (or rather
> core duo target) compatibility.
> 
> Regards,
> 
> Alex

> diff --git a/kernel/x86.c b/kernel/x86.c
> index b55c177..73312e9 100644
> --- a/kernel/x86.c
> +++ b/kernel/x86.c
> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  				    struct kvm_cpuid *cpuid,
>  				    struct kvm_cpuid_entry __user *entries)
>  {
> -	int r, i;
> +	int r, i, n = 0;
>  	struct kvm_cpuid_entry *cpuid_entries;
>  
>  	r = -E2BIG;
> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
> -		vcpu->arch.cpuid_entries[i].index = 0;
> -		vcpu->arch.cpuid_entries[i].flags = 0;
> +                switch(vcpu->arch.cpuid_entries[i].function) {
> +                    case 4:
> +                        vcpu->arch.cpuid_entries[i].index = n;
> +                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +                        n++;
> +                        break;
> +                    default:
> +                        vcpu->arch.cpuid_entries[i].index = 0;
> +                        vcpu->arch.cpuid_entries[i].flags = 0;
> +                        break;
> +                }

I will not mention the whitespace damage here :-). Instead, I'd ask you
to review, comment, and even try, the patch that I posted here not long
ago, exposing all safe host cpuid functions to guests.

Thanks,
    Dan.


diff --git a/libkvm/libkvm-x86.c b/libkvm/libkvm-x86.c
index 2fa8146..e9a8113 100644
--- a/libkvm/libkvm-x86.c
+++ b/libkvm/libkvm-x86.c
@@ -485,20 +485,109 @@ __u64 kvm_get_cr8(kvm_context_t kvm, int vcpu)
 }
 
 int kvm_setup_cpuid(kvm_context_t kvm, int vcpu, int nent,
-		    struct kvm_cpuid_entry *entries)
+		    struct kvm_cpuid_entry2 *entries)
 {
-	struct kvm_cpuid *cpuid;
-	int r;
+	int r = -ENOSYS;
 
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0) 
+#endif
+	{ /* kernel has no KVM_SET_CPUID2 */
+		int i;
+		struct kvm_cpuid *cpuid;
+
+		cpuid = malloc(sizeof(*cpuid) +
+				nent * sizeof(struct kvm_cpuid_entry));
+		if (!cpuid)
+			return -ENOMEM;
+
+		cpuid->nent = nent;
+		for (i = 0; i < nent; ++i) {
+			cpuid->entries[i].function = entries[i].function;
+			cpuid->entries[i].eax = entries[i].eax;
+			cpuid->entries[i].ebx = entries[i].ebx;
+			cpuid->entries[i].ecx = entries[i].ecx;
+			cpuid->entries[i].edx = entries[i].edx;
+		}
+		r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID, cpuid);
+		if (r)
+			r = -errno;
+
+		free(cpuid);
+	}
+#ifdef KVM_CAP_EXT_CPUID
+	else {
+		struct kvm_cpuid2 *cpuid;
 	cpuid = malloc(sizeof(*cpuid) + nent * sizeof(*entries));
 	if (!cpuid)
 		return -ENOMEM;
 
 	cpuid->nent = nent;
 	memcpy(cpuid->entries, entries, nent * sizeof(*entries));
-	r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID, cpuid);
+		r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID2, cpuid);
+		if (r)
+			r = -errno;
+
+		free(cpuid);
+	}
+#endif
+	return r;
+}
+
+int kvm_get_supported_cpuid(kvm_context_t kvm, int *nent,
+		struct kvm_cpuid_entry2 *entries)
+{
+	struct kvm_cpuid2 *cpuid;
+	int r = -ENOSYS;
+
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0)
+		return -ENOSYS;
+
+	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
+	if (!cpuid)
+		return -ENOMEM;
+	cpuid->nent = *nent;
 
+	r = ioctl(kvm->vm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
+	if (r)
+		r = -errno;
+	else {
+		memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
+		*nent = cpuid->nent;
+	}
 	free(cpuid);
+#endif
+	return r;
+}
+
+int kvm_get_cpuid(kvm_context_t kvm, int vcpu, int *nent,
+		    struct kvm_cpuid_entry2 *entries)
+{
+	struct kvm_cpuid2 *cpuid;
+	int r = -ENOSYS;
+
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0)
+		return -ENOSYS;
+
+	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
+	if (!cpuid)
+		return -ENOMEM;
+	cpuid->nent = *nent;
+
+	r = ioctl(kvm->vcpu_fd[vcpu], KVM_GET_CPUID2, cpuid);
+	if (r)
+		r = -errno;
+	else {
+		memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
+		*nent = cpuid->nent;
+	}
+	free(cpuid);
+#endif
 	return r;
 }
 
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 2574abe..920c4b1 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -305,6 +305,19 @@ int kvm_inject_irq(kvm_context_t kvm, int vcpu, unsigned irq);
 int kvm_guest_debug(kvm_context_t, int vcpu, struct kvm_debug_guest *dbg);
 
 #if defined(__i386__) || defined(__x86_64__)
+#ifndef KVM_CAP_EXT_CPUID
+/* for compilation against old kernels */
+struct kvm_cpuid_entry2 {
+	__u32 function;
+	__u32 index;
+	__u32 flags;
+	__u32 eax;
+	__u32 ebx;
+	__u32 ecx;
+	__u32 edx;
+	__u32 padding[3];
+};
+#endif
 /*!
  * \brief Setup a vcpu's cpuid instruction emulation
  *
@@ -317,7 +330,36 @@ int kvm_guest_debug(kvm_context_t, int vcpu, struct kvm_debug_guest *dbg);
  * \return 0 on success, or -errno on error
  */
 int kvm_setup_cpuid(kvm_context_t kvm, int vcpu, int nent,
-		    struct kvm_cpuid_entry *entries);
+		    struct kvm_cpuid_entry2 *entries);
+
+/*!
+ * \brief Obtain vcpu's cpuid table
+ *
+ * Get the currently-held table of cpuid functions and their states.\n
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param vcpu Which virtual CPU should be initialized
+ * \param nent Pointer to the number of entries to be obtained.
+ *             On return, the number that was actually obtained.
+ * \param entries cpuid function entries table
+ * \return 0 on success, or -errno on error
+ */
+int kvm_get_cpuid(kvm_context_t kvm, int vcpu, int *nent,
+		    struct kvm_cpuid_entry2 *entries);
+
+/*!
+ * \brief Obtain cpuid supported by kvm
+ *
+ * Obtain a table of cpuid function supported by kvm.\n
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param nent Pointer to the number of entries to be obtained.
+ *             On return, the number that was actually obtained.
+ * \param entries cpuid function entries table
+ * \return 0 on success, or -errno on error
+ */
+int kvm_get_supported_cpuid(kvm_context_t kvm, int *nent,
+	struct kvm_cpuid_entry2 *entries);
 
 /*!
  * \brief Setting the number of shadow pages to be allocated to the vm
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 3972ab4..228b3ad 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -783,6 +783,10 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 #else
         cpu_model = "qemu32";
 #endif
+#ifdef USE_KVM
+        if (kvm_allowed)
+            cpu_model = "host";
+#endif
     }
     
     for(i = 0; i < smp_cpus; i++) {
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index c79ca36..ea08c4e 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -461,8 +461,8 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 }
 
 
-static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
-			 CPUState *env)
+static void do_cpuid_ent(struct kvm_cpuid_entry2 *e, uint32_t function,
+			 CPUState *env, int keep_index_flags)
 {
     env->regs[R_EAX] = function;
     qemu_kvm_cpuid_on_env(env);
@@ -471,6 +471,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
     e->ebx = env->regs[R_EBX];
     e->ecx = env->regs[R_ECX];
     e->edx = env->regs[R_EDX];
+    if (!keep_index_flags)
+        e->index = e->flags = 0;
     if (function == 0x80000001) {
 	uint32_t h_eax, h_edx;
 	struct utsname utsname;
@@ -506,11 +508,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
     }
 }
 
+static struct kvm_cpuid_entry2 *cpuid_entry_lookup(struct kvm_cpuid_entry2 *entries,
+       int nent, uint32_t function, uint32_t index)
+{
+    int i;
+
+    for (i = 0; i < nent; ++i) {
+        struct kvm_cpuid_entry2 *e = &entries[i];
+        if (e->function == function &&
+            (!(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
+               || e->index == index)) {
+                        return e;
+        }
+    }
+    fprintf(stderr, "failed to find cpuid func %x index %x\n", function, index);
+    return 0;
+}
+
 int kvm_arch_qemu_init_env(CPUState *cenv)
 {
-    struct kvm_cpuid_entry cpuid_ent[100];
+    struct kvm_cpuid_entry2 cpuid_ent[100];
 #ifdef KVM_CPUID_SIGNATURE
-    struct kvm_cpuid_entry *pv_ent;
+    struct kvm_cpuid_entry2 *pv_ent;
     uint32_t signature[3];
 #endif
     int cpuid_nent = 0;
@@ -519,6 +538,72 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
 
     copy = *cenv;
 
+    if (cenv->cpuid_hostlike) {
+        struct kvm_cpuid_entry2 *e;
+        int r;
+        extern struct hostlike_user_disable {
+            uint32_t features;
+            uint32_t ext_features;
+            uint32_t ext2_features;
+            uint32_t ext3_features;
+        } hostlike_user_disable;
+
+        cpuid_nent = sizeof(cpuid_ent) / sizeof(struct kvm_cpuid_entry2);
+        r = kvm_get_supported_cpuid(kvm_context, &cpuid_nent, cpuid_ent);
+        if (r) {
+            printf("failed to obtain supported cpuid from kvm module (%d)\n", r);
+            exit(1);
+        }
+
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0, 0);
+        copy.cpuid_level = cenv->cpuid_level = e->eax;
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0x80000000, 0);
+        copy.cpuid_xlevel = cenv->cpuid_xlevel = e->eax;
+
+        /* override some of the advertized data with qemu's */
+        /* family-model-stepping */
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 1, 0);
+        if (cenv->cpuid_version >> 8 == 0)
+            cenv->cpuid_version |= e->eax & 0xFF00;
+        if ((cenv->cpuid_version >> 4 & 0xF) == 0)
+            cenv->cpuid_version |= e->eax & 0xF0;
+        if ((cenv->cpuid_version & 0xF) == 0)
+            cenv->cpuid_version |= e->eax & 0xF;
+        e->eax = cenv->cpuid_version;
+        e->ecx = (e->ecx | cenv->cpuid_ext_features) & ~hostlike_user_disable.ext_features;
+        e->edx = (e->edx | cenv->cpuid_features) & ~hostlike_user_disable.features;
+
+        /* extended features */
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0x80000001, 0);
+        e->eax = (e->eax | cenv->cpuid_features) & ~hostlike_user_disable.features;
+        e->ecx = (e->ecx | cenv->cpuid_ext3_features) & ~hostlike_user_disable.ext3_features;
+        e->edx = (e->edx | cenv->cpuid_ext2_features) & ~hostlike_user_disable.ext2_features;
+
+        /* cpuid_model */
+        {
+            extern const char *cpu_vendor_string;
+            if (cpu_vendor_string)
+                for (i = 0x80000002; i <= 0x80000004; ++i) {
+                    e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, i, 0);
+                    do_cpuid_ent(e, i, &copy, 1);
+                }
+        }
+    } else {
+        copy.regs[R_EAX] = 0;
+        qemu_kvm_cpuid_on_env(&copy);
+        limit = copy.regs[R_EAX];
+
+        for (i = 0; i <= limit; ++i)
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy, 0);
+
+        copy.regs[R_EAX] = 0x80000000;
+        qemu_kvm_cpuid_on_env(&copy);
+        limit = copy.regs[R_EAX];
+
+        for (i = 0x80000000; i <= limit; ++i)
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy, 0);
+    }
+
 #ifdef KVM_CPUID_SIGNATURE
     /* Paravirtualization CPUIDs */
     memcpy(signature, "KVMKVMKVM", 12);
@@ -536,20 +621,6 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
     pv_ent->eax = 0;
 #endif
 
-    copy.regs[R_EAX] = 0;
-    qemu_kvm_cpuid_on_env(&copy);
-    limit = copy.regs[R_EAX];
-
-    for (i = 0; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
-
-    copy.regs[R_EAX] = 0x80000000;
-    qemu_kvm_cpuid_on_env(&copy);
-    limit = copy.regs[R_EAX];
-
-    for (i = 0x80000000; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
-
     kvm_setup_cpuid(kvm_context, cenv->cpu_index, cpuid_nent, cpuid_ent);
     return 0;
 }
diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
index 7143ab3..2034381 100644
--- a/qemu/target-i386/cpu.h
+++ b/qemu/target-i386/cpu.h
@@ -588,6 +588,9 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
+#ifdef USE_KVM
+    int cpuid_hostlike;
+#endif
 
 #ifdef USE_KQEMU
     int kqemu_enabled;
diff --git a/qemu/target-i386/helper2.c b/qemu/target-i386/helper2.c
index ac663aa..f637d82 100644
--- a/qemu/target-i386/helper2.c
+++ b/qemu/target-i386/helper2.c
@@ -123,6 +123,15 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
     return env;
 }
 
+#ifdef USE_KVM
+struct hostlike_user_disable {
+    uint32_t features;
+    uint32_t ext_features;
+    uint32_t ext2_features;
+    uint32_t ext3_features;
+} hostlike_user_disable;
+#endif
+
 typedef struct x86_def_t {
     const char *name;
     uint32_t level;
@@ -131,6 +140,9 @@ typedef struct x86_def_t {
     int model;
     int stepping;
     uint32_t features, ext_features, ext2_features, ext3_features;
+#ifdef USE_KVM
+    int hostlike;
+#endif
     uint32_t xlevel;
 } x86_def_t;
 
@@ -207,6 +219,12 @@ static x86_def_t x86_defs[] = {
         .features = 0x0383F9FF,
         .xlevel = 0,
     },
+#ifdef USE_KVM
+    {
+        .name = "host",
+        .hostlike = 1,
+    },
+#endif
 };
 
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
@@ -229,6 +247,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     }
     if (!def)
         goto error;
+#ifdef USE_KVM
+    if (!kvm_allowed && def->hostlike) {
+        fprintf(stderr, "hostlike cpu requires KVM.\n");
+        goto error;
+    }
+#endif
     memcpy(x86_cpu_def, def, sizeof(*def));
 
     featurestr = strtok(NULL, ",");
@@ -288,6 +312,14 @@ 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;
+#ifdef USE_KVM
+    if (kvm_allowed) {
+        hostlike_user_disable.features = minus_features;
+        hostlike_user_disable.ext_features = minus_ext_features;
+        hostlike_user_disable.ext2_features = minus_ext2_features;
+        hostlike_user_disable.ext3_features = minus_ext3_features;
+    }
+#endif
     free(s);
     return 0;
 
@@ -341,6 +373,9 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
             env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
         }
     }
+#ifdef USE_KVM
+    env->cpuid_hostlike = def->hostlike;
+#endif
     return 0;
 }
 

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] fix cpuid function 4

[Alexander Graf]

Dan Kenigsberg wrote:
> On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
>   
>> Hi,
>>
>> Currently CPUID function 4 is broken. This function's values rely on the
>> value of ECX.
>> To solve the issue cleanly, there is already a new API for cpuid
>> settings, which is not used yet.
>> Using the current interface, the function 4 can be easily passed
>> through, by giving multiple function 4 outputs and increasing the
>> index-identifier on the fly. This does not break compatibility.
>>
>> This fix is really important for Mac OS X, as it requires cache
>> information. Please also see my previous patches for Mac OS X (or rather
>> core duo target) compatibility.
>>
>> Regards,
>>
>> Alex
>>     
>
>   
>> diff --git a/kernel/x86.c b/kernel/x86.c
>> index b55c177..73312e9 100644
>> --- a/kernel/x86.c
>> +++ b/kernel/x86.c
>> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>  				    struct kvm_cpuid *cpuid,
>>  				    struct kvm_cpuid_entry __user *entries)
>>  {
>> -	int r, i;
>> +	int r, i, n = 0;
>>  	struct kvm_cpuid_entry *cpuid_entries;
>>  
>>  	r = -E2BIG;
>> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
>> -		vcpu->arch.cpuid_entries[i].index = 0;
>> -		vcpu->arch.cpuid_entries[i].flags = 0;
>> +                switch(vcpu->arch.cpuid_entries[i].function) {
>> +                    case 4:
>> +                        vcpu->arch.cpuid_entries[i].index = n;
>> +                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>> +                        n++;
>> +                        break;
>> +                    default:
>> +                        vcpu->arch.cpuid_entries[i].index = 0;
>> +                        vcpu->arch.cpuid_entries[i].flags = 0;
>> +                        break;
>> +                }
>>     
>
> I will not mention the whitespace damage here :-). Instead, I'd ask you
>   
Oh well, after having been into qemu source, I just got used to use
spaces instead of tabs ;-).

> to review, comment, and even try, the patch that I posted here not long
> ago, exposing all safe host cpuid functions to guests.
>   
Sure.
Basically your patch targets at a completely different use case than
mine though. You want to expose the host features on the virtual CPU,
whereas my goal is to have a virtual Core Duo/Solo CPU, even if your
host CPU is actually an SVM capable one.

So my CoreDuo CPU definition still fails to populate a proper CPUID
function 4. With the -cpu host option, Linux works (as it's bright
enough to know that some values are just plain wrong), but Darwin
crashes. I am not exactly sure why it is, but I guess it's due to the
function 4 values exposing a 2-core CPU, which kvm simply doesn't emulate.

So far I still like the approach of the cpuid2 API, so it would be great
if you could take the patch one step further and have qemu defined CPUs
work properly as well.

I do not know too much about the kvm code style, so I can't really say
anything regarding that.

Regards,

Alex

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] fix cpuid function 4

[Dan Kenigsberg]

[-- Attachment #1: Type: text/plain, Size: 3716 bytes --]

On Tue, Jan 15, 2008 at 08:57:45AM +0100, Alexander Graf wrote:
> Dan Kenigsberg wrote:
> > On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
> >   
> >> Hi,
> >>
> >> Currently CPUID function 4 is broken. This function's values rely on the
> >> value of ECX.
> >> To solve the issue cleanly, there is already a new API for cpuid
> >> settings, which is not used yet.
> >> Using the current interface, the function 4 can be easily passed
> >> through, by giving multiple function 4 outputs and increasing the
> >> index-identifier on the fly. This does not break compatibility.
> >>
> >> This fix is really important for Mac OS X, as it requires cache
> >> information. Please also see my previous patches for Mac OS X (or rather
> >> core duo target) compatibility.
> >>
> >> Regards,
> >>
> >> Alex
> >>     
> >
> >   
> >> diff --git a/kernel/x86.c b/kernel/x86.c
> >> index b55c177..73312e9 100644
> >> --- a/kernel/x86.c
> >> +++ b/kernel/x86.c
> >> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> >>  				    struct kvm_cpuid *cpuid,
> >>  				    struct kvm_cpuid_entry __user *entries)
> >>  {
> >> -	int r, i;
> >> +	int r, i, n = 0;
> >>  	struct kvm_cpuid_entry *cpuid_entries;
> >>  
> >>  	r = -E2BIG;
> >> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> >>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
> >>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
> >>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
> >> -		vcpu->arch.cpuid_entries[i].index = 0;
> >> -		vcpu->arch.cpuid_entries[i].flags = 0;
> >> +                switch(vcpu->arch.cpuid_entries[i].function) {
> >> +                    case 4:
> >> +                        vcpu->arch.cpuid_entries[i].index = n;
> >> +                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> >> +                        n++;
> >> +                        break;
> >> +                    default:
> >> +                        vcpu->arch.cpuid_entries[i].index = 0;
> >> +                        vcpu->arch.cpuid_entries[i].flags = 0;
> >> +                        break;
> >> +                }
> >>     
> >
> > I will not mention the whitespace damage here :-). Instead, I'd ask you
> >   
> Oh well, after having been into qemu source, I just got used to use
> spaces instead of tabs ;-).
> 
> > to review, comment, and even try, the patch that I posted here not long
> > ago, exposing all safe host cpuid functions to guests.
> >   
> Sure.
> Basically your patch targets at a completely different use case than
> mine though. You want to expose the host features on the virtual CPU,
> whereas my goal is to have a virtual Core Duo/Solo CPU, even if your
> host CPU is actually an SVM capable one.
> 
> So my CoreDuo CPU definition still fails to populate a proper CPUID
> function 4. With the -cpu host option, Linux works (as it's bright
> enough to know that some values are just plain wrong), but Darwin
> crashes. I am not exactly sure why it is, but I guess it's due to the
> function 4 values exposing a 2-core CPU, which kvm simply doesn't emulate.

What I wanted to say is that the fact that the usermode support is not
used, is not IMHO a good-enough reason to change the kernel:
kvm_vcpu_ioctl_set_cpuid() was ment to be a stupid function, to be used
only with old usermode. I hate to teach it the true complex logic of Intel's
CPUID.

What I would like to see is something that uses the cpuid2 API, and not
circumvene it... For this to happen, I need a deep review of my code.

How about the (untested) attched kvm-cpuid.patch, on top of the attached
cpuid-user patch?

Regards,
Dan.

[-- Attachment #2: cpuid-user.patch --]
[-- Type: text/plain, Size: 13576 bytes --]

>From a8b795151e225452745dcd9831b828c06dff438c Mon Sep 17 00:00:00 2001
From: Dan Kenigsberg <danken-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Tue, 15 Jan 2008 15:31:19 +0200
Subject: [PATCH] Add -cpu host option. Expose to guest the CPUID that is supported by the
 host (or specifically requested in the command line).
 Note that the default cpu is now `host' instead of `qemu64'

Signed-off-by: Dan Kenigsberg <danken-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 libkvm/libkvm-x86.c        |   97 +++++++++++++++++++++++++++++++++++++++--
 libkvm/libkvm.h            |   44 ++++++++++++++++++-
 qemu/hw/pc.c               |    4 ++
 qemu/qemu-kvm-x86.c        |  103 ++++++++++++++++++++++++++++++++++++-------
 qemu/target-i386/cpu.h     |    3 +
 qemu/target-i386/helper2.c |   35 +++++++++++++++
 6 files changed, 264 insertions(+), 22 deletions(-)

diff --git a/libkvm/libkvm-x86.c b/libkvm/libkvm-x86.c
index 2fa8146..e9a8113 100644
--- a/libkvm/libkvm-x86.c
+++ b/libkvm/libkvm-x86.c
@@ -485,20 +485,109 @@ __u64 kvm_get_cr8(kvm_context_t kvm, int vcpu)
 }
 
 int kvm_setup_cpuid(kvm_context_t kvm, int vcpu, int nent,
-		    struct kvm_cpuid_entry *entries)
+		    struct kvm_cpuid_entry2 *entries)
 {
-	struct kvm_cpuid *cpuid;
-	int r;
+	int r = -ENOSYS;
 
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0) 
+#endif
+	{ /* kernel has no KVM_SET_CPUID2 */
+		int i;
+		struct kvm_cpuid *cpuid;
+
+		cpuid = malloc(sizeof(*cpuid) +
+				nent * sizeof(struct kvm_cpuid_entry));
+		if (!cpuid)
+			return -ENOMEM;
+
+		cpuid->nent = nent;
+		for (i = 0; i < nent; ++i) {
+			cpuid->entries[i].function = entries[i].function;
+			cpuid->entries[i].eax = entries[i].eax;
+			cpuid->entries[i].ebx = entries[i].ebx;
+			cpuid->entries[i].ecx = entries[i].ecx;
+			cpuid->entries[i].edx = entries[i].edx;
+		}
+		r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID, cpuid);
+		if (r)
+			r = -errno;
+
+		free(cpuid);
+	}
+#ifdef KVM_CAP_EXT_CPUID
+	else {
+		struct kvm_cpuid2 *cpuid;
 	cpuid = malloc(sizeof(*cpuid) + nent * sizeof(*entries));
 	if (!cpuid)
 		return -ENOMEM;
 
 	cpuid->nent = nent;
 	memcpy(cpuid->entries, entries, nent * sizeof(*entries));
-	r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID, cpuid);
+		r = ioctl(kvm->vcpu_fd[vcpu], KVM_SET_CPUID2, cpuid);
+		if (r)
+			r = -errno;
+
+		free(cpuid);
+	}
+#endif
+	return r;
+}
+
+int kvm_get_supported_cpuid(kvm_context_t kvm, int *nent,
+		struct kvm_cpuid_entry2 *entries)
+{
+	struct kvm_cpuid2 *cpuid;
+	int r = -ENOSYS;
+
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0)
+		return -ENOSYS;
+
+	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
+	if (!cpuid)
+		return -ENOMEM;
+	cpuid->nent = *nent;
 
+	r = ioctl(kvm->vm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
+	if (r)
+		r = -errno;
+	else {
+		memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
+		*nent = cpuid->nent;
+	}
 	free(cpuid);
+#endif
+	return r;
+}
+
+int kvm_get_cpuid(kvm_context_t kvm, int vcpu, int *nent,
+		    struct kvm_cpuid_entry2 *entries)
+{
+	struct kvm_cpuid2 *cpuid;
+	int r = -ENOSYS;
+
+#ifdef KVM_CAP_EXT_CPUID
+	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
+	if (r <= 0)
+		return -ENOSYS;
+
+	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
+	if (!cpuid)
+		return -ENOMEM;
+	cpuid->nent = *nent;
+
+	r = ioctl(kvm->vcpu_fd[vcpu], KVM_GET_CPUID2, cpuid);
+	if (r)
+		r = -errno;
+	else {
+		memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
+		*nent = cpuid->nent;
+	}
+	free(cpuid);
+#endif
 	return r;
 }
 
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 2574abe..920c4b1 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -305,6 +305,19 @@ int kvm_inject_irq(kvm_context_t kvm, int vcpu, unsigned irq);
 int kvm_guest_debug(kvm_context_t, int vcpu, struct kvm_debug_guest *dbg);
 
 #if defined(__i386__) || defined(__x86_64__)
+#ifndef KVM_CAP_EXT_CPUID
+/* for compilation against old kernels */
+struct kvm_cpuid_entry2 {
+	__u32 function;
+	__u32 index;
+	__u32 flags;
+	__u32 eax;
+	__u32 ebx;
+	__u32 ecx;
+	__u32 edx;
+	__u32 padding[3];
+};
+#endif
 /*!
  * \brief Setup a vcpu's cpuid instruction emulation
  *
@@ -317,7 +330,36 @@ int kvm_guest_debug(kvm_context_t, int vcpu, struct kvm_debug_guest *dbg);
  * \return 0 on success, or -errno on error
  */
 int kvm_setup_cpuid(kvm_context_t kvm, int vcpu, int nent,
-		    struct kvm_cpuid_entry *entries);
+		    struct kvm_cpuid_entry2 *entries);
+
+/*!
+ * \brief Obtain vcpu's cpuid table
+ *
+ * Get the currently-held table of cpuid functions and their states.\n
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param vcpu Which virtual CPU should be initialized
+ * \param nent Pointer to the number of entries to be obtained.
+ *             On return, the number that was actually obtained.
+ * \param entries cpuid function entries table
+ * \return 0 on success, or -errno on error
+ */
+int kvm_get_cpuid(kvm_context_t kvm, int vcpu, int *nent,
+		    struct kvm_cpuid_entry2 *entries);
+
+/*!
+ * \brief Obtain cpuid supported by kvm
+ *
+ * Obtain a table of cpuid function supported by kvm.\n
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param nent Pointer to the number of entries to be obtained.
+ *             On return, the number that was actually obtained.
+ * \param entries cpuid function entries table
+ * \return 0 on success, or -errno on error
+ */
+int kvm_get_supported_cpuid(kvm_context_t kvm, int *nent,
+	struct kvm_cpuid_entry2 *entries);
 
 /*!
  * \brief Setting the number of shadow pages to be allocated to the vm
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 3972ab4..228b3ad 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -783,6 +783,10 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 #else
         cpu_model = "qemu32";
 #endif
+#ifdef USE_KVM
+        if (kvm_allowed)
+            cpu_model = "host";
+#endif
     }
     
     for(i = 0; i < smp_cpus; i++) {
diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index c79ca36..aa9120c 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -461,7 +461,7 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 }
 
 
-static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
+static void do_cpuid_ent(struct kvm_cpuid_entry2 *e, uint32_t function,
 			 CPUState *env)
 {
     env->regs[R_EAX] = function;
@@ -506,11 +506,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
     }
 }
 
+static struct kvm_cpuid_entry2 *cpuid_entry_lookup(struct kvm_cpuid_entry2 *entries,
+       int nent, uint32_t function, uint32_t index)
+{
+    int i;
+
+    for (i = 0; i < nent; ++i) {
+        struct kvm_cpuid_entry2 *e = &entries[i];
+        if (e->function == function &&
+            (!(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
+               || e->index == index)) {
+                        return e;
+        }
+    }
+    fprintf(stderr, "failed to find cpuid func %x index %x\n", function, index);
+    return 0;
+}
+
 int kvm_arch_qemu_init_env(CPUState *cenv)
 {
-    struct kvm_cpuid_entry cpuid_ent[100];
+    struct kvm_cpuid_entry2 cpuid_ent[100];
 #ifdef KVM_CPUID_SIGNATURE
-    struct kvm_cpuid_entry *pv_ent;
+    struct kvm_cpuid_entry2 *pv_ent;
     uint32_t signature[3];
 #endif
     int cpuid_nent = 0;
@@ -519,6 +536,72 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
 
     copy = *cenv;
 
+    if (cenv->cpuid_hostlike) {
+        struct kvm_cpuid_entry2 *e;
+        int r;
+        extern struct hostlike_user_disable {
+            uint32_t features;
+            uint32_t ext_features;
+            uint32_t ext2_features;
+            uint32_t ext3_features;
+        } hostlike_user_disable;
+
+        cpuid_nent = sizeof(cpuid_ent) / sizeof(struct kvm_cpuid_entry2);
+        r = kvm_get_supported_cpuid(kvm_context, &cpuid_nent, cpuid_ent);
+        if (r) {
+            printf("failed to obtain supported cpuid from kvm module (%d)\n", r);
+            exit(1);
+        }
+
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0, 0);
+        copy.cpuid_level = cenv->cpuid_level = e->eax;
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0x80000000, 0);
+        copy.cpuid_xlevel = cenv->cpuid_xlevel = e->eax;
+
+        /* override some of the advertized data with qemu's */
+        /* family-model-stepping */
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 1, 0);
+        if (cenv->cpuid_version >> 8 == 0)
+            cenv->cpuid_version |= e->eax & 0xFF00;
+        if ((cenv->cpuid_version >> 4 & 0xF) == 0)
+            cenv->cpuid_version |= e->eax & 0xF0;
+        if ((cenv->cpuid_version & 0xF) == 0)
+            cenv->cpuid_version |= e->eax & 0xF;
+        e->eax = cenv->cpuid_version;
+        e->ecx = (e->ecx | cenv->cpuid_ext_features) & ~hostlike_user_disable.ext_features;
+        e->edx = (e->edx | cenv->cpuid_features) & ~hostlike_user_disable.features;
+
+        /* extended features */
+        e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, 0x80000001, 0);
+        e->eax = (e->eax | cenv->cpuid_features) & ~hostlike_user_disable.features;
+        e->ecx = (e->ecx | cenv->cpuid_ext3_features) & ~hostlike_user_disable.ext3_features;
+        e->edx = (e->edx | cenv->cpuid_ext2_features) & ~hostlike_user_disable.ext2_features;
+
+        /* cpuid_model */
+        {
+            extern const char *cpu_vendor_string;
+            if (cpu_vendor_string)
+                for (i = 0x80000002; i <= 0x80000004; ++i) {
+                    e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, i, 0);
+                    do_cpuid_ent(e, i, &copy);
+                }
+        }
+    } else {
+        copy.regs[R_EAX] = 0;
+        qemu_kvm_cpuid_on_env(&copy);
+        limit = copy.regs[R_EAX];
+
+        for (i = 0; i <= limit; ++i)
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+
+        copy.regs[R_EAX] = 0x80000000;
+        qemu_kvm_cpuid_on_env(&copy);
+        limit = copy.regs[R_EAX];
+
+        for (i = 0x80000000; i <= limit; ++i)
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+    }
+
 #ifdef KVM_CPUID_SIGNATURE
     /* Paravirtualization CPUIDs */
     memcpy(signature, "KVMKVMKVM", 12);
@@ -536,20 +619,6 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
     pv_ent->eax = 0;
 #endif
 
-    copy.regs[R_EAX] = 0;
-    qemu_kvm_cpuid_on_env(&copy);
-    limit = copy.regs[R_EAX];
-
-    for (i = 0; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
-
-    copy.regs[R_EAX] = 0x80000000;
-    qemu_kvm_cpuid_on_env(&copy);
-    limit = copy.regs[R_EAX];
-
-    for (i = 0x80000000; i <= limit; ++i)
-	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
-
     kvm_setup_cpuid(kvm_context, cenv->cpu_index, cpuid_nent, cpuid_ent);
     return 0;
 }
diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
index 7143ab3..2034381 100644
--- a/qemu/target-i386/cpu.h
+++ b/qemu/target-i386/cpu.h
@@ -588,6 +588,9 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
+#ifdef USE_KVM
+    int cpuid_hostlike;
+#endif
 
 #ifdef USE_KQEMU
     int kqemu_enabled;
diff --git a/qemu/target-i386/helper2.c b/qemu/target-i386/helper2.c
index ac663aa..f637d82 100644
--- a/qemu/target-i386/helper2.c
+++ b/qemu/target-i386/helper2.c
@@ -123,6 +123,15 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
     return env;
 }
 
+#ifdef USE_KVM
+struct hostlike_user_disable {
+    uint32_t features;
+    uint32_t ext_features;
+    uint32_t ext2_features;
+    uint32_t ext3_features;
+} hostlike_user_disable;
+#endif
+
 typedef struct x86_def_t {
     const char *name;
     uint32_t level;
@@ -131,6 +140,9 @@ typedef struct x86_def_t {
     int model;
     int stepping;
     uint32_t features, ext_features, ext2_features, ext3_features;
+#ifdef USE_KVM
+    int hostlike;
+#endif
     uint32_t xlevel;
 } x86_def_t;
 
@@ -207,6 +219,12 @@ static x86_def_t x86_defs[] = {
         .features = 0x0383F9FF,
         .xlevel = 0,
     },
+#ifdef USE_KVM
+    {
+        .name = "host",
+        .hostlike = 1,
+    },
+#endif
 };
 
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
@@ -229,6 +247,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     }
     if (!def)
         goto error;
+#ifdef USE_KVM
+    if (!kvm_allowed && def->hostlike) {
+        fprintf(stderr, "hostlike cpu requires KVM.\n");
+        goto error;
+    }
+#endif
     memcpy(x86_cpu_def, def, sizeof(*def));
 
     featurestr = strtok(NULL, ",");
@@ -288,6 +312,14 @@ 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;
+#ifdef USE_KVM
+    if (kvm_allowed) {
+        hostlike_user_disable.features = minus_features;
+        hostlike_user_disable.ext_features = minus_ext_features;
+        hostlike_user_disable.ext2_features = minus_ext2_features;
+        hostlike_user_disable.ext3_features = minus_ext3_features;
+    }
+#endif
     free(s);
     return 0;
 
@@ -341,6 +373,9 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
             env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
         }
     }
+#ifdef USE_KVM
+    env->cpuid_hostlike = def->hostlike;
+#endif
     return 0;
 }
 
-- 
1.5.3.7


[-- Attachment #3: kvm-cpuid.patch --]
[-- Type: text/plain, Size: 2380 bytes --]

diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index aa9120c..765a590 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -462,9 +462,10 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 
 
 static void do_cpuid_ent(struct kvm_cpuid_entry2 *e, uint32_t function,
-			 CPUState *env)
+                         uint32_t index, CPUState *env)
 {
     env->regs[R_EAX] = function;
+    env->regs[R_ECX] = index;
     qemu_kvm_cpuid_on_env(env);
     e->function = function;
     e->eax = env->regs[R_EAX];
@@ -532,7 +533,7 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
 #endif
     int cpuid_nent = 0;
     CPUState copy;
-    uint32_t i, limit;
+    uint32_t i, j, limit;
 
     copy = *cenv;
 
@@ -583,7 +584,7 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
             if (cpu_vendor_string)
                 for (i = 0x80000002; i <= 0x80000004; ++i) {
                     e = cpuid_entry_lookup(cpuid_ent, cpuid_nent, i, 0);
-                    do_cpuid_ent(e, i, &copy);
+                    do_cpuid_ent(e, i, 0, &copy);
                 }
         }
     } else {
@@ -591,15 +592,32 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
         qemu_kvm_cpuid_on_env(&copy);
         limit = copy.regs[R_EAX];
 
+        for (i = 0; i <= limit; ++i) {
+            switch(i) {
+                case 4:
+                    for(j = 0; ; j++) {
+                        struct kvm_cpuid_entry2 *e = &cpuid_ent[cpuid_nent++];
+
+                        do_cpuid_ent(e, i, j, &copy);
+                        e->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+                        if (!copy.regs[R_EAX])
+                            break;
+                    }
+                    break;
+                default:
+                    do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
+                    break;
+            }
+        }
         for (i = 0; i <= limit; ++i)
-            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
 
         copy.regs[R_EAX] = 0x80000000;
         qemu_kvm_cpuid_on_env(&copy);
         limit = copy.regs[R_EAX];
 
         for (i = 0x80000000; i <= limit; ++i)
-            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
+            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
     }
 
 #ifdef KVM_CPUID_SIGNATURE

[-- Attachment #4: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #5: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] fix cpuid function 4

[Alexander Graf]

Dan Kenigsberg wrote:
> On Tue, Jan 15, 2008 at 08:57:45AM +0100, Alexander Graf wrote:
>   
>> Dan Kenigsberg wrote:
>>     
>>> On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> Currently CPUID function 4 is broken. This function's values rely on the
>>>> value of ECX.
>>>> To solve the issue cleanly, there is already a new API for cpuid
>>>> settings, which is not used yet.
>>>> Using the current interface, the function 4 can be easily passed
>>>> through, by giving multiple function 4 outputs and increasing the
>>>> index-identifier on the fly. This does not break compatibility.
>>>>
>>>> This fix is really important for Mac OS X, as it requires cache
>>>> information. Please also see my previous patches for Mac OS X (or rather
>>>> core duo target) compatibility.
>>>>
>>>> Regards,
>>>>
>>>> Alex
>>>>     
>>>>         
>>>   
>>>       
>>>> diff --git a/kernel/x86.c b/kernel/x86.c
>>>> index b55c177..73312e9 100644
>>>> --- a/kernel/x86.c
>>>> +++ b/kernel/x86.c
>>>> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>>>  				    struct kvm_cpuid *cpuid,
>>>>  				    struct kvm_cpuid_entry __user *entries)
>>>>  {
>>>> -	int r, i;
>>>> +	int r, i, n = 0;
>>>>  	struct kvm_cpuid_entry *cpuid_entries;
>>>>  
>>>>  	r = -E2BIG;
>>>> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>>>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>>>>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>>>>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
>>>> -		vcpu->arch.cpuid_entries[i].index = 0;
>>>> -		vcpu->arch.cpuid_entries[i].flags = 0;
>>>> +                switch(vcpu->arch.cpuid_entries[i].function) {
>>>> +                    case 4:
>>>> +                        vcpu->arch.cpuid_entries[i].index = n;
>>>> +                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>>>> +                        n++;
>>>> +                        break;
>>>> +                    default:
>>>> +                        vcpu->arch.cpuid_entries[i].index = 0;
>>>> +                        vcpu->arch.cpuid_entries[i].flags = 0;
>>>> +                        break;
>>>> +                }
>>>>     
>>>>         
>>> I will not mention the whitespace damage here :-). Instead, I'd ask you
>>>   
>>>       
>> Oh well, after having been into qemu source, I just got used to use
>> spaces instead of tabs ;-).
>>
>>     
>>> to review, comment, and even try, the patch that I posted here not long
>>> ago, exposing all safe host cpuid functions to guests.
>>>   
>>>       
>> Sure.
>> Basically your patch targets at a completely different use case than
>> mine though. You want to expose the host features on the virtual CPU,
>> whereas my goal is to have a virtual Core Duo/Solo CPU, even if your
>> host CPU is actually an SVM capable one.
>>
>> So my CoreDuo CPU definition still fails to populate a proper CPUID
>> function 4. With the -cpu host option, Linux works (as it's bright
>> enough to know that some values are just plain wrong), but Darwin
>> crashes. I am not exactly sure why it is, but I guess it's due to the
>> function 4 values exposing a 2-core CPU, which kvm simply doesn't emulate.
>>     
>
> What I wanted to say is that the fact that the usermode support is not
> used, is not IMHO a good-enough reason to change the kernel:
> kvm_vcpu_ioctl_set_cpuid() was ment to be a stupid function, to be used
> only with old usermode. I hate to teach it the true complex logic of Intel's
> CPUID.
>
>   

The funny part is, you don't have to. Every complex I know of so far is
simply repetitive. If the userspace just sends x cpuid values and the
kernel takes x, where's the problem?

Of course having a full descriptionary approach is way better, but I see
no real need to not use a stupid interface.

> What I would like to see is something that uses the cpuid2 API, and not
> circumvene it... For this to happen, I need a deep review of my code.
>   

I have to admin that I am really bad at reviewing, so don't expect
anything glorious from me.

> How about the (untested) attched kvm-cpuid.patch, on top of the attached
> cpuid-user patch?
>   

Is there any real difference between this kvm-cpuid.patch and the one I
sent?

What I was really wondering about is, why do you fetch the cpuid
information about the host from the kernel module? CPUID does not get
intercepted and can be easily triggered from userspace.
All the fancy processing of capabilities could be done in userspace as
well (except for features that'd need to be implemented in the kernel,
like MTRR) and this might even reduce the code, and in any case the
amount of code changes in the kernel.

Furthermore most people probably don't even want their host cpu to be
the default one. It renders migration near impossible.

>  #else
>          cpu_model = "qemu32";
>  #endif
> +#ifdef USE_KVM

Why is every other ifdef set on KVM_CAP_EXT_CPUID? Couldn't this be used
here as well?

> +        if (kvm_allowed)
> +            cpu_model = "host";
> +#endif
   
[...]

> +    } else {
> +        copy.regs[R_EAX] = 0;
> +        qemu_kvm_cpuid_on_env(&copy);
> +        limit = copy.regs[R_EAX];
> +
> +        for (i = 0; i <= limit; ++i)
> +            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);

I don't see any index here?

> +
> +        copy.regs[R_EAX] = 0x80000000;
> +        qemu_kvm_cpuid_on_env(&copy);
> +        limit = copy.regs[R_EAX];
> +
> +        for (i = 0x80000000; i <= limit; ++i)
> +            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);

Same here.

> +    }
> +


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix cpuid function 4

[Dan Kenigsberg]

On Wed, Jan 16, 2008 at 06:34:08PM +0100, Alexander Graf wrote:
> Dan Kenigsberg wrote:
> > On Tue, Jan 15, 2008 at 08:57:45AM +0100, Alexander Graf wrote:
> >   
> >> Dan Kenigsberg wrote:
> >>     
> >>> On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
> >>>   
> >>>       
> >>>> Hi,
> >>>>
> >>>> Currently CPUID function 4 is broken. This function's values rely on the
> >>>> value of ECX.
> >>>> To solve the issue cleanly, there is already a new API for cpuid
> >>>> settings, which is not used yet.
> >>>> Using the current interface, the function 4 can be easily passed
> >>>> through, by giving multiple function 4 outputs and increasing the
> >>>> index-identifier on the fly. This does not break compatibility.
> >>>>
> >>>> This fix is really important for Mac OS X, as it requires cache
> >>>> information. Please also see my previous patches for Mac OS X (or rather
> >>>> core duo target) compatibility.
> >>>>
> >>>> Regards,
> >>>>
> >>>> Alex
> >>>>     
> >>>>         
> >>>   
> >>>       
> >>>> diff --git a/kernel/x86.c b/kernel/x86.c
> >>>> index b55c177..73312e9 100644
> >>>> --- a/kernel/x86.c
> >>>> +++ b/kernel/x86.c
> >>>> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> >>>>  				    struct kvm_cpuid *cpuid,
> >>>>  				    struct kvm_cpuid_entry __user *entries)
> >>>>  {
> >>>> -	int r, i;
> >>>> +	int r, i, n = 0;
> >>>>  	struct kvm_cpuid_entry *cpuid_entries;
> >>>>  
> >>>>  	r = -E2BIG;
> >>>> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> >>>>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
> >>>>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
> >>>>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
> >>>> -		vcpu->arch.cpuid_entries[i].index = 0;
> >>>> -		vcpu->arch.cpuid_entries[i].flags = 0;
> >>>> +                switch(vcpu->arch.cpuid_entries[i].function) {
> >>>> +                    case 4:
> >>>> +                        vcpu->arch.cpuid_entries[i].index = n;
> >>>> +                        vcpu->arch.cpuid_entries[i].flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> >>>> +                        n++;
> >>>> +                        break;
> >>>> +                    default:
> >>>> +                        vcpu->arch.cpuid_entries[i].index = 0;
> >>>> +                        vcpu->arch.cpuid_entries[i].flags = 0;
> >>>> +                        break;
> >>>> +                }
> >>>>     
> >>>>         
> >>> I will not mention the whitespace damage here :-). Instead, I'd ask you
> >>>   
> >>>       
> >> Oh well, after having been into qemu source, I just got used to use
> >> spaces instead of tabs ;-).
> >>
> >>     
> >>> to review, comment, and even try, the patch that I posted here not long
> >>> ago, exposing all safe host cpuid functions to guests.
> >>>   
> >>>       
> >> Sure.
> >> Basically your patch targets at a completely different use case than
> >> mine though. You want to expose the host features on the virtual CPU,
> >> whereas my goal is to have a virtual Core Duo/Solo CPU, even if your
> >> host CPU is actually an SVM capable one.
> >>
> >> So my CoreDuo CPU definition still fails to populate a proper CPUID
> >> function 4. With the -cpu host option, Linux works (as it's bright
> >> enough to know that some values are just plain wrong), but Darwin
> >> crashes. I am not exactly sure why it is, but I guess it's due to the
> >> function 4 values exposing a 2-core CPU, which kvm simply doesn't emulate.
> >>     
> >
> > What I wanted to say is that the fact that the usermode support is not
> > used, is not IMHO a good-enough reason to change the kernel:
> > kvm_vcpu_ioctl_set_cpuid() was ment to be a stupid function, to be used
> > only with old usermode. I hate to teach it the true complex logic of Intel's
> > CPUID.
> >
> >   
> 
> The funny part is, you don't have to. Every complex I know of so far is
> simply repetitive. If the userspace just sends x cpuid values and the
> kernel takes x, where's the problem?
> 
> Of course having a full descriptionary approach is way better, but I see
> no real need to not use a stupid interface.

The only reason is that a smarter interface exists, and I want it to be used,
not hacked arround.

> > What I would like to see is something that uses the cpuid2 API, and not
> > circumvene it... For this to happen, I need a deep review of my code.
> >   
> 
> I have to admin that I am really bad at reviewing, so don't expect
> anything glorious from me.

Anything beyond silence would be glorious.

> > How about the (untested) attched kvm-cpuid.patch, on top of the attached
> > cpuid-user patch?
> >   
> 
> Is there any real difference between this kvm-cpuid.patch and the one I
> sent?

There is none. I just wanted to recruit you to test my own patch.

> What I was really wondering about is, why do you fetch the cpuid
> information about the host from the kernel module? 

Because only the kernel knows which cpu features are safe to be exposed. You've
added the SSSSSSSSE3 bit there, haven't you?

> CPUID does not get
> intercepted and can be easily triggered from userspace.
> All the fancy processing of capabilities could be done in userspace as
> well (except for features that'd need to be implemented in the kernel,
> like MTRR) and this might even reduce the code, and in any case the
> amount of code changes in the kernel.

I don't understand. Are you speaking about the cpuid2 api already committed
into kvm.git? Guest cpuid operations are intercepted and handled by the kernel,
don't they?

> Furthermore most people probably don't even want their host cpu to be
> the default one. It renders migration near impossible.

I think "most people" are end users who want to run kvm as fast as they can on
their PC, and care less about migration between different hosts. People who run
kvm in large farms would probably use their own least-common-denominator and
not any default I choose.

> 
> >  #else
> >          cpu_model = "qemu32";
> >  #endif
> > +#ifdef USE_KVM
> 
> Why is every other ifdef set on KVM_CAP_EXT_CPUID? Couldn't this be used
> here as well?

Right. Thanks.

> > +        if (kvm_allowed)
> > +            cpu_model = "host";
> > +#endif
>    
> [...]
> 
> > +    } else {
> > +        copy.regs[R_EAX] = 0;
> > +        qemu_kvm_cpuid_on_env(&copy);
> > +        limit = copy.regs[R_EAX];
> > +
> > +        for (i = 0; i <= limit; ++i)
> > +            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
> 
> I don't see any index here?
> 

That's the job of your kvm-cpuid.patch...

Regards,
    Dan.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fix cpuid function 4

[Alexander Graf]

On Jan 16, 2008, at 9:12 PM, Dan Kenigsberg wrote:

> On Wed, Jan 16, 2008 at 06:34:08PM +0100, Alexander Graf wrote:
>> Dan Kenigsberg wrote:
>>> On Tue, Jan 15, 2008 at 08:57:45AM +0100, Alexander Graf wrote:
>>>
>>>> Dan Kenigsberg wrote:
>>>>
>>>>> On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
>>>>>
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Currently CPUID function 4 is broken. This function's values  
>>>>>> rely on the
>>>>>> value of ECX.
>>>>>> To solve the issue cleanly, there is already a new API for cpuid
>>>>>> settings, which is not used yet.
>>>>>> Using the current interface, the function 4 can be easily passed
>>>>>> through, by giving multiple function 4 outputs and increasing the
>>>>>> index-identifier on the fly. This does not break compatibility.
>>>>>>
>>>>>> This fix is really important for Mac OS X, as it requires cache
>>>>>> information. Please also see my previous patches for Mac OS X  
>>>>>> (or rather
>>>>>> core duo target) compatibility.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>> diff --git a/kernel/x86.c b/kernel/x86.c
>>>>>> index b55c177..73312e9 100644
>>>>>> --- a/kernel/x86.c
>>>>>> +++ b/kernel/x86.c
>>>>>> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct  
>>>>>> kvm_vcpu *vcpu,
>>>>>> 				    struct kvm_cpuid *cpuid,
>>>>>> 				    struct kvm_cpuid_entry __user *entries)
>>>>>> {
>>>>>> -	int r, i;
>>>>>> +	int r, i, n = 0;
>>>>>> 	struct kvm_cpuid_entry *cpuid_entries;
>>>>>>
>>>>>> 	r = -E2BIG;
>>>>>> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct  
>>>>>> kvm_vcpu *vcpu,
>>>>>> 		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>>>>>> 		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>>>>>> 		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
>>>>>> -		vcpu->arch.cpuid_entries[i].index = 0;
>>>>>> -		vcpu->arch.cpuid_entries[i].flags = 0;
>>>>>> +                switch(vcpu->arch.cpuid_entries[i].function) {
>>>>>> +                    case 4:
>>>>>> +                        vcpu->arch.cpuid_entries[i].index = n;
>>>>>> +                        vcpu->arch.cpuid_entries[i].flags =  
>>>>>> KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>>>>>> +                        n++;
>>>>>> +                        break;
>>>>>> +                    default:
>>>>>> +                        vcpu->arch.cpuid_entries[i].index = 0;
>>>>>> +                        vcpu->arch.cpuid_entries[i].flags = 0;
>>>>>> +                        break;
>>>>>> +                }
>>>>>>
>>>>>>
>>>>> I will not mention the whitespace damage here :-). Instead, I'd  
>>>>> ask you
>>>>>
>>>>>
>>>> Oh well, after having been into qemu source, I just got used to use
>>>> spaces instead of tabs ;-).
>>>>
>>>>
>>>>> to review, comment, and even try, the patch that I posted here  
>>>>> not long
>>>>> ago, exposing all safe host cpuid functions to guests.
>>>>>
>>>>>
>>>> Sure.
>>>> Basically your patch targets at a completely different use case  
>>>> than
>>>> mine though. You want to expose the host features on the virtual  
>>>> CPU,
>>>> whereas my goal is to have a virtual Core Duo/Solo CPU, even if  
>>>> your
>>>> host CPU is actually an SVM capable one.
>>>>
>>>> So my CoreDuo CPU definition still fails to populate a proper CPUID
>>>> function 4. With the -cpu host option, Linux works (as it's bright
>>>> enough to know that some values are just plain wrong), but Darwin
>>>> crashes. I am not exactly sure why it is, but I guess it's due to  
>>>> the
>>>> function 4 values exposing a 2-core CPU, which kvm simply doesn't  
>>>> emulate.
>>>>
>>>
>>> What I wanted to say is that the fact that the usermode support is  
>>> not
>>> used, is not IMHO a good-enough reason to change the kernel:
>>> kvm_vcpu_ioctl_set_cpuid() was ment to be a stupid function, to be  
>>> used
>>> only with old usermode. I hate to teach it the true complex logic  
>>> of Intel's
>>> CPUID.
>>>
>>>
>>
>> The funny part is, you don't have to. Every complex I know of so  
>> far is
>> simply repetitive. If the userspace just sends x cpuid values and the
>> kernel takes x, where's the problem?
>>
>> Of course having a full descriptionary approach is way better, but  
>> I see
>> no real need to not use a stupid interface.
>
> The only reason is that a smarter interface exists, and I want it to  
> be used,
> not hacked arround.
>

This is a valid complaint. Still, one wouldn't have needed the smart  
interface in the first place. Now that it is in, one should of course  
use it.

>>> What I would like to see is something that uses the cpuid2 API,  
>>> and not
>>> circumvene it... For this to happen, I need a deep review of my  
>>> code.
>>>
>>
>> I have to admin that I am really bad at reviewing, so don't expect
>> anything glorious from me.
>
> Anything beyond silence would be glorious.
>

Let's break it and get cpuid2 support in libkvm upstream, then! I  
believe having the host CPU's features exposed is a really beneficial  
feature here. I do believe this might be useful in kqemu as well  
though, so you might want to keep it as independent from kvm as  
possible.

Any complaints about these patches from anyone?

>>> How about the (untested) attched kvm-cpuid.patch, on top of the  
>>> attached
>>> cpuid-user patch?
>>>
>>
>> Is there any real difference between this kvm-cpuid.patch and the  
>> one I
>> sent?
>
> There is none. I just wanted to recruit you to test my own patch.
>

Ok, I will.

>> What I was really wondering about is, why do you fetch the cpuid
>> information about the host from the kernel module?
>
> Because only the kernel knows which cpu features are safe to be  
> exposed. You've
> added the SSSSSSSSE3 bit there, haven't you?
>

Right. Maybe I did get something wrong, but where are the host cpuid  
flags received from?

>> CPUID does not get
>> intercepted and can be easily triggered from userspace.
>> All the fancy processing of capabilities could be done in userspace  
>> as
>> well (except for features that'd need to be implemented in the  
>> kernel,
>> like MTRR) and this might even reduce the code, and in any case the
>> amount of code changes in the kernel.
>
> I don't understand. Are you speaking about the cpuid2 api already  
> committed
> into kvm.git? Guest cpuid operations are intercepted and handled by  
> the kernel,
> don't they?
>

I'm talking about normal userspace code running on the host OS. Guest  
CPUIDs of course get intercepted and handled by kvm.

>> Furthermore most people probably don't even want their host cpu to be
>> the default one. It renders migration near impossible.
>
> I think "most people" are end users who want to run kvm as fast as  
> they can on
> their PC, and care less about migration between different hosts.  
> People who run
> kvm in large farms would probably use their own least-common- 
> denominator and
> not any default I choose.
>

I wouldn't bet on this, but it's a valid thought. I don't know which  
direction kvm is going to develop to. S390 support definitely is  
nothing for "end users on their PCs".

>>
>>> #else
>>>         cpu_model = "qemu32";
>>> #endif
>>> +#ifdef USE_KVM
>>
>> Why is every other ifdef set on KVM_CAP_EXT_CPUID? Couldn't this be  
>> used
>> here as well?
>
> Right. Thanks.
>
>>> +        if (kvm_allowed)
>>> +            cpu_model = "host";
>>> +#endif
>>
>> [...]
>>
>>> +    } else {
>>> +        copy.regs[R_EAX] = 0;
>>> +        qemu_kvm_cpuid_on_env(&copy);
>>> +        limit = copy.regs[R_EAX];
>>> +
>>> +        for (i = 0; i <= limit; ++i)
>>> +            do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, &copy);
>>
>> I don't see any index here?
>>
>
> That's the job of your kvm-cpuid.patch...

I assumed you touched it, sorry ;-).

Regards,

Alex

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/