Re: [Xen-devel] [v3 03/15] Add cmpxchg16b support for x86-64

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/06/15 06:18, Feng Wu wrote:
> This patch adds cmpxchg16b support for x86-64, so software
> can perform 128-bit atomic write/read.
>
> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> ---
> v3:
> Newly added.
>
>  xen/include/asm-x86/x86_64/system.h | 28 ++++++++++++++++++++++++++++
>  xen/include/xen/types.h             |  5 +++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/xen/include/asm-x86/x86_64/system.h 
> b/xen/include/asm-x86/x86_64/system.h
> index 662813a..a910d00 100644
> --- a/xen/include/asm-x86/x86_64/system.h
> +++ b/xen/include/asm-x86/x86_64/system.h
> @@ -6,6 +6,34 @@
>                                     (unsigned long)(n),sizeof(*(ptr))))
>  
>  /*
> + * Atomic 16 bytes compare and exchange.  Compare OLD with MEM, if
> + * identical, store NEW in MEM.  Return the initial value in MEM.
> + * Success is indicated by comparing RETURN with OLD.
> + *
> + * This function can only be called when cpu_has_cx16 is ture.
> + */
> +
> +static always_inline uint128_t __cmpxchg16b(
> +    volatile void *ptr, uint128_t old, uint128_t new)

It is not nice for register scheduling taking uint128_t's by value. 
Instead, I would pass them by pointer and let the inlining sort the
eventual references out.

> +{
> +    uint128_t prev;
> +
> +    ASSERT(cpu_has_cx16);

Given that if this assertion were to fail, cmpxchg16b would fail with
#UD, I would hand-code a asm_fixup section which in turn panics.  This
avoids a situation where non-debug builds could die with an unqualified
#UD exception.

Also, you must enforce 16-byte alignment of the memory reference, as
described in the manual.

~Andrew

> +
> +    asm volatile ( "lock; cmpxchg16b %4"
> +                   : "=d" (prev.high), "=a" (prev.low)
> +                   : "c" (new.high), "b" (new.low),
> +                   "m" (*__xg((volatile void *)ptr)),
> +                   "0" (old.high), "1" (old.low)
> +                   : "memory" );
> +
> +    return prev;
> +}
> +
> +#define cmpxchg16b(ptr,o,n)                                             \
> +    __cmpxchg16b((ptr), *(uint128_t *)(o), *(uint128_t *)(n))
> +
> +/*
>   * This function causes value _o to be changed to _n at location _p.
>   * If this access causes a fault then we return 1, otherwise we return 0.
>   * If no fault occurs then _o is updated to the value we saw at _p. If this
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 8596ded..30f8a44 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -47,6 +47,11 @@ typedef         __u64           uint64_t;
>  typedef         __u64           u_int64_t;
>  typedef         __s64           int64_t;
>  
> +typedef struct {
> +        uint64_t low;
> +        uint64_t high;
> +} uint128_t;
> +
>  struct domain;
>  struct vcpu;
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel