Message ID | 48284701fe497bb4f5bede5c55bbce9d70309562.camel@kernel.crashing.org (mailing list archive) |
---|---|
State | Deferred |
Headers | show |
Series | powerpc: Ensure gcc doesn't move around cache flushing in __patch_instruction | expand |
Hi! 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. (And the memory clobber does not prevent the compiler from moving the asm around, or duplicating it, etc., and neither does the volatile). Segher
On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote: > Hi! > > 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 :-) > (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. Cheers, Ben. > > Segher
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 ;-) Segher
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
--- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -32,8 +32,9 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr, if (err) return err; - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr), - "r" (exec_addr)); + asm volatile("dcbst 0, %0; sync; icbi 0,%1; sync; isync" + :: "r" (patch_addr), "r" (exec_addr) + : "memory"); return 0; }
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). Add both to ensure gcc doesn't play games. Found by code inspection, no actual bug reported. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> ---