From: Mark Rutland <mark.rutland@arm.com> Date: 2021-06-09 10:26:09
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-06-09 10:26:46
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(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-06-09 10:27:18
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(-)
From: Will Deacon <will@kernel.org> Date: 2021-06-11 16:18:06
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.
[...]
Hi Mark,
On Wed, Jun 09, 2021 at 11:23:01AM +0100, Mark Rutland wrote:
quoted hunk
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(-)
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-06-18 15:21:01
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:
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.
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(-)
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:
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.
Thanks 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.
Will, are you happy to take the fixup patch below, or would you prefer
to drop this patch for now?
Thanks,
Mark.
---->8----
quoted
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-06-21 08:10:52
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:
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.
Thanks 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?
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
From: Will Deacon <will@kernel.org> Date: 2021-06-21 11:00:51
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:
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.
Thanks 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?
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
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.
Thanks 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?
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
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