[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e0e373fa13636a403322fd0ba96915fd25dbbefa.camel@physik.fu-berlin.de>
Date: Mon, 24 Jun 2024 08:14:36 +0200
From: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To: Arnd Bergmann <arnd@...db.de>, Arnd Bergmann <arnd@...nel.org>,
Linux-Arch <linux-arch@...r.kernel.org>, linux-kernel@...r.kernel.org
Cc: Rich Felker <dalias@...c.org>, Andreas Larsson <andreas@...sler.com>,
guoren <guoren@...nel.org>, Christophe Leroy <christophe.leroy@...roup.eu>,
"H. Peter Anvin" <hpa@...or.com>, sparclinux@...r.kernel.org,
linux-s390@...r.kernel.org, Helge Deller <deller@....de>,
linux-sh@...r.kernel.org, "linux-csky@...r.kernel.org"
<linux-csky@...r.kernel.org>, "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>, "musl@...ts.openwall.com"
<musl@...ts.openwall.com>, Nicholas Piggin <npiggin@...il.com>, Alexander
Viro <viro@...iv.linux.org.uk>, LTP List <ltp@...ts.linux.it>, Brian Cain
<bcain@...cinc.com>, Christian Brauner <brauner@...nel.org>, Thomas
Bogendoerfer <tsbogend@...ha.franken.de>, Xi Ruoyao
<libc-alpha@...rceware.org>, linux-parisc@...r.kernel.org,
linux-mips@...r.kernel.org, stable@...r.kernel.org,
linux-hexagon@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH 09/15] sh: rework sync_file_range ABI
Hi Arnd,
On Fri, 2024-06-21 at 11:41 +0200, Arnd Bergmann wrote:
> On Fri, Jun 21, 2024, at 10:44, John Paul Adrian Glaubitz wrote:
> > On Thu, 2024-06-20 at 18:23 +0200, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@...db.de>
> > >
> > > The unusual function calling conventions on superh ended up causing
> > ^^^^^^
> > It's spelled SuperH
>
> Fixed now.
>
> > > diff --git a/arch/sh/kernel/sys_sh32.c b/arch/sh/kernel/sys_sh32.c
> > > index 9dca568509a5..d5a4f7c697d8 100644
> > > --- a/arch/sh/kernel/sys_sh32.c
> > > +++ b/arch/sh/kernel/sys_sh32.c
> > > @@ -59,3 +59,14 @@ asmlinkage int sys_fadvise64_64_wrapper(int fd, u32 offset0, u32 offset1,
> > > (u64)len0 << 32 | len1, advice);
> > > #endif
> > > }
> > > +
> > > +/*
> > > + * swap the arguments the way that libc wants it instead of
> >
> > I think "swap the arguments to the order that libc wants them" would
> > be easier to understand here.
>
> Done
Thanks for the two improvements!
> > > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> > > index bbf83a2db986..c55fd7696d40 100644
> > > --- a/arch/sh/kernel/syscalls/syscall.tbl
> > > +++ b/arch/sh/kernel/syscalls/syscall.tbl
> > > @@ -321,7 +321,7 @@
> > > 311 common set_robust_list sys_set_robust_list
> > > 312 common get_robust_list sys_get_robust_list
> > > 313 common splice sys_splice
> > > -314 common sync_file_range sys_sync_file_range
> > > +314 common sync_file_range sys_sh_sync_file_range6
> > ^^^^^^
> > Why the suffix 6 here?
>
> In a later part of my cleanup, I'm consolidating all the
> copies of this function (arm64, mips, parisc, powerpc,
> s390, sh, sparc, x86) and picked the name
> sys_sync_file_range6() for common implementation.
>
> I end up with four entry points here, so the naming is a bit
> confusing:
>
> - sys_sync_file_range() is only used on 64-bit architectures,
> on x32 and on mips-n32. This uses four arguments, including
> two 64-bit wide ones.
>
> - sys_sync_file_range2() continues to be used on arm, powerpc,
> xtensa and now on sh, hexagon and csky. I change the
> implementation to take six 32-bit arguments, but the ABI
> remains the same as before, with the flags before offset.
>
> - sys_sync_file_range6() is used for most other 32-bit ABIs:
> arc, m68k, microblaze, nios2, openrisc, parisc, s390, sh, sparc
> and x86. This also has six 32-bit arguments but in the
> default order (fd, offset, nbytes, flags).
>
> - sys_sync_file_range7() is exclusive to mips-o32, this one
> has an unused argument and is otherwise the same as
> sys_sync_file_range6().
>
> My plan is to then have some infrastructure to ensure
> userspace tools (libc, strace, qemu, rust, ...) use the
> same calling conventions as the kernel. I'm doing the
> same thing for all other syscalls that have architecture
> specific calling conventions, so far I'm using
>
> fadvise64_64_7
> fanotify_mark6
> truncate3
> truncate4
> ftruncate3
> ftruncate4
> fallocate6
> pread5
> pread6
> pwrite5
> pwrite6
> preadv5
> preadv6
> pwritev5
> pwritev6
> sync_file_range6
> fadvise64_64_2
> fadvise64_64_6
> fadvise64_5
> fadvise64_6
> readahead4
> readahead5
>
> The last number here is usually the number of 32-bit
> arguments, except for fadvise64_64_2 that uses the
> same argument reordering trick as sync_file_range2.
>
> I'm not too happy with the naming but couldn't come up with
> anything clearer either, so let me know if you have any
> ideas there.
OK, gotcha. I thought the 6 suffix was for SH only. I'm fine
with the naming scheme.
> > > 315 common tee sys_tee
> > > 316 common vmsplice sys_vmsplice
> > > 317 common move_pages sys_move_pages
> > > @@ -395,6 +395,7 @@
> > > 385 common pkey_alloc sys_pkey_alloc
> > > 386 common pkey_free sys_pkey_free
> > > 387 common rseq sys_rseq
> > > +388 common sync_file_range2 sys_sync_file_range2
> > > # room for arch specific syscalls
> > > 393 common semget sys_semget
> > > 394 common semctl sys_semctl
> >
> > I wonder how you discovered this bug. Did you look up the calling
> > convention on SuperH
> > and compare the argument order for the sys_sync_file_range system call
> > documented there
> > with the order in the kernel?
>
> I had to categorize all architectures based on their calling
> conventions to see if 64-bit arguments need aligned pairs or
> not, so I wrote a set of simple C files that I compiled for
> all architectures to see in which cases they insert unused
> arguments or swap the order of the upper and lower halves.
>
> SuperH, parisc and s390 are each slightly different from all the
> others here, so I ended up reading the ELF psABI docs and/or
> the compiler sources to be sure.
> I also a lot of git history.
Great job, thanks for doing the extra work to verify the ABI.
> > Did you also check what order libc uses? I would expect libc on SuperH
> > misordering the
> > arguments as well unless I am missing something. Or do we know that the
> > code is actually
> > currently broken?
>
> Yes, I checked glibc, musl and uclibc-ng for all the cases in
> which the ABI made no sense, as well as to check that my analysis
> of the kernel sources matches the expectations of the libc.
OK, awesome.
Will you send a v2 so I can ack the updated version of the patch?
I'm also fine with the patch going through your tree, as I would
like to start with the changes for v6.11 this week.
Thanks,
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
