[PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Nadav Amit]

Using macros for inline assembly improves both readability and
compilation decisions that are distorted by big assembly blocks that use
alternative sections. Compile macros.S and use it to assemble all C
files. Currently, only x86 will use it.

Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-kbuild@vger.kernel.org
Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 Makefile                 |  9 +++++++--
 arch/x86/Makefile        | 11 +++++++++--
 arch/x86/kernel/macros.S |  7 +++++++
 scripts/Kbuild.include   |  4 +++-
 scripts/mod/Makefile     |  2 ++
 5 files changed, 28 insertions(+), 5 deletions(-)
 create mode 100644 arch/x86/kernel/macros.S

diff --git a/Makefile b/Makefile
index f03a1e062503..af5dba613533 100644
--- a/Makefile
+++ b/Makefile
@@ -1071,7 +1071,7 @@ scripts: scripts_basic asm-generic gcc-plugins $(autoksyms_h)
 # version.h and scripts_basic is processed / created.
 
 # Listed in dependency order
-PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
+PHONY += prepare archprepare macroprepare prepare0 prepare1 prepare2 prepare3
 
 # prepare3 is used to check if we are building in a separate output directory,
 # and if so do:
@@ -1094,7 +1094,9 @@ prepare2: prepare3 outputmakefile asm-generic
 prepare1: prepare2 $(version_h) $(autoksyms_h) include/generated/utsrelease.h
 	$(cmd_crmodverdir)
 
-archprepare: archheaders archscripts prepare1 scripts_basic
+macroprepare: prepare1 archmacros
+
+archprepare: archheaders archscripts macroprepare scripts_basic
 
 prepare0: archprepare gcc-plugins
 	$(Q)$(MAKE) $(build)=.
@@ -1162,6 +1164,9 @@ archheaders:
 PHONY += archscripts
 archscripts:
 
+PHONY += archmacros
+archmacros:
+
 PHONY += __headers
 __headers: $(version_h) scripts_basic uapi-asm-generic archheaders archscripts
 	$(Q)$(MAKE) $(build)=scripts build_unifdef
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 8f6e7eb8ae9f..944fa3bc9376 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
 KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
 endif
 
-# Speed up the build
-KBUILD_CFLAGS += -pipe
+# We cannot use -pipe flag since we give an additional .s file to the compiler
+#KBUILD_CFLAGS += -pipe
 # Workaround for a gcc prelease that unfortunately was shipped in a suse release
 KBUILD_CFLAGS += -Wno-sign-compare
 #
@@ -237,6 +237,13 @@ archscripts: scripts_basic
 archheaders:
 	$(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all
 
+archmacros:
+	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
+
+ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
+export ASM_MACRO_FLAGS
+KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
+
 ###
 # Kernel objects
 
diff --git a/arch/x86/kernel/macros.S b/arch/x86/kernel/macros.S
new file mode 100644
index 000000000000..cfc1c7d1a6eb
--- /dev/null
+++ b/arch/x86/kernel/macros.S
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file includes headers whose assembly part includes macros which are
+ * commonly used. The macros are precompiled into assmebly file which is later
+ * assembled together with each compiled file.
+ */
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index ce53639a864a..8aeb60eb6ee3 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -115,7 +115,9 @@ __cc-option = $(call try-run,\
 
 # Do not attempt to build with gcc plugins during cc-option tests.
 # (And this uses delayed resolution so the flags will be up to date.)
-CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
+# In addition, do not include the asm macros which are built later.
+CC_OPTION_FILTERED = $(GCC_PLUGINS_CFLAGS) $(ASM_MACRO_FLAGS)
+CC_OPTION_CFLAGS = $(filter-out $(CC_OPTION_FILTERED),$(KBUILD_CFLAGS))
 
 # cc-option
 # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index 42c5d50f2bcc..a5b4af47987a 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -4,6 +4,8 @@ OBJECT_FILES_NON_STANDARD := y
 hostprogs-y	:= modpost mk_elfconfig
 always		:= $(hostprogs-y) empty.o
 
+CFLAGS_REMOVE_empty.o := $(ASM_MACRO_FLAGS)
+
 modpost-objs	:= modpost.o file2alias.o sumversion.o
 
 devicetable-offsets-file := devicetable-offsets.h
-- 
2.17.1

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Rasmus Villemoes]

On 2018-09-18 23:28, Nadav Amit wrote:

> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 8f6e7eb8ae9f..944fa3bc9376 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
>  KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
>  endif
>  
> -# Speed up the build
> -KBUILD_CFLAGS += -pipe
> +# We cannot use -pipe flag since we give an additional .s file to the compiler
> +#KBUILD_CFLAGS += -pipe

Is this really necessary? The gas manual says that one can use -- to
name stdin, though that's probably a typo and should just be - . Doing

gcc -pipe -Wa,foo.s -Wa,-

does seem to work as expected (and would also make it possible to append
some .s file should that ever be required).
>  
> +archmacros:
> +	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
> +
> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
> +export ASM_MACRO_FLAGS
> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
How does this affect what gets rebuilt when one of the asm/foo.h files
going into macros.s changes? Does that cause a global rebuild because
everything depends on macros.s, or do we still only rebuild the files
that actually include asm/foo.h?

Rasmus

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Nadav Amit]

at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 2018-09-18 23:28, Nadav Amit wrote:
> 
>> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
>> index 8f6e7eb8ae9f..944fa3bc9376 100644
>> --- a/arch/x86/Makefile
>> +++ b/arch/x86/Makefile
>> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
>> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
>> endif
>> 
>> -# Speed up the build
>> -KBUILD_CFLAGS += -pipe
>> +# We cannot use -pipe flag since we give an additional .s file to the compiler
>> +#KBUILD_CFLAGS += -pipe
> 
> Is this really necessary? The gas manual says that one can use -- to
> name stdin, though that's probably a typo and should just be - . Doing
> 
> gcc -pipe -Wa,foo.s -Wa,-

Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in
v9.

> does seem to work as expected (and would also make it possible to append
> some .s file should that ever be required).
>> +archmacros:
>> +	$(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
>> +
>> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
>> +export ASM_MACRO_FLAGS
>> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
> How does this affect what gets rebuilt when one of the asm/foo.h files
> going into macros.s changes? Does that cause a global rebuild because
> everything depends on macros.s, or do we still only rebuild the files
> that actually include asm/foo.h?

There will not be a global rebuild. Any source file that uses the header
files that are included in macros.S will be rebuilt.

But your question actually raises two issues:

1. If macros.S changes, there *should* be a global rebuild, and currently
there wouldn’t be one. Do you happen to know what would be the appropriate
way to trigger one?

2. Someone might mistakenly use the macros through inline assembly without
including the header file. It would be better to detect such cases and fail
the build. I may be able to create another asm macro in the C part of the
header that would cause the build to fail. But let me know if you any
better idea.

Regards,
Nadav

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Rasmus Villemoes]

On 2018-09-26 19:56, Nadav Amit wrote:
> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>>> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
>>> +export ASM_MACRO_FLAGS
>>> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
>> How does this affect what gets rebuilt when one of the asm/foo.h files
>> going into macros.s changes? Does that cause a global rebuild because
>> everything depends on macros.s, or do we still only rebuild the files
>> that actually include asm/foo.h?
> 
> There will not be a global rebuild. Any source file that uses the header
> files that are included in macros.S will be rebuilt.
> 
> But your question actually raises two issues:
> 
> 1. If macros.S changes, there *should* be a global rebuild, and currently
> there wouldn’t be one. Do you happen to know what would be the appropriate
> way to trigger one?

Hm, that's a question for Kbuild folks, I think. Maybe one could just
ensure macros.s gets written into the dependency file for each
translation unit (maybe fixdep already has support for that). But if we
do that, we probably want macros.s to be generated with some if_changed
makefile magic, so that changes in one of the asm/ headers that do not
actually change macros.s does not cause a global rebuild (it would still
rebuild the files that actually inclue that asm/ header, of course).

> 2. Someone might mistakenly use the macros through inline assembly without
> including the header file.

That's rather unlikely, I think. An open-coded asm("SOME_MACRO ...")
would hopefully be caught in review. But yeah...

> It would be better to detect such cases and fail
> the build. I may be able to create another asm macro in the C part of the
> header that would cause the build to fail. But let me know if you any
> better idea.

The best I can think of (and I'm not suggesting to do this, or that it
would actually work): In asm/foo.h, inside its cpp include guard, and
inside a guard that excludes this piece when building macros.s itself,
have a 'asm("___asm_foo_included__ = 1\n")'. Then inside each asm macro
defined in asm/foo.h, have a ".ifndef ___asm_foo_included___\n.error
...\n.endif". That latter part might be macrofied itself so that it's
just one line, "CHECK_INCLUDE ___asm_foo_included___\n", with
CHECK_INCLUDE being defined in macros.S.

Rasmus

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Masahiro Yamada]

Hi.



2018年9月27日(木) 2:57 Nadav Amit <namit@vmware.com>:
>
> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> > On 2018-09-18 23:28, Nadav Amit wrote:
> >
> >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> >> index 8f6e7eb8ae9f..944fa3bc9376 100644
> >> --- a/arch/x86/Makefile
> >> +++ b/arch/x86/Makefile
> >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
> >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
> >> endif
> >>
> >> -# Speed up the build
> >> -KBUILD_CFLAGS += -pipe
> >> +# We cannot use -pipe flag since we give an additional .s file to the compiler
> >> +#KBUILD_CFLAGS += -pipe
> >
> > Is this really necessary? The gas manual says that one can use -- to
> > name stdin, though that's probably a typo and should just be - . Doing
> >
> > gcc -pipe -Wa,foo.s -Wa,-
>
> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in
> v9.
>
> > does seem to work as expected (and would also make it possible to append
> > some .s file should that ever be required).
> >> +archmacros:
> >> +    $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
> >> +
> >> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
> >> +export ASM_MACRO_FLAGS
> >> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
> > How does this affect what gets rebuilt when one of the asm/foo.h files
> > going into macros.s changes? Does that cause a global rebuild because
> > everything depends on macros.s, or do we still only rebuild the files
> > that actually include asm/foo.h?
>
> There will not be a global rebuild. Any source file that uses the header
> files that are included in macros.S will be rebuilt.
>
> But your question actually raises two issues:
>
> 1. If macros.S changes, there *should* be a global rebuild,


Looking at this series, I can see this rule:
"macros and inline functions that calls them are placed in the same header".


For example,
REFCOUNT_CHECK_LE_ZERO is defined in <asm/refcount.h>,
and REFCOUNT_CHECK_LE_ZERO is invoked via refcount_sub_and_test() etc.
in the same header.

Therefore, all users of REFCOUNT_CHECK_LE_ZERO must have included
<asm/refcount.h>

This means all C files using REFCOUNT_CHECK_LE_ZERO
will be appropriately recompiled as long as the rule above is observed.



> and currently
> there wouldn’t be one. Do you happen to know what would be the appropriate
> way to trigger one?
>
> 2. Someone might mistakenly use the macros through inline assembly without
> including the header file.

Right, it is possible to use REFCOUNT_CHECK_LE_ZERO directly.
If this happens, my assumption breaks.

It would be unlikely to happen, though...


> It would be better to detect such cases and fail
> the build. I may be able to create another asm macro in the C part of the
> header that would cause the build to fail. But let me know if you any
> better idea.
>
> Regards,
> Nadav
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Ingo Molnar]

* Nadav Amit <namit@vmware.com> wrote:

> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > On 2018-09-18 23:28, Nadav Amit wrote:
> > 
> >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> >> index 8f6e7eb8ae9f..944fa3bc9376 100644
> >> --- a/arch/x86/Makefile
> >> +++ b/arch/x86/Makefile
> >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
> >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
> >> endif
> >> 
> >> -# Speed up the build
> >> -KBUILD_CFLAGS += -pipe
> >> +# We cannot use -pipe flag since we give an additional .s file to the compiler
> >> +#KBUILD_CFLAGS += -pipe
> > 
> > Is this really necessary? The gas manual says that one can use -- to
> > name stdin, though that's probably a typo and should just be - . Doing
> > 
> > gcc -pipe -Wa,foo.s -Wa,-
> 
> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in
> v9.

Ok, will wait for v9.

Thanks,

	Ingo

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Nadav Amit]

at 3:01 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi.

Thanks for your reply. Your advices are always valuable!

> 
> 
> 
> 2018年9月27日(木) 2:57 Nadav Amit <namit@vmware.com>:
>> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>> 
>>> On 2018-09-18 23:28, Nadav Amit wrote:
>>> 
>>>> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
>>>> index 8f6e7eb8ae9f..944fa3bc9376 100644
>>>> --- a/arch/x86/Makefile
>>>> +++ b/arch/x86/Makefile
>>>> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
>>>> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
>>>> endif
>>>> 
>>>> -# Speed up the build
>>>> -KBUILD_CFLAGS += -pipe
>>>> +# We cannot use -pipe flag since we give an additional .s file to the compiler
>>>> +#KBUILD_CFLAGS += -pipe
>>> 
>>> Is this really necessary? The gas manual says that one can use -- to
>>> name stdin, though that's probably a typo and should just be - . Doing
>>> 
>>> gcc -pipe -Wa,foo.s -Wa,-
>> 
>> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do it in
>> v9.
>> 
>>> does seem to work as expected (and would also make it possible to append
>>> some .s file should that ever be required).
>>>> +archmacros:
>>>> +    $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s
>>>> +
>>>> +ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s
>>>> +export ASM_MACRO_FLAGS
>>>> +KBUILD_CFLAGS += $(ASM_MACRO_FLAGS)
>>> How does this affect what gets rebuilt when one of the asm/foo.h files
>>> going into macros.s changes? Does that cause a global rebuild because
>>> everything depends on macros.s, or do we still only rebuild the files
>>> that actually include asm/foo.h?
>> 
>> There will not be a global rebuild. Any source file that uses the header
>> files that are included in macros.S will be rebuilt.
>> 
>> But your question actually raises two issues:
>> 
>> 1. If macros.S changes, there *should* be a global rebuild,
> 
> 
> Looking at this series, I can see this rule:
> "macros and inline functions that calls them are placed in the same header".
> 
> 
> For example,
> REFCOUNT_CHECK_LE_ZERO is defined in <asm/refcount.h>,
> and REFCOUNT_CHECK_LE_ZERO is invoked via refcount_sub_and_test() etc.
> in the same header.
> 
> Therefore, all users of REFCOUNT_CHECK_LE_ZERO must have included
> <asm/refcount.h>
> 
> This means all C files using REFCOUNT_CHECK_LE_ZERO
> will be appropriately recompiled as long as the rule above is observed.

Yes. My concern is not what happens if any of the files included from
macros.S changes, but what happens if macros.S changes (e.g., to include
an additional header).

Maybe I can create some artificial dependency based on __ASSEMBLY__ . I’ll
give it another thought.

>> and currently
>> there wouldn’t be one. Do you happen to know what would be the appropriate
>> way to trigger one?
>> 
>> 2. Someone might mistakenly use the macros through inline assembly without
>> including the header file.
> 
> Right, it is possible to use REFCOUNT_CHECK_LE_ZERO directly.
> If this happens, my assumption breaks.
> 
> It would be unlikely to happen, though...

Yes. I’ll let it go for now.

Thanks,
Nadav

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[hpa]

On October 2, 2018 3:59:52 AM PDT, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Nadav Amit <namit@vmware.com> wrote:
>
>> at 1:58 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>> 
>> > On 2018-09-18 23:28, Nadav Amit wrote:
>> > 
>> >> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
>> >> index 8f6e7eb8ae9f..944fa3bc9376 100644
>> >> --- a/arch/x86/Makefile
>> >> +++ b/arch/x86/Makefile
>> >> @@ -214,8 +214,8 @@ ifdef CONFIG_X86_64
>> >> KBUILD_LDFLAGS += $(call ld-option, -z max-page-size=0x200000)
>> >> endif
>> >> 
>> >> -# Speed up the build
>> >> -KBUILD_CFLAGS += -pipe
>> >> +# We cannot use -pipe flag since we give an additional .s file to
>the compiler
>> >> +#KBUILD_CFLAGS += -pipe
>> > 
>> > Is this really necessary? The gas manual says that one can use --
>to
>> > name stdin, though that's probably a typo and should just be - .
>Doing
>> > 
>> > gcc -pipe -Wa,foo.s -Wa,-
>> 
>> Good idea. I didn’t think of it. Yes, this can do the trick. I’ll do
>it in
>> v9.
>
>Ok, will wait for v9.
>
>Thanks,
>
>	Ingo

Does -pipe actually win anything these days?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v8 02/10] Makefile: Prepare for using macros for inline asm

[Nadav Amit]

at 12:43 PM, hpa@zytor.com wrote:

> 
> Does -pipe actually win anything these days?

Sorry, I don’t have the time to check it right now. Anyhow, I presume it is
best that such a change will be in a separate patch.