From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9DAB3C07CB1 for ; Mon, 27 Nov 2023 13:10:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=5OhpgZa6BAy9f/mz8nTCAELJHzENfgdZLrPyJQQGkCM=; b=JQc25i+BfULFG/ jfsFIZ5NZ1YcITYpZHFmL91jslrV19CLo6z202zNNH6q8FoHPGBdE8Eifx8yVx/0QmYF9yw3pndii X2K2Nt5RJw5JczBc4ISoDzLm6paBbXwmxdyIjt8bvo6wOGgvj/rk5ZXitasPkijRVgRDAy7DQ4ecI Ok1e29zAewSlqZcMSt5uOnESw7rQYIOgFtoig/aq79l3j1F4rvA+uPzTHIKgWzDRTMpj0o7L7AfjR 0M0dhUlPaWspYpCH7SHltL/sXes6HPutLIc1Kl7h5M0rhdvgsNNfaFmXBXmJeX+X0tv6CG1ttUJxe PAvTWJ0NIZ4eZ4tsI6FA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r7bND-002WdX-0e; Mon, 27 Nov 2023 13:10:03 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r7bN9-002Wcq-2L for linux-arm-kernel@lists.infradead.org; Mon, 27 Nov 2023 13:10:01 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7C7102F4; Mon, 27 Nov 2023 05:10:43 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.43.171]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 29BD73F73F; Mon, 27 Nov 2023 05:09:54 -0800 (PST) Date: Mon, 27 Nov 2023 13:09:51 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: linux-arm-kernel@lists.infradead.org, linux-crypto@vger.kernel.org, Ard Biesheuvel , Marc Zyngier , Will Deacon , Kees Cook , Catalin Marinas , Mark Brown , Eric Biggers , Sebastian Andrzej Siewior Subject: Re: [PATCH v3 2/5] arm64: fpsimd: Preserve/restore kernel mode NEON at context switch Message-ID: References: <20231127122259.2265164-7-ardb@google.com> <20231127122259.2265164-9-ardb@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231127122259.2265164-9-ardb@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231127_050959_860365_E07C8A1B X-CRM114-Status: GOOD ( 51.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Ard, On Mon, Nov 27, 2023 at 01:23:02PM +0100, Ard Biesheuvel wrote: > From: Ard Biesheuvel > > Currently, the FPSIMD register file is not preserved and restored along > with the general registers on exception entry/exit or context switch. > For this reason, we disable preemption when enabling FPSIMD for kernel > mode use in task context, and suspend the processing of softirqs so that > there are no concurrent uses in the kernel. (Kernel mode FPSIMD may not > be used at all in other contexts). > > Disabling preemption while doing CPU intensive work on inputs of > potentially unbounded size is bad for real-time performance, which is > why we try and ensure that SIMD crypto code does not operate on more > than ~4k at a time, which is an arbitrary limit and requires assembler > code to implement efficiently. > > We can avoid the need for disabling preemption if we can ensure that any > in-kernel users of the NEON will not lose the FPSIMD register state > across a context switch. And given that disabling softirqs implicitly > disables preemption as well, we will also have to ensure that a softirq > that runs code using FPSIMD can safely interrupt an in-kernel user. > > So introduce a thread_info flag TIF_USING_KMODE_FPSIMD, and modify the > context switch hook for FPSIMD to preserve and restore the kernel mode > FPSIMD to/from struct thread_struct when it is set. This avoids any > scheduling blackouts due to prolonged use of FPSIMD in kernel mode, > without the need for manual yielding. > > In order to support softirq processing while FPSIMD is being used in > kernel task context, use the same flag to decide whether the kernel mode > FPSIMD state needs to be preserved and restored before allowing FPSIMD > to be used in softirq context. > > Signed-off-by: Ard Biesheuvel > Reviewed-by: Mark Brown I have a couple of naming/structural comments below, but this looks functionally good to me. I appreciate those are arguable bikeshedding, so either way: Reviewed-by: Mark Rutland > --- > arch/arm64/include/asm/processor.h | 2 + > arch/arm64/include/asm/thread_info.h | 1 + > arch/arm64/kernel/fpsimd.c | 92 ++++++++++++++++---- > 3 files changed, 77 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index e5bc54522e71..dcb51c0571af 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -167,6 +167,8 @@ struct thread_struct { > unsigned long fault_address; /* fault info */ > unsigned long fault_code; /* ESR_EL1 value */ > struct debug_info debug; /* debugging */ > + > + struct user_fpsimd_state kmode_fpsimd_state; > #ifdef CONFIG_ARM64_PTR_AUTH > struct ptrauth_keys_user keys_user; > #ifdef CONFIG_ARM64_PTR_AUTH_KERNEL > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index 553d1bc559c6..6b254cf90e8b 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -80,6 +80,7 @@ void arch_setup_new_exec(void); > #define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */ > #define TIF_SME 27 /* SME in use */ > #define TIF_SME_VL_INHERIT 28 /* Inherit SME vl_onexec across exec */ > +#define TIF_USING_KMODE_FPSIMD 29 /* Task is in a kernel mode FPSIMD section */ Sorry for the bikeshedding, but "KMODE" isn't a term we use elsewhere, and I think it'd be nicer/clearer if this had "KERNEL" spelled out in full, especially as it's only 1 additional character. Could this be TIF_FPSIMD_KERNEL, or maybe TIF_KERNEL_FPSTATE to align with TIF_FORIEGN_FPSTATE? > > #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) > #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index ccc4a78a70e4..198918805bf6 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -357,6 +357,7 @@ static void task_fpsimd_load(void) > > WARN_ON(!system_supports_fpsimd()); > WARN_ON(preemptible()); > + WARN_ON(test_thread_flag(TIF_USING_KMODE_FPSIMD)); > > if (system_supports_sve() || system_supports_sme()) { > switch (current->thread.fp_type) { > @@ -379,7 +380,7 @@ static void task_fpsimd_load(void) > default: > /* > * This indicates either a bug in > - * fpsimd_save() or memory corruption, we > + * fpsimd_save_user_state() or memory corruption, we > * should always record an explicit format > * when we save. We always at least have the > * memory allocated for FPSMID registers so > @@ -430,7 +431,7 @@ static void task_fpsimd_load(void) > * than via current, if we are saving KVM state then it will have > * ensured that the type of registers to save is set in last->to_save. > */ > -static void fpsimd_save(void) > +static void fpsimd_save_user_state(void) > { > struct cpu_fp_state const *last = > this_cpu_ptr(&fpsimd_last_state); > @@ -861,7 +862,7 @@ int vec_set_vector_length(struct task_struct *task, enum vec_type type, > if (task == current) { > get_cpu_fpsimd_context(); > > - fpsimd_save(); > + fpsimd_save_user_state(); > } > > fpsimd_flush_task_state(task); > @@ -1473,6 +1474,16 @@ void do_fpsimd_exc(unsigned long esr, struct pt_regs *regs) > current); > } > > +static void fpsimd_load_kernel_state(struct task_struct *task) > +{ > + fpsimd_load_state(&task->thread.kmode_fpsimd_state); > +} > + > +static void fpsimd_save_kernel_state(struct task_struct *task) > +{ > + fpsimd_save_state(&task->thread.kmode_fpsimd_state); > +} > + > void fpsimd_thread_switch(struct task_struct *next) > { > bool wrong_task, wrong_cpu; > @@ -1483,19 +1494,28 @@ void fpsimd_thread_switch(struct task_struct *next) > WARN_ON_ONCE(!irqs_disabled()); > > /* Save unsaved fpsimd state, if any: */ > - fpsimd_save(); > + if (!test_thread_flag(TIF_USING_KMODE_FPSIMD)) > + fpsimd_save_user_state(); > + else > + fpsimd_save_kernel_state(current); Minor nit: I find this condition inversion slightly hard to read since in prose it'd be "if there's not kernel state, save the user state; else save the kernel state", whereas: if (test_thread_flag(TIF_USING_KMODE_FPSIMD)) fpsimd_save_kernel_state(current); else fpsimd_save_user_state(); ... is more clearly "if there's kernel state, save it; else save the user state", and I think that'd be preferable. > > - /* > - * Fix up TIF_FOREIGN_FPSTATE to correctly describe next's > - * state. For kernel threads, FPSIMD registers are never loaded > - * and wrong_task and wrong_cpu will always be true. > - */ > - wrong_task = __this_cpu_read(fpsimd_last_state.st) != > - &next->thread.uw.fpsimd_state; > - wrong_cpu = next->thread.fpsimd_cpu != smp_processor_id(); > + if (test_tsk_thread_flag(next, TIF_USING_KMODE_FPSIMD)) { > + fpsimd_load_kernel_state(next); > + set_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); > + } else { > + /* > + * Fix up TIF_FOREIGN_FPSTATE to correctly describe next's > + * state. For kernel threads, FPSIMD registers are never > + * loaded with user mode FPSIMD state and so wrong_task and > + * wrong_cpu will always be true. > + */ > + wrong_task = __this_cpu_read(fpsimd_last_state.st) != > + &next->thread.uw.fpsimd_state; > + wrong_cpu = next->thread.fpsimd_cpu != smp_processor_id(); > > - update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE, > - wrong_task || wrong_cpu); > + update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE, > + wrong_task || wrong_cpu); > + } > } > > static void fpsimd_flush_thread_vl(enum vec_type type) > @@ -1585,7 +1605,7 @@ void fpsimd_preserve_current_state(void) > return; > > get_cpu_fpsimd_context(); > - fpsimd_save(); > + fpsimd_save_user_state(); > put_cpu_fpsimd_context(); > } > > @@ -1801,7 +1821,7 @@ void fpsimd_save_and_flush_cpu_state(void) > return; > WARN_ON(preemptible()); > get_cpu_fpsimd_context(); > - fpsimd_save(); > + fpsimd_save_user_state(); > fpsimd_flush_cpu_state(); > put_cpu_fpsimd_context(); > } > @@ -1835,10 +1855,37 @@ void kernel_neon_begin(void) > get_cpu_fpsimd_context(); > > /* Save unsaved fpsimd state, if any: */ > - fpsimd_save(); > + if (!test_thread_flag(TIF_USING_KMODE_FPSIMD)) { > + fpsimd_save_user_state(); > + > + /* > + * Set the thread flag so that the kernel mode FPSIMD state > + * will be context switched along with the rest of the task > + * state. > + * > + * On non-PREEMPT_RT, softirqs may interrupt task level kernel > + * mode FPSIMD, but the task will not be preemptible so setting > + * TIF_USING_KMODE_FPSIMD for those would be both wrong (as it > + * would mark the task context FPSIMD state as requiring a > + * context switch) and unnecessary. > + * > + * On PREEMPT_RT, softirqs are serviced from a separate thread, > + * which is scheduled as usual, and this guarantees that these > + * softirqs are not interrupting use of the FPSIMD in kernel > + * mode in task context. So in this case, setting the flag here > + * is always appropriate. > + */ > + if (IS_ENABLED(CONFIG_PREEMPT_RT) || !in_serving_softirq()) > + set_thread_flag(TIF_USING_KMODE_FPSIMD); > + } else { > + BUG_ON(IS_ENABLED(CONFIG_PREEMPT_RT) || !in_serving_softirq()); > + fpsimd_save_kernel_state(current); > + } Same comment as above for condition inversion here. Mark. > > /* Invalidate any task state remaining in the fpsimd regs: */ > fpsimd_flush_cpu_state(); > + > + put_cpu_fpsimd_context(); > } > EXPORT_SYMBOL_GPL(kernel_neon_begin); > > @@ -1856,7 +1903,16 @@ void kernel_neon_end(void) > if (!system_supports_fpsimd()) > return; > > - put_cpu_fpsimd_context(); > + /* > + * If we are returning from a nested use of kernel mode FPSIMD, restore > + * the task context kernel mode FPSIMD state. This can only happen when > + * running in softirq context on non-PREEMPT_RT. > + */ > + if (!IS_ENABLED(CONFIG_PREEMPT_RT) && in_serving_softirq() && > + test_thread_flag(TIF_USING_KMODE_FPSIMD)) > + fpsimd_load_kernel_state(current); > + else > + clear_thread_flag(TIF_USING_KMODE_FPSIMD); > } > EXPORT_SYMBOL_GPL(kernel_neon_end); > > -- > 2.43.0.rc1.413.gea7ed67945-goog > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel