* [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 ` Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs" Masahiro Yamada
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ 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] 12+ messages in thread* [PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around 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-17 16:03 ` 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
3 siblings, 1 reply; 12+ 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, linux-arch, Arnd Bergmann, linux-kernel,
Nadav Amit
This reverts commit f81f8ad56fd1c7b99b2ed1c314527f7d9ac447c6.
The in-kernel workarounds will be replaced with GCC's new
"asm inline" syntax.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/x86/include/asm/bug.h | 98 ++++++++++++++++++++--------------------------
arch/x86/kernel/macros.S | 1 -
include/asm-generic/bug.h | 8 ++--
3 files changed, 46 insertions(+), 61 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 5090035..6804d66 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -4,8 +4,6 @@
#include <linux/stringify.h>
-#ifndef __ASSEMBLY__
-
/*
* Despite that some emulators terminate on UD2, we use it for WARN().
*
@@ -22,15 +20,53 @@
#define LEN_UD2 2
+#ifdef CONFIG_GENERIC_BUG
+
+#ifdef CONFIG_X86_32
+# define __BUG_REL(val) ".long " __stringify(val)
+#else
+# define __BUG_REL(val) ".long " __stringify(val) " - 2b"
+#endif
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"aw\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
+ "\t.word %c1" "\t# bug_entry::line\n" \
+ "\t.word %c2" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c3\n" \
+ ".popsection" \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
+} while (0)
+
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
#define _BUG_FLAGS(ins, flags) \
do { \
- asm volatile("ASM_BUG ins=\"" ins "\" file=%c0 line=%c1 " \
- "flags=%c2 size=%c3" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (flags), \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"aw\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t.word %c0" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c1\n" \
+ ".popsection" \
+ : : "i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
+#else
+
+#define _BUG_FLAGS(ins, flags) asm volatile(ins)
+
+#endif /* CONFIG_GENERIC_BUG */
+
#define HAVE_ARCH_BUG
#define BUG() \
do { \
@@ -46,54 +82,4 @@ do { \
#include <asm-generic/bug.h>
-#else /* __ASSEMBLY__ */
-
-#ifdef CONFIG_GENERIC_BUG
-
-#ifdef CONFIG_X86_32
-.macro __BUG_REL val:req
- .long \val
-.endm
-#else
-.macro __BUG_REL val:req
- .long \val - 2b
-.endm
-#endif
-
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
-1: \ins
- .pushsection __bug_table,"aw"
-2: __BUG_REL val=1b # bug_entry::bug_addr
- __BUG_REL val=\file # bug_entry::file
- .word \line # bug_entry::line
- .word \flags # bug_entry::flags
- .org 2b+\size
- .popsection
-.endm
-
-#else /* !CONFIG_DEBUG_BUGVERBOSE */
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
-1: \ins
- .pushsection __bug_table,"aw"
-2: __BUG_REL val=1b # bug_entry::bug_addr
- .word \flags # bug_entry::flags
- .org 2b+\size
- .popsection
-.endm
-
-#endif /* CONFIG_DEBUG_BUGVERBOSE */
-
-#else /* CONFIG_GENERIC_BUG */
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
- \ins
-.endm
-
-#endif /* CONFIG_GENERIC_BUG */
-
-#endif /* __ASSEMBLY__ */
-
#endif /* _ASM_X86_BUG_H */
diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S
index 66ccb8e..852487a 100644
--- a/arch/x86/kernel/macros.S
+++ b/arch/x86/kernel/macros.S
@@ -9,4 +9,3 @@
#include <linux/compiler.h>
#include <asm/refcount.h>
#include <asm/alternative-asm.h>
-#include <asm/bug.h>
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index cdafa5e..20561a6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -17,8 +17,10 @@
#ifndef __ASSEMBLY__
#include <linux/kernel.h>
-struct bug_entry {
+#ifdef CONFIG_BUG
+
#ifdef CONFIG_GENERIC_BUG
+struct bug_entry {
#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
unsigned long bug_addr;
#else
@@ -33,10 +35,8 @@ struct bug_entry {
unsigned short line;
#endif
unsigned short flags;
-#endif /* CONFIG_GENERIC_BUG */
};
-
-#ifdef CONFIG_BUG
+#endif /* CONFIG_GENERIC_BUG */
/*
* Don't use BUG() or BUG_ON() unless there's really no way out; one
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs"
2018-12-17 16:03 ` [PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs" Masahiro Yamada
@ 2018-12-17 16:03 ` Masahiro Yamada
0 siblings, 0 replies; 12+ 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, linux-arch, Arnd Bergmann, linux-kernel,
Nadav Amit
This reverts commit f81f8ad56fd1c7b99b2ed1c314527f7d9ac447c6.
The in-kernel workarounds will be replaced with GCC's new
"asm inline" syntax.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
arch/x86/include/asm/bug.h | 98 ++++++++++++++++++++--------------------------
arch/x86/kernel/macros.S | 1 -
include/asm-generic/bug.h | 8 ++--
3 files changed, 46 insertions(+), 61 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 5090035..6804d66 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -4,8 +4,6 @@
#include <linux/stringify.h>
-#ifndef __ASSEMBLY__
-
/*
* Despite that some emulators terminate on UD2, we use it for WARN().
*
@@ -22,15 +20,53 @@
#define LEN_UD2 2
+#ifdef CONFIG_GENERIC_BUG
+
+#ifdef CONFIG_X86_32
+# define __BUG_REL(val) ".long " __stringify(val)
+#else
+# define __BUG_REL(val) ".long " __stringify(val) " - 2b"
+#endif
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags) \
+do { \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"aw\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
+ "\t.word %c1" "\t# bug_entry::line\n" \
+ "\t.word %c2" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c3\n" \
+ ".popsection" \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
+} while (0)
+
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
#define _BUG_FLAGS(ins, flags) \
do { \
- asm volatile("ASM_BUG ins=\"" ins "\" file=%c0 line=%c1 " \
- "flags=%c2 size=%c3" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (flags), \
+ asm volatile("1:\t" ins "\n" \
+ ".pushsection __bug_table,\"aw\"\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t.word %c0" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c1\n" \
+ ".popsection" \
+ : : "i" (flags), \
"i" (sizeof(struct bug_entry))); \
} while (0)
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
+#else
+
+#define _BUG_FLAGS(ins, flags) asm volatile(ins)
+
+#endif /* CONFIG_GENERIC_BUG */
+
#define HAVE_ARCH_BUG
#define BUG() \
do { \
@@ -46,54 +82,4 @@ do { \
#include <asm-generic/bug.h>
-#else /* __ASSEMBLY__ */
-
-#ifdef CONFIG_GENERIC_BUG
-
-#ifdef CONFIG_X86_32
-.macro __BUG_REL val:req
- .long \val
-.endm
-#else
-.macro __BUG_REL val:req
- .long \val - 2b
-.endm
-#endif
-
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
-1: \ins
- .pushsection __bug_table,"aw"
-2: __BUG_REL val=1b # bug_entry::bug_addr
- __BUG_REL val=\file # bug_entry::file
- .word \line # bug_entry::line
- .word \flags # bug_entry::flags
- .org 2b+\size
- .popsection
-.endm
-
-#else /* !CONFIG_DEBUG_BUGVERBOSE */
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
-1: \ins
- .pushsection __bug_table,"aw"
-2: __BUG_REL val=1b # bug_entry::bug_addr
- .word \flags # bug_entry::flags
- .org 2b+\size
- .popsection
-.endm
-
-#endif /* CONFIG_DEBUG_BUGVERBOSE */
-
-#else /* CONFIG_GENERIC_BUG */
-
-.macro ASM_BUG ins:req file:req line:req flags:req size:req
- \ins
-.endm
-
-#endif /* CONFIG_GENERIC_BUG */
-
-#endif /* __ASSEMBLY__ */
-
#endif /* _ASM_X86_BUG_H */
diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S
index 66ccb8e..852487a 100644
--- a/arch/x86/kernel/macros.S
+++ b/arch/x86/kernel/macros.S
@@ -9,4 +9,3 @@
#include <linux/compiler.h>
#include <asm/refcount.h>
#include <asm/alternative-asm.h>
-#include <asm/bug.h>
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index cdafa5e..20561a6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -17,8 +17,10 @@
#ifndef __ASSEMBLY__
#include <linux/kernel.h>
-struct bug_entry {
+#ifdef CONFIG_BUG
+
#ifdef CONFIG_GENERIC_BUG
+struct bug_entry {
#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
unsigned long bug_addr;
#else
@@ -33,10 +35,8 @@ struct bug_entry {
unsigned short line;
#endif
unsigned short flags;
-#endif /* CONFIG_GENERIC_BUG */
};
-
-#ifdef CONFIG_BUG
+#endif /* CONFIG_GENERIC_BUG */
/*
* Don't use BUG() or BUG_ON() unless there's really no way out; one
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ 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 ` Masahiro Yamada
2018-12-17 16:03 ` [PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs" Masahiro Yamada
@ 2018-12-18 19:43 ` Nadav Amit
2018-12-18 19:43 ` Nadav Amit
2018-12-19 3:19 ` Masahiro Yamada
2018-12-19 11:20 ` Ingo Molnar
3 siblings, 2 replies; 12+ 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,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn
> 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] 12+ 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-18 19:43 ` Nadav Amit
2018-12-19 3:19 ` Masahiro Yamada
1 sibling, 0 replies; 12+ 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,
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, David Woodhouse,
Alexei Starovoitov, linux-kernel
> 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] 12+ 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-18 19:43 ` Nadav Amit
@ 2018-12-19 3:19 ` Masahiro Yamada
2018-12-19 3:19 ` Masahiro Yamada
1 sibling, 1 reply; 12+ 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,
Luc Van Oostenryck, Alok Kataria, Ard Biesheuvel, Jann Horn,
linux-arch, Alexey Dobriyan, linux-sparse, Andrew
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] 12+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-19 3:19 ` Masahiro Yamada
@ 2018-12-19 3:19 ` Masahiro Yamada
0 siblings, 0 replies; 12+ 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,
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, David Woodhouse,
Alexei Starovoitov, linux-kernel
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] 12+ 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
` (2 preceding siblings ...)
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 11:20 ` Ingo Molnar
2018-12-19 14:33 ` Masahiro Yamada
3 siblings, 2 replies; 12+ 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
* 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] 12+ 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 11:20 ` Ingo Molnar
2018-12-19 14:33 ` Masahiro Yamada
1 sibling, 0 replies; 12+ 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] 12+ 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 11:20 ` Ingo Molnar
@ 2018-12-19 14:33 ` Masahiro Yamada
2018-12-19 14:33 ` Masahiro Yamada
1 sibling, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2018-12-19 14:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: Peter Zijlstra, Yonghong Song, Alexei Starovoitov,
virtualization, Nadav Amit, Jan Beulich, H . Peter Anvin,
Alok Kataria, linux-arch, Arnd Bergmann, X86 ML, linux-sparse,
Ingo Molnar, Andrey Ryabinin, Alexey Dobriyan, Kees Cook,
Segher Boessenkool, Jann Horn, Arnaldo Carvalho de Melo,
Borislav Petkov, Josh Poimboeuf, Thomas Gleixner,
Linux Kbuild mailing list, Juergen Gross
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] 12+ messages in thread
* Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code
2018-12-19 14:33 ` Masahiro Yamada
@ 2018-12-19 14:33 ` Masahiro Yamada
0 siblings, 0 replies; 12+ 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] 12+ messages in thread