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

From: Mark Brown <broonie@kernel.org>
To: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel@lists.infradead.org, jslaby@suse.com
Subject: Re: [PATCH 2/3] arm64: lib: Use modern annotations for assembly functions
Date: Tue, 7 Jan 2020 17:47:41 +0000	[thread overview]
Message-ID: <20200107174741.GG4877@sirena.org.uk> (raw)
In-Reply-To: <20200107144445.GC29001@willie-the-truck>

[-- Attachment #1.1: Type: text/plain, Size: 2104 bytes --]

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.

> > --- 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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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