Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1602045 > unrolled thread
| Started by | Ingo Molnar <mingo@kernel.org> |
|---|---|
| First post | 2017-03-16 09:10 +0100 |
| Last post | 2017-03-20 13:40 +0100 |
| Articles | 10 on this page of 30 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC] linkage: new macros for functions and data Ingo Molnar <mingo@kernel.org> - 2017-03-16 09:10 +0100
Re: [RFC] linkage: new macros for functions and data Jiri Slaby <jslaby@suse.cz> - 2017-03-16 09:20 +0100
[PATCH v2 07/10] x86: assembly, annotate aliases Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 04/10] x86: boot, annotate functions properly Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 09/10] x86: entry, annotate interrupt symbols properly Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-21 15:50 +0100
Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Ingo Molnar <mingo@kernel.org> - 2017-03-22 08:40 +0100
Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-22 15:30 +0100
Re: [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions Jiri Slaby <jslaby@suse.cz> - 2017-03-22 17:00 +0100
[PATCH v2 05/10] x86: kernel+lib, annotate local functions Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 08/10] x86: entry, annotate THUNKs Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 06/10] x86: crypto, annotate local functions Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-20 14:40 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby <jslaby@suse.cz> - 2017-03-20 16:50 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-20 17:10 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Pavel Machek <pavel@ucw.cz> - 2017-03-21 15:20 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Ingo Molnar <mingo@kernel.org> - 2017-03-22 08:30 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby <jslaby@suse.cz> - 2017-03-22 08:50 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Ingo Molnar <mingo@kernel.org> - 2017-03-22 08:50 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-22 15:20 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby <jslaby@suse.cz> - 2017-03-22 16:10 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-22 16:40 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Ingo Molnar <mingo@kernel.org> - 2017-03-23 08:40 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-23 14:30 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Jiri Slaby <jslaby@suse.cz> - 2017-03-22 13:10 +0100
Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data Pavel Machek <pavel@ucw.cz> - 2017-03-22 17:00 +0100
[RFC v2 10/10] x86: boot, extract efi_pe_entry from startup_64 Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
[PATCH v2 01/10] linkage: new macros for assembler symbols Jiri Slaby <jslaby@suse.cz> - 2017-03-20 13:40 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-22 08:50 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnJbQ-7m2-9@gated-at.bofh.it> |
| In reply to | #1606263 |
* Jiri Slaby <jslaby@suse.cz> wrote: > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > >> Hi! > >> > >>> -ENTRY(saved_rbp) .quad 0 > >>> -ENTRY(saved_rsi) .quad 0 > >>> -ENTRY(saved_rdi) .quad 0 > >>> -ENTRY(saved_rbx) .quad 0 > >>> +SYM_DATA_START(saved_rbp) .quad 0 > >>> +SYM_DATA_START(saved_rsi) .quad 0 > >>> +SYM_DATA_START(saved_rdi) .quad 0 > >>> +SYM_DATA_START(saved_rbx) .quad 0 > >> > >> Does it make sense to call it SYM_DATA_*START* when there's no > >> corresponding end? > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > SYM_*_START() uses? This could be done by emitting debug data into a special > > section and then analyzing that section for unpaired entries. The section can be > > discarded in the final link, it won't show up in the kernel image. > > It should be easier than that. No introduction of other info needed -- > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > be a bug now. I'm all for that! Can we detect double ends as well - i.e. do a build check of the full syntax of these symbol definition primitives? Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-22 15:20 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnPhh-3Fu-51@gated-at.bofh.it> |
| In reply to | #1606264 |
On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
>
> * Jiri Slaby <jslaby@suse.cz> wrote:
>
> > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > >
> > > * Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > >> Hi!
> > >>
> > >>> -ENTRY(saved_rbp) .quad 0
> > >>> -ENTRY(saved_rsi) .quad 0
> > >>> -ENTRY(saved_rdi) .quad 0
> > >>> -ENTRY(saved_rbx) .quad 0
> > >>> +SYM_DATA_START(saved_rbp) .quad 0
> > >>> +SYM_DATA_START(saved_rsi) .quad 0
> > >>> +SYM_DATA_START(saved_rdi) .quad 0
> > >>> +SYM_DATA_START(saved_rbx) .quad 0
> > >>
> > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > >> corresponding end?
> > >
> > > That looks like a bug - I think we should strive for them to always be in pairs.
> > >
> > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated'
> > > SYM_*_START() uses? This could be done by emitting debug data into a special
> > > section and then analyzing that section for unpaired entries. The section can be
> > > discarded in the final link, it won't show up in the kernel image.
> >
> > It should be easier than that. No introduction of other info needed --
> > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > be a bug now.
>
> I'm all for that!
It would be easy to add this checking to objtool since it already reads
the symbol table. The hard part is figuring out the logistics. :-)
- Should the warnings be on by default?
- Part of the "objtool check" command or something else?
- Separate config option or just include it with
CONFIG_STACK_VALIDATION?
- Should all asm files be checked, including those currently skipped by
objtool with OBJECT_FILES_NON_STANDARD?
> Can we detect double ends as well - i.e. do a build check of the full syntax of
> these symbol definition primitives?
Detecting double ends would be a little trickier. The second SYM_*_END
supersedes the first, so that information isn't in the ELF symbol table.
We could use a special section to annotate all the macro uses and have
objtool do the checking, similar to what you suggested earlier.
Or, here's a much easier way to do it, without involving objtool:
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -138,9 +138,17 @@
name:
#endif
+#ifndef CHECK_DUP_SYM_END
+#define CHECK_DUP_SYM_END(name) \
+ .pushsection .discard.sym_func_end ASM_NL \
+ SYM_END_##name: .byte 0 ASM_NL \
+ .popsection
+#endif
+
/* SYM_END -- use only if you have to */
#ifndef SYM_END
#define SYM_END(name, sym_type) \
+ CHECK_DUP_SYM_END(name) ASM_NL \
.type name sym_type ASM_NL \
.size name, .-name
#endif
If there's an extra SYM_*_END, the build fails. For example, if I add
an extra SYM_FUNC_END(\name) to the THUNK macro:
AS arch/x86/entry/thunk_64.o
arch/x86/entry/thunk_64.S: Assembler messages:
arch/x86/entry/thunk_64.S:42: Error: symbol `SYM_END_trace_hardirqs_on_thunk' is already defined
arch/x86/entry/thunk_64.S:43: Error: symbol `SYM_END_trace_hardirqs_off_thunk' is already defined
arch/x86/entry/thunk_64.S:47: Error: symbol `SYM_END_lockdep_sys_exit_thunk' is already defined
arch/x86/entry/thunk_64.S:51: Error: symbol `SYM_END____preempt_schedule' is already defined
arch/x86/entry/thunk_64.S:52: Error: symbol `SYM_END____preempt_schedule_notrace' is already defined
scripts/Makefile.build:395: recipe for target 'arch/x86/entry/thunk_64.o' failed
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-03-22 16:10 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnQ3D-4lf-7@gated-at.bofh.it> |
| In reply to | #1606550 |
On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote:
> Or, here's a much easier way to do it, without involving objtool:
>
> --- a/include/linux/linkage.h
> +++ b/include/linux/linkage.h
> @@ -138,9 +138,17 @@
> name:
> #endif
>
> +#ifndef CHECK_DUP_SYM_END
> +#define CHECK_DUP_SYM_END(name) \
> + .pushsection .discard.sym_func_end ASM_NL \
> + SYM_END_##name: .byte 0 ASM_NL \
> + .popsection
> +#endif
> +
> /* SYM_END -- use only if you have to */
> #ifndef SYM_END
> #define SYM_END(name, sym_type) \
> + CHECK_DUP_SYM_END(name) ASM_NL \
> .type name sym_type ASM_NL \
> .size name, .-name
> #endif
I tried this approach and it didn't work for me inside .macros. Oh,
well, the name cannot be first, so now, we can have a check for both
correct pairing _and_ duplicate ends in one:
#define SYM_CHECK_START(name) \
.pushsection .rodata.bubak ASM_NL \
.long has_no_SYM_END_##name - . ASM_NL \
.popsection
#define SYM_CHECK_END(name) \
has_no_SYM_END_##name:
/* SYM_START -- use only if you have to */
#ifndef SYM_START
#define SYM_START(name, align, visibility, entry) \
SYM_CHECK_START(name) ASM_NL \
visibility(name) ASM_NL \
align ASM_NL \
name: ASM_NL \
entry
#endif
/* SYM_END -- use only if you have to */
#ifndef SYM_END
#define SYM_END(name, sym_type, exit) \
exit ASM_NL \
SYM_CHECK_END(name) ASM_NL \
.type name sym_type ASM_NL \
.size name, .-name
#endif
So for the ftrace mistake I did:
AS arch/x86/kernel/mcount_64.o
/home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages:
/home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol
`has_no_SYM_END_ftrace_caller' is already defined
or if I remove SYM_END_FUNC completely:
LD vmlinux.o
MODPOST vmlinux.o
arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to
`has_no_SYM_END_ftrace_stub'
Sad is that this occurs only during linking, so I cannot put it in the
.discard section -- ideas?
thanks,
--
js
suse labs
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-22 16:40 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnQwG-4GP-1@gated-at.bofh.it> |
| In reply to | #1606631 |
On Wed, Mar 22, 2017 at 04:01:08PM +0100, Jiri Slaby wrote: > On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote: > > Or, here's a much easier way to do it, without involving objtool: > > > > --- a/include/linux/linkage.h > > +++ b/include/linux/linkage.h > > @@ -138,9 +138,17 @@ > > name: > > #endif > > > > +#ifndef CHECK_DUP_SYM_END > > +#define CHECK_DUP_SYM_END(name) \ > > + .pushsection .discard.sym_func_end ASM_NL \ > > + SYM_END_##name: .byte 0 ASM_NL \ > > + .popsection > > +#endif > > + > > /* SYM_END -- use only if you have to */ > > #ifndef SYM_END > > #define SYM_END(name, sym_type) \ > > + CHECK_DUP_SYM_END(name) ASM_NL \ > > .type name sym_type ASM_NL \ > > .size name, .-name > > #endif > > I tried this approach and it didn't work for me inside .macros. Oh, > well, the name cannot be first, so now, we can have a check for both > correct pairing _and_ duplicate ends in one: > > #define SYM_CHECK_START(name) \ > .pushsection .rodata.bubak ASM_NL \ > .long has_no_SYM_END_##name - . ASM_NL \ > .popsection > > #define SYM_CHECK_END(name) \ > has_no_SYM_END_##name: > > /* SYM_START -- use only if you have to */ > #ifndef SYM_START > #define SYM_START(name, align, visibility, entry) \ > SYM_CHECK_START(name) ASM_NL \ > visibility(name) ASM_NL \ > align ASM_NL \ > name: ASM_NL \ > entry > #endif > > /* SYM_END -- use only if you have to */ > #ifndef SYM_END > #define SYM_END(name, sym_type, exit) \ > exit ASM_NL \ > SYM_CHECK_END(name) ASM_NL \ > .type name sym_type ASM_NL \ > .size name, .-name > #endif > > > So for the ftrace mistake I did: > > AS arch/x86/kernel/mcount_64.o > /home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages: > /home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol > `has_no_SYM_END_ftrace_caller' is already defined > > > or if I remove SYM_END_FUNC completely: > LD vmlinux.o > MODPOST vmlinux.o > arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to > `has_no_SYM_END_ftrace_stub' > > > Sad is that this occurs only during linking, so I cannot put it in the > .discard section -- ideas? Ah, interesting idea but I can't think of a way to do the missing end check before link time. But it would be easy for objtool to check for a missing end because the symbol would have a zero size. -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-23 08:40 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <to5vJ-7s8-27@gated-at.bofh.it> |
| In reply to | #1606550 |
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote: > > > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > >> Hi! > > > >> > > > >>> -ENTRY(saved_rbp) .quad 0 > > > >>> -ENTRY(saved_rsi) .quad 0 > > > >>> -ENTRY(saved_rdi) .quad 0 > > > >>> -ENTRY(saved_rbx) .quad 0 > > > >>> +SYM_DATA_START(saved_rbp) .quad 0 > > > >>> +SYM_DATA_START(saved_rsi) .quad 0 > > > >>> +SYM_DATA_START(saved_rdi) .quad 0 > > > >>> +SYM_DATA_START(saved_rbx) .quad 0 > > > >> > > > >> Does it make sense to call it SYM_DATA_*START* when there's no > > > >> corresponding end? > > > > > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > > > SYM_*_START() uses? This could be done by emitting debug data into a special > > > > section and then analyzing that section for unpaired entries. The section can be > > > > discarded in the final link, it won't show up in the kernel image. > > > > > > It should be easier than that. No introduction of other info needed -- > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > > > be a bug now. > > > > I'm all for that! > > It would be easy to add this checking to objtool since it already reads > the symbol table. The hard part is figuring out the logistics. :-) > > - Should the warnings be on by default? Yes, if objtool is running. Keep it simple. > - Part of the "objtool check" command or something else? Yes - I think it's still within the 'object file check' functionality. > - Separate config option or just include it with > CONFIG_STACK_VALIDATION? Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As I predicted early on, objtool will go beyond stack checking! ;-) > - Should all asm files be checked, including those currently skipped by > objtool with OBJECT_FILES_NON_STANDARD? The symbol syntax check should definitely be for all files, yes. Could we perhaps emit 'non-standard stack frames' information into the .o itself (via a flag or a special section?), so that objtool can decide on its own whether to complain about any weirdnesses there? > > Can we detect double ends as well - i.e. do a build check of the full syntax of > > these symbol definition primitives? > > Detecting double ends would be a little trickier. The second SYM_*_END > supersedes the first, so that information isn't in the ELF symbol table. Indeed. > We could use a special section to annotate all the macro uses and have > objtool do the checking, similar to what you suggested earlier. That might be useful for other purposes as well - such as the non-standard stack frame annotations? But it's your call really: I'm principally fine with any of the solutions, as long as the checking is done. Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2017-03-23 14:30 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <toaYq-2KA-29@gated-at.bofh.it> |
| In reply to | #1607189 |
On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote: > > > > > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > > > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > > > > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > > > >> Hi! > > > > >> > > > > >>> -ENTRY(saved_rbp) .quad 0 > > > > >>> -ENTRY(saved_rsi) .quad 0 > > > > >>> -ENTRY(saved_rdi) .quad 0 > > > > >>> -ENTRY(saved_rbx) .quad 0 > > > > >>> +SYM_DATA_START(saved_rbp) .quad 0 > > > > >>> +SYM_DATA_START(saved_rsi) .quad 0 > > > > >>> +SYM_DATA_START(saved_rdi) .quad 0 > > > > >>> +SYM_DATA_START(saved_rbx) .quad 0 > > > > >> > > > > >> Does it make sense to call it SYM_DATA_*START* when there's no > > > > >> corresponding end? > > > > > > > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > > > > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > > > > SYM_*_START() uses? This could be done by emitting debug data into a special > > > > > section and then analyzing that section for unpaired entries. The section can be > > > > > discarded in the final link, it won't show up in the kernel image. > > > > > > > > It should be easier than that. No introduction of other info needed -- > > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > > > > be a bug now. > > > > > > I'm all for that! > > > > It would be easy to add this checking to objtool since it already reads > > the symbol table. The hard part is figuring out the logistics. :-) > > > > - Should the warnings be on by default? > > Yes, if objtool is running. Keep it simple. > > > - Part of the "objtool check" command or something else? > > Yes - I think it's still within the 'object file check' functionality. > > > - Separate config option or just include it with > > CONFIG_STACK_VALIDATION? > > Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or such. As > I predicted early on, objtool will go beyond stack checking! ;-) > > > - Should all asm files be checked, including those currently skipped by > > objtool with OBJECT_FILES_NON_STANDARD? > > The symbol syntax check should definitely be for all files, yes. That all sounds reasonable. I'll work something up. > Could we perhaps emit 'non-standard stack frames' information into the .o itself > (via a flag or a special section?), so that objtool can decide on its own whether > to complain about any weirdnesses there? For the OBJECT_FILES_NON_STANDARD case, where the whole file is "special", we can just provide a flag to "objtool check" to tell it to skip stack checking for that file, but still do the symbol checks. > > > Can we detect double ends as well - i.e. do a build check of the full syntax of > > > these symbol definition primitives? > > > > Detecting double ends would be a little trickier. The second SYM_*_END > > supersedes the first, so that information isn't in the ELF symbol table. > > Indeed. > > > We could use a special section to annotate all the macro uses and have > > objtool do the checking, similar to what you suggested earlier. > > That might be useful for other purposes as well - such as the non-standard stack > frame annotations? To start with we can try going without all the special sections (other than the SYM_END double end check). If we end up finding another case which isn't covered then we can always add the special sections later. -- Josh
[toc] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-03-22 13:10 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnNfs-2bK-17@gated-at.bofh.it> |
| In reply to | #1605635 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
On 03/21/2017, 03:08 PM, Pavel Machek wrote:
>> -ENTRY(saved_rbp) .quad 0
>> -ENTRY(saved_rsi) .quad 0
>> -ENTRY(saved_rdi) .quad 0
>> -ENTRY(saved_rbx) .quad 0
>> +SYM_DATA_START(saved_rbp) .quad 0
>> +SYM_DATA_START(saved_rsi) .quad 0
>> +SYM_DATA_START(saved_rdi) .quad 0
>> +SYM_DATA_START(saved_rbx) .quad 0
>
> Does it make sense to call it SYM_DATA_*START* when there's no
> corresponding end?
>
> Plus... it looks like saved_rsi (and friends) are only used inside
> wakeup_64.S. Could we just delete the "ENTRY" annotations?
So, now I have:
=== linkage.h ===
/* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */
#ifndef SYM_DATA_SIMPLE
#define SYM_DATA_SIMPLE(name, data) \
SYM_DATA_START(name) ASM_NL \
data ASM_NL \
SYM_DATA_END(name)
#endif
/* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */
#ifndef SYM_DATA_SIMPLE_LOCAL
#define SYM_DATA_SIMPLE_LOCAL(name, data) \
SYM_DATA_START_LOCAL(name) ASM_NL \
data ASM_NL \
SYM_DATA_END(name)
#endif
=== wakeup_64.S ===
SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0)
SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0)
=== original ===
10: 0000000000000060 0 NOTYPE GLOBAL DEFAULT 3 saved_magic
11: 0000000000000050 0 NOTYPE GLOBAL DEFAULT 3 saved_rsp
12: 0000000000000030 0 NOTYPE GLOBAL DEFAULT 3 saved_rbx
13: 0000000000000020 0 NOTYPE GLOBAL DEFAULT 3 saved_rdi
14: 0000000000000010 0 NOTYPE GLOBAL DEFAULT 3 saved_rsi
15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 3 saved_rbp
16: 0000000000000040 0 NOTYPE GLOBAL DEFAULT 3 saved_rip
=== new ===
4: 0000000000000030 8 OBJECT LOCAL DEFAULT 3 saved_magic
6: 0000000000000028 8 OBJECT LOCAL DEFAULT 3 saved_rsp
7: 0000000000000018 8 OBJECT LOCAL DEFAULT 3 saved_rbx
8: 0000000000000010 8 OBJECT LOCAL DEFAULT 3 saved_rdi
9: 0000000000000008 8 OBJECT LOCAL DEFAULT 3 saved_rsi
10: 0000000000000000 8 OBJECT LOCAL DEFAULT 3 saved_rbp
11: 0000000000000020 8 OBJECT LOCAL DEFAULT 3 saved_rip
=== EOF ===
BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original.
But I see no point aligning data like this.
thanks,
--
js
suse labs
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-03-22 17:00 +0100 |
| Subject | Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data |
| Message-ID | <tnQQ2-4OP-49@gated-at.bofh.it> |
| In reply to | #1606408 |
[Multipart message — attachments visible in raw view] — view raw
On Wed 2017-03-22 13:06:54, Jiri Slaby wrote: > Hi, > > On 03/21/2017, 03:08 PM, Pavel Machek wrote: > >> -ENTRY(saved_rbp) .quad 0 > >> -ENTRY(saved_rsi) .quad 0 > >> -ENTRY(saved_rdi) .quad 0 > >> -ENTRY(saved_rbx) .quad 0 > >> +SYM_DATA_START(saved_rbp) .quad 0 > >> +SYM_DATA_START(saved_rsi) .quad 0 > >> +SYM_DATA_START(saved_rdi) .quad 0 > >> +SYM_DATA_START(saved_rbx) .quad 0 > > > > Does it make sense to call it SYM_DATA_*START* when there's no > > corresponding end? > > > > Plus... it looks like saved_rsi (and friends) are only used inside > > wakeup_64.S. Could we just delete the "ENTRY" annotations? > > > So, now I have: > > === linkage.h === > > /* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */ > #ifndef SYM_DATA_SIMPLE > #define SYM_DATA_SIMPLE(name, data) \ > SYM_DATA_START(name) ASM_NL \ > data ASM_NL \ > SYM_DATA_END(name) > #endif > > /* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */ > #ifndef SYM_DATA_SIMPLE_LOCAL > #define SYM_DATA_SIMPLE_LOCAL(name, data) \ > SYM_DATA_START_LOCAL(name) ASM_NL \ > data ASM_NL \ > SYM_DATA_END(name) > #endif > > === wakeup_64.S === > > SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0) > > SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0) > > SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0) > > === original === > > 10: 0000000000000060 0 NOTYPE GLOBAL DEFAULT 3 saved_magic > 11: 0000000000000050 0 NOTYPE GLOBAL DEFAULT 3 saved_rsp > 12: 0000000000000030 0 NOTYPE GLOBAL DEFAULT 3 saved_rbx > 13: 0000000000000020 0 NOTYPE GLOBAL DEFAULT 3 saved_rdi > 14: 0000000000000010 0 NOTYPE GLOBAL DEFAULT 3 saved_rsi > 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 3 saved_rbp > 16: 0000000000000040 0 NOTYPE GLOBAL DEFAULT 3 saved_rip > > === new === > > 4: 0000000000000030 8 OBJECT LOCAL DEFAULT 3 saved_magic > 6: 0000000000000028 8 OBJECT LOCAL DEFAULT 3 saved_rsp > 7: 0000000000000018 8 OBJECT LOCAL DEFAULT 3 saved_rbx > 8: 0000000000000010 8 OBJECT LOCAL DEFAULT 3 saved_rdi > 9: 0000000000000008 8 OBJECT LOCAL DEFAULT 3 saved_rsi > 10: 0000000000000000 8 OBJECT LOCAL DEFAULT 3 saved_rbp > 11: 0000000000000020 8 OBJECT LOCAL DEFAULT 3 saved_rip > > === EOF === > > BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original. > But I see no point aligning data like this. Yep, that's a bug, too. I guess it hurts even more on 32-bit.... Thanks for fixing this, Pavel
[toc] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-03-20 13:40 +0100 |
| Subject | [RFC v2 10/10] x86: boot, extract efi_pe_entry from startup_64 |
| Message-ID | <tn4Lp-4J7-51@gated-at.bofh.it> |
| In reply to | #1602062 |
efi_pe_entry body is somehow squashed into startup_64. It makes the code less readable and illogical. Extract the inlined efi_pe_entry body from startup_64 into a separate function and mark it appropriatelly by SYM_FUNC_START+SYM_FUNC_END. ABI offsets are preserved: 0000000000000200 T startup_64 0000000000000280 T efi_pe_entry 00000000000002d0 t handover_entry 00000000000002fd t fail 0000000000000390 T efi64_stub_entry Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: <x86@kernel.org> --- arch/x86/boot/compressed/head_64.S | 112 ++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 59 deletions(-) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 6f037b3af204..30f3c1117243 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -243,65 +243,6 @@ ENTRY(startup_64) * that maps our entire kernel(text+data+bss+brk), zero page * and command line. */ -#ifdef CONFIG_EFI_STUB - /* - * The entry point for the PE/COFF executable is efi_pe_entry, so - * only legacy boot loaders will execute this jmp. - */ - jmp preferred_addr - -SYM_FUNC_INNER_LABEL(efi_pe_entry) - movq %rcx, efi64_config(%rip) /* Handle */ - movq %rdx, efi64_config+8(%rip) /* EFI System table pointer */ - - leaq efi64_config(%rip), %rax - movq %rax, efi_config(%rip) - - call 1f -1: popq %rbp - subq $1b, %rbp - - /* - * Relocate efi_config->call(). - */ - addq %rbp, efi64_config+40(%rip) - - movq %rax, %rdi - call make_boot_params - cmpq $0,%rax - je fail - mov %rax, %rsi - leaq startup_32(%rip), %rax - movl %eax, BP_code32_start(%rsi) - jmp 2f /* Skip the relocation */ - -handover_entry: - call 1f -1: popq %rbp - subq $1b, %rbp - - /* - * Relocate efi_config->call(). - */ - movq efi_config(%rip), %rax - addq %rbp, 40(%rax) -2: - movq efi_config(%rip), %rdi - call efi_main - movq %rax,%rsi - cmpq $0,%rax - jne 2f -fail: - /* EFI init failed, so hang. */ - hlt - jmp fail -2: - movl BP_code32_start(%esi), %eax - leaq preferred_addr(%rax), %rax - jmp *%rax - -preferred_addr: -#endif /* Setup data segments. */ xorl %eax, %eax @@ -372,6 +313,59 @@ preferred_addr: SYM_FUNC_END(startup_64) #ifdef CONFIG_EFI_STUB + +/* The entry point for the PE/COFF executable is efi_pe_entry. */ +SYM_FUNC_START(efi_pe_entry) + movq %rcx, efi64_config(%rip) /* Handle */ + movq %rdx, efi64_config+8(%rip) /* EFI System table pointer */ + + leaq efi64_config(%rip), %rax + movq %rax, efi_config(%rip) + + call 1f +1: popq %rbp + subq $1b, %rbp + + /* + * Relocate efi_config->call(). + */ + addq %rbp, efi64_config+40(%rip) + + movq %rax, %rdi + call make_boot_params + cmpq $0,%rax + je fail + mov %rax, %rsi + leaq startup_32(%rip), %rax + movl %eax, BP_code32_start(%rsi) + jmp 2f /* Skip the relocation */ + +handover_entry: + call 1f +1: popq %rbp + subq $1b, %rbp + + /* + * Relocate efi_config->call(). + */ + movq efi_config(%rip), %rax + addq %rbp, 40(%rax) +2: + movq efi_config(%rip), %rdi + call efi_main + movq %rax,%rsi + cmpq $0,%rax + jne 2f +fail: + /* EFI init failed, so hang. */ + hlt + jmp fail +2: + movl BP_code32_start(%esi), %eax + leaq startup_64(%rax), %rax + jmp *%rax +SYM_FUNC_END(efi_pe_entry) + .org 0x390 ENTRY(efi64_stub_entry) movq %rdi, efi64_config(%rip) /* Handle */ -- 2.12.0
[toc] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-03-20 13:40 +0100 |
| Subject | [PATCH v2 01/10] linkage: new macros for assembler symbols |
| Message-ID | <tn4Ln-4J7-5@gated-at.bofh.it> |
| In reply to | #1602062 |
Introduce new C macros for annotations of functions and data in assembly. There is a long-term mess in macros like ENTRY, END, ENDPROC and similar. They are used in different manners and sometimes incorrectly. So introduce macros with clear use to annotate assembly as follows: a) Support macros SYM_T_FUNC -- type used by assembler to mark functions SYM_T_OBJECT -- type used by assembler to mark data They are defined as STT_FUNC and STT_OBJECT respectively. According to the gas manual, this is the most portable way. I am not sure about other assemblers, so we can switch this back to %function and %object if this turns into a problem. Architectures can also override them by something like ", @function" if need be. SYM_A_ALIGN, SYM_A_NOALIGN -- should we align the symbol? SYM_V_GLOBAL, SYM_V_WEAK, SYM_V_LOCAL -- visibility of symbols b) Mostly internal annotations, used by the ones below SYM_START -- use only if you have to SYM_END -- use only if you have to c) Generic annotations d) Annotations for code SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for one code SYM_FUNC_START_ALIAS -- use where there are two global names for one code SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed code SYM_FUNC_START -- use for global functions SYM_FUNC_START_LOCAL -- use for local functions SYM_FUNC_START_WEAK -- use for weak functions SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START, SYM_FUNC_START_WEAK, ... SYM_FUNC_INNER_LABEL -- only for global labels in the middle of functions d) For data SYM_DATA_START -- global data symbol SYM_DATA_END -- the end of SYM_DATA_START symbol ========== Note that SYM_FUNC_START_WEAK aligns symbols now too. The macros allow to pair starts and ends of functions and mark function correctly in the output ELF objects. This will also help a lot to generate DWARF information automatically during build of asm. Finally, all users of the old macros will be converted to use these later. [v2] * use SYM_ prefix and sane names * add SYM_START and SYM_END and parametrize all the macros Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: hpa@zytor.com Cc: Ingo Molnar <mingo@kernel.org> Cc: jpoimboe@redhat.com Cc: Juergen Gross <jgross@suse.com> Cc: Len Brown <len.brown@intel.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org Cc: mingo@redhat.com Cc: Pavel Machek <pavel@ucw.cz> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: xen-devel@lists.xenproject.org Cc: x86@kernel.org --- arch/x86/include/asm/linkage.h | 5 +- include/linux/linkage.h | 131 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 126 insertions(+), 10 deletions(-) diff --git a/arch/x86/include/asm/linkage.h b/arch/x86/include/asm/linkage.h index 0ccb26dda126..a96f6fc36011 100644 --- a/arch/x86/include/asm/linkage.h +++ b/arch/x86/include/asm/linkage.h @@ -12,9 +12,8 @@ #ifdef __ASSEMBLY__ -#define GLOBAL(name) \ - .globl name; \ - name: +/* deprecated, use SYM_DATA_START, SYM_FUNC_START, or SYM_FUNC_INNER_LABEL */ +#define GLOBAL(name) SYM_DATA_START(name) #if defined(CONFIG_X86_64) || defined(CONFIG_X86_ALIGNMENT_16) #define __ALIGN .p2align 4, 0x90 diff --git a/include/linux/linkage.h b/include/linux/linkage.h index a6a42dd02466..c1dc824d2bc6 100644 --- a/include/linux/linkage.h +++ b/include/linux/linkage.h @@ -74,25 +74,46 @@ #ifdef __ASSEMBLY__ +/* SYM_T_FUNC -- type used by assembler to mark functions */ +#ifndef SYM_T_FUNC +#define SYM_T_FUNC STT_FUNC +#endif + +/* SYM_T_OBJECT -- type used by assembler to mark data */ +#ifndef SYM_T_OBJECT +#define SYM_T_OBJECT STT_OBJECT +#endif + +/* SYM_A_* -- should we align the symbol? */ +#define SYM_A_ALIGN ALIGN +#define SYM_A_NOALIGN /* nothing */ + +/* SYM_V_* -- visibility of symbols */ +#define SYM_V_GLOBAL(name) .globl name +#define SYM_V_WEAK(name) .weak name +#define SYM_V_LOCAL(name) /* nothing */ + #ifndef LINKER_SCRIPT #define ALIGN __ALIGN #define ALIGN_STR __ALIGN_STR +/* === DEPRECATED annotations === */ + #ifndef ENTRY +/* deprecated, use SYM_FUNC_START */ #define ENTRY(name) \ - .globl name ASM_NL \ - ALIGN ASM_NL \ - name: + SYM_FUNC_START(name) #endif #endif /* LINKER_SCRIPT */ #ifndef WEAK +/* deprecated, use SYM_FUNC_START_WEAK */ #define WEAK(name) \ - .weak name ASM_NL \ - name: + SYM_FUNC_START_WEAK(name) #endif #ifndef END +/* deprecated, use SYM_FUNC_END, SYM_DATA_END, or SYM_END */ #define END(name) \ .size name, .-name #endif @@ -102,9 +123,105 @@ * static analysis tools such as stack depth analyzer. */ #ifndef ENDPROC +/* deprecated, use SYM_FUNC_END */ #define ENDPROC(name) \ - .type name, @function ASM_NL \ - END(name) + SYM_FUNC_END(name) +#endif + +/* === generic annotations === */ + +/* SYM_START -- use only if you have to */ +#ifndef SYM_START +#define SYM_START(name, align, visibility) \ + visibility(name) ASM_NL \ + align ASM_NL \ + name: +#endif + +/* SYM_END -- use only if you have to */ +#ifndef SYM_END +#define SYM_END(name, sym_type) \ + .type name sym_type ASM_NL \ + .size name, .-name +#endif + +/* === code annotations === */ + +/* SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for one code */ +#ifndef SYM_FUNC_START_LOCAL_ALIAS +#define SYM_FUNC_START_LOCAL_ALIAS(name) \ + SYM_START(name, SYM_A_ALIGN, SYM_V_LOCAL) +#endif + +/* SYM_FUNC_START_ALIAS -- use where there are two global names for one code */ +#ifndef SYM_FUNC_START_ALIAS +#define SYM_FUNC_START_ALIAS(name) \ + SYM_START(name, SYM_A_ALIGN, SYM_V_GLOBAL) +#endif + +/* SYM_FUNC_START -- use for global functions */ +#ifndef SYM_FUNC_START +/* + * The same as SYM_FUNC_START_ALIAS, but we will need to distinguish these two + * later. + */ +#define SYM_FUNC_START(name) \ + SYM_START(name, SYM_A_ALIGN, SYM_V_GLOBAL) +#endif + +/* SYM_FUNC_START_LOCAL -- use for local functions */ +#ifndef SYM_FUNC_START_LOCAL +/* the same as SYM_FUNC_START_LOCAL_ALIAS, see comment near SYM_FUNC_START */ +#define SYM_FUNC_START_LOCAL(name) \ + SYM_START(name, SYM_A_ALIGN, SYM_V_LOCAL) +#endif + +/* SYM_FUNC_START_WEAK -- use for weak functions */ +#ifndef SYM_FUNC_START_WEAK +#define SYM_FUNC_START_WEAK(name) \ + SYM_START(name, SYM_A_ALIGN, SYM_V_WEAK) +#endif + +/* SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed code */ +#ifndef SYM_FUNC_END_ALIAS +#define SYM_FUNC_END_ALIAS(name) \ + SYM_END(name, SYM_T_FUNC) +#endif + +/* + * SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START, + * SYM_FUNC_START_WEAK, ... + */ +#ifndef SYM_FUNC_END +/* the same as SYM_FUNC_END_ALIAS, see comment near SYM_FUNC_START */ +#define SYM_FUNC_END(name) \ + SYM_END(name, SYM_T_FUNC) +#endif + +/* SYM_FUNC_INNER_LABEL -- only for global labels to the middle of functions */ +#ifndef SYM_FUNC_INNER_LABEL +#define SYM_FUNC_INNER_LABEL(name) \ + SYM_START(name, SYM_A_NOALIGN, SYM_V_GLOBAL) +#endif + +/* === data annotations === */ + +/* SYM_DATA_START -- global data symbol */ +#ifndef SYM_DATA_START +#define SYM_DATA_START(name) \ + SYM_START(name, SYM_A_NOALIGN, SYM_V_GLOBAL) +#endif + +/* SYM_DATA_START -- local data symbol */ +#ifndef SYM_DATA_START_LOCAL +#define SYM_DATA_START_LOCAL(name) \ + SYM_START(name, SYM_A_NOALIGN, SYM_V_LOCAL) +#endif + +/* SYM_DATA_END -- the end of SYM_DATA_START symbol */ +#ifndef SYM_DATA_END +#define SYM_DATA_END(name) \ + SYM_END(name, SYM_T_OBJECT) #endif #endif -- 2.12.0
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web