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

Will Deacon will at kernel.org
Tue Jan 7 06:44:46 PST 2020


Hi Mark,

[+Jiri -- couple of questions below]

On Mon, Jan 06, 2020 at 07:58:17PM +0000, Mark Brown wrote:
> In an effort to clarify and simplify the annotation of assembly functions
> in the kernel new macros have been introduced. These replace ENTRY and
> ENDPROC and also add a new annotation for static functions which previously
> had no ENTRY equivalent. Update the annotations in the library code to the
> new macros.
> 
> Signed-off-by: Mark Brown <broonie at kernel.org>
> ---
>  arch/arm64/lib/clear_page.S     |  4 ++--
>  arch/arm64/lib/clear_user.S     |  4 ++--
>  arch/arm64/lib/copy_from_user.S |  4 ++--
>  arch/arm64/lib/copy_in_user.S   |  4 ++--
>  arch/arm64/lib/copy_page.S      |  4 ++--
>  arch/arm64/lib/copy_to_user.S   |  4 ++--
>  arch/arm64/lib/crc32.S          |  8 ++++----
>  arch/arm64/lib/memchr.S         |  4 ++--
>  arch/arm64/lib/memcmp.S         |  4 ++--
>  arch/arm64/lib/memcpy.S         |  8 ++++----
>  arch/arm64/lib/memmove.S        |  8 ++++----
>  arch/arm64/lib/memset.S         |  8 ++++----
>  arch/arm64/lib/strchr.S         |  4 ++--
>  arch/arm64/lib/strcmp.S         |  4 ++--
>  arch/arm64/lib/strlen.S         |  4 ++--
>  arch/arm64/lib/strncmp.S        |  4 ++--
>  arch/arm64/lib/strnlen.S        |  4 ++--
>  arch/arm64/lib/strrchr.S        |  4 ++--
>  arch/arm64/lib/tishift.S        | 12 ++++++------
>  19 files changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S
> index 78a9ef66288a..073acbf02a7c 100644
> --- a/arch/arm64/lib/clear_page.S
> +++ b/arch/arm64/lib/clear_page.S
> @@ -14,7 +14,7 @@
>   * Parameters:
>   *	x0 - dest
>   */
> -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?

> diff --git a/arch/arm64/lib/crc32.S b/arch/arm64/lib/crc32.S
> index e6135f16649b..243e107e9896 100644
> --- a/arch/arm64/lib/crc32.S
> +++ b/arch/arm64/lib/crc32.S
> @@ -85,17 +85,17 @@ CPU_BE(	rev16		w3, w3		)
>  	.endm
>  
>  	.align		5
> -ENTRY(crc32_le)
> +SYM_FUNC_START(crc32_le)
>  alternative_if_not ARM64_HAS_CRC32
>  	b		crc32_le_base

Similar thing here -- I'm assuming we are ok to tail-call crc32_le_base()
from a function marked with SYM_FUNC_START like this?

>  alternative_else_nop_endif
>  	__crc32
> -ENDPROC(crc32_le)
> +SYM_FUNC_END(crc32_le)
>  
>  	.align		5
> -ENTRY(__crc32c_le)
> +SYM_FUNC_START(__crc32c_le)
>  alternative_if_not ARM64_HAS_CRC32
>  	b		__crc32c_le_base
>  alternative_else_nop_endif
>  	__crc32		c
> -ENDPROC(__crc32c_le)
> +SYM_FUNC_END(__crc32c_le)
> diff --git a/arch/arm64/lib/memchr.S b/arch/arm64/lib/memchr.S
> index 48a3ab636e4f..99910c5d5db7 100644
> --- a/arch/arm64/lib/memchr.S
> +++ b/arch/arm64/lib/memchr.S
> @@ -19,7 +19,7 @@
>   * Returns:
>   *	x0 - address of first occurrence of 'c' or 0
>   */
> -WEAK(memchr)
> +SYM_FUNC_START_PI_WEAK(memchr)
>  	and	w1, w1, #0xff
>  1:	subs	x2, x2, #1
>  	b.mi	2f
> @@ -30,5 +30,5 @@ WEAK(memchr)
>  	ret
>  2:	mov	x0, #0
>  	ret
> -ENDPIPROC(memchr)
> +SYM_FUNC_END_PI(memchr)
>  EXPORT_SYMBOL_NOKASAN(memchr)
> diff --git a/arch/arm64/lib/memcmp.S b/arch/arm64/lib/memcmp.S
> index b297bdaaf549..b889f312bdb3 100644
> --- a/arch/arm64/lib/memcmp.S
> +++ b/arch/arm64/lib/memcmp.S
> @@ -46,7 +46,7 @@ pos		.req	x11
>  limit_wd	.req	x12
>  mask		.req	x13
>  
> -WEAK(memcmp)
> +SYM_FUNC_START_PI_WEAK(memcmp)
>  	cbz	limit, .Lret0
>  	eor	tmp1, src1, src2
>  	tst	tmp1, #7
> @@ -243,5 +243,5 @@ CPU_LE( rev	data2, data2 )
>  .Lret0:
>  	mov	result, #0
>  	ret
> -ENDPIPROC(memcmp)
> +SYM_FUNC_END_PI(memcmp)
>  EXPORT_SYMBOL_NOKASAN(memcmp)
> diff --git a/arch/arm64/lib/memcpy.S b/arch/arm64/lib/memcpy.S
> index d79f48994dbb..9f382adfa88a 100644
> --- 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)

Will



More information about the linux-arm-kernel mailing list