[PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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

[PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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

[PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs"

[Masahiro Yamada]

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

[PATCH v3 05/12] Revert "x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs"

[Masahiro Yamada]

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

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Nadav Amit]

> 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&amp;data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&amp;sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&amp;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.

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Nadav Amit]

> 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&amp;data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&amp;sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&amp;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.

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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&amp;data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&amp;sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&amp;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

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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&amp;data=02%7C01%7Cnamit%40vmware.com%7Cc43f433d8c6244de15f108d6643a49e4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636806598899962669&amp;sdata=88UJ25RoiHik9vTCJKZV6%2F7xpzCGsvKb9LFg1kfEYL0%3D&amp;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

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Ingo Molnar]

* 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
 

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Ingo Molnar]

* 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
 

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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

Re: [PATCH v3 00/12] x86, kbuild: revert macrofying inline assembly code

[Masahiro Yamada]

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