From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id AB79C21E93 for ; Mon, 24 Jun 2024 15:15:48 +0200 (CEST) Received: (qmail 30008 invoked by uid 550); 24 Jun 2024 13:15:43 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 32103 invoked from network); 24 Jun 2024 06:14:56 -0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=fu-berlin.de; s=fub01; h=MIME-Version:Content-Transfer-Encoding: Content-Type:References:In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=ew1xoLiUEpaRO/Rd2+BU3bX5mn9fF1xUB9iaO1b3Vho=; t=1719209698; x=1719814498; b=PfBTZtn5MrYimAnY7VO0qnnJovS2qBwpGZJPBOPKxfOO+U5lDmBE/PlDMI49qmUArBPqpv8tsHf 43GVx5UAJOOeenQ0YlWRpQOXhGwlSq1VOWeP9EOfx1t2wptrgZnJn+ZmgVLrJdpz6b6kysZEZySNg Qoa8Nrd99539+saSR2rIb3rZ134xS5RzOhx886nCS5Jg+8htWsstbCpdFJSDa+hIjygks4XPdADqb gYnIjx0/vniKISpwnQV4VRcwhZ5/CgZbyEwoSeCkXzijMKhbc61rQnBsTPTFXINQj0xdViFE2t8R7 Z6IKZ/fKHL4H3c60rO0zecCkFTGYSav1hRcA==; Message-ID: From: John Paul Adrian Glaubitz To: Arnd Bergmann , Arnd Bergmann , Linux-Arch , linux-kernel@vger.kernel.org Cc: Rich Felker , Andreas Larsson , guoren , Christophe Leroy , "H. Peter Anvin" , sparclinux@vger.kernel.org, linux-s390@vger.kernel.org, Helge Deller , linux-sh@vger.kernel.org, "linux-csky@vger.kernel.org" , "Naveen N. Rao" , Heiko Carstens , "musl@lists.openwall.com" , Nicholas Piggin , Alexander Viro , LTP List , Brian Cain , Christian Brauner , Thomas Bogendoerfer , Xi Ruoyao , linux-parisc@vger.kernel.org, linux-mips@vger.kernel.org, stable@vger.kernel.org, linux-hexagon@vger.kernel.org, linux-fsdevel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, "David S . Miller" Date: Mon, 24 Jun 2024 08:14:36 +0200 In-Reply-To: <9d4ba5e5-bb7f-432e-9354-47cc84eaa9e1@app.fastmail.com> References: <20240620162316.3674955-1-arnd@kernel.org> <20240620162316.3674955-10-arnd@kernel.org> <366548c1a0d9749e42c0d0c993414a353c9b0b02.camel@physik.fu-berlin.de> <9d4ba5e5-bb7f-432e-9354-47cc84eaa9e1@app.fastmail.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.52.2 MIME-Version: 1.0 X-Original-Sender: glaubitz@physik.fu-berlin.de X-Originating-IP: 77.191.15.86 X-ZEDAT-Hint: PO Subject: [musl] 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 > > >=20 > > > The unusual function calling conventions on superh ended up causing > > ^^^^^^ > > It's spelled SuperH >=20 > Fixed now. >=20 > > > 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, u3= 2 offset0, u32 offset1, > > > (u64)len0 << 32 | len1, advice); > > > #endif > > > } > > > + > > > +/* > > > + * swap the arguments the way that libc wants it instead of > >=20 > > I think "swap the arguments to the order that libc wants them" would > > be easier to understand here. >=20 > Done Thanks for the two improvements! > > > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/sys= calls/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 > > ^^^^^^= =20 > > Why the suffix 6 here? >=20 > 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. >=20 > I end up with four entry points here, so the naming is a bit > confusing: >=20 > - 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. >=20 > - 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. >=20 > - 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). >=20 > - 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(). >=20 > 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 >=20 > 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 >=20 > 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. >=20 > 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 > >=20 > > I wonder how you discovered this bug. Did you look up the calling=20 > > convention on SuperH > > and compare the argument order for the sys_sync_file_range system call= =20 > > documented there > > with the order in the kernel? >=20 > 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. >=20 > 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= =20 > > misordering the > > arguments as well unless I am missing something. Or do we know that the= =20 > > code is actually > > currently broken? >=20 > 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 --=20 .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913