From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2B200219A8E for ; Tue, 16 Dec 2025 09:46:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765878402; cv=none; b=Z0+83V7PsEclyJPUG44LJ7Bxnw2UHOfrEUkPh1kXO8Bwy30Lt4Kdb9vX/l8kCBVGi8WdkLSxa+My3nFs8xW15NJ8mHrCyTUYUqMwjJC2BHnTfhW5l2ihhn/g3LBy1NL8irXIHg8vwr6K6hcozOi2UVd/UyKGw/IU00cwdE4UPnE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765878402; c=relaxed/simple; bh=TGMHFK24tNRBpxQq8qH9ZxIW046JIxdiOpQ7jMhz834=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=ohNreOTh1K3mmnUMKHE5gk+eytxjurCAsORhsHiUORPZjmqIXZiY6hYHZTZb31pcL5QyJTGGClASbeEaew2tyDzuNZfQxCJrVnn2IxcHsyDyDvpgt46LVeMp/EcmSa4Br1R6DvM1uoLbCc2Zz4CxIXD+b+n/jJsgL9mteCYgPuY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Mw4FdrAC; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Mw4FdrAC" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E678AC4CEF1; Tue, 16 Dec 2025 09:46:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1765878401; bh=TGMHFK24tNRBpxQq8qH9ZxIW046JIxdiOpQ7jMhz834=; h=Date:Subject:To:References:From:In-Reply-To:From; b=Mw4FdrACFB7nfJhg3cjGSayy90XjQRbOmF7jVQS308YtKNWtsAD7I6aYZqQf5fdQL J3fgfMIl7e8LnX0dok+AVjy5j5ByKFVd5Lk+cVVwXLo1J+22mY3k1O8VADmE1nPq6s obL+YF05RkQM0z/rQpldfguH3pvFqeGgDfH74F9VxoKQPcFMtG5NR1OFAiEhYRy4/d pKNUDmDGUiwoaEFgf1in2B4ItJoIV1LRmHrvOsFCC8YhhtqqVgcdvZstUklr0s6g47 zCTs4SgRQmkC5bkr12iX9dgBYb2rgkhCdkuWH1o9eTwBKir+vHH7lpYyVCwINhnBo1 VBxZTE8VZM8OQ== Message-ID: <89a25fd0-2d49-4062-a194-55f21350e5a8@kernel.org> Date: Tue, 16 Dec 2025 10:46:28 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 4/8] powerpc: Introduce syscall exit arch functions To: Mukesh Kumar Chaurasiya , maddy@linux.ibm.com, mpe@ellerman.id.au, npiggin@gmail.com, oleg@redhat.com, kees@kernel.org, luto@amacapital.net, wad@chromium.org, mchauras@linux.ibm.com, thuth@redhat.com, sshegde@linux.ibm.com, charlie@rivosinc.com, macro@orcam.me.uk, akpm@linux-foundation.org, ldv@strace.io, deller@gmx.de, ankur.a.arora@oracle.com, segher@kernel.crashing.org, tglx@linutronix.de, thomas.weissschuh@linutronix.de, peterz@infradead.org, menglong8.dong@gmail.com, bigeasy@linutronix.de, namcao@linutronix.de, kan.liang@linux.intel.com, mingo@kernel.org, atrajeev@linux.vnet.ibm.com, mark.barnett@arm.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org References: <20251214130245.43664-1-mkchauras@linux.ibm.com> <20251214130245.43664-5-mkchauras@linux.ibm.com> Content-Language: fr-FR From: "Christophe Leroy (CS GROUP)" In-Reply-To: <20251214130245.43664-5-mkchauras@linux.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Le 14/12/2025 à 14:02, Mukesh Kumar Chaurasiya a écrit : > From: Mukesh Kumar Chaurasiya > > Add PowerPC-specific implementations of the generic syscall exit hooks > used by the generic entry/exit framework: > > - arch_exit_to_user_mode_work_prepare() > - arch_exit_to_user_mode_work() > > These helpers handle user state restoration when returning from the > kernel to userspace, including FPU/VMX/VSX state, transactional memory, > KUAP restore, and per-CPU accounting. > > Additionally, move check_return_regs_valid() from interrupt.c to > interrupt.h so it can be shared by the new entry/exit logic, and add > arch_do_signal_or_restart() for use with the generic entry flow. > > No functional change is intended with this patch. > > Signed-off-by: Mukesh Kumar Chaurasiya > --- > arch/powerpc/include/asm/entry-common.h | 49 +++++++++++++++ > arch/powerpc/include/asm/interrupt.h | 82 +++++++++++++++++++++++++ > arch/powerpc/kernel/interrupt.c | 81 ------------------------ > arch/powerpc/kernel/signal.c | 14 +++++ > 4 files changed, 145 insertions(+), 81 deletions(-) > > diff --git a/arch/powerpc/include/asm/entry-common.h b/arch/powerpc/include/asm/entry-common.h > index 093ece06ef79..e8ebd42a4e6d 100644 > --- a/arch/powerpc/include/asm/entry-common.h > +++ b/arch/powerpc/include/asm/entry-common.h > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > #include > > static __always_inline void arch_enter_from_user_mode(struct pt_regs *regs) > @@ -104,5 +105,53 @@ static __always_inline void arch_enter_from_user_mode(struct pt_regs *regs) > > #define arch_enter_from_user_mode arch_enter_from_user_mode > > +static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs, > + unsigned long ti_work) > +{ > + unsigned long mathflags; > + > + if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && IS_ENABLED(CONFIG_PPC_FPU)) { > + if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) && > + unlikely((ti_work & _TIF_RESTORE_TM))) { > + restore_tm_state(regs); > + } else { > + mathflags = MSR_FP; > + > + if (cpu_has_feature(CPU_FTR_VSX)) > + mathflags |= MSR_VEC | MSR_VSX; > + else if (cpu_has_feature(CPU_FTR_ALTIVEC)) > + mathflags |= MSR_VEC; > + > + /* > + * If userspace MSR has all available FP bits set, > + * then they are live and no need to restore. If not, > + * it means the regs were given up and restore_math > + * may decide to restore them (to avoid taking an FP > + * fault). > + */ > + if ((regs->msr & mathflags) != mathflags) > + restore_math(regs); > + } > + } > + > + check_return_regs_valid(regs); > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + local_paca->tm_scratch = regs->msr; > +#endif > + /* Restore user access locks last */ > + kuap_user_restore(regs); > +} > + > +#define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare > + > +static __always_inline void arch_exit_to_user_mode(void) > +{ > + booke_load_dbcr0(); > + > + account_cpu_user_exit(); > +} > + > +#define arch_exit_to_user_mode arch_exit_to_user_mode > + > #endif /* CONFIG_GENERIC_IRQ_ENTRY */ > #endif /* _ASM_PPC_ENTRY_COMMON_H */ > diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h > index ca8a2cda9400..77ff8e33f8cd 100644 > --- a/arch/powerpc/include/asm/interrupt.h > +++ b/arch/powerpc/include/asm/interrupt.h > @@ -68,6 +68,8 @@ > > #include > #include > +#include /* for show_regs */ > + > #include > #include > #include > @@ -172,6 +174,86 @@ static inline void booke_restore_dbcr0(void) > #endif > } > > +static inline void check_return_regs_valid(struct pt_regs *regs) This was previously a notrace function. Should it be marked __always_inline instead of just inline ? > +{ > +#ifdef CONFIG_PPC_BOOK3S_64 > + unsigned long trap, srr0, srr1; > + static bool warned; > + u8 *validp; > + char *h; > + > + if (trap_is_scv(regs)) > + return; > + > + trap = TRAP(regs); > + // EE in HV mode sets HSRRs like 0xea0 > + if (cpu_has_feature(CPU_FTR_HVMODE) && trap == INTERRUPT_EXTERNAL) > + trap = 0xea0; > + > + switch (trap) { > + case 0x980: > + case INTERRUPT_H_DATA_STORAGE: > + case 0xe20: > + case 0xe40: > + case INTERRUPT_HMI: > + case 0xe80: > + case 0xea0: > + case INTERRUPT_H_FAC_UNAVAIL: > + case 0x1200: > + case 0x1500: > + case 0x1600: > + case 0x1800: > + validp = &local_paca->hsrr_valid; > + if (!READ_ONCE(*validp)) > + return; > + > + srr0 = mfspr(SPRN_HSRR0); > + srr1 = mfspr(SPRN_HSRR1); > + h = "H"; > + > + break; > + default: > + validp = &local_paca->srr_valid; > + if (!READ_ONCE(*validp)) > + return; > + > + srr0 = mfspr(SPRN_SRR0); > + srr1 = mfspr(SPRN_SRR1); > + h = ""; > + break; > + } > + > + if (srr0 == regs->nip && srr1 == regs->msr) > + return; > + > + /* > + * A NMI / soft-NMI interrupt may have come in after we found > + * srr_valid and before the SRRs are loaded. The interrupt then > + * comes in and clobbers SRRs and clears srr_valid. Then we load > + * the SRRs here and test them above and find they don't match. > + * > + * Test validity again after that, to catch such false positives. > + * > + * This test in general will have some window for false negatives > + * and may not catch and fix all such cases if an NMI comes in > + * later and clobbers SRRs without clearing srr_valid, but hopefully > + * such things will get caught most of the time, statistically > + * enough to be able to get a warning out. > + */ > + if (!READ_ONCE(*validp)) > + return; > + > + if (!data_race(warned)) { > + data_race(warned = true); > + pr_warn("%sSRR0 was: %lx should be: %lx\n", h, srr0, regs->nip); > + pr_warn("%sSRR1 was: %lx should be: %lx\n", h, srr1, regs->msr); > + show_regs(regs); > + } > + > + WRITE_ONCE(*validp, 0); /* fixup */ > +#endif > +} > + > static inline void interrupt_enter_prepare(struct pt_regs *regs) > { > #ifdef CONFIG_PPC64 > diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c > index 2a09ac5dabd6..f53d432f6087 100644 > --- a/arch/powerpc/kernel/interrupt.c > +++ b/arch/powerpc/kernel/interrupt.c > @@ -4,7 +4,6 @@ > #include > #include > #include > -#include /* for show_regs */ > > #include > #include > @@ -78,86 +77,6 @@ static notrace __always_inline bool prep_irq_for_enabled_exit(bool restartable) > return true; > } > > -static notrace void check_return_regs_valid(struct pt_regs *regs) > -{ > -#ifdef CONFIG_PPC_BOOK3S_64 > - unsigned long trap, srr0, srr1; > - static bool warned; > - u8 *validp; > - char *h; > - > - if (trap_is_scv(regs)) > - return; > - > - trap = TRAP(regs); > - // EE in HV mode sets HSRRs like 0xea0 > - if (cpu_has_feature(CPU_FTR_HVMODE) && trap == INTERRUPT_EXTERNAL) > - trap = 0xea0; > - > - switch (trap) { > - case 0x980: > - case INTERRUPT_H_DATA_STORAGE: > - case 0xe20: > - case 0xe40: > - case INTERRUPT_HMI: > - case 0xe80: > - case 0xea0: > - case INTERRUPT_H_FAC_UNAVAIL: > - case 0x1200: > - case 0x1500: > - case 0x1600: > - case 0x1800: > - validp = &local_paca->hsrr_valid; > - if (!READ_ONCE(*validp)) > - return; > - > - srr0 = mfspr(SPRN_HSRR0); > - srr1 = mfspr(SPRN_HSRR1); > - h = "H"; > - > - break; > - default: > - validp = &local_paca->srr_valid; > - if (!READ_ONCE(*validp)) > - return; > - > - srr0 = mfspr(SPRN_SRR0); > - srr1 = mfspr(SPRN_SRR1); > - h = ""; > - break; > - } > - > - if (srr0 == regs->nip && srr1 == regs->msr) > - return; > - > - /* > - * A NMI / soft-NMI interrupt may have come in after we found > - * srr_valid and before the SRRs are loaded. The interrupt then > - * comes in and clobbers SRRs and clears srr_valid. Then we load > - * the SRRs here and test them above and find they don't match. > - * > - * Test validity again after that, to catch such false positives. > - * > - * This test in general will have some window for false negatives > - * and may not catch and fix all such cases if an NMI comes in > - * later and clobbers SRRs without clearing srr_valid, but hopefully > - * such things will get caught most of the time, statistically > - * enough to be able to get a warning out. > - */ > - if (!READ_ONCE(*validp)) > - return; > - > - if (!data_race(warned)) { > - data_race(warned = true); > - printk("%sSRR0 was: %lx should be: %lx\n", h, srr0, regs->nip); > - printk("%sSRR1 was: %lx should be: %lx\n", h, srr1, regs->msr); > - show_regs(regs); > - } > - > - WRITE_ONCE(*validp, 0); /* fixup */ > -#endif > -} > - > static notrace unsigned long > interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs) > { > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c > index aa17e62f3754..719930cf4ae1 100644 > --- a/arch/powerpc/kernel/signal.c > +++ b/arch/powerpc/kernel/signal.c > @@ -22,6 +22,11 @@ > > #include "signal.h" > > +/* This will be removed */ > +#ifdef CONFIG_GENERIC_ENTRY Is this #ifdef really needed ? > +#include > +#endif /* CONFIG_GENERIC_ENTRY */ > + > #ifdef CONFIG_VSX > unsigned long copy_fpr_to_user(void __user *to, > struct task_struct *task) > @@ -368,3 +373,12 @@ void signal_fault(struct task_struct *tsk, struct pt_regs *regs, > printk_ratelimited(regs->msr & MSR_64BIT ? fm64 : fm32, tsk->comm, > task_pid_nr(tsk), where, ptr, regs->nip, regs->link); > } > + > +#ifdef CONFIG_GENERIC_ENTRY Why is this #ifdef needed ? > +void arch_do_signal_or_restart(struct pt_regs *regs) > +{ > + BUG_ON(regs != current->thread.regs); Is this BUG_ON() needed ? Can't we use something smoother ? > + local_paca->generic_fw_flags |= GFW_RESTORE_ALL; > + do_signal(current); > +} > +#endif /* CONFIG_GENERIC_ENTRY */