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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.18 at 13:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/11/2018 08:57, Jan Beulich wrote:
>>  >>> On 21.11.18 at 20:37, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> * Some of the single-byte versions specify "=q" as the output.  AFAICT, 
>>> there
>>>    was not a legitimate reason to restrict the use of %esi/%edi in the 
>>> 32-bit
>>>    build.  Either way, in 64-bit, it is equivelent to "=r".
>> I'm confused about the 32-bit part here: Of course it was necessary
>> to restrict the compiler to the low 4 registers in that case. It's just
>> not clear to me whether you've just written it down wrongly, or
>> whether you indeed think the way it reads to me.
> 
> Wait - are you saying that the combination of "=r" and %b0 would
> actually fail to build if the compiler happened to chose %edi/%esi?

Of course: It would produce %sil / %dil, which exist only in 64-bit
mode.

> Now that you point it out, I can see why %esi/%edi aren't actually
> encodable in this circumstance, but surely the fact that the compiler
> has to pick a byte register means that it wouldn't end up choosing these?

How does it know it needs to pick a byte register when your
constraint is "r"? That's what "q" is for.

>>> @@ -79,31 +72,27 @@ static always_inline unsigned long __cmpxchg(
>>>      switch ( size )
>>>      {
>>>      case 1:
>>> -        asm volatile ( "lock; cmpxchgb %b1,%2"
>>> -                       : "=a" (prev)
>>> -                       : "q" (new), "m" (*__xg(ptr)),
>>> -                       "0" (old)
>>> +        asm volatile ( "lock; cmpxchg %b[new], %[ptr]"
>>> +                       : "=a" (prev), [ptr] "+m" (*(uint8_t *)ptr)
>>> +                       : [new] "r" (new), "0" (old)
>> Any reason you retain the reference by number in the input
>> constraint here, rather than giving its corresponding output
>> one a name?
> 
> Not specifically.  I suppose this is doable because the constraint is an
> explicitly register.

I don't understand: What does register or no have to do with
it? Did you perhaps misunderstand? I'm asking for [prev] "a" (prev)
and then "[prev]" (old).

>>>          "       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), "=r" (_rc),                                        \
>>> +          [ptr] "+m" (*(volatile typeof(*(_p)) *)(_p))                  \
>> Does casting to add "volatile" here really make any difference,
>> considering the asm() itself is a volatile one and has a memory
>> clobber?
> 
> Yes.  mod_l1_entry() hits a BUG() without it.

Ouch.

> Until I understand why, I purposefully didn't change the volatility of
> any of these constructs in what is otherwise a cleanup patch.

I think this would be quite relevant to understand irrespective
of the exact shape this change is going to end up having. Which
exact BUG() is getting triggered?

Jan



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