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

Groups > linux.kernel > #1188200 > unrolled thread

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

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: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in  aesni-intel_asm.S Josh Poimboeuf <jpoimboe@redhat.com> - 2015-07-20 16:00 +0200
    Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in  aesni-intel_asm.S Ingo Molnar <mingo@kernel.org> - 2015-07-20 19:30 +0200
      Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in  aesni-intel_asm.S Josh Poimboeuf <jpoimboe@redhat.com> - 2015-07-20 20:10 +0200
        Re: [RFC PATCH 13/21] x86/asm/crypto: Fix frame pointer usage in  aesni-intel_asm.S Josh Poimboeuf <jpoimboe@redhat.com> - 2015-07-22 14:00 +0200

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1188388

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1188414

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1189843

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web