From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) by sourceware.org (Postfix) with ESMTPS id C216F3858D37 for ; Sun, 3 Apr 2022 07:37:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C216F3858D37 Received: by mail-ej1-x62f.google.com with SMTP id o10so14034000ejd.1 for ; Sun, 03 Apr 2022 00:37:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=IGvz+saGqGJ7n4uIJ25zKRG9x6+wAmAWhzfjK7OHNP8=; b=KzcIvH3M18ur75+MqSJ6w7YMZ2IQHWKY61xLYDV7kbhDjISxWSId+jWi1AM/GUWEEn TJ8/RiGIlALqM2nZJ+TJwsGiKKcdUzWBuckcRwGqe9pMjic1XC7ucwSCG0u8L5sYINv0 Lg9pEteowBSQoKqqla4gQUz9c4+gLPhaomBVFlhb1h6HGXmjryfcJZzNfaCf1zSHE+E/ 7pCF9n0R3mDhWmjXnr5vQ6gmxoAOOi2q0EyKWwYNX3ilpBW9xJ23Kpe5TGxTK62H7a5c HObi9gCTibiKpDhFm+7MiAyOLvQ7Mk0eifqUuyv2WqvD5pRV04mxWXhUePlMdAqOy7zo Ny4g== X-Gm-Message-State: AOAM53166Zic/F2rJN/ALm11NU3DgFKhI+BuOelddZnps295vKHp0gXD WwN9WdqejIADtbNFByso7fv9B3sF3YwRDgUvvAg= X-Google-Smtp-Source: ABdhPJybhkbRVQxK8WVuOfZRCgeD+Vb4Z0HllGMHwZI1hv0bKzlEbnFYsHiCO3xxy/9xs9Yuu4WMyoOkWamj/7QWdeU= X-Received: by 2002:a17:906:7314:b0:6df:839f:af7 with SMTP id di20-20020a170906731400b006df839f0af7mr6271448ejc.65.1648971426309; Sun, 03 Apr 2022 00:37:06 -0700 (PDT) MIME-Version: 1.0 References: <20220401164406.61583-1-jeremy.linton@arm.com> In-Reply-To: From: Andrew Pinski Date: Sun, 3 Apr 2022 00:36:53 -0700 Message-ID: Subject: Re: [PATCH] arm64/io: Remind compiler that there is a memory side effect To: Mark Rutland Cc: Jeremy Linton , GCC Mailing List , f.fainelli@gmail.com, maz@kernel.org, marcan@marcan.st, LKML , opendmb@gmail.com, Catalin Marinas , will@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Apr 2022 07:37:09 -0000 On Fri, Apr 1, 2022 at 10:24 AM Mark Rutland via Gcc 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 > > > 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: It is NOT a bug in GCC 12. Just you depended on behavior which accidently worked in the cases you were looking at. GCC 12 did not change in this area at all even. Thanks, Andrew Pinski > > 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 > > --- > > 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 > >