Message-Id: <C5ZIJ5D6XYSA.191D18GH311GD@geist>
Date:   Mon, 28 Sep 2020 21:55:17 -0500
From:   "Christopher M. Riedl" <cmr@...ormatik.wtf>
To:     "Christophe Leroy" <christophe.leroy@...roup.eu>,
        "Benjamin Herrenschmidt" <benh@...nel.crashing.org>,
        "Paul Mackerras" <paulus@...ba.org>,
        "Michael Ellerman" <mpe@...erman.id.au>
Cc:     <linuxppc-dev@...ts.ozlabs.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs()
 and save_tm_user_regs() in 'unsafe' version

On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
> Change those two functions to be used within a user access block.
>
> For that, change save_general_regs() to and unsafe_save_general_regs(),
> then replace all user accesses by unsafe_ versions.
>
> This series leads to a reduction from 2.55s to 1.73s of
> the system CPU time with the following microbench app
> on an mpc832x with KUAP (approx 32%)
>
> Without KUAP, the difference is in the noise.
>
> void sigusr1(int sig) { }
>
> int main(int argc, char **argv)
> {
> int i = 100000;
>
> signal(SIGUSR1, sigusr1);
> for (;i--;)
> raise(SIGUSR1);
> exit(0);
> }
>
> An additional 0.10s reduction is achieved by removing
> CONFIG_PPC_FPU, as the mpc832x has no FPU.
>
> A bit less spectacular on an 8xx as KUAP is less heavy, prior to
> the series (with KUAP) it ran in 8.10 ms. Once applies the removal
> of FPU regs handling, we get 7.05s. With the full series, we get 6.9s.
> If artificially re-activating FPU regs handling with the full series,
> we get 7.6s.
>
> So for the 8xx, the removal of the FPU regs copy is what makes the
> difference, but the rework of handle_signal also have a benefit.
>
> Same as above, without KUAP the difference is in the noise.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
> arch/powerpc/kernel/signal_32.c | 224 ++++++++++++++++----------------
> 1 file changed, 111 insertions(+), 113 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c
> b/arch/powerpc/kernel/signal_32.c
> index 86539a4e0514..f795fe0240a1 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -93,8 +93,8 @@ static inline int get_sigset_t(sigset_t *set,
> #define to_user_ptr(p) ptr_to_compat(p)
> #define from_user_ptr(p) compat_ptr(p)
>  
> -static inline int save_general_regs(struct pt_regs *regs,
> - struct mcontext __user *frame)
> +static __always_inline int
> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> *frame)
> {
> elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
> int val, i;
> @@ -108,10 +108,12 @@ static inline int save_general_regs(struct pt_regs
> *regs,
> else
> val = gregs[i];
>  
> - if (__put_user(val, &frame->mc_gregs[i]))
> - return -EFAULT;
> + unsafe_put_user(val, &frame->mc_gregs[i], failed);
> }
> return 0;
> +
> +failed:
> + return 1;
> }
>  
> static inline int restore_general_regs(struct pt_regs *regs,
> @@ -148,11 +150,15 @@ static inline int get_sigset_t(sigset_t *set,
> const sigset_t __user *uset)
> #define to_user_ptr(p) ((unsigned long)(p))
> #define from_user_ptr(p) ((void __user *)(p))
>  
> -static inline int save_general_regs(struct pt_regs *regs,
> - struct mcontext __user *frame)
> +static __always_inline int
> +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> *frame)
> {
> WARN_ON(!FULL_REGS(regs));
> - return __copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE);
> + unsafe_copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE, failed);
> + return 0;
> +
> +failed:
> + return 1;
> }
>  
> static inline int restore_general_regs(struct pt_regs *regs,
> @@ -170,6 +176,11 @@ static inline int restore_general_regs(struct
> pt_regs *regs,
> }
> #endif
>  
> +#define unsafe_save_general_regs(regs, frame, label) do { \
> + if (save_general_regs_unsafe(regs, frame)) \

Minor nitpick (sorry); this naming seems a bit strange to me, in x86 it
is "__unsafe_" as a prefix instead of "_unsafe" as a suffix. That sounds
a bit better to me, what do you think? Unless there is some convention I
am not aware of here apart from "unsafe_" using a goto label for errors.

> + goto label; \
> +} while (0)
> +
> /*
> * When we have signals to deliver, we set up on the
> * user stack, going down from the original stack pointer:
> @@ -249,21 +260,19 @@ static void prepare_save_user_regs(int
> ctx_has_vsx_region)
> #endif
> }
>  
> -static int save_user_regs(struct pt_regs *regs, struct mcontext __user
> *frame,
> - struct mcontext __user *tm_frame, int ctx_has_vsx_region)
> +static int save_user_regs_unsafe(struct pt_regs *regs, struct mcontext
> __user *frame,
> + struct mcontext __user *tm_frame, int ctx_has_vsx_region)
> {
> unsigned long msr = regs->msr;
>  
> /* save general registers */
> - if (save_general_regs(regs, frame))
> - return 1;
> + unsafe_save_general_regs(regs, frame, failed);
>  
> #ifdef CONFIG_ALTIVEC
> /* save altivec registers */
> if (current->thread.used_vr) {
> - if (__copy_to_user(&frame->mc_vregs, &current->thread.vr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, &current->thread.vr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> /* set MSR_VEC in the saved MSR value to indicate that
> frame->mc_vregs contains valid data */
> msr |= MSR_VEC;
> @@ -276,11 +285,10 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * most significant bits of that same vector. --BenH
> * Note that the current VRSAVE value is in the SPR at this point.
> */
> - if (__put_user(current->thread.vrsave, (u32 __user
> *)&frame->mc_vregs[32]))
> - return 1;
> + unsafe_put_user(current->thread.vrsave, (u32 __user
> *)&frame->mc_vregs[32],
> + failed);
> #endif /* CONFIG_ALTIVEC */
> - if (copy_fpr_to_user(&frame->mc_fregs, current))
> - return 1;
> + unsafe_copy_fpr_to_user(&frame->mc_fregs, current, failed);
>  
> /*
> * Clear the MSR VSX bit to indicate there is no valid state attached
> @@ -295,17 +303,15 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * contains valid data
> */
> if (current->thread.used_vsr && ctx_has_vsx_region) {
> - if (copy_vsx_to_user(&frame->mc_vsregs, current))
> - return 1;
> + unsafe_copy_vsx_to_user(&frame->mc_vsregs, current, failed);
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> /* save spe registers */
> if (current->thread.used_spe) {
> - if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
> - ELF_NEVRREG * sizeof(u32)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> + ELF_NEVRREG * sizeof(u32)), failed);
> /* set MSR_SPE in the saved MSR value to indicate that
> frame->mc_vregs contains valid data */
> msr |= MSR_SPE;
> @@ -313,21 +319,29 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> /* else assert((regs->msr & MSR_SPE) == 0) */
>  
> /* We always copy to/from spefscr */
> - if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs
> + ELF_NEVRREG))
> - return 1;
> + unsafe_put_user(current->thread.spefscr,
> + (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
> #endif /* CONFIG_SPE */
>  
> - if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
> +
> /* We need to write 0 the MSR top 32 bits in the tm frame so that we
> * can check it on the restore to see if TM is active
> */
> - if (tm_frame && __put_user(0, &tm_frame->mc_gregs[PT_MSR]))
> - return 1;
> + if (tm_frame)
> + unsafe_put_user(0, &tm_frame->mc_gregs[PT_MSR], failed);
>  
> return 0;
> +
> +failed:
> + return 1;
> }
>  
> +#define unsafe_save_user_regs(regs, frame, tm_frame, has_vsx, label) do
> { \
> + if (save_user_regs_unsafe(regs, frame, tm_frame, has_vsx)) \
> + goto label; \
> +} while (0)
> +
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /*
> * Save the current user registers on the user stack.
> @@ -336,7 +350,7 @@ static int save_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame,
> * We also save the transactional registers to a second ucontext in the
> * frame.
> *
> - * See save_user_regs() and signal_64.c:setup_tm_sigcontexts().
> + * See save_user_regs_unsafe() and signal_64.c:setup_tm_sigcontexts().
> */
> static void prepare_save_tm_user_regs(void)
> {
> @@ -352,13 +366,12 @@ static void prepare_save_tm_user_regs(void)
> #endif
> }
>  
> -static int save_tm_user_regs(struct pt_regs *regs, struct mcontext
> __user *frame,
> - struct mcontext __user *tm_frame, unsigned long msr)
> +static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct
> mcontext __user *frame,
> + struct mcontext __user *tm_frame, unsigned long msr)
> {
> /* Save both sets of general registers */
> - if (save_general_regs(&current->thread.ckpt_regs, frame)
> - || save_general_regs(regs, tm_frame))
> - return 1;
> + unsafe_save_general_regs(&current->thread.ckpt_regs, frame, failed);
> + unsafe_save_general_regs(regs, tm_frame, failed);
>  
> /* Stash the top half of the 64bit MSR into the 32bit MSR word
> * of the transactional mcontext. This way we have a backward-compatible
> @@ -366,26 +379,21 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * also look at what type of transaction (T or S) was active at the
> * time of the signal.
> */
> - if (__put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR], failed);
>  
> #ifdef CONFIG_ALTIVEC
> /* save altivec registers */
> if (current->thread.used_vr) {
> - if (__copy_to_user(&frame->mc_vregs, &current->thread.ckvr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - if (msr & MSR_VEC) {
> - if (__copy_to_user(&tm_frame->mc_vregs,
> - &current->thread.vr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - } else {
> - if (__copy_to_user(&tm_frame->mc_vregs,
> - &current->thread.ckvr_state,
> - ELF_NVRREG * sizeof(vector128)))
> - return 1;
> - }
> + unsafe_copy_to_user(&frame->mc_vregs, &current->thread.ckvr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> + if (msr & MSR_VEC)
> + unsafe_copy_to_user(&tm_frame->mc_vregs,
> + &current->thread.vr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
> + else
> + unsafe_copy_to_user(&tm_frame->mc_vregs,
> + &current->thread.ckvr_state,
> + ELF_NVRREG * sizeof(vector128), failed);
>  
> /* set MSR_VEC in the saved MSR value to indicate that
> * frame->mc_vregs contains valid data
> @@ -398,29 +406,21 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * significant bits of a vector, we "cheat" and stuff VRSAVE in the
> * most significant bits of that same vector. --BenH
> */
> - if (__put_user(current->thread.ckvrsave,
> - (u32 __user *)&frame->mc_vregs[32]))
> - return 1;
> - if (msr & MSR_VEC) {
> - if (__put_user(current->thread.vrsave,
> - (u32 __user *)&tm_frame->mc_vregs[32]))
> - return 1;
> - } else {
> - if (__put_user(current->thread.ckvrsave,
> - (u32 __user *)&tm_frame->mc_vregs[32]))
> - return 1;
> - }
> + unsafe_put_user(current->thread.ckvrsave,
> + (u32 __user *)&frame->mc_vregs[32], failed);
> + if (msr & MSR_VEC)
> + unsafe_put_user(current->thread.vrsave,
> + (u32 __user *)&tm_frame->mc_vregs[32], failed);
> + else
> + unsafe_put_user(current->thread.ckvrsave,
> + (u32 __user *)&tm_frame->mc_vregs[32], failed);
> #endif /* CONFIG_ALTIVEC */
>  
> - if (copy_ckfpr_to_user(&frame->mc_fregs, current))
> - return 1;
> - if (msr & MSR_FP) {
> - if (copy_fpr_to_user(&tm_frame->mc_fregs, current))
> - return 1;
> - } else {
> - if (copy_ckfpr_to_user(&tm_frame->mc_fregs, current))
> - return 1;
> - }
> + unsafe_copy_ckfpr_to_user(&frame->mc_fregs, current, failed);
> + if (msr & MSR_FP)
> + unsafe_copy_fpr_to_user(&tm_frame->mc_fregs, current, failed);
> + else
> + unsafe_copy_ckfpr_to_user(&tm_frame->mc_fregs, current, failed);
>  
> #ifdef CONFIG_VSX
> /*
> @@ -430,53 +430,54 @@ static int save_tm_user_regs(struct pt_regs *regs,
> struct mcontext __user *frame
> * contains valid data
> */
> if (current->thread.used_vsr) {
> - if (copy_ckvsx_to_user(&frame->mc_vsregs, current))
> - return 1;
> - if (msr & MSR_VSX) {
> - if (copy_vsx_to_user(&tm_frame->mc_vsregs,
> - current))
> - return 1;
> - } else {
> - if (copy_ckvsx_to_user(&tm_frame->mc_vsregs, current))
> - return 1;
> - }
> + unsafe_copy_ckvsx_to_user(&frame->mc_vsregs, current, failed);
> + if (msr & MSR_VSX)
> + unsafe_copy_vsx_to_user(&tm_frame->mc_vsregs, current, failed);
> + else
> + unsafe_copy_ckvsx_to_user(&tm_frame->mc_vsregs, current, failed);
>  
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> /* SPE regs are not checkpointed with TM, so this section is
> - * simply the same as in save_user_regs().
> + * simply the same as in save_user_regs_unsafe().
> */
> if (current->thread.used_spe) {
> - if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
> - ELF_NEVRREG * sizeof(u32)))
> - return 1;
> + unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> + ELF_NEVRREG * sizeof(u32), failed);
> /* set MSR_SPE in the saved MSR value to indicate that
> * frame->mc_vregs contains valid data */
> msr |= MSR_SPE;
> }
>  
> /* We always copy to/from spefscr */
> - if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs
> + ELF_NEVRREG))
> - return 1;
> + unsafe_put_user(current->thread.spefscr,
> + (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
> #endif /* CONFIG_SPE */
>  
> - if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
> - return 1;
> + unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
>  
> return 0;
> +
> +failed:
> + return 1;
> }
> #else
> static void prepare_save_tm_user_regs(void) { }
>  
> -static int save_tm_user_regs(struct pt_regs *regs, struct mcontext
> __user *frame,
> - struct mcontext __user *tm_frame, unsigned long msr)
> +static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct
> mcontext __user *frame,
> + struct mcontext __user *tm_frame, unsigned long msr)
> {
> return 0;
> }
> #endif
>  
> +#define unsafe_save_tm_user_regs(regs, frame, tm_frame, msr, label) do
> { \
> + if (save_tm_user_regs_unsafe(regs, frame, tm_frame, msr)) \
> + goto label; \
> +} while (0)
> +
> /*
> * Restore the current user register values from the user stack,
> * (except for MSR).
> @@ -769,6 +770,11 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_mctx = &frame->uc_transact.uc_mcontext;
> #endif
> + if (MSR_TM_ACTIVE(msr))
> + prepare_save_tm_user_regs();
> + else
> + prepare_save_user_regs(1);
> +
> if (!user_write_access_begin(frame, sizeof(*frame)))
> goto badframe;
>  
> @@ -788,8 +794,10 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> unsafe_put_user((unsigned long)tm_mctx,
> &frame->uc_transact.uc_regs, failed);
> #endif
> + unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
> } else {
> unsafe_put_user(0, &frame->uc.uc_link, failed);
> + unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
> }
>  
> /* Save user registers on the stack */
> @@ -812,15 +820,6 @@ int handle_rt_signal32(struct ksignal *ksig,
> sigset_t *oldset,
> if (tramp == (unsigned long)mctx->mc_pad)
> flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
>  
> - if (MSR_TM_ACTIVE(msr)) {
> - prepare_save_tm_user_regs();
> - if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
> - goto badframe;
> - } else {
> - prepare_save_user_regs(1);
> - if (save_user_regs(regs, mctx, tm_mctx, 1))
> - goto badframe;
> - }
> regs->link = tramp;
>  
> #ifdef CONFIG_PPC_FPU_REGS
> @@ -875,6 +874,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> tm_mctx = &frame->mctx_transact;
> #endif
> + if (MSR_TM_ACTIVE(msr))
> + prepare_save_tm_user_regs();
> + else
> + prepare_save_user_regs(1);
> +
> if (!user_write_access_begin(frame, sizeof(*frame)))
> goto badframe;
> sc = (struct sigcontext __user *) &frame->sctx;
> @@ -892,6 +896,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> unsafe_put_user(to_user_ptr(mctx), &sc->regs, failed);
> unsafe_put_user(ksig->sig, &sc->signal, failed);
>  
> + if (MSR_TM_ACTIVE(msr))
> + unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
> + else
> + unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
> +
> if (vdso32_sigtramp && tsk->mm->context.vdso_base) {
> tramp = tsk->mm->context.vdso_base + vdso32_sigtramp;
> } else {
> @@ -905,16 +914,6 @@ int handle_signal32(struct ksignal *ksig, sigset_t
> *oldset,
> if (tramp == (unsigned long)mctx->mc_pad)
> flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
>  
> - if (MSR_TM_ACTIVE(msr)) {
> - prepare_save_tm_user_regs();
> - if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
> - goto badframe;
> - } else {
> - prepare_save_user_regs(1);
> - if (save_user_regs(regs, mctx, tm_mctx, 1))
> - goto badframe;
> - }
> -
> regs->link = tramp;
>  
> #ifdef CONFIG_PPC_FPU_REGS
> @@ -1066,10 +1065,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext
> __user *, old_ctx,
> mctx = (struct mcontext __user *)
> ((unsigned long) &old_ctx->uc_mcontext & ~0xfUL);
> prepare_save_user_regs(ctx_has_vsx_region);
> - if (save_user_regs(regs, mctx, NULL, ctx_has_vsx_region))
> - return -EFAULT;
> if (!user_write_access_begin(old_ctx, ctx_size))
> return -EFAULT;
> + unsafe_save_user_regs(regs, mctx, NULL, ctx_has_vsx_region, failed);
> unsafe_put_sigset_t(&old_ctx->uc_sigmask, &current->blocked, failed);
> unsafe_put_user(to_user_ptr(mctx), &old_ctx->uc_regs, failed);
> user_write_access_end();
> --
> 2.25.0

Powered by blists - more mailing lists