diff mbox

Add ifunc memcpy and memmove for aarch64

Message ID 1487809656.2866.171.camel@caviumnetworks.com
State New, archived
Headers show

Commit Message

Steve Ellcey Feb. 23, 2017, 12:27 a.m. UTC
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

Siddhesh Poyarekar Feb. 23, 2017, 2:21 p.m. UTC | #1
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
Steve Ellcey Feb. 23, 2017, 4:20 p.m. UTC | #2
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
Siddhesh Poyarekar Feb. 23, 2017, 4:34 p.m. UTC | #3
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
Steve Ellcey Feb. 23, 2017, 4:42 p.m. UTC | #4
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
Siddhesh Poyarekar Feb. 23, 2017, 4:53 p.m. UTC | #5
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
Steve Ellcey March 1, 2017, 6:48 p.m. UTC | #6
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
Szabolcs Nagy March 14, 2017, 6:45 p.m. UTC | #7
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.
Andrew Pinski March 14, 2017, 6:51 p.m. UTC | #8
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

>
Steve Ellcey March 15, 2017, 11:52 p.m. UTC | #9
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
Siddhesh Poyarekar March 22, 2017, 5:37 a.m. UTC | #10
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
Joseph Myers March 22, 2017, 5:34 p.m. UTC | #11
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).
Siddhesh Poyarekar March 22, 2017, 5:52 p.m. UTC | #12
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
diff mbox

Patch

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 9a56dcb..ec19466 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -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 ();
 
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 4053ff3..c963a1e 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -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
diff --git a/sysdeps/aarch64/dl-procinfo.h b/sysdeps/aarch64/dl-procinfo.h
index e69de29..0e0829f 100644
--- a/sysdeps/aarch64/dl-procinfo.h
+++ b/sysdeps/aarch64/dl-procinfo.h
@@ -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 */
diff --git a/sysdeps/aarch64/ldsodefs.h b/sysdeps/aarch64/ldsodefs.h
index f277074..ba4ada3 100644
--- a/sysdeps/aarch64/ldsodefs.h
+++ b/sysdeps/aarch64/ldsodefs.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;
diff --git a/sysdeps/unix/sysv/linux/aarch64/Makefile b/sysdeps/unix/sysv/linux/aarch64/Makefile
index 6b4e620..d17dafe 100644
--- a/sysdeps/unix/sysv/linux/aarch64/Makefile
+++ b/sysdeps/unix/sysv/linux/aarch64/Makefile
@@ -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
diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
index e69de29..867e1ca 100644
--- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
+++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
@@ -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;
+    }
+}
diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
index e69de29..0b2a51b 100644
--- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
+++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
@@ -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  */
diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
index e69de29..438046a 100644
--- a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
+++ b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.c
@@ -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