Message ID | 87r28hpb0l.fsf_-_@oldenburg2.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | Remove ioperm etc. support for arm (was: Re: _ioperm support for Arm) | expand |
Hi, On Wed, May 29, 2019 at 04:53:46PM +0200, Florian Weimer wrote: > For libc.so.6 on arm-linux-gnueabi (as built by build-many-glibcs.py), > readelf shows this: > > Attribute Section: aeabi > File Attributes > Tag_CPU_name: "5T" > Tag_CPU_arch: v5T Doesn't this depend on what is the default in the toolchain (GCC) being used? Looking at this script it enforces arch for some variants, but not for plain arm-linux-gnueabi. A.
* Aaro Koskinen: > Hi, > > On Wed, May 29, 2019 at 04:53:46PM +0200, Florian Weimer wrote: >> For libc.so.6 on arm-linux-gnueabi (as built by build-many-glibcs.py), >> readelf shows this: >> >> Attribute Section: aeabi >> File Attributes >> Tag_CPU_name: "5T" >> Tag_CPU_arch: v5T > > Doesn't this depend on what is the default in the toolchain (GCC) > being used? Yes, I think so. > Looking at this script it enforces arch for some variants, but not for > plain arm-linux-gnueabi. Has GCC ever changed default here? Maybe we used to test ARMv4, and GCC changed under us without us noticing? Thanks, Florian
Hi, On Wed, May 29, 2019 at 08:46:43PM +0200, Florian Weimer wrote: > > On Wed, May 29, 2019 at 04:53:46PM +0200, Florian Weimer wrote: > >> For libc.so.6 on arm-linux-gnueabi (as built by build-many-glibcs.py), > >> readelf shows this: > >> > >> Attribute Section: aeabi > >> File Attributes > >> Tag_CPU_name: "5T" > >> Tag_CPU_arch: v5T > > > > Doesn't this depend on what is the default in the toolchain (GCC) > > being used? > > Yes, I think so. > > > Looking at this script it enforces arch for some variants, but not for > > plain arm-linux-gnueabi. > > Has GCC ever changed default here? Interesting, it seems that v5t is indeed what GCC defaults to. I thought it would have been the lowest one currently supported (v4t). > Maybe we used to test ARMv4, and GCC changed under us without us > noticing? Don't know if they have ever changed that, but it would probably make sense to add flags to build-many-glibcs.py to build for the lowest supported arch level. ARMv4 is already deprecated in GCC, so probably it's OK to remove this stuff from glibc. Personally I'm concerned about armv4t support as I'm running such systems still with modern glibc. A.
On Wed, May 29, 2019 at 04:53:46PM +0200, Florian Weimer wrote: > So it looks like to me like a later architectue version. Yes, right. build-many-glibcs.py seems to explicitly call out ARMv5TE and later, and it also builds at least one library with gcc's default architecture settings (which you get to choose at the point you build the compiler, and I think the default defaults have also changed at various points in the past). But there's no explicit selection of anything older than ARMv5TE so I think we should assume it isn't getting tested. > I would welcome comments on the commit message. The patch itself has > been “tested” with build-many-glibcs.py on all 32-bit Arm architectures. The commit message looks fine to me, or at least I couldn't think of anything better to write. > void > -_outb (unsigned char b, unsigned long int port) > +outb (unsigned char b, unsigned long int port) Does this cause symbols that were previously weak to become strong? If so, is that ok? It would be a bit unfortunate if the act of deprecating these functions caused them to suddenly start interposing something else, or do symbol versions prevent that from happening? > unsigned int > -_inl (unsigned long int port) > +inl (unsigned long int port) > { > - return *((volatile unsigned long *)(IO_ADDR (port))); > + return 0; > } The outcome of calling inl() etc without a previous successful call to ioperm() would have been SIGSEGV, so it would be ok to replace this with *((volatile unsigned long *)0). But what you have is fine too. p.
* Phil Blundell: >> void >> -_outb (unsigned char b, unsigned long int port) >> +outb (unsigned char b, unsigned long int port) > > Does this cause symbols that were previously weak to become strong? Yes, it does. > If so, is that ok? It would be a bit unfortunate if the act of > deprecating these functions caused them to suddenly start interposing > something else, or do symbol versions prevent that from happening? The SHLIB_COMPAT check returns false for static builds, so nothing ends up in libc.a after this change. The weak/strong alias difference is not supposed to matter for dynamic linking. (glibc used to be buggy, and there is the LD_DYNAMIC_WEAK override to get the old behavior, but I haven't heard of anyone using it.) It's also hard to get the dynamic linker to search beyond libc.so.6 in a different DSO because libc.so.6 obviously does not have a DT_NEEDED dependency on some application-provided DSO. >> unsigned int >> -_inl (unsigned long int port) >> +inl (unsigned long int port) >> { >> - return *((volatile unsigned long *)(IO_ADDR (port))); >> + return 0; >> } > > The outcome of calling inl() etc without a previous successful call to > ioperm() would have been SIGSEGV, so it would be ok to replace this > with *((volatile unsigned long *)0). But what you have is fine too. Thanks. Florian
* Aaro Koskinen: >> Maybe we used to test ARMv4, and GCC changed under us without us >> noticing? > > Don't know if they have ever changed that, but it would probably make > sense to add flags to build-many-glibcs.py to build for the lowest > supported arch level. Indeed. > ARMv4 is already deprecated in GCC, so probably it's OK to remove this > stuff from glibc. Personally I'm concerned about armv4t support as I'm > running such systems still with modern glibc. Interesting. Could you perhaps suggest changes to build-many-glibcs.py so that we can build an armv4t target as part of the regular runs? (And I assume you aren't interested in ioperm support anymore. 8-) Thanks, Florian
On Wed, May 29, 2019 at 11:10:32PM +0200, Florian Weimer wrote: > The SHLIB_COMPAT check returns false for static builds, so nothing ends > up in libc.a after this change. > > The weak/strong alias difference is not supposed to matter for dynamic > linking. Right, makes sense. In that case, I think this patch is OK. Thanks Phil
On Wed, 29 May 2019, Phil Blundell wrote: > On Wed, May 29, 2019 at 04:53:46PM +0200, Florian Weimer wrote: > > So it looks like to me like a later architectue version. > > Yes, right. build-many-glibcs.py seems to explicitly call out ARMv5TE and > later, and it also builds at least one library with gcc's default > architecture settings (which you get to choose at the point you build > the compiler, and I think the default defaults have also changed at > various points in the past). But there's no explicit selection of > anything older than ARMv5TE so I think we should assume it isn't getting > tested. The --with-cpu=arm926ej-s is essentially "default architecture but with FPU", since you can't build for hard float now (GCC 8 and later) unless you specify an architecture or CPU that imply the presence of an FPU, or specify an explicit FPU selection.
Hi, On Wed, May 29, 2019 at 11:12:44PM +0200, Florian Weimer wrote: > >> Maybe we used to test ARMv4, and GCC changed under us without us > >> noticing? > > > > Don't know if they have ever changed that, but it would probably make > > sense to add flags to build-many-glibcs.py to build for the lowest > > supported arch level. > > Indeed. > > > ARMv4 is already deprecated in GCC, so probably it's OK to remove this > > stuff from glibc. Personally I'm concerned about armv4t support as I'm > > running such systems still with modern glibc. > > Interesting. Could you perhaps suggest changes to build-many-glibcs.py > so that we can build an armv4t target as part of the regular runs? Just building with -march=armv4t should be enough for a start. A.
* Aaro Koskinen: > Hi, > > On Wed, May 29, 2019 at 11:12:44PM +0200, Florian Weimer wrote: >> >> Maybe we used to test ARMv4, and GCC changed under us without us >> >> noticing? >> > >> > Don't know if they have ever changed that, but it would probably make >> > sense to add flags to build-many-glibcs.py to build for the lowest >> > supported arch level. >> >> Indeed. >> >> > ARMv4 is already deprecated in GCC, so probably it's OK to remove this >> > stuff from glibc. Personally I'm concerned about armv4t support as I'm >> > running such systems still with modern glibc. >> >> Interesting. Could you perhaps suggest changes to build-many-glibcs.py >> so that we can build an armv4t target as part of the regular runs? > > Just building with -march=armv4t should be enough for a start. This architecture doesn't have its own target triplet, right? With the patch below, I get this in csu/init-first.os: 00000000 <__libc_init_first>: 0: e12fff1e bx lr 0: R_ARM_V4BX *ABS* But after linking, get this in libc.so.6: 000175a0 <__libc_init_first>: 175a0: e12fff1e bx lr Does this mean I need to pass some other flags as well? Sorry, I'm really not familiar with Arm at all. Thanks, Florian build-many-glibcs.py: Add v4t variant for arm-linux-gnueabi 2019-06-26 Florian Weimer <fweimer@redhat.com> * scripts/build-many-glibcs.py (Context.add_all_configs): Add v4t variant for arm-linux-gnueabi. diff --git a/scripts/build-many-glibcs.py b/scripts/build-many-glibcs.py index c5821df25e..dacd116f8e 100755 --- a/scripts/build-many-glibcs.py +++ b/scripts/build-many-glibcs.py @@ -158,7 +158,9 @@ class Context(object): self.add_config(arch='alpha', os_name='linux-gnu') self.add_config(arch='arm', - os_name='linux-gnueabi') + os_name='linux-gnueabi', + extra_glibcs=[{'variant': 'v4t', + 'ccopts': '-march=armv4t'}]) self.add_config(arch='armeb', os_name='linux-gnueabi') self.add_config(arch='armeb',
On Wed, Jun 26, 2019 at 03:48:08PM +0200, Florian Weimer wrote: > * Aaro Koskinen: > > Just building with -march=armv4t should be enough for a start. > > This architecture doesn't have its own target triplet, right? Right. You can configure with "armv4t-unknown-linuxgnueabi" if you like but I don't think the glibc configury does anything different for "armv4t" than for plain "arm". > With the patch below, I get this in csu/init-first.os: > > 00000000 <__libc_init_first>: > 0: e12fff1e bx lr > 0: R_ARM_V4BX *ABS* > > But after linking, get this in libc.so.6: > > 000175a0 <__libc_init_first>: > 175a0: e12fff1e bx lr > > Does this mean I need to pass some other flags as well? No, that looks about right. ARMv4T does have "bx rN" so it is OK for that instruction to be generated. You might be thinking of ARMv4, which doesn't have bx at all and needs the linker to patch them (gcc passes --fix-v4bx from its specs when building for ARMv4) or the "blx" instruction which is only in ARMv5T and upwards. p.
* Phil Blundell: > On Wed, Jun 26, 2019 at 03:48:08PM +0200, Florian Weimer wrote: >> * Aaro Koskinen: >> > Just building with -march=armv4t should be enough for a start. >> >> This architecture doesn't have its own target triplet, right? > > Right. You can configure with "armv4t-unknown-linuxgnueabi" if > you like but I don't think the glibc configury does anything > different for "armv4t" than for plain "arm". > >> With the patch below, I get this in csu/init-first.os: >> >> 00000000 <__libc_init_first>: >> 0: e12fff1e bx lr >> 0: R_ARM_V4BX *ABS* >> >> But after linking, get this in libc.so.6: >> >> 000175a0 <__libc_init_first>: >> 175a0: e12fff1e bx lr >> >> Does this mean I need to pass some other flags as well? > > No, that looks about right. ARMv4T does have "bx rN" so it > is OK for that instruction to be generated. You might be > thinking of ARMv4, which doesn't have bx at all and needs > the linker to patch them (gcc passes --fix-v4bx from its > specs when building for ARMv4) or the "blx" instruction > which is only in ARMv5T and upwards. Okay, so the patch is essentially complete? I don't know if the code generation and relocation differences make this a viable additional architecture build-only testing. Do you have an opinion regarding this matter? Thanks, Florian
On Wed, Jun 26, 2019 at 05:23:32PM +0200, Florian Weimer wrote: > Okay, so the patch is essentially complete? I think so. I will give it a try later and see if I can confirm that it's doing the right thing, but assuming that the -march= setting is indeed ending up on the compiler command line then I think the answer is that the patch is correct. > I don't know if the code generation and relocation differences make this > a viable additional architecture build-only testing. Do you have an > opinion regarding this matter? Yes, I think it's a useful test. The assembler knows what instructions are allowed in each architecture, so this build test is sufficient to confirm (for example) that no blx instructions have leaked into the generic ARM assembly sources without being suitably protected. p.
* Phil Blundell: > On Wed, Jun 26, 2019 at 05:23:32PM +0200, Florian Weimer wrote: >> Okay, so the patch is essentially complete? > > I think so. I will give it a try later and see if I can confirm > that it's doing the right thing, but assuming that the -march= > setting is indeed ending up on the compiler command line then I > think the answer is that the patch is correct. Thanks, that would be useful information to have. If it all works out, I will push my patch. Florian
On Wed, Jun 26, 2019 at 05:56:15PM +0200, Florian Weimer wrote: > * Phil Blundell: > > I think so. I will give it a try later and see if I can confirm > > that it's doing the right thing, but assuming that the -march= > > setting is indeed ending up on the compiler command line then I > > think the answer is that the patch is correct. > > Thanks, that would be useful information to have. If it all works out, > I will push my patch. I (finally!) inspected the output from build-many-glibcs.py with your patch and it looks correct for ARMv4T. So I think you can go ahead and push that. Sorry it took me so long. Thanks Phil
* Phil Blundell: > On Wed, Jun 26, 2019 at 05:56:15PM +0200, Florian Weimer wrote: >> * Phil Blundell: >> > I think so. I will give it a try later and see if I can confirm >> > that it's doing the right thing, but assuming that the -march= >> > setting is indeed ending up on the compiler command line then I >> > think the answer is that the patch is correct. >> >> Thanks, that would be useful information to have. If it all works out, >> I will push my patch. > > I (finally!) inspected the output from build-many-glibcs.py with your > patch and it looks correct for ARMv4T. So I think you can go ahead > and push that. Sorry it took me so long. Thanks, I've pushed my patch. Florian
diff --git a/NEWS b/NEWS index c885b960ca..31ead24851 100644 --- a/NEWS +++ b/NEWS @@ -58,6 +58,9 @@ Deprecated and removed features, and other changes affecting compatibility: configurations) has been removed, following the deprecation of this subarchitecture in version 8 of GCC, and its removal in version 9. +* On 32-bit Arm, support for the port-based I/O emulation and the <sys/io.h> + header have been removed. + Changes to build and runtime requirements: * GCC 6.2 or later is required to build the GNU C Library. diff --git a/sysdeps/unix/sysv/linux/arm/Makefile b/sysdeps/unix/sysv/linux/arm/Makefile index 4adc35de04..d7a2f6a8a7 100644 --- a/sysdeps/unix/sysv/linux/arm/Makefile +++ b/sysdeps/unix/sysv/linux/arm/Makefile @@ -5,7 +5,7 @@ endif ifeq ($(subdir),misc) sysdep_routines += ioperm -sysdep_headers += sys/elf.h sys/io.h +sysdep_headers += sys/elf.h endif ifeq ($(subdir),signal) diff --git a/sysdeps/unix/sysv/linux/arm/ioperm.c b/sysdeps/unix/sysv/linux/arm/ioperm.c index 0e338c4504..e832a6605e 100644 --- a/sysdeps/unix/sysv/linux/arm/ioperm.c +++ b/sysdeps/unix/sysv/linux/arm/ioperm.c @@ -17,167 +17,71 @@ License along with the GNU C Library. If not, see <http://www.gnu.org/licenses/>. */ -/* I/O port access on the ARM is something of a fiction. What we do is to - map an appropriate area of /dev/mem into user space so that a program - can blast away at the hardware in such a way as to generate I/O cycles - on the bus. To insulate user code from dependencies on particular - hardware we don't allow calls to inb() and friends to be inlined, but - force them to come through code in here every time. Performance-critical - registers tend to be memory mapped these days so this should be no big - problem. */ +#include <shlib-compat.h> -/* Once upon a time this file used mprotect to enable and disable - access to particular areas of I/O space. Unfortunately the - mprotect syscall also has the side effect of enabling caching for - the area affected (this is a kernel limitation). So we now just - enable all the ports all of the time. */ +#if SHLIB_COMPAT (libc, GLIBC_2_4, GLIBC_2_30) +# include <errno.h> -#include <errno.h> -#include <fcntl.h> -#include <stdio.h> -#include <ctype.h> -#include <stdlib.h> -#include <unistd.h> - -#include <sys/types.h> -#include <sys/mman.h> - -#include <sys/sysctl.h> - -#define MAX_PORT 0x10000 - -static struct { - unsigned long int base; - unsigned long int io_base; - unsigned int shift; - unsigned int initdone; /* since all the above could be 0 */ -} io; - -#define IO_ADDR(port) (io.base + ((port) << io.shift)) - -/* - * Initialize I/O system. The io_bae and port_shift values are fetched - * using sysctl (CTL_BUS, CTL_BUS_ISA, ISA_*). - */ - -static int -init_iosys (void) +int +ioperm (unsigned long int from, unsigned long int num, int turn_on) { - static int iobase_name[] = { CTL_BUS, CTL_BUS_ISA, BUS_ISA_PORT_BASE }; - static int ioshift_name[] = { CTL_BUS, CTL_BUS_ISA, BUS_ISA_PORT_SHIFT }; - size_t len = sizeof (io.base); - - if (! __sysctl (iobase_name, 3, &io.io_base, &len, NULL, 0) - && ! __sysctl (ioshift_name, 3, &io.shift, &len, NULL, 0)) - { - io.initdone = 1; - return 0; - } - - /* sysctl has failed... */ - __set_errno (ENODEV); + __set_errno (ENOSYS); return -1; } +compat_symbol (libc, ioperm, ioperm, GLIBC_2_4); int -_ioperm (unsigned long int from, unsigned long int num, int turn_on) +iopl (unsigned int level) { - if (! io.initdone && init_iosys () < 0) - return -1; - - /* this test isn't as silly as it may look like; consider overflows! */ - if (from >= MAX_PORT || from + num > MAX_PORT) - { - __set_errno (EINVAL); - return -1; - } - - if (turn_on) - { - if (! io.base) - { - int fd; - - fd = __open ("/dev/mem", O_RDWR); - if (fd < 0) - return -1; - - io.base = - (unsigned long int) __mmap (0, MAX_PORT << io.shift, - PROT_READ | PROT_WRITE, - MAP_SHARED, fd, io.io_base); - __close (fd); - if ((long) io.base == -1) - return -1; - } - } - - return 0; + __set_errno (ENOSYS); + return -1; } +compat_symbol (libc, iopl, iopl, GLIBC_2_4); -int -_iopl (unsigned int level) -{ - if (level > 3) - { - __set_errno (EINVAL); - return -1; - } - if (level) - { - return _ioperm (0, MAX_PORT, 1); - } - return 0; -} - +/* The remaining functions do not have any way to indicate failure. + However, it is only valid to call them after calling ioperm/iopl, + which will have indicated failure. */ void -_outb (unsigned char b, unsigned long int port) +outb (unsigned char b, unsigned long int port) { - *((volatile unsigned char *)(IO_ADDR (port))) = b; } - +compat_symbol (libc, outb, outb, GLIBC_2_4); void -_outw (unsigned short b, unsigned long int port) +outw (unsigned short b, unsigned long int port) { - *((volatile unsigned short *)(IO_ADDR (port))) = b; } - +compat_symbol (libc, outw, outw, GLIBC_2_4); void -_outl (unsigned int b, unsigned long int port) +outl (unsigned int b, unsigned long int port) { - *((volatile unsigned long *)(IO_ADDR (port))) = b; } - +compat_symbol (libc, outl, outl, GLIBC_2_4); unsigned int -_inb (unsigned long int port) +inb (unsigned long int port) { - return *((volatile unsigned char *)(IO_ADDR (port))); + return 0; } +compat_symbol (libc, inb, inb, GLIBC_2_4); unsigned int -_inw (unsigned long int port) +inw (unsigned long int port) { - return *((volatile unsigned short *)(IO_ADDR (port))); + return 0; } +compat_symbol (libc, inw, inw, GLIBC_2_4); unsigned int -_inl (unsigned long int port) +inl (unsigned long int port) { - return *((volatile unsigned long *)(IO_ADDR (port))); + return 0; } +compat_symbol (libc, inl, inl, GLIBC_2_4); -weak_alias (_ioperm, ioperm); -weak_alias (_iopl, iopl); -weak_alias (_inb, inb); -weak_alias (_inw, inw); -weak_alias (_inl, inl); -weak_alias (_outb, outb); -weak_alias (_outw, outw); -weak_alias (_outl, outl); +#endif /* SHLIB_COMAT */ diff --git a/sysdeps/unix/sysv/linux/arm/sys/io.h b/sysdeps/unix/sysv/linux/arm/sys/io.h deleted file mode 100644 index 6f5ae5079a..0000000000 --- a/sysdeps/unix/sysv/linux/arm/sys/io.h +++ /dev/null @@ -1,47 +0,0 @@ -/* Copyright (C) 1996-2019 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/>. */ - -#ifndef _SYS_IO_H - -#define _SYS_IO_H 1 -#include <features.h> - -__BEGIN_DECLS - -/* If TURN_ON is TRUE, request for permission to do direct i/o on the - port numbers in the range [FROM,FROM+NUM-1]. Otherwise, turn I/O - permission off for that range. This call requires root privileges. */ -extern int ioperm (unsigned long int __from, unsigned long int __num, - int __turn_on) __THROW; - -/* Set the I/O privilege level to LEVEL. If LEVEL is nonzero, - permission to access any I/O port is granted. This call requires - root privileges. */ -extern int iopl (int __level) __THROW; - -/* The functions that actually perform reads and writes. */ -extern unsigned char inb (unsigned long int __port) __THROW; -extern unsigned short int inw (unsigned long int __port) __THROW; -extern unsigned long int inl (unsigned long int __port) __THROW; - -extern void outb (unsigned char __value, unsigned long int __port) __THROW; -extern void outw (unsigned short __value, unsigned long int __port) __THROW; -extern void outl (unsigned long __value, unsigned long int __port) __THROW; - -__END_DECLS - -#endif /* _SYS_IO_H */