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

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

[Keir Fraser]

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

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

[Keir Fraser]

On 02/12/2011 15:26, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Fri, Dec 02, Keir Fraser wrote:
> 
>> I think reg constraint failures had only been reported on 32-bit. So how
>> about the attached patch?
> 
> +        : "0" (input[0]), "1" (count), "S" (_regs)
> 
> _regs is undeclared in 32bit, I think it should be regs.

Oops! Fixed!

 Thanks,
 Keir

> Olaf

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

[Jan Beulich]

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.

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).

Thanks, Jan

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

[Keir Fraser]

On 02/12/2011 16:47, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Fri, Dec 02, Keir Fraser wrote:
> 
>> I think reg constraint failures had only been reported on 32-bit. So how
>> about the attached patch?
> 
> So finally I was able to test this change, and it fixes the reported segfault.
> Thanks!

I will sweep this through into 4.0/4.1 as part of a general backport sweep
next week.

 -- Keir

> Olaf

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

[Jan Beulich]

>>> On 02.12.11 at 07:02, Keir Fraser <keir.xen@gmail.com> wrote:
> On 02/12/2011 08:54, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 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?

Looks good.

Acked-by: Jan Beulich <jbeulich@novell.com>

While we only got problems with this on 4.1.2, I would suggest to also
put this back into 4.0-testing.

Thanks, Jan


E-mail confidentiality notice.  This message is intended for the addressees only.  It may be private, confidential and may be covered by legal professional privilege or other confidentiality requirements.  If you are not one of the intended recipients, please notify the sender immediately on +44 0 20-8215-3000 and delete the message from all locations in your computer network.  Do not copy this email or use it for any purpose or disclose its contents to any person:to do so maybe unlawful.

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

[Olaf Hering]

On Fri, Dec 02, Keir Fraser wrote:

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

+        : "0" (input[0]), "1" (count), "S" (_regs)

_regs is undeclared in 32bit, I think it should be regs.

Olaf

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

[Olaf Hering]

On Fri, Dec 02, Keir Fraser wrote:

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

So finally I was able to test this change, and it fixes the reported segfault.
Thanks!

Olaf