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

[Ingo Molnar <mingo@kernel.org>]

* 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