Re: [PATCH] powerpc: Ensure gcc doesn't move around cache flushing in __patch_instruction

[Christophe Leroy <christophe.leroy@c-s.fr>]

Le 18/05/2018 à 01:00, Segher Boessenkool a écrit :
> On Fri, May 18, 2018 at 08:30:27AM +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote:
>>> On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:
>>>> The current asm statement in __patch_instruction() for the cache flushes
>>>> doesn't have a "volatile" statement and no memory clobber. That means
>>>> gcc can potentially move it around (or move the store done by put_user
>>>> past the flush).
>>>
>>> volatile is completely superfluous here, except maybe as documentation:
>>> any asm without outputs is always volatile.
>>
>> I wasn't aware of that. I was drilled early on to always stick volatile
>> in my asm statements if they have any form of side effect :-)
> 
> If an asm without output was not marked automatically as having another
> side effect, every such asm would be immediately deleted ;-)
> 
> Adding volatile as documentation for side effects can be good; it just
> doesn't do much (nothing, in fact) for asms without output as far as
> the compiler is concerned.
> 
>>> (And the memory clobber does not prevent the compiler from moving the
>>> asm around, or duplicating it, etc., and neither does the volatile).
>>
>> It prevents load/stores from moving around doesn't it ? I wanted to
>> make sure the store of the instruction doesn't move in/pass the asm. If
>> you say that's not needed then ignore the patch.
> 
> No, it's fine here, and you want either that or put exactly the memory
> you are touching in a constraint (probably overkill here).  I just
> wanted to say that a "memory" clobber does nothing more than say the
> asm touches some unspecified memory; there is no magic other meaning
> to it.  Your patch is correct, just the "volatile" part isn't needed,
> and the explanation was a bit cargo-culty ;-)
> 

Any plan to get that merged ?

Christophe