* [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
@ 2018-12-17 16:03 Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 09/12] Revert "kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs" Masahiro Yamada
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-17 16:03 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin
Cc: Richard Biener, Segher Boessenkool, Peter Zijlstra, Juergen Gross,
Josh Poimboeuf, Kees Cook, Linus Torvalds, Masahiro Yamada,
Arnd Bergmann, Andrey Ryabinin, virtualization,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn,
linux-arch, Alexey Dobriyan, linux-sparse, Andrew Morton,
linux-kbuild, Yonghong Song, Michal Marek,
Arnaldo Carvalho de Melo, Jan Beulich, Nadav Amit,
David Woodhouse, Alexei Starovoitov, linux-kernel
This series reverts the in-kernel workarounds for inlining issues.
The commit description of 77b0bf55bc67 mentioned
"We also hope that GCC will eventually get fixed,..."
Now, GCC provides a solution.
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
explains the new "asm inline" syntax.
The performance issue will be eventually solved.
[About Code cleanups]
I know Nadam Amit is opposed to the full revert.
He also claims his motivation for macrofying was not only
performance, but also cleanups.
IIUC, the criticism addresses the code duplication between C and ASM.
If so, I'd like to suggest a different approach for cleanups.
Please see the last 3 patches.
IMHO, preprocessor approach is more straight-forward, and readable.
Basically, this idea should work because it is what we already do for
__ASM_FORM() etc.
[Quick Test of "asm inline" of GCC 9]
If you want to try "asm inline" feature, the patch is available:
https://lore.kernel.org/patchwork/patch/1024590/
The number of symbols for arch/x86/configs/x86_64_defconfig:
nr_symbols
[1] v4.20-rc7 : 96502
[2] [1]+full revert : 96705 (+203)
[3] [2]+"asm inline": 96568 (-137)
[3]: apply my patch, then replace "asm" -> "asm_inline"
for _BUG_FLAGS(), refcount_add(), refcount_inc(), refcount_dec(),
annotate_reachable(), annotate_unreachable()
Changes in v3:
- Split into per-commit revert (per Nadav Amit)
- Add some cleanups with preprocessor approach
Changes in v2:
- Revive clean-ups made by 5bdcd510c2ac (per Peter Zijlstra)
- Fix commit quoting style (per Peter Zijlstra)
Masahiro Yamada (12):
Revert "x86/jump-labels: Macrofy inline assembly code to work around
GCC inlining bugs"
Revert "x86/cpufeature: Macrofy inline assembly code to work around
GCC inlining bugs"
Revert "x86/extable: Macrofy inline assembly code to work around GCC
inlining bugs"
Revert "x86/paravirt: Work around GCC inlining bugs when compiling
paravirt ops"
Revert "x86/bug: Macrofy the BUG table section handling, to work
around GCC inlining bugs"
Revert "x86/alternatives: Macrofy lock prefixes to work around GCC
inlining bugs"
Revert "x86/refcount: Work around GCC inlining bug"
Revert "x86/objtool: Use asm macros to work around GCC inlining bugs"
Revert "kbuild/Makefile: Prepare for using macros in inline assembly
code to work around asm() related GCC inlining bugs"
linux/linkage: add ASM() macro to reduce duplication between C/ASM
code
x86/alternatives: consolidate LOCK_PREFIX macro
x86/asm: consolidate ASM_EXTABLE_* macros
Makefile | 9 +--
arch/x86/Makefile | 7 ---
arch/x86/include/asm/alternative-asm.h | 22 +------
arch/x86/include/asm/alternative-common.h | 47 +++++++++++++++
arch/x86/include/asm/alternative.h | 30 +---------
arch/x86/include/asm/asm.h | 46 +++++----------
arch/x86/include/asm/bug.h | 98 +++++++++++++------------------
arch/x86/include/asm/cpufeature.h | 82 +++++++++++---------------
arch/x86/include/asm/jump_label.h | 22 +++++--
arch/x86/include/asm/paravirt_types.h | 56 +++++++++---------
arch/x86/include/asm/refcount.h | 81 +++++++++++--------------
arch/x86/kernel/macros.S | 16 -----
include/asm-generic/bug.h | 8 +--
include/linux/compiler.h | 56 ++++--------------
include/linux/linkage.h | 8 +++
scripts/Kbuild.include | 4 +-
scripts/mod/Makefile | 2 -
17 files changed, 249 insertions(+), 345 deletions(-)
create mode 100644 arch/x86/include/asm/alternative-common.h
delete mode 100644 arch/x86/kernel/macros.S
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 09/12] Revert "kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs"
2018-12-17 16:03 [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Masahiro Yamada
@ 2018-12-17 16:03 ` Masahiro Yamada
2018-12-18 19:43 ` [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Nadav Amit
2018-12-19 11:20 ` Ingo Molnar
2 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-17 16:03 UTC (permalink / raw)
To: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin
Cc: Richard Biener, Segher Boessenkool, Peter Zijlstra, Juergen Gross,
Josh Poimboeuf, Kees Cook, Linus Torvalds, Masahiro Yamada,
Logan Gunthorpe, Nadav Amit, linux-kbuild, linux-kernel,
Michal Marek
This reverts commit 77b0bf55bc675233d22cd5df97605d516d64525e.
A few days after the patch set applied, discussion started to solve
the issue more elegantly with the help of compiler:
https://lkml.org/lkml/2018/10/7/92
The new syntax "asm inline" was implemented by Segher Boessenkool, and
now queued up for GCC 9. (People were positive even for back-porting it
to older compilers).
Since the in-kernel workarounds merged, some issues have been reported.
The current urgent issue is distro packages for module building. More
fundamentally, we cannot build external modules after 'make clean'
because *.s files are globally removed.
We could fix those in Makefiles, but I do not want to mess up the build
system any more.
Given that this issue will be solved in a cleaner way sooner or later,
let's revert the in-kernel workarounds, and wait for GCC 9.
Link: https://lkml.org/lkml/2018/11/15/467
Link: https://marc.info/?t=154212770600037&r=1&w=2
Reported-by: Sedat Dilek <sedat.dilek@gmail.com> # deb/rpm package
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Nadav Amit <namit@vmware.com>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
---
Makefile | 9 ++-------
arch/x86/Makefile | 7 -------
arch/x86/kernel/macros.S | 7 -------
scripts/Kbuild.include | 4 +---
scripts/mod/Makefile | 2 --
5 files changed, 3 insertions(+), 26 deletions(-)
delete mode 100644 arch/x86/kernel/macros.S
diff --git a/Makefile b/Makefile
index 56d5270..885c74b 100644
--- a/Makefile
+++ b/Makefile
@@ -1081,7 +1081,7 @@ scripts: scripts_basic scripts_dtc asm-generic gcc-plugins $(autoksyms_h)
# version.h and scripts_basic is processed / created.
# Listed in dependency order
-PHONY += prepare archprepare macroprepare prepare0 prepare1 prepare2 prepare3
+PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
# prepare3 is used to check if we are building in a separate output directory,
# and if so do:
@@ -1104,9 +1104,7 @@ prepare2: prepare3 outputmakefile asm-generic
prepare1: prepare2 $(version_h) $(autoksyms_h) include/generated/utsrelease.h
$(cmd_crmodverdir)
-macroprepare: prepare1 archmacros
-
-archprepare: archheaders archscripts macroprepare scripts_basic
+archprepare: archheaders archscripts prepare1 scripts_basic
prepare0: archprepare gcc-plugins
$(Q)$(MAKE) $(build)=.
@@ -1174,9 +1172,6 @@ archheaders:
PHONY += archscripts
archscripts:
-PHONY += archmacros
-archmacros:
-
PHONY += __headers
__headers: $(version_h) scripts_basic uapi-asm-generic archheaders archscripts
$(Q)$(MAKE) $(build)=scripts build_unifdef
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 75ef499..85a66c4 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -232,13 +232,6 @@ archscripts: scripts_basic
archheaders:
$(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all
-archmacros:
- $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
-
-ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
-export ASM_MACRO_FLAGS
-KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
-
###
# Kernel objects
diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S
deleted file mode 100644
index cfc1c7d..0000000
--- a/arch/x86/kernel/macros.S
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-/*
- * This file includes headers whose assembly part includes macros which are
- * commonly used. The macros are precompiled into assmebly file which is later
- * assembled together with each compiled file.
- */
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index bb01555..3d09844 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -115,9 +115,7 @@ __cc-option = $(call try-run,\
# Do not attempt to build with gcc plugins during cc-option tests.
# (And this uses delayed resolution so the flags will be up to date.)
-# In addition, do not include the asm macros which are built later.
-CC_OPTION_FILTERED = $(GCC_PLUGINS_CFLAGS) $(ASM_MACRO_FLAGS)
-CC_OPTION_CFLAGS = $(filter-out $(CC_OPTION_FILTERED),$(KBUILD_CFLAGS))
+CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
# cc-option
# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index a5b4af4..42c5d50 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -4,8 +4,6 @@ OBJECT_FILES_NON_STANDARD := y
hostprogs-y := modpost mk_elfconfig
always := $(hostprogs-y) empty.o
-CFLAGS_REMOVE_empty.o := $(ASM_MACRO_FLAGS)
-
modpost-objs := modpost.o file2alias.o sumversion.o
devicetable-offsets-file := devicetable-offsets.h
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-17 16:03 [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 09/12] Revert "kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs" Masahiro Yamada
@ 2018-12-18 19:43 ` Nadav Amit
2018-12-19 3:19 ` Masahiro Yamada
2018-12-19 11:20 ` Ingo Molnar
2 siblings, 1 reply; 6+ messages in thread
From: Nadav Amit @ 2018-12-18 19:43 UTC (permalink / raw)
To: Masahiro Yamada
Cc: X86 ML, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Richard Biener, Segher Boessenkool,
Peter Zijlstra, Juergen Gross, Josh Poimboeuf, Kees Cook,
Linus Torvalds, Arnd Bergmann, Andrey Ryabinin,
virtualization@lists.linux-foundation.org, Luc Van Oostenryck,
Alok Kataria, Ard Biesheuvel, Jann Horn, linux-arch,
Alexey Dobriyan, linux-sparse@vger.kernel.org, Andrew Morton,
Linux Kbuild mailing list, Yonghong Song, Michal Marek,
Arnaldo Carvalho de Melo, Jan Beulich, David Woodhouse,
Alexei Starovoitov, linux-kernel@vger.kernel.org
> On Dec 17, 2018, at 8:03 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> This series reverts the in-kernel workarounds for inlining issues.
>
> The commit description of 77b0bf55bc67 mentioned
> "We also hope that GCC will eventually get fixed,..."
>
> Now, GCC provides a solution.
>
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FExtended-Asm.html&data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&reserved=0
> explains the new "asm inline" syntax.
>
> The performance issue will be eventually solved.
>
> [About Code cleanups]
>
> I know Nadam Amit is opposed to the full revert.
My name is Nadav.
> He also claims his motivation for macrofying was not only
> performance, but also cleanups.
Masahiro, I understand your concerns and criticism, and indeed various
alternative solutions exist. I do have my reservations about the one you
propose, since it makes coding more complicated to simplify the Make system.
Yet, more important, starting this discussion suddenly now after RC7 is
strange. Anyhow, since it’s obviously not my call, please don’t make it
sound as if I am a side in the decision.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-18 19:43 ` [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Nadav Amit
@ 2018-12-19 3:19 ` Masahiro Yamada
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-19 3:19 UTC (permalink / raw)
To: Nadav Amit, X86 ML, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin
Cc: Richard Biener, Segher Boessenkool, Peter Zijlstra, Juergen Gross,
Josh Poimboeuf, Kees Cook, Linus Torvalds, Arnd Bergmann,
Andrey Ryabinin, virtualization@lists.linux-foundation.org,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn,
linux-arch, Alexey Dobriyan, linux-sparse@vger.kernel.org,
Andrew Morton, Linux Kbuild mailing list, Yonghong Song,
Michal Marek, Arnaldo Carvalho de Melo, Jan Beulich,
David Woodhouse, Alexei Starovoitov, linux-kernel@vger.kernel.org
On Wed, Dec 19, 2018 at 5:26 AM Nadav Amit <namit@vmware.com> wrote:
>
> > On Dec 17, 2018, at 8:03 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> > This series reverts the in-kernel workarounds for inlining issues.
> >
> > The commit description of 77b0bf55bc67 mentioned
> > "We also hope that GCC will eventually get fixed,..."
> >
> > Now, GCC provides a solution.
> >
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FExtended-Asm.html&data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&reserved=0
> > explains the new "asm inline" syntax.
> >
> > The performance issue will be eventually solved.
> >
> > [About Code cleanups]
> >
> > I know Nadam Amit is opposed to the full revert.
>
> My name is Nadav.
Sorry about that.
(I relied on a spell checker. I should be careful about typos in people's name.)
> > He also claims his motivation for macrofying was not only
> > performance, but also cleanups.
>
> Masahiro, I understand your concerns and criticism, and indeed various
> alternative solutions exist. I do have my reservations about the one you
> propose, since it makes coding more complicated to simplify the Make system.
> Yet, more important, starting this discussion suddenly now after RC7 is
> strange.
I did not think this was so sudden.
When I suggested the revert a few weeks ago,
Borislav was for it.
I did not hear from the other x86 maintainers.
Anyway, it is true we are running out of time for the release.
> Anyhow, since it’s obviously not my call, please don’t make it
> sound as if I am a side in the decision.
>
Not my call, either.
That's why I put the x86 maintainers in the TO list,
and other people in CC.
The original patch set went in via x86 tree.
So, its revert set, if we want,
should go in the same path.
Anyway, we have to do something for v4.20.
Maybe, discussing short-term / long-term solutions
separately could move things forward.
[1] Solution for v4.20
[2] Solution for future kernel
If we do not want to see the revert for [1],
the best I can suggest is to copy arch/x86/kernel/macros.s
to include/generated/macros.h
so that it is kept for the external module build.
(It is not literally included by anyone, but should work at least.)
For [2], what do we want to see?
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-17 16:03 [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 09/12] Revert "kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs" Masahiro Yamada
2018-12-18 19:43 ` [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Nadav Amit
@ 2018-12-19 11:20 ` Ingo Molnar
2018-12-19 14:33 ` Masahiro Yamada
2 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2018-12-19 11:20 UTC (permalink / raw)
To: Masahiro Yamada
Cc: x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Richard Biener, Segher Boessenkool,
Peter Zijlstra, Juergen Gross, Josh Poimboeuf, Kees Cook,
Linus Torvalds, Arnd Bergmann, Andrey Ryabinin, virtualization,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn,
linux-arch, Alexey Dobriyan, linux-sparse, Andrew Morton,
linux-kbuild, Yonghong Song, Michal Marek,
Arnaldo Carvalho de Melo, Jan Beulich, Nadav Amit,
David Woodhouse, Alexei Starovoitov, linux-kernel
* Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> This series reverts the in-kernel workarounds for inlining issues.
>
> The commit description of 77b0bf55bc67 mentioned
> "We also hope that GCC will eventually get fixed,..."
>
> Now, GCC provides a solution.
>
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> explains the new "asm inline" syntax.
>
> The performance issue will be eventually solved.
>
> [About Code cleanups]
>
> I know Nadam Amit is opposed to the full revert.
> He also claims his motivation for macrofying was not only
> performance, but also cleanups.
>
> IIUC, the criticism addresses the code duplication between C and ASM.
>
> If so, I'd like to suggest a different approach for cleanups.
> Please see the last 3 patches.
> IMHO, preprocessor approach is more straight-forward, and readable.
> Basically, this idea should work because it is what we already do for
> __ASM_FORM() etc.
>
> [Quick Test of "asm inline" of GCC 9]
>
> If you want to try "asm inline" feature, the patch is available:
> https://lore.kernel.org/patchwork/patch/1024590/
>
> The number of symbols for arch/x86/configs/x86_64_defconfig:
>
> nr_symbols
> [1] v4.20-rc7 : 96502
> [2] [1]+full revert : 96705 (+203)
> [3] [2]+"asm inline": 96568 (-137)
>
> [3]: apply my patch, then replace "asm" -> "asm_inline"
> for _BUG_FLAGS(), refcount_add(), refcount_inc(), refcount_dec(),
> annotate_reachable(), annotate_unreachable()
>
>
> Changes in v3:
> - Split into per-commit revert (per Nadav Amit)
> - Add some cleanups with preprocessor approach
>
> Changes in v2:
> - Revive clean-ups made by 5bdcd510c2ac (per Peter Zijlstra)
> - Fix commit quoting style (per Peter Zijlstra)
>
> Masahiro Yamada (12):
> Revert "x86/jump-labels: Macrofy inline assembly code to work around
> GCC inlining bugs"
> Revert "x86/cpufeature: Macrofy inline assembly code to work around
> GCC inlining bugs"
> Revert "x86/extable: Macrofy inline assembly code to work around GCC
> inlining bugs"
> Revert "x86/paravirt: Work around GCC inlining bugs when compiling
> paravirt ops"
> Revert "x86/bug: Macrofy the BUG table section handling, to work
> around GCC inlining bugs"
> Revert "x86/alternatives: Macrofy lock prefixes to work around GCC
> inlining bugs"
> Revert "x86/refcount: Work around GCC inlining bug"
> Revert "x86/objtool: Use asm macros to work around GCC inlining bugs"
> Revert "kbuild/Makefile: Prepare for using macros in inline assembly
> code to work around asm() related GCC inlining bugs"
> linux/linkage: add ASM() macro to reduce duplication between C/ASM
> code
> x86/alternatives: consolidate LOCK_PREFIX macro
> x86/asm: consolidate ASM_EXTABLE_* macros
>
> Makefile | 9 +--
> arch/x86/Makefile | 7 ---
> arch/x86/include/asm/alternative-asm.h | 22 +------
> arch/x86/include/asm/alternative-common.h | 47 +++++++++++++++
> arch/x86/include/asm/alternative.h | 30 +---------
> arch/x86/include/asm/asm.h | 46 +++++----------
> arch/x86/include/asm/bug.h | 98 +++++++++++++------------------
> arch/x86/include/asm/cpufeature.h | 82 +++++++++++---------------
> arch/x86/include/asm/jump_label.h | 22 +++++--
> arch/x86/include/asm/paravirt_types.h | 56 +++++++++---------
> arch/x86/include/asm/refcount.h | 81 +++++++++++--------------
> arch/x86/kernel/macros.S | 16 -----
> include/asm-generic/bug.h | 8 +--
> include/linux/compiler.h | 56 ++++--------------
> include/linux/linkage.h | 8 +++
> scripts/Kbuild.include | 4 +-
> scripts/mod/Makefile | 2 -
> 17 files changed, 249 insertions(+), 345 deletions(-)
> create mode 100644 arch/x86/include/asm/alternative-common.h
> delete mode 100644 arch/x86/kernel/macros.S
I absolutely agree that this needs to be resolved in v4.20.
So I did the 1-9 reverts manually myself as well, because I think the
first commit should be reverted fully to get as close to the starting
point as possible (we are late in the cycle) - and came to the attached
interdiff between your series and mine.
Does this approach look OK to you, or did I miss something?
Thanks,
Ingo
=============>
entry/calling.h | 2 -
include/asm/jump_label.h | 50 ++++++++++++++++++++++++++++++++++-------------
2 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 25e5a6bda8c3..20d0885b00fb 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -352,7 +352,7 @@ For 32-bit we have the following conventions - kernel is built with
.macro CALL_enter_from_user_mode
#ifdef CONFIG_CONTEXT_TRACKING
#ifdef HAVE_JUMP_LABEL
- STATIC_BRANCH_JMP l_yes=.Lafter_call_\@, key=context_tracking_enabled, branch=1
+ STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
#endif
call enter_from_user_mode
.Lafter_call_\@:
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index cf88ebf9a4ca..21efc9d07ed9 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -2,6 +2,19 @@
#ifndef _ASM_X86_JUMP_LABEL_H
#define _ASM_X86_JUMP_LABEL_H
+#ifndef HAVE_JUMP_LABEL
+/*
+ * For better or for worse, if jump labels (the gcc extension) are missing,
+ * then the entire static branch patching infrastructure is compiled out.
+ * If that happens, the code in here will malfunction. Raise a compiler
+ * error instead.
+ *
+ * In theory, jump labels and the static branch patching infrastructure
+ * could be decoupled to fix this.
+ */
+#error asm/jump_label.h included on a non-jump-label kernel
+#endif
+
#define JUMP_LABEL_NOP_SIZE 5
#ifdef CONFIG_X86_64
@@ -53,26 +66,37 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
#else /* __ASSEMBLY__ */
-.macro STATIC_BRANCH_NOP l_yes:req key:req branch:req
-.Lstatic_branch_nop_\@:
- .byte STATIC_KEY_INIT_NOP
-.Lstatic_branch_no_after_\@:
+.macro STATIC_JUMP_IF_TRUE target, key, def
+.Lstatic_jump_\@:
+ .if \def
+ /* Equivalent to "jmp.d32 \target" */
+ .byte 0xe9
+ .long \target - .Lstatic_jump_after_\@
+.Lstatic_jump_after_\@:
+ .else
+ .byte STATIC_KEY_INIT_NOP
+ .endif
.pushsection __jump_table, "aw"
_ASM_ALIGN
- .long .Lstatic_branch_nop_\@ - ., \l_yes - .
- _ASM_PTR \key + \branch - .
+ .long .Lstatic_jump_\@ - ., \target - .
+ _ASM_PTR \key - .
.popsection
.endm
-.macro STATIC_BRANCH_JMP l_yes:req key:req branch:req
-.Lstatic_branch_jmp_\@:
- .byte 0xe9
- .long \l_yes - .Lstatic_branch_jmp_after_\@
-.Lstatic_branch_jmp_after_\@:
+.macro STATIC_JUMP_IF_FALSE target, key, def
+.Lstatic_jump_\@:
+ .if \def
+ .byte STATIC_KEY_INIT_NOP
+ .else
+ /* Equivalent to "jmp.d32 \target" */
+ .byte 0xe9
+ .long \target - .Lstatic_jump_after_\@
+.Lstatic_jump_after_\@:
+ .endif
.pushsection __jump_table, "aw"
_ASM_ALIGN
- .long .Lstatic_branch_jmp_\@ - ., \l_yes - .
- _ASM_PTR \key + \branch - .
+ .long .Lstatic_jump_\@ - ., \target - .
+ _ASM_PTR \key + 1 - .
.popsection
.endm
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-19 11:20 ` Ingo Molnar
@ 2018-12-19 14:33 ` Masahiro Yamada
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Yamada @ 2018-12-19 14:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: X86 ML, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Richard Biener, Segher Boessenkool,
Peter Zijlstra, Juergen Gross, Josh Poimboeuf, Kees Cook,
Linus Torvalds, Arnd Bergmann, Andrey Ryabinin, virtualization,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn,
linux-arch, Alexey Dobriyan, linux-sparse, Andrew Morton,
Linux Kbuild mailing list, Yonghong Song, Michal Marek,
Arnaldo Carvalho de Melo, Jan Beulich, Nadav Amit,
David Woodhouse, Alexei Starovoitov, Linux Kernel Mailing List
On Wed, Dec 19, 2018 at 9:44 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> > This series reverts the in-kernel workarounds for inlining issues.
> >
> > The commit description of 77b0bf55bc67 mentioned
> > "We also hope that GCC will eventually get fixed,..."
> >
> > Now, GCC provides a solution.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> > explains the new "asm inline" syntax.
> >
> > The performance issue will be eventually solved.
> >
> > [About Code cleanups]
> >
> > I know Nadam Amit is opposed to the full revert.
> > He also claims his motivation for macrofying was not only
> > performance, but also cleanups.
> >
> > IIUC, the criticism addresses the code duplication between C and ASM.
> >
> > If so, I'd like to suggest a different approach for cleanups.
> > Please see the last 3 patches.
> > IMHO, preprocessor approach is more straight-forward, and readable.
> > Basically, this idea should work because it is what we already do for
> > __ASM_FORM() etc.
> >
> > [Quick Test of "asm inline" of GCC 9]
> >
> > If you want to try "asm inline" feature, the patch is available:
> > https://lore.kernel.org/patchwork/patch/1024590/
> >
> > The number of symbols for arch/x86/configs/x86_64_defconfig:
> >
> > nr_symbols
> > [1] v4.20-rc7 : 96502
> > [2] [1]+full revert : 96705 (+203)
> > [3] [2]+"asm inline": 96568 (-137)
> >
> > [3]: apply my patch, then replace "asm" -> "asm_inline"
> > for _BUG_FLAGS(), refcount_add(), refcount_inc(), refcount_dec(),
> > annotate_reachable(), annotate_unreachable()
> >
> >
> > Changes in v3:
> > - Split into per-commit revert (per Nadav Amit)
> > - Add some cleanups with preprocessor approach
> >
> > Changes in v2:
> > - Revive clean-ups made by 5bdcd510c2ac (per Peter Zijlstra)
> > - Fix commit quoting style (per Peter Zijlstra)
> >
> > Masahiro Yamada (12):
> > Revert "x86/jump-labels: Macrofy inline assembly code to work around
> > GCC inlining bugs"
> > Revert "x86/cpufeature: Macrofy inline assembly code to work around
> > GCC inlining bugs"
> > Revert "x86/extable: Macrofy inline assembly code to work around GCC
> > inlining bugs"
> > Revert "x86/paravirt: Work around GCC inlining bugs when compiling
> > paravirt ops"
> > Revert "x86/bug: Macrofy the BUG table section handling, to work
> > around GCC inlining bugs"
> > Revert "x86/alternatives: Macrofy lock prefixes to work around GCC
> > inlining bugs"
> > Revert "x86/refcount: Work around GCC inlining bug"
> > Revert "x86/objtool: Use asm macros to work around GCC inlining bugs"
> > Revert "kbuild/Makefile: Prepare for using macros in inline assembly
> > code to work around asm() related GCC inlining bugs"
> > linux/linkage: add ASM() macro to reduce duplication between C/ASM
> > code
> > x86/alternatives: consolidate LOCK_PREFIX macro
> > x86/asm: consolidate ASM_EXTABLE_* macros
> >
> > Makefile | 9 +--
> > arch/x86/Makefile | 7 ---
> > arch/x86/include/asm/alternative-asm.h | 22 +------
> > arch/x86/include/asm/alternative-common.h | 47 +++++++++++++++
> > arch/x86/include/asm/alternative.h | 30 +---------
> > arch/x86/include/asm/asm.h | 46 +++++----------
> > arch/x86/include/asm/bug.h | 98 +++++++++++++------------------
> > arch/x86/include/asm/cpufeature.h | 82 +++++++++++---------------
> > arch/x86/include/asm/jump_label.h | 22 +++++--
> > arch/x86/include/asm/paravirt_types.h | 56 +++++++++---------
> > arch/x86/include/asm/refcount.h | 81 +++++++++++--------------
> > arch/x86/kernel/macros.S | 16 -----
> > include/asm-generic/bug.h | 8 +--
> > include/linux/compiler.h | 56 ++++--------------
> > include/linux/linkage.h | 8 +++
> > scripts/Kbuild.include | 4 +-
> > scripts/mod/Makefile | 2 -
> > 17 files changed, 249 insertions(+), 345 deletions(-)
> > create mode 100644 arch/x86/include/asm/alternative-common.h
> > delete mode 100644 arch/x86/kernel/macros.S
>
> I absolutely agree that this needs to be resolved in v4.20.
>
> So I did the 1-9 reverts manually myself as well, because I think the
> first commit should be reverted fully to get as close to the starting
> point as possible (we are late in the cycle) - and came to the attached
> interdiff between your series and mine.
>
> Does this approach look OK to you, or did I miss something?
It looks OK to me.
I thought the diff was a good cleanup part,
but we can deal with it later on,
so I do not mind it.
Thanks!
> Thanks,
>
> Ingo
>
> =============>
>
> entry/calling.h | 2 -
> include/asm/jump_label.h | 50 ++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 25e5a6bda8c3..20d0885b00fb 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -352,7 +352,7 @@ For 32-bit we have the following conventions - kernel is built with
> .macro CALL_enter_from_user_mode
> #ifdef CONFIG_CONTEXT_TRACKING
> #ifdef HAVE_JUMP_LABEL
> - STATIC_BRANCH_JMP l_yes=.Lafter_call_\@, key=context_tracking_enabled, branch=1
> + STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
> #endif
> call enter_from_user_mode
> .Lafter_call_\@:
> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
> index cf88ebf9a4ca..21efc9d07ed9 100644
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -2,6 +2,19 @@
> #ifndef _ASM_X86_JUMP_LABEL_H
> #define _ASM_X86_JUMP_LABEL_H
>
> +#ifndef HAVE_JUMP_LABEL
> +/*
> + * For better or for worse, if jump labels (the gcc extension) are missing,
> + * then the entire static branch patching infrastructure is compiled out.
> + * If that happens, the code in here will malfunction. Raise a compiler
> + * error instead.
> + *
> + * In theory, jump labels and the static branch patching infrastructure
> + * could be decoupled to fix this.
> + */
> +#error asm/jump_label.h included on a non-jump-label kernel
> +#endif
> +
> #define JUMP_LABEL_NOP_SIZE 5
>
> #ifdef CONFIG_X86_64
> @@ -53,26 +66,37 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
>
> #else /* __ASSEMBLY__ */
>
> -.macro STATIC_BRANCH_NOP l_yes:req key:req branch:req
> -.Lstatic_branch_nop_\@:
> - .byte STATIC_KEY_INIT_NOP
> -.Lstatic_branch_no_after_\@:
> +.macro STATIC_JUMP_IF_TRUE target, key, def
> +.Lstatic_jump_\@:
> + .if \def
> + /* Equivalent to "jmp.d32 \target" */
> + .byte 0xe9
> + .long \target - .Lstatic_jump_after_\@
> +.Lstatic_jump_after_\@:
> + .else
> + .byte STATIC_KEY_INIT_NOP
> + .endif
> .pushsection __jump_table, "aw"
> _ASM_ALIGN
> - .long .Lstatic_branch_nop_\@ - ., \l_yes - .
> - _ASM_PTR \key + \branch - .
> + .long .Lstatic_jump_\@ - ., \target - .
> + _ASM_PTR \key - .
> .popsection
> .endm
>
> -.macro STATIC_BRANCH_JMP l_yes:req key:req branch:req
> -.Lstatic_branch_jmp_\@:
> - .byte 0xe9
> - .long \l_yes - .Lstatic_branch_jmp_after_\@
> -.Lstatic_branch_jmp_after_\@:
> +.macro STATIC_JUMP_IF_FALSE target, key, def
> +.Lstatic_jump_\@:
> + .if \def
> + .byte STATIC_KEY_INIT_NOP
> + .else
> + /* Equivalent to "jmp.d32 \target" */
> + .byte 0xe9
> + .long \target - .Lstatic_jump_after_\@
> +.Lstatic_jump_after_\@:
> + .endif
> .pushsection __jump_table, "aw"
> _ASM_ALIGN
> - .long .Lstatic_branch_jmp_\@ - ., \l_yes - .
> - _ASM_PTR \key + \branch - .
> + .long .Lstatic_jump_\@ - ., \target - .
> + _ASM_PTR \key + 1 - .
> .popsection
> .endm
>
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-19 14:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-17 16:03 [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 09/12] Revert "kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs" Masahiro Yamada
2018-12-18 19:43 ` [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code Nadav Amit
2018-12-19 3:19 ` Masahiro Yamada
2018-12-19 11:20 ` Ingo Molnar
2018-12-19 14:33 ` Masahiro Yamada
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox