[PATCH] Fix host_cpuid() in qemu/qemu-kvm-x86.c

[PATCH] Fix host_cpuid() in qemu/qemu-kvm-x86.c

[Bernhard Kaindl]

Hi,

I found that on kvm-61 the cpuid in the guest was reported incorrectly
when qemu-kvm was compiled with gcc-4.1 or 4.3.

This resulted in linux-64bit not booting, complaining that it is not
running on a 64-bit machine.

Symptom: Unexpected behaviour after the assembly snippet.

Solution: New assembly which is simpler and leaves optimizations to gcc,
resulting in much shorter and maintainable code.

Comments are welcome,
Bernhard

PS: Thanks a lot to Alex Graf for the fix.

--- qemu/qemu-kvm-x86.c
+++ qemu/qemu-kvm-x86.c
@@ -428,35 +428,7 @@ static void host_cpuid(uint32_t function
     uint32_t vec[4];
 
     vec[0] = function;
-    asm volatile (
-#ifdef __x86_64__
-	 "sub $128, %%rsp \n\t"  /* skip red zone */
-         "push %0;  push %%rsi \n\t"
-	 "push %%rax; push %%rbx; push %%rcx; push %%rdx \n\t"
-	 "mov 8*5(%%rsp), %%rsi \n\t"
-	 "mov (%%rsi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%rsi) \n\t"
-	 "mov %%ebx, 4(%%rsi) \n\t"
-	 "mov %%ecx, 8(%%rsi) \n\t"
-	 "mov %%edx, 12(%%rsi) \n\t"
-	 "pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax \n\t"
-	 "pop %%rsi; pop %0 \n\t"
-	 "add $128, %%rsp"
-#else
-         "push %0;  push %%esi \n\t"
-	 "push %%eax; push %%ebx; push %%ecx; push %%edx \n\t"
-	 "mov 4*5(%%esp), %%esi \n\t"
-	 "mov (%%esi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%esi) \n\t"
-	 "mov %%ebx, 4(%%esi) \n\t"
-	 "mov %%ecx, 8(%%esi) \n\t"
-	 "mov %%edx, 12(%%esi) \n\t"
-	 "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax \n\t"
-	 "pop %%esi; pop %0 \n\t"
-#endif
-	 : : "rm"(vec) : "memory");
+    asm volatile("cpuid" : "+a" (vec[0]), "=b" (vec[1]),"=c" (vec[2]), "=d" (vec[3]));
     if (eax)
 	*eax = vec[0];
     if (ebx)

-------------------------------------------------------------------------
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 host_cpuid() in qemu/qemu-kvm-x86.c

[Avi Kivity]

Bernhard Kaindl wrote:
> Hi,
>
> I found that on kvm-61 the cpuid in the guest was reported incorrectly
> when qemu-kvm was compiled with gcc-4.1 or 4.3.
>
> This resulted in linux-64bit not booting, complaining that it is not
> running on a 64-bit machine.
>
> Symptom: Unexpected behaviour after the assembly snippet.
>
> Solution: New assembly which is simpler and leaves optimizations to gcc,
> resulting in much shorter and maintainable code.
>
> Comments are welcome,
>   

There were two reasons for the obfuscated code: first, qemu-kvm.c used 
to be compiled with ebx assigned to a global register, which can't be 
clobbered, and second not to cause excessive register pressure on i386.  
The first reason is gone, but can you test the second by compiling on i386?

> Bernhard
>
> PS: Thanks a lot to Alex Graf for the fix.
>
> --- qemu/qemu-kvm-x86.c
> +++ qemu/qemu-kvm-x86.c
> @@ -428,35 +428,7 @@ static void host_cpuid(uint32_t function
>      uint32_t vec[4];
>  
>      vec[0] = function;
> -    asm volatile (
> -#ifdef __x86_64__
> -	 "sub $128, %%rsp \n\t"  /* skip red zone */
> -         "push %0;  push %%rsi \n\t"
> -	 "push %%rax; push %%rbx; push %%rcx; push %%rdx \n\t"
> -	 "mov 8*5(%%rsp), %%rsi \n\t"
> -	 "mov (%%rsi), %%eax \n\t"
> -	 "cpuid \n\t"
> -	 "mov %%eax, (%%rsi) \n\t"
> -	 "mov %%ebx, 4(%%rsi) \n\t"
> -	 "mov %%ecx, 8(%%rsi) \n\t"
> -	 "mov %%edx, 12(%%rsi) \n\t"
> -	 "pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax \n\t"
> -	 "pop %%rsi; pop %0 \n\t"
> -	 "add $128, %%rsp"
> -#else
> -         "push %0;  push %%esi \n\t"
> -	 "push %%eax; push %%ebx; push %%ecx; push %%edx \n\t"
> -	 "mov 4*5(%%esp), %%esi \n\t"
> -	 "mov (%%esi), %%eax \n\t"
> -	 "cpuid \n\t"
> -	 "mov %%eax, (%%esi) \n\t"
> -	 "mov %%ebx, 4(%%esi) \n\t"
> -	 "mov %%ecx, 8(%%esi) \n\t"
> -	 "mov %%edx, 12(%%esi) \n\t"
> -	 "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax \n\t"
> -	 "pop %%esi; pop %0 \n\t"
> -#endif
> -	 : : "rm"(vec) : "memory");
> +    asm volatile("cpuid" : "+a" (vec[0]), "=b" (vec[1]),"=c" (vec[2]), "=d" (vec[3]));
>      if (eax)
>  	*eax = vec[0];
>      if (ebx)
>
> -------------------------------------------------------------------------
> 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/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
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 host_cpuid() in qemu/qemu-kvm-x86.c

[Alexander Graf]

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


On Feb 21, 2008, at 9:46 AM, Avi Kivity wrote:

> Bernhard Kaindl wrote:
>> Hi,
>>
>> I found that on kvm-61 the cpuid in the guest was reported  
>> incorrectly
>> when qemu-kvm was compiled with gcc-4.1 or 4.3.
>>
>> This resulted in linux-64bit not booting, complaining that it is not
>> running on a 64-bit machine.
>>
>> Symptom: Unexpected behaviour after the assembly snippet.
>>
>> Solution: New assembly which is simpler and leaves optimizations to  
>> gcc,
>> resulting in much shorter and maintainable code.
>>
>> Comments are welcome,
>>
>
> There were two reasons for the obfuscated code: first, qemu-kvm.c used
> to be compiled with ebx assigned to a global register, which can't be
> clobbered, and second not to cause excessive register pressure on  
> i386.

If a globally assigned register is in the clobbered list, gcc  
automatically stores and restores it. But if the problem is gone,  
that's even better.

This new patch should address the second issue as well. For most of  
what I tested, using ebx worked perfectly fine. It failed with PIC  
code though, so here you are. We now save and restore ebx in the  
assembly and assign the value to a random free register.

[-- Attachment #2: kvm-cpuid.patch --]
[-- Type: application/octet-stream, Size: 1388 bytes --]

--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -428,35 +428,15 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
     uint32_t vec[4];
 
     vec[0] = function;
-    asm volatile (
 #ifdef __x86_64__
-	 "sub $128, %%rsp \n\t"  /* skip red zone */
-         "push %0;  push %%rsi \n\t"
-	 "push %%rax; push %%rbx; push %%rcx; push %%rdx \n\t"
-	 "mov 8*5(%%rsp), %%rsi \n\t"
-	 "mov (%%rsi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%rsi) \n\t"
-	 "mov %%ebx, 4(%%rsi) \n\t"
-	 "mov %%ecx, 8(%%rsi) \n\t"
-	 "mov %%edx, 12(%%rsi) \n\t"
-	 "pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax \n\t"
-	 "pop %%rsi; pop %0 \n\t"
-	 "add $128, %%rsp"
+    asm volatile("cpuid" : "+a" (vec[0]), "=b" (vec[1]),"=c" (vec[2]), "=d" (vec[3]));
 #else
-         "push %0;  push %%esi \n\t"
-	 "push %%eax; push %%ebx; push %%ecx; push %%edx \n\t"
-	 "mov 4*5(%%esp), %%esi \n\t"
-	 "mov (%%esi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%esi) \n\t"
-	 "mov %%ebx, 4(%%esi) \n\t"
-	 "mov %%ecx, 8(%%esi) \n\t"
-	 "mov %%edx, 12(%%esi) \n\t"
-	 "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax \n\t"
-	 "pop %%esi; pop %0 \n\t"
+    asm volatile("pushl %%ebx \n\t"
+		 "cpuid \n\t"
+		 "movl %%ebx, %0 \n\t"
+		 "popl %%ebx"
+		 : "+a" (vec[0]), "=r" (vec[1]),"=c" (vec[2]), "=d" (vec[3]));
 #endif
-	 : : "rm"(vec) : "memory");
     if (eax)
 	*eax = vec[0];
     if (ebx)

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


>
> The first reason is gone, but can you test the second by compiling  
> on i386?
>
>> Bernhard
>>
>> PS: Thanks a lot to Alex Graf for the fix.
>>
>> --- qemu/qemu-kvm-x86.c
>> +++ qemu/qemu-kvm-x86.c
>> @@ -428,35 +428,7 @@ static void host_cpuid(uint32_t function
>>     uint32_t vec[4];
>>
>>     vec[0] = function;
>> -    asm volatile (
>> -#ifdef __x86_64__
>> -	 "sub $128, %%rsp \n\t"  /* skip red zone */
>> -         "push %0;  push %%rsi \n\t"
>> -	 "push %%rax; push %%rbx; push %%rcx; push %%rdx \n\t"
>> -	 "mov 8*5(%%rsp), %%rsi \n\t"
>> -	 "mov (%%rsi), %%eax \n\t"
>> -	 "cpuid \n\t"
>> -	 "mov %%eax, (%%rsi) \n\t"
>> -	 "mov %%ebx, 4(%%rsi) \n\t"
>> -	 "mov %%ecx, 8(%%rsi) \n\t"
>> -	 "mov %%edx, 12(%%rsi) \n\t"
>> -	 "pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax \n\t"
>> -	 "pop %%rsi; pop %0 \n\t"
>> -	 "add $128, %%rsp"
>> -#else
>> -         "push %0;  push %%esi \n\t"
>> -	 "push %%eax; push %%ebx; push %%ecx; push %%edx \n\t"
>> -	 "mov 4*5(%%esp), %%esi \n\t"
>> -	 "mov (%%esi), %%eax \n\t"
>> -	 "cpuid \n\t"
>> -	 "mov %%eax, (%%esi) \n\t"
>> -	 "mov %%ebx, 4(%%esi) \n\t"
>> -	 "mov %%ecx, 8(%%esi) \n\t"
>> -	 "mov %%edx, 12(%%esi) \n\t"
>> -	 "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax \n\t"
>> -	 "pop %%esi; pop %0 \n\t"
>> -#endif
>> -	 : : "rm"(vec) : "memory");
>> +    asm volatile("cpuid" : "+a" (vec[0]),  
>> "=b" (vec[1]),"=c" (vec[2]), "=d" (vec[3]));
>>     if (eax)
>> 	*eax = vec[0];
>>     if (ebx)
>>
>> -------------------------------------------------------------------------
>> 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/
>> _______________________________________________
>> kvm-devel mailing list
>> kvm-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>>
>
>
> -- 
> error compiling committee.c: too many arguments to function
>
>
> -------------------------------------------------------------------------
> 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/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel

Signed-off-by: Alexander Graf <alex@csgraf.de.>

[-- 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: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix host_cpuid() in qemu/qemu-kvm-x86.c

[Alexander Graf]

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


On Feb 21, 2008, at 10:14 AM, Alexander Graf wrote:

>
> On Feb 21, 2008, at 9:46 AM, Avi Kivity wrote:
>
>> Bernhard Kaindl wrote:
>>> Hi,
>>>
>>> I found that on kvm-61 the cpuid in the guest was reported  
>>> incorrectly
>>> when qemu-kvm was compiled with gcc-4.1 or 4.3.
>>>
>>> This resulted in linux-64bit not booting, complaining that it is not
>>> running on a 64-bit machine.
>>>
>>> Symptom: Unexpected behaviour after the assembly snippet.
>>>
>>> Solution: New assembly which is simpler and leaves optimizations  
>>> to gcc,
>>> resulting in much shorter and maintainable code.
>>>
>>> Comments are welcome,
>>>
>>
>> There were two reasons for the obfuscated code: first, qemu-kvm.c  
>> used
>> to be compiled with ebx assigned to a global register, which can't be
>> clobbered, and second not to cause excessive register pressure on  
>> i386.
>
> If a globally assigned register is in the clobbered list, gcc  
> automatically stores and restores it. But if the problem is gone,  
> that's even better.
>
> This new patch should address the second issue as well. For most of  
> what I tested, using ebx worked perfectly fine. It failed with PIC  
> code though, so here you are. We now save and restore ebx in the  
> assembly and assign the value to a random free register.
> <kvm-cpuid.patch>

This version fixes a wrong identifier (%0 instead of %1) and makes  
things work without touching the stack. This works by using %esi as  
backup-register for %ebx. It furthermore moves "function" directly to  
eax, without relying on gcc optimizations to realize this.

Signed-off-by: Alexander Graf <alex@csgraf.de>


[-- Attachment #2: kvm-cpuid.patch --]
[-- Type: application/octet-stream, Size: 1445 bytes --]

--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -427,36 +427,15 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 {
     uint32_t vec[4];
 
-    vec[0] = function;
-    asm volatile (
 #ifdef __x86_64__
-	 "sub $128, %%rsp \n\t"  /* skip red zone */
-         "push %0;  push %%rsi \n\t"
-	 "push %%rax; push %%rbx; push %%rcx; push %%rdx \n\t"
-	 "mov 8*5(%%rsp), %%rsi \n\t"
-	 "mov (%%rsi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%rsi) \n\t"
-	 "mov %%ebx, 4(%%rsi) \n\t"
-	 "mov %%ecx, 8(%%rsi) \n\t"
-	 "mov %%edx, 12(%%rsi) \n\t"
-	 "pop %%rdx; pop %%rcx; pop %%rbx; pop %%rax \n\t"
-	 "pop %%rsi; pop %0 \n\t"
-	 "add $128, %%rsp"
+    asm volatile("cpuid" : "=a" (vec[0]), "=b" (vec[1]),"=c" (vec[2]), "=d" (vec[3]) : "0"(function));
 #else
-         "push %0;  push %%esi \n\t"
-	 "push %%eax; push %%ebx; push %%ecx; push %%edx \n\t"
-	 "mov 4*5(%%esp), %%esi \n\t"
-	 "mov (%%esi), %%eax \n\t"
-	 "cpuid \n\t"
-	 "mov %%eax, (%%esi) \n\t"
-	 "mov %%ebx, 4(%%esi) \n\t"
-	 "mov %%ecx, 8(%%esi) \n\t"
-	 "mov %%edx, 12(%%esi) \n\t"
-	 "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax \n\t"
-	 "pop %%esi; pop %0 \n\t"
+    asm volatile("movl %%ebx, %%esi \n\t"
+		 "cpuid \n\t"
+		 "movl %%ebx, %1 \n\t"
+		 "movl %%esi, %%ebx"
+		 : "=a" (vec[0]), "=r" (vec[1]), "=c" (vec[2]), "=d" (vec[3]) : "0"(function) : "esi");
 #endif
-	 : : "rm"(vec) : "memory");
     if (eax)
 	*eax = vec[0];
     if (ebx)

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



[-- 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: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix host_cpuid() in qemu/qemu-kvm-x86.c

[Avi Kivity]

Alexander Graf wrote:
>
> This version fixes a wrong identifier (%0 instead of %1) and makes 
> things work without touching the stack. This works by using %esi as 
> backup-register for %ebx. It furthermore moves "function" directly to 
> eax, without relying on gcc optimizations to realize this.
>

Applied, thanks.  Let's see if gcc can handle 4 register constraints + 1 
clobber.

-- 
error compiling committee.c: too many arguments to function


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