Add ifunc memcpy and memmove for aarch64
Commit Message
Here is a new version of the IFUNC functionality for aarch64. It does
not include the memcpy changes to use it. I tried to move the cpu
feature check to later in the start up process so that I could check
GLRO(dl_hwcap_mask) but it does not seem to be working. I was
wondering if anyone could look at this and see if I am doing something
wrong.
If I understand the code correctly, for a dynamically linked program,
dl_main is going to get called before dl_sysdep_start and dl_main is
where environment variables are processed and where dl_hwcap_mask
should be getting set. But when I check dl_hwcap_mask in
init_cpu_features (called from dl_sysdep_start), it does not seem to
have changed from its original value that is set in the new
dl-procinfo.h header file.
Any ideas on why this isn't working?
Steve Ellcey
sellcey@cavium.com
2017-02-22 Steve Ellcey <sellcey@caviumnetworks.com>
Adhemerval Zanella <adhemerval.zanella@linaro.org>
* csu/libc-start.c (LIBC_START_MAIN): Use INIT_CPU_FEATURES.
* elf/dl-sysdep.c (_dl_sysdep_start): Likewise.
* sysdeps/aarch64/dl-procinfo.h: New file.
* sysdeps/aarch64/ldsodefs.h: Include cpu-features.h.
* sysdeps/unix/sysv/linux/aarch64/Makefile (sysdep_routines):
Add cpu-features.
* sysdeps/unix/sysv/linux/aarch64/cpu-features.c: New file.
* sysdeps/unix/sysv/linux/aarch64/cpu-features.h: Likewise.
* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c: Likewise.
Comments
On Thursday 23 February 2017 05:57 AM, Steve Ellcey wrote:
> Here is a new version of the IFUNC functionality for aarch64. It does
> not include the memcpy changes to use it. I tried to move the cpu
> feature check to later in the start up process so that I could check
> GLRO(dl_hwcap_mask) but it does not seem to be working. I was
> wondering if anyone could look at this and see if I am doing something
> wrong.
>
> If I understand the code correctly, for a dynamically linked program,
> dl_main is going to get called before dl_sysdep_start and dl_main is
> where environment variables are processed and where dl_hwcap_mask
> should be getting set. But when I check dl_hwcap_mask in
> init_cpu_features (called from dl_sysdep_start), it does not seem to
> have changed from its original value that is set in the new
> dl-procinfo.h header file.
dl_sysdep_start calls dl_main, so you've got the order wrong. If you
want init_cpu_features to read dl_hwcap_mask then you'll have to move
the code to read LD_* environment variables earlier. In fact they need
to go into tunables, something I've had in mind for this release.
Do you want me to move the envvars into tunables or would you like to do it?
Siddhesh
On Thu, 2017-02-23 at 19:51 +0530, Siddhesh Poyarekar wrote:
>
> dl_sysdep_start calls dl_main, so you've got the order wrong. If you
> want init_cpu_features to read dl_hwcap_mask then you'll have to move
> the code to read LD_* environment variables earlier. In fact they need
> to go into tunables, something I've had in mind for this release.
>
> Do you want me to move the envvars into tunables or would you like to
> do it?
>
> Siddhesh
If you want to move it, that would be great.
Steve Ellcey
sellcey@cavium.com
On Thursday 23 February 2017 09:50 PM, Steve Ellcey wrote:
> If you want to move it, that would be great.
OK, I'll get started on it. Meanwhile, it would be nice to get the
earlier patch in since it works well for everything except
dl_hwcap_mask. Do the aarch64 machine maintainers have a strong opinion
on this?
Siddhesh
On Thu, 2017-02-23 at 22:04 +0530, Siddhesh Poyarekar wrote:
> On Thursday 23 February 2017 09:50 PM, Steve Ellcey wrote:
> >
> > If you want to move it, that would be great.
> OK, I'll get started on it. Meanwhile, it would be nice to get the
> earlier patch in since it works well for everything except
> dl_hwcap_mask. Do the aarch64 machine maintainers have a strong opinion
> on this?
>
> Siddhesh
Just to be clear, the earlier patch would be this one, right?
https://sourceware.org/ml/libc-alpha/2017-02/msg00175.html
Steve Ellcey
sellcey@cavium.com
On Thursday 23 February 2017 10:12 PM, Steve Ellcey wrote:
> Just to be clear, the earlier patch would be this one, right?
>
> https://sourceware.org/ml/libc-alpha/2017-02/msg00175.html
Yes. It does everything necessary to get multiarch enabled and working
correctly for kernels that support it. The hwcap_mask feature is
something that can be added on top and I can commit to doing that within
this release.
Siddhesh
On Thu, 2017-02-23 at 22:23 +0530, Siddhesh Poyarekar wrote:
> On Thursday 23 February 2017 10:12 PM, Steve Ellcey wrote:
> >
> > Just to be clear, the earlier patch would be this one, right?
> >
> > https://sourceware.org/ml/libc-alpha/2017-02/msg00175.html
> Yes. It does everything necessary to get multiarch enabled and working
> correctly for kernels that support it. The hwcap_mask feature is
> something that can be added on top and I can commit to doing that within
> this release.
>
> Siddhesh
Ping. Does anyone have any comments or objections to this patch
that enables IFUNC on aarch64?
Steve Ellcey
On 01/03/17 18:48, Steve Ellcey wrote:
> On Thu, 2017-02-23 at 22:23 +0530, Siddhesh Poyarekar wrote:
>> On Thursday 23 February 2017 10:12 PM, Steve Ellcey wrote:
>>>
>>> Just to be clear, the earlier patch would be this one, right?
>>>
>>> https://sourceware.org/ml/libc-alpha/2017-02/msg00175.html
>> Yes. It does everything necessary to get multiarch enabled and working
>> correctly for kernels that support it. The hwcap_mask feature is
>> something that can be added on top and I can commit to doing that within
>> this release.
>>
>> Siddhesh
>
> Ping. Does anyone have any comments or objections to this patch
> that enables IFUNC on aarch64?
the patch looks ok, with HWCAP in bits/hwcap.h instead of
+/* Needed here until this gets into kernel sources. */
+#ifndef HWCAP_CPUID
+# define HWCAP_CPUID (1 << 11)
+#endif
the hwcap value is not yet in linux v4.10, but already
allocated, if we are committed to this value then i
think it's better to only have it in one place.
you may need to include bis/hwcap.h in some files.
i was not sure if we should wait for this to be in a
linux release, but i guess the value won't change now.
an unrelated issue i was wondering about is how the
upcoming ilp32 ifunc resolver will receive the hwcap
argument: will it only see 32bit (unsigned long hwcap)
or 64 bits (with different ifunc resolver prototype)
and if there is something in this area that needs to
be changed before ifuncs are used.
On Tue, Mar 14, 2017 at 11:45 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> On 01/03/17 18:48, Steve Ellcey wrote:
>> On Thu, 2017-02-23 at 22:23 +0530, Siddhesh Poyarekar wrote:
>>> On Thursday 23 February 2017 10:12 PM, Steve Ellcey wrote:
>>>>
>>>> Just to be clear, the earlier patch would be this one, right?
>>>>
>>>> https://sourceware.org/ml/libc-alpha/2017-02/msg00175.html
>>> Yes. It does everything necessary to get multiarch enabled and working
>>> correctly for kernels that support it. The hwcap_mask feature is
>>> something that can be added on top and I can commit to doing that within
>>> this release.
>>>
>>> Siddhesh
>>
>> Ping. Does anyone have any comments or objections to this patch
>> that enables IFUNC on aarch64?
>
> the patch looks ok, with HWCAP in bits/hwcap.h instead of
>
> +/* Needed here until this gets into kernel sources. */
> +#ifndef HWCAP_CPUID
> +# define HWCAP_CPUID (1 << 11)
> +#endif
>
> the hwcap value is not yet in linux v4.10, but already
> allocated, if we are committed to this value then i
> think it's better to only have it in one place.
> you may need to include bis/hwcap.h in some files.
>
> i was not sure if we should wait for this to be in a
> linux release, but i guess the value won't change now.
It is in 4.11rc1 though. Is that ok enough?
>
> an unrelated issue i was wondering about is how the
> upcoming ilp32 ifunc resolver will receive the hwcap
> argument: will it only see 32bit (unsigned long hwcap)
> or 64 bits (with different ifunc resolver prototype)
> and if there is something in this area that needs to
> be changed before ifuncs are used.
Right now, the hwcap is not using the full 64bit. But the kernel will
have to split it into hwcap and hwcap2 anyways when we hit that point.
Most likely we should have the ifunc resolver take an uint64_t now
rather than latter.
Thanks,
Andrew Pinski
>
On Tue, 2017-03-14 at 18:45 +0000, Szabolcs Nagy wrote:
>
> the hwcap value is not yet in linux v4.10, but already
> allocated, if we are committed to this value then i
> think it's better to only have it in one place.
> you may need to include bis/hwcap.h in some files.
I checked this patch in after moving the HWCAP_CPUID to bits/hwcap.h
and adding an include of sys/auxv.h to cpu-features.c to get the value.
When I added an include of bits/hwcap.h directly I got an error about
including auxv.h instead of hwcap.h.
Steve Ellcey
sellcey@cavium.com
On Thursday 16 March 2017 05:22 AM, Steve Ellcey wrote:
> I checked this patch in after moving the HWCAP_CPUID to bits/hwcap.h
> and adding an include of sys/auxv.h to cpu-features.c to get the value.
> When I added an include of bits/hwcap.h directly I got an error about
> including auxv.h instead of hwcap.h.
Technically you still needed to get an ack from Marcus who is the only
machine maintainer for aarch64. Practically though, the patch is fine
and it appears that Marcus hasn't had the time to review the patch and
it doesn't make sense to wait too long for something that has had
all-round consensus.
That said, it would be nice to have one of two things happen going forward:
Either Marcus names one or more machine maintainers for aarch64 that
have the bandwidth to review and approve aarch64 patches for glibc. I
would like to propose Adhemerval as an aarch64 machine maintainer too
since he has the necessary experience in glibc and also the involvement
in aarch64 work.
Alternatively, make aarch64 patch review open like x86. This might be
the way forward given that aarch64 development interests are getting
increasingly diverse (probably more so than x86) and it may become quite
difficult for a single maintainer from ARM to gate everything that goes in.
Siddhesh
On Wed, 22 Mar 2017, Siddhesh Poyarekar wrote:
> Technically you still needed to get an ack from Marcus who is the only
> machine maintainer for aarch64. Practically though, the patch is fine
No, machine patches are subject to consensus just like any other patches,
and such consensus can be reached without needing a machine maintainer to
comment (although one might hope they would review most substantial
patches for their machine).
On Wednesday 22 March 2017 11:04 PM, Joseph Myers wrote:
> No, machine patches are subject to consensus just like any other patches,
> and such consensus can be reached without needing a machine maintainer to
> comment (although one might hope they would review most substantial
> patches for their machine).
That is not very clear from the Consensus wiki page, so I added the
following to the machine maintainer section:
* If you are not a maintainer for the machine you're proposing the
change for, your patches are subject to consensus like any other
patches and while review from a machine maintainer may be ideal, it
is not strictly necessary for the patch to be accepted.
Siddhesh
@@ -182,6 +182,10 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
__tunables_init (__environ);
+#ifdef INIT_CPU_FEATURES
+ INIT_CPU_FEATURES;
+#endif
+
/* Perform IREL{,A} relocations. */
apply_irel ();
@@ -223,6 +223,10 @@ _dl_sysdep_start (void **start_argptr,
__tunables_init (_environ);
+#ifdef INIT_CPU_FEATURES
+ INIT_CPU_FEATURES;
+#endif
+
#ifdef DL_SYSDEP_INIT
DL_SYSDEP_INIT;
#endif
@@ -0,0 +1,50 @@
+/* Stub version of processor capability information handling macros.
+ Copyright (C) 1998-2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Ulrich Drepper <drepper@cygnus.com>, 1998.
+
+ 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/>. */
+
+#ifndef _DL_PROCINFO_H
+#define _DL_PROCINFO_H 1
+
+/* We cannot provide a general printing function. */
+#define _dl_procinfo(type, word) -1
+
+/* There are no hardware capabilities defined. */
+#define _dl_hwcap_string(idx) ""
+
+/* There are no different platforms defined. */
+#define _dl_platform_string(idx) ""
+
+/* Needed here until this gets into kernel sources. */
+#ifndef HWCAP_CPUID
+# define HWCAP_CPUID (1 << 11)
+#endif
+
+/* By default there is no important hardware capability. */
+#define HWCAP_IMPORTANT (HWCAP_CPUID)
+
+/* There're no platforms to filter out. */
+#define _DL_HWCAP_PLATFORM 0
+
+/* We don't have any hardware capabilities. */
+#define _DL_HWCAP_COUNT 0
+
+#define _dl_string_hwcap(str) (-1)
+
+#define _dl_string_platform(str) (-1)
+
+#endif /* dl-procinfo.h */
@@ -20,6 +20,7 @@
#define _AARCH64_LDSODEFS_H 1
#include <elf.h>
+#include <cpu-features.h>
struct La_aarch64_regs;
struct La_aarch64_retval;
@@ -1,5 +1,6 @@
ifeq ($(subdir),csu)
sysdep_routines += __read_tp libc-__read_tp
+sysdep_routines += cpu-features
static-only-routines += __read_tp
shared-only-routines += libc-__read_tp
endif
@@ -0,0 +1,45 @@
+/* Initialize CPU feature data. AArch64 version.
+ This file is part of the GNU C Library.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ 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 <cpu-features.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <ldsodefs.h>
+#include <exit-thread.h>
+#include <libc-internal.h>
+
+#ifndef HWCAP_CPUID
+# define HWCAP_CPUID (1 << 11)
+#endif
+
+void
+init_cpu_features (struct cpu_features *cpu_features)
+{
+ if (HWCAP_CPUID & GLRO(dl_hwcap) & GLRO(dl_hwcap_mask))
+ {
+ register uint64_t id = 0;
+ asm volatile ("mrs %0, midr_el1" : "=r"(id));
+ cpu_features->midr_el1 = id;
+ }
+ else
+ {
+ cpu_features->midr_el1 = 0;
+ }
+}
@@ -0,0 +1,53 @@
+/* Initialize CPU feature data. AArch64 version.
+ This file is part of the GNU C Library.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ 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/>. */
+
+#ifndef _CPU_FEATURES_AARCH64_H
+#define _CPU_FEATURES_AARCH64_H
+
+#include <stdint.h>
+
+#define MIDR_PARTNUM_SHIFT 4
+#define MIDR_PARTNUM_MASK (0xfff << MIDR_PARTNUM_SHIFT)
+#define MIDR_PARTNUM(midr) \
+ (((midr) & MIDR_PARTNUM_MASK) >> MIDR_PARTNUM_SHIFT)
+#define MIDR_ARCHITECTURE_SHIFT 16
+#define MIDR_ARCHITECTURE_MASK (0xf << MIDR_ARCHITECTURE_SHIFT)
+#define MIDR_ARCHITECTURE(midr) \
+ (((midr) & MIDR_ARCHITECTURE_MASK) >> MIDR_ARCHITECTURE_SHIFT)
+#define MIDR_VARIANT_SHIFT 20
+#define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT)
+#define MIDR_VARIANT(midr) \
+ (((midr) & MIDR_VARIANT_MASK) >> MIDR_VARIANT_SHIFT)
+#define MIDR_IMPLEMENTOR_SHIFT 24
+#define MIDR_IMPLEMENTOR_MASK (0xff << MIDR_IMPLEMENTOR_SHIFT)
+#define MIDR_IMPLEMENTOR(midr) \
+ (((midr) & MIDR_IMPLEMENTOR_MASK) >> MIDR_IMPLEMENTOR_SHIFT)
+
+#define IS_THUNDERX(midr) (MIDR_IMPLEMENTOR(midr) == 'C' \
+ && MIDR_PARTNUM(midr) == 0x0a1)
+
+struct cpu_features
+{
+ uint64_t midr_el1;
+};
+
+void init_cpu_features (struct cpu_features *cpu_features);
+
+#define INIT_CPU_FEATURES init_cpu_features(&GLRO(dl_aarch64_cpu_features))
+
+#endif /* _CPU_FEATURES_AARCH64_H */
@@ -0,0 +1,60 @@
+/* Data for AArch64 version of processor capability information.
+ Linux version.
+ Copyright (C) 2017 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/>. */
+
+/* If anything should be added here check whether the size of each string
+ is still ok with the given array size.
+
+ All the #ifdefs in the definitions are quite irritating but
+ necessary if we want to avoid duplicating the information. There
+ are three different modes:
+
+ - PROCINFO_DECL is defined. This means we are only interested in
+ declarations.
+
+ - PROCINFO_DECL is not defined:
+
+ + if SHARED is defined the file is included in an array
+ initializer. The .element = { ... } syntax is needed.
+
+ + if SHARED is not defined a normal array initialization is
+ needed.
+ */
+
+#ifndef PROCINFO_CLASS
+# define PROCINFO_CLASS
+#endif
+
+#if !IS_IN (ldconfig)
+# if !defined PROCINFO_DECL && defined SHARED
+ ._dl_aarch64_cpu_features
+# else
+PROCINFO_CLASS struct cpu_features _dl_aarch64_cpu_features
+# endif
+# ifndef PROCINFO_DECL
+= { }
+# endif
+# if !defined SHARED || defined PROCINFO_DECL
+;
+# else
+,
+# endif
+#endif
+
+#undef PROCINFO_DECL
+#undef PROCINFO_CLASS