On Sun, 3 Apr 2022 at 09:47, Ard Biesheuvel <a...@kernel.org> wrote: > > On Sun, 3 Apr 2022 at 09:38, Andrew Pinski <pins...@gmail.com> wrote: > > > > On Fri, Apr 1, 2022 at 10:24 AM Mark Rutland via Gcc <gcc@gcc.gnu.org> > > wrote: > > > > > > Hi Jeremy, > > > > > > Thanks for raising this. > > > > > > On Fri, Apr 01, 2022 at 11:44:06AM -0500, Jeremy Linton wrote: > > > > The relaxed variants of read/write macros are only declared > > > > as `asm volatile()` which forces the compiler to generate the > > > > instruction in the code path as intended. The only problem > > > > is that it doesn't also tell the compiler that there may > > > > be memory side effects. Meaning that if a function is comprised > > > > entirely of relaxed io operations, the compiler may think that > > > > it only has register side effects and doesn't need to be called. > > > > > > As I mentioned on a private mail, I don't think that reasoning above is > > > correct, and I think this is a miscompilation (i.e. a compiler bug). > > > > > > The important thing is that any `asm volatile` may have a side effects > > > generally outside of memory or GPRs, and whether the assembly contains a > > > memory > > > load/store is immaterial. We should not need to add a memory clobber in > > > order > > > to retain the volatile semantic. > > > > > > See: > > > > > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile > > > > > > ... and consider the x86 example that reads rdtsc, or an arm64 sequence > > > like: > > > > > > | void do_sysreg_thing(void) > > > | { > > > | unsigned long tmp; > > > | > > > | tmp = read_sysreg(some_reg); > > > | tmp |= SOME_BIT; > > > | write_sysreg(some_reg); > > > | } > > > > > > ... where there's no memory that we should need to hazard against. > > > > > > This patch might workaround the issue, but I don't believe it is a > > > correct fix. > > > > It might not be the most restricted fix but it is a fix. > > The best fix is to tell that you are writing to that location of memory. > > volatile asm does not do what you think it does. > > You didn't read further down about memory clobbers: > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers > > Specifically this part: > > The "memory" clobber tells the compiler that the assembly code > > performs memory reads or writes to items other than those listed in > > the input and output operands > > > > So should we be using "m"(*addr) instead of "r"(addr) here? > > (along with the appropriately sized casts)
I mean "=m" not "m"
