On Fri, 3 Apr 2009, Gerd Hoffmann wrote:
> Linus Torvalds wrote:
> >
> > How married are people to this crazy 2x "32 bits" model?
>
> The tricky corner case are 32bit compat syscalls on 64bit kernel, where
> a noticable number of archs (IIRC at least ppc, mips and s390) can't
> pass unaligned 64bit values through the syscall abi. The 32/32 split
> was done because of that.
I really think that the 32-bit interfaces are fine. So I'm literally
suggesting a patch something like the appended.
Some notes:
- I changed the order to "low first". Why? Because when we have it like
this, the 64-bit system calls now don't use the "pos_high" argument at
all, and it makes more sense for the native system call to simply match
the user-mode prototype.
But I don't much _care_ about this particular change. I just made it
because it seems more logical to me, and if we're changing the order
anyway..
- I checked the resulting assembly code. At least on x86-64, gcc is
indeed smart enough to convert the "shift by half twice" into just a
zero, and I get the following code:
testq %rcx, %rcx # pos_l
js .L122 #,
movq %rcx, -48(%rbp) # pos_l, pos
which is from the C source
loff_t pos = pos_from_hilo(pos_h, pos_l);
...
if (pos < 0)
return -EINVAL;
and the 'pos_h' register isn't even touched. So this does indeed
improve code generation.
- It _used_ to generate code like
mov %r8d, %r8d # pos_low, pos_low
salq $32, %rcx #, tmp71
movq %r8, %rax # pos_low, pos.386
orq %rcx, %rax # tmp71, pos.386
js .L122 #,
movq %rax, -48(%rbp) # pos.386, pos
which isn't _that_ horrible, but I just think the new thing is more
sensible. Does it really matter? Probably not.
(In both code sequences, the actual code had other instructions
interspersed, so the above code-sequences are just me manually picking out
the relevant instructions. Maybe I did something wrong).
So I'm not going to commit the suggested patch, but I just personally
think it makes more sense that way.
Note: in all cases the user code wrapper can again be the same. You can
just do
#define HALF_BITS (sizeof(unsigned long)*4)
__syscall(PWRITEV, fd, iov, count, offset, (offset >> HALF_BITS) >>
HALF_BITS);
or something like that (and passing in a zero for the last argument is a
good idea, even if nobody will ever care - this will be the _least_ of our
problems if we ever do 128-bit loff_t). So the user call wrapper isn't
really any more complicated - the HALF_BITS thing may look more
complicated than just doing a hard-coded '16' there twice, but if you
understand what it's doing it's actually more logical.
I think.
Comments?
Linus
---
fs/compat.c | 4 ++--
fs/read_write.c | 14 ++++++++++----
include/linux/compat.h | 4 ++--
include/linux/syscalls.h | 4 ++--
4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/fs/compat.c b/fs/compat.c
index 1c859da..3f84d5f 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1236,7 +1236,7 @@ compat_sys_readv(unsigned long fd, const struct
compat_iovec __user *vec,
asmlinkage ssize_t
compat_sys_preadv(unsigned long fd, const struct compat_iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low)
+ unsigned long vlen, u32 pos_low, u32 pos_high)
{
loff_t pos = ((loff_t)pos_high << 32) | pos_low;
struct file *file;
@@ -1293,7 +1293,7 @@ compat_sys_writev(unsigned long fd, const struct
compat_iovec __user *vec,
asmlinkage ssize_t
compat_sys_pwritev(unsigned long fd, const struct compat_iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low)
+ unsigned long vlen, u32 pos_low, u32 pos_high)
{
loff_t pos = ((loff_t)pos_high << 32) | pos_low;
struct file *file;
diff --git a/fs/read_write.c b/fs/read_write.c
index 6d5d8ff..9ac8b6f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -731,10 +731,16 @@ SYSCALL_DEFINE3(writev, unsigned long, fd, const struct
iovec __user *, vec,
return ret;
}
+static inline loff_t pos_from_hilo(unsigned long high, unsigned long low)
+{
+#define HALF_LONG_BITS (BITS_PER_LONG / 2)
+ return ((high << HALF_LONG_BITS) << HALF_LONG_BITS) | low;
+}
+
SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
- unsigned long, vlen, u32, pos_high, u32, pos_low)
+ unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
{
- loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+ loff_t pos = pos_from_hilo(pos_h, pos_l);
struct file *file;
ssize_t ret = -EBADF;
int fput_needed;
@@ -757,9 +763,9 @@ SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct
iovec __user *, vec,
}
SYSCALL_DEFINE5(pwritev, unsigned long, fd, const struct iovec __user *, vec,
- unsigned long, vlen, u32, pos_high, u32, pos_low)
+ unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
{
- loff_t pos = ((loff_t)pos_high << 32) | pos_low;
+ loff_t pos = pos_from_hilo(pos_h, pos_l);
struct file *file;
ssize_t ret = -EBADF;
int fput_needed;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 9723edd..f2ded21 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -193,10 +193,10 @@ asmlinkage ssize_t compat_sys_writev(unsigned long fd,
const struct compat_iovec __user *vec, unsigned long vlen);
asmlinkage ssize_t compat_sys_preadv(unsigned long fd,
const struct compat_iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low);
+ unsigned long vlen, u32 pos_low, u32 pos_high);
asmlinkage ssize_t compat_sys_pwritev(unsigned long fd,
const struct compat_iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low);
+ unsigned long vlen, u32 pos_low, u32 pos_high);
int compat_do_execve(char * filename, compat_uptr_t __user *argv,
compat_uptr_t __user *envp, struct pt_regs * regs);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b299a82..18771ca 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -462,9 +462,9 @@ asmlinkage long sys_pread64(unsigned int fd, char __user
*buf,
asmlinkage long sys_pwrite64(unsigned int fd, const char __user *buf,
size_t count, loff_t pos);
asmlinkage long sys_preadv(unsigned long fd, const struct iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low);
+ unsigned long vlen, unsigned long pos_l, unsigned
long pos_h);
asmlinkage long sys_pwritev(unsigned long fd, const struct iovec __user *vec,
- unsigned long vlen, u32 pos_high, u32 pos_low);
+ unsigned long vlen, unsigned long pos_l, unsigned
long pos_h);
asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
asmlinkage long sys_mkdir(const char __user *pathname, int mode);
asmlinkage long sys_chdir(const char __user *filename);
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
