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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2019 17:29, Andrew Cooper wrote:
>   * Constraints in the form "=r" (x) : "0" (x) can be folded to just "+r" (x)
>   * Switch to using named parameters (mostly for legibility) which in
>     particular helps with...
>   * __xchg(), __cmpxchg() and __cmpxchg_user() modify their memory operand, so
>     must list it as an output operand.  This only works because they each have
>     a memory clobber to give the construct full compiler-barrier properties.
>   * Every memory operand has an explicit known size.  Letting the compiler see
>     the real size rather than obscuring it with __xg() allows for the removal
>     of the instruction size suffixes without introducing ambiguity.
>   * Misc style changes.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
In principle I would give this my R-b, but ...

> 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));
... this is concerning me, seeing that elsewhere you eliminate
casts to pointers to volatile (and often you actively cast away
the attribute from the function parameters), which was one of
the effects of __xg(). Therefore only with "volatile" restored
everywhere where it was in effect before

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Alternatively justification for the removal would need to be
added to the description.

As to the description: The dropping of the semicolons after
the LOCK prefixes would perhaps be worth another bullet point?
It's not really just a "misc style change" imo.

Jan

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