Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050613002:
Add s390/s390x support to linux_syscall_support.h. (Closed)
DescriptionAdd s390/s390x support to linux_syscall_support.h.
This CL adds support for s390 and s390x, based on glibc and kernel code.
A subset of this code has been tested with (and merged into) gperftools.
BUG=none
Patch Set 1 #
Total comments: 10
Patch Set 2 : Re-order s390/s390x syscalls, and clarify mmap/mmap2 APIs. #Patch Set 3 : Revert unintended deletion. No functional change. #
Total comments: 1
Patch Set 4 : Standardize on int64_t for the type of the offset argument to sys_mmap(), and internally use sys_mm… #
Total comments: 10
Patch Set 5 : Fix sys_fadvise64 and sys_fallocate; hide sys_mmap2. #Messages
Total messages: 15 (2 generated)
Description was changed from ========== Add s390/s390x support to linux_syscall_support.h. This CL adds support for s390 and s390x, based on glibc and kernel code. A subset of this code has been tested with (and merged into) gperftools. BUG=none ========== to ========== Add s390/s390x support to linux_syscall_support.h. This CL adds support for s390 and s390x, based on glibc and kernel code. A subset of this code has been tested with (and merged into) gperftools. BUG=none ==========
bryanpkc@gmail.com changed reviewers: + vapier@chromium.org
Hi Mike, I needed to switch to a different email address, and there is no way in Rietveld to change the ownership of issue 1679683002, so I have created a new issue. I have added support for 31-bit s390 (and verified it with some simple tests, plus gperftools), and also made the sys_mmap()/sys_mmap2() interfaces consistent with other platforms, as per your suggestion.
https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:1584: #ifndef __s390x__ this nest is super hard to follow. rewrite it like so: #elif defined(__s390__) ... common syscalls ... # ifdef __s390x__ ... s390x stuff # else ... s390 stuff ... # endif #endif don't worry about keeping the syscall # list in the same order as other arches https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3233: : "=r"(__res_r2) \ i think you want to use a "d" constraint here (and in all the ones below). "r" is a "general register" while "d" is a "general data register" for s390. not 100% sure there's a practical diff (and i don't really want to dive into the gcc md code to find out), but "d" is what glibc uses. https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3591: LSS_INLINE void* LSS_NAME(mmap2)(void *s, size_t l, int p, int f, int d, do you need mmap2 at all ? the args are all truncated to (unsigned long). in LSS, we really only want to expose mmap(). if the mmap2 syscall is "better", then internally we should use it, but the API for the caller should still just be mmap(). https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3866: LSS_INLINE _syscall1(void*, mmap, void*, a) would be nice to move your new generic func above here. i'm aware that'll change the API, but we want that change :).
https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:1584: #ifndef __s390x__ On 2016/06/08 17:46:05, vapier wrote: > this nest is super hard to follow. rewrite it like so: > > #elif defined(__s390__) > ... common syscalls ... > # ifdef __s390x__ > ... s390x stuff > # else > ... s390 stuff ... > # endif > #endif > > don't worry about keeping the syscall # list in the same order as other arches Done. https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3233: : "=r"(__res_r2) \ On 2016/06/08 17:46:05, vapier wrote: > i think you want to use a "d" constraint here (and in all the ones below). "r" > is a "general register" while "d" is a "general data register" for s390. not > 100% sure there's a practical diff (and i don't really want to dive into the gcc > md code to find out), but "d" is what glibc uses. Done. https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3591: LSS_INLINE void* LSS_NAME(mmap2)(void *s, size_t l, int p, int f, int d, On 2016/06/08 17:46:05, vapier wrote: > do you need mmap2 at all ? the args are all truncated to (unsigned long). I added support for mmap2 since it already existed for the other 32-bit architectures. The arguments to mmap2 are 32 bits wide; I believe glibc packs them in the same manner. We can make sys_mmap() use mmap2 internally, at the expense of making the offset argument always 64-bit (__off64_t instead of off_t), even on s390. That could potentially surprise some users. https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3866: LSS_INLINE _syscall1(void*, mmap, void*, a) On 2016/06/08 17:46:05, vapier wrote: > would be nice to move your new generic func above here. i'm aware that'll > change the API, but we want that change :). OK, I wasn't sure if I should touch the ARM code, but I will group all the mmap/mmap2 code together to make things clearer.
https://codereview.chromium.org/2050613002/diff/40001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/40001/linux_syscall_support.h... linux_syscall_support.h:4002: /* mmap() is obsolete on ARM EABI; implement with mmap2() instead. */ I can't test on ARM EABI but I think this is the desired API.
https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3591: LSS_INLINE void* LSS_NAME(mmap2)(void *s, size_t l, int p, int f, int d, i'm not sure why it'd be a problem ? mmap() requires offset be a multiple of _SC_PAGE_SIZE (which iirc is never <4096), while mmap2() implicitly shifts the offset by 4096 bytes. so using the mmap2() syscall in place of mmap() should always be transparent: if they passed a 32-bit quantity as the offset, it'd get automatically extended to 64-bit. then we'd never need a mmap2() helper, and mmap() would be the same signature for all targets. https://codereview.chromium.org/2050613002/diff/1/linux_syscall_support.h#new... linux_syscall_support.h:3866: LSS_INLINE _syscall1(void*, mmap, void*, a) if it's the right thing to do, don't worry about touching other arches :)
On 2016/06/08 21:03:29, vapier wrote: > i'm not sure why it'd be a problem ? mmap() requires offset be a multiple of > _SC_PAGE_SIZE (which iirc is never <4096), while mmap2() implicitly shifts the > offset by 4096 bytes. so using the mmap2() syscall in place of mmap() should > always be transparent: if they passed a 32-bit quantity as the offset, it'd get > automatically extended to 64-bit. I was just worried that it would break existing user of LSS's sys_mmap(). I think I understand what you are saying, and I have re-done the sys_mmap code. Please take a look. :-)
https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3591: int, fd, loff_t, offset, loff_t, len, int, advice) does this need to be s390-specific ? seems like this def works for all arches that have fadvise64. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3592: LSS_INLINE _syscall4(int, fallocate, why doesn't the common one work ? https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3852: LSS_INLINE void* LSS_NAME(mmap2)(void *s, size_t l, int p, int f, int d, we can just omit this entirely since mmap() does everything we need https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3996: unsigned int pgsz = __getpagesize(); mmap2 is defined as always being shifted by 4096 bytes, not by the current page size. so you can replace this with a 4096 constant. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:4022: /* aarch64 and MIPS64. */ i would say /* Remaining 64-bit arches. */
https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3591: int, fd, loff_t, offset, loff_t, len, int, advice) On 2016/06/11 06:51:20, vapier wrote: > does this need to be s390-specific ? seems like this def works for all arches > that have fadvise64. We can define this for all 64-bit arches that have __NR_advise64, but the implementations for 32-bit arches are incompatible with each other, and I can only provide the one for s390. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3592: LSS_INLINE _syscall4(int, fallocate, On 2016/06/11 06:51:19, vapier wrote: > why doesn't the common one work ? There was no common code for fallocate; the function was only defined for x86_64 and i386. We can define this for all 64-bit arches, and it seems that the i386 version will work for s390 too. I am not sure if there will be endianness issues with the other 31-bit arches. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3852: LSS_INLINE void* LSS_NAME(mmap2)(void *s, size_t l, int p, int f, int d, On 2016/06/11 06:51:20, vapier wrote: > we can just omit this entirely since mmap() does everything we need I think we need this to support 44-bit byte offsets on 31-bit s390 (see LSS_NAME(mmap) below). We can name this _mmap2 to mark it as internal. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:3996: unsigned int pgsz = __getpagesize(); On 2016/06/11 06:51:20, vapier wrote: > mmap2 is defined as always being shifted by 4096 bytes, not by the current page > size. so you can replace this with a 4096 constant. Done. https://codereview.chromium.org/2050613002/diff/60001/linux_syscall_support.h... linux_syscall_support.h:4022: /* aarch64 and MIPS64. */ On 2016/06/11 06:51:20, vapier wrote: > i would say /* Remaining 64-bit arches. */ Done.
there are endian issues with the splitting of the 64-bit fields, and also ABI alignment issues (with reg pairs). but i don't think they're new issues, and i won't make you fix those here as we've already gotten kind of far down the rabbit hole :). lgtm, thanks!
On 2016/06/14 03:31:58, vapier wrote: > lgtm, thanks! Thanks for your time reviewing and all the suggestions! Do I need do anything else? Will you commit the patch and close this issue?
i can push it. unfortunately, the Code Review system handles git poorly. what is your name so i can apply author info ?
On 2016/06/14 12:40:09, vapier wrote: > i can push it. unfortunately, the Code Review system handles git poorly. what > is your name so i can apply author info ? Thanks Mike. My name is Bryan Chan (bryanpkc at gmail dot com).
Message was sent while issue was closed.
https://chromium.googlesource.com/linux-syscall-support/+/3f6478ac95edf86cd3d... |
