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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/2018 13:19, Jan Beulich wrote:
>>>> 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.

Ok fine.  I'll reword in light of this behaviour.

>
>>>> @@ -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).

Hmm - I'd forgotten that syntax, but I'm still not going to do it like that.

Using "=a" (prev) : "a" (old) is fine, and slightly better than what we
have currently.  It also closely matches how the instruction is
described in the manual, whereas using "[prev]" (old) is longer, and
adds a level of indirection which makes the code harder to follow.

>
>>>>          "       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?

Well - that's a good question.  We definitely hit a ud2 instruction, but
get a bogus details printed.

I'm fairly sure I've ruled out a partially stale build, but I'm
beginning to suspect that release builds without debugging symbols
(which I'm using to reduce the binary diff between builds) causes Xen's
symbol table to generated incorrectly.

~Andrew

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