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 X-Spam-Level: X-Spam-Status: No, score=-13.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5100EC43464 for ; Mon, 21 Sep 2020 12:38:00 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C9FB620756 for ; Mon, 21 Sep 2020 12:37:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="VTqhWSp+"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="TYTSoVEV" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C9FB620756 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=UOOq4rNmOSgQ/ORbFL/YdSplZtDbY9Mo+iNwFan2RWo=; b=VTqhWSp+5SF8zLRfDTypopk5y 1e6ndQCrTPijG2+BaOv3zcRIb6usfPfE3hQr+hQ0EAhXo49u76AAuZGph2wXgO+/8FT1opjnx33es gv6pk+ft+UUeuZ0j2giUhJaOzvbFfaYqA1iGbb1nrPNWpKIT0JJGHdtoKs/CDQmE57Ja1IQNFLCsE hBXf0MlMAmEmCe3zWI18pAGp4nds8kI/kgy8I/h2CC0AJEFHuxbahLDrNSoWn767t3SIXpXonFjLN Nltcczr8F6ql7DxLzwxKfjYNZi/FuiaCzcjYljPWNdU3BP9XfgkjkkBmnntFhnhH21+ilRBcIJ6dy s7bll9FGw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKL3b-0000JN-81; Mon, 21 Sep 2020 12:36:35 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKL3Y-0000J0-Ba for linux-arm-kernel@lists.infradead.org; Mon, 21 Sep 2020 12:36:33 +0000 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 33D2720756; Mon, 21 Sep 2020 12:36:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600691791; bh=2YqkouaO9VoOp3uGBib4624brv3AggQe5qf14nL8uF8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TYTSoVEVcAD69rg+/5NytE2BYgLYW5KgyQDSuRaoCVe67DZRB7qaqce/iRIXvVRAH WgTA5C0qIkOb05KrzREIzD6SPBnsoyTt24lgjjJKRx0YYzLcprU44M37M1n5u7X0v5 h2A8q34vhlsT/PsxovK/enlx+xx57JcWnVvFbwjQ= Date: Mon, 21 Sep 2020 13:36:27 +0100 From: Will Deacon To: Mark Brown Subject: Re: [PATCH v4 7/8] arm64/sve: Don't disable SVE on syscalls return Message-ID: <20200921123625.GF2139@willie-the-truck> References: <20200828181155.17745-1-broonie@kernel.org> <20200828181155.17745-8-broonie@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200828181155.17745-8-broonie@kernel.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200921_083632_544789_330DED66 X-CRM114-Status: GOOD ( 47.98 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Julien Grall , Catalin Marinas , Zhang Lei , Julien Grall , Dave Martin , linux-arm-kernel@lists.infradead.org, Daniel Kiss 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 On Fri, Aug 28, 2020 at 07:11:54PM +0100, Mark Brown wrote: > From: Julien Grall > > Per the syscalls ABI the state of the SVE registers is unknown after a > syscall. In practice the kernel will disable SVE and zero all the > registers but the first 128-bits of the vector on the next SVE > instruction. In workloads mixing SVE and syscalls this will result in at > least one extra entry/exit to the kernel per syscall when the SVE > registers are accessed for the first time after the syscall. > > To avoid the second entry/exit a new flag TIF_SVE_NEEDS_FLUSH is > introduced to mark a task that needs to flush the SVE context on > return to userspace. > > On entry to a syscall the flag TIF_SVE will still be cleared, it will > be restored on return to userspace once the SVE state has been flushed. > This means that if a task requires to synchronize the FP state during a > syscall (e.g context switch, signal) only the FPSIMD registers will be > saved. When the task is rescheduled the SVE state will be loaded from > FPSIMD state. > > We could instead handle flushing the SVE state in do_el0_svc() however > doing this reduces the potential for further optimisations such as > initializing the SVE registers directly from the FPSIMD state when > taking a SVE access trap and has some potential edge cases if we > schedule before we return to userspace after do_el0_svc(). > > Signed-off-by: Julien Grall > Signed-off-by: Mark Brown > --- > arch/arm64/include/asm/thread_info.h | 6 ++- > arch/arm64/kernel/fpsimd.c | 61 +++++++++++++++++++++++++--- > arch/arm64/kernel/process.c | 1 + > arch/arm64/kernel/ptrace.c | 11 +++++ > arch/arm64/kernel/signal.c | 16 +++++++- > arch/arm64/kernel/syscall.c | 13 +++--- > 6 files changed, 91 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index 5e784e16ee89..dfaf872c0a07 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk); > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > +#define TIF_SVE_NEEDS_FLUSH 6 /* Flush SVE registers on return */ > #define TIF_SYSCALL_TRACE 8 /* syscall trace active */ > #define TIF_SYSCALL_AUDIT 9 /* syscall auditing */ > #define TIF_SYSCALL_TRACEPOINT 10 /* syscall tracepoint for ftrace */ > @@ -97,9 +98,12 @@ void arch_release_task_struct(struct task_struct *tsk); > #define _TIF_32BIT (1 << TIF_32BIT) > #define _TIF_SVE (1 << TIF_SVE) > > +#define _TIF_SVE_NEEDS_FLUSH (1 << TIF_SVE_NEEDS_FLUSH) > + > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ > _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ > - _TIF_UPROBE | _TIF_FSCHECK) > + _TIF_UPROBE | _TIF_FSCHECK | \ > + _TIF_SVE_NEEDS_FLUSH) > > #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ > _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 1c6a82083d5c..b0fc8823d731 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -213,6 +213,8 @@ static bool have_cpu_fpsimd_context(void) > */ > static void __sve_free(struct task_struct *task) > { > + /* SVE context will be zeroed when allocated. */ > + clear_tsk_thread_flag(task, TIF_SVE_NEEDS_FLUSH); > kfree(task->thread.sve_state); > task->thread.sve_state = NULL; > } > @@ -269,6 +271,14 @@ static void sve_free(struct task_struct *task) > * * FPSR and FPCR are always stored in task->thread.uw.fpsimd_state > * irrespective of whether TIF_SVE is clear or set, since these are > * not vector length dependent. > + * > + * * When TIF_SVE_NEEDS_FLUSH is set, all the SVE registers but the first > + * 128-bits of the Z-registers are logically zero but not stored anywhere. > + * Saving logically zero bits across context switches is therefore > + * pointless, although they must be zeroed before re-entering userspace. > + * This can be set at the same time as TIF_FPSIMD_FOREIGN_STATE, when it > + * is then the first 128 bits of the SVE registers will be restored from > + * the FPSIMD state. > */ > > /* > @@ -277,18 +287,38 @@ static void sve_free(struct task_struct *task) > * This function should be called only when the FPSIMD/SVE state in > * thread_struct is known to be up to date, when preparing to enter > * userspace. > + * > + * When TIF_SVE_NEEDS_FLUSH is set, the SVE state will be restored from the > + * FPSIMD state. > + * > + * TIF_SVE_NEEDS_FLUSH and TIF_SVE set at the same time should never happen. > + * In the unlikely case it happens, the code is able to cope with it. It will > + * first restore the SVE registers and then flush them in > + * fpsimd_restore_current_state. I find this pretty confusing and, if anything, I'd have expected it to be the other way around: TIF_SVE_NEEDS_FLUSH should only be checked if TIF_SVE is set. Can we leave TIF_SVE set on syscall entry and just check whether we need to flush on return? Having said that, one overall concern I have with this patch is that there is a lot of ad-hoc flag manipulation which feels like a disaster to maintain. Do we really need all 8 states provided by FOREIGN_FPSTATE, SVE and SVE_NEEDS_FLUSH? > */ > static void task_fpsimd_load(void) > { > WARN_ON(!system_supports_fpsimd()); > WARN_ON(!have_cpu_fpsimd_context()); > > - if (system_supports_sve() && test_thread_flag(TIF_SVE)) > - sve_load_state(sve_pffr(¤t->thread), > - ¤t->thread.uw.fpsimd_state.fpsr, > - sve_vq_from_vl(current->thread.sve_vl) - 1); > - else > - fpsimd_load_state(¤t->thread.uw.fpsimd_state); > + /* Ensure that we only evaluate system_supports_sve() once. */ > + if (system_supports_sve()) { I don't understand what the comment is getting at here, or how this code ensure we only evaluate this once. What's the issue? > + if (test_thread_flag(TIF_SVE)) { > + WARN_ON_ONCE(test_thread_flag(TIF_SVE_NEEDS_FLUSH)); > + sve_load_state(sve_pffr(¤t->thread), > + ¤t->thread.uw.fpsimd_state.fpsr, > + sve_vq_from_vl(current->thread.sve_vl) - 1); > + return; > + } else if (test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) { > + WARN_ON_ONCE(test_thread_flag(TIF_SVE)); We already checked TIF_SVE and we know it's false (unless there was a concurrent update, but then this would be racy anyway). > + sve_load_from_fpsimd_state(¤t->thread.uw.fpsimd_state, > + sve_vq_from_vl(current->thread.sve_vl) - 1); > + set_thread_flag(TIF_SVE); > + return; > + } > + } > + > + fpsimd_load_state(¤t->thread.uw.fpsimd_state); > } > > /* > @@ -1159,10 +1189,29 @@ void fpsimd_restore_current_state(void) > get_cpu_fpsimd_context(); > > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > + /* > + * If TIF_SVE_NEEDS_FLUSH is set this takes care of > + * restoring the SVE state that is preserved over > + * syscalls should we have context switched. > + */ > task_fpsimd_load(); > fpsimd_bind_task_to_cpu(); > } > > + if (system_supports_sve() && > + test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) { Why do we need to check system_supports_sve() here? > + /* > + * The userspace had SVE enabled on entry to the kernel > + * and requires the state to be flushed. > + * > + * We rely on the vector length to be set correctly beforehand > + * when converting a loaded FPSIMD state to SVE state. > + */ > + sve_flush_live(); > + sve_user_enable(); > + set_thread_flag(TIF_SVE); > + } > + > put_cpu_fpsimd_context(); > } > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index b63ce4c54cfe..db951c63fc6a 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -369,6 +369,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > */ > dst->thread.sve_state = NULL; > clear_tsk_thread_flag(dst, TIF_SVE); > + clear_tsk_thread_flag(dst, TIF_SVE_NEEDS_FLUSH); > > return 0; > } > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index d8ebfd813e28..2ab7102f5fd7 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -768,6 +768,10 @@ static int sve_get(struct task_struct *target, > > /* Otherwise: full SVE case */ > > + /* The flush should have happened when the thread was stopped */ > + if (test_and_clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH)) > + WARN(1, "TIF_SVE_NEEDS_FLUSH was set"); Given that this adds an atomic operation, I don't think we should be doing this unless it's necessary and it looks like a debug check to me. > BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header)); > start = SVE_PT_SVE_OFFSET; > end = SVE_PT_SVE_FFR_OFFSET(vq) + SVE_PT_SVE_FFR_SIZE(vq); > @@ -830,6 +834,11 @@ static int sve_set(struct task_struct *target, > ret = __fpr_set(target, regset, pos, count, kbuf, ubuf, > SVE_PT_FPSIMD_OFFSET); > clear_tsk_thread_flag(target, TIF_SVE); > + /* > + * If ptrace requested to use FPSIMD, then don't try to > + * re-enable SVE when the task is running again. > + */ I think this comment needs some help. Is "ptrace" the tracer and "the task" the tracee? > + clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH); > goto out; > } > > @@ -854,6 +863,8 @@ static int sve_set(struct task_struct *target, > */ > fpsimd_sync_to_sve(target); > set_tsk_thread_flag(target, TIF_SVE); > + /* Don't flush SVE registers on return as ptrace will update them. */ Same here. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel