Re: [Xen-devel] [PATCH v2] x86/atomic: Improvements and simplifications to assembly constraints

["Jan Beulich" <JBeulich@xxxxxxxx>]

 >>> On 18.03.19 at 12:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> * Some of the single-byte versions specify "=q" as the output.  This is a
>    remnent of the 32bit build and can be relaxed to "=r" in 64bit builds.

I have to admit that I don't understand the "relaxed" part of this:
"q" and "r" represent the exact same set of registers on 64-bit.
Unless the conversion allows further code folding, I think it wouldn't
be a bad idea to retain the distinction, just for cases like code
eventually getting shared via something like lib/x86/.

> The reason the volatile cast in __cmpxchg_user() can't be dropped is because
> without it, the compiler uses a stack copy rather than the in-memory copy,
> which ends up tripping:
> 
>   /* Allowed to change in Accessed/Dirty flags only. */
>   BUG_ON((t ^ old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));

Isn't this hinting at some other shortcoming or even flaw then?
If the compiler generally did such transformations, I'm afraid a
lot of other code would be at risk too, including some of what
you modify here.

In any event I think it would be a good idea to have a code
comment for this as well.

> @@ -40,28 +37,24 @@ static always_inline unsigned long __xchg(
>      switch ( size )
>      {
>      case 1:
> -        asm volatile ( "xchgb %b0,%1"
> -                       : "=q" (x)
> -                       : "m" (*__xg(ptr)), "0" (x)
> -                       : "memory" );
> +        asm volatile ( "xchg %b[x], %[ptr]"
> +                       : [x] "+r" (x), [ptr] "+m" (*(uint8_t *)ptr)
> +                       :: "memory" );
>          break;
>      case 2:
> -        asm volatile ( "xchgw %w0,%1"
> -                       : "=r" (x)
> -                       : "m" (*__xg(ptr)), "0" (x)
> -                       : "memory" );
> +        asm volatile ( "xchg %w[x], %[ptr]"
> +                       : [x] "+r" (x), [ptr] "+m" (*(uint16_t *)ptr)
> +                       :: "memory" );
>          break;
>      case 4:
> -        asm volatile ( "xchgl %k0,%1"
> -                       : "=r" (x)
> -                       : "m" (*__xg(ptr)), "0" (x)
> -                       : "memory" );
> +        asm volatile ( "xchg %k[x], %[ptr]"
> +                       : [x] "+r" (x), [ptr] "+m" (*(uint32_t *)ptr)
> +                       :: "memory" );
>          break;
>      case 8:
> -        asm volatile ( "xchgq %0,%1"
> -                       : "=r" (x)
> -                       : "m" (*__xg(ptr)), "0" (x)
> -                       : "memory" );
> +        asm volatile ( "xchg %q[x], %[ptr]"
> +                       : [x] "+r" (x), [ptr] "+m" (*(uint64_t *)ptr)
> +                       :: "memory" );
>          break;

Is the q modifier really useful to have here (and elsewhere below)?

> @@ -63,36 +65,38 @@ static always_inline __uint128_t cmpxchg16b_local_(
>   * If no fault occurs then _o is updated to the value we saw at _p. If this
>   * is the same as the initial value of _o then _n is written to location _p.
>   */
> -#define __cmpxchg_user(_p,_o,_n,_isuff,_oppre,_regtype)                 \
> +#define __cmpxchg_user(_p, _o, _n, _oppre)                              \
>      stac();                                                             \
>      asm volatile (                                                      \
> -        "1: lock; cmpxchg"_isuff" %"_oppre"2,%3\n"                      \
> +        "1: lock cmpxchg %"_oppre"[new], %[ptr]\n"                      \
>          "2:\n"                                                          \
>          ".section .fixup,\"ax\"\n"                                      \
> -        "3:     movl $1,%1\n"                                           \
> +        "3:     movl $1, %[rc]\n"                                       \
>          "       jmp 2b\n"                                               \
>          ".previous\n"                                                   \
>          _ASM_EXTABLE(1b, 3b)                                            \
> -        : "=a" (_o), "=r" (_rc)                                         \
> -        : _regtype (_n), "m" (*__xg((volatile void *)_p)), "0" (_o), "1" (0) 
> \
> +        : "+a" (_o), [rc] "=r" (_rc),                                   \
> +          [ptr] "+m" (*(volatile typeof(*(_p)) *)(_p))                  \
> +        : [new] "r" (_n), "[rc]" (0)                                    \

Wouldn't it further help readability a little if _rc was initialized to zero
right when getting declared, eliminating the last input arg here (the
output then would need to be "+r" of course)? And since then you
actually touch all lines containing uses of _rc, it would be a good
opportunity to also rename the variable to get rid of the leading
underscore.

Anyway, with at least the "relaxed" part of the description changed
(e.g. to "converted") or explained verbally in a reply, with or without
the other items taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel