Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf  wrote:
> 
> > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > > 
> > > * Jiri Slaby  wrote:
> > > 
> > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > > 
> > > > > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf  wrote:
> 
> > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > > 
> > > * Jiri Slaby  wrote:
> > > 
> > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > > 
> > > > > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Josh Poimboeuf  wrote:

> On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > 
> > * Jiri Slaby  wrote:
> > 
> > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > 
> > > > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Josh Poimboeuf  wrote:

> On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > 
> > * Jiri Slaby  wrote:
> > 
> > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > 
> > > > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

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: 0060 0 NOTYPE  GLOBAL DEFAULT3 saved_magic
> 11: 0050 0 NOTYPE  GLOBAL DEFAULT3 saved_rsp
> 12: 0030 0 NOTYPE  GLOBAL DEFAULT3 saved_rbx
> 13: 0020 0 NOTYPE  GLOBAL DEFAULT3 saved_rdi
> 14: 0010 0 NOTYPE  GLOBAL DEFAULT3 saved_rsi
> 15:  0 NOTYPE  GLOBAL DEFAULT3 saved_rbp
> 16: 0040 0 NOTYPE  GLOBAL DEFAULT3 saved_rip
> 
> === new ===
> 
>  4: 0030 8 OBJECT  LOCAL  DEFAULT3 saved_magic
>  6: 0028 8 OBJECT  LOCAL  DEFAULT3 saved_rsp
>  7: 0018 8 OBJECT  LOCAL  DEFAULT3 saved_rbx
>  8: 0010 8 OBJECT  LOCAL  DEFAULT3 saved_rdi
>  9: 0008 8 OBJECT  LOCAL  DEFAULT3 saved_rsi
> 10:  8 OBJECT  LOCAL  DEFAULT3 saved_rbp
> 11: 0020 8 OBJECT  LOCAL  DEFAULT3 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


signature.asc
Description: Digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

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: 0060 0 NOTYPE  GLOBAL DEFAULT3 saved_magic
> 11: 0050 0 NOTYPE  GLOBAL DEFAULT3 saved_rsp
> 12: 0030 0 NOTYPE  GLOBAL DEFAULT3 saved_rbx
> 13: 0020 0 NOTYPE  GLOBAL DEFAULT3 saved_rdi
> 14: 0010 0 NOTYPE  GLOBAL DEFAULT3 saved_rsi
> 15:  0 NOTYPE  GLOBAL DEFAULT3 saved_rbp
> 16: 0040 0 NOTYPE  GLOBAL DEFAULT3 saved_rip
> 
> === new ===
> 
>  4: 0030 8 OBJECT  LOCAL  DEFAULT3 saved_magic
>  6: 0028 8 OBJECT  LOCAL  DEFAULT3 saved_rsp
>  7: 0018 8 OBJECT  LOCAL  DEFAULT3 saved_rbx
>  8: 0010 8 OBJECT  LOCAL  DEFAULT3 saved_rdi
>  9: 0008 8 OBJECT  LOCAL  DEFAULT3 saved_rsi
> 10:  8 OBJECT  LOCAL  DEFAULT3 saved_rbp
> 11: 0020 8 OBJECT  LOCAL  DEFAULT3 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


signature.asc
Description: Digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> 
> * Jiri Slaby  wrote:
> 
> > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > 
> > > * Pavel Machek  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_ENDpreempt_schedule' is 
already defined
  arch/x86/entry/thunk_64.S:52: Error: symbol 
`SYM_ENDpreempt_schedule_notrace' is already defined
  scripts/Makefile.build:395: recipe for target 'arch/x86/entry/thunk_64.o' 
failed

-- 
Josh

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> 
> * Jiri Slaby  wrote:
> 
> > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > 
> > > * Pavel Machek  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_ENDpreempt_schedule' is 
already defined
  arch/x86/entry/thunk_64.S:52: Error: symbol 
`SYM_ENDpreempt_schedule_notrace' is already defined
  scripts/Makefile.build:395: recipe for target 'arch/x86/entry/thunk_64.o' 
failed

-- 
Josh

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

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: 0060 0 NOTYPE  GLOBAL DEFAULT3 saved_magic
11: 0050 0 NOTYPE  GLOBAL DEFAULT3 saved_rsp
12: 0030 0 NOTYPE  GLOBAL DEFAULT3 saved_rbx
13: 0020 0 NOTYPE  GLOBAL DEFAULT3 saved_rdi
14: 0010 0 NOTYPE  GLOBAL DEFAULT3 saved_rsi
15:  0 NOTYPE  GLOBAL DEFAULT3 saved_rbp
16: 0040 0 NOTYPE  GLOBAL DEFAULT3 saved_rip

=== new ===

 4: 0030 8 OBJECT  LOCAL  DEFAULT3 saved_magic
 6: 0028 8 OBJECT  LOCAL  DEFAULT3 saved_rsp
 7: 0018 8 OBJECT  LOCAL  DEFAULT3 saved_rbx
 8: 0010 8 OBJECT  LOCAL  DEFAULT3 saved_rdi
 9: 0008 8 OBJECT  LOCAL  DEFAULT3 saved_rsi
10:  8 OBJECT  LOCAL  DEFAULT3 saved_rbp
11: 0020 8 OBJECT  LOCAL  DEFAULT3 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



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

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: 0060 0 NOTYPE  GLOBAL DEFAULT3 saved_magic
11: 0050 0 NOTYPE  GLOBAL DEFAULT3 saved_rsp
12: 0030 0 NOTYPE  GLOBAL DEFAULT3 saved_rbx
13: 0020 0 NOTYPE  GLOBAL DEFAULT3 saved_rdi
14: 0010 0 NOTYPE  GLOBAL DEFAULT3 saved_rsi
15:  0 NOTYPE  GLOBAL DEFAULT3 saved_rbp
16: 0040 0 NOTYPE  GLOBAL DEFAULT3 saved_rip

=== new ===

 4: 0030 8 OBJECT  LOCAL  DEFAULT3 saved_magic
 6: 0028 8 OBJECT  LOCAL  DEFAULT3 saved_rsp
 7: 0018 8 OBJECT  LOCAL  DEFAULT3 saved_rbx
 8: 0010 8 OBJECT  LOCAL  DEFAULT3 saved_rdi
 9: 0008 8 OBJECT  LOCAL  DEFAULT3 saved_rsi
10:  8 OBJECT  LOCAL  DEFAULT3 saved_rbp
11: 0020 8 OBJECT  LOCAL  DEFAULT3 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



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Jiri Slaby  wrote:

> On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > 
> > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Jiri Slaby  wrote:

> On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > 
> > * Pavel Machek  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

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> 
> * Pavel Machek  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.

> We don't ever nest symbols, right?

AFAI could see so far, correct.

>> Plus... it looks like saved_rsi (and friends) are only used inside
>> wakeup_64.S. Could we just delete the "ENTRY" annotations?
> 
> That appears to make sense as well.

+1, will fix this.

thanks,
-- 
js
suse labs

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> 
> * Pavel Machek  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.

> We don't ever nest symbols, right?

AFAI could see so far, correct.

>> Plus... it looks like saved_rsi (and friends) are only used inside
>> wakeup_64.S. Could we just delete the "ENTRY" annotations?
> 
> That appears to make sense as well.

+1, will fix this.

thanks,
-- 
js
suse labs

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Pavel Machek  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.

We don't ever nest symbols, right?

> Plus... it looks like saved_rsi (and friends) are only used inside
> wakeup_64.S. Could we just delete the "ENTRY" annotations?

That appears to make sense as well.

Thanks,

Ingo

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Ingo Molnar]

* Pavel Machek  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.

We don't ever nest symbols, right?

> Plus... it looks like saved_rsi (and friends) are only used inside
> wakeup_64.S. Could we just delete the "ENTRY" annotations?

That appears to make sense as well.

Thanks,

Ingo

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

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?

Plus... it looks like saved_rsi (and friends) are only used inside
wakeup_64.S. Could we just delete the "ENTRY" annotations?

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Pavel Machek]

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?

Plus... it looks like saved_rsi (and friends) are only used inside
wakeup_64.S. Could we just delete the "ENTRY" annotations?

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 04:32:09PM +0100, Jiri Slaby wrote:
> On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> >> This is a start of series to cleanup macros used for starting functions,
> >> data, globals etc. across x86. When we have all this sorted out, this
> >> will help to inject DWARF unwinding info by objtool later.
> >>
> >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> > 
> > Do we still want to emit .cfi_startproc/endproc from the macro?  From
> > our last discussion, that seemed to be up in the air.
> > 
> >   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble
> 
> "Automatically at best" above means "completely from objtool". I am
> still uncertain whether it will work 100% or we would have to help by
> generating some pieces from the added macros. In particular, the ALIASes
> are evil which cause harm here:
> 
> fun_alias:
> fun:
> 
> .size fun, .-fun
> .type fun STT_FUNC
> .size fun_alias, .-fun_alias
> .type fun_alias STT_FUNC
> 
> Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
> shall.
> 
> But it seems so far, that we might be able to deal with all of that from
> objtool... (I have not been thinking about this particular thing deep
> enough yet.) Some sort of "from the last label that is marked as
> STT_FUNC till its .size" might work.

Ok.

> > What did you think about making CFI read-only for .c object files and
> > write-only for .S object files?
> 
> There are those functions like sync_core() or native_save_fl() with
> inline asm. And they seem to need a) read-write support, or b) manual
> annotation. I would like to avoid b) for sure.

Ah, so I guess those inline asm functions cause problems because they
muck with the stack pointer with pushes and pops?

I don't think manual annotation of inline asm would be so bad.  IIUC, it
would only mean replacing the pushes and pops with a macro which does
the CFI-annotated version, like PUSH_CFI and POP_CFI.  And the benefit
would be that objtool doesn't have to try to rewrite a bunch of .c
object files.

Objtool read-write worries me because it gives more responsibility to
objtool.  It could be tricky to insert CFI instructions within the ones
already created by gcc.  Also, while unlikely, a bug in objtool could
theoretically corrupt an object file and brick the kernel.  Also I
wonder how all those extra file writes would affect build performance.

If at all possible, I would rather objtool stay out of the way of the
compiler and let gcc do its job of generating CFI.

-- 
Josh

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 04:32:09PM +0100, Jiri Slaby wrote:
> On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> >> This is a start of series to cleanup macros used for starting functions,
> >> data, globals etc. across x86. When we have all this sorted out, this
> >> will help to inject DWARF unwinding info by objtool later.
> >>
> >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> > 
> > Do we still want to emit .cfi_startproc/endproc from the macro?  From
> > our last discussion, that seemed to be up in the air.
> > 
> >   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble
> 
> "Automatically at best" above means "completely from objtool". I am
> still uncertain whether it will work 100% or we would have to help by
> generating some pieces from the added macros. In particular, the ALIASes
> are evil which cause harm here:
> 
> fun_alias:
> fun:
> 
> .size fun, .-fun
> .type fun STT_FUNC
> .size fun_alias, .-fun_alias
> .type fun_alias STT_FUNC
> 
> Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
> shall.
> 
> But it seems so far, that we might be able to deal with all of that from
> objtool... (I have not been thinking about this particular thing deep
> enough yet.) Some sort of "from the last label that is marked as
> STT_FUNC till its .size" might work.

Ok.

> > What did you think about making CFI read-only for .c object files and
> > write-only for .S object files?
> 
> There are those functions like sync_core() or native_save_fl() with
> inline asm. And they seem to need a) read-write support, or b) manual
> annotation. I would like to avoid b) for sure.

Ah, so I guess those inline asm functions cause problems because they
muck with the stack pointer with pushes and pops?

I don't think manual annotation of inline asm would be so bad.  IIUC, it
would only mean replacing the pushes and pops with a macro which does
the CFI-annotated version, like PUSH_CFI and POP_CFI.  And the benefit
would be that objtool doesn't have to try to rewrite a bunch of .c
object files.

Objtool read-write worries me because it gives more responsibility to
objtool.  It could be tricky to insert CFI instructions within the ones
already created by gcc.  Also, while unlikely, a bug in objtool could
theoretically corrupt an object file and brick the kernel.  Also I
wonder how all those extra file writes would affect build performance.

If at all possible, I would rather objtool stay out of the way of the
compiler and let gcc do its job of generating CFI.

-- 
Josh

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
>> This is a start of series to cleanup macros used for starting functions,
>> data, globals etc. across x86. When we have all this sorted out, this
>> will help to inject DWARF unwinding info by objtool later.
>>
>> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
>> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> 
> Do we still want to emit .cfi_startproc/endproc from the macro?  From
> our last discussion, that seemed to be up in the air.
> 
>   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

"Automatically at best" above means "completely from objtool". I am
still uncertain whether it will work 100% or we would have to help by
generating some pieces from the added macros. In particular, the ALIASes
are evil which cause harm here:

fun_alias:
fun:

.size fun, .-fun
.type fun STT_FUNC
.size fun_alias, .-fun_alias
.type fun_alias STT_FUNC

Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
shall.

But it seems so far, that we might be able to deal with all of that from
objtool... (I have not been thinking about this particular thing deep
enough yet.) Some sort of "from the last label that is marked as
STT_FUNC till its .size" might work.

> What did you think about making CFI read-only for .c object files and
> write-only for .S object files?

There are those functions like sync_core() or native_save_fl() with
inline asm. And they seem to need a) read-write support, or b) manual
annotation. I would like to avoid b) for sure.

thanks,
-- 
js
suse labs

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
>> This is a start of series to cleanup macros used for starting functions,
>> data, globals etc. across x86. When we have all this sorted out, this
>> will help to inject DWARF unwinding info by objtool later.
>>
>> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
>> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> 
> Do we still want to emit .cfi_startproc/endproc from the macro?  From
> our last discussion, that seemed to be up in the air.
> 
>   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

"Automatically at best" above means "completely from objtool". I am
still uncertain whether it will work 100% or we would have to help by
generating some pieces from the added macros. In particular, the ALIASes
are evil which cause harm here:

fun_alias:
fun:

.size fun, .-fun
.type fun STT_FUNC
.size fun_alias, .-fun_alias
.type fun_alias STT_FUNC

Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
shall.

But it seems so far, that we might be able to deal with all of that from
objtool... (I have not been thinking about this particular thing deep
enough yet.) Some sort of "from the last label that is marked as
STT_FUNC till its .size" might work.

> What did you think about making CFI read-only for .c object files and
> write-only for .S object files?

There are those functions like sync_core() or native_save_fl() with
inline asm. And they seem to need a) read-write support, or b) manual
annotation. I would like to avoid b) for sure.

thanks,
-- 
js
suse labs

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> This is a start of series to cleanup macros used for starting functions,
> data, globals etc. across x86. When we have all this sorted out, this
> will help to inject DWARF unwinding info by objtool later.
> 
> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

Do we still want to emit .cfi_startproc/endproc from the macro?  From
our last discussion, that seemed to be up in the air.

  https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

What did you think about making CFI read-only for .c object files and
write-only for .S object files?

-- 
Josh

Re: [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Josh Poimboeuf]

On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> This is a start of series to cleanup macros used for starting functions,
> data, globals etc. across x86. When we have all this sorted out, this
> will help to inject DWARF unwinding info by objtool later.
> 
> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

Do we still want to emit .cfi_startproc/endproc from the macro?  From
our last discussion, that seemed to be up in the air.

  https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

What did you think about making CFI read-only for .c object files and
write-only for .S object files?

-- 
Josh

[PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

This is a start of series to cleanup macros used for starting functions,
data, globals etc. across x86. When we have all this sorted out, this
will help to inject DWARF unwinding info by objtool later.

The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

This particular patch makes proper use of SYM_DATA_START on data and
SYM_FUNC_START on a function which was not the case on 4 locations.

Signed-off-by: Jiri Slaby 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Reviewed-by: Juergen Gross  [xen parts]
Cc: 
Cc: "Rafael J. Wysocki" 
Cc: Len Brown 
Cc: Pavel Machek 
Cc: 
---
 arch/x86/entry/entry_64_compat.S |  3 +--
 arch/x86/kernel/acpi/wakeup_64.S | 14 +++---
 arch/x86/kernel/head_64.S|  2 +-
 arch/x86/kernel/mcount_64.S  |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index e1721dafbcb1..73d7ff0b125c 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -342,8 +342,7 @@ ENTRY(entry_INT80_compat)
jmp restore_regs_and_iret
 END(entry_INT80_compat)
 
-   ALIGN
-GLOBAL(stub32_clone)
+SYM_FUNC_START(stub32_clone)
/*
 * The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr).
 * The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val).
diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index 50b8ed0317a3..0b5a5573f57d 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S
@@ -125,12 +125,12 @@ ENTRY(do_suspend_lowlevel)
 ENDPROC(do_suspend_lowlevel)
 
 .data
-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
 
-ENTRY(saved_rip)   .quad   0
-ENTRY(saved_rsp)   .quad   0
+SYM_DATA_START(saved_rip)  .quad   0
+SYM_DATA_START(saved_rsp)  .quad   0
 
-ENTRY(saved_magic) .quad   0
+SYM_DATA_START(saved_magic).quad   0
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ac9d327d2e42..e093a804f1fb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -487,7 +487,7 @@ early_gdt_descr:
 early_gdt_descr_base:
.quad   INIT_PER_CPU_VAR(gdt_page)
 
-ENTRY(phys_base)
+SYM_DATA_START(phys_base)
/* This must match the first entry in level2_kernel_pgt */
.quad   0x
 EXPORT_SYMBOL(phys_base)
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index 7b0d3da52fb4..2b4d7045e823 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -320,7 +320,7 @@ ENTRY(ftrace_graph_caller)
retq
 END(ftrace_graph_caller)
 
-GLOBAL(return_to_handler)
+SYM_FUNC_START(return_to_handler)
subq  $24, %rsp
 
/* Save the return values */
-- 
2.12.0

[PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

[Jiri Slaby]

This is a start of series to cleanup macros used for starting functions,
data, globals etc. across x86. When we have all this sorted out, this
will help to inject DWARF unwinding info by objtool later.

The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

This particular patch makes proper use of SYM_DATA_START on data and
SYM_FUNC_START on a function which was not the case on 4 locations.

Signed-off-by: Jiri Slaby 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Reviewed-by: Juergen Gross  [xen parts]
Cc: 
Cc: "Rafael J. Wysocki" 
Cc: Len Brown 
Cc: Pavel Machek 
Cc: 
---
 arch/x86/entry/entry_64_compat.S |  3 +--
 arch/x86/kernel/acpi/wakeup_64.S | 14 +++---
 arch/x86/kernel/head_64.S|  2 +-
 arch/x86/kernel/mcount_64.S  |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index e1721dafbcb1..73d7ff0b125c 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -342,8 +342,7 @@ ENTRY(entry_INT80_compat)
jmp restore_regs_and_iret
 END(entry_INT80_compat)
 
-   ALIGN
-GLOBAL(stub32_clone)
+SYM_FUNC_START(stub32_clone)
/*
 * The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr).
 * The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val).
diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index 50b8ed0317a3..0b5a5573f57d 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S
@@ -125,12 +125,12 @@ ENTRY(do_suspend_lowlevel)
 ENDPROC(do_suspend_lowlevel)
 
 .data
-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
 
-ENTRY(saved_rip)   .quad   0
-ENTRY(saved_rsp)   .quad   0
+SYM_DATA_START(saved_rip)  .quad   0
+SYM_DATA_START(saved_rsp)  .quad   0
 
-ENTRY(saved_magic) .quad   0
+SYM_DATA_START(saved_magic).quad   0
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ac9d327d2e42..e093a804f1fb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -487,7 +487,7 @@ early_gdt_descr:
 early_gdt_descr_base:
.quad   INIT_PER_CPU_VAR(gdt_page)
 
-ENTRY(phys_base)
+SYM_DATA_START(phys_base)
/* This must match the first entry in level2_kernel_pgt */
.quad   0x
 EXPORT_SYMBOL(phys_base)
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index 7b0d3da52fb4..2b4d7045e823 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -320,7 +320,7 @@ ENTRY(ftrace_graph_caller)
retq
 END(ftrace_graph_caller)
 
-GLOBAL(return_to_handler)
+SYM_FUNC_START(return_to_handler)
subq  $24, %rsp
 
/* Save the return values */
-- 
2.12.0