Re: [PATCH 5/5] ARM64: Add support for ILP32 ABI.

[Andrew Pinski]

On Wed, Sep 11, 2013 at 7:32 AM, Catalin Marinas
<catalin.marinas@xxxxxxx> 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.

Ok. I will do so after my vacation next week.

>
> On top of these, I would really like to see
> Documentation/arm64/ilp32.txt describing the ABI.

No other target does not, not even x86_64 for x32.

>
> 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?).

We did test LTP on an earlier version of this patch for all three
ABIs, I will make sure that the next version I send out is tested on
all three ABIs also.  We also tested ILP32/LP64 on big-endian at the
same time which we will continue to do (I should push for our team
here to push out the big-endian patches).

>
>> 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.

I copied this from the MIPS64 n32 description in Kconfig.  But I will
make it more explicit.

>
>> 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))).

I will check on 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).

This is not useful and as you shown can be very messy and even worse
when it comes taking into account big and little-endian.  Even x32
does not do that.

>
> 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.

I had first thought about this and even started to implement it but I
found the glibc and the kernel messier than it was already.

>
> 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.

And I am not a fan of changing the generic UAPI files just so it is no
longer generic like you are doing.

>
> 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 <asm-generic/siginfo.h>
>
>  #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 <linux/compiler.h>
>  #include <linux/types.h>
>
> +#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 *.
>
>
> So, I'm looking for feedback on this proposal.


As I mentioned before even x32 does not do that and it is very messy
to make sure things get zero'd on the glibc and kernel sides.

>
>> 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 <uapi/asm/stat.h>
>>
>> -#ifdef CONFIG_ARM64_AARCH32
>> -
>> +#ifdef CONFIG_COMPAT
>>  #include <asm/compat.h>
>> +#endif
>
> Doesn't asm/compat.h have guards already? Do you get a conflict with
> is_compat_task()?

The guard was there already, I am just changing it back to be under
CONFIG_COMPAT from when I added CONFIG_ARM64_AARCH32. When I submit
the next version of the set of patches, this change will be done
correctly in the patch which adds CONFIG_ARM64_AARCH32.

>
>> 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.

See above why I think this is a bad thing and even worse since even
x32 did not do that already; it was the last added ABI like ILP32 to
the kernel.

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

No again this is messy due to the zeroing of the values.  As I
mentioned, I started to implement that but it got messy and there was
no right way of having the headers be sane.

>
>> --- /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 <apinski@xxxxxxxxxx>
>> + *
>> + * 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 <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +/* 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:

No using __SYSCALL_COMPAT is the correct thing to do and then only
reverting back what is needed.

The main reason is due to using the generic support inside glibc already.

>
> #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

You even forgot compat_sys_openat (where O_LARGEFILE differences does
make a difference).

>
>> 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?

I started out that way but "make clean ARCH=arm64" did not clean the
vdso files all the time.

>
>> 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 <asm/sections.h>
>>  #include <asm/setup.h>
>>  #include <asm/sizes.h>
>> +#include <asm/compat.h>
>>  #include <asm/tlb.h>
>
> Same issue, other header files doesn't include what is necessary.

I will double check these but I think it was a fall out due to my
change that I had originally did for asm/stat.h .

Thanks,
Andrew Pinski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/