Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751253AbaDOVnG (ORCPT ); Tue, 15 Apr 2014 17:43:06 -0400 Received: from mail-ve0-f182.google.com ([209.85.128.182]:61324 "EHLO mail-ve0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751028AbaDOVnD (ORCPT ); Tue, 15 Apr 2014 17:43:03 -0400 MIME-Version: 1.0 In-Reply-To: <20130911143102.GA8825@darko.cambridge.arm.com> References: <1378762380-13152-1-git-send-email-apinski@cavium.com> <1378762380-13152-5-git-send-email-apinski@cavium.com> <20130911143102.GA8825@darko.cambridge.arm.com> Date: Tue, 15 Apr 2014 14:43:01 -0700 Message-ID: Subject: Re: [PATCH 5/5] ARM64: Add support for ILP32 ABI. From: Andrew Pinski To: Catalin Marinas Cc: "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Will Deacon , Andrew Pinski , Arnd Bergmann Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 11, 2013 at 7:32 AM, Catalin Marinas wrote: > On Mon, Sep 09, 2013 at 10:32:59PM +0100, Andrew Pinski wrote: >> This patch adds full support of the ABI to the ARM64 target. > > This description is too short. Please describe what the ABI is, what are > the commonalities with AArch64 and AArch32, what other non-obvious > things had to be done (like __kernel_long_t being long long). Split this > patch into multiple patches like base syscall handling, signal handling, > pselect6/ppoll, vdso etc. It's too much to review at once. I will try to do this. > > On top of these, I would really like to see > Documentation/arm64/ilp32.txt describing the ABI. Ok, I will add some documentation. > > I would also like to know (you can state this in the cover letter) the > level of testing for all 3 types of ABI. I'm worried that at least this > patch breaks the current compat ABI (has LTP reported anything?). When I submit the next version, I will cover this in my cover letter. > >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index cc64df5..7fdc994 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -248,7 +248,7 @@ source "fs/Kconfig.binfmt" >> >> config COMPAT >> def_bool y >> - depends on ARM64_AARCH32 >> + depends on ARM64_AARCH32 || ARM64_ILP32 >> select COMPAT_BINFMT_ELF >> >> config ARM64_AARCH32 >> @@ -263,7 +263,14 @@ config ARM64_AARCH32 >> the user helper functions, VFP support and the ptrace interface are >> handled appropriately by the kernel. >> >> - If you want to execute 32-bit userspace applications, say Y. >> + If you want to execute Aarch32 userspace applications, say Y. >> + >> +config ARM64_ILP32 >> + bool "Kernel support for ILP32" >> + help >> + This option enables support for AArch64 ILP32 user space. These are >> + 64-bit binaries using 32-bit quantities for addressing and certain >> + data that would normally be 64-bit. > > You could be even more precise by mentioning that longs and pointers are > 32-bit, together with the derived types. Will fix this as mentioned before. > >> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h >> index 5ab2676..91bcf13 100644 >> --- a/arch/arm64/include/asm/compat.h >> +++ b/arch/arm64/include/asm/compat.h >> @@ -62,6 +62,8 @@ typedef u32 compat_ulong_t; >> typedef u64 compat_u64; >> typedef u32 compat_uptr_t; >> >> +typedef s64 ilp32_clock_t; >> + >> struct compat_timespec { >> compat_time_t tv_sec; >> s32 tv_nsec; >> @@ -180,6 +182,15 @@ typedef struct compat_siginfo { >> compat_clock_t _stime; >> } _sigchld; >> >> + /* SIGCHLD (ILP32 version) */ >> + struct { >> + compat_pid_t _pid; /* which child */ >> + __compat_uid32_t _uid; /* sender's uid */ >> + int _status; /* exit code */ >> + ilp32_clock_t _utime; >> + ilp32_clock_t _stime; >> + } _sigchld_ilp32; > > This *breaks* the compat_siginfo alignment. ilp32_clock_t is 64-bit > which forces the _sigchld_ilp32 to be 64-bit which makes the preamble 16 > instead of 12 bytes. This ilp32_clock_t needs > __attribute__((aligned(4))). Will fix this. > > The other approach I've been looking at is just using the native siginfo > instead of the compat one for ILP32. But this requires wider debate > (cc'ed Arnd if he has time). I will try to do this though it will take a week to get how much I can do with siginfo as the glibc side can be a pain to work with. The kernel is cleaner here since it is always 64bit. > > Basically if you use the current siginfo in the ILP32 context with > __kernel_clock_t being 64-bit you end up with a structure that doesn't > match any of the native or compat siginfo. This is because we have some > pointers which will turn into 32-bit values in ILP32: > > void __user *sival_ptr; /* accessed via si_ptr */ > void __user *_addr; /* accessed via si_addr */ > void __user *_call_addr; /* accessed via si_call_addr */ > > We also have __ARCH_SI_BAND_T defined as long. This has to stay long as POSIX defines it as a long type as mentioned in the header file. I will use an anonymous union/struct to get the correct definition for ilp32. I have to also fix the ptrace interface after I do this. > > AFAICT, Linux only does a put_user() on these and never reads them from > user space. This means that we can add the right padding on either side > of these pointers (for endianness reasons) and Linux would write 0 as > the top part of a 64-bit pointer (since the user address is restricted > to 32-bit anyway). User ILP32 would only access the corresponding > pointer as a 32-bit value and ignore the padding. > > It's easier to explain with some code (untested): > > diff --git a/arch/arm64/include/uapi/asm/siginfo.h b/arch/arm64/include/uapi/asm/siginfo.h > index 5a74a08..e3848be 100644 > --- a/arch/arm64/include/uapi/asm/siginfo.h > +++ b/arch/arm64/include/uapi/asm/siginfo.h > @@ -18,6 +18,13 @@ > > #define __ARCH_SI_PREAMBLE_SIZE (4 * sizeof(int)) > > +#ifndef __LP64__ /* ILP32 */ > +#define __ARCH_SI_BAND_T long long > +#define VOID_USER_PTR(x) \ > + void __user *x __attribute__((aligned(8))); \ > + char _pad[4] > +#endif > + > #include > > #endif > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index ba5be7f..9c50257 100644 > --- a/include/uapi/asm-generic/siginfo.h > +++ b/include/uapi/asm-generic/siginfo.h > @@ -4,9 +4,13 @@ > #include > #include > > +#ifndef VOID_USER_PTR > +#define VOID_USER_PTR(x) void __user *x > +#endif > + > typedef union sigval { > int sival_int; > - void __user *sival_ptr; > + VOID_USER_PTR(sival_ptr); > } sigval_t; > > /* > @@ -86,7 +90,7 @@ typedef struct siginfo { > > /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */ > struct { > - void __user *_addr; /* faulting insn/memory ref. */ > + VOID_USER_PTR(_addr); /* faulting insn/memory ref. */ > #ifdef __ARCH_SI_TRAPNO > int _trapno; /* TRAP # which caused the signal */ > #endif > @@ -101,7 +105,7 @@ typedef struct siginfo { > > /* SIGSYS */ > struct { > - void __user *_call_addr; /* calling user insn */ > + VOID_USER_PTR(_call_addr); /* calling user insn */ > int _syscall; /* triggering system call number */ > unsigned int _arch; /* AUDIT_ARCH_* of syscall */ > } _sigsys; > > > __LP64__ is always defined for AArch64 (kernel and native applications). > ILP32 user would not get this symbol and compat use a separate > compat_siginfo anyway. > > I'm not entirely sure about defining __ARCH_SI_BAND_T to long long but > as it also seems to be just written by the kernel, we can use some > padding as for the void __user *. POSIX defines it as long as mentioned above. I don't think we want ILP32 to violate POSIX here if we can not help it. > > > So, I'm looking for feedback on this proposal. > >> diff --git a/arch/arm64/include/asm/stat.h b/arch/arm64/include/asm/stat.h >> index 989128a..f2e0d3c 100644 >> --- a/arch/arm64/include/asm/stat.h >> +++ b/arch/arm64/include/asm/stat.h >> @@ -18,9 +18,11 @@ >> >> #include >> >> -#ifdef CONFIG_ARM64_AARCH32 >> - >> +#ifdef CONFIG_COMPAT >> #include >> +#endif > > Doesn't asm/compat.h have guards already? Do you get a conflict with > is_compat_task()? This was due to the order of the patches were applied and I did not notice the breakage until after I finished the last one. I will fix this so it is correctly done before I submit the next version of the patches. > >> diff --git a/arch/arm64/include/uapi/asm/siginfo.h b/arch/arm64/include/uapi/asm/siginfo.h >> index 5a74a08..297fb4f 100644 >> --- a/arch/arm64/include/uapi/asm/siginfo.h >> +++ b/arch/arm64/include/uapi/asm/siginfo.h >> @@ -16,7 +16,13 @@ >> #ifndef __ASM_SIGINFO_H >> #define __ASM_SIGINFO_H >> >> +#ifdef __LP64__ >> #define __ARCH_SI_PREAMBLE_SIZE (4 * sizeof(int)) >> +#else /* ILP32 */ >> +typedef long long __kernel_si_clock_t __attribute__((aligned(4))); >> +#define __ARCH_SI_CLOCK_T __kernel_si_clock_t >> +#define __ARCH_SI_ATTRIBUTES __attribute__((aligned(8))) >> +#endif > > This could go away if we manage to use the native siginfo. Yes I agree. > >> --- /dev/null >> +++ b/arch/arm64/kernel/signalilp32.c >> @@ -0,0 +1,30 @@ >> + >> +#ifdef CONFIG_ARM64_ILP32 >> + >> +#define rt_sigframe rt_sigframe_ilp32 > > Can we have the same native rt_sigframe (if we go for the native > siginfo)? ucontext is an arm64 structure, so we can add padding for > pointers and long. This one is a little harder at least in glibc itself. I tried a few times but I always failed due to not having that much experience with the code there but I will try one more time. > >> --- /dev/null >> +++ b/arch/arm64/kernel/sys_ilp32.c >> @@ -0,0 +1,274 @@ >> +/* >> + * AArch64- ILP32 specific system calls implementation >> + * >> + * Copyright (C) 2013 Cavium Inc. >> + * Author: Andrew Pinski >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program 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 General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see . >> + */ >> + >> +/* Adjust unistd.h to provide 32-bit numbers and functions. */ >> +#define __SYSCALL_COMPAT > > No. We need to use as many native syscalls as possible and only define > those absolutely necessary. In my investigation, I only ended up needing > these: > > #define sys_ioctl compat_sys_ioctl > #define sys_readv compat_sys_readv > #define sys_writev compat_sys_writev > #define sys_preadv compat_sys_preadv64 > #define sys_pwritev compat_sys_pwritev64 > #define sys_vmsplice compat_sys_vmsplice > #define sys_waitid compat_sys_waitid > #define sys_set_robust_list compat_sys_set_robust_list > #define sys_get_robust_list compat_sys_get_robust_list > #define sys_kexec_load compat_sys_kexec_load > #define sys_timer_create compat_sys_timer_create > #define sys_ptrace compat_sys_ptrace > #define sys_sigaltstack compat_sys_sigaltstack > #define sys_rt_sigaction compat_sys_rt_sigaction > #define sys_rt_sigpending compat_sys_rt_sigpending > #define sys_rt_sigtimedwait compat_sys_rt_sigtimedwait > #define sys_rt_sigqueueinfo compat_sys_rt_sigqueueinfo > #define sys_rt_sigreturn compat_sys_rt_sigreturn_wrapper > #define sys_mq_notify compat_sys_mq_notify > #define sys_recvfrom compat_sys_recvfrom > #define sys_setsockopt compat_sys_setsockopt > #define sys_getsockopt compat_sys_getsockopt > #define sys_sendmsg compat_sys_sendmsg > #define sys_recvmsg compat_sys_recvmsg > #define sys_execve compat_sys_execve > #define sys_move_pages compat_sys_move_pages > #define sys_rt_tgsigqueueinfo compat_sys_rt_tgsigqueueinfo > #define sys_recvmmsg compat_sys_recvmmsg > #define sys_sendmmsg compat_sys_sendmmsg > #define sys_process_vm_readv compat_sys_process_vm_readv > #define sys_process_vm_writev compat_sys_process_vm_writev Here is some additional ones I can up with after understanding this code better:\ /* Pointer in struct */ #define sys_mount compat_sys_mount /* NUMA */ /* unsigned long bitmaps */ #define sys_get_mempolicy compat_sys_get_mempolicy #define sys_set_mempolicy compat_sys_set_mempolicy #define sys_mbind compat_sys_mbind /* array of pointers */ /* unsigned long bitmaps */ #define sys_migrate_pages compat_sys_migrate_pages /* Scheduler */ /* unsigned long bitmaps */ #define sys_sched_setaffinity compat_sys_sched_setaffinity #define sys_sched_getaffinity compat_sys_sched_getaffinity /* iov usage */ #define sys_keyctl compat_sys_keyctl /* aio */ /* Pointer to Pointer */ #define sys_io_setup compat_sys_io_setup /* Array of pointers */ #define sys_io_submit compat_sys_io_submit --- CUT ---- I added a couple of comments on why the ones listed above are needed. Also I don't think ilp32 needs its own sys_ptrace rather than using the compat one. > >> diff --git a/arch/arm64/kernel/vdsoilp32/Makefile b/arch/arm64/kernel/vdsoilp32/Makefile >> new file mode 100644 >> index 0000000..ec93f3f >> --- /dev/null >> +++ b/arch/arm64/kernel/vdsoilp32/Makefile > > Could we not keep vdso in the same directory? > >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 67e8d7c..17b9c39 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -35,6 +35,7 @@ >> #include >> #include >> #include >> +#include >> #include > > Same issue, other header files doesn't include what is necessary. > > -- > Catalin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/