Hi Linus,
I'm looking at
c1f64a58003f ("x86: MMIO and gcc re-ordering issue")
and trying to figure out was there any particular reason the address to
the MMIO write routines had to be an input operand?
Because if not, please have a look at the patch below. It boots here and
from the couple of resulting asm I looked at, it doesn't change. But
there might be some other aspect I'm missing so...
Thx.
---
From: Borislav Petkov <[email protected]>
Currently, all the MMIO write operations specify the output @addr
operand as an input operand to the extended asm statement. This works
because the asm statement is marked volatile and "memory" is on the
clobbered list, preventing gcc from optimizing around an inline asm
without output operands.
However, the more correct way of writing this is to make the target MMIO
write address an input *and* output operand so that gcc is aware that
modifications have happened through it.
Now, one could speculate further and say, the memory clobber could be
dropped:
*P; // (1)
mmio_write("..." : "+m" (whatever)); // no memory clobber
*P; // (2)
but then gcc would at -O2 optimize the access in (2) by replacing it
with its value from (1), which would be wrong.
The solution would be sticking a memory barrier after (1) but then that
puts the onus on the programmer to make sure it doesn't get forgotten,
and that is the wrong approach: generic interfaces like that should
JustWork(tm) so let's leave the "memory" clobber.
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Michael Matz <[email protected]>
---
arch/x86/include/asm/io.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 686247db3106..33c4d8776b47 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -49,10 +49,11 @@ static inline type name(const volatile void __iomem *addr) \
{ type ret; asm volatile("mov" size " %1,%0":reg (ret) \
:"m" (*(volatile type __force *)addr) barrier); return ret; }
-#define build_mmio_write(name, size, type, reg, barrier) \
-static inline void name(type val, volatile void __iomem *addr) \
-{ asm volatile("mov" size " %0,%1": :reg (val), \
-"m" (*(volatile type __force *)addr) barrier); }
+#define build_mmio_write(name, size, type, reg, barrier) \
+static inline void name(type val, volatile void __iomem *mem) \
+{ asm volatile("mov" size " %1,%0" \
+ : "+m" (*(volatile type __force *)mem) \
+ : reg (val) barrier); }
build_mmio_read(readb, "b", unsigned char, "=q", :"memory")
build_mmio_read(readw, "w", unsigned short, "=r", :"memory")
--
2.21.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Wed, Apr 17, 2019 at 1:50 AM Borislav Petkov <[email protected]> wrote:
>
> I'm looking at
>
> c1f64a58003f ("x86: MMIO and gcc re-ordering issue")
>
> and trying to figure out was there any particular reason the address to
> the MMIO write routines had to be an input operand?
It doesn't have to be an input operand, but as long as it's a "asm
volatile" it simply doesn't matter, and it won't be re-ordered or
optimized wrt other mmio accesses (that are also "asm volatile").
The memory clobber we have is to make sure that it's not re-ordered
with non-mmio accesses to other addresses (and thats' true for reads
_or_ writes, so both mmio read and mmio write have the memory
clobber).
So changing the input "m" to an output "+m" simply shouldn't matter.
There's no upside. You can't remove the memory clobber anyway, and you
can't remove the "asm volatile".
The "__" versions lack the memory clobber and aren't ordered wrt
normal memory (but are ordered wrt other mmio due to the "asm
volaile").
So I see no upside to changing it.
Linus
Hi,
On Wed, 17 Apr 2019, Linus Torvalds wrote:
> So I see no upside to changing it.
As long as the memory clobber stays (and it can't be removed in the
general case, as you say) the change has no practical effect.
<outsider>
It does have a psychological upside, though: people won't continue to
wonder why the asm doesn't precisely specify what it does, and instead
only declares a memory-read while clearly being a memory write.
Normally, with asms, leaving nothing to wonder about is a good thing.
</outsider>
Ciao,
Michael.