Remove ioperm etc. support for arm (was: Re: _ioperm support for Arm)

Message ID 87r28hpb0l.fsf_-_@oldenburg2.str.redhat.com
State Committed
Headers

Commit Message

Florian Weimer May 29, 2019, 2:53 p.m. UTC
  * Phil Blundell:

> On Wed, May 29, 2019 at 04:03:26PM +0200, Florian Weimer wrote:
>> There's this bug:
>> 
>>   Remove ARM old-ABI support
>>   <https://sourceware.org/bugzilla/show_bug.cgi?id=13556>
>> 
>> I believe this removed core support for non-EABI platforms.
>
> That's not quite the same thing.  The old ABI differed from the EABI
> in several ways, including the stack alignment rules and the way
> that system calls were done.  I think the bug you mentioned above
> was to remove the support in glibc for those specific "old ABI"
> mechanisms.
>
> What you currently get if you build glibc for ARMv4 is an "almost-EABI"
> configuration which does everything in the same way as the EABI except
> that there is no interworking support.  I think we still have a certain
> amount of #ifdef __ARM_ARCH_4__ for that purpose.  If we wanted to clean
> things up there then removing support for the half-baked V4T interworking
> would probably be more of a win since I think that accounts for a larger
> amount of #ifdef scar tissue.
>
>> All the target triplets we test upstream have gnueabi in them, and these
>> days, anything that is not built by build-many-glibcs.py in at least
>> variant is not considered supported, I think.
>
> For the reasons above, even when building glibc for armv4 you still put
> "gnueabi" in the triplet.  But I think you're still correct that
> build-many-glibcs.py doesn't actually test anything older than ARMv5TE.

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
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-1
  Tag_ABI_PCS_wchar_t: 4
  Tag_ABI_FP_rounding: Needed
  Tag_ABI_FP_denormal: Needed
  Tag_ABI_FP_exceptions: Needed
  Tag_ABI_FP_user_exceptions: Needed
  Tag_ABI_FP_number_model: IEEE 754
  Tag_ABI_align_needed: 8-byte
  Tag_ABI_align_preserved: 8-byte, except leaf SP
  Tag_ABI_enum_size: int

So it looks like to me like a later architectue version.

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.

Thanks,
Florian

arm: Remove ioperm/iopl/inb/inw/inl/outb/outw/outl support

Linux only supports the required ISA sysctls on StrongARM devices,
which are armv4 and no longer tested during glibc development
and probably bit-rotted by this point.  (No reported test results,
and the last discussion of armv4 support was in the glibc 2.19
release notes.)

2019-05-29  Florian Weimer  <fweimer@redhat.com>

	arm: Remove ioperm/iopl/inb/inw/inl/outb/outw/outl support.
	* sysdeps/unix/sysv/linux/arm/Makefile
	[$(subdir) == misc] (sysdep_headers): Remove sys/io.h.
	* sysdeps/unix/sysv/linux/arm/sys/io.h: Remove file.
	* sysdeps/unix/sysv/linux/arm/ioperm.c: Rewrite file.
	(ioperm, iopl, inb, inw, inl, outb, outw, outl): Turn into
	compatibility symbols.
  

Comments

Aaro Koskinen May 29, 2019, 6:40 p.m. UTC | #1
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.
  
Florian Weimer May 29, 2019, 6:46 p.m. UTC | #2
* 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
  
Aaro Koskinen May 29, 2019, 8:12 p.m. UTC | #3
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.
  
Phil Blundell May 29, 2019, 8:56 p.m. UTC | #4
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.
  
Florian Weimer May 29, 2019, 9:10 p.m. UTC | #5
* 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
  
Florian Weimer May 29, 2019, 9:12 p.m. UTC | #6
* 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
  
Phil Blundell May 29, 2019, 9:15 p.m. UTC | #7
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
  
Joseph Myers May 30, 2019, 4:27 p.m. UTC | #8
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.
  
Aaro Koskinen June 6, 2019, 9:53 p.m. UTC | #9
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.
  

Patch

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 */