On 18/03/2019 13:20, Jan Beulich wrote:
> >>> On 18.03.19 at 12:27, <andrew.coop...@citrix.com> 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 change from =q to =r is specifically to allow the folding to +r
>
>> 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.
I don't think there is any flaw or shortcoming. Without the volatile,
the compiler doesn't know that there are any side effects, so can
legitimately operate on a local stack copy so long as it copies things
back later.
In practice, this is an operation on shared memory which has to happen
on the shared memory pointer.
> In any event I think it would be a good idea to have a code
> comment for this as well.
I don't see how that would help. The same applies to all atomic
operations, even test_bit().
>
>> @@ -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)?
Yes - it is strictly necessary, because otherwise it gets derived from
the type of (x) which is unsigned long even in the smaller size constructs.
>
>> @@ -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.
> 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.
I'm not sure that is a sensible move. Its a macro-scope variable from
cmpxchg_user() which still needs disambiguating from potential names of
parameters.
>
> 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 <jbeul...@suse.com>
Hopefully my reply is sufficient?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
