Re: [PATCH 2/3] arm64: lib: Use modern annotations for assembly functions

[Will Deacon <will@kernel.org>]

On Tue, Jan 07, 2020 at 05:47:41PM +0000, Mark Brown wrote:
> On Tue, Jan 07, 2020 at 02:44:46PM +0000, Will Deacon wrote:
> > On Mon, Jan 06, 2020 at 07:58:17PM +0000, Mark Brown wrote:
> 
> > > -ENTRY(clear_page)
> > > +SYM_FUNC_START(clear_page)
> > >  	mrs	x1, dczid_el0
> > >  	and	w1, w1, #0xf
> > >  	mov	x2, #4
> 
> > Since this doesn't change behaviour, I think the patch is fine, however
> > on reading Documentation/asm-annotations.rst it's not completely clear to
> > me when SYM_FUNC_START() should be used. In this case, for example, we are
> > *not* pushing a stackframe and that would appear to be at odds with the
> > documentation.
> 
> > Jiri -- is it ok to omit the stack frame for leaf functions annotated with
> > SYM_FUNC_START? I'm guessing it should be, since the link register isn't
> > going to be clobbered. Could we update the documentation to reflect that?
> 
> Yeah, the documentation isn't great on that.  I was going on the basis
> of both trying to minimize changes to the generated output as part of
> the bulk change and looking at it from the point of view of the caller -
> if as in this case the caller thinks it's a regular C function it seems
> sensible to annotate it as such.

Maybe a small tweak to the documentation as per below, indicating that the
stack stuff is just an x86-specific example?

Jiri?

diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index f55c2bb74d00..32ea57483378 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -73,10 +73,11 @@ The new macros are prefixed with the ``SYM_`` prefix and can be divided into
 three main groups:
 
 1. ``SYM_FUNC_*`` -- to annotate C-like functions. This means functions with
-   standard C calling conventions, i.e. the stack contains a return address at
-   the predefined place and a return from the function can happen in a
-   standard way. When frame pointers are enabled, save/restore of frame
-   pointer shall happen at the start/end of a function, respectively, too.
+   standard C calling conventions. For example, on x86, this means that the
+   stack contains a return address at the predefined place and a return from
+   the function can happen in a standard way. When frame pointers are enabled,
+   save/restore of frame pointer shall happen at the start/end of a function,
+   respectively, too.
 
    Checking tools like ``objtool`` should ensure such marked functions conform
    to these rules. The tools can also easily annotate these functions with

> > > --- a/arch/arm64/lib/memcpy.S
> > > +++ b/arch/arm64/lib/memcpy.S
> > > @@ -57,11 +57,11 @@
> > >  	.endm
> > >  
> > >  	.weak memcpy
> 
> > Hmm, any idea why we use .weak explicitly here? Maybe better off using
> > the proper macros now? (same applies to many of the other lib/ functions
> > you're touching)
> 
> Nope, there's a whole bunch of stuff where what we're currently doing is
> a bit interesting and I'm a bit worried that we might be relying on some
> of it.  My theory here was to do the bulk of the changes as a 1:1
> replacement so the generated output is as close as possible for any big
> changes and then do anything more detailed that isn't actually *needed*
> on top of that.  It's looking like there'll also be some stuff that
> definitely changes the output going in as well, I was going to do those
> as individual patches so that it's easier to find any breakages that get
> introduced and so the big, repetitive changes don't have other stuff
> mixed in.

Fair enough, I'll queue these three with the minor naming change and you can
send extra stuff on top of that.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel