Re: tools/libxc/xc_cpuid_x86.c:cpuid()'s inline asm

[Keir Fraser <keir.xen@gmail.com>]

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

On 02/12/2011 08:54, "Jan Beulich" <JBeulich@suse.com> wrote:

> Keir,
> 
> do you recall what it was that 20976:f8692cc67d67 was supposed to fix?
> The code is clearly wrong now on x86-64 - inline asm that uses push/pop
> (or other stack pointer manipulation instructions) and memory accesses
> that may reference the stack (even if not through %rsp) can't be
> expected to work without -mno-red-zone.

I totally missed the red-zone constraint. :-(

> The question is whether to use that command line option, or whether to
> correct the inline assembly (besides the purpose of your change, I also
> wonder why this isn't coded the obvious way, with rBX and rDX explicitly
> named in the constraints - on 32-bit this may be to reduce register
> pressure, but on 64-bit it's entirely unclear).

I think reg constraint failures had only been reported on 32-bit. So how
about the attached patch?

 -- Keir

> Thanks, Jan
> 


[-- Attachment #2: 00-cpuid-asm --]
[-- Type: application/octet-stream, Size: 2441 bytes --]

diff -r 771e2a303753 tools/libxc/xc_cpuid_x86.c
--- a/tools/libxc/xc_cpuid_x86.c	Fri Dec 02 14:22:47 2011 +0100
+++ b/tools/libxc/xc_cpuid_x86.c	Fri Dec 02 05:59:44 2011 -0800
@@ -43,23 +43,23 @@ static int hypervisor_is_64bit(xc_interf
 static void cpuid(const unsigned int *input, unsigned int *regs)
 {
     unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
+#ifdef __i386__
+    /* Use the stack to avoid reg constraint failures with some gcc flags */
     asm (
-#ifdef __i386__
         "push %%ebx; push %%edx\n\t"
-#else
-        "push %%rbx; push %%rdx\n\t"
-#endif
         "cpuid\n\t"
         "mov %%ebx,4(%4)\n\t"
         "mov %%edx,12(%4)\n\t"
-#ifdef __i386__
         "pop %%edx; pop %%ebx\n\t"
+        : "=a" (regs[0]), "=c" (regs[2])
+        : "0" (input[0]), "1" (count), "S" (_regs)
+        : "memory" );
 #else
-        "pop %%rdx; pop %%rbx\n\t"
+    asm (
+        "cpuid"
+        : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3])
+        : "0" (input[0]), "2" (count) );
 #endif
-        : "=a" (regs[0]), "=c" (regs[2])
-        : "0" (input[0]), "1" (count), "S" (regs)
-        : "memory" );
 }
 
 /* Get the manufacturer brand name of the host processor. */
diff -r 771e2a303753 tools/misc/xen-detect.c
--- a/tools/misc/xen-detect.c	Fri Dec 02 14:22:47 2011 +0100
+++ b/tools/misc/xen-detect.c	Fri Dec 02 05:59:44 2011 -0800
@@ -35,18 +35,21 @@
 
 static void cpuid(uint32_t idx, uint32_t *regs, int pv_context)
 {
+#ifdef __i386__
+    /* Use the stack to avoid reg constraint failures with some gcc flags */
     asm volatile (
-#ifdef __i386__
-#define R(x) "%%e"#x"x"
-#else
-#define R(x) "%%r"#x"x"
-#endif
-        "push "R(a)"; push "R(b)"; push "R(c)"; push "R(d)"\n\t"
+        "push %%eax; push %%ebx; push %%ecx; push %%edx\n\t"
         "test %1,%1 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t"
         "mov %%eax,(%2); mov %%ebx,4(%2)\n\t"
         "mov %%ecx,8(%2); mov %%edx,12(%2)\n\t"
-        "pop "R(d)"; pop "R(c)"; pop "R(b)"; pop "R(a)"\n\t"
+        "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax\n\t"
         : : "a" (idx), "c" (pv_context), "S" (regs) : "memory" );
+#else
+    asm volatile (
+        "test %5,%5 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t"
+        : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3])
+        : "0" (idx), "1" (pv_context), "2" (0) );
+#endif
 }
 
 static int check_for_xen(int pv_context)

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel