Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Gustavo da Silva]

Hi, brothers.

I was reading the e-mails about the topic, and I have a simple suggestion:

FRAMED_FUNCTION_ENTRYPOINT(xyz)
...
FRAMED_FUNCTION_RETPOINT(xyz)

--
Atenciosamente,

Gustavo da Silva.
(Brazil)

> On Sat, Jul 18, 2015 at 04:25:25PM +0200, Borislav Petkov wrote:
> > On Sat, Jul 18, 2015 at 08:46:55AM -0500, Josh Poimboeuf wrote:
> > > I like the balance, but the "ret" is still non-obvious.
> >
> > Does it have to be obvious?
>
> I feel that making "ret" obvious is better.
>
> But if somebody messes up and adds a second "ret", I suppose
> stackvalidate would warn about the fact that it returned without
> restoring the frame pointer.  So if there are no other objections, your
> suggestion of ENTRY_FRAME and ENDPROC_FRAME is fine with me.

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Mon, Jul 20, 2015 at 01:00:06PM -0500, Josh Poimboeuf wrote:
> On Mon, Jul 20, 2015 at 07:21:24PM +0200, Ingo Molnar wrote:
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > Ok, I see how the naming scheme I proposed won't work with all that very well, but 
> > I'd still suggest using consistently named patterns.
> > 
> > Let me suggest yet another approach. How about open-coding something like this:
> > 
> >  FUNCTION_START(func)
> > 
> > 	push_bp
> > 	mov_sp_bp
> > 
> > 	...
> > 
> > 	pop_bp
> > 	ret
> > 
> >  FUNCTION_END(func)
> > 
> > This is just two easy things:
> > 
> >  - a redefine of the FUNCTION_ENTRY and ENDPROC names
> > 
> >  - the introduction of three quasi-mnemonics: push_bp, mov_sp_bp, pop_bp - which 
> >    all look very similar to a real frame setup sequence, except that we can easily 
> >    make them go away in the !CONFIG_FRAME_POINTERS case.
> > 
> > The advantage of this approach would be:
> > 
> >  - it looks pretty 'natural' and very close to how the real disassembly looks
> >    like in CONFIG_FRAME_POINTERS=y kernels. So while it's not as compact as some 
> >    of the other variants, it's close to what the real instruction sequence looks 
> >    like and that is a positive quality in itself.
> > 
> >  - it also makes it apparent 'on sight' that it's probably a bug to have
> >    unbalanced push/pop sequences in a regular function, to any reasonably alert 
> >    assembly coder.
> > 
> >  - if we ever unsupport framepointer kernels in the (far far) future, we can get
> >    rid of all lines with those 3 mnemonics and be done with it.
> > 
> >  - it's finegrained enough so that we can express all the special function/tail
> >    variants you listed above.
> > 
> > What do you think?
> 
> I agree that the edge cases make FUNCTION_ENTRY and FUNCTION_RETURN less
> attractive.  Slowly we are circling around to where we started :-)
> 
> Personally, I prefer FRAME/ENDFRAME instead of push_bp/mov_sp_bp/pop_bp,
> because it more communicates *what* it's doing rather than how.  IMO,
> it's easier to grok with a quick glance.

Ingo, any chance this last paragraph was a convincing argument to
continue to use FRAME/ENDFRAME over push_bp/mov_sp_bp/pop_bp?

(I think this is the last outstanding issue from the reviews, so I'm all
set to send out a new version of the patches once there's agreement on
this issue.)

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Tue, Jul 21, 2015 at 10:00:16AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 08:30:52AM -0700, Andy Lutomirski wrote:
> > > On Fri, Jul 17, 2015 at 8:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > The reason I suggested to put FRAME in the macro name is to try to
> > > > prevent it from being accidentally used for leaf functions, where it
> > > > isn't needed.
> > > >
> > > 
> > > Could someone remind me why it isn't needed for leaf functions?
> > 
> > If a function doesn't call any other functions, then it won't ever show
> > up in a stack trace unless:
> > 
> > a) the function itself walks the stack, in which case the frame pointer
> >    isn't necessary; or
> > 
> > b) The function gets hit by an interrupt/exception, in which case frame
> >    pointers can't be 100% relied upon anyway.
> > 
> > I've noticed that gcc *does* seem to create stack frames for leaf functions.  
> > But it's inconsistent, because the early exit path of some functions will skip 
> > the stack frame creation and go straight to the return.
> > 
> > We could probably get a good performance boost with the 
> > -momit-leaf-frame-pointer flag.  Though it would make stack traces less reliable 
> > when a leaf function gets interrupted.
> 
> So in theory we could resolve this during the stack walk: when we pass from the 
> IRQ stack to the process stack we actually know the RIP of the interrupted 
> context, and could include that.

The problem is with the *caller* of the leaf function.  Without the
leaf's frame pointer there's no way to find the call site pointer on the
stack, so the leaf's caller gets skipped.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Jul 20, 2015 at 08:30:52AM -0700, Andy Lutomirski wrote:
> > On Fri, Jul 17, 2015 at 8:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > The reason I suggested to put FRAME in the macro name is to try to
> > > prevent it from being accidentally used for leaf functions, where it
> > > isn't needed.
> > >
> > 
> > Could someone remind me why it isn't needed for leaf functions?
> 
> If a function doesn't call any other functions, then it won't ever show
> up in a stack trace unless:
> 
> a) the function itself walks the stack, in which case the frame pointer
>    isn't necessary; or
> 
> b) The function gets hit by an interrupt/exception, in which case frame
>    pointers can't be 100% relied upon anyway.
> 
> I've noticed that gcc *does* seem to create stack frames for leaf functions.  
> But it's inconsistent, because the early exit path of some functions will skip 
> the stack frame creation and go straight to the return.
> 
> We could probably get a good performance boost with the 
> -momit-leaf-frame-pointer flag.  Though it would make stack traces less reliable 
> when a leaf function gets interrupted.

So in theory we could resolve this during the stack walk: when we pass from the 
IRQ stack to the process stack we actually know the RIP of the interrupted 
context, and could include that.

Visualizing context boundaries in the stack dump would also be pretty useful 
independently of the performance boost. We already demark them to a certain 
degree:

[   37.287036] Call Trace:
[   37.287042]  <IRQ>  [<ffffffffb5161eee>] dump_stack+0x4c/0x65
[   37.287045]  [<ffffffffb3d40288>] ? down_trylock+0x28/0x33
[   37.287048]  [<ffffffffb3cfa74d>] warn_slowpath_common+0x7b/0xb1
[   37.287050]  [<ffffffffb3cfa7f7>] warn_slowpath_fmt+0x50/0x6e
[   37.287053]  [<ffffffffb3cfe0ba>] ? __do_softirq+0x224/0x289
[   37.287055]  [<ffffffffb3cfe047>] ? __do_softirq+0x1b1/0x289
[   37.287057]  [<ffffffffb3d71b4c>] can_stop_full_tick+0x69/0xa1
[   37.287059]  [<ffffffffb3d72273>] tick_nohz_irq_exit+0x72/0x98
[   37.287061]  [<ffffffffb3cfe3b5>] irq_exit+0xbf/0x104
[   37.287064]  [<ffffffffb3c6b01a>] smp_apic_timer_interrupt+0x42/0x51
[   37.287067]  [<ffffffffb51821d9>] apic_timer_interrupt+0x89/0x90
[   37.287071]  <EOI>  [<ffffffffb3d283ab>] ? ___might_sleep+0x150/0x232
[   37.287075]  [<ffffffffb3d4b684>] ? lock_torture_reader+0x102/0x102

including the RIP of interrupted nested contexts would be helpful.

Thanks,

	Ingo

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Mon, Jul 20, 2015 at 07:21:24PM +0200, Ingo Molnar wrote:
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, Jul 20, 2015 at 09:56:11AM +0200, Ingo Molnar wrote:
> > I should point out that there are still a few cases where the more granular 
> > FRAME/ENDFRAME and ENTRY/ENDPROC macros would still be needed.
> > 
> > For example, if the function ends with a jump instead of a ret.  If the
> > jump is a sibling call, the code would look like:
> > 
> > FUNCTION_ENTRY(func)
> > 	...
> > 	ENDFRAME
> > 	jmp another_func
> > ENDPROC(func)
> > 
> > 
> > Or if it's a jump within the function to an internal ret:
> > 
> > FUNCTION_ENTRY(func)
> > 	...
> > 1:	...
> > 	ENDFRAME
> > 	ret
> > 2:	...
> > 	jmp 1b
> > ENDPROC(func)
> > 
> > 
> > Or if it jumps to some shared code before returning:
> > 
> > FUNCTION_ENTRY(func_1)
> > 	...
> > 	jmp common_return
> > ENDPROC(func_1)
> > 
> > FUNCTION_ENTRY(func_2)
> > 	...
> > 	jmp common_return
> > ENDPROC(func_2)
> > 
> > common_return:
> > 	...
> > 	ENDFRAME
> > 	ret
> > 
> > 
> > So in some cases we'd still need the more granular macros, unless we
> > decided to make special macros for these cases as well.
> 
> Ok, I see how the naming scheme I proposed won't work with all that very well, but 
> I'd still suggest using consistently named patterns.
> 
> Let me suggest yet another approach. How about open-coding something like this:
> 
>  FUNCTION_START(func)
> 
> 	push_bp
> 	mov_sp_bp
> 
> 	...
> 
> 	pop_bp
> 	ret
> 
>  FUNCTION_END(func)
> 
> This is just two easy things:
> 
>  - a redefine of the FUNCTION_ENTRY and ENDPROC names
> 
>  - the introduction of three quasi-mnemonics: push_bp, mov_sp_bp, pop_bp - which 
>    all look very similar to a real frame setup sequence, except that we can easily 
>    make them go away in the !CONFIG_FRAME_POINTERS case.
> 
> The advantage of this approach would be:
> 
>  - it looks pretty 'natural' and very close to how the real disassembly looks
>    like in CONFIG_FRAME_POINTERS=y kernels. So while it's not as compact as some 
>    of the other variants, it's close to what the real instruction sequence looks 
>    like and that is a positive quality in itself.
> 
>  - it also makes it apparent 'on sight' that it's probably a bug to have
>    unbalanced push/pop sequences in a regular function, to any reasonably alert 
>    assembly coder.
> 
>  - if we ever unsupport framepointer kernels in the (far far) future, we can get
>    rid of all lines with those 3 mnemonics and be done with it.
> 
>  - it's finegrained enough so that we can express all the special function/tail
>    variants you listed above.
> 
> What do you think?

I agree that the edge cases make FUNCTION_ENTRY and FUNCTION_RETURN less
attractive.  Slowly we are circling around to where we started :-)

Personally, I prefer FRAME/ENDFRAME instead of push_bp/mov_sp_bp/pop_bp,
because it more communicates *what* it's doing rather than how.  IMO,
it's easier to grok with a quick glance.

> I'd still keep existing frame setup functionality and names and only use these in 
> fixes, new code and new annotations - and do a full rename and cleanup once the 
> dust has settled.

That sounds good.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Jul 20, 2015 at 09:56:11AM +0200, Ingo Molnar wrote:
> > > 
> > > The reason I suggested to put FRAME in the macro name is to try to prevent it 
> > > from being accidentally used for leaf functions, where it isn't needed.
> > 
> > Well, we could use LEAF_FUNCTION to mark that fact.
> > 
> > Wether a function written in assembly is a leaf function or not is a higher level 
> > (and thus more valuable) piece of information whether we generate frame pointer 
> > debuginfo or not.
> > 
> > > Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to 
> > > distinguish them from the already ubiquitous ENTRY and ENDPROC.  So as a kernel 
> > > developer it seems confusing to me, e.g. how do I remember when to use 
> > > FUNCTION_ENTRY vs ENTRY?
> > 
> > 'ENDPROC' is really leftover from older debuginfo cruft, it's not a valuable 
> > construct IMHO, even if it's (sadly) ubiquitious.
> > 
> > We want to create new, clean, as minimal as possible and as clearly named as 
> > possible debuginfo constructs from first principles.
> 
> Ok. So if I understand right, the proposal is:
> 
> Replace *all* x86 usage of ENTRY/ENDPROC with either:
> 
> FUNCTION_ENTRY(func)
> FUNCTION_RETURN(func)
> 
> or
> 
> LEAF_FUNCTION_ENTRY(func)
> LEAF_FUNCTION_RETURN(func)
> 
> Those sound fine to me.

Yeah - but keep the old constructs as well and don't necessarily do the full 
migration straight away, only once the dust has settled - to reduce churn.

> I should point out that there are still a few cases where the more granular 
> FRAME/ENDFRAME and ENTRY/ENDPROC macros would still be needed.
> 
> For example, if the function ends with a jump instead of a ret.  If the
> jump is a sibling call, the code would look like:
> 
> FUNCTION_ENTRY(func)
> 	...
> 	ENDFRAME
> 	jmp another_func
> ENDPROC(func)
> 
> 
> Or if it's a jump within the function to an internal ret:
> 
> FUNCTION_ENTRY(func)
> 	...
> 1:	...
> 	ENDFRAME
> 	ret
> 2:	...
> 	jmp 1b
> ENDPROC(func)
> 
> 
> Or if it jumps to some shared code before returning:
> 
> FUNCTION_ENTRY(func_1)
> 	...
> 	jmp common_return
> ENDPROC(func_1)
> 
> FUNCTION_ENTRY(func_2)
> 	...
> 	jmp common_return
> ENDPROC(func_2)
> 
> common_return:
> 	...
> 	ENDFRAME
> 	ret
> 
> 
> So in some cases we'd still need the more granular macros, unless we
> decided to make special macros for these cases as well.

Ok, I see how the naming scheme I proposed won't work with all that very well, but 
I'd still suggest using consistently named patterns.

Let me suggest yet another approach. How about open-coding something like this:

 FUNCTION_START(func)

	push_bp
	mov_sp_bp

	...

	pop_bp
	ret

 FUNCTION_END(func)

This is just two easy things:

 - a redefine of the FUNCTION_ENTRY and ENDPROC names

 - the introduction of three quasi-mnemonics: push_bp, mov_sp_bp, pop_bp - which 
   all look very similar to a real frame setup sequence, except that we can easily 
   make them go away in the !CONFIG_FRAME_POINTERS case.

The advantage of this approach would be:

 - it looks pretty 'natural' and very close to how the real disassembly looks
   like in CONFIG_FRAME_POINTERS=y kernels. So while it's not as compact as some 
   of the other variants, it's close to what the real instruction sequence looks 
   like and that is a positive quality in itself.

 - it also makes it apparent 'on sight' that it's probably a bug to have
   unbalanced push/pop sequences in a regular function, to any reasonably alert 
   assembly coder.

 - if we ever unsupport framepointer kernels in the (far far) future, we can get
   rid of all lines with those 3 mnemonics and be done with it.

 - it's finegrained enough so that we can express all the special function/tail
   variants you listed above.

What do you think?

I'd still keep existing frame setup functionality and names and only use these in 
fixes, new code and new annotations - and do a full rename and cleanup once the 
dust has settled.

Thanks,

	Ingo

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Mon, Jul 20, 2015 at 06:52:32PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 20, 2015 at 11:36:46AM -0500, Josh Poimboeuf wrote:
> > If a function doesn't call any other functions, then it won't ever show
> > up in a stack trace unless:
> > 
> > a) the function itself walks the stack, in which case the frame pointer
> >    isn't necessary; or
> > 
> > b) The function gets hit by an interrupt/exception, in which case frame
> >    pointers can't be 100% relied upon anyway.
> 
> In case the interrupt happens whilst setting up the frame, right?

Right.

> > I've noticed that gcc *does* seem to create stack frames for leaf
> > functions.  But it's inconsistent, because the early exit path of some
> > functions will skip the stack frame creation and go straight to the
> > return.
> > 
> > We could probably get a good performance boost with the
> > -momit-leaf-frame-pointer flag.  Though it would make stack traces less
> > reliable when a leaf function gets interrupted.
> 
> So the information we'd loose in that case would be the location in the
> calling function, right?

Right.

> Which isn't a problem, if the current function (as obtained
> through RIP) is only ever called once. However if there's multiple call
> sites this might be a wee bit confusing.

Agreed, though the stack dump code always prints '?' for any kernel
address it finds on the stack.  So there would still be a good clue.


-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Peter Zijlstra]

On Mon, Jul 20, 2015 at 11:36:46AM -0500, Josh Poimboeuf wrote:
> If a function doesn't call any other functions, then it won't ever show
> up in a stack trace unless:
> 
> a) the function itself walks the stack, in which case the frame pointer
>    isn't necessary; or
> 
> b) The function gets hit by an interrupt/exception, in which case frame
>    pointers can't be 100% relied upon anyway.

In case the interrupt happens whilst setting up the frame, right?

> I've noticed that gcc *does* seem to create stack frames for leaf
> functions.  But it's inconsistent, because the early exit path of some
> functions will skip the stack frame creation and go straight to the
> return.
> 
> We could probably get a good performance boost with the
> -momit-leaf-frame-pointer flag.  Though it would make stack traces less
> reliable when a leaf function gets interrupted.

So the information we'd loose in that case would be the location in the
calling function, right? Which isn't a problem, if the current function (as obtained
through RIP) is only ever called once. However if there's multiple call
sites this might be a wee bit confusing.

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Mon, Jul 20, 2015 at 08:30:52AM -0700, Andy Lutomirski wrote:
> On Fri, Jul 17, 2015 at 8:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > The reason I suggested to put FRAME in the macro name is to try to
> > prevent it from being accidentally used for leaf functions, where it
> > isn't needed.
> >
> 
> Could someone remind me why it isn't needed for leaf functions?

If a function doesn't call any other functions, then it won't ever show
up in a stack trace unless:

a) the function itself walks the stack, in which case the frame pointer
   isn't necessary; or

b) The function gets hit by an interrupt/exception, in which case frame
   pointers can't be 100% relied upon anyway.

I've noticed that gcc *does* seem to create stack frames for leaf
functions.  But it's inconsistent, because the early exit path of some
functions will skip the stack frame creation and go straight to the
return.

We could probably get a good performance boost with the
-momit-leaf-frame-pointer flag.  Though it would make stack traces less
reliable when a leaf function gets interrupted.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Andy Lutomirski]

On Fri, Jul 17, 2015 at 8:56 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> The reason I suggested to put FRAME in the macro name is to try to
> prevent it from being accidentally used for leaf functions, where it
> isn't needed.
>

Could someone remind me why it isn't needed for leaf functions?

--Andy

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Mon, Jul 20, 2015 at 09:56:11AM +0200, Ingo Molnar wrote:
> > 
> > The reason I suggested to put FRAME in the macro name is to try to prevent it 
> > from being accidentally used for leaf functions, where it isn't needed.
> 
> Well, we could use LEAF_FUNCTION to mark that fact.
> 
> Wether a function written in assembly is a leaf function or not is a higher level 
> (and thus more valuable) piece of information whether we generate frame pointer 
> debuginfo or not.
> 
> > Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to 
> > distinguish them from the already ubiquitous ENTRY and ENDPROC.  So as a kernel 
> > developer it seems confusing to me, e.g. how do I remember when to use 
> > FUNCTION_ENTRY vs ENTRY?
> 
> 'ENDPROC' is really leftover from older debuginfo cruft, it's not a valuable 
> construct IMHO, even if it's (sadly) ubiquitious.
> 
> We want to create new, clean, as minimal as possible and as clearly named as 
> possible debuginfo constructs from first principles.

Ok. So if I understand right, the proposal is:

Replace *all* x86 usage of ENTRY/ENDPROC with either:

FUNCTION_ENTRY(func)
FUNCTION_RETURN(func)

or

LEAF_FUNCTION_ENTRY(func)
LEAF_FUNCTION_RETURN(func)

Those sound fine to me.


I should point out that there are still a few cases where the more
granular FRAME/ENDFRAME and ENTRY/ENDPROC macros would still be needed.

For example, if the function ends with a jump instead of a ret.  If the
jump is a sibling call, the code would look like:

FUNCTION_ENTRY(func)
	...
	ENDFRAME
	jmp another_func
ENDPROC(func)


Or if it's a jump within the function to an internal ret:

FUNCTION_ENTRY(func)
	...
1:	...
	ENDFRAME
	ret
2:	...
	jmp 1b
ENDPROC(func)


Or if it jumps to some shared code before returning:

FUNCTION_ENTRY(func_1)
	...
	jmp common_return
ENDPROC(func_1)

FUNCTION_ENTRY(func_2)
	...
	jmp common_return
ENDPROC(func_2)

common_return:
	...
	ENDFRAME
	ret


So in some cases we'd still need the more granular macros, unless we
decided to make special macros for these cases as well.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Sat, Jul 18, 2015 at 04:51:16AM +0200, Ingo Molnar wrote:
> > Note what the names _don't_ contain: that we generate debug info! That fact is not 
> > present in the naming, and that's very much intentional, because the precise form 
> > of debug info is conditional:
> > 
> >   - if CONFIG_FRAME_POINTERS=y then we push/pop a stack frame
> > 
> >   - if (later on) we do CFI annotations we don't push/pop a stack frame but emit 
> >     CFI debuginfo
> 
> According to current plan, the macro won't add CFI annotations.  That will be 
> done instead by a separate tool.  So the macro really is frame pointer specific.

Still the same argument applies: it's a debug info detail we should hide as much 
as sensibly possible. In the CONFIG_FRAME_POINTERS=y case it will be code, in the 
future !CONFIG_FRAME_POINTERS case it will be nothing.

> > In that sense 'FRAME' should never be in these names I think, nor 'PROC' 
> > (which is not symmetric).
> > 
> > Plus all 3 variants I suggested are very easy to remember, why I'd always have 
> > to look up any non-symmetric macro name called 'PROC'...
> 
> The reason I suggested to put FRAME in the macro name is to try to prevent it 
> from being accidentally used for leaf functions, where it isn't needed.

Well, we could use LEAF_FUNCTION to mark that fact.

Wether a function written in assembly is a leaf function or not is a higher level 
(and thus more valuable) piece of information whether we generate frame pointer 
debuginfo or not.

> Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do anything to 
> distinguish them from the already ubiquitous ENTRY and ENDPROC.  So as a kernel 
> developer it seems confusing to me, e.g. how do I remember when to use 
> FUNCTION_ENTRY vs ENTRY?

'ENDPROC' is really leftover from older debuginfo cruft, it's not a valuable 
construct IMHO, even if it's (sadly) ubiquitious.

We want to create new, clean, as minimal as possible and as clearly named as 
possible debuginfo constructs from first principles.

Thanks,

	Ingo

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Sat, Jul 18, 2015 at 04:25:25PM +0200, Borislav Petkov wrote:
> On Sat, Jul 18, 2015 at 08:46:55AM -0500, Josh Poimboeuf wrote:
> > I like the balance, but the "ret" is still non-obvious.
> 
> Does it have to be obvious?

I feel that making "ret" obvious is better.

But if somebody messes up and adds a second "ret", I suppose
stackvalidate would warn about the fact that it returned without
restoring the frame pointer.  So if there are no other objections, your
suggestion of ENTRY_FRAME and ENDPROC_FRAME is fine with me.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Borislav Petkov]

On Sat, Jul 18, 2015 at 08:46:55AM -0500, Josh Poimboeuf wrote:
> I like the balance, but the "ret" is still non-obvious.

Does it have to be obvious?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Sat, Jul 18, 2015 at 10:42:36AM +0200, Borislav Petkov wrote:
> On Fri, Jul 17, 2015 at 04:10:19PM -0500, Josh Poimboeuf wrote:
> > Actually I'm not done painting.  Personally it seems a little too
> > verbose.  I still like ENTRY_FRAME and ENDPROC_FRAME_RETURN :p
> 
> Let's balance it out even more:
> 
> ENTRY_FRAME(..)
> 
> 	...
> 
> ENDPROC_FRAME(..)

I like the balance, but the "ret" is still non-obvious.

How about:

FRAME_ENTRY(...)

FRAME_RETURN(...)

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Borislav Petkov]

On Fri, Jul 17, 2015 at 04:10:19PM -0500, Josh Poimboeuf wrote:
> Actually I'm not done painting.  Personally it seems a little too
> verbose.  I still like ENTRY_FRAME and ENDPROC_FRAME_RETURN :p

Let's balance it out even more:

ENTRY_FRAME(..)

	...

ENDPROC_FRAME(..)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Sat, Jul 18, 2015 at 04:51:16AM +0200, Ingo Molnar wrote:
> Note what the names _don't_ contain: that we generate debug info! That fact is not 
> present in the naming, and that's very much intentional, because the precise form 
> of debug info is conditional:
> 
>   - if CONFIG_FRAME_POINTERS=y then we push/pop a stack frame
> 
>   - if (later on) we do CFI annotations we don't push/pop a stack frame but emit 
>     CFI debuginfo

According to current plan, the macro won't add CFI annotations.  That
will be done instead by a separate tool.  So the macro really is frame
pointer specific.

> 
> In that sense 'FRAME' should never be in these names I think, nor 'PROC' (which is 
> not symmetric).
> 
> Plus all 3 variants I suggested are very easy to remember, why I'd always have to 
> look up any non-symmetric macro name called 'PROC'...

The reason I suggested to put FRAME in the macro name is to try to
prevent it from being accidentally used for leaf functions, where it
isn't needed.

Also the naming of FUNCTION_ENTRY and FUNCTION_RETURN doesn't do
anything to distinguish them from the already ubiquitous ENTRY and
ENDPROC.  So as a kernel developer it seems confusing to me, e.g. how do
I remember when to use FUNCTION_ENTRY vs ENTRY?

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
> > On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > >>  ENTRY(aesni_set_key)
> > >> +     FRAME
> > >>  #ifndef __x86_64__
> > >>       pushl KEYP
> > >>       movl 8(%esp), KEYP              # ctx
> > >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
> > >>  #ifndef __x86_64__
> > >>       popl KEYP
> > >>  #endif
> > >> +     ENDFRAME
> > >>       ret
> > >>  ENDPROC(aesni_set_key)
> > >
> > > So cannot we make this a bit more compact and less fragile?
> > >
> > > Instead of:
> > >
> > >         ENTRY(aesni_set_key)
> > >                 FRAME
> > >         ...
> > >                 ENDFRAME
> > >                 ret
> > >         ENDPROC(aesni_set_key)
> > >
> > >
> > > How about writing this as:
> > >
> > >         FUNCTION_ENTRY(aesni_set_key)
> > >         ...
> > >         FUNCTION_RETURN(aesni_set_key)
> > >
> > > which does the same thing in a short, symmetric construct?
> > >
> > > One potential problem with this approach would be that what 'looks' like an entry
> > > declaration, but it will now generate real code.
> > >
> > > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> > > and I think 'RETURN' makes it clear enough that there's a real instruction
> > > generated there.
> > >
> > 
> > How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
> 
> Perhaps the macro name should describe what the epilogue does, since
> frame pointers aren't required for _all_ functions, only those which
> don't have call instructions.
> 
> What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
> ending macro is kind of long, but at least it a) matches the existing
> ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
> pointers are involved; and c) lets you know that the return is there.

So the thing I like about these:

	FUNCTION_ENTRY(aesni_set_key)
	...
	FUNCTION_RETURN(aesni_set_key)

is the symmetry - it's a lot harder to misplace/miswrite these than two completely 
separately named things:

	ENTRY_FRAME(aesni_set_key)
	...
	ENDPROC_FRAME_RETURN(aesni_set_key)

Also, the 'FRAME' part will be pointless and somewhat misleading once we do 
dwarves, right?

Another valid variants would be:

	FUNCTION_ENTER(aesni_set_key)
	...
	FUNCTION_RET(aesni_set_key)

or:

	FUNCTION_START(aesni_set_key)
	...
	FUNCTION_RET(aesni_set_key)

or:

	ASM_FUNCTION_START(aesni_set_key)
	...
	ASM_FUNCTION_RET(aesni_set_key)

Note that the name has two parts:

 - The symmetric 'FUNCTION_' prefix tells us that this is a callable function that 
   we are defining. That is a very significant property of this construct, and 
   should be present in both the 'start' and the 'end' markers.

 - The '_RET' stresses the fact that it always generates a 'ret' instruction.

Note what the names _don't_ contain: that we generate debug info! That fact is not 
present in the naming, and that's very much intentional, because the precise form 
of debug info is conditional:

  - if CONFIG_FRAME_POINTERS=y then we push/pop a stack frame

  - if (later on) we do CFI annotations we don't push/pop a stack frame but emit 
    CFI debuginfo

In that sense 'FRAME' should never be in these names I think, nor 'PROC' (which is 
not symmetric).

Plus all 3 variants I suggested are very easy to remember, why I'd always have to 
look up any non-symmetric macro name called 'PROC'...

Thanks,

	Ingo

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Fri, Jul 17, 2015 at 02:01:33PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 17, 2015 at 1:59 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Jul 17, 2015 at 01:46:39PM -0700, Andy Lutomirski wrote:
> >> On Fri, Jul 17, 2015 at 1:44 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Fri, Jul 17, 2015 at 01:39:09PM -0700, Andy Lutomirski wrote:
> >> >> On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> >> > On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
> >> >> >> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >> >> >
> >> >> >> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> >> >> >
> >> >> >> >>  ENTRY(aesni_set_key)
> >> >> >> >> +     FRAME
> >> >> >> >>  #ifndef __x86_64__
> >> >> >> >>       pushl KEYP
> >> >> >> >>       movl 8(%esp), KEYP              # ctx
> >> >> >> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
> >> >> >> >>  #ifndef __x86_64__
> >> >> >> >>       popl KEYP
> >> >> >> >>  #endif
> >> >> >> >> +     ENDFRAME
> >> >> >> >>       ret
> >> >> >> >>  ENDPROC(aesni_set_key)
> >> >> >> >
> >> >> >> > So cannot we make this a bit more compact and less fragile?
> >> >> >> >
> >> >> >> > Instead of:
> >> >> >> >
> >> >> >> >         ENTRY(aesni_set_key)
> >> >> >> >                 FRAME
> >> >> >> >         ...
> >> >> >> >                 ENDFRAME
> >> >> >> >                 ret
> >> >> >> >         ENDPROC(aesni_set_key)
> >> >> >> >
> >> >> >> >
> >> >> >> > How about writing this as:
> >> >> >> >
> >> >> >> >         FUNCTION_ENTRY(aesni_set_key)
> >> >> >> >         ...
> >> >> >> >         FUNCTION_RETURN(aesni_set_key)
> >> >> >> >
> >> >> >> > which does the same thing in a short, symmetric construct?
> >> >> >> >
> >> >> >> > One potential problem with this approach would be that what 'looks' like an entry
> >> >> >> > declaration, but it will now generate real code.
> >> >> >> >
> >> >> >> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> >> >> >> > and I think 'RETURN' makes it clear enough that there's a real instruction
> >> >> >> > generated there.
> >> >> >> >
> >> >> >>
> >> >> >> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
> >> >> >
> >> >> > Perhaps the macro name should describe what the epilogue does, since
> >> >> > frame pointers aren't required for _all_ functions, only those which
> >> >> > don't have call instructions.
> >> >> >
> >> >> > What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
> >> >> > ending macro is kind of long, but at least it a) matches the existing
> >> >> > ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
> >> >> > pointers are involved; and c) lets you know that the return is there.
> >> >> >
> >> >>
> >> >> This really is about frame pointers, right?  How about
> >> >> ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
> >> >> whatever?
> >> >
> >> > Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of
> >> > a function?
> >>
> >> I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz
> >> (ENTRY is debatable, but that's what we currently have).  ENDPROC
> >> could easily be replaced with anything else.
> >
> > So do you mean ENTRY_FRAMEPTR_PROLOGUE and ENDPROC_FRAMEPTR_EPILOGUE?
> > Or something else?
> >
> 
> I like it.  I think this bikeshed might be well painted now!

Actually I'm not done painting.  Personally it seems a little too
verbose.  I still like ENTRY_FRAME and ENDPROC_FRAME_RETURN :p

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Andy Lutomirski]

On Fri, Jul 17, 2015 at 1:59 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Jul 17, 2015 at 01:46:39PM -0700, Andy Lutomirski wrote:
>> On Fri, Jul 17, 2015 at 1:44 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Fri, Jul 17, 2015 at 01:39:09PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >> > On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
>> >> >> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >> >
>> >> >> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >> >> >
>> >> >> >>  ENTRY(aesni_set_key)
>> >> >> >> +     FRAME
>> >> >> >>  #ifndef __x86_64__
>> >> >> >>       pushl KEYP
>> >> >> >>       movl 8(%esp), KEYP              # ctx
>> >> >> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
>> >> >> >>  #ifndef __x86_64__
>> >> >> >>       popl KEYP
>> >> >> >>  #endif
>> >> >> >> +     ENDFRAME
>> >> >> >>       ret
>> >> >> >>  ENDPROC(aesni_set_key)
>> >> >> >
>> >> >> > So cannot we make this a bit more compact and less fragile?
>> >> >> >
>> >> >> > Instead of:
>> >> >> >
>> >> >> >         ENTRY(aesni_set_key)
>> >> >> >                 FRAME
>> >> >> >         ...
>> >> >> >                 ENDFRAME
>> >> >> >                 ret
>> >> >> >         ENDPROC(aesni_set_key)
>> >> >> >
>> >> >> >
>> >> >> > How about writing this as:
>> >> >> >
>> >> >> >         FUNCTION_ENTRY(aesni_set_key)
>> >> >> >         ...
>> >> >> >         FUNCTION_RETURN(aesni_set_key)
>> >> >> >
>> >> >> > which does the same thing in a short, symmetric construct?
>> >> >> >
>> >> >> > One potential problem with this approach would be that what 'looks' like an entry
>> >> >> > declaration, but it will now generate real code.
>> >> >> >
>> >> >> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
>> >> >> > and I think 'RETURN' makes it clear enough that there's a real instruction
>> >> >> > generated there.
>> >> >> >
>> >> >>
>> >> >> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
>> >> >
>> >> > Perhaps the macro name should describe what the epilogue does, since
>> >> > frame pointers aren't required for _all_ functions, only those which
>> >> > don't have call instructions.
>> >> >
>> >> > What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
>> >> > ending macro is kind of long, but at least it a) matches the existing
>> >> > ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
>> >> > pointers are involved; and c) lets you know that the return is there.
>> >> >
>> >>
>> >> This really is about frame pointers, right?  How about
>> >> ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
>> >> whatever?
>> >
>> > Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of
>> > a function?
>>
>> I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz
>> (ENTRY is debatable, but that's what we currently have).  ENDPROC
>> could easily be replaced with anything else.
>
> So do you mean ENTRY_FRAMEPTR_PROLOGUE and ENDPROC_FRAMEPTR_EPILOGUE?
> Or something else?
>

I like it.  I think this bikeshed might be well painted now!

--Andy

> --
> Josh



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Fri, Jul 17, 2015 at 01:46:39PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 17, 2015 at 1:44 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Jul 17, 2015 at 01:39:09PM -0700, Andy Lutomirski wrote:
> >> On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
> >> >> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >> >
> >> >> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> >> >
> >> >> >>  ENTRY(aesni_set_key)
> >> >> >> +     FRAME
> >> >> >>  #ifndef __x86_64__
> >> >> >>       pushl KEYP
> >> >> >>       movl 8(%esp), KEYP              # ctx
> >> >> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
> >> >> >>  #ifndef __x86_64__
> >> >> >>       popl KEYP
> >> >> >>  #endif
> >> >> >> +     ENDFRAME
> >> >> >>       ret
> >> >> >>  ENDPROC(aesni_set_key)
> >> >> >
> >> >> > So cannot we make this a bit more compact and less fragile?
> >> >> >
> >> >> > Instead of:
> >> >> >
> >> >> >         ENTRY(aesni_set_key)
> >> >> >                 FRAME
> >> >> >         ...
> >> >> >                 ENDFRAME
> >> >> >                 ret
> >> >> >         ENDPROC(aesni_set_key)
> >> >> >
> >> >> >
> >> >> > How about writing this as:
> >> >> >
> >> >> >         FUNCTION_ENTRY(aesni_set_key)
> >> >> >         ...
> >> >> >         FUNCTION_RETURN(aesni_set_key)
> >> >> >
> >> >> > which does the same thing in a short, symmetric construct?
> >> >> >
> >> >> > One potential problem with this approach would be that what 'looks' like an entry
> >> >> > declaration, but it will now generate real code.
> >> >> >
> >> >> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> >> >> > and I think 'RETURN' makes it clear enough that there's a real instruction
> >> >> > generated there.
> >> >> >
> >> >>
> >> >> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
> >> >
> >> > Perhaps the macro name should describe what the epilogue does, since
> >> > frame pointers aren't required for _all_ functions, only those which
> >> > don't have call instructions.
> >> >
> >> > What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
> >> > ending macro is kind of long, but at least it a) matches the existing
> >> > ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
> >> > pointers are involved; and c) lets you know that the return is there.
> >> >
> >>
> >> This really is about frame pointers, right?  How about
> >> ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
> >> whatever?
> >
> > Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of
> > a function?
> 
> I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz
> (ENTRY is debatable, but that's what we currently have).  ENDPROC
> could easily be replaced with anything else.

So do you mean ENTRY_FRAMEPTR_PROLOGUE and ENDPROC_FRAMEPTR_EPILOGUE?
Or something else?

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Andy Lutomirski]

On Fri, Jul 17, 2015 at 1:44 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Jul 17, 2015 at 01:39:09PM -0700, Andy Lutomirski wrote:
>> On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >> >
>> >> >>  ENTRY(aesni_set_key)
>> >> >> +     FRAME
>> >> >>  #ifndef __x86_64__
>> >> >>       pushl KEYP
>> >> >>       movl 8(%esp), KEYP              # ctx
>> >> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
>> >> >>  #ifndef __x86_64__
>> >> >>       popl KEYP
>> >> >>  #endif
>> >> >> +     ENDFRAME
>> >> >>       ret
>> >> >>  ENDPROC(aesni_set_key)
>> >> >
>> >> > So cannot we make this a bit more compact and less fragile?
>> >> >
>> >> > Instead of:
>> >> >
>> >> >         ENTRY(aesni_set_key)
>> >> >                 FRAME
>> >> >         ...
>> >> >                 ENDFRAME
>> >> >                 ret
>> >> >         ENDPROC(aesni_set_key)
>> >> >
>> >> >
>> >> > How about writing this as:
>> >> >
>> >> >         FUNCTION_ENTRY(aesni_set_key)
>> >> >         ...
>> >> >         FUNCTION_RETURN(aesni_set_key)
>> >> >
>> >> > which does the same thing in a short, symmetric construct?
>> >> >
>> >> > One potential problem with this approach would be that what 'looks' like an entry
>> >> > declaration, but it will now generate real code.
>> >> >
>> >> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
>> >> > and I think 'RETURN' makes it clear enough that there's a real instruction
>> >> > generated there.
>> >> >
>> >>
>> >> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
>> >
>> > Perhaps the macro name should describe what the epilogue does, since
>> > frame pointers aren't required for _all_ functions, only those which
>> > don't have call instructions.
>> >
>> > What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
>> > ending macro is kind of long, but at least it a) matches the existing
>> > ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
>> > pointers are involved; and c) lets you know that the return is there.
>> >
>>
>> This really is about frame pointers, right?  How about
>> ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
>> whatever?
>
> Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of
> a function?

I meant ENTRY_FRAMEPTR_xyz and the beginning and ENDPROC_FRAMEPTR_xyz
(ENTRY is debatable, but that's what we currently have).  ENDPROC
could easily be replaced with anything else.

--Andy

>
> --
> Josh



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Fri, Jul 17, 2015 at 01:39:09PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
> >> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> >
> >> >>  ENTRY(aesni_set_key)
> >> >> +     FRAME
> >> >>  #ifndef __x86_64__
> >> >>       pushl KEYP
> >> >>       movl 8(%esp), KEYP              # ctx
> >> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
> >> >>  #ifndef __x86_64__
> >> >>       popl KEYP
> >> >>  #endif
> >> >> +     ENDFRAME
> >> >>       ret
> >> >>  ENDPROC(aesni_set_key)
> >> >
> >> > So cannot we make this a bit more compact and less fragile?
> >> >
> >> > Instead of:
> >> >
> >> >         ENTRY(aesni_set_key)
> >> >                 FRAME
> >> >         ...
> >> >                 ENDFRAME
> >> >                 ret
> >> >         ENDPROC(aesni_set_key)
> >> >
> >> >
> >> > How about writing this as:
> >> >
> >> >         FUNCTION_ENTRY(aesni_set_key)
> >> >         ...
> >> >         FUNCTION_RETURN(aesni_set_key)
> >> >
> >> > which does the same thing in a short, symmetric construct?
> >> >
> >> > One potential problem with this approach would be that what 'looks' like an entry
> >> > declaration, but it will now generate real code.
> >> >
> >> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> >> > and I think 'RETURN' makes it clear enough that there's a real instruction
> >> > generated there.
> >> >
> >>
> >> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
> >
> > Perhaps the macro name should describe what the epilogue does, since
> > frame pointers aren't required for _all_ functions, only those which
> > don't have call instructions.
> >
> > What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
> > ending macro is kind of long, but at least it a) matches the existing
> > ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
> > pointers are involved; and c) lets you know that the return is there.
> >
> 
> This really is about frame pointers, right?  How about
> ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
> whatever?

Wouldn't the "ENTRY" in ENTRY_FRAMEPTR_RETURN be confusing at the end of
a function?

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Andy Lutomirski]

On Fri, Jul 17, 2015 at 1:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
>> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> >
>> >>  ENTRY(aesni_set_key)
>> >> +     FRAME
>> >>  #ifndef __x86_64__
>> >>       pushl KEYP
>> >>       movl 8(%esp), KEYP              # ctx
>> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
>> >>  #ifndef __x86_64__
>> >>       popl KEYP
>> >>  #endif
>> >> +     ENDFRAME
>> >>       ret
>> >>  ENDPROC(aesni_set_key)
>> >
>> > So cannot we make this a bit more compact and less fragile?
>> >
>> > Instead of:
>> >
>> >         ENTRY(aesni_set_key)
>> >                 FRAME
>> >         ...
>> >                 ENDFRAME
>> >                 ret
>> >         ENDPROC(aesni_set_key)
>> >
>> >
>> > How about writing this as:
>> >
>> >         FUNCTION_ENTRY(aesni_set_key)
>> >         ...
>> >         FUNCTION_RETURN(aesni_set_key)
>> >
>> > which does the same thing in a short, symmetric construct?
>> >
>> > One potential problem with this approach would be that what 'looks' like an entry
>> > declaration, but it will now generate real code.
>> >
>> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
>> > and I think 'RETURN' makes it clear enough that there's a real instruction
>> > generated there.
>> >
>>
>> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?
>
> Perhaps the macro name should describe what the epilogue does, since
> frame pointers aren't required for _all_ functions, only those which
> don't have call instructions.
>
> What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
> ending macro is kind of long, but at least it a) matches the existing
> ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
> pointers are involved; and c) lets you know that the return is there.
>

This really is about frame pointers, right?  How about
ENTRY_FRAMEPTR_xyz where xyz can be prologue, epilogue, return,
whatever?

> --
> Josh



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

On Fri, Jul 17, 2015 at 12:44:42PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> >>  ENTRY(aesni_set_key)
> >> +     FRAME
> >>  #ifndef __x86_64__
> >>       pushl KEYP
> >>       movl 8(%esp), KEYP              # ctx
> >> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
> >>  #ifndef __x86_64__
> >>       popl KEYP
> >>  #endif
> >> +     ENDFRAME
> >>       ret
> >>  ENDPROC(aesni_set_key)
> >
> > So cannot we make this a bit more compact and less fragile?
> >
> > Instead of:
> >
> >         ENTRY(aesni_set_key)
> >                 FRAME
> >         ...
> >                 ENDFRAME
> >                 ret
> >         ENDPROC(aesni_set_key)
> >
> >
> > How about writing this as:
> >
> >         FUNCTION_ENTRY(aesni_set_key)
> >         ...
> >         FUNCTION_RETURN(aesni_set_key)
> >
> > which does the same thing in a short, symmetric construct?
> >
> > One potential problem with this approach would be that what 'looks' like an entry
> > declaration, but it will now generate real code.
> >
> > OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> > and I think 'RETURN' makes it clear enough that there's a real instruction
> > generated there.
> >
> 
> How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

Perhaps the macro name should describe what the epilogue does, since
frame pointers aren't required for _all_ functions, only those which
don't have call instructions.

What do you think about ENTRY_FRAME and ENDPROC_FRAME_RETURN?  The
ending macro is kind of long, but at least it a) matches the existing
ENTRY/ENDPROC convention for asm functions; b) gives a clue that frame
pointers are involved; and c) lets you know that the return is there.

-- 
Josh

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Andy Lutomirski]

On Fri, Jul 17, 2015 at 12:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
>>  ENTRY(aesni_set_key)
>> +     FRAME
>>  #ifndef __x86_64__
>>       pushl KEYP
>>       movl 8(%esp), KEYP              # ctx
>> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
>>  #ifndef __x86_64__
>>       popl KEYP
>>  #endif
>> +     ENDFRAME
>>       ret
>>  ENDPROC(aesni_set_key)
>
> So cannot we make this a bit more compact and less fragile?
>
> Instead of:
>
>         ENTRY(aesni_set_key)
>                 FRAME
>         ...
>                 ENDFRAME
>                 ret
>         ENDPROC(aesni_set_key)
>
>
> How about writing this as:
>
>         FUNCTION_ENTRY(aesni_set_key)
>         ...
>         FUNCTION_RETURN(aesni_set_key)
>
> which does the same thing in a short, symmetric construct?
>
> One potential problem with this approach would be that what 'looks' like an entry
> declaration, but it will now generate real code.
>
> OTOH if people find this intuitive enough then it's a lot harder to mess it up,
> and I think 'RETURN' makes it clear enough that there's a real instruction
> generated there.
>

How about FUNCTION_PROLOGUE and FUNCTION_EPILOGUE?

-Andy

Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

>  ENTRY(aesni_set_key)
> +	FRAME
>  #ifndef __x86_64__
>  	pushl KEYP
>  	movl 8(%esp), KEYP		# ctx
> @@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
>  #ifndef __x86_64__
>  	popl KEYP
>  #endif
> +	ENDFRAME
>  	ret
>  ENDPROC(aesni_set_key)

So cannot we make this a bit more compact and less fragile?

Instead of:

	ENTRY(aesni_set_key)
		FRAME
	...
		ENDFRAME
		ret
	ENDPROC(aesni_set_key)


How about writing this as:

	FUNCTION_ENTRY(aesni_set_key)
	...
	FUNCTION_RETURN(aesni_set_key)

which does the same thing in a short, symmetric construct?

One potential problem with this approach would be that what 'looks' like an entry 
declaration, but it will now generate real code.

OTOH if people find this intuitive enough then it's a lot harder to mess it up, 
and I think 'RETURN' makes it clear enough that there's a real instruction 
generated there.

Thanks,

	Ingo

[RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in aesni-intel_asm.S

[Josh Poimboeuf]

aesni-intel_asm.S has several callable non-leaf functions which don't
honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create stack frames for them when CONFIG_FRAME_POINTER is enabled.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/crypto/aesni-intel_asm.S | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
index 6bd2c6c..3df557b 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -31,6 +31,7 @@
 
 #include <linux/linkage.h>
 #include <asm/inst.h>
+#include <asm/frame.h>
 
 /*
  * The following macros are used to move an (un)aligned 16 byte value to/from
@@ -1800,6 +1801,7 @@ ENDPROC(_key_expansion_256b)
  *                   unsigned int key_len)
  */
 ENTRY(aesni_set_key)
+	FRAME
 #ifndef __x86_64__
 	pushl KEYP
 	movl 8(%esp), KEYP		# ctx
@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
 	popl KEYP
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_set_key)
 
@@ -1912,6 +1915,7 @@ ENDPROC(aesni_set_key)
  * void aesni_enc(struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
  */
 ENTRY(aesni_enc)
+	FRAME
 #ifndef __x86_64__
 	pushl KEYP
 	pushl KLEN
@@ -1927,6 +1931,7 @@ ENTRY(aesni_enc)
 	popl KLEN
 	popl KEYP
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_enc)
 
@@ -2101,6 +2106,7 @@ ENDPROC(_aesni_enc4)
  * void aesni_dec (struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
  */
 ENTRY(aesni_dec)
+	FRAME
 #ifndef __x86_64__
 	pushl KEYP
 	pushl KLEN
@@ -2117,6 +2123,7 @@ ENTRY(aesni_dec)
 	popl KLEN
 	popl KEYP
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_dec)
 
@@ -2292,6 +2299,7 @@ ENDPROC(_aesni_dec4)
  *		      size_t len)
  */
 ENTRY(aesni_ecb_enc)
+	FRAME
 #ifndef __x86_64__
 	pushl LEN
 	pushl KEYP
@@ -2342,6 +2350,7 @@ ENTRY(aesni_ecb_enc)
 	popl KEYP
 	popl LEN
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_ecb_enc)
 
@@ -2350,6 +2359,7 @@ ENDPROC(aesni_ecb_enc)
  *		      size_t len);
  */
 ENTRY(aesni_ecb_dec)
+	FRAME
 #ifndef __x86_64__
 	pushl LEN
 	pushl KEYP
@@ -2401,6 +2411,7 @@ ENTRY(aesni_ecb_dec)
 	popl KEYP
 	popl LEN
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_ecb_dec)
 
@@ -2409,6 +2420,7 @@ ENDPROC(aesni_ecb_dec)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_cbc_enc)
+	FRAME
 #ifndef __x86_64__
 	pushl IVP
 	pushl LEN
@@ -2443,6 +2455,7 @@ ENTRY(aesni_cbc_enc)
 	popl LEN
 	popl IVP
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_cbc_enc)
 
@@ -2451,6 +2464,7 @@ ENDPROC(aesni_cbc_enc)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_cbc_dec)
+	FRAME
 #ifndef __x86_64__
 	pushl IVP
 	pushl LEN
@@ -2534,6 +2548,7 @@ ENTRY(aesni_cbc_dec)
 	popl LEN
 	popl IVP
 #endif
+	ENDFRAME
 	ret
 ENDPROC(aesni_cbc_dec)
 
@@ -2598,6 +2613,7 @@ ENDPROC(_aesni_inc)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_ctr_enc)
+	FRAME
 	cmp $16, LEN
 	jb .Lctr_enc_just_ret
 	mov 480(KEYP), KLEN
@@ -2651,6 +2667,7 @@ ENTRY(aesni_ctr_enc)
 .Lctr_enc_ret:
 	movups IV, (IVP)
 .Lctr_enc_just_ret:
+	ENDFRAME
 	ret
 ENDPROC(aesni_ctr_enc)
 
@@ -2677,6 +2694,7 @@ ENDPROC(aesni_ctr_enc)
  *			 bool enc, u8 *iv)
  */
 ENTRY(aesni_xts_crypt8)
+	FRAME
 	cmpb $0, %cl
 	movl $0, %ecx
 	movl $240, %r10d
@@ -2777,6 +2795,7 @@ ENTRY(aesni_xts_crypt8)
 	pxor INC, STATE4
 	movdqu STATE4, 0x70(OUTP)
 
+	ENDFRAME
 	ret
 ENDPROC(aesni_xts_crypt8)
 
-- 
2.1.0