Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
From: Nathan Chancellor <nathan@kernel.org>
Date: 2021-06-21 16:14:29
On 6/21/2021 1:08 AM, Mark Rutland wrote:
On Fri, Jun 18, 2021 at 09:53:31AM -0700, Nathan Chancellor wrote:quoted
On Fri, Jun 18, 2021 at 04:18:35PM +0100, Mark Rutland wrote:quoted
On Thu, Jun 17, 2021 at 06:25:27PM -0700, Nathan Chancellor wrote:quoted
Hi Mark,Hi Nathan,quoted
On Wed, Jun 09, 2021 at 11:23:01AM +0100, Mark Rutland wrote:quoted
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.quoted
I bisected a CONFIG_LTO_CLANG_THIN=y build failure that our CI reported to this patch: https://builds.tuxbuild.com/1u4Fpx2FQkkgkyPxWtq0Ke4YFCQ/build.logThanks for reporting this; the lopg is really helpful!quoted
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.quoted
Will, are you happy to take the fixup patch below, or would you prefer to drop this patch for now?quoted
quoted
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?
Yes, that is exactly what I did so the tag should still be good. Cheers, Nathan
quoted hunk ↗ jump to hunk
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.hdiff --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_NCAPSdiff --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__ /*
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel