From: Matheus Castanho <msc@linux.ibm.com> To: Nicholas Piggin <npiggin@gmail.com> Cc: "Dmitry V. Levin" <ldv@altlinux.org>, Michael Ellerman <mpe@ellerman.id.au>, libc-alpha@sourceware.org, libc-dev@lists.llvm.org, linux-api@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, musl@lists.openwall.com Subject: Re: Linux powerpc new system call instruction and ABI Date: Fri, 21 May 2021 16:40:30 -0300 [thread overview] Message-ID: <87a6oo4312.fsf@linux.ibm.com> (raw) In-Reply-To: <1621400263.gf0mbqhkrf.astroid@bobo.none> Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm: >> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am: >>> Hi, >>> >>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >>> [...] >>>> - Error handling: The consensus among kernel, glibc, and musl is to move to >>>> using negative return values in r3 rather than CR0[SO]=1 to indicate error, >>>> which matches most other architectures, and is closer to a function call. >>> >>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was >>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and >>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used. >>> This includes syscall_get_error() and all its users including >>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable >>> when scv is used. >>> >>> See also https://bugzilla.redhat.com/1929836 >> >> I see, thanks. Using latest strace from github.com, the attached kernel >> patch makes strace -k check results a lot greener. >> >> Some of the remaining failing tests look like this (I didn't look at all >> of them yet): >> >> signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> ) = 50 >> signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef >> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41 >> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737 >> ) = 26 >> exit_group(1) = ? >> >> I think the problem is glibc testing for -ve, but it should be comparing >> against -4095 (+cc Matheus) >> >> #define RET_SCV \ >> cmpdi r3,0; \ >> bgelr+; \ >> neg r3,r3; > > This glibc patch at least gets that signal test working. Haven't run the > full suite yet because of trouble making it work with a local glibc > install... > > Thanks, > Nick > > --- > > diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h > index c57bb1c05d..1ea4c3b917 100644 > --- a/sysdeps/powerpc/powerpc64/sysdep.h > +++ b/sysdeps/powerpc/powerpc64/sysdep.h > @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ > #endif > > #define RET_SCV \ > - cmpdi r3,0; \ > - bgelr+; \ > + li r9,-4095; \ > + cmpld r3,r9; \ > + bltlr+; \ > neg r3,r3; > > #define RET_SC \ Hi Nick, I agree the current code is accepting more values as errors than it should. This change looks good to me. All glibc tests are passing with it. I also built strace and checked one of the failing tests against a glibc with your patch: ~/src/strace/tests$ uname -r 5.10.16-1-default ~/src/strace/tests$ /lib64/libc.so.6 GNU C Library (GNU libc) release release version 2.33 (git 9826b03b74). [...] ~/src/strace/tests$ ./signal.gen.test errno2name.c:461: unknown errno 559038737: Unknown error 559038737 signal.gen.test: failed test: ../signal failed with code 1 ~/src/strace/tests$ ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) errno2name.c:461: unknown errno 559038737: Unknown error 559038737 Running with glibc containing the patch: ~/src/strace/tests$ ~/build/glibc/testrun.sh ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef signal(-559038737, SIG_IGN) = -1 EINVAL (Invalid argument) +++ exited with 0 +++ If the patch below looks OK to you and no one objects, I'll commit it to glibc on Monday. Thanks, Matheus Castanho --- From: Nicholas Piggin <npiggin@gmail.com> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes When using scv on templated ASM syscalls, current code interprets any negative return value as error, but the only valid error codes are in the range -4095..-1 according to the ABI. Reviewed-by: Matheus Castanho <msc@linux.ibm.com> --- sysdeps/powerpc/powerpc64/sysdep.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h index c57bb1c05d..1ea4c3b917 100644 --- a/sysdeps/powerpc/powerpc64/sysdep.h +++ b/sysdeps/powerpc/powerpc64/sysdep.h @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ #endif #define RET_SCV \ - cmpdi r3,0; \ - bgelr+; \ + li r9,-4095; \ + cmpld r3,r9; \ + bltlr+; \ neg r3,r3; #define RET_SC \ -- 2.31.1
WARNING: multiple messages have this Message-ID (diff)
From: Matheus Castanho <msc@linux.ibm.com> To: Nicholas Piggin <npiggin@gmail.com> Cc: libc-alpha@sourceware.org, musl@lists.openwall.com, "Dmitry V. Levin" <ldv@altlinux.org>, linux-api@vger.kernel.org, libc-dev@lists.llvm.org, linuxppc-dev@lists.ozlabs.org Subject: Re: Linux powerpc new system call instruction and ABI Date: Fri, 21 May 2021 16:40:30 -0300 [thread overview] Message-ID: <87a6oo4312.fsf@linux.ibm.com> (raw) In-Reply-To: <1621400263.gf0mbqhkrf.astroid@bobo.none> Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm: >> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am: >>> Hi, >>> >>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote: >>> [...] >>>> - Error handling: The consensus among kernel, glibc, and musl is to move to >>>> using negative return values in r3 rather than CR0[SO]=1 to indicate error, >>>> which matches most other architectures, and is closer to a function call. >>> >>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was >>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and >>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used. >>> This includes syscall_get_error() and all its users including >>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable >>> when scv is used. >>> >>> See also https://bugzilla.redhat.com/1929836 >> >> I see, thanks. Using latest strace from github.com, the attached kernel >> patch makes strace -k check results a lot greener. >> >> Some of the remaining failing tests look like this (I didn't look at all >> of them yet): >> >> signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) >> ) = 50 >> signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef >> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41 >> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737 >> ) = 26 >> exit_group(1) = ? >> >> I think the problem is glibc testing for -ve, but it should be comparing >> against -4095 (+cc Matheus) >> >> #define RET_SCV \ >> cmpdi r3,0; \ >> bgelr+; \ >> neg r3,r3; > > This glibc patch at least gets that signal test working. Haven't run the > full suite yet because of trouble making it work with a local glibc > install... > > Thanks, > Nick > > --- > > diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h > index c57bb1c05d..1ea4c3b917 100644 > --- a/sysdeps/powerpc/powerpc64/sysdep.h > +++ b/sysdeps/powerpc/powerpc64/sysdep.h > @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ > #endif > > #define RET_SCV \ > - cmpdi r3,0; \ > - bgelr+; \ > + li r9,-4095; \ > + cmpld r3,r9; \ > + bltlr+; \ > neg r3,r3; > > #define RET_SC \ Hi Nick, I agree the current code is accepting more values as errors than it should. This change looks good to me. All glibc tests are passing with it. I also built strace and checked one of the failing tests against a glibc with your patch: ~/src/strace/tests$ uname -r 5.10.16-1-default ~/src/strace/tests$ /lib64/libc.so.6 GNU C Library (GNU libc) release release version 2.33 (git 9826b03b74). [...] ~/src/strace/tests$ ./signal.gen.test errno2name.c:461: unknown errno 559038737: Unknown error 559038737 signal.gen.test: failed test: ../signal failed with code 1 ~/src/strace/tests$ ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) errno2name.c:461: unknown errno 559038737: Unknown error 559038737 Running with glibc containing the patch: ~/src/strace/tests$ ~/build/glibc/testrun.sh ./signal signal(SIGUSR1, SIG_IGN) = 0 (SIG_DFL) signal(SIGUSR1, SIG_DFL) = 0x1 (SIG_IGN) signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL) signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef signal(-559038737, SIG_IGN) = -1 EINVAL (Invalid argument) +++ exited with 0 +++ If the patch below looks OK to you and no one objects, I'll commit it to glibc on Monday. Thanks, Matheus Castanho --- From: Nicholas Piggin <npiggin@gmail.com> Subject: [PATCH 1/1] powerpc: Fix handling of scv return error codes When using scv on templated ASM syscalls, current code interprets any negative return value as error, but the only valid error codes are in the range -4095..-1 according to the ABI. Reviewed-by: Matheus Castanho <msc@linux.ibm.com> --- sysdeps/powerpc/powerpc64/sysdep.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h index c57bb1c05d..1ea4c3b917 100644 --- a/sysdeps/powerpc/powerpc64/sysdep.h +++ b/sysdeps/powerpc/powerpc64/sysdep.h @@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \ #endif #define RET_SCV \ - cmpdi r3,0; \ - bgelr+; \ + li r9,-4095; \ + cmpld r3,r9; \ + bltlr+; \ neg r3,r3; #define RET_SC \ -- 2.31.1
next prev parent reply other threads:[~2021-05-21 19:41 UTC|newest] Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-11 8:12 Linux powerpc new system call instruction and ABI Nicholas Piggin 2020-06-11 8:12 ` Nicholas Piggin 2020-06-11 8:12 ` [PATCH 1/2] powerpc/64s/exception: treat NIA below __end_interrupts as soft-masked Nicholas Piggin 2020-06-11 8:12 ` Nicholas Piggin 2020-07-24 13:25 ` Michael Ellerman 2020-07-24 13:25 ` Michael Ellerman 2020-06-11 8:12 ` [PATCH 2/2] powerpc/64s: system call support for scv/rfscv instructions Nicholas Piggin 2020-06-11 8:12 ` Nicholas Piggin 2020-07-23 6:47 ` Michael Ellerman 2020-07-23 16:48 ` Christophe Leroy 2020-07-23 16:48 ` Christophe Leroy 2020-07-24 10:45 ` Michael Ellerman 2020-07-24 10:45 ` Michael Ellerman 2020-06-11 21:02 ` Linux powerpc new system call instruction and ABI Segher Boessenkool 2020-06-11 21:02 ` Segher Boessenkool 2020-06-14 9:26 ` Nicholas Piggin 2020-06-14 9:26 ` Nicholas Piggin 2021-05-18 23:13 ` Dmitry V. Levin 2021-05-18 23:13 ` Dmitry V. Levin 2021-05-19 2:50 ` Nicholas Piggin 2021-05-19 2:50 ` Nicholas Piggin 2021-05-19 5:01 ` Nicholas Piggin 2021-05-19 5:01 ` Nicholas Piggin 2021-05-21 19:40 ` Matheus Castanho [this message] 2021-05-21 19:40 ` Matheus Castanho 2021-05-21 19:52 ` Florian Weimer 2021-05-21 19:52 ` Florian Weimer 2021-05-21 20:00 ` Matheus Castanho 2021-05-21 20:00 ` Matheus Castanho 2021-05-21 20:52 ` Dmitry V. Levin 2021-05-21 20:52 ` Dmitry V. Levin 2021-05-24 12:11 ` Matheus Castanho 2021-05-24 12:11 ` Matheus Castanho 2021-05-24 20:33 ` Matheus Castanho 2021-05-24 20:33 ` Matheus Castanho 2021-05-19 10:24 ` Dmitry V. Levin 2021-05-19 10:24 ` Dmitry V. Levin 2021-05-19 10:59 ` Nicholas Piggin 2021-05-19 10:59 ` Nicholas Piggin 2021-05-19 12:39 ` Tulio Magno Quites Machado Filho 2021-05-19 12:39 ` Tulio Magno Quites Machado Filho 2021-05-19 13:26 ` Dmitry V. Levin 2021-05-19 13:26 ` Dmitry V. Levin 2021-05-19 22:51 ` Nicholas Piggin 2021-05-19 22:51 ` Nicholas Piggin 2021-05-19 23:27 ` Dmitry V. Levin 2021-05-19 23:27 ` Dmitry V. Levin 2021-05-20 2:40 ` Nicholas Piggin 2021-05-20 2:40 ` Nicholas Piggin 2021-05-20 3:06 ` Dmitry V. Levin 2021-05-20 3:06 ` Dmitry V. Levin 2021-05-20 5:12 ` Nicholas Piggin 2021-05-20 5:12 ` Nicholas Piggin 2021-05-19 7:33 ` Joakim Tjernlund 2021-05-19 7:33 ` Joakim Tjernlund 2021-05-19 7:55 ` Nicholas Piggin 2021-05-19 7:55 ` Nicholas Piggin 2021-05-19 8:08 ` Joakim Tjernlund 2021-05-19 8:08 ` Joakim Tjernlund 2021-05-19 8:42 ` Nicholas Piggin 2021-05-19 8:42 ` Nicholas Piggin 2021-05-19 11:12 ` Nicholas Piggin 2021-05-19 11:12 ` Nicholas Piggin 2021-05-19 14:38 ` Segher Boessenkool 2021-05-19 14:38 ` Segher Boessenkool 2021-05-19 15:06 ` Joakim Tjernlund 2021-05-19 15:06 ` Joakim Tjernlund 2021-05-19 15:22 ` Segher Boessenkool 2021-05-19 15:22 ` Segher Boessenkool 2021-05-19 15:36 ` [musl] " Rich Felker 2021-05-19 15:36 ` Rich Felker 2021-05-19 18:09 ` Joakim Tjernlund 2021-05-19 18:09 ` Joakim Tjernlund 2021-05-19 23:48 ` [musl] " Rich Felker 2021-05-19 23:48 ` Rich Felker 2021-05-20 1:06 ` Dmitry V. Levin 2021-05-20 1:06 ` Dmitry V. Levin 2021-05-20 2:45 ` Nicholas Piggin 2021-05-20 2:45 ` Nicholas Piggin 2021-05-20 2:59 ` Dmitry V. Levin 2021-05-20 2:59 ` Dmitry V. Levin 2021-05-20 7:20 ` Nicholas Piggin 2021-05-20 7:20 ` Nicholas Piggin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87a6oo4312.fsf@linux.ibm.com \ --to=msc@linux.ibm.com \ --cc=ldv@altlinux.org \ --cc=libc-alpha@sourceware.org \ --cc=libc-dev@lists.llvm.org \ --cc=linux-api@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=musl@lists.openwall.com \ --cc=npiggin@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.