Current KVM head crashes on startup

Current KVM head crashes on startup

[Brian Kress]

When I try to run KVM built off the current head, it crashes with a 
Segmentation fault.  KVM-84 does
not.  Seems to be dealing with the CPUID changes:


    0x081a5c70 in host_cpuid ()
        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
    1426        asm volatile("pusha \n\t"


At a guess, it's this commit:


    commit b9bacf8975c5db7b6f14cabcae98130913f2d013
    Author: aliguori <aliguori>
    Date:   Mon Feb 9 15:50:08 2009 +0000

        KVM: CPUID takes ecx as input value for some functions (Amit Shah)

        The CPUID instruction takes the value of ECX as an input parameter
        in addition to the value of EAX as the count for functions 4, 0xb
        and 0xd. Make sure we pass the value to the instruction.

        Also convert to the qemu-style whitespace for the surrounding code.

        Signed-off-by: Amit Shah <amit.shah@redhat.com>
        Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>


In case it matters, the host CPU is:


    Intel(R) Core(TM)2 CPU          6420  @ 2.13GHz

Re: Current KVM head crashes on startup

[Amit Shah]

On (Tue) Feb 17 2009 [12:47:10], Brian Kress wrote:
> When I try to run KVM built off the current head, it crashes with a  
> Segmentation fault.  KVM-84 does
> not.  Seems to be dealing with the CPUID changes:
>
>
>    0x081a5c70 in host_cpuid ()
>        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
>    1426        asm volatile("pusha \n\t"

This looks like some kind of stack corruption on 32-bit:

1472            if (kvm_enabled())
(gdb)
1473                host_cpuid(0, 0, NULL, ebx, ecx, edx);
(gdb)

Program received signal SIGSEGV, Segmentation fault.
0x081a2d60 in host_cpuid (function=10, count=1231384169, eax=0x0, ebx=0xadfc1914,
    ecx=0xadfc1910, edx=0xadfc190c)
    at /home/amit/src/kvm-userspace/qemu/target-i386/helper.c:1426
1426        asm volatile("pusha \n\t"

I don't see this on 64-bit. Investigating.

Amit

Re: Current KVM head crashes on startup

[Amit Shah]

On (Wed) Feb 18 2009 [13:21:26], Amit Shah wrote:
> On (Tue) Feb 17 2009 [12:47:10], Brian Kress wrote:
> > When I try to run KVM built off the current head, it crashes with a  
> > Segmentation fault.  KVM-84 does
> > not.  Seems to be dealing with the CPUID changes:
> >
> >
> >    0x081a5c70 in host_cpuid ()
> >        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
> >    1426        asm volatile("pusha \n\t"
> 
> This looks like some kind of stack corruption on 32-bit:
> 
> 1472            if (kvm_enabled())
> (gdb)
> 1473                host_cpuid(0, 0, NULL, ebx, ecx, edx);
> (gdb)
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x081a2d60 in host_cpuid (function=10, count=1231384169, eax=0x0, ebx=0xadfc1914,
>     ecx=0xadfc1910, edx=0xadfc190c)
>     at /home/amit/src/kvm-userspace/qemu/target-i386/helper.c:1426
> 1426        asm volatile("pusha \n\t"
> 
> I don't see this on 64-bit. Investigating.

Avi, what's the reason for doing this in the host_cpuid code? As I see
it, the first version should work for both 64-bit and 32-bit code.

#ifdef __x86_64__
    asm volatile("cpuid"
                 : "=a"(vec[0]), "=b"(vec[1]),
                   "=c"(vec[2]), "=d"(vec[3])
                 : "0"(function), "c"(count) : "cc");
#else
    asm volatile("pusha \n\t"
                 "cpuid \n\t"
                 "mov %%eax, 0(%1) \n\t"
                 "mov %%ebx, 4(%1) \n\t"
                 "mov %%ecx, 8(%1) \n\t"
                 "mov %%edx, 12(%1) \n\t"
                 "popa"
                 : : "a"(function), "c"(count), "S"(vec)
                 : "memory", "cc");
#endif

Amit

Re: Current KVM head crashes on startup

[Avi Kivity]

Amit Shah wrote:
> On (Wed) Feb 18 2009 [13:21:26], Amit Shah wrote:
>   
>> On (Tue) Feb 17 2009 [12:47:10], Brian Kress wrote:
>>     
>>> When I try to run KVM built off the current head, it crashes with a  
>>> Segmentation fault.  KVM-84 does
>>> not.  Seems to be dealing with the CPUID changes:
>>>
>>>
>>>    0x081a5c70 in host_cpuid ()
>>>        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
>>>    1426        asm volatile("pusha \n\t"
>>>       
>> This looks like some kind of stack corruption on 32-bit:
>>
>> 1472            if (kvm_enabled())
>> (gdb)
>> 1473                host_cpuid(0, 0, NULL, ebx, ecx, edx);
>> (gdb)
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x081a2d60 in host_cpuid (function=10, count=1231384169, eax=0x0, ebx=0xadfc1914,
>>     ecx=0xadfc1910, edx=0xadfc190c)
>>     at /home/amit/src/kvm-userspace/qemu/target-i386/helper.c:1426
>> 1426        asm volatile("pusha \n\t"
>>
>> I don't see this on 64-bit. Investigating.
>>     
>
> Avi, what's the reason for doing this in the host_cpuid code? As I see
> it, the first version should work for both 64-bit and 32-bit code.
>
> #ifdef __x86_64__
>     asm volatile("cpuid"
>                  : "=a"(vec[0]), "=b"(vec[1]),
>                    "=c"(vec[2]), "=d"(vec[3])
>                  : "0"(function), "c"(count) : "cc");
> #else
>     asm volatile("pusha \n\t"
>                  "cpuid \n\t"
>                  "mov %%eax, 0(%1) \n\t"
>                  "mov %%ebx, 4(%1) \n\t"
>                  "mov %%ecx, 8(%1) \n\t"
>                  "mov %%edx, 12(%1) \n\t"
>                  "popa"
>                  : : "a"(function), "c"(count), "S"(vec)
>                  : "memory", "cc");
> #endif
>   

The first version generates too much register pressure for some 
compilers on i386, leading to compilation failures.  The second version 
is surely wrong, though?  Counting from zero, the "vec" parameter would 
be %2, not %1.


(copied Anthony)

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

Re: Current KVM head crashes on startup

[Amit Shah]

On (Wed) Feb 18 2009 [08:49:33], Avi Kivity wrote:
> Amit Shah wrote:
>> On (Wed) Feb 18 2009 [13:21:26], Amit Shah wrote:
>>   
>>> On (Tue) Feb 17 2009 [12:47:10], Brian Kress wrote:
>>>     
>>>> When I try to run KVM built off the current head, it crashes with a 
>>>>  Segmentation fault.  KVM-84 does
>>>> not.  Seems to be dealing with the CPUID changes:
>>>>
>>>>
>>>>    0x081a5c70 in host_cpuid ()
>>>>        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
>>>>    1426        asm volatile("pusha \n\t"
>>>>       
>>> This looks like some kind of stack corruption on 32-bit:
>>>
>>> 1472            if (kvm_enabled())
>>> (gdb)
>>> 1473                host_cpuid(0, 0, NULL, ebx, ecx, edx);
>>> (gdb)
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x081a2d60 in host_cpuid (function=10, count=1231384169, eax=0x0, ebx=0xadfc1914,
>>>     ecx=0xadfc1910, edx=0xadfc190c)
>>>     at /home/amit/src/kvm-userspace/qemu/target-i386/helper.c:1426
>>> 1426        asm volatile("pusha \n\t"
>>>
>>> I don't see this on 64-bit. Investigating.
>>>     
>>
>> Avi, what's the reason for doing this in the host_cpuid code? As I see
>> it, the first version should work for both 64-bit and 32-bit code.
>>
>> #ifdef __x86_64__
>>     asm volatile("cpuid"
>>                  : "=a"(vec[0]), "=b"(vec[1]),
>>                    "=c"(vec[2]), "=d"(vec[3])
>>                  : "0"(function), "c"(count) : "cc");
>> #else
>>     asm volatile("pusha \n\t"
>>                  "cpuid \n\t"
>>                  "mov %%eax, 0(%1) \n\t"
>>                  "mov %%ebx, 4(%1) \n\t"
>>                  "mov %%ecx, 8(%1) \n\t"
>>                  "mov %%edx, 12(%1) \n\t"
>>                  "popa"
>>                  : : "a"(function), "c"(count), "S"(vec)
>>                  : "memory", "cc");
>> #endif
>>   
>
> The first version generates too much register pressure for some  
> compilers on i386, leading to compilation failures.  The second version  

Is it still valid? I tried with gcc-4.1.2 and that worked fine with the
first version. Should we just use that version instead?

> is surely wrong, though?  Counting from zero, the "vec" parameter would  
> be %2, not %1.

Looks like I missed out updating that when I introduced 'count'. Fixing
that fixes the problem.

Amit

Re: Current KVM head crashes on startup

[Avi Kivity]

Amit Shah wrote:

  

>> The first version generates too much register pressure for some  
>> compilers on i386, leading to compilation failures.  The second version  
>>     
>
> Is it still valid? I tried with gcc-4.1.2 and that worked fine with the
> first version. Should we just use that version instead?
>
>   

I don't see why it would change, unless you can destroy all copies of 
the compilers that fail with it.


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

Re: Current KVM head crashes on startup

[Amit Shah]

On (Wed) Feb 18 2009 [10:19:44], Avi Kivity wrote:
> Amit Shah wrote:
>
>  
>
>>> The first version generates too much register pressure for some   
>>> compilers on i386, leading to compilation failures.  The second 
>>> version      
>>
>> Is it still valid? I tried with gcc-4.1.2 and that worked fine with the
>> first version. Should we just use that version instead?
>>
>>   
>
> I don't see why it would change, unless you can destroy all copies of  
> the compilers that fail with it.

I'd like to know which compilers fail to compile it -- maintaining
specific code can introduce such regressions.

qemu too doesn't have a dependency on gcc-3 anymore.

Also, softwares do periodically bump up the minimum required versions of
their dependencies.

Amit

Re: Current KVM head crashes on startup

[Avi Kivity]

Amit Shah wrote:
>>>   
>>>       
>> I don't see why it would change, unless you can destroy all copies of  
>> the compilers that fail with it.
>>     
>
> I'd like to know which compilers fail to compile it

I don't recall, it probably depends on whether frame pointers are used 
or not as well.

>  -- maintaining
> specific code can introduce such regressions.
>   

That's a problem with assembly.  x86 and x86_64 are different 
instruction sets.

> qemu too doesn't have a dependency on gcc-3 anymore.
>   

We aren't forcing users to use gcc 4.

> Also, softwares do periodically bump up the minimum required versions of
> their dependencies.

Not for this kind of bug.

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

Re: Current KVM head crashes on startup

[Amit Shah]

On (Wed) Feb 18 2009 [11:26:42], Avi Kivity wrote:
> Amit Shah wrote:
>>>>         
>>> I don't see why it would change, unless you can destroy all copies of 
>>>  the compilers that fail with it.
>>>     
>>
>> I'd like to know which compilers fail to compile it
>
> I don't recall, it probably depends on whether frame pointers are used  
> or not as well.

As far as I know, kvm-userspace build arguments have remained the same
for quite some time. Also, we still pass the -g flag for userspace
compilations.

>>  -- maintaining
>> specific code can introduce such regressions.
>>   
>
> That's a problem with assembly.  x86 and x86_64 are different  
> instruction sets.

But the code in question isn't different on the two architectures. Just
a cpuid call that hasn't changed.

>> qemu too doesn't have a dependency on gcc-3 anymore.
>>   
>
> We aren't forcing users to use gcc 4.
>
>> Also, softwares do periodically bump up the minimum required versions of
>> their dependencies.
>
> Not for this kind of bug.

Just enumerating why just destroying all the copies isn't the only option
:-)

OK, given a patch to have just one version of the cpuid call, would you
be willing to take the risk of finding out which users it breaks for?
I'll send patches to revert and restore correct behaviour for 32-bit if
that does happen.

Re: Current KVM head crashes on startup

[Avi Kivity]

Amit Shah wrote:
>> I don't recall, it probably depends on whether frame pointers are used  
>> or not as well.
>>     
>
> As far as I know, kvm-userspace build arguments have remained the same
> for quite some time. Also, we still pass the -g flag for userspace
> compilations.
>
>   

Some distros change CFLAGS.

>>>  -- maintaining
>>> specific code can introduce such regressions.
>>>   
>>>       
>> That's a problem with assembly.  x86 and x86_64 are different  
>> instruction sets.
>>     
>
> But the code in question isn't different on the two architectures. Just
> a cpuid call that hasn't changed.
>
>   

The amount of available registers is different, as is the specification 
of which registers may be clobbered.

> OK, given a patch to have just one version of the cpuid call, would you
> be willing to take the risk of finding out which users it breaks for?
> I'll send patches to revert and restore correct behaviour for 32-bit if
> that does happen.
>   

This is in upstream qemu so it's not my call but I wouldn't recommend 
breaking the build when a trivial one liner is possible.

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

Re: Current KVM head crashes on startup

[Avi Kivity]

Brian Kress wrote:
> When I try to run KVM built off the current head, it crashes with a 
> Segmentation fault.  KVM-84 does
> not.  Seems to be dealing with the CPUID changes:
>
>
>    0x081a5c70 in host_cpuid ()
>        at /home/kressb/kvm/src/qemu/target-i386/helper.c:1426
>    1426        asm volatile("pusha \n\t"

I've pushed a fix for this.

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