[Qemu-devel] [PATCH][STABLE] kvm: Fix cpuid initialization

[Qemu-devel] [PATCH][STABLE] kvm: Fix cpuid initialization

[Jan Kiszka]

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

[ Looks like we need more kvm users via upstream qemu... ]

Fix (more or less) spurious guest boot failures due to corrupted cpuid
states. The reason was insufficient initialization of cpuid entries
before passing them to the kernel.

At this chance also fix improper entry pointer progression and simplify
the code a bit.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-i386/kvm.c |   60 ++++++++++++++++++-----------------------------------
 1 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 4f437c2..2de8b81 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -41,12 +41,11 @@ int kvm_arch_init_vcpu(CPUState *env)
         struct kvm_cpuid_entry2 entries[100];
     } __attribute__((packed)) cpuid_data;
     uint32_t limit, i, j, cpuid_i;
-    uint32_t eax, ebx, ecx, edx;
+    uint32_t unused;
 
     cpuid_i = 0;
 
-    cpu_x86_cpuid(env, 0, 0, &eax, &ebx, &ecx, &edx);
-    limit = eax;
+    cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
 
     for (i = 0; i <= limit; i++) {
         struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
@@ -56,26 +55,17 @@ int kvm_arch_init_vcpu(CPUState *env)
             /* Keep reading function 2 till all the input is received */
             int times;
 
-            cpu_x86_cpuid(env, i, 0, &eax, &ebx, &ecx, &edx);
-            times = eax & 0xff;
-
             c->function = i;
-            c->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
-            c->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
-            c->eax = eax;
-            c->ebx = ebx;
-            c->ecx = ecx;
-            c->edx = edx;
+            c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC |
+                       KVM_CPUID_FLAG_STATE_READ_NEXT;
+            cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+            times = c->eax & 0xff;
 
             for (j = 1; j < times; ++j) {
-                cpu_x86_cpuid(env, i, 0, &eax, &ebx, &ecx, &edx);
+                c = &cpuid_data.entries[cpuid_i++];
                 c->function = i;
-                c->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
-                c->eax = eax;
-                c->ebx = ebx;
-                c->ecx = ecx;
-                c->edx = edx;
-                c = &cpuid_data.entries[++cpuid_i];
+                c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC;
+                cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
             }
             break;
         }
@@ -83,46 +73,36 @@ int kvm_arch_init_vcpu(CPUState *env)
         case 0xb:
         case 0xd:
             for (j = 0; ; j++) {
-                cpu_x86_cpuid(env, i, j, &eax, &ebx, &ecx, &edx);
                 c->function = i;
                 c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
                 c->index = j;
-                c->eax = eax;
-                c->ebx = ebx;
-                c->ecx = ecx;
-                c->edx = edx;
-                c = &cpuid_data.entries[++cpuid_i];
+                cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
 
-                if (i == 4 && eax == 0)
+                if (i == 4 && c->eax == 0)
                     break;
-                if (i == 0xb && !(ecx & 0xff00))
+                if (i == 0xb && !(c->ecx & 0xff00))
                     break;
-                if (i == 0xd && eax == 0)
+                if (i == 0xd && c->eax == 0)
                     break;
+
+                c = &cpuid_data.entries[cpuid_i++];
             }
             break;
         default:
-            cpu_x86_cpuid(env, i, 0, &eax, &ebx, &ecx, &edx);
             c->function = i;
-            c->eax = eax;
-            c->ebx = ebx;
-            c->ecx = ecx;
-            c->edx = edx;
+            c->flags = 0;
+            cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
             break;
         }
     }
-    cpu_x86_cpuid(env, 0x80000000, 0, &eax, &ebx, &ecx, &edx);
-    limit = eax;
+    cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused);
 
     for (i = 0x80000000; i <= limit; i++) {
         struct kvm_cpuid_entry2 *c = &cpuid_data.entries[cpuid_i++];
 
-        cpu_x86_cpuid(env, i, 0, &eax, &ebx, &ecx, &edx);
         c->function = i;
-        c->eax = eax;
-        c->ebx = ebx;
-        c->ecx = ecx;
-        c->edx = edx;
+        c->flags = 0;
+        cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
     }
 
     cpuid_data.cpuid.nent = cpuid_i;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH][STABLE] kvm: Fix cpuid initialization

[Christoph Hellwig]

On Mon, Apr 13, 2009 at 10:53:06AM +0200, Jan Kiszka wrote:
> [ Looks like we need more kvm users via upstream qemu... ]
> 
> Fix (more or less) spurious guest boot failures due to corrupted cpuid
> states. The reason was insufficient initialization of cpuid entries
> before passing them to the kernel.
> 
> At this chance also fix improper entry pointer progression and simplify
> the code a bit.

Is that guest kernel stuck on the "testing hlt" message in the Linux
kernel with that one?  I've seen that hang for a while now, but haven't
been able to bisect it yet.

[Qemu-devel] Re: [PATCH][STABLE] kvm: Fix cpuid initialization

[Jan Kiszka]

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

Christoph Hellwig wrote:
> On Mon, Apr 13, 2009 at 10:53:06AM +0200, Jan Kiszka wrote:
>> [ Looks like we need more kvm users via upstream qemu... ]
>>
>> Fix (more or less) spurious guest boot failures due to corrupted cpuid
>> states. The reason was insufficient initialization of cpuid entries
>> before passing them to the kernel.
>>
>> At this chance also fix improper entry pointer progression and simplify
>> the code a bit.
> 
> Is that guest kernel stuck on the "testing hlt" message in the Linux
> kernel with that one?  I've seen that hang for a while now, but haven't
> been able to bisect it yet.

Maybe. For me it was "This kernel requires an XXX CPU, but only detected
an *i086* CPU." from arch/x86/boot/cpu.c. But if the hlt test depends on
certain cpuid values to do the right thing, your case may be fixed, too.
Does qemu from the kvm tree work for you?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH][STABLE] kvm: Fix cpuid initialization

[Christoph Hellwig]

On Mon, Apr 13, 2009 at 06:20:59PM +0200, Jan Kiszka wrote:
> Maybe. For me it was "This kernel requires an XXX CPU, but only detected
> an *i086* CPU." from arch/x86/boot/cpu.c. But if the hlt test depends on
> certain cpuid values to do the right thing, your case may be fixed, too.

Still hangs for me with your patch applies, so it must be something
else.

> Does qemu from the kvm tree work for you?

I'll have to try getting it build again, last I gave up on the edge of
turning insane over it :)

Re: [Qemu-devel] [PATCH][STABLE] kvm: Fix cpuid initialization

[Anthony Liguori]

Jan Kiszka wrote:
> [ Looks like we need more kvm users via upstream qemu... ]
>
> Fix (more or less) spurious guest boot failures due to corrupted cpuid
> states. The reason was insufficient initialization of cpuid entries
> before passing them to the kernel.
>
> At this chance also fix improper entry pointer progression and simplify
> the code a bit.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

Applied to trunk and stable.   Thanks.


-- 
Regards,

Anthony Liguori