All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Nathan Chancellor <nathan@kernel.org>
Cc: will@kernel.org, linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	clang-built-linux@googlegroups.com
Subject: Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
Date: Mon, 21 Jun 2021 09:08:30 +0100	[thread overview]
Message-ID: <20210621080830.GA37068@C02TD0UTHF1T.local> (raw)
In-Reply-To: <YMzPi0Ckyd9wqO5d@archlinux-ax161>

On Fri, Jun 18, 2021 at 09:53:31AM -0700, Nathan Chancellor wrote:
> On Fri, Jun 18, 2021 at 04:18:35PM +0100, Mark Rutland wrote:
> > On Thu, Jun 17, 2021 at 06:25:27PM -0700, Nathan Chancellor wrote:
> > > Hi Mark,
> > 
> > Hi Nathan,
> > 
> > > On Wed, Jun 09, 2021 at 11:23:01AM +0100, Mark Rutland wrote:
> > > > For histroical reasons, we define AARCH64_INSN_SIZE in
> > > > <asm/alternative-macros.h>, but it would make more sense to do so in
> > > > <asm/insn.h>. Let's move it into <asm/insn.h>, and add the necessary
> > > > include directives for this.
> > 
> > > I bisected a CONFIG_LTO_CLANG_THIN=y build failure that our CI reported
> > > to this patch:
> > > 
> > > https://builds.tuxbuild.com/1u4Fpx2FQkkgkyPxWtq0Ke4YFCQ/build.log
> > 
> > Thanks for reporting this; the lopg is really helpful!
> > 
> > > I have not had a whole ton of time to look into this (dealing with a
> > > million fires it seems :^) but it is not immediately obvious to me why
> > > this fails because include/linux/build_bug.h is included within
> > > arch/arm64/include/asm/insn.h.
> > 
> > The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
> > includes <asm/insn.h>, creating a circular include chain:
> > 
> > 	<linux/build_bug.h>
> > 	<linux/compiler.h>
> > 	<asm/rwonce.h>
> > 	<asm/alternative-macros.h>
> > 	<asm/insn.h>
> > 	<linux/build-bug.h>
> > 
> > ... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
> > BUILD_BUG* definitions have happened yet.
> 
> Aha, that would certainly explain it. I figured something like this
> would be the root cause but figuring out header dependencies is not my
> cup of tea.
> 
> > Will, are you happy to take the fixup patch below, or would you prefer
> > to drop this patch for now?

> >  arch/arm64/include/asm/alternative-macros.h | 2 +-
> >  arch/arm64/include/asm/insn.h               | 5 +----
> 
> Looks like arch/arm64/include/asm/insn-def.h is missing from this patch?
> 
> If I add one with just the two deleted lines plus a header guard, the
> build passes.
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org>

Whoops; that should have been as below.

Was that the same as you tested?

Thanks,
Mark.

---->8----
From 622fd784c57423b1a276fbbfb270b84839e3afa8 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Fri, 18 Jun 2021 16:11:22 +0100
Subject: [PATCH] arm64: insn: avoid circular include dependency

Nathan reports that when building with CONFIG_LTO_CLANG_THIN=y, the
build fails due to BUILD_BUG_ON() not being defined before its uss in
<asm/insn.h>.

The problem is that with LTO, we patch READ_ONCE(), and <asm/rwonce.h>
includes <asm/insn.h>, creating a circular include chain:

        <linux/build_bug.h>
        <linux/compiler.h>
        <asm/rwonce.h>
        <asm/alternative-macros.h>
        <asm/insn.h>
        <linux/build-bug.h>

... and so when <asm/insn.h> includes <linux/build_bug.h>, none of the
BUILD_BUG* definitions have happened yet.

To avoid this, let's move AARCH64_INSN_SIZE into a header without any
dependencies, such that it can always be safely included. At the same
time, avoid including <asm/alternative.h> in <asm/insn.h>, which should
no longer be necessary (and doesn't make sense when insn.h is consumed
by userspace).

Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/alternative-macros.h | 2 +-
 arch/arm64/include/asm/insn-def.h           | 9 +++++++++
 arch/arm64/include/asm/insn.h               | 5 +----
 3 files changed, 11 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm64/include/asm/insn-def.h

diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
index 703fbf310b79..eba3173a2a2c 100644
--- a/arch/arm64/include/asm/alternative-macros.h
+++ b/arch/arm64/include/asm/alternative-macros.h
@@ -3,7 +3,7 @@
 #define __ASM_ALTERNATIVE_MACROS_H
 
 #include <asm/cpucaps.h>
-#include <asm/insn.h>
+#include <asm/insn-def.h>
 
 #define ARM64_CB_PATCH ARM64_NCAPS
 
diff --git a/arch/arm64/include/asm/insn-def.h b/arch/arm64/include/asm/insn-def.h
new file mode 100644
index 000000000000..2c075f615c6a
--- /dev/null
+++ b/arch/arm64/include/asm/insn-def.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_INSN_DEF_H
+#define __ASM_INSN_DEF_H
+
+/* A64 instructions are always 32 bits. */
+#define	AARCH64_INSN_SIZE		4
+
+#endif /* __ASM_INSN_DEF_H */
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 1430b4973039..6b776c8667b2 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -10,10 +10,7 @@
 #include <linux/build_bug.h>
 #include <linux/types.h>
 
-#include <asm/alternative.h>
-
-/* A64 instructions are always 32 bits. */
-#define	AARCH64_INSN_SIZE		4
+#include <asm/insn-def.h>
 
 #ifndef __ASSEMBLY__
 /*
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-06-21  8:10 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-09 10:22 [PATCH 0/2] arm64: insn: cleanups Mark Rutland
2021-06-09 10:23 ` [PATCH 1/2] arm64: insn: decouple patching from insn code Mark Rutland
2021-06-09 10:23 ` [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h> Mark Rutland
2021-06-18  1:25   ` Nathan Chancellor
2021-06-18 15:18     ` Mark Rutland
2021-06-18 16:53       ` Nathan Chancellor
2021-06-21  8:08         ` Mark Rutland [this message]
2021-06-21 10:59           ` Will Deacon
2021-06-21 16:12           ` Nathan Chancellor
2021-06-11 16:15 ` [PATCH 0/2] arm64: insn: cleanups Will Deacon

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=20210621080830.GA37068@C02TD0UTHF1T.local \
    --to=mark.rutland@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=nathan@kernel.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.