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.le...@csgroup.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, ¤t->thread.vr_state, > - ELF_NVRREG * sizeof(vector128))) > - return 1; > + unsafe_copy_to_user(&frame->mc_vregs, ¤t->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(¤t->thread.ckpt_regs, frame) > - || save_general_regs(regs, tm_frame)) > - return 1; > + unsafe_save_general_regs(¤t->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, ¤t->thread.ckvr_state, > - ELF_NVRREG * sizeof(vector128))) > - return 1; > - if (msr & MSR_VEC) { > - if (__copy_to_user(&tm_frame->mc_vregs, > - ¤t->thread.vr_state, > - ELF_NVRREG * sizeof(vector128))) > - return 1; > - } else { > - if (__copy_to_user(&tm_frame->mc_vregs, > - ¤t->thread.ckvr_state, > - ELF_NVRREG * sizeof(vector128))) > - return 1; > - } > + unsafe_copy_to_user(&frame->mc_vregs, ¤t->thread.ckvr_state, > + ELF_NVRREG * sizeof(vector128), failed); > + if (msr & MSR_VEC) > + unsafe_copy_to_user(&tm_frame->mc_vregs, > + ¤t->thread.vr_state, > + ELF_NVRREG * sizeof(vector128), failed); > + else > + unsafe_copy_to_user(&tm_frame->mc_vregs, > + ¤t->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, ¤t->blocked, failed); > unsafe_put_user(to_user_ptr(mctx), &old_ctx->uc_regs, failed); > user_write_access_end(); > -- > 2.25.0