On Sun, 3 Apr 2022 at 09:38, Andrew Pinski <[email protected]> wrote:
>
> On Fri, Apr 1, 2022 at 10:24 AM Mark Rutland via Gcc <[email protected]> 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)
On Sun, 3 Apr 2022 at 09:47, Ard Biesheuvel <[email protected]> wrote:
>
> On Sun, 3 Apr 2022 at 09:38, Andrew Pinski <[email protected]> wrote:
> >
> > On Fri, Apr 1, 2022 at 10:24 AM Mark Rutland via Gcc <[email protected]> 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"
On Sun, Apr 03, 2022 at 09:47:47AM +0200, Ard Biesheuvel wrote:
> On Sun, 3 Apr 2022 at 09:47, Ard Biesheuvel <[email protected]> wrote:
> > On Sun, 3 Apr 2022 at 09:38, Andrew Pinski <[email protected]> wrote:
> > > 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"
That can generate writeback addressing modes, which I think breaks
MMIO virtualisation. We usually end up using "Q" instead but the codegen
tends to be worse iirc.
In any case, for this specific problem I think we either need a fixed
compiler or some kbuild magic to avoid using it / disable the new behaviour.
We rely on 'asm volatile' not being elided in other places too.
Will