Messages in this thread Patch in this message |  | | Date | Wed, 17 Apr 2019 10:50:08 +0200 | From | Borislav Petkov <> | Subject | [PATCH] asm/io: Correct output operand specification of the MMIO write* routines |
| |
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 <bp@suse.de>
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 <bp@suse.de> Cc: Michael Matz <matz@suse.de> --- 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.
|  |