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.


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


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

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 <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:
>
> #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 <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.
>
> --
> Catalin
--
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/