Re: [PATCH v3 8/8] aarch64: Add aarch64-specific files for memory tagging support

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 11/23/2020 15:42, Richard Earnshaw wrote:
> This final patch provides the architecture-specific implementation of
> the memory-tagging support hooks for aarch64.
...
> diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
> index d8e06d27b2..2e88fc84a4 100644
> --- a/sysdeps/aarch64/Makefile
> +++ b/sysdeps/aarch64/Makefile
> @@ -40,4 +40,9 @@ endif
>  
>  ifeq ($(subdir),misc)
>  sysdep_headers += sys/ifunc.h
> +sysdep_routines += __mtag_tag_region __mtag_new_tag __mtag_address_get_tag
> +endif
> +
> +ifeq ($(subdir),string)
> +sysdep_routines += __mtag_memset_tag
>  endif

this is ok, but i think all can go to misc.

> diff --git a/sysdeps/aarch64/__mtag_address_get_tag.S b/sysdeps/aarch64/__mtag_address_get_tag.S
> new file mode 100644
> index 0000000000..654c9d660c
> --- /dev/null
> +++ b/sysdeps/aarch64/__mtag_address_get_tag.S
> @@ -0,0 +1,31 @@
> +/* Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library.  If not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <sysdep.h>
> +

i'd like the content after sysdep.h to be in

  #ifdef _LIBC_MTAG

so we don't build unused armv8.5-a code into glibc
(which can cause trouble with old binutils).

> +#define ptr x0
> +
> +	.arch armv8.5-a
> +	.arch_extension memtag
> +
> +ENTRY (__libc_mtag_address_get_tag)
> +
> +	ldg	ptr, [ptr]
> +	ret
> +END (__libc_mtag_address_get_tag)
> +libc_hidden_builtin_def (__libc_mtag_address_get_tag)

i think we don't need libc_hidden_builtin_def here:
that is only necessary when we make this a public api
(by adding it to a Versions file) but then we need
other changes like libc_hidden_proto on declarations
as well. as long as these are only used from libc.so
no magic is needed.

same for the other functions.

> diff --git a/sysdeps/aarch64/__mtag_memset_tag.S b/sysdeps/aarch64/__mtag_memset_tag.S
> new file mode 100644
> index 0000000000..bc98dc49d2
> --- /dev/null
> +++ b/sysdeps/aarch64/__mtag_memset_tag.S
> @@ -0,0 +1,46 @@
...
> +#include <sysdep.h>
> +/* Use the same register names and assignments as memset.  */
> +#include "memset-reg.h"
> +
> +	.arch armv8.5-a
> +	.arch_extension memtag
> +
> +/* NB, only supported on variants with 64-bit pointers.  */
> +
> +/* FIXME: This is a minimal implementation.  We could do much better than
> +   this for large values of COUNT.  */
> +
> +ENTRY_ALIGN(__libc_mtag_memset_with_tag, 6)

this should be plain ENTRY
(which defaults to 2^6 alignment)

same for __libc_mtag_tag_region.

> +
> +	and	valw, valw, 255
> +	orr	valw, valw, valw, lsl 8
> +	orr	valw, valw, valw, lsl 16
> +	orr	val, val, val, lsl 32
> +	mov	dst, dstin
> +
> +L(loop):
> +	stgp	val, val, [dst], #16
> +	subs	count, count, 16
> +	bne	L(loop)
> +	ldg	dstin, [dstin] // Recover the tag created (might be untagged).

i think ldg is not needed: for now we only use
this on PROT_MTE enabled memory.

(calling the function with non-heap memory or
disabling mte on heap memory can be treated as
a bug. this decision can be changed later if
we want to expose public mtag api to user code:
in particular the unwinder for stack tagging
may want to use such api, but we don't need to
design that right now.)

same for __libc_mtag_tag_region.

> +	ret
> +END (__libc_mtag_memset_with_tag)
> +libc_hidden_builtin_def (__libc_mtag_memset_with_tag)
> diff --git a/sysdeps/aarch64/__mtag_new_tag.S b/sysdeps/aarch64/__mtag_new_tag.S
> new file mode 100644
> index 0000000000..3a22995e9f
> --- /dev/null
> +++ b/sysdeps/aarch64/__mtag_new_tag.S
> @@ -0,0 +1,38 @@
...
> +#include <sysdep.h>
> +
> +	.arch armv8.5-a
> +	.arch_extension memtag
> +
> +/* NB, only supported on variants with 64-bit pointers.  */
> +
> +/* FIXME: This is a minimal implementation.  We could do better than
> +   this for larger values of COUNT.  */

this fixme is not needed here.

> +
> +#define ptr x0
> +#define xset x1
> +
> +ENTRY(__libc_mtag_new_tag)
> +	// Guarantee that the new tag is not the same as now.
> +	gmi	xset, ptr, xzr
> +	irg	ptr, ptr, xset
> +	ret
> +END (__libc_mtag_new_tag)
> +libc_hidden_builtin_def (__libc_mtag_new_tag)
...

rest looked ok.