| Message ID | 20180621162324.36656-4-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Nick Desaulniers <ndesaulniers@google.com> 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? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 21, 2018 at 7:24 PM Ingo Molnar <mingo@kernel.org> wrote: > * Nick Desaulniers <ndesaulniers@google.com> 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 ffffffff81059140 <native_save_fl>: ffffffff81059140: 9c pushfq ffffffff81059141: 58 pop %rax ffffffff81059142: c3 retq $ git checkout HEAD~3 $ make CC=gcc-8 -j46 $ objdump -d vmlinux | grep native_save_fl --context=3 ffffffff81079410 <native_save_fl>: ffffffff81079410: 9c pushfq ffffffff81079411: 58 pop %rax ffffffff81079412: c3 retq 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-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Nick Desaulniers <ndesaulniers@google.com> wrote: > On Thu, Jun 21, 2018 at 7:24 PM Ingo Molnar <mingo@kernel.org> wrote: > > * Nick Desaulniers <ndesaulniers@google.com> 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 > ffffffff81059140 <native_save_fl>: > ffffffff81059140: 9c pushfq > ffffffff81059141: 58 pop %rax > ffffffff81059142: c3 retq > $ git checkout HEAD~3 > $ make CC=gcc-8 -j46 > $ objdump -d vmlinux | grep native_save_fl --context=3 > ffffffff81079410 <native_save_fl>: > ffffffff81079410: 9c pushfq > ffffffff81079411: 58 pop %rax > ffffffff81079412: c3 retq > > 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-- Ok! Acked-by: Ingo Molnar <mingo@kernel.org> What's the planned upstreaming route for these patches/fixes? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 3:13 AM Ingo Molnar <mingo@kernel.org> wrote: > Ok! > > Acked-by: Ingo Molnar <mingo@kernel.org> > > 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?)
On 26/06/18 18:22, Nick Desaulniers wrote: > On Tue, Jun 26, 2018 at 3:13 AM Ingo Molnar <mingo@kernel.org> wrote: >> Ok! >> >> Acked-by: Ingo Molnar <mingo@kernel.org> >> >> 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. As there is no paravirt tree the x86 one seems to be appropriate. :-) Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 13, 2018 at 3:15 AM David Laight <David.Laight@aculab.com> 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.
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 000000000000..ddeeaac8adda --- /dev/null +++ b/arch/x86/kernel/irqflags.S @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <asm/asm.h> +#include <asm/export.h> +#include <linux/linkage.h> + +/* + * 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)