[request for review] cmpxchg8b asm stuff

[request for review] cmpxchg8b asm stuff

[Gerd Knorr]

  Hi,

Below is my very first attempt to write inline assembler, for atomic
updates of PAE pagetable entries using cmpxchg8b.  It builds, but is
completely untested otherwise as I don't have a PAE-enabled guest kernel
yet ...

Can someone with more experience than me please have a look at it?

Thanks,

  Gerd

--- xen.orig/arch/x86/mm.c	2005-04-19 11:56:20.000000000 +0200
+++ xen/arch/x86/mm.c	2005-04-19 15:29:01.000000000 +0200
@@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf
 
 #endif /* __x86_64__ */
 
+static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval)
+{
+    u32 o_hi = oval >> 32;
+    u32 o_lo = oval & 0xffffffff;
+    u32 n_hi = nval >> 32;
+    u32 n_lo = nval & 0xffffffff;
+    u32 rc = 0;
+
+    __asm__ __volatile__ (
+        "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"
+        "2:\n"
+        ".section .fixup,\"ax\"\n"
+        "3:     movl $1, %[rc]\n"
+        "       jmp 2b\n"
+        ".previous\n"
+        ".section __ex_table,\"a\"\n"
+        "       .align 4\n"
+        "       .long 1b,3b\n"
+        ".previous"
+        : "=d" (o_hi), "=a" (o_lo), "=c" (n_hi), "=b" (n_lo),
+          [rc] "=r" (rc), [ptr] "m" (*__xg((volatile void *)ptr))
+        : "0" (o_hi), "1" (o_lo)
+        : "memory");
+    if ((o_hi != (oval >> 32)) ||
+        (o_lo != (oval & 0xffffffff)))
+        rc = 1;
+    return rc;
+}
 
 static inline int update_l1e(l1_pgentry_t *pl1e, 
                              l1_pgentry_t  ol1e, 
                              l1_pgentry_t  nl1e)
 {
-    /* FIXME: breaks with PAE */
+#if defined(__i386__) && defined(CONFIG_X86_PAE)
+
+    int rc;
+
+    rc = cmpxchg8b_user(pl1e, l1e_get_value(ol1e),
+                        l1e_get_value(nl1e));
+    if (unlikely(rc))
+    {
+        MEM_LOG("Failed to update %llx -> %llx [pae]\n",
+                l1e_get_value(ol1e), l1e_get_value(nl1e));
+        return 0;
+    }
+    return 1;
+
+#else
+
     unsigned long o = l1e_get_value(ol1e);
     unsigned long n = l1e_get_value(nl1e);
 
@@ -850,8 +905,9 @@ static inline int update_l1e(l1_pgentry_
                 l1e_get_value(ol1e), l1e_get_value(nl1e), o);
         return 0;
     }
-
     return 1;
+
+#endif
 }

Re: [request for review] cmpxchg8b asm stuff

[Hollis Blanchard]

On Tue, 2005-04-19 at 15:50 +0200, Gerd Knorr wrote:

> Below is my very first attempt to write inline assembler, for atomic
> updates of PAE pagetable entries using cmpxchg8b.  It builds, but is
> completely untested otherwise as I don't have a PAE-enabled guest kernel
> yet ...
> 
> Can someone with more experience than me please have a look at it?
> 
> Thanks,
> 
>   Gerd
> 
> --- xen.orig/arch/x86/mm.c	2005-04-19 11:56:20.000000000 +0200
> +++ xen/arch/x86/mm.c	2005-04-19 15:29:01.000000000 +0200
> @@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf
>  
>  #endif /* __x86_64__ */
>  
> +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval)
> +{
...
> +}

Shouldn't this go into include/asm-x86/system.h? You can add another
case to the __i386__ cmpxchg_user switch.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [request for review] cmpxchg8b asm stuff

[Gerd Knorr]

> > +++ xen/arch/x86/mm.c	2005-04-19 15:29:01.000000000 +0200
> >  
> > +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval)

> Shouldn't this go into include/asm-x86/system.h?

Well, maybe later.  I want to have it working correctly first, then
optimize and then check how to integrate that nicely.

> You can add another case to the __i386__ cmpxchg_user switch.

That would be the most obvious place, yes.  Not fully sure yet that this
is really a good idea though as it is sort-of special case for 64bit
data on a 32bit machine, you have to split the 64bit values into two
32bit regs and so on (unlike the 64bit version on x86_64 which simply
uses the 64bit registers).  Maybe it's better to keep that separate as
it might be easier to optimize it then, not evaluated yet.

  Gerd

-- 
#define printk(args...) fprintf(stderr, ## args)

Re: [request for review] cmpxchg8b asm stuff

[Keir Fraser]

>   Hi,
> 
> Below is my very first attempt to write inline assembler, for atomic
> updates of PAE pagetable entries using cmpxchg8b.  It builds, but is
> completely untested otherwise as I don't have a PAE-enabled guest kernel
> yet ...
> 
> Can someone with more experience than me please have a look at it?

Thanks - I've checked in an 8-byte extension to cmpxchg_user() for
32-bit builds. Differences from yours include:

 1. The "A" register specifier is used to match a 64-bit C variable
    with EDX:EAX. This is neater than manually splitting into "a"
    and "d" constraints, then manually recombining at the end.
    Unfortunately there is no such shortcut for ECX:EBX.

 2. n_hi, n_lo and ptr are not output constraints, so they belong in
    the second constraint list, with no "=" in the specifier string.

 3. I don't like the non-positional argument specifiers (e.g., [rc],
    [ptr]) very much. Maybe worthwhile for longer code fragments but
    for very short ones I'd rather have less clutter in the constraint
    lists. 

 4. No need to return non-zero if the cmpxchg fails. The return code
    indicates only whether the cmpxchg access faulted or not: it is up
    to the caller to compare the value seen with what they expected.
    This gets rid of a few lines in your cmpxchg8b_user.

I think that covers it. :-)

 -- Keir

> 
> Thanks,
> 
>   Gerd
> 
> --- xen.orig/arch/x86/mm.c	2005-04-19 11:56:20.000000000 +0200
> +++ xen/arch/x86/mm.c	2005-04-19 15:29:01.000000000 +0200
> @@ -834,12 +846,55 @@ static void free_l4_table(struct pfn_inf
>  
>  #endif /* __x86_64__ */
>  
> +static inline int cmpxchg8b_user(u64 *ptr, u64 oval, u64 nval)
> +{
> +    u32 o_hi = oval >> 32;
> +    u32 o_lo = oval & 0xffffffff;
> +    u32 n_hi = nval >> 32;
> +    u32 n_lo = nval & 0xffffffff;
> +    u32 rc = 0;
> +
> +    __asm__ __volatile__ (
> +        "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"
> +        "2:\n"
> +        ".section .fixup,\"ax\"\n"
> +        "3:     movl $1, %[rc]\n"
> +        "       jmp 2b\n"
> +        ".previous\n"
> +        ".section __ex_table,\"a\"\n"
> +        "       .align 4\n"
> +        "       .long 1b,3b\n"
> +        ".previous"
> +        : "=d" (o_hi), "=a" (o_lo), "=c" (n_hi), "=b" (n_lo),
> +          [rc] "=r" (rc), [ptr] "m" (*__xg((volatile void *)ptr))
> +        : "0" (o_hi), "1" (o_lo)
> +        : "memory");
> +    if ((o_hi != (oval >> 32)) ||
> +        (o_lo != (oval & 0xffffffff)))
> +        rc = 1;
> +    return rc;
> +}
>  
>  static inline int update_l1e(l1_pgentry_t *pl1e, 
>                               l1_pgentry_t  ol1e, 
>                               l1_pgentry_t  nl1e)
>  {
> -    /* FIXME: breaks with PAE */
> +#if defined(__i386__) && defined(CONFIG_X86_PAE)
> +
> +    int rc;
> +
> +    rc = cmpxchg8b_user(pl1e, l1e_get_value(ol1e),
> +                        l1e_get_value(nl1e));
> +    if (unlikely(rc))
> +    {
> +        MEM_LOG("Failed to update %llx -> %llx [pae]\n",
> +                l1e_get_value(ol1e), l1e_get_value(nl1e));
> +        return 0;
> +    }
> +    return 1;
> +
> +#else
> +
>      unsigned long o = l1e_get_value(ol1e);
>      unsigned long n = l1e_get_value(nl1e);
>  
> @@ -850,8 +905,9 @@ static inline int update_l1e(l1_pgentry_
>                  l1e_get_value(ol1e), l1e_get_value(nl1e), o);
>          return 0;
>      }
> -
>      return 1;
> +
> +#endif
>  }
>  
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>