Re: Building arm64 EFI stub with -fpie breaks build of 4.9.x (undefined reference to `__efistub__GLOBAL_OFFSET_TABLE_')
On Wed, Jun 5, 2019 at 11:42 AM Ard Biesheuvel wrote: > For the record, this is an example of why I think backporting those > clang enablement patches is a bad idea. There's always a risk involved with backports of any kind; more CI coverage can help us mitigate some of these risks in an automated fashion before we get user reports like this. I meet with the KernelCI folks weekly, so I'll double check on the coverage of the stable tree's branches. The 0day folks are also very responsive and I've spoken with them a few times, so I'll try to get to the bottom of why this wasn't reported by either of those. Also, these patches help keep Android, CrOS, and Google internal production kernels closer to their upstream sources. > We can't actually build those > kernels with clang, can we? So what is the point? Here's last night's build: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/114388434 Also, Android and CrOS have shipped X million devices w/ 4.9 kernels built with Clang. I think this number will grow at least one order of magnitude imminently. > Alternatively, we can just revert this patch from 4.9 That would break at least the above devices next time Android and CrOS pulled from stable. > It would be helpful to get a relocation dump (objdump -r) of > arm64-stub.o to figure out which symbol needs a 'hidden' annotation to > prevent GCC from emitting it as a PIC reference requiring a GOT. Sounds like the best way forward, as well as having more info on which config/toolchain reliably reproduces the issue. -- Thanks, ~Nick Desaulniers
Re: Building arm64 EFI stub with -fpie breaks build of 4.9.x (undefined reference to `__efistub__GLOBAL_OFFSET_TABLE_')
On Wed, Jun 5, 2019 at 10:27 AM Nick Desaulniers
wrote:
>
> On Wed, Jun 5, 2019 at 9:26 AM Greg KH wrote:
> >
> > On Wed, Jun 05, 2019 at 05:19:40PM +0200, Rolf Eike Beer wrote:
> > > I decided to dig out a toy project which uses a DragonBoard 410c. This has
> > > been "running" with kernel 4.9, which I would keep this way for unrelated
> > > reasons. The vanilla 4.9 kernel wasn't bootable back then, but it was
> > > buildable, which was good enough.
> > >
> > > Upgrading the kernel to 4.9.180 caused the boot to suddenly fail:
> > >
> > > aarch64-unknown-linux-gnueabi-ld:
> > > ./drivers/firmware/efi/libstub/lib.a(arm64-
> > > stub.stub.o): in function `handle_kernel_image':
> > > /tmp/e2/build/linux-4.9.139/drivers/firmware/efi/libstub/arm64-stub.c:63:
> > > undefined reference to `__efistub__GLOBAL_OFFSET_TABLE_'
> > > aarch64-unknown-linux-gnueabi-ld:
> > > ./drivers/firmware/efi/libstub/lib.a(arm64-
> > > stub.stub.o): relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol
> > > `__efistub__GLOBAL_OFFSET_TABLE_' which may bind externally can not be
> > > used
> > > when making a shared object; recompile with -fPIC
> > > /tmp/e2/build/linux-4.9.139/drivers/firmware/efi/libstub/arm64-stub.c:63:
> > > (.init.text+0xc): dangerous relocation: unsupported relocation
> > > /tmp/e2/build/linux-4.9.139/Makefile:1001: recipe for target 'vmlinux'
> > > failed
> > > -make[1]: *** [vmlinux] Error 1
> > >
> > > This is caused by commit 27b5ebf61818749b3568354c64a8ec2d9cd5ecca from
> > > linux-4.9.y (which is 91ee5b21ee026c49e4e7483de69b55b8b47042be), reverting
> > > this commit fixes the build.
> > >
> > > This happens with vanilla binutils 2.32 and gcc 8.3.0 as well as 9.1.0.
> > > See
> > > the attached .config for reference.
> > >
> > > If you have questions or patches just ping me.
> >
> > Does Linus's latest tree also fail for you (or 5.1)?
> >
> > Nick, do we need to add another fix that is in mainline for this to work
> > properly?
> >
> > thanks,
> >
> > greg k-h
>
> Doesn't immediately ring any bells for me.
Upstream commits:
dd6846d77469 ("arm64: drop linker script hack to hide __efistub_ symbols")
1212f7a16af4 ("scripts/kallsyms: filter arm64's __efistub_ symbols")
Look related to __efistub__ prefixes on symbols and aren't in stable
4.9 (maybe Rolf can try cherry picks of those).
--
Thanks,
~Nick Desaulniers
Re: Building arm64 EFI stub with -fpie breaks build of 4.9.x (undefined reference to `__efistub__GLOBAL_OFFSET_TABLE_')
On Wed, Jun 5, 2019 at 9:26 AM Greg KH wrote: > > On Wed, Jun 05, 2019 at 05:19:40PM +0200, Rolf Eike Beer wrote: > > I decided to dig out a toy project which uses a DragonBoard 410c. This has > > been "running" with kernel 4.9, which I would keep this way for unrelated > > reasons. The vanilla 4.9 kernel wasn't bootable back then, but it was > > buildable, which was good enough. > > > > Upgrading the kernel to 4.9.180 caused the boot to suddenly fail: > > > > aarch64-unknown-linux-gnueabi-ld: > > ./drivers/firmware/efi/libstub/lib.a(arm64- > > stub.stub.o): in function `handle_kernel_image': > > /tmp/e2/build/linux-4.9.139/drivers/firmware/efi/libstub/arm64-stub.c:63: > > undefined reference to `__efistub__GLOBAL_OFFSET_TABLE_' > > aarch64-unknown-linux-gnueabi-ld: > > ./drivers/firmware/efi/libstub/lib.a(arm64- > > stub.stub.o): relocation R_AARCH64_ADR_PREL_PG_HI21 against symbol > > `__efistub__GLOBAL_OFFSET_TABLE_' which may bind externally can not be used > > when making a shared object; recompile with -fPIC > > /tmp/e2/build/linux-4.9.139/drivers/firmware/efi/libstub/arm64-stub.c:63: > > (.init.text+0xc): dangerous relocation: unsupported relocation > > /tmp/e2/build/linux-4.9.139/Makefile:1001: recipe for target 'vmlinux' > > failed > > -make[1]: *** [vmlinux] Error 1 > > > > This is caused by commit 27b5ebf61818749b3568354c64a8ec2d9cd5ecca from > > linux-4.9.y (which is 91ee5b21ee026c49e4e7483de69b55b8b47042be), reverting > > this commit fixes the build. > > > > This happens with vanilla binutils 2.32 and gcc 8.3.0 as well as 9.1.0. See > > the attached .config for reference. > > > > If you have questions or patches just ping me. > > Does Linus's latest tree also fail for you (or 5.1)? > > Nick, do we need to add another fix that is in mainline for this to work > properly? > > thanks, > > greg k-h Doesn't immediately ring any bells for me. +mka@ who helped test 91ee5b21ee026c49e4e7483de69b55b8b47042be. Nothing in that series (https://lore.kernel.org/lkml/20170818194947.19347-5-ard.biesheu...@linaro.org/T/#u) is immediately obvious. Rolf, can you please email me your config so I can see if I can reproduce? Also, which version of GCC are you using, and binutils? (would be good to know if you hit this in mainline too, as if not maybe there's an existing fix to be backported to stable). -- Thanks, ~Nick Desaulniers
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Tue, Nov 6, 2018 at 6:16 AM Ard Biesheuvel wrote: > > On 6 November 2018 at 14:15, Sedat Dilek wrote: > > > > Thanks Ard. > > This means efi-for-4.21 / efi-for-5.0? > > > > Yes Thanks for picking up this patch, Ard. Is there any way to get it into 4.20? -- Thanks, ~Nick Desaulniers
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Mon, Oct 15, 2018 at 12:46 PM Nathan Chancellor
wrote:
>
> On Mon, Oct 15, 2018 at 12:40:36PM -0700, Nick Desaulniers wrote:
> > On Fri, Oct 12, 2018 at 6:06 PM Nathan Chancellor
> > wrote:
> > >
> > > When building the kernel with Clang, some disabled warnings appear
> > > because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
> > > this list so that the build is clean again.
> > >
> > > -Wpointer-sign was disabled for the whole kernel before the beginning
> > > of git history.
> > >
> > > -Waddress-of-packed-member was disabled for the whole kernel in
> > > commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
> > > warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
> > > Disable the address-of-packed-member compiler warning").
> > >
> > > -Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
> > > LLVMLinux: Add Kbuild support for building kernel with Clang") and for
> > > x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
> > > warnings about GNU extensions").
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/112
> > > Signed-off-by: Nathan Chancellor
> > > ---
> > >
> > > Nick expressed concern that this Makefile is overwriting KBUILD_CFLAGS
> > > and suggested potentially rewriting the x86 portion of this Makefile to
> > > behave like the arm/arm64 one where problematic flags are filtered out.
> > > While that comes to fruition, it would be nice for this folder to behave
> > > like the rest of the kernel when it comes to this warnings so that the
> > > build is cleaner, thus this patch.
> > >
> > > drivers/firmware/efi/libstub/Makefile | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/Makefile
> > > b/drivers/firmware/efi/libstub/Makefile
> > > index c51627660dbb..d9845099635e 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32):= -march=i386
> > > cflags-$(CONFIG_X86_64):= -mcmodel=small
> > > cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
> > >-fPIC -fno-strict-aliasing
> > > -mno-red-zone \
> > > - -mno-mmx -mno-sse -fshort-wchar
> > > + -mno-mmx -mno-sse -fshort-wchar \
> > > + -Wno-pointer-sign \
> >
> > Hi Nathan, thanks for this patch.
> >
> > Should this be:
> > $(call cc-disable-warning, pointer-sign)
> > as well?
> >
>
> According to commit 1d6bf3a9a546 ("kbuild: add -Wno-pointer-sign flag
> unconditionally") in -next, all supported compilers recognize the flag
> so I don't think it's necessary.
No problem.
Tested-by: Nick Desaulniers
>
> > > + $(call cc-disable-warning,
> > > address-of-packed-member) \
> > > + $(call cc-disable-warning, gnu)
> > >
> > > # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> > > # disable the stackleak plugin
> > > --
> > > 2.19.1
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> Thanks for the review!
> Nathan
--
Thanks,
~Nick Desaulniers
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Fri, Oct 12, 2018 at 6:06 PM Nathan Chancellor
wrote:
>
> When building the kernel with Clang, some disabled warnings appear
> because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
> this list so that the build is clean again.
>
> -Wpointer-sign was disabled for the whole kernel before the beginning
> of git history.
>
> -Waddress-of-packed-member was disabled for the whole kernel in
> commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
> warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
> Disable the address-of-packed-member compiler warning").
>
> -Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
> LLVMLinux: Add Kbuild support for building kernel with Clang") and for
> x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
> warnings about GNU extensions").
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/112
> Signed-off-by: Nathan Chancellor
> ---
>
> Nick expressed concern that this Makefile is overwriting KBUILD_CFLAGS
> and suggested potentially rewriting the x86 portion of this Makefile to
> behave like the arm/arm64 one where problematic flags are filtered out.
> While that comes to fruition, it would be nice for this folder to behave
> like the rest of the kernel when it comes to this warnings so that the
> build is cleaner, thus this patch.
>
> drivers/firmware/efi/libstub/Makefile | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> index c51627660dbb..d9845099635e 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32):= -march=i386
> cflags-$(CONFIG_X86_64):= -mcmodel=small
> cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
>-fPIC -fno-strict-aliasing -mno-red-zone \
> - -mno-mmx -mno-sse -fshort-wchar
> + -mno-mmx -mno-sse -fshort-wchar \
> + -Wno-pointer-sign \
Hi Nathan, thanks for this patch.
Should this be:
$(call cc-disable-warning, pointer-sign)
as well?
> + $(call cc-disable-warning,
> address-of-packed-member) \
> + $(call cc-disable-warning, gnu)
>
> # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> # disable the stackleak plugin
> --
> 2.19.1
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH 4.4 018/107] x86/paravirt: Make native_save_fl() extern inline
On Fri, Aug 24, 2018 at 4:08 PM Ben Hutchings wrote: > > On Mon, 2018-07-23 at 14:41 +0200, Greg Kroah-Hartman wrote: > > 4.4-stable review patch. If anyone has any objections, please let me know. > > > > -- > > > > From: Nick Desaulniers > > > > commit d0a8d9378d16eb3c69bd8e6d23779fbdbee3a8c7 upstream. > > > > native_save_fl() is marked static inline, but by using it as > > a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined. > > > > paravirt's use of native_save_fl() also requires that no GPRs other than > > %rax are clobbered. > [...] > > Shouldn't native_restore_fl() be changed similarly? You added an out- > of-line definition, but it doesn't seem like it will actually be used. > Good catch, will send a patch with your reported by. Thank you for the report. -- Thanks, ~Nick Desaulniers
Re: [PATCH v6 3/3] x86: paravirt: make native_save_fl extern inline
On Fri, Jul 13, 2018 at 3:15 AM David Laight wrote: > > From: Nick Desaulniers > > Sent: 21 June 2018 17:23 > > > > native_save_fl() is marked static inline, but by using it as > > a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined. > > > > paravirt's use of native_save_fl() also requires that no GPRs other than > > %rax are clobbered. > ... > > diff --git a/arch/x86/include/asm/irqflags.h > > b/arch/x86/include/asm/irqflags.h > > index 89f08955fff7..c4fc17220df9 100644 > > --- a/arch/x86/include/asm/irqflags.h > > +++ b/arch/x86/include/asm/irqflags.h > > @@ -13,7 +13,7 @@ > > * Interrupt control: > > */ > > > > -static inline unsigned long native_save_fl(void) > > +extern inline unsigned long native_save_fl(void) > > This is generating a the compilation warning (that we treat as an error): > "no previous prototype for 'native_save_fl". > Fixable by replicating the line with an appended ; Thanks for the report and sorry for breaking things for you. Just curious about more information to try to reproduce the issue to make sure I fix the issue correctly: * What compiler and compiler version are you using? * Are you setting any configs or enabling any warning CFLAGS to see this? * Do you see this warning for other `extern inline` functions? (It seems like the few other ones in the kernel are for non-x86 archs) I would have guessed that extern inline functions with gnu_inline semantics (gnu89 behavior) should not have a previous declaration, but it probably doesn't hurt to add it. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/3] x86: paravirt: make native_save_fl extern inline
On Tue, Jun 26, 2018 at 3:13 AM Ingo Molnar wrote: > Ok! > > Acked-by: Ingo Molnar > > What's the planned upstreaming route for these patches/fixes? While the fix is mainly for paravirt, 2/3 of the patches exclusively touch arch/x86, so I think they should go up in the x86 tree (as opposed to paravirt's), if you would be so kind. hpa mentioned not handling merges in https://lkml.org/lkml/2018/6/14/903, so I reached out to you and tglx. (Also, this is the first series I've ever sent (as opposed to one off commits), so I'm unfamiliar with the protocol for merging series that may be touch more than one subsystem. I assume we don't split up series to take parts in one tree vs another?) -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/3] x86: paravirt: make native_save_fl extern inline
On Thu, Jun 21, 2018 at 7:24 PM Ingo Molnar wrote:
> * Nick Desaulniers wrote:
>
> > native_save_fl() is marked static inline, but by using it as
> > a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
>
> > --- a/arch/x86/include/asm/irqflags.h
> > +++ b/arch/x86/include/asm/irqflags.h
> > @@ -13,7 +13,7 @@
> > * Interrupt control:
> > */
> >
> > -static inline unsigned long native_save_fl(void)
> > +extern inline unsigned long native_save_fl(void)
> > {
> > unsigned long flags;
> >
>
> What's the code generation effect of this on say a defconfig kernel vmlinux
> with
> paravirt enabled?
Starting with this patch set applied:
$ make CC=gcc-8 -j46
$ objdump -d vmlinux | grep native_save_fl --context=3
81059140 :
81059140: 9cpushfq
81059141: 58pop%rax
81059142: c3retq
$ git checkout HEAD~3
$ make CC=gcc-8 -j46
$ objdump -d vmlinux | grep native_save_fl --context=3
81079410 :
81079410: 9cpushfq
81079411: 58pop%rax
81079412: c3retq
Mainly, this is to prevent the compiler from adding a stack protector
to the outlined version, as the stack protector clobbers %rcx, but
paravirt expects %rcx to be preserved. More info can be found:
https://lkml.org/lkml/2018/5/24/1242--
Thanks,
~Nick Desaulniers
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
From: "H. Peter Anvin" i386 and x86-64 uses different registers for arguments; make them available so we don't have to #ifdef in the actual code. Native size and specified size (q, l, w, b) versions are provided. Acked-by: Juergen Gross Reviewed-by: Sedat Dilek Signed-off-by: H. Peter Anvin Signed-off-by: Nick Desaulniers --- arch/x86/include/asm/asm.h | 59 ++ 1 file changed, 59 insertions(+) diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h index 219faaec51df..990770f9e76b 100644 --- a/arch/x86/include/asm/asm.h +++ b/arch/x86/include/asm/asm.h @@ -46,6 +46,65 @@ #define _ASM_SI__ASM_REG(si) #define _ASM_DI__ASM_REG(di) +#ifndef __x86_64__ +/* 32 bit */ + +#define _ASM_ARG1 _ASM_AX +#define _ASM_ARG2 _ASM_DX +#define _ASM_ARG3 _ASM_CX + +#define _ASM_ARG1L eax +#define _ASM_ARG2L edx +#define _ASM_ARG3L ecx + +#define _ASM_ARG1W ax +#define _ASM_ARG2W dx +#define _ASM_ARG3W cx + +#define _ASM_ARG1B al +#define _ASM_ARG2B dl +#define _ASM_ARG3B cl + +#else +/* 64 bit */ + +#define _ASM_ARG1 _ASM_DI +#define _ASM_ARG2 _ASM_SI +#define _ASM_ARG3 _ASM_DX +#define _ASM_ARG4 _ASM_CX +#define _ASM_ARG5 r8 +#define _ASM_ARG6 r9 + +#define _ASM_ARG1Q rdi +#define _ASM_ARG2Q rsi +#define _ASM_ARG3Q rdx +#define _ASM_ARG4Q rcx +#define _ASM_ARG5Q r8 +#define _ASM_ARG6Q r9 + +#define _ASM_ARG1L edi +#define _ASM_ARG2L esi +#define _ASM_ARG3L edx +#define _ASM_ARG4L ecx +#define _ASM_ARG5L r8d +#define _ASM_ARG6L r9d + +#define _ASM_ARG1W di +#define _ASM_ARG2W si +#define _ASM_ARG3W dx +#define _ASM_ARG4W cx +#define _ASM_ARG5W r8w +#define _ASM_ARG6W r9w + +#define _ASM_ARG1B dil +#define _ASM_ARG2B sil +#define _ASM_ARG3B dl +#define _ASM_ARG4B cl +#define _ASM_ARG5B r8b +#define _ASM_ARG6B r9b + +#endif + /* * Macros to generate condition code outputs from inline assembly, * The output operand must be type "bool". -- 2.18.0.rc2.346.g013aa6912e-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
Functions marked extern inline do not emit an externally visible function when the gnu89 C standard is used. Some KBUILD Makefiles overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without an explicit C standard specified, the default is gnu11. Since c99, the semantics of extern inline have changed such that an externally visible function is always emitted. This can lead to multiple definition errors of extern inline functions at link time of compilation units whose build files have removed an explicit C standard compiler flag for users of GCC 5.1+ or Clang. Acked-by: Juergen Gross Signed-off-by: Nick Desaulniers Suggested-by: Arnd Bergmann Suggested-by: H. Peter Anvin Suggested-by: Joe Perches --- include/linux/compiler-gcc.h | 29 ++--- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index b4bf73f5e38f..c7cfca4ba02b 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -65,6 +65,18 @@ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) #endif +/* + * Feature detection for gnu_inline (gnu89 extern inline semantics). Either + * __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, + * and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not + * defined so the gnu89 semantics are the default. + */ +#ifdef __GNUC_STDC_INLINE__ +#define __gnu_inline __attribute__((gnu_inline)) +#else +#define __gnu_inline +#endif + /* * Force always-inline if the user requests it so via the .config, * or if gcc is too old. @@ -72,19 +84,22 @@ * -Wunused-function. This turns out to avoid the need for complex #ifdef * directives. Suppress the warning in clang as well by using "unused" * function attribute, which is redundant but not harmful for gcc. + * Prefer gnu_inline, so that extern inline functions do not emit an + * externally visible function. This makes extern inline behave as per gnu89 + * semantics rather than c99. This prevents multiple symbol definition errors + * of extern inline functions at link time. + * A lot of inline functions can cause havoc with function tracing. */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -#define inline inline __attribute__((always_inline,unused)) notrace -#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace -#define __inline __inline __attribute__((always_inline,unused)) notrace +#define inline \ + inline __attribute__((always_inline, unused)) notrace __gnu_inline #else -/* A lot of inline functions can cause havoc with function tracing */ -#define inline inline __attribute__((unused)) notrace -#define __inline__ __inline__ __attribute__((unused)) notrace -#define __inline __inline __attribute__((unused)) notrace +#define inline inline __attribute__((unused)) notrace __gnu_inline #endif +#define __inline__ inline +#define __inline inline #define __always_inlineinline __attribute__((always_inline)) #define noinline __attribute__((noinline)) -- 2.18.0.rc2.346.g013aa6912e-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 0/3] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Changes since v5: Reworded commits to include Juergen's ack and switch a Suggested-by for Reviewed-by tag. No code changes. Changes since v4: Take Arnd's and hpa's suggestions properly feature detect gnu_inline attribute to support gcc 4.1. Changes since v3: Take Joe's suggestion to hoist __inline__ and __inline out of conditional block. Changes since v2: Take hpa's _ASM_ARG patch into the set in order to simplify cross 32b/64b x86 assembly and rebase my final patch to use it. Apply Sedat's typo fix to commit message and sign off on it. Take Joe's suggestion to simplify __inline__ and __inline. Add Arnd to list of folks who made helpful suggestions. Changes since v1: Prefer gnu_inline function attribute instead of explicitly setting C standard compiler flag in problematic Makefiles. We should instead carefully evaluate if those Makefiles should be overwriting KBUILD_CFLAGS at all. Dropped the previous first two patches and added a new first patch. *** BLURB HERE *** H. Peter Anvin (1): x86/asm: add _ASM_ARG* constants for argument registers to Nick Desaulniers (2): compiler-gcc.h: add gnu_inline to all inline declarations x86: paravirt: make native_save_fl extern inline arch/x86/include/asm/asm.h | 59 + arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile| 1 + arch/x86/kernel/irqflags.S | 26 +++ include/linux/compiler-gcc.h| 29 5 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.18.0.rc2.346.g013aa6912e-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/3] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Acked-by: Juergen Gross
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: Arnd Bergmann
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..ddeeaac8adda
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_ARG1
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.18.0.rc2.346.g013aa6912e-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
On Thu, Jun 14, 2018 at 5:17 PM H. Peter Anvin wrote: > > On 06/14/18 13:59, Nick Desaulniers wrote: > > On Thu, Jun 14, 2018 at 1:48 PM H. Peter Anvin wrote: > >> > >> On 06/13/18 14:05, Nick Desaulniers wrote: > >>> From: "H. Peter Anvin" > >>> > >>> i386 and x86-64 uses different registers for arguments; make them > >>> available so we don't have to #ifdef in the actual code. > >>> > >>> Native size and specified size (q, l, w, b) versions are provided. > >>> > >>> Suggested-by: Sedat Dilek > >>> Signed-off-by: H. Peter Anvin > >>> Signed-off-by: Nick Desaulniers > >> > >> I still object to Suggested-by: here. Sedat did a correction, which is > >> a Reviewed-by:, but unless I'm completely out to sea, there was no > >> suggestion on Sedat's part of this; and I had certainly not seen it when > >> I wrote the code. > > > > I'm fine with changing it from a Suggested-by to a Reviewed-by. Can > > you do that when you apply the set, or should I send a v6? > > > > I'm not handling patch mechanics for x86 at the moment. Hi Ingo and Thomas, Have you had a chance to review this patch series for application? Note that Juergen (the paravirt maintainer) acked the series: https://bugs.llvm.org/show_bug.cgi?id=37880 The only issue being swapping a Suggested-by for a Reviewed-by tag for one commit, as above. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
On Thu, Jun 14, 2018 at 1:48 PM H. Peter Anvin wrote: > > On 06/13/18 14:05, Nick Desaulniers wrote: > > From: "H. Peter Anvin" > > > > i386 and x86-64 uses different registers for arguments; make them > > available so we don't have to #ifdef in the actual code. > > > > Native size and specified size (q, l, w, b) versions are provided. > > > > Suggested-by: Sedat Dilek > > Signed-off-by: H. Peter Anvin > > Signed-off-by: Nick Desaulniers > > I still object to Suggested-by: here. Sedat did a correction, which is > a Reviewed-by:, but unless I'm completely out to sea, there was no > suggestion on Sedat's part of this; and I had certainly not seen it when > I wrote the code. I'm fine with changing it from a Suggested-by to a Reviewed-by. Can you do that when you apply the set, or should I send a v6? -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
Functions marked extern inline do not emit an externally visible function when the gnu89 C standard is used. Some KBUILD Makefiles overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without an explicit C standard specified, the default is gnu11. Since c99, the semantics of extern inline have changed such that an externally visible function is always emitted. This can lead to multiple definition errors of extern inline functions at link time of compilation units whose build files have removed an explicit C standard compiler flag for users of GCC 5.1+ or Clang. Signed-off-by: Nick Desaulniers Suggested-by: Arnd Bergmann Suggested-by: H. Peter Anvin Suggested-by: Joe Perches --- include/linux/compiler-gcc.h | 29 ++--- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index b4bf73f5e38f..c7cfca4ba02b 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -65,6 +65,18 @@ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) #endif +/* + * Feature detection for gnu_inline (gnu89 extern inline semantics). Either + * __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, + * and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not + * defined so the gnu89 semantics are the default. + */ +#ifdef __GNUC_STDC_INLINE__ +#define __gnu_inline __attribute__((gnu_inline)) +#else +#define __gnu_inline +#endif + /* * Force always-inline if the user requests it so via the .config, * or if gcc is too old. @@ -72,19 +84,22 @@ * -Wunused-function. This turns out to avoid the need for complex #ifdef * directives. Suppress the warning in clang as well by using "unused" * function attribute, which is redundant but not harmful for gcc. + * Prefer gnu_inline, so that extern inline functions do not emit an + * externally visible function. This makes extern inline behave as per gnu89 + * semantics rather than c99. This prevents multiple symbol definition errors + * of extern inline functions at link time. + * A lot of inline functions can cause havoc with function tracing. */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -#define inline inline __attribute__((always_inline,unused)) notrace -#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace -#define __inline __inline __attribute__((always_inline,unused)) notrace +#define inline \ + inline __attribute__((always_inline, unused)) notrace __gnu_inline #else -/* A lot of inline functions can cause havoc with function tracing */ -#define inline inline __attribute__((unused)) notrace -#define __inline__ __inline__ __attribute__((unused)) notrace -#define __inline __inline __attribute__((unused)) notrace +#define inline inline __attribute__((unused)) notrace __gnu_inline #endif +#define __inline__ inline +#define __inline inline #define __always_inlineinline __attribute__((always_inline)) #define noinline __attribute__((noinline)) -- 2.18.0.rc1.244.gcf134e6275-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
From: "H. Peter Anvin" i386 and x86-64 uses different registers for arguments; make them available so we don't have to #ifdef in the actual code. Native size and specified size (q, l, w, b) versions are provided. Suggested-by: Sedat Dilek Signed-off-by: H. Peter Anvin Signed-off-by: Nick Desaulniers --- arch/x86/include/asm/asm.h | 59 ++ 1 file changed, 59 insertions(+) diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h index 219faaec51df..990770f9e76b 100644 --- a/arch/x86/include/asm/asm.h +++ b/arch/x86/include/asm/asm.h @@ -46,6 +46,65 @@ #define _ASM_SI__ASM_REG(si) #define _ASM_DI__ASM_REG(di) +#ifndef __x86_64__ +/* 32 bit */ + +#define _ASM_ARG1 _ASM_AX +#define _ASM_ARG2 _ASM_DX +#define _ASM_ARG3 _ASM_CX + +#define _ASM_ARG1L eax +#define _ASM_ARG2L edx +#define _ASM_ARG3L ecx + +#define _ASM_ARG1W ax +#define _ASM_ARG2W dx +#define _ASM_ARG3W cx + +#define _ASM_ARG1B al +#define _ASM_ARG2B dl +#define _ASM_ARG3B cl + +#else +/* 64 bit */ + +#define _ASM_ARG1 _ASM_DI +#define _ASM_ARG2 _ASM_SI +#define _ASM_ARG3 _ASM_DX +#define _ASM_ARG4 _ASM_CX +#define _ASM_ARG5 r8 +#define _ASM_ARG6 r9 + +#define _ASM_ARG1Q rdi +#define _ASM_ARG2Q rsi +#define _ASM_ARG3Q rdx +#define _ASM_ARG4Q rcx +#define _ASM_ARG5Q r8 +#define _ASM_ARG6Q r9 + +#define _ASM_ARG1L edi +#define _ASM_ARG2L esi +#define _ASM_ARG3L edx +#define _ASM_ARG4L ecx +#define _ASM_ARG5L r8d +#define _ASM_ARG6L r9d + +#define _ASM_ARG1W di +#define _ASM_ARG2W si +#define _ASM_ARG3W dx +#define _ASM_ARG4W cx +#define _ASM_ARG5W r8w +#define _ASM_ARG6W r9w + +#define _ASM_ARG1B dil +#define _ASM_ARG2B sil +#define _ASM_ARG3B dl +#define _ASM_ARG4B cl +#define _ASM_ARG5B r8b +#define _ASM_ARG6B r9b + +#endif + /* * Macros to generate condition code outputs from inline assembly, * The output operand must be type "bool". -- 2.18.0.rc1.244.gcf134e6275-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/3] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: Arnd Bergmann
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..ddeeaac8adda
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_ARG1
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.18.0.rc1.244.gcf134e6275-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/3] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Changes since v4: Take Arnd's and hpa's suggestions properly feature detect gnu_inline attribute to support gcc 4.1. Changes since v3: Take Joe's suggestion to hoist __inline__ and __inline out of conditional block. Changes since v2: Take hpa's _ASM_ARG patch into the set in order to simplify cross 32b/64b x86 assembly and rebase my final patch to use it. Apply Sedat's typo fix to commit message and sign off on it. Take Joe's suggestion to simplify __inline__ and __inline. Add Arnd to list of folks who made helpful suggestions. Changes since v1: Prefer gnu_inline function attribute instead of explicitly setting C standard compiler flag in problematic Makefiles. We should instead carefully evaluate if those Makefiles should be overwriting KBUILD_CFLAGS at all. Dropped the previous first two patches and added a new first patch. H. Peter Anvin (1): x86/asm: add _ASM_ARG* constants for argument registers to Nick Desaulniers (2): compiler-gcc.h: add gnu_inline to all inline declarations x86: paravirt: make native_save_fl extern inline arch/x86/include/asm/asm.h | 59 + arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile| 1 + arch/x86/kernel/irqflags.S | 26 +++ include/linux/compiler-gcc.h| 29 5 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.18.0.rc1.244.gcf134e6275-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 12, 2018 at 2:31 PM H. Peter Anvin wrote: > > On 06/12/18 13:19, Nick Desaulniers wrote: > > > > Oh, by [0] __GCC_STDC_INLINE__ is indeed actually the correct symbol > > to check for. Clang does support that, so nothing to fix there. > > > >> By the way, you should check clang against gcc's predefined macros by > >> doing: > >> > >> gcc [options] -x c -Wp,-dM -E /dev/null | sort > >> > >> Options can change the predefined macros substantially, especially the, > >> -std=, arch and -O options. -x c can be replaced with e.g. -x c++, > >> objective-c, assembler-with-cpp etc. > > > > Neat, I'll have to bookmark that incantation. I can s/gcc/clang/ to > > get a similar list (which is how I know it supports > > __GCC_STDC_INLINE__).> > > I bet that if you add -fgnu89-inlines then it *does* define > __GNUC_GNU_INLINE__. Indeed! I see what you mean about [options] changing the predefined symbol list. > > Patch now becomes something like: > > > > #ifdef __GNUC_GNU_INLINE__ > > #define __gnu_inline __attribute__((gnu_inline)) > > #else > > #define __gnu_inline > > #endif > > > > #define inline inline __attribute__((always_inline, unused)) notrace > > __gnu_inline > > ... > > > > Issues with that approach? > > > > s/__GNUC_GNU_INLINE__/__GNUC_STDC_INLINE__/ Oops, sorry, yes good catch. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 12, 2018 at 11:51 AM H. Peter Anvin wrote: > > ,Greg KH > ,jarkko.sakki...@linux.intel.com,jgr...@suse.com,Josh > Poimboeuf ,Matthias Kaehlcke > ,thomas.lenda...@amd.com,Thiebaud Weksteen > ,mj...@google.com,j...@perches.com > From: h...@zytor.com > Message-ID: <191e4ebe-4cb2-4c8b-ab61-689a91ffe...@zytor.com> > > On June 12, 2018 11:33:14 AM PDT, Nick Desaulniers > wrote: > >On Fri, Jun 8, 2018 at 3:04 AM Sedat Dilek > >wrote: > >> > >> On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann wrote: > >> > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers > >> > wrote: > >> >> Functions marked extern inline do not emit an externally visible > >> >> function when the gnu89 C standard is used. Some KBUILD Makefiles > >> >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as > >without > >> >> an explicit C standard specified, the default is gnu11. Since c99, > >the > >> >> semantics of extern inline have changed such that an externally > >visible > >> >> function is always emitted. This can lead to multiple definition > >errors > >> >> of extern inline functions at link time of compilation units whose > >build > >> >> files have removed an explicit C standard compiler flag for users > >of GCC > >> >> 5.1+ or Clang. > >> >> > >> >> Signed-off-by: Nick Desaulniers > >> >> Suggested-by: H. Peter Anvin > >> >> Suggested-by: Joe Perches > >> > > >> > I suspect this will break Geert's gcc-4.1.2, which I think doesn't > >have that > >> > attribute yet (4.1.3 or higher have it according to the > >documentation. > >> > > >> > It wouldn't be hard to work around that if we want to keep that > >version > >> > working, or we could decide that it's time to officially stop > >supporting > >> > that version, but we should probably decide on one or the other. > > > >Heh, so earlier we decided against compiler flags (-std=gnu89 or > >-fgnu89-inline) in preference to function attributes. The function > >attribute is preferable as some of the Makefiles [accidentally?] > >overwrite KBUILD_CFLAGS, which is problematic for gcc 5.1 users as the > >implicit c standard used was changed to gnu11 from gnu89. What's nice > >is that to support gcc 4.1 users, we simply don't need to add any > >attribute, as their implicit c standard is gnu89 which has the > >semantics for extern inline that we want. I have a simple change to > >this patch that can support users of various gcc versions, see below: > > > >> Good point. > >> What is the minimum requirement of GCC version currently? > >> AFAICS x86/asm-goto support requires GCC >= 4.5? > > > >Yes, but that's only for x86, IIUC. It seems the kernel may have > >different minimum required versions of GCC based on arch then? That > >may be ok, but I'm not sure that's easy to keep track of without > >having it explicitly stated somewhere like the docs perhaps? > > > >> Just FYI... > >> ...saw the last days in upstream commits that kbuild/kconfig for > >> 4.18-rc1 offers possibilities to check for cc-version dependencies. > > > >Those will be helpful. If we want to pursue compiler flags, which get > >set some Makefiles, then yes. But I think a simpler change to my > >patch would be as below. > > > >It seems gcc did not get __has_attribute [0] until 5.1, but will > >define __GNUC_GNU_INLINE__ if supported. [1] Unfortunately, Clang > >does not define __GNUC_GNU_INLINE__ [2]. So a proper feature test > >might look like: > > > >``` > >#ifndef __has_attribute > >#define __has_attribute(x) 0 > >#endif > > > >#if defined(__GNUC_GNU_INLINE__) || __has_attribute(gnu_inline) > >#define __gnu_inline __attribute__(gnu_inline) > >#endif > > > >#define inline inline __attribute__((always_inline, unused)) notrace > >__gnu_inline > >``` > > > >Thoughts on this approach? I can send a v5 tomorrow if there's no > >major issues. Feedback appreciated, as always. > > > >[0] https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute > >[1] > >https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes > >[2] https://bugs.llvm.org/show_bug.cgi?id=37784 > > Please fix clang. It isn't all that hard to fix. > > However, __GCC_GNU_INLINE__ means you are in GNU mode by default,
Re: [PATCH v4 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
On Fri, Jun 8, 2018 at 3:04 AM Sedat Dilek wrote: > > On Fri, Jun 8, 2018 at 9:59 AM, Arnd Bergmann wrote: > > On Thu, Jun 7, 2018 at 10:49 PM, Nick Desaulniers > > wrote: > >> Functions marked extern inline do not emit an externally visible > >> function when the gnu89 C standard is used. Some KBUILD Makefiles > >> overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without > >> an explicit C standard specified, the default is gnu11. Since c99, the > >> semantics of extern inline have changed such that an externally visible > >> function is always emitted. This can lead to multiple definition errors > >> of extern inline functions at link time of compilation units whose build > >> files have removed an explicit C standard compiler flag for users of GCC > >> 5.1+ or Clang. > >> > >> Signed-off-by: Nick Desaulniers > >> Suggested-by: H. Peter Anvin > >> Suggested-by: Joe Perches > > > > I suspect this will break Geert's gcc-4.1.2, which I think doesn't have that > > attribute yet (4.1.3 or higher have it according to the documentation. > > > > It wouldn't be hard to work around that if we want to keep that version > > working, or we could decide that it's time to officially stop supporting > > that version, but we should probably decide on one or the other. Heh, so earlier we decided against compiler flags (-std=gnu89 or -fgnu89-inline) in preference to function attributes. The function attribute is preferable as some of the Makefiles [accidentally?] overwrite KBUILD_CFLAGS, which is problematic for gcc 5.1 users as the implicit c standard used was changed to gnu11 from gnu89. What's nice is that to support gcc 4.1 users, we simply don't need to add any attribute, as their implicit c standard is gnu89 which has the semantics for extern inline that we want. I have a simple change to this patch that can support users of various gcc versions, see below: > Good point. > What is the minimum requirement of GCC version currently? > AFAICS x86/asm-goto support requires GCC >= 4.5? Yes, but that's only for x86, IIUC. It seems the kernel may have different minimum required versions of GCC based on arch then? That may be ok, but I'm not sure that's easy to keep track of without having it explicitly stated somewhere like the docs perhaps? > Just FYI... > ...saw the last days in upstream commits that kbuild/kconfig for > 4.18-rc1 offers possibilities to check for cc-version dependencies. Those will be helpful. If we want to pursue compiler flags, which get set some Makefiles, then yes. But I think a simpler change to my patch would be as below. It seems gcc did not get __has_attribute [0] until 5.1, but will define __GNUC_GNU_INLINE__ if supported. [1] Unfortunately, Clang does not define __GNUC_GNU_INLINE__ [2]. So a proper feature test might look like: ``` #ifndef __has_attribute #define __has_attribute(x) 0 #endif #if defined(__GNUC_GNU_INLINE__) || __has_attribute(gnu_inline) #define __gnu_inline __attribute__(gnu_inline) #endif #define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline ``` Thoughts on this approach? I can send a v5 tomorrow if there's no major issues. Feedback appreciated, as always. [0] https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes [2] https://bugs.llvm.org/show_bug.cgi?id=37784 -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/3] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Changes since v3: Take Joe's suggestion to hoist __inline__ and __inline out of conditional block. Changes since v2: Take hpa's _ASM_ARG patch into the set in order to simplify cross 32b/64b x86 assembly and rebase my final patch to use it. Apply Sedat's typo fix to commit message and sign off on it. Take Joe's suggestion to simplify __inline__ and __inline. Add Arnd to list of folks who made helpful suggestions. Changes since v1: Prefer gnu_inline function attribute instead of explicitly setting C standard compiler flag in problematic Makefiles. We should instead carefully evaluate if those Makefiles should be overwriting KBUILD_CFLAGS at all. Dropped the previous first two patches and added a new first patch. H. Peter Anvin (1): x86/asm: add _ASM_ARG* constants for argument registers to Nick Desaulniers (2): compiler-gcc.h: add gnu_inline to all inline declarations x86: paravirt: make native_save_fl extern inline arch/x86/include/asm/asm.h | 59 + arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile| 1 + arch/x86/kernel/irqflags.S | 26 +++ include/linux/compiler-gcc.h| 17 ++ 5 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/3] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: Arnd Bergmann
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..ddeeaac8adda
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_ARG1
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.17.1.1185.g55be947832-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
From: "H. Peter Anvin" i386 and x86-64 uses different registers for arguments; make them available so we don't have to #ifdef in the actual code. Native size and specified size (q, l, w, b) versions are provided. Suggested-by: Sedat Dilek Signed-off-by: H. Peter Anvin Signed-off-by: Nick Desaulniers --- arch/x86/include/asm/asm.h | 59 ++ 1 file changed, 59 insertions(+) diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h index 219faaec51df..990770f9e76b 100644 --- a/arch/x86/include/asm/asm.h +++ b/arch/x86/include/asm/asm.h @@ -46,6 +46,65 @@ #define _ASM_SI__ASM_REG(si) #define _ASM_DI__ASM_REG(di) +#ifndef __x86_64__ +/* 32 bit */ + +#define _ASM_ARG1 _ASM_AX +#define _ASM_ARG2 _ASM_DX +#define _ASM_ARG3 _ASM_CX + +#define _ASM_ARG1L eax +#define _ASM_ARG2L edx +#define _ASM_ARG3L ecx + +#define _ASM_ARG1W ax +#define _ASM_ARG2W dx +#define _ASM_ARG3W cx + +#define _ASM_ARG1B al +#define _ASM_ARG2B dl +#define _ASM_ARG3B cl + +#else +/* 64 bit */ + +#define _ASM_ARG1 _ASM_DI +#define _ASM_ARG2 _ASM_SI +#define _ASM_ARG3 _ASM_DX +#define _ASM_ARG4 _ASM_CX +#define _ASM_ARG5 r8 +#define _ASM_ARG6 r9 + +#define _ASM_ARG1Q rdi +#define _ASM_ARG2Q rsi +#define _ASM_ARG3Q rdx +#define _ASM_ARG4Q rcx +#define _ASM_ARG5Q r8 +#define _ASM_ARG6Q r9 + +#define _ASM_ARG1L edi +#define _ASM_ARG2L esi +#define _ASM_ARG3L edx +#define _ASM_ARG4L ecx +#define _ASM_ARG5L r8d +#define _ASM_ARG6L r9d + +#define _ASM_ARG1W di +#define _ASM_ARG2W si +#define _ASM_ARG3W dx +#define _ASM_ARG4W cx +#define _ASM_ARG5W r8w +#define _ASM_ARG6W r9w + +#define _ASM_ARG1B dil +#define _ASM_ARG2B sil +#define _ASM_ARG3B dl +#define _ASM_ARG4B cl +#define _ASM_ARG5B r8b +#define _ASM_ARG6B r9b + +#endif + /* * Macros to generate condition code outputs from inline assembly, * The output operand must be type "bool". -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
Functions marked extern inline do not emit an externally visible function when the gnu89 C standard is used. Some KBUILD Makefiles overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without an explicit C standard specified, the default is gnu11. Since c99, the semantics of extern inline have changed such that an externally visible function is always emitted. This can lead to multiple definition errors of extern inline functions at link time of compilation units whose build files have removed an explicit C standard compiler flag for users of GCC 5.1+ or Clang. Signed-off-by: Nick Desaulniers Suggested-by: H. Peter Anvin Suggested-by: Joe Perches --- include/linux/compiler-gcc.h | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index b4bf73f5e38f..0c79c588e572 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -72,19 +72,22 @@ * -Wunused-function. This turns out to avoid the need for complex #ifdef * directives. Suppress the warning in clang as well by using "unused" * function attribute, which is redundant but not harmful for gcc. + * Prefer gnu_inline, so that extern inline functions do not emit an + * externally visible function. This makes extern inline behave as per gnu89 + * semantics rather than c99. This prevents multiple symbol definition errors + * of extern inline functions at link time. + * A lot of inline functions can cause havoc with function tracing. */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -#define inline inline __attribute__((always_inline,unused)) notrace -#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace -#define __inline __inline __attribute__((always_inline,unused)) notrace +#define inline \ + inline __attribute__((always_inline, unused, gnu_inline)) notrace #else -/* A lot of inline functions can cause havoc with function tracing */ -#define inline inline __attribute__((unused)) notrace -#define __inline__ __inline__ __attribute__((unused)) notrace -#define __inline __inline __attribute__((unused)) notrace +#define inline inline __attribute__((unused, gnu_inline)) notrace #endif +#define __inline__ inline +#define __inline inline #define __always_inlineinline __attribute__((always_inline)) #define noinline __attribute__((noinline)) -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
On Thu, Jun 7, 2018 at 1:14 PM H. Peter Anvin wrote: > > On 06/07/18 11:32, Nick Desaulniers wrote: > > > > Suggested-by: Sedat Dilek > > If this was suggested by Sedat, I didn't see that suggestion... I took his suggestion which was to fix the typo in the commit message. https://lkml.org/lkml/2018/6/5/776 -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Thu, Jun 7, 2018 at 11:31 AM Joe Perches wrote: > > On Thu, 2018-06-07 at 10:26 -0700, Nick Desaulniers wrote: > > I get the feeling that the use of __inline__ or __inline (vs inline) > > in the kernel may be wrong and their use should be eradicated in the > > follow up patch set, but it would be cool if others have additional > > insight. > > __inline is easy and useful to remove as it's used in > just a few files. > > But __inline__ is used in a lot of files and locations: > > $ git grep -w --name-only __inline__ | wc -l > 154 > $ git grep -w __inline__ | wc -l > 503 > > Some of these files are used in asm includes as > well where __inline__ may be preferred by gcc > > https://gcc.gnu.org/onlinedocs/gcc/Alternate-Keywords.html#Alternate-Keywords > > so perhaps asm exclusions would be necessary. Oops, just saw this email after sending v3: https://lkml.org/lkml/2018/6/7/1055 Guess I should re-add __inline__ and __inline then to 1/3? https://lkml.org/lkml/2018/6/7/1060 ie: +#define __inline__ __inline__ inline instead of +#define __inline__ inline ? On the other hand, I was able to assemble with gcc+binutils and v3 as it stands. Reading the link you sent, it seems more about compiling with -pedantic or non-gnu C standards being used (I'd call that unlikely for the kernel). So it still seems incorrect to me the use of __inline__ (but I do appreciate the link!) at least in the kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] x86: paravirt: make native_save_fl extern inline
On Thu, Jun 7, 2018 at 11:32 AM Nick Desaulniers
wrote:
>
> native_save_fl() is marked static inline, but by using it as
> a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
>
> paravirt's use of native_save_fl() also requires that no GPRs other than
> %rax are clobbered.
>
> Compilers have different heuristics which they use to emit stack guard
> code, the emittance of which can break paravirt's callee saved assumption
> by clobbering %rcx.
>
> Marking a function definition extern inline means that if this version
> cannot be inlined, then the out-of-line version will be preferred. By
> having the out-of-line version be implemented in assembly, it cannot be
> instrumented with a stack protector, which might violate custom calling
> conventions that code like paravirt rely on.
>
> The semantics of extern inline has changed since gnu89. This means that
> folks using GCC versions >= 5.1 may see symbol redefinition errors at
> link time for subdirs that override KBUILD_CFLAGS (making the C standard
> used implicit) regardless of this patch. This has been cleaned up
> earlier in the patch set, but is left as a note in the commit message
> for future travelers.
>
> Reports:
> https://lkml.org/lkml/2018/5/7/534
> https://github.com/ClangBuiltLinux/linux/issues/16
>
> Discussion:
> https://bugs.llvm.org/show_bug.cgi?id=37512
> https://lkml.org/lkml/2018/5/24/1371
>
> Thanks to the many folks that participated in the discussion.
>
> Debugged-by: Alistair Strachan
> Debugged-by: Matthias Kaehlcke
> Reported-by: Sedat Dilek
> Signed-off-by: Nick Desaulniers
> Suggested-by: Arnd Bergmann
> Suggested-by: H. Peter Anvin
> Suggested-by: Tom Stellar
> Tested-by: Sedat Dilek
> ---
> arch/x86/include/asm/irqflags.h | 2 +-
> arch/x86/kernel/Makefile| 1 +
> arch/x86/kernel/irqflags.S | 26 ++
> 3 files changed, 28 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/kernel/irqflags.S
>
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index 89f08955fff7..c4fc17220df9 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -13,7 +13,7 @@
> * Interrupt control:
> */
>
> -static inline unsigned long native_save_fl(void)
> +extern inline unsigned long native_save_fl(void)
> {
> unsigned long flags;
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 02d6f5cf4e70..8824d01c0c35 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o
> hw_breakpoint.o
> obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
> obj-y += pci-iommu_table.o
> obj-y += resource.o
> +obj-y += irqflags.o
>
> obj-y += process.o
> obj-y += fpu/
> diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
> new file mode 100644
> index ..ddeeaac8adda
> --- /dev/null
> +++ b/arch/x86/kernel/irqflags.S
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * unsigned long native_save_fl(void)
> + */
> +ENTRY(native_save_fl)
> + pushf
> + pop %_ASM_AX
> + ret
> +ENDPROC(native_save_fl)
> +EXPORT_SYMBOL(native_save_fl)
> +
> +/*
> + * void native_restore_fl(unsigned long flags)
> + * %eax/%rdi: flags
> + */
> +ENTRY(native_restore_fl)
> + push %_ASM_ARG1
> + popf
> + ret
> +ENDPROC(native_restore_fl)
> +EXPORT_SYMBOL(native_restore_fl)
> --
> 2.17.1.1185.g55be947832-goog
>
Probably should have mentioned in the notes/cover letter that this was
rebased on hpa's patch (2/3) in this series. As a change from v2.
--
Thanks,
~Nick Desaulniers
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: Arnd Bergmann
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..ddeeaac8adda
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %eax/%rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_ARG1
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.17.1.1185.g55be947832-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] compiler-gcc.h: add gnu_inline to all inline declarations
Functions marked extern inline do not emit an externally visible function when the gnu89 C standard is used. Some KBUILD Makefiles overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without an explicit C standard specified, the default is gnu11. Since c99, the semantics of extern inline have changed such that an externally visible function is always emitted. This can lead to multiple definition errors of extern inline functions at link time of compilation units whose build files have removed an explicit C standard compiler flag for users of GCC 5.1+ or Clang. Signed-off-by: Nick Desaulniers Suggested-by: H. Peter Anvin Suggested-by: Joe Perches --- include/linux/compiler-gcc.h | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index b4bf73f5e38f..3365f20dba5a 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -72,17 +72,22 @@ * -Wunused-function. This turns out to avoid the need for complex #ifdef * directives. Suppress the warning in clang as well by using "unused" * function attribute, which is redundant but not harmful for gcc. + * Prefer gnu_inline, so that extern inline functions do not emit an + * externally visible function. This makes extern inline behave as per gnu89 + * semantics rather than c99. This prevents multiple symbol definition errors + * of extern inline functions at link time. */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -#define inline inline __attribute__((always_inline,unused)) notrace -#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace -#define __inline __inline __attribute__((always_inline,unused)) notrace +#define inline \ + inline __attribute__((always_inline, unused, gnu_inline)) notrace +#define __inline__ inline +#define __inline inline #else /* A lot of inline functions can cause havoc with function tracing */ -#define inline inline __attribute__((unused)) notrace -#define __inline__ __inline__ __attribute__((unused)) notrace -#define __inline __inline __attribute__((unused)) notrace +#define inline inline __attribute__((unused, gnu_inline)) notrace +#define __inline__ inline +#define __inline inline #endif #define __always_inlineinline __attribute__((always_inline)) -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] x86/asm: add _ASM_ARG* constants for argument registers to
From: "H. Peter Anvin" i386 and x86-64 uses different registers for arguments; make them available so we don't have to #ifdef in the actual code. Native size and specified size (q, l, w, b) versions are provided. Suggested-by: Sedat Dilek Signed-off-by: H. Peter Anvin Signed-off-by: Nick Desaulniers --- arch/x86/include/asm/asm.h | 59 ++ 1 file changed, 59 insertions(+) diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h index 219faaec51df..990770f9e76b 100644 --- a/arch/x86/include/asm/asm.h +++ b/arch/x86/include/asm/asm.h @@ -46,6 +46,65 @@ #define _ASM_SI__ASM_REG(si) #define _ASM_DI__ASM_REG(di) +#ifndef __x86_64__ +/* 32 bit */ + +#define _ASM_ARG1 _ASM_AX +#define _ASM_ARG2 _ASM_DX +#define _ASM_ARG3 _ASM_CX + +#define _ASM_ARG1L eax +#define _ASM_ARG2L edx +#define _ASM_ARG3L ecx + +#define _ASM_ARG1W ax +#define _ASM_ARG2W dx +#define _ASM_ARG3W cx + +#define _ASM_ARG1B al +#define _ASM_ARG2B dl +#define _ASM_ARG3B cl + +#else +/* 64 bit */ + +#define _ASM_ARG1 _ASM_DI +#define _ASM_ARG2 _ASM_SI +#define _ASM_ARG3 _ASM_DX +#define _ASM_ARG4 _ASM_CX +#define _ASM_ARG5 r8 +#define _ASM_ARG6 r9 + +#define _ASM_ARG1Q rdi +#define _ASM_ARG2Q rsi +#define _ASM_ARG3Q rdx +#define _ASM_ARG4Q rcx +#define _ASM_ARG5Q r8 +#define _ASM_ARG6Q r9 + +#define _ASM_ARG1L edi +#define _ASM_ARG2L esi +#define _ASM_ARG3L edx +#define _ASM_ARG4L ecx +#define _ASM_ARG5L r8d +#define _ASM_ARG6L r9d + +#define _ASM_ARG1W di +#define _ASM_ARG2W si +#define _ASM_ARG3W dx +#define _ASM_ARG4W cx +#define _ASM_ARG5W r8w +#define _ASM_ARG6W r9w + +#define _ASM_ARG1B dil +#define _ASM_ARG2B sil +#define _ASM_ARG3B dl +#define _ASM_ARG4B cl +#define _ASM_ARG5B r8b +#define _ASM_ARG6B r9b + +#endif + /* * Macros to generate condition code outputs from inline assembly, * The output operand must be type "bool". -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/3] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Changes since v2: Take hpa's _ASM_ARG patch into the set in order to simplify cross 32b/64b x86 assembly. Apply Sedat's typo fix to commit message and sign off on it. Take Joe's sugguestion to simplify __inline__ and __inline. Add Arnd to list of folks who made helpful suggestions. Changes since v1: Prefer gnu_inline function attribute instead of explicitly setting C standard compiler flag in problematic Makefiles. We should instead carefully evaluate if those Makefiles should be overwriting KBUILD_CFLAGS at all. Dropped the previous first two patches and added a new first patch. H. Peter Anvin (1): x86/asm: add _ASM_ARG* constants for argument registers to Nick Desaulniers (2): compiler-gcc.h: add gnu_inline to all inline declarations x86: paravirt: make native_save_fl extern inline arch/x86/include/asm/asm.h | 59 + arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile| 1 + arch/x86/kernel/irqflags.S | 26 +++ include/linux/compiler-gcc.h| 17 ++ 5 files changed, 98 insertions(+), 7 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 5, 2018 at 10:23 AM Joe Perches wrote: > > On Tue, 2018-06-05 at 10:05 -0700, Nick Desaulniers wrote: > And only set once along with: > > > #define __always_inline inline __attribute__((always_inline)) > > And perhaps this __always_inline should be updated > with gnu_inline as well > This should pick up the additional attributes from adding `inline` to the redefinition. Attributes can be combined via: __attribute__((a)) __attribute__((b)) or __attribute__((a, b)) -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Thu, Jun 7, 2018 at 10:26 AM Nick Desaulniers wrote: > > On Tue, Jun 5, 2018 at 10:23 AM Joe Perches wrote: > > > > On Tue, 2018-06-05 at 10:05 -0700, Nick Desaulniers wrote: > > > Functions marked extern inline do not emit an externally visible > > > function when the gnu89 C standard is used. Some KBUILD Makefiles > > > overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without > > > an explicit C standard specified, the default is gnu11. Since c99, the > > > semantics of extern inline have changed such that an externally visible > > > function is always emitted. This can lead to multiple definition errors > > > of extern inline functions at link time of compilation units whose build > > > files have removed an explicit C standard compiler flag for users of GCC > > > 5.1+ or Clang. > > [] > > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > [] > > > @@ -72,17 +72,24 @@ > > > * -Wunused-function. This turns out to avoid the need for complex > > > #ifdef > > > * directives. Suppress the warning in clang as well by using "unused" > > > * function attribute, which is redundant but not harmful for gcc. > > > + * Prefer gnu_inline, so that extern inline functions do not emit an > > > + * externally visible function. This makes extern inline behave as per > > > gnu89 > > > + * semantics rather than c99. This prevents multiple symbol definition > > > errors > > > + * of extern inline functions at link time. > > > */ > > > #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ > > > !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) > > > -#define inline inline > > > __attribute__((always_inline,unused)) notrace > > > -#define __inline__ __inline__ > > > __attribute__((always_inline,unused)) notrace > > > -#define __inline __inline__attribute__((always_inline,unused)) > > > notrace > > > +#define inline \ > > > + inline __attribute__((always_inline, unused, gnu_inline)) notrace > > > +#define __inline__ \ > > > + __inline__ __attribute__((always_inline, unused, gnu_inline)) > > > notrace > > > +#define __inline \ > > > + __inline __attribute__((always_inline, unused, gnu_inline)) notrace > > > > Perhaps these are simpler as > > > > #define __inline__ inline > > #define __inlineinline > > Probably want: > > #define __inline__ __inline__ inline > #define __inline __inline inline Oh, never mind, if my changes to `inline` add the `inline` keyword, then we can remove the redefinition __inline__ and __inline. All that to say your original suggestion is better than my follow up. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 5, 2018 at 10:23 AM Joe Perches wrote: > > On Tue, 2018-06-05 at 10:05 -0700, Nick Desaulniers wrote: > > Functions marked extern inline do not emit an externally visible > > function when the gnu89 C standard is used. Some KBUILD Makefiles > > overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without > > an explicit C standard specified, the default is gnu11. Since c99, the > > semantics of extern inline have changed such that an externally visible > > function is always emitted. This can lead to multiple definition errors > > of extern inline functions at link time of compilation units whose build > > files have removed an explicit C standard compiler flag for users of GCC > > 5.1+ or Clang. > [] > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > [] > > @@ -72,17 +72,24 @@ > > * -Wunused-function. This turns out to avoid the need for complex #ifdef > > * directives. Suppress the warning in clang as well by using "unused" > > * function attribute, which is redundant but not harmful for gcc. > > + * Prefer gnu_inline, so that extern inline functions do not emit an > > + * externally visible function. This makes extern inline behave as per > > gnu89 > > + * semantics rather than c99. This prevents multiple symbol definition > > errors > > + * of extern inline functions at link time. > > */ > > #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ > > !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) > > -#define inline inline__attribute__((always_inline,unused)) > > notrace > > -#define __inline__ __inline____attribute__((always_inline,unused)) > > notrace > > -#define __inline __inline__attribute__((always_inline,unused)) notrace > > +#define inline \ > > + inline __attribute__((always_inline, unused, gnu_inline)) notrace > > +#define __inline__ \ > > + __inline__ __attribute__((always_inline, unused, gnu_inline)) notrace > > +#define __inline \ > > + __inline __attribute__((always_inline, unused, gnu_inline)) notrace > > Perhaps these are simpler as > > #define __inline__ inline > #define __inlineinline Working on this now, going to push v3 soon. I was wondering more about these definitions of inline. Probably want: #define __inline__ __inline__ inline #define __inline __inline inline These are the only references I found to: __inline__: https://gcc.gnu.org/onlinedocs/gcc/Inline.html __inline: http://www.keil.com/support/man/docs/armcc/armcc_chr1359124967692.htm The commit that introduced them was: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a1365647022eb05a5993f270a78e9bef3bf554eb which was an interesting read. I get the feeling that the use of __inline__ or __inline (vs inline) in the kernel may be wrong and their use should be eradicated in the follow up patch set, but it would be cool if others have additional insight. This code in Clang seems to treat them all as the same: https://github.com/llvm-mirror/clang/blob/1a597eeed3579b4320b62ff55150195482545992/lib/Lex/PPDirectives.cpp#L2285 -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
On Tue, Jun 5, 2018 at 2:31 PM Arnd Bergmann wrote: > > On Tue, Jun 5, 2018 at 11:28 PM, Arnd Bergmann wrote: > > On Tue, Jun 5, 2018 at 7:05 PM, Nick Desaulniers > > wrote: > >> > >> The semantics of extern inline has changed since gnu89. This means that > >> folks using GCC versions >= 5.1 may see symbol redefinition errors at > >> link time for subdirs that override KBUILD_CFLAGS (making the C standard > >> used implicit) regardless of this patch. This has been cleaned up > >> earlier in the patch set, but is left as a note in the commit message > >> for future travelers. > > > > I think the keyword you are missing is > > > > __attribute__((gnu_inline)) > > > > which forces the gnu89 behavior on all compiler versions. It's been > > supported > > since gcc-4.2, so it should not cause problems on any compiler that is able > > to build an x86 kernel. > > Nevermind, I just saw you already posted that. > >Arnd That's ok, I appreciate you taking the time to review. Your point made me consider adding the function attribute to just native_save_fl() in this patch, but it seems that that lone function attribute is not used outside of compiler-gcc.h, so it would be good to change all users of inline in one place. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 5, 2018 at 12:14 PM Joe Perches wrote: > > On Tue, 2018-06-05 at 10:23 -0700, Joe Perches wrote: > > Perhaps these are simpler as > > > > #define __inline__inline > > #define __inline inline > > Currently, there are these uses of inline variants in the kernel > > $ git grep -w inline | wc -l > 68410 > $ git grep -w __inline__ | wc -l > 503 > $ git grep -w __inline | wc -l > 57 > > So it seems it's also reasonable to sed all uses of __inline to inline > and perhaps remove __inline eventually altogether. > (perhaps there are still too many __inline__ uses) Yeah, that sounds good. Should I split that into 3 patches: > Excluding scripts and a few other files, > here's a possible patch done with: > > $ git grep -w --name-only __inline | \ > grep -vP '^(?:arch/alpha/|include/|scripts/)' | \ > xargs sed -r -i -e 's/\b__inline\b/inline/g' \ > -e 's/\binline\s+static\b/static inline/g' > --- > Documentation/trace/tracepoint-analysis.rst | 2 +- > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c| 4 ++-- > drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 2 +- > drivers/staging/rtl8723bs/include/drv_types.h | 6 +++--- > drivers/staging/rtl8723bs/include/ieee80211.h | 6 +++--- > drivers/staging/rtl8723bs/include/osdep_service.h | 10 +- > drivers/staging/rtl8723bs/include/osdep_service_linux.h | 14 +++--- > drivers/staging/rtl8723bs/include/rtw_mlme.h| 14 +++--- > drivers/staging/rtl8723bs/include/rtw_recv.h| 16 > drivers/staging/rtl8723bs/include/sta_info.h| 2 +- > drivers/staging/rtl8723bs/include/wifi.h| 14 +++--- > drivers/staging/rtl8723bs/include/wlan_bssdef.h | 2 +- > lib/zstd/mem.h | 2 +- > 13 files changed, 47 insertions(+), 47 deletions(-) 1 for documentation, 1 for rtl8723bs, 1 for zstd? Follow up set or include in v3? -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
On Tue, Jun 5, 2018 at 10:23 AM Joe Perches wrote: > On Tue, 2018-06-05 at 10:05 -0700, Nick Desaulniers wrote: > > #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ > > !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) > > -#define inline inline__attribute__((always_inline,unused)) > > notrace > > -#define __inline__ __inline____attribute__((always_inline,unused)) > > notrace > > -#define __inline __inline__attribute__((always_inline,unused)) notrace > > +#define inline \ > > + inline __attribute__((always_inline, unused, gnu_inline)) notrace > > +#define __inline__ \ > > + __inline__ __attribute__((always_inline, unused, gnu_inline)) notrace > > +#define __inline \ > > + __inline __attribute__((always_inline, unused, gnu_inline)) notrace > > Perhaps these are simpler as > > #define __inline__ inline > #define __inlineinline > > > #else > > /* A lot of inline functions can cause havoc with function tracing */ > > -#define inline inline__attribute__((unused)) notrace > > -#define __inline__ __inline____attribute__((unused)) notrace > > -#define __inline __inline__attribute__((unused)) notrace > > +#define inline inline__attribute__((unused, gnu_inline)) > > notrace > > +#define __inline__ __inline____attribute__((unused, gnu_inline)) > > notrace > > +#define __inline __inline__attribute__((unused, gnu_inline)) notrace > > #endif > > And only set once along with: > > > #define __always_inline inline __attribute__((always_inline)) > > And perhaps this __always_inline should be updated > with gnu_inline as well > Great idea, I'll pick this up and add you to the Suggested-by: tag in v3. Thanks Joe! -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
On 06/05/18 10:28, H. Peter Anvin wrote: > On 06/05/18 10:05, Nick Desaulniers wrote: >> + >> +/* >> + * void native_restore_fl(unsigned long flags) >> + * %rdi: flags >> + */ >> +ENTRY(native_restore_fl) >> +push %_ASM_DI >> +popf >> +ret >> +ENDPROC(native_restore_fl) >> +EXPORT_SYMBOL(native_restore_fl) >> > > To work on i386, this would have to be %_ASM_AX in that case. ? Does the kernel have a different calling convention for 32b x86? How does that work? regparm=3? Does that need to be added to the declaration? > Something like this added to might be useful; then you can > simply: > > push %_ASM_ARG1 > > Version with fixed typo... Oh, nice, thanks! I'll pick this up and add it to my patch set for v3 (or did you want me to review/sign-off now?) I can pick up Sedat's suggestion. -- Thanks, ~Nick Desaulniers -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Changes since v2: Prefer gnu_inline function attribute instead of explicitly setting C standard compiler flag in problematic Makefiles. We should instead carefully evaluate if those Makefiles should be overwriting KBUILD_CFLAGS at all. Dropped the previous first two patches and added a new first patch. Nick Desaulniers (2): compiler-gcc.h: add gnu_inline to all inline declarations x86: paravirt: make native_save_fl extern inline arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile| 1 + arch/x86/kernel/irqflags.S | 26 ++ include/linux/compiler-gcc.h| 19 +-- 4 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
Functions marked extern inline do not emit an externally visible function when the gnu89 C standard is used. Some KBUILD Makefiles overwrite KBUILD_CFLAGS. This is an issue for GCC 5.1+ users as without an explicit C standard specified, the default is gnu11. Since c99, the semantics of extern inline have changed such that an externally visible function is always emitted. This can lead to multiple definition errors of extern inline functions at link time of compilation units whose build files have removed an explicit C standard compiler flag for users of GCC 5.1+ or Clang. Signed-off-by: Nick Desaulniers Suggested-by: H. Peter Anvin Tested-by: Sedat Dilek --- include/linux/compiler-gcc.h | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index b4bf73f5e38f..7827bdf0e5e9 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -72,17 +72,24 @@ * -Wunused-function. This turns out to avoid the need for complex #ifdef * directives. Suppress the warning in clang as well by using "unused" * function attribute, which is redundant but not harmful for gcc. + * Prefer gnu_inline, so that extern inline functions do not emit an + * externally visible function. This makes extern inline behave as per gnu89 + * semantics rather than c99. This prevents multiple symbol definition errors + * of extern inline functions at link time. */ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \ !defined(CONFIG_OPTIMIZE_INLINING) || (__GNUC__ < 4) -#define inline inline __attribute__((always_inline,unused)) notrace -#define __inline__ __inline__ __attribute__((always_inline,unused)) notrace -#define __inline __inline __attribute__((always_inline,unused)) notrace +#define inline \ + inline __attribute__((always_inline, unused, gnu_inline)) notrace +#define __inline__ \ + __inline__ __attribute__((always_inline, unused, gnu_inline)) notrace +#define __inline \ + __inline __attribute__((always_inline, unused, gnu_inline)) notrace #else /* A lot of inline functions can cause havoc with function tracing */ -#define inline inline __attribute__((unused)) notrace -#define __inline__ __inline__ __attribute__((unused)) notrace -#define __inline __inline __attribute__((unused)) notrace +#define inline inline __attribute__((unused, gnu_inline)) notrace +#define __inline__ __inline__ __attribute__((unused, gnu_inline)) notrace +#define __inline __inline __attribute__((unused, gnu_inline)) notrace #endif #define __always_inlineinline __attribute__((always_inline)) -- 2.17.1.1185.g55be947832-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
No changes in v2, but here's the discussion of v1:
https://patchwork.kernel.org/patch/10444075/
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..937bd08c3f79
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_DI
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.17.1.1185.g55be947832-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] x86: paravirt: make native_save_fl extern inline
native_save_fl() is marked static inline, but by using it as
a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
paravirt's use of native_save_fl() also requires that no GPRs other than
%rax are clobbered.
Compilers have different heuristics which they use to emit stack guard
code, the emittance of which can break paravirt's callee saved assumption
by clobbering %rcx.
Marking a function definition extern inline means that if this version
cannot be inlined, then the out-of-line version will be preferred. By
having the out-of-line version be implemented in assembly, it cannot be
instrumented with a stack protector, which might violate custom calling
conventions that code like paravirt rely on.
The semantics of extern inline has changed since gnu89. This means that
folks using GCC versions >= 5.1 may see symbol redefinition errors at
link time for subdirs that override KBUILD_CFLAGS (making the C standard
used implicit) regardless of this patch. This has been cleaned up
earlier in the patch set, but is left as a note in the commit message
for future travelers.
Reports:
https://lkml.org/lkml/2018/5/7/534
https://github.com/ClangBuiltLinux/linux/issues/16
Discussion:
https://bugs.llvm.org/show_bug.cgi?id=37512
https://lkml.org/lkml/2018/5/24/1371
Thanks to the many folks that participated in the discussion.
Debugged-by: Alistair Strachan
Debugged-by: Matthias Kaehlcke
Reported-by: Sedat Dilek
Signed-off-by: Nick Desaulniers
Suggested-by: H. Peter Anvin
Suggested-by: Tom Stellar
Tested-by: Sedat Dilek
---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/kernel/Makefile| 1 +
arch/x86/kernel/irqflags.S | 26 ++
3 files changed, 28 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/irqflags.S
diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index 89f08955fff7..c4fc17220df9 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -13,7 +13,7 @@
* Interrupt control:
*/
-static inline unsigned long native_save_fl(void)
+extern inline unsigned long native_save_fl(void)
{
unsigned long flags;
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e70..8824d01c0c35 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -61,6 +61,7 @@ obj-y += alternative.o i8253.o hw_breakpoint.o
obj-y += tsc.o tsc_msr.o io_delay.o rtc.o
obj-y += pci-iommu_table.o
obj-y += resource.o
+obj-y += irqflags.o
obj-y += process.o
obj-y += fpu/
diff --git a/arch/x86/kernel/irqflags.S b/arch/x86/kernel/irqflags.S
new file mode 100644
index ..937bd08c3f79
--- /dev/null
+++ b/arch/x86/kernel/irqflags.S
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include
+#include
+#include
+
+/*
+ * unsigned long native_save_fl(void)
+ */
+ENTRY(native_save_fl)
+ pushf
+ pop %_ASM_AX
+ ret
+ENDPROC(native_save_fl)
+EXPORT_SYMBOL(native_save_fl)
+
+/*
+ * void native_restore_fl(unsigned long flags)
+ * %rdi: flags
+ */
+ENTRY(native_restore_fl)
+ push %_ASM_DI
+ popf
+ ret
+ENDPROC(native_restore_fl)
+EXPORT_SYMBOL(native_restore_fl)
--
2.17.0.921.gf22659ad46-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] efi: use -std=gnu89 for proper extern inline semantics
The top level Makefile explicitly sets the C standard used in the kernel to gnu89. By overriding KBUILD_CFLAGS, the C standard used for this subdir is now implicit based on compiler and compiler version. GCC changes this implicit default from gnu89 to gnu11 in v5.1. This implies that depending on compiler version, parts of the kernel are being linked together from object files that were compiled with different C standard compiler flags. This is problematic for symbols declared as extern inline, as the semantics have switched since gnu89. See also: http://blahg.josefsipek.net/?p=529 Signed-off-by: Nick Desaulniers Suggested-by: Sedat Dilek Tested-by: Sedat Dilek --- drivers/firmware/efi/libstub/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index a34e9290a699..97ca53ab2b69 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -7,7 +7,7 @@ # cflags-$(CONFIG_X86_32):= -march=i386 cflags-$(CONFIG_X86_64):= -mcmodel=small -cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \ +cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 -std=gnu89 \ -fPIC -fno-strict-aliasing -mno-red-zone \ -mno-mmx -mno-sse -fshort-wchar -- 2.17.0.921.gf22659ad46-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] x86/build: use -std=gnu89 for proper extern inline semantics
The top level Makefile explicitly sets the C standard used in the kernel to gnu89. By overriding KBUILD_CFLAGS, the C standard used for this subdir is now implicit based on compiler and compiler version. GCC changes this implicit default from gnu89 to gnu11 in v5.1. This implies that depending on compiler version, parts of the kernel are being linked together from object files that were compiled with different C standard compiler flags. This is problematic for symbols declared as extern inline, as the semantics have switched since gnu89. See also: http://blahg.josefsipek.net/?p=529 Signed-off-by: Nick Desaulniers Tested-by: Sedat Dilek --- arch/x86/boot/compressed/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index fa42f895fdde..1a04c7e9add1 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -26,7 +26,7 @@ KCOV_INSTRUMENT := n targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \ vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4 -KBUILD_CFLAGS := -m$(BITS) -O2 +KBUILD_CFLAGS := -m$(BITS) -O2 -std=gnu89 KBUILD_CFLAGS += -fno-strict-aliasing $(call cc-option, -fPIE, -fPIC) KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING cflags-$(CONFIG_X86_32) := -march=i386 -- 2.17.0.921.gf22659ad46-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] extern inline native_save_fl for paravirt
paravirt depends on a custom calling convention (callee saved), but expects this from a static inline function that it then forces to be outlined. This is problematic because different compilers or flags can then add a stack guard that violates the calling conventions. Uses extern inline with the out-of-line definition in assembly to prevent compilers from adding stack guards to the outlined version. Other parts of the codebase overwrite KBUILD_CFLAGS, which is *extremely problematic* for extern inline, as the sematics are completely the opposite depending on what C standard is used. http://blahg.josefsipek.net/?p=529 Nick Desaulniers (3): efi: use -std=gnu89 for proper extern inline semantics x86/build: use -std=gnu89 for proper extern inline semantics x86: paravirt: make native_save_fl extern inline arch/x86/boot/compressed/Makefile | 2 +- arch/x86/include/asm/irqflags.h | 2 +- arch/x86/kernel/Makefile | 1 + arch/x86/kernel/irqflags.S| 26 ++ drivers/firmware/efi/libstub/Makefile | 2 +- 5 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 arch/x86/kernel/irqflags.S -- 2.17.0.921.gf22659ad46-goog -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
