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=-10.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,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 A8EFAC433B4 for ; Wed, 7 Apr 2021 11:47:22 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 466B661394 for ; Wed, 7 Apr 2021 11:47:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 466B661394 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com 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=desiato.20200630; 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=gxKacEg/75l9b9aMG827AQ9ncrC8xmUgnn819SAGit8=; b=bVg3gTI6Fj8+l23mAaar2YPBY OOHWjCXfaeLq04Df9rp49mOQy0E/V73J6b8h2iKlwyihO4uIm2MeAgtgSgeOVIK3/6irTOvZyCCIU YO75zLNaeJFGy25Wy/FwpS1sxnBhuQoGfFJb9c34TRNJ/5umJrNNtQrp229jLZtROXcPMKrewIk7c s37VmWu5Q4mKOkt4xBgxzptLc1eQ7aNG4kU8RDHiA6pyP4aZId+Y14K1yHk3YtJ3Smuw7wmfV4egl mQh+QizpTLZAPLgVeBI1QNgirRvmFXrtaWdEAUn/aMXPJE+S6Eqqk6spTppYe0bSOYAryksDPH+BY vTirP8wqw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lU6ci-004uGA-BR; Wed, 07 Apr 2021 11:45:29 +0000 Received: from mail.kernel.org ([198.145.29.99]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lU6cb-004uEH-I2 for linux-arm-kernel@lists.infradead.org; Wed, 07 Apr 2021 11:45:24 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8DD5761394; Wed, 7 Apr 2021 11:45:14 +0000 (UTC) Date: Wed, 7 Apr 2021 12:45:12 +0100 From: Catalin Marinas To: Mark Brown Cc: Will Deacon , Julien Grall , Zhang Lei , Dave Martin , Daniel Kiss , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm64/sve: Rework SVE access trap to convert state in registers Message-ID: <20210407114511.GA21451@arm.com> References: <20210312190313.24598-1-broonie@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210312190313.24598-1-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-20210407_124522_311723_337E55A9 X-CRM114-Status: GOOD ( 26.36 ) 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 On Fri, Mar 12, 2021 at 07:03:13PM +0000, Mark Brown wrote: > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index bec5f14b622a..ebb263b2d3b1 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -73,6 +73,7 @@ extern void sve_flush_live(void); > extern void sve_load_from_fpsimd_state(struct user_fpsimd_state const *state, > unsigned long vq_minus_1); > extern unsigned int sve_get_vl(void); > +extern void sve_set_vq(unsigned long vq_minus_1); > > struct arm64_cpu_capabilities; > extern void sve_kernel_enable(const struct arm64_cpu_capabilities *__unused); > diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S > index 2ca395c25448..3ecec60d3295 100644 > --- a/arch/arm64/kernel/entry-fpsimd.S > +++ b/arch/arm64/kernel/entry-fpsimd.S > @@ -48,6 +48,11 @@ SYM_FUNC_START(sve_get_vl) > ret > SYM_FUNC_END(sve_get_vl) > > +SYM_FUNC_START(sve_set_vq) > + sve_load_vq x0, x1, x2 > + ret > +SYM_FUNC_END(sve_set_vq) > + > /* > * Load SVE state from FPSIMD state. > * > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 062b21f30f94..0f58e45bd3d1 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -926,9 +926,8 @@ void fpsimd_release_task(struct task_struct *dead_task) > * Trapped SVE access > * > * Storage is allocated for the full SVE state, the current FPSIMD > - * register contents are migrated across, and TIF_SVE is set so that > - * the SVE access trap will be disabled the next time this task > - * reaches ret_to_user. > + * register contents are migrated across, and the access trap is > + * disabled. > * > * TIF_SVE should be clear on entry: otherwise, fpsimd_restore_current_state() > * would have disabled the SVE access trap for userspace during > @@ -946,15 +945,24 @@ void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > get_cpu_fpsimd_context(); > > - fpsimd_save(); > - > - /* Force ret_to_user to reload the registers: */ > - fpsimd_flush_task_state(current); > - > - fpsimd_to_sve(current); > if (test_and_set_thread_flag(TIF_SVE)) > WARN_ON(1); /* SVE access shouldn't have trapped */ > > + /* > + * Convert the FPSIMD state to SVE, zeroing all the state that > + * is not shared with FPSIMD. If (as is likely) the current > + * state is live in the registers then do this there and > + * update our metadata for the current task including > + * disabling the trap, otherwise update our in-memory copy. > + */ > + if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) { > + sve_set_vq(sve_vq_from_vl(current->thread.sve_vl) - 1); > + sve_flush_live(); > + fpsimd_bind_task_to_cpu(); > + } else { > + fpsimd_to_sve(current); > + } > + > put_cpu_fpsimd_context(); > } Just to make sure I understand: in the current code, if TIF_FOREIGN_FPSTATE is cleared and we trapped (TIF_SVE is also cleared), the FPSIMD state is transferred to the SVE one in memory and both TIF_SVE and TIF_FOREIGN_FPSTATE get set. The (potentially stale) SVE state is subsequently restored via do_notify_resume(). I couldn't find where we actually zero the in-memory SVE state or save the latest one via do_el0_svc(). So it looks like we may restore some old SVE state after a syscall (maybe I'm missing something but it would be nice to follow zero or preserved approach). With your patch in the above scenario, we no longer restore the SVE state from memory. We just flush the live SVE registers and disable trapping. The assumption here is that (1) when TIF_SVE is cleared, we no longer care about any of the SVE state (memory or regs) and (2) we only trap if TIF_SVE was cleared (i.e. we don't have a different scenario of lazy restoring where TIF_SVE but we still trap). That's fine by me and matches the code paths but it wasn't entirely clear in the TIF_SVE description higher up in this file. The other case is TIF_FOREIGN_FPSTATE being set in do_sve_acc(). Since we never return to user with this flag set, when can we actually hit the 'else' path in your patch? -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel