Re: [Xen-devel] [v4 03/17] Add cmpxchg16b support for x86-64

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.15 at 13:35, <feng.wu@xxxxxxxxx> wrote:
> This patch adds cmpxchg16b support for x86-64, so software
> can perform 128-bit atomic write/read.
> 
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> ---
> v4:
> - Use pointer as the parameter of __cmpxchg16b().
> - Use gcc's __uint128_t built-in type
> - Make the parameters of __cmpxchg16b() void *

I haven't been able to spot who asked you when to do this.
Looking at the code ...

> --- a/xen/include/asm-x86/x86_64/system.h
> +++ b/xen/include/asm-x86/x86_64/system.h
> @@ -6,6 +6,38 @@
>                                     (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, void *old, void *new)
> +{
> +    uint64_t prev_high, prev_low;
> +    uint64_t new_high = (*(__uint128_t *)new) >> 64;
> +    uint64_t new_low = (uint64_t)(*(__uint128_t *)new);
> +    uint64_t old_high = (*(__uint128_t *)old) >> 64;
> +    uint64_t old_low = (uint64_t)(*(__uint128_t *)old);

... I certainly think this goes too far - way too many casts.

> +    ASSERT(cpu_has_cx16);
> +
> +    asm volatile ( "lock; cmpxchg16b %4"
> +                   : "=d" (prev_high), "=a" (prev_low)

Is there a reason not to use the A constraint here?

> +                   : "c" (new_high), "b" (new_low),
> +                   "m" (*__xg((volatile void *)ptr)),

Regardless of the memory clobber this should be an output. And
once an output (ideally properly typed - right now the cast is
pointless), I'm not sure the memory clobber has to stay.

> +                   "0" (old_high), "1" (old_low)
> +                   : "memory" );
> +
> +    return ( ((__uint128_t)prev_high) << 64 | prev_low );

Badly chose parentheses placement:

   return ((__uint128_t)prev_high << 64) | prev_low;

> +}
> +
> +#define cmpxchg16b(ptr,o,n)                                             \
> +    __cmpxchg16b((ptr), (__uint128_t *)(o), (__uint128_t *)(n))

This makes the use of void * pointers above even more
questionable.

But I think now I recall that it may have been me asking for using
void pointers here: That way one can pass pointers to other types.
But then you would better cast to void * here, and have the inline
function have properly typed pointers. And it should go without
saying that if you cast (or implicitly convert to void), you should
validate that what your caller handed you is at least the size of a
__uint128_t (and for "ptr" perhaps also, as I think Andrew already
pointed out, that it is suitably aligned).

Jan


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