From: Mark Rutland <mark.rutland@arm.com>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, gcc@gcc.gnu.org,
catalin.marinas@arm.com, will@kernel.org, marcan@marcan.st,
maz@kernel.org, linux-kernel@vger.kernel.org,
szabolcs.nagy@arm.com, f.fainelli@gmail.com, opendmb@gmail.com
Subject: Re: [PATCH] arm64/io: Remind compiler that there is a memory side effect
Date: Fri, 1 Apr 2022 18:22:14 +0100 [thread overview]
Message-ID: <Ykc0xrLv391/jdJj@FVFF77S0Q05N> (raw)
In-Reply-To: <20220401164406.61583-1-jeremy.linton@arm.com>
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.
> For an example function look at bcmgenet_enable_dma(), before the
> relaxed variants were removed. When built with gcc12 the code
> contains the asm blocks as expected, but then the function is
> never called.
So it sounds like this is a regression in GCC 12, which IIUC isn't released yet
per:
https://gcc.gnu.org/gcc-12/changes.html
... which says:
| Note: GCC 12 has not been released yet
Surely we can fix it prior to release?
Thanks,
Mark.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> arch/arm64/include/asm/io.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 7fd836bea7eb..3cceda7948a0 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -24,25 +24,25 @@
> #define __raw_writeb __raw_writeb
> static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
> {
> - asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr) : "memory");
> }
>
> #define __raw_writew __raw_writew
> static inline void __raw_writew(u16 val, volatile void __iomem *addr)
> {
> - asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr) : "memory");
> }
>
> #define __raw_writel __raw_writel
> static __always_inline void __raw_writel(u32 val, volatile void __iomem *addr)
> {
> - asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("str %w0, [%1]" : : "rZ" (val), "r" (addr) : "memory");
> }
>
> #define __raw_writeq __raw_writeq
> static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> {
> - asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr) : "memory");
> }
>
> #define __raw_readb __raw_readb
> --
> 2.35.1
>
next prev parent reply other threads:[~2022-04-01 17:22 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-01 16:44 Jeremy Linton
2022-04-01 17:22 ` Mark Rutland [this message]
2022-04-03 7:36 ` Andrew Pinski
2022-04-03 7:47 ` Ard Biesheuvel
2022-04-03 7:47 ` Ard Biesheuvel
2022-04-04 9:14 ` Will Deacon
2022-04-03 17:40 ` Doug Berger
2022-04-05 12:51 ` GCC 12 miscompilation of volatile asm (was: Re: [PATCH] arm64/io: Remind compiler that there is a memory side effect) Mark Rutland
2022-04-05 13:04 ` Mark Rutland
2022-04-05 13:20 ` Andrew Cooper
2022-04-05 14:05 ` Peter Zijlstra
2022-04-11 10:22 ` Mark Rutland
2022-04-11 10:31 ` Mark Rutland
2022-04-11 19:02 ` Jeremy Linton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ykc0xrLv391/jdJj@FVFF77S0Q05N \
--to=mark.rutland@arm.com \
--cc=catalin.marinas@arm.com \
--cc=f.fainelli@gmail.com \
--cc=gcc@gcc.gnu.org \
--cc=jeremy.linton@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcan@marcan.st \
--cc=maz@kernel.org \
--cc=opendmb@gmail.com \
--cc=szabolcs.nagy@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).