Message-ID: <20170512221532.s2wuaoq65uvaprgq@treble>
Date:   Fri, 12 May 2017 17:15:32 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Jiri Slaby <jslaby@...e.cz>
Cc:     mingo@...hat.com, tglx@...utronix.de, hpa@...or.com,
        x86@...nel.org, linux-kernel@...r.kernel.org,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>, xen-devel@...ts.xenproject.org
Subject: Re: [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

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

Powered by blists - more mailing lists