| Message ID | 6D39441BF12EF246A7ABCE6654B0235320F271C0@LEMAIL01.le.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 14 Oct 2014, Matthew Fortune wrote: > +#define HWCAP_MIPS_R6 0x00000001 > +#define HWCAP_MIPS_MSA 0x00000002 HWCAP values are assigned by the kernel; they are not ELF-level interfaces but interfaces between the kernel and userspace; we normally add new values once there's been a kernel.org release with those values in, as part of the routine review of new kernel releases for any new interfaces of relevance to glibc. In this case, I don't see these values in kernel.org git at all, so it seems premature to add them to glibc; please resubmit this patch once the values have been assigned in the kernel (or if one value is assigned before the other, it's fine to submit a patch with just that value as soon as it's been assigned). If I've missed the definition of these values in the kernel, please give more details of where they are defined (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git rather than any other, e.g. architecture-specific, git tree). Being Linux-specific, I think the right location for these files is sysdeps/unix/sysv/linux/mips/ similar to ARM (although other architectures putting such OS-specific definitions in OS-independent files doesn't cause practical problems in the absence of other-OS ports for those architectures). If you want to get other patches depending on HWCAP tests in some way into glibc before the HWCAP values are allocated in the kernel, I advise setting up the glibc code to hardcode whatever it would do with an old kernel that never sets a nonzero HWCAP, and then adding support for handling new kernel features later when the values are allocated.
Joseph S. Myers <joseph@codesourcery.com> writes > On Tue, 14 Oct 2014, Matthew Fortune wrote: > > > +#define HWCAP_MIPS_R6 0x00000001 > > +#define HWCAP_MIPS_MSA 0x00000002 > > HWCAP values are assigned by the kernel; they are not ELF-level interfaces > but interfaces between the kernel and userspace; we normally add new > values once there's been a kernel.org release with those values in, as > part of the routine review of new kernel releases for any new interfaces > of relevance to glibc. In this case, I don't see these values in For architecture independent interfaces/extensions I completely agree as it is not possible to predict what values will be assigned. In this case we have a spec which the kernel and glibc can follow for MIPS. I'm not sure I see the need to wait for the kernel changes to hit the main kernel repo. I.e. if we had agreement on a spec between an architecture maintainer for the kernel and glibc then that would seem sufficient. > Being Linux-specific, I think the right location for these files is > sysdeps/unix/sysv/linux/mips/ similar to ARM (although other architectures > putting such OS-specific definitions in OS-independent files doesn't cause > practical problems in the absence of other-OS ports for those > architectures). OK. > If you want to get other patches depending on HWCAP tests in some way into > glibc before the HWCAP values are allocated in the kernel, I advise > setting up the glibc code to hardcode whatever it would do with an old > kernel that never sets a nonzero HWCAP, and then adding support for > handling new kernel features later when the values are allocated. Given the current approach is to wait for changes to hit the kernel I'll skip this for now and hard-code as suggested. Thanks, Matthew
On Wed, 15 Oct 2014, Matthew Fortune wrote: > For architecture independent interfaces/extensions I completely agree as it > is not possible to predict what values will be assigned. In this case we > have a spec which the kernel and glibc can follow for MIPS. I'm not sure I > see the need to wait for the kernel changes to hit the main kernel repo. > > I.e. if we had agreement on a spec between an architecture maintainer for > the kernel and glibc then that would seem sufficient. I don't think it's wise to assume that these particular values will be the first ones getting into the kernel, rather than someone else deciding to implement some other HWCAP values on MIPS and getting those in first (and thereby taking the first available values, which is the natural choice). Cf. cases where people have had out-of-tree patches for socket address/protocol families and have had to change the numbers assigned because of others going in the kernel and taking those values. If the values get in the kernel quickly, there isn't much delay in glibc. If they take as long to get in the kernel as NaN2008 support (where more than a year after glibc support went in, glibc built for NaN2008 still uses arch_minimum_kernel=10.0.0 because the kernel support still hasn't gone in), that's a very long time to hope that no-one else gets in some other MIPS HWCAP values. I've no idea if you can persuade kernel people of the merits of defining a value in a kernel header to fix the ABI even if the actual implementation follows later (cf. the O_* discussions, though I think the conclusion there ended up being that the kernel should implement the actual semantics rather than just reserving values for userspace implementation).
On 14 Oct 2014 21:51, Matthew Fortune wrote: > +static inline int > +__attribute__ ((unused)) > +_dl_procinfo (unsigned int type, unsigned long int word) i think we just use "unsigned long". pairing "int" with "long" is just a waste of space. > +{ > + int i; > + > + /* Fallback to unknown output mechanism. */ > + if (type == AT_HWCAP2) > + return -1; > + > + _dl_printf ("AT_HWCAP: "); > + > + for (i = 0; i < _DL_HWCAP_COUNT; ++i) > + if (word & (1 << i)) i is an int, but word is an unsigned long int. probably want to harmonize those types. > +static inline const char * > +__attribute__ ((unused)) > +_dl_hwcap_string (int idx) > +{ > + return GLRO(dl_mips_cap_flags)[idx]; > +}; no trailing semi-colons on func defs > +static inline int > +__attribute__ ((unused)) > +_dl_string_hwcap (const char *str) > +{ > + int i; > > -#define _dl_string_hwcap(str) (-1) > + for (i = 0; i < _DL_HWCAP_COUNT; i++) nit: ++i > + { > + if (strcmp (str, GLRO(dl_mips_cap_flags)[i]) == 0) > + return i; > + } no need for the braces > + return -1; GNU style says to put a blank line above the return > +}; no trailing semi-colons on func defs -mike
Mike Frysinger <vapier@gentoo.org> writes: >> +{ >> + int i; >> + >> + /* Fallback to unknown output mechanism. */ >> + if (type == AT_HWCAP2) >> + return -1; >> + >> + _dl_printf ("AT_HWCAP: "); >> + >> + for (i = 0; i < _DL_HWCAP_COUNT; ++i) >> + if (word & (1 << i)) > > i is an int, but word is an unsigned long int. That's not a problem. The problem is that 1 is an int. Andreas.
On 03/06/2015 10:52 AM, Andreas Schwab wrote: > Mike Frysinger <vapier@gentoo.org> writes: > >>> +{ >>> + int i; >>> + >>> + /* Fallback to unknown output mechanism. */ >>> + if (type == AT_HWCAP2) >>> + return -1; >>> + >>> + _dl_printf ("AT_HWCAP: "); >>> + >>> + for (i = 0; i < _DL_HWCAP_COUNT; ++i) >>> + if (word & (1 << i)) >> >> i is an int, but word is an unsigned long int. > > That's not a problem. The problem is that 1 is an int. Right, you want 1U. Which reminds me that elf.h is all wrong also and needs 1U everywhere. Cheers, Carlos.
Carlos O'Donell <carlos@redhat.com> writes: > On 03/06/2015 10:52 AM, Andreas Schwab wrote: > > Mike Frysinger <vapier@gentoo.org> writes: > > > >>> +{ > >>> + int i; > >>> + > >>> + /* Fallback to unknown output mechanism. */ if (type == > >>> + AT_HWCAP2) > >>> + return -1; > >>> + > >>> + _dl_printf ("AT_HWCAP: "); > >>> + > >>> + for (i = 0; i < _DL_HWCAP_COUNT; ++i) > >>> + if (word & (1 << i)) > >> > >> i is an int, but word is an unsigned long int. > > > > That's not a problem. The problem is that 1 is an int. > > Right, you want 1U. > > Which reminds me that elf.h is all wrong also and needs 1U everywhere. Thanks Mike for the review. I admit to blindly stealing this from another port in glibc (I don't recall which). Since I still haven't managed to get any HWCAPs into mainline kernel then I was just going to wait until I had a HWCAP to include before pushing ahead with this. Should I update this and get the framework committed while waiting for things to trickle in to the kernel? Thanks, Matthew
"Carlos O'Donell" <carlos@redhat.com> writes: > On 03/06/2015 10:52 AM, Andreas Schwab wrote: >> Mike Frysinger <vapier@gentoo.org> writes: >> >>>> +{ >>>> + int i; >>>> + >>>> + /* Fallback to unknown output mechanism. */ >>>> + if (type == AT_HWCAP2) >>>> + return -1; >>>> + >>>> + _dl_printf ("AT_HWCAP: "); >>>> + >>>> + for (i = 0; i < _DL_HWCAP_COUNT; ++i) >>>> + if (word & (1 << i)) >>> >>> i is an int, but word is an unsigned long int. >> >> That's not a problem. The problem is that 1 is an int. > > Right, you want 1U. unsigned int is still a problem. Andreas.
Now that the initial HWCAPs are in Linux 4.3, will you be resubmitting HWCAP support for MIPS?
diff --git a/sysdeps/mips/bits/hwcap.h b/sysdeps/mips/bits/hwcap.h new file mode 100644 index 0000000..0013717 --- /dev/null +++ b/sysdeps/mips/bits/hwcap.h @@ -0,0 +1,24 @@ +/* Defines for bits in AT_HWCAP. + Copyright (C) 2014 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 !defined(_SYS_AUXV_H) && !defined(_SYSDEPS_SYSDEP_H) +# error "Never include <bits/hwcap.h> directly; use <sys/auxv.h> instead." +#endif + +#define HWCAP_MIPS_R6 0x00000001 +#define HWCAP_MIPS_MSA 0x00000002 diff --git a/sysdeps/mips/dl-procinfo.c b/sysdeps/mips/dl-procinfo.c index 4a3dbf3..3e7b7ed 100644 --- a/sysdeps/mips/dl-procinfo.c +++ b/sysdeps/mips/dl-procinfo.c @@ -59,5 +59,21 @@ PROCINFO_CLASS const char _dl_mips_platforms[4][11] , #endif +#if !defined PROCINFO_DECL && defined SHARED + ._dl_mips_cap_flags +#else +PROCINFO_CLASS const char _dl_mips_cap_flags[2][4] +#endif +#ifndef PROCINFO_DECL += { + "r6", "msa" + } +#endif +#if !defined SHARED || defined PROCINFO_DECL +; +#else +, +#endif + #undef PROCINFO_DECL #undef PROCINFO_CLASS diff --git a/sysdeps/mips/dl-procinfo.h b/sysdeps/mips/dl-procinfo.h index b2b7702..3aa236a 100644 --- a/sysdeps/mips/dl-procinfo.h +++ b/sysdeps/mips/dl-procinfo.h @@ -50,18 +50,50 @@ _dl_string_platform (const char *str) return -1; }; -/* We cannot provide a general printing function. */ -#define _dl_procinfo(type, word) -1 +#define _DL_HWCAP_COUNT 2 -/* There are no hardware capabilities defined. */ -#define _dl_hwcap_string(idx) "" +#define HWCAP_IMPORTANT (HWCAP_MIPS_MSA) -/* By default there is no important hardware capability. */ -#define HWCAP_IMPORTANT (0) +static inline int +__attribute__ ((unused)) +_dl_procinfo (unsigned int type, unsigned long int word) +{ + int i; + + /* Fallback to unknown output mechanism. */ + if (type == AT_HWCAP2) + return -1; + + _dl_printf ("AT_HWCAP: "); + + for (i = 0; i < _DL_HWCAP_COUNT; ++i) + if (word & (1 << i)) + _dl_printf (" %s", GLRO(dl_mips_cap_flags)[i]); + + _dl_printf ("\n"); + + return 0; +} + +static inline const char * +__attribute__ ((unused)) +_dl_hwcap_string (int idx) +{ + return GLRO(dl_mips_cap_flags)[idx]; +}; -/* We don't have any hardware capabilities. */ -#define _DL_HWCAP_COUNT 0 +static inline int +__attribute__ ((unused)) +_dl_string_hwcap (const char *str) +{ + int i; -#define _dl_string_hwcap(str) (-1) + for (i = 0; i < _DL_HWCAP_COUNT; i++) + { + if (strcmp (str, GLRO(dl_mips_cap_flags)[i]) == 0) + return i; + } + return -1; +}; #endif /* dl-procinfo.h */