From: "Christopher M. Riedl" <cmr@codefail•de>
To: "Christophe Leroy" <christophe.leroy@csgroup•eu>,
<linuxppc-dev@lists•ozlabs.org>
Subject: Re: [PATCH v3 4/8] powerpc/signal64: Remove TM ifdefery in middle of if/else block
Date: Sun, 17 Jan 2021 11:16:12 -0600 [thread overview]
Message-ID: <C8LLQ8KX9GGS.1V196MK9ZRRSE@geist> (raw)
In-Reply-To: <80d49969-880f-5161-c829-cf319f94351d@csgroup.eu>
On Mon Jan 11, 2021 at 7:29 AM CST, Christophe Leroy wrote:
>
>
> Le 09/01/2021 à 04:25, Christopher M. Riedl a écrit :
> > Rework the messy ifdef breaking up the if-else for TM similar to
> > commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").
> >
> > Unlike that commit for ppc32, the ifdef can't be removed entirely since
> > uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail•de>
> > ---
> > arch/powerpc/kernel/signal_64.c | 17 +++++++----------
> > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index b211a8ea4f6e..dd3787f67a78 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > struct pt_regs *regs = current_pt_regs();
> > struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
> > sigset_t set;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > unsigned long msr;
> > -#endif
> >
> > /* Always make any pending restarted system calls return -EINTR */
> > current->restart_block.fn = do_no_restart_syscall;
> > @@ -762,10 +760,12 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > * restore_tm_sigcontexts.
> > */
> > regs->msr &= ~MSR_TS_MASK;
> > +#endif
> >
> > if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> > goto badframe;
>
> This means you are doing that __get_user() even when msr is not used.
> That should be avoided.
>
Thanks, I moved it into the #ifdef block right above it instead for the
next spin.
> > if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* We recheckpoint on return. */
> > struct ucontext __user *uc_transact;
> >
> > @@ -778,9 +778,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> > &uc_transact->uc_mcontext))
> > goto badframe;
> > - } else
> > #endif
> > - {
> > + } else {
> > /*
> > * Fall through, for non-TM restore
> > *
> > @@ -818,10 +817,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > unsigned long newsp = 0;
> > long err = 0;
> > struct pt_regs *regs = tsk->thread.regs;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* Save the thread's msr before get_tm_stackpointer() changes it */
> > - unsigned long msr = regs->msr;
> > -#endif
> > + unsigned long msr __maybe_unused = regs->msr;
>
> I don't thing __maybe_unused() is the right solution.
>
> I think MSR_TM_ACTIVE() should be fixed instead, either by changing it
> into a static inline
> function, or doing something similar to
> https://github.com/linuxppc/linux/commit/05a4ab823983d9136a460b7b5e0d49ee709a6f86
>
Agreed, I'll change MSR_TM_ACTIVE() to reference its argument in the
macro. This keeps it consistent with all the other MSR_TM_* macros in
reg.h. Probably better than changing it to static inline since that
would mean changing all the macros too which seems unecessary.
> >
> > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
> > if (!access_ok(frame, sizeof(*frame)))
> > @@ -836,8 +833,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > /* Create the ucontext. */
> > err |= __put_user(0, &frame->uc.uc_flags);
> > err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +
> > if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > /* The ucontext_t passed to userland points to the second
> > * ucontext_t (for transactional state) with its uc_link ptr.
> > */
> > @@ -847,9 +845,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > tsk, ksig->sig, NULL,
> > (unsigned long)ksig->ka.sa.sa_handler,
> > msr);
> > - } else
> > #endif
> > - {
> > + } else {
> > err |= __put_user(0, &frame->uc.uc_link);
> > prepare_setup_sigcontext(tsk, 1);
> > err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> >
>
> Christophe
next prev parent reply other threads:[~2021-01-17 17:16 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-09 3:25 [PATCH v3 0/8] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 1/8] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
2021-01-11 13:22 ` Christophe Leroy
2021-01-17 17:19 ` Christopher M. Riedl
2021-01-19 2:11 ` Michael Ellerman
2021-01-19 12:33 ` Christophe Leroy
2021-01-19 17:02 ` Christopher M. Riedl
2021-01-19 17:27 ` Christophe Leroy
2021-01-20 5:08 ` Christopher M. Riedl
2021-02-09 14:09 ` Christophe Leroy
2021-01-19 7:29 ` Christophe Leroy
2021-01-09 3:25 ` [PATCH v3 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user() Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 3/8] powerpc/signal64: Move non-inline functions out of setup_sigcontext() Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 4/8] powerpc/signal64: Remove TM ifdefery in middle of if/else block Christopher M. Riedl
2021-01-11 13:29 ` Christophe Leroy
2021-01-17 17:16 ` Christopher M. Riedl [this message]
2021-01-09 3:25 ` [PATCH v3 5/8] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 6/8] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches Christopher M. Riedl
2021-01-09 3:25 ` [PATCH v3 8/8] powerpc/signal64: Rewrite rt_sigreturn() " Christopher M. Riedl
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=C8LLQ8KX9GGS.1V196MK9ZRRSE@geist \
--to=cmr@codefail$(echo .)de \
--cc=christophe.leroy@csgroup$(echo .)eu \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox