linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] arm64: insn: cleanups
@ 2021-06-09 10:22 Mark Rutland
  2021-06-09 10:23 ` [PATCH 1/2] arm64: insn: decouple patching from insn code Mark Rutland
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Mark Rutland @ 2021-06-09 10:22 UTC (permalink / raw)
  To: linux-arm-kernel, will; +Cc: Mark Rutland

Hi Will,

These couple of patches are fixups for the arm64 for-next/insn branch, which
try to bring that inline with the intent of the original patches.

The main change is to have users of patching code explicitly include
<asm/patching.h> (which was the intent of the original patches), as having
<asm/insn.h> include <asm/patching.h> can create some painful header
dependencies, and this more clearly separates the insn parts under arm64/lib/
from the parts under arm64/kernel/. The header dependencies don't appear to be
an issue today, but get in the way of some subsequent rework patches I'm
developing, and I'd like to fix that now before there's any additional reliance
on <asm/insn.h> pulling things in.

The other change is to move AARCH64_INSN_SIZE into <asm/insn.h>, to keep all
the insn bits in one place.

The big diff for kprobes.c is due to sorting the includes, which are unchanged
other than the <asm/patching.h> inclusion.

Thanks.
Mark.

Mark Rutland (2):
  arm64: insn: decouple patching from insn code
  arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>

 arch/arm64/include/asm/alternative-macros.h |  4 +---
 arch/arm64/include/asm/insn.h               |  4 +++-
 arch/arm64/include/asm/kvm_asm.h            |  1 +
 arch/arm64/include/asm/patching.h           |  2 --
 arch/arm64/kernel/cpufeature.c              |  1 +
 arch/arm64/kernel/ftrace.c                  |  1 +
 arch/arm64/kernel/jump_label.c              |  1 +
 arch/arm64/kernel/kgdb.c                    |  1 +
 arch/arm64/kernel/patching.c                |  2 ++
 arch/arm64/kernel/probes/kprobes.c          | 18 ++++++++++--------
 arch/arm64/kernel/traps.c                   |  2 ++
 arch/arm64/net/bpf_jit_comp.c               |  1 +
 12 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.11.0


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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 1/2] arm64: insn: decouple patching from insn code
  2021-06-09 10:22 [PATCH 0/2] arm64: insn: cleanups Mark Rutland
@ 2021-06-09 10:23 ` Mark Rutland
  2021-06-09 10:23 ` [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h> Mark Rutland
  2021-06-11 16:15 ` [PATCH 0/2] arm64: insn: cleanups Will Deacon
  2 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2021-06-09 10:23 UTC (permalink / raw)
  To: linux-arm-kernel, will; +Cc: Mark Rutland, Catalin Marinas

Currently, <asm/insn.h> includes <asm/patching.h>. We intend that
<asm/insn.h> will be usable from userspace, so it doesn't make sense to
include headers for kernel-only features such as the patching routines,
and we'd intended to restrict <asm/insn.h> to instruction encoding
details.

Let's decouple the patching code from <asm/insn.h>, and explicitly
include <asm/patching.h> where it is needed. Since <asm/patching.h>
isn't included from assembly, we can drop the __ASSEMBLY__ guards.

At the same time, sort the kprobes includes so that it's easier to see
what is and isn't incldued.

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/insn.h      |  1 -
 arch/arm64/include/asm/patching.h  |  2 --
 arch/arm64/kernel/ftrace.c         |  1 +
 arch/arm64/kernel/jump_label.c     |  1 +
 arch/arm64/kernel/kgdb.c           |  1 +
 arch/arm64/kernel/patching.c       |  1 +
 arch/arm64/kernel/probes/kprobes.c | 18 ++++++++++--------
 arch/arm64/kernel/traps.c          |  1 +
 8 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 1ea9611545bb..a6f3f45fc46f 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -11,7 +11,6 @@
 #include <linux/types.h>
 
 #include <asm/alternative.h>
-#include <asm/patching.h>
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/arch/arm64/include/asm/patching.h b/arch/arm64/include/asm/patching.h
index 5ebab129222f..6bf5adc56295 100644
--- a/arch/arm64/include/asm/patching.h
+++ b/arch/arm64/include/asm/patching.h
@@ -4,12 +4,10 @@
 
 #include <linux/types.h>
 
-#ifndef __ASSEMBLY__
 int aarch64_insn_read(void *addr, u32 *insnp);
 int aarch64_insn_write(void *addr, u32 insn);
 
 int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
 int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
-#endif /* __ASSEMBLY__ */
 
 #endif	/* __ASM_PATCHING_H */
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index b5d3ddaf69d9..7f467bd9db7a 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -15,6 +15,7 @@
 #include <asm/debug-monitors.h>
 #include <asm/ftrace.h>
 #include <asm/insn.h>
+#include <asm/patching.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 /*
diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c
index 9a8a0ae1e75f..fc98037e1220 100644
--- a/arch/arm64/kernel/jump_label.c
+++ b/arch/arm64/kernel/jump_label.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
 #include <asm/insn.h>
+#include <asm/patching.h>
 
 void arch_jump_label_transform(struct jump_entry *entry,
 			       enum jump_label_type type)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 1a157ca33262..2aede780fb80 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -17,6 +17,7 @@
 
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
+#include <asm/patching.h>
 #include <asm/traps.h>
 
 struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
index 9d050e33901b..7aa55b33c8c7 100644
--- a/arch/arm64/kernel/patching.c
+++ b/arch/arm64/kernel/patching.c
@@ -9,6 +9,7 @@
 #include <asm/cacheflush.h>
 #include <asm/fixmap.h>
 #include <asm/kprobes.h>
+#include <asm/patching.h>
 #include <asm/sections.h>
 
 static DEFINE_RAW_SPINLOCK(patch_lock);
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d607c9912025..609edde7a5dd 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -7,26 +7,28 @@
  * Copyright (C) 2013 Linaro Limited.
  * Author: Sandeepa Prabhu <sandeepa.prabhu@linaro.org>
  */
+#include <linux/extable.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>
 #include <linux/kprobes.h>
-#include <linux/extable.h>
-#include <linux/slab.h>
-#include <linux/stop_machine.h>
 #include <linux/sched/debug.h>
 #include <linux/set_memory.h>
+#include <linux/slab.h>
+#include <linux/stop_machine.h>
 #include <linux/stringify.h>
+#include <linux/uaccess.h>
 #include <linux/vmalloc.h>
-#include <asm/traps.h>
-#include <asm/ptrace.h>
+
 #include <asm/cacheflush.h>
-#include <asm/debug-monitors.h>
 #include <asm/daifflags.h>
-#include <asm/system_misc.h>
+#include <asm/debug-monitors.h>
 #include <asm/insn.h>
-#include <linux/uaccess.h>
 #include <asm/irq.h>
+#include <asm/patching.h>
+#include <asm/ptrace.h>
 #include <asm/sections.h>
+#include <asm/system_misc.h>
+#include <asm/traps.h>
 
 #include "decode-insn.h"
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 9b683b2381cf..48ff6fb888e0 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -37,6 +37,7 @@
 #include <asm/exception.h>
 #include <asm/extable.h>
 #include <asm/kprobes.h>
+#include <asm/patching.h>
 #include <asm/traps.h>
 #include <asm/smp.h>
 #include <asm/stack_pointer.h>
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  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 ` Mark Rutland
  2021-06-18  1:25   ` Nathan Chancellor
  2021-06-11 16:15 ` [PATCH 0/2] arm64: insn: cleanups Will Deacon
  2 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2021-06-09 10:23 UTC (permalink / raw)
  To: linux-arm-kernel, will; +Cc: Mark Rutland, Catalin Marinas

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.

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 | 4 +---
 arch/arm64/include/asm/insn.h               | 3 +++
 arch/arm64/include/asm/kvm_asm.h            | 1 +
 arch/arm64/kernel/cpufeature.c              | 1 +
 arch/arm64/kernel/patching.c                | 1 +
 arch/arm64/kernel/traps.c                   | 1 +
 arch/arm64/net/bpf_jit_comp.c               | 1 +
 7 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
index 8a078fc662ac..703fbf310b79 100644
--- a/arch/arm64/include/asm/alternative-macros.h
+++ b/arch/arm64/include/asm/alternative-macros.h
@@ -3,12 +3,10 @@
 #define __ASM_ALTERNATIVE_MACROS_H
 
 #include <asm/cpucaps.h>
+#include <asm/insn.h>
 
 #define ARM64_CB_PATCH ARM64_NCAPS
 
-/* A64 instructions are always 32 bits. */
-#define	AARCH64_INSN_SIZE		4
-
 #ifndef __ASSEMBLY__
 
 #include <linux/stringify.h>
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index a6f3f45fc46f..1430b4973039 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -12,6 +12,9 @@
 
 #include <asm/alternative.h>
 
+/* A64 instructions are always 32 bits. */
+#define	AARCH64_INSN_SIZE		4
+
 #ifndef __ASSEMBLY__
 /*
  * ARM Architecture Reference Manual for ARMv8 Profile-A, Issue A.a
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index cf8df032b9c3..894edda8cc85 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -8,6 +8,7 @@
 #define __ARM_KVM_ASM_H__
 
 #include <asm/hyp_image.h>
+#include <asm/insn.h>
 #include <asm/virt.h>
 
 #define ARM_EXIT_WITH_SERROR_BIT  31
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index efed2830d141..16d35cfffcea 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -76,6 +76,7 @@
 #include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
 #include <asm/fpsimd.h>
+#include <asm/insn.h>
 #include <asm/kvm_host.h>
 #include <asm/mmu_context.h>
 #include <asm/mte.h>
diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
index 7aa55b33c8c7..9a6edb9c48c7 100644
--- a/arch/arm64/kernel/patching.c
+++ b/arch/arm64/kernel/patching.c
@@ -8,6 +8,7 @@
 
 #include <asm/cacheflush.h>
 #include <asm/fixmap.h>
+#include <asm/insn.h>
 #include <asm/kprobes.h>
 #include <asm/patching.h>
 #include <asm/sections.h>
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 48ff6fb888e0..8f66072fa5cb 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -36,6 +36,7 @@
 #include <asm/esr.h>
 #include <asm/exception.h>
 #include <asm/extable.h>
+#include <asm/insn.h>
 #include <asm/kprobes.h>
 #include <asm/patching.h>
 #include <asm/traps.h>
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index f7b194878a99..dd5000da18b8 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -16,6 +16,7 @@
 #include <asm/byteorder.h>
 #include <asm/cacheflush.h>
 #include <asm/debug-monitors.h>
+#include <asm/insn.h>
 #include <asm/set_memory.h>
 
 #include "bpf_jit.h"
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] arm64: insn: cleanups
  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-11 16:15 ` Will Deacon
  2 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2021-06-11 16:15 UTC (permalink / raw)
  To: Mark Rutland, linux-arm-kernel; +Cc: catalin.marinas, kernel-team, Will Deacon

On Wed, 9 Jun 2021 11:22:59 +0100, Mark Rutland wrote:
> These couple of patches are fixups for the arm64 for-next/insn branch, which
> try to bring that inline with the intent of the original patches.
> 
> The main change is to have users of patching code explicitly include
> <asm/patching.h> (which was the intent of the original patches), as having
> <asm/insn.h> include <asm/patching.h> can create some painful header
> dependencies, and this more clearly separates the insn parts under arm64/lib/
> from the parts under arm64/kernel/. The header dependencies don't appear to be
> an issue today, but get in the way of some subsequent rework patches I'm
> developing, and I'd like to fix that now before there's any additional reliance
> on <asm/insn.h> pulling things in.
> 
> [...]

Applied to arm64 (for-next/insn), thanks!

[1/2] arm64: insn: decouple patching from insn code
      https://git.kernel.org/arm64/c/78b92c7337e1
[2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
      https://git.kernel.org/arm64/c/3e00e39d9dad

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  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
  0 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2021-06-18  1:25 UTC (permalink / raw)
  To: Mark Rutland; +Cc: linux-arm-kernel, will, Catalin Marinas, clang-built-linux

Hi Mark,

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.
> 
> 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 | 4 +---
>  arch/arm64/include/asm/insn.h               | 3 +++
>  arch/arm64/include/asm/kvm_asm.h            | 1 +
>  arch/arm64/kernel/cpufeature.c              | 1 +
>  arch/arm64/kernel/patching.c                | 1 +
>  arch/arm64/kernel/traps.c                   | 1 +
>  arch/arm64/net/bpf_jit_comp.c               | 1 +
>  7 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
> index 8a078fc662ac..703fbf310b79 100644
> --- a/arch/arm64/include/asm/alternative-macros.h
> +++ b/arch/arm64/include/asm/alternative-macros.h
> @@ -3,12 +3,10 @@
>  #define __ASM_ALTERNATIVE_MACROS_H
>  
>  #include <asm/cpucaps.h>
> +#include <asm/insn.h>
>  
>  #define ARM64_CB_PATCH ARM64_NCAPS
>  
> -/* A64 instructions are always 32 bits. */
> -#define	AARCH64_INSN_SIZE		4
> -
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/stringify.h>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index a6f3f45fc46f..1430b4973039 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -12,6 +12,9 @@
>  
>  #include <asm/alternative.h>
>  
> +/* A64 instructions are always 32 bits. */
> +#define	AARCH64_INSN_SIZE		4
> +
>  #ifndef __ASSEMBLY__
>  /*
>   * ARM Architecture Reference Manual for ARMv8 Profile-A, Issue A.a
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index cf8df032b9c3..894edda8cc85 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -8,6 +8,7 @@
>  #define __ARM_KVM_ASM_H__
>  
>  #include <asm/hyp_image.h>
> +#include <asm/insn.h>
>  #include <asm/virt.h>
>  
>  #define ARM_EXIT_WITH_SERROR_BIT  31
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index efed2830d141..16d35cfffcea 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -76,6 +76,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/cpu_ops.h>
>  #include <asm/fpsimd.h>
> +#include <asm/insn.h>
>  #include <asm/kvm_host.h>
>  #include <asm/mmu_context.h>
>  #include <asm/mte.h>
> diff --git a/arch/arm64/kernel/patching.c b/arch/arm64/kernel/patching.c
> index 7aa55b33c8c7..9a6edb9c48c7 100644
> --- a/arch/arm64/kernel/patching.c
> +++ b/arch/arm64/kernel/patching.c
> @@ -8,6 +8,7 @@
>  
>  #include <asm/cacheflush.h>
>  #include <asm/fixmap.h>
> +#include <asm/insn.h>
>  #include <asm/kprobes.h>
>  #include <asm/patching.h>
>  #include <asm/sections.h>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 48ff6fb888e0..8f66072fa5cb 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -36,6 +36,7 @@
>  #include <asm/esr.h>
>  #include <asm/exception.h>
>  #include <asm/extable.h>
> +#include <asm/insn.h>
>  #include <asm/kprobes.h>
>  #include <asm/patching.h>
>  #include <asm/traps.h>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index f7b194878a99..dd5000da18b8 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -16,6 +16,7 @@
>  #include <asm/byteorder.h>
>  #include <asm/cacheflush.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/insn.h>
>  #include <asm/set_memory.h>
>  
>  #include "bpf_jit.h"
> -- 
> 2.11.0
> 

I bisected a CONFIG_LTO_CLANG_THIN=y build failure that our CI reported
to this patch:

https://builds.tuxbuild.com/1u4Fpx2FQkkgkyPxWtq0Ke4YFCQ/build.log

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. It seems only CONFIG_LTO_CLANG_THIN=y (or
FULL) is enough to trigger this, a regular defconfig build is fine:

https://builds.tuxbuild.com/1u4Fpt5M8quEVUd4kNSMrdzobC2/build.log

I will try to look into this tomorrow but I figured I would let you know
in case something obviously stuck out.

Cheers,
Nathan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  2021-06-18  1:25   ` Nathan Chancellor
@ 2021-06-18 15:18     ` Mark Rutland
  2021-06-18 16:53       ` Nathan Chancellor
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2021-06-18 15:18 UTC (permalink / raw)
  To: Nathan Chancellor, will
  Cc: linux-arm-kernel, Catalin Marinas, clang-built-linux

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.

Will, are you happy to take the fixup patch below, or would you prefer
to drop this patch for now?

Thanks,
Mark.

---->8----
From 0acc3d92302f54475d938f55749805adf74faec1 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.h               | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

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.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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  2021-06-18 15:18     ` Mark Rutland
@ 2021-06-18 16:53       ` Nathan Chancellor
  2021-06-21  8:08         ` Mark Rutland
  0 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2021-06-18 16:53 UTC (permalink / raw)
  To: Mark Rutland; +Cc: will, linux-arm-kernel, Catalin Marinas, clang-built-linux

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?
> 
> Thanks,
> Mark.
> 
> ---->8----
> >From 0acc3d92302f54475d938f55749805adf74faec1 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.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>

Thank you for the quick fix!

Cheers,
Nathan

>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> 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.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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  2021-06-18 16:53       ` Nathan Chancellor
@ 2021-06-21  8:08         ` Mark Rutland
  2021-06-21 10:59           ` Will Deacon
  2021-06-21 16:12           ` Nathan Chancellor
  0 siblings, 2 replies; 10+ messages in thread
From: Mark Rutland @ 2021-06-21  8:08 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: will, linux-arm-kernel, Catalin Marinas, clang-built-linux

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  2021-06-21  8:08         ` Mark Rutland
@ 2021-06-21 10:59           ` Will Deacon
  2021-06-21 16:12           ` Nathan Chancellor
  1 sibling, 0 replies; 10+ messages in thread
From: Will Deacon @ 2021-06-21 10:59 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Nathan Chancellor, linux-arm-kernel, Catalin Marinas, clang-built-linux

On Mon, Jun 21, 2021 at 09:08:30AM +0100, Mark Rutland wrote:
> 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.

I've queued this locally on top of for-next/insn and will push out later
on (once my LTO build has finished).

Will

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] arm64: insn: move AARCH64_INSN_SIZE into <asm/insn.h>
  2021-06-21  8:08         ` Mark Rutland
  2021-06-21 10:59           ` Will Deacon
@ 2021-06-21 16:12           ` Nathan Chancellor
  1 sibling, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2021-06-21 16:12 UTC (permalink / raw)
  To: Mark Rutland; +Cc: will, linux-arm-kernel, Catalin Marinas, clang-built-linux

On 6/21/2021 1:08 AM, Mark Rutland wrote:
> 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?

Yes, that is exactly what I did so the tag should still be good.

Cheers,
Nathan

> 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__
>   /*
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-21 16:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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 NNTP newsgroup(s).