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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 15:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/03/2019 14:11, Andrew Cooper wrote:
>>
>>>> @@ -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)?
>> I can do.
> 
> I've got the following incremental fix which I intend to fold in.

LGTM.

Jan

> diff --git a/xen/include/asm-x86/x86_64/system.h 
> b/xen/include/asm-x86/x86_64/system.h
> index 5b6e964..d65562b 100644
> --- a/xen/include/asm-x86/x86_64/system.h
> +++ b/xen/include/asm-x86/x86_64/system.h
> @@ -65,7 +65,7 @@ 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, _oppre)                              \
> +#define __cmpxchg_user(_p, _o, _n, _rc, _oppre)                         \
>      stac();                                                             \
>      asm volatile (                                                      \
>          "1: lock cmpxchg %"_oppre"[new], %[ptr]\n"                      \
> @@ -75,28 +75,29 @@ static always_inline __uint128_t cmpxchg16b_local_(
>          "       jmp 2b\n"                                               \
>          ".previous\n"                                                   \
>          _ASM_EXTABLE(1b, 3b)                                            \
> -        : "+a" (_o), [rc] "=r" (_rc),                                   \
> +        : "+a" (_o), [rc] "+r" (_rc),                                   \
>            [ptr] "+m" (*(volatile typeof(*(_p)) *)(_p))                  \
> -        : [new] "r" (_n), "[rc]" (0)                                    \
> +        : [new] "r" (_n)                                                \
>          : "memory");                                                    \
>      clac()
>  
>  #define cmpxchg_user(_p, _o, _n)                                        \
>  ({                                                                      \
> -    int _rc;                                                            \
> +    int _rc = 0;                                                        \
> +                                                                        \
>      switch ( sizeof(*(_p)) )                                            \
>      {                                                                   \
>      case 1:                                                             \
> -        __cmpxchg_user(_p, _o, _n, "b");                                \
> +        __cmpxchg_user(_p, _o, _n, _rc, "b");                           \
>          break;                                                          \
>      case 2:                                                             \
> -        __cmpxchg_user(_p, _o, _n, "w");                                \
> +        __cmpxchg_user(_p, _o, _n, _rc, "w");                           \
>          break;                                                          \
>      case 4:                                                             \
> -        __cmpxchg_user(_p, _o, _n, "k");                                \
> +        __cmpxchg_user(_p, _o, _n, _rc, "k");                           \
>          break;                                                          \
>      case 8:                                                             \
> -        __cmpxchg_user(_p, _o, _n, "q");                                \
> +        __cmpxchg_user(_p, _o, _n, _rc, "q");                           \
>          break;                                                          \
>      }                                                                   \
>      _rc;                                                                \




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