Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631093 > unrolled thread

Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

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.

Contents

  Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Josh Poimboeuf <jpoimboe@redhat.com> - 2017-04-26 03:50 +0200
    Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Jiri Slaby <jslaby@suse.cz> - 2017-05-12 10:00 +0200
      Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-13 00:20 +0200
        Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Jiri Slaby <jslaby@suse.cz> - 2017-05-17 15:30 +0200
          Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Jiri Slaby <jslaby@suse.cz> - 2017-05-19 11:20 +0200
            Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-19 22:00 +0200

#1631093 — Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

On Fri, Apr 21, 2017 at 04:12:40PM +0200, Jiri Slaby wrote:
> Somewhere END was used to end a function. It is not intended to be used
> for functions, because it does not mark the actual symbols as functions.
> Use ENDPROC in such cases which does the right job.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Reviewed-by: Juergen Gross <jgross@suse.com> [xen parts]
> Cc: <xen-devel@lists.xenproject.org>
> ---
>  arch/x86/entry/entry_32.S        | 58 ++++++++++++++++++++--------------------
>  arch/x86/entry/entry_64.S        | 40 +++++++++++++--------------
>  arch/x86/entry/entry_64_compat.S |  4 +--
>  arch/x86/kernel/ftrace_32.S      |  8 +++---
>  arch/x86/kernel/ftrace_64.S      | 10 +++----
>  arch/x86/xen/xen-pvh.S           |  2 +-
>  6 files changed, 61 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 50bc26949e9e..a546b84aec01 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -249,7 +249,7 @@ ENTRY(__switch_to_asm)
>  	popl	%ebp
>  
>  	jmp	__switch_to
> -END(__switch_to_asm)
> +ENDPROC(__switch_to_asm)
>  
>  /*
>   * A newly forked process directly context switches into this address.
> @@ -289,7 +289,7 @@ ENTRY(ret_from_fork)
>  	 */
>  	movl	$0, PT_EAX(%esp)
>  	jmp	2b
> -END(ret_from_fork)
> +ENDPROC(ret_from_fork)
>  
>  /*
>   * Return to user mode is not as complex as all this looks,
> @@ -323,7 +323,7 @@ ENTRY(resume_userspace)
>  	movl	%esp, %eax
>  	call	prepare_exit_to_usermode
>  	jmp	restore_all
> -END(ret_from_exception)
> +ENDPROC(ret_from_exception)

What exactly is the motivation of this patch?  It would be good to
describe that in the commit message.

Is the point to allow objtool to generate CFI for it?  If so, I don't
really see how that would work.  Today, objtool considers ENDPROC to
annotate a *callable* function which conforms to the C calling ABI and
can be called by another function.  The stack is in a known state at
function entry, and so the CFI (or frame pointer info) can be reliably
determined.

But entry code is different.  In most cases, the global symbols aren't
actually called, and they don't follow any conventions.  The code is
spaghetti-esque, with HW handlers and jumps everywhere.  The state of
the stack at symbol entry varies per "function".  That's why objtool
ignores these files.

For special cases (like entry code), I was thinking we'd need manual CFI
annotations, like we had before.  Or maybe there's another way, like
some new macros which tell objtool about the HW entry points and the
state of the registers there.

But I'm having trouble seeing how marking these code snippets with
ENTRY/ENDPROC would help objtool make any sense of the code and where
things are on the stack.

-- 
Josh

[toc] | [next] | [standalone]

#1640278

On 04/26/2017, 03:42 AM, Josh Poimboeuf wrote:
>> @@ -323,7 +323,7 @@ ENTRY(resume_userspace)
>>  	movl	%esp, %eax
>>  	call	prepare_exit_to_usermode
>>  	jmp	restore_all
>> -END(ret_from_exception)
>> +ENDPROC(ret_from_exception)
> 
> What exactly is the motivation of this patch?  It would be good to
> describe that in the commit message.
> 
> Is the point to allow objtool to generate CFI for it?  If so, I don't
> really see how that would work.  Today, objtool considers ENDPROC to
> annotate a *callable* function which conforms to the C calling ABI and
> can be called by another function.  The stack is in a known state at
> function entry, and so the CFI (or frame pointer info) can be reliably
> determined.

Ugh, I haven't checked this in 100 % of cases, but this looks pretty
fragile to me. From reading the code, the use of END or ENDPROC is
rather random -- depending on mood and who wrote the code.

> But entry code is different.  In most cases, the global symbols aren't
> actually called, and they don't follow any conventions.  The code is
> spaghetti-esque, with HW handlers and jumps everywhere.  The state of
> the stack at symbol entry varies per "function".  That's why objtool
> ignores these files.

Unfortunately, this is true.

> For special cases (like entry code), I was thinking we'd need manual CFI
> annotations, like we had before.  Or maybe there's another way, like
> some new macros which tell objtool about the HW entry points and the
> state of the registers there.
> 
> But I'm having trouble seeing how marking these code snippets with
> ENTRY/ENDPROC would help objtool make any sense of the code and where
> things are on the stack.

Ok, my intention was to have every line of assembly code in between of
FUNC_START/FUNC_END. That way, every rsp related push/pop/sub/add can be
annotated very easily. For the C-like functions this is all what needs
to be done.

Then there is the spaghetti code. And I was thinking about manual
annotations like:

  # skip the frame pointer checking between START+END here
  OBJTOOL(SKIP_CHECKING)

  # this fn has unusual frame (like interrupts have),
    and you can find return RIP stored at fp + 0x20
  OBJTOOL(RIP_IS_AT, 0x20)

  # put this raw CFI for this location into eh_frame
  OBJTOOL(RAW_CFI, 0x00, 0x00, 0x00)


Similarly, I have OBJTOOL(START_FUNC) and OBJTOOL(END_FUNC) emitted with
each FUNC_START/FUNC_END. So far, when manually expanded for simplicity,
it looks like this:

#define OBJTOOL_START_FUNC                              \
        .pushsection .discard.asmfunctions ASM_NL       \
        .long 0xfd111111 ASM_NL                         \
        .long 1f - . ASM_NL                             \
        .popsection ASM_NL                              \
        1:

#define OBJTOOL_END_FUNC                                \
        .pushsection .discard.asmfunctions ASM_NL       \
        .long 0xfe111111 ASM_NL                         \
        .long 1f - . ASM_NL                             \
        .popsection ASM_NL                              \
        1:

0xfd111111, 0xfe111111 are "opcodes" for objtool meaning
START_FUNC/END_FUNC. Similar would be SKIP_CHECKING, RIP_IS_AT, and
RAW_CFI from the above.

So on the objtool side, it looks like:
        switch (data->magic) {
        case 0xfd111111:
                pc_begin = rela->addend;
                break;
        case 0xfe111111:
                ret = dwarf_annotate_func(dwarf, rela->sym->sec,
                                pc_begin, rela->addend - pc_begin);
                if (ret < 0)
                        return -1;

                break;

So this was my idea -- having all code marked as function and manually
annotate those which are different.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1640785

On Fri, May 12, 2017 at 09:53:48AM +0200, Jiri Slaby wrote:
> On 04/26/2017, 03:42 AM, Josh Poimboeuf wrote:
> >> @@ -323,7 +323,7 @@ ENTRY(resume_userspace)
> >>  	movl	%esp, %eax
> >>  	call	prepare_exit_to_usermode
> >>  	jmp	restore_all
> >> -END(ret_from_exception)
> >> +ENDPROC(ret_from_exception)
> > 
> > What exactly is the motivation of this patch?  It would be good to
> > describe that in the commit message.
> > 
> > Is the point to allow objtool to generate CFI for it?  If so, I don't
> > really see how that would work.  Today, objtool considers ENDPROC to
> > annotate a *callable* function which conforms to the C calling ABI and
> > can be called by another function.  The stack is in a known state at
> > function entry, and so the CFI (or frame pointer info) can be reliably
> > determined.
> 
> Ugh, I haven't checked this in 100 % of cases, but this looks pretty
> fragile to me. From reading the code, the use of END or ENDPROC is
> rather random -- depending on mood and who wrote the code.

Yes, it would be fragile, but objtool has a fix for that.  It looks at
every instruction in the object file and warns if it finds a return
instruction outside of an ENDPROC function.  That works because all
callable instructions have return instructions (except when they have
sibling calls, but objtool detects those too).  So objtool will flag any
C-type functions that forgot to use ENDPROC.

> > But entry code is different.  In most cases, the global symbols aren't
> > actually called, and they don't follow any conventions.  The code is
> > spaghetti-esque, with HW handlers and jumps everywhere.  The state of
> > the stack at symbol entry varies per "function".  That's why objtool
> > ignores these files.
> 
> Unfortunately, this is true.
> 
> > For special cases (like entry code), I was thinking we'd need manual CFI
> > annotations, like we had before.  Or maybe there's another way, like
> > some new macros which tell objtool about the HW entry points and the
> > state of the registers there.
> > 
> > But I'm having trouble seeing how marking these code snippets with
> > ENTRY/ENDPROC would help objtool make any sense of the code and where
> > things are on the stack.
> 
> Ok, my intention was to have every line of assembly code in between of
> FUNC_START/FUNC_END. That way, every rsp related push/pop/sub/add can be
> annotated very easily. For the C-like functions this is all what needs
> to be done.
> 
> Then there is the spaghetti code. And I was thinking about manual
> annotations like:
> 
>   # skip the frame pointer checking between START+END here
>   OBJTOOL(SKIP_CHECKING)
> 
>   # this fn has unusual frame (like interrupts have),
>     and you can find return RIP stored at fp + 0x20
>   OBJTOOL(RIP_IS_AT, 0x20)
> 
>   # put this raw CFI for this location into eh_frame
>   OBJTOOL(RAW_CFI, 0x00, 0x00, 0x00)
> 
> 
> Similarly, I have OBJTOOL(START_FUNC) and OBJTOOL(END_FUNC) emitted with
> each FUNC_START/FUNC_END. So far, when manually expanded for simplicity,
> it looks like this:

I like the idea of making objtool smart enough to read the entry code,
and of combining automated annotations (where possible) with manual
annotations (where necessary).  And it does make sense for objtool to
automate every rsp-related push/pop/sub/add annotation.  That will make
the entry code quite a bit cleaner since we don't need 'push_cfi' and
friends anymore.

However, I think trying to force the entry code snippets into being
normal functions would be awkward.  For example, C-type functions all
start off with the following initial CFI state:

     LOC           CFA      ra
  0000000000000000 rsp+8    c-8

That means the previous frame's stack pointer was at rsp+8 and the
return instruction pointer is (rsp).  But those assumptions don't hold
for non-C-type functions, which usually start with pt_regs or iret regs
on the stack, or a blank slate.

So the initial CFI state is different between the two types of
"functions".  And there are a lot of other differences.  C-type
functions have to follow frame pointer conventions, for example.  So
your FUNC_START macro (and objtool) would have to somehow figure out a
way to make a distinction between the two.  So it would probably work
out better if we kept the distinction between C-type functions and other
code.

I think ENDPROC (or FUNC_START/FUNC_END) should mean "this function is
100% standardized to the C ABI and its debuginfo can be completely
automated".  And any code outside of that would be "this code is special
and needs a mix of automated and manual debuginfo annotations."

I'm also not sure we need the objtool-specific macros.  It might be
simpler to have macros which just output the cfi instead.  I guess this
goes back to our previous discussions about whether objtool's CFI access
should be read/write or write-only.  I don't remember, did we ever to
come to a conclusion with that?

Either way, from looking at the entry code, we may be able to get away
with only the following .macros:

- DWARF_EMPTY_FRAME signal=0

  Mark all registers as undefined and potentially mark the frame as a
  signal frame.

- DWARF_SET_CFA base=rsp offset=0 c_regs=0 extra_regs=0 iret_regs=0

  Set the CFA value.  Set c_regs, extra_regs, and/or iret_regs to
  indicate which regs (if any) are stored just below the CFA.

- DWARF_SET_INDIRECT_CFA base=rsp offset=0 val_offset=0

  Set CFA = *(base + offset) + val_offset.  I only saw a few places
  where this is needed, where it switches to the irq stack.  We might be
  able to figure out a way to simplify the code in a non-intrusive way
  to get rid of the need for this one.

And we could create higher-level macros from these primitives if needed.

I think we'd only need the macros in relatively few places in the entry
code.  It would be a lot less intrusive than what we had before.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1643358

On 05/13/2017, 12:15 AM, Josh Poimboeuf wrote:
>> Similarly, I have OBJTOOL(START_FUNC) and OBJTOOL(END_FUNC) emitted with
>> each FUNC_START/FUNC_END. So far, when manually expanded for simplicity,
>> it looks like this:
> 
> I like the idea of making objtool smart enough to read the entry code,
> and of combining automated annotations (where possible) with manual
> annotations (where necessary).  And it does make sense for objtool to
> automate every rsp-related push/pop/sub/add annotation.  That will make
> the entry code quite a bit cleaner since we don't need 'push_cfi' and
> friends anymore.
> 
> However, I think trying to force the entry code snippets into being
> normal functions would be awkward.  For example, C-type functions all
> start off with the following initial CFI state:
> 
>      LOC           CFA      ra
>   0000000000000000 rsp+8    c-8
> 
> That means the previous frame's stack pointer was at rsp+8 and the
> return instruction pointer is (rsp).  But those assumptions don't hold
> for non-C-type functions, which usually start with pt_regs or iret regs
> on the stack, or a blank slate.
> 
> So the initial CFI state is different between the two types of
> "functions".  And there are a lot of other differences.  C-type
> functions have to follow frame pointer conventions, for example.  So
> your FUNC_START macro (and objtool) would have to somehow figure out a
> way to make a distinction between the two.  So it would probably work
> out better if we kept the distinction between C-type functions and other
> code.

Ok, that makes a lot of sense.

> I think ENDPROC (or FUNC_START/FUNC_END) should mean "this function is
> 100% standardized to the C ABI and its debuginfo can be completely
> automated".  And any code outside of that would be "this code is special
> and needs a mix of automated and manual debuginfo annotations."

I only hesitate how to call the others. I assume, SYM_FUNC_START and
SYM_FUNC_END were agreed upon for the C-func-like functions.

For the others, what about simply:
  SYM_FUNC_START_SPECIAL/SYM_FUNC_END_SPECIAL
or
  SYM_CODE_START/SYM_CODE_END
or
  SOMETHING_ELSE
?

> I'm also not sure we need the objtool-specific macros.  It might be
> simpler to have macros which just output the cfi instead.  I guess this
> goes back to our previous discussions about whether objtool's CFI access
> should be read/write or write-only.  I don't remember, did we ever to
> come to a conclusion with that?

Correct, exactly to avoid r-w on dwarfinfo in objtool, I introduced the
special objtool macros. They would just put the same cfis into the
.discard section for objtool to combine them with the automatic injected
annotations and put them to the correct place. For -- almost -- free.

Our last discussion on this topic ended up with w-only for objtool at
the moment. I originally wanted r-w to support inline assembly in C, but
you suggested r-only is quite easier, therefore we should start with it.
So the r-w extension is doable, but the question is whether we want the
complexity now.

> Either way, from looking at the entry code, we may be able to get away
> with only the following .macros:
> 
> - DWARF_EMPTY_FRAME signal=0
> 
>   Mark all registers as undefined and potentially mark the frame as a
>   signal frame.
> 
> - DWARF_SET_CFA base=rsp offset=0 c_regs=0 extra_regs=0 iret_regs=0
> 
>   Set the CFA value.  Set c_regs, extra_regs, and/or iret_regs to
>   indicate which regs (if any) are stored just below the CFA.
> 
> - DWARF_SET_INDIRECT_CFA base=rsp offset=0 val_offset=0
> 
>   Set CFA = *(base + offset) + val_offset.  I only saw a few places
>   where this is needed, where it switches to the irq stack.  We might be
>   able to figure out a way to simplify the code in a non-intrusive way
>   to get rid of the need for this one.

Correct, it corresponds with what I had locally to make DWARF unwinder
working through interrupts, in terms of CFI's:
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -463,6 +463,7 @@ SYM_FUNC_END(irq_entries_start)
        ALLOC_PT_GPREGS_ON_STACK
        SAVE_C_REGS
        SAVE_EXTRA_REGS
+       DW_CFI(.cfi_rel_offset rbp, RBP+8)
        ENCODE_FRAME_POINTER

        testb   $3, CS(%rsp)
@@ -497,7 +498,17 @@ SYM_FUNC_END(irq_entries_start)
        movq    %rsp, %rdi
        incl    PER_CPU_VAR(irq_count)
        cmovzq  PER_CPU_VAR(irq_stack_ptr), %rsp
+       DW_CFI(.cfi_def_cfa_register rdi)
+
        pushq   %rdi
+       DW_CFI(.cfi_escape 0x0f /* DW_CFA_def_cfa_expression */, 6 /*
block len */, \
+               0x77 /* DW_OP_breg7 (rsp) */, 0 /* offset */, \
+               0x06 /* DW_OP_deref */, \
+               0x08 /* DW_OP_const1u */, SIZEOF_PTREGS, \
+               0x22 /* DW_OP_plus */)
+       DW_CFI(.cfi_offset rsp, -2*8)
+       DW_CFI(.cfi_offset rip, -5*8)
+
        /* We entered an interrupt context - irqs are off: */
        TRACE_IRQS_OFF

@@ -654,9 +665,15 @@ SYM_FUNC_END(common_interrupt)
  * APIC interrupts.
  */
 .macro apicinterrupt3 num sym do_sym
-SYM_FUNC_START(\sym)
+SYM_FUNC_START_ALIAS(\sym)
+       DW_CFI(.cfi_startproc simple)
+       DW_CFI(.cfi_signal_frame)
+       DW_CFI(.cfi_def_cfa rsp, 6*8)
+       DW_CFI(.cfi_rel_offset rsp, 4*8)
+       DW_CFI(.cfi_rel_offset rip, 1*8)
        ASM_CLAC
        pushq   $~(\num)
+       DW_CFI(.cfi_adjust_cfa_offset 8)


(DW_CFI is my local-only macro to kill the annotations by a single
switch whenever I want.)

> And we could create higher-level macros from these primitives if needed.
> 
> I think we'd only need the macros in relatively few places in the entry
> code.  It would be a lot less intrusive than what we had before.

Sure, that's the whole point of this exercise :).

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1645448

On 05/17/2017, 03:23 PM, Jiri Slaby wrote:
>> So the initial CFI state is different between the two types of
>> "functions".  And there are a lot of other differences.  C-type
>> functions have to follow frame pointer conventions, for example.  So
>> your FUNC_START macro (and objtool) would have to somehow figure out a
>> way to make a distinction between the two.  So it would probably work
>> out better if we kept the distinction between C-type functions and other
>> code.
> 
> Ok, that makes a lot of sense.

A quick question:
Do you consider these to be C-type functions?

  ENTRY(function_hook)
        ret
  END(function_hook)

or this?

  ENTRY(native_load_gs_index)
        pushfq
        DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
        SWAPGS
        movl    %edi, %gs
        SWAPGS
        popfq
        ret
  END(native_load_gs_index)

Both are called from C, but they do not setup frame pointer etc.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1645875

On Fri, May 19, 2017 at 11:17:24AM +0200, Jiri Slaby wrote:
> On 05/17/2017, 03:23 PM, Jiri Slaby wrote:
> >> So the initial CFI state is different between the two types of
> >> "functions".  And there are a lot of other differences.  C-type
> >> functions have to follow frame pointer conventions, for example.  So
> >> your FUNC_START macro (and objtool) would have to somehow figure out a
> >> way to make a distinction between the two.  So it would probably work
> >> out better if we kept the distinction between C-type functions and other
> >> code.
> > 
> > Ok, that makes a lot of sense.
> 
> A quick question:
> Do you consider these to be C-type functions?
> 
>   ENTRY(function_hook)
>         ret
>   END(function_hook)
> 
> or this?
> 
>   ENTRY(native_load_gs_index)
>         pushfq
>         DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
>         SWAPGS
>         movl    %edi, %gs
>         SWAPGS
>         popfq
>         ret
>   END(native_load_gs_index)
> 
> Both are called from C, but they do not setup frame pointer etc.

Yeah, those are valid C-type functions.  Setting up the frame pointer is
optional for leaf functions (i.e.  functions which don't call other
functions).

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web