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=-7.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 AED5FC4363A for ; Mon, 21 Sep 2020 18:05:56 +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 42CCE206FB for ; Mon, 21 Sep 2020 18:05:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="29jd7ML6"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="a02AO5uL" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 42CCE206FB 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-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-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ijdqMUM+OWT57TcDYKWnsYAEWpvd9T5WoCXX3a2rka4=; b=29jd7ML6QuNWMA9z6+9RvhwNM o0yQ6vRjsrAryWOSRnOeqVwQ5NFJ49cBMFqkvKTfQrDZ8QN16MjuCyo6Q6V1o3Ax6cmqxK/KrRGkf HQ12wsBKt5n6m4tToejLPNDaGTeZHKB1Gj45nSPxqgI56H0YqBk5UTQgmi6Egt9AIUkimTZ5L/q8o d/hJeWQC1cMROVer3qAqygR1d0/1mZdR9BbRfKWDbdM77EAsoa+qJ2Y/1RN4d3w0m0n0Lwk2LfWad tJIgNLu9OS3d6jexPjzb9etbnsli+Fu0fZSjnQUA2MLnnFP39NbDgCyy9iQhbO9cvzn5AsI+mpJmf Lqp/CSfQw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKQAz-0006m8-LX; Mon, 21 Sep 2020 18:04:33 +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 1kKQAw-0006l8-N1 for linux-arm-kernel@lists.infradead.org; Mon, 21 Sep 2020 18:04:31 +0000 Received: from localhost (fw-tnat.cambridge.arm.com [217.140.96.140]) (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 667872071A; Mon, 21 Sep 2020 18:04:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600711469; bh=zgBAnQTEP6sxfCNaAUFByVf1oQz0+TelqaLy7vHJ54A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=a02AO5uLCPUaRZsQEKo4cx5a9NwcVamLNwdQpdvS+FolI44TzbAS8i0/eEQgks7ik w2EQFSmoa2ES2/2Q2m/0daqWa/HFPJpZdRkChPF+fi7TNdOWKf9rSq5gFxeq6wkecR 4W+k9PyxPq0OZTeB2whNiual4TknNp1oje96qLkM= Date: Mon, 21 Sep 2020 19:03:37 +0100 From: Mark Brown To: Will Deacon Subject: Re: [PATCH v4 7/8] arm64/sve: Don't disable SVE on syscalls return Message-ID: <20200921180337.GC4792@sirena.org.uk> References: <20200828181155.17745-1-broonie@kernel.org> <20200828181155.17745-8-broonie@kernel.org> <20200921123625.GF2139@willie-the-truck> MIME-Version: 1.0 In-Reply-To: <20200921123625.GF2139@willie-the-truck> X-Cookie: Love thy neighbor, tune thy piano. 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_140430_888400_F65F4A6D X-CRM114-Status: GOOD ( 42.44 ) 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: multipart/mixed; boundary="===============8836323000706040071==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============8836323000706040071== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="XWOWbaMNXpFDWE00" Content-Disposition: inline --XWOWbaMNXpFDWE00 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 21, 2020 at 01:36:27PM +0100, Will Deacon wrote: > On Fri, Aug 28, 2020 at 07:11:54PM +0100, Mark Brown wrote: > > + * TIF_SVE_NEEDS_FLUSH and TIF_SVE set at the same time should never h= appen. > > + * 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_S= VE > is set. Can we leave TIF_SVE set on syscall entry and just check whether > we need to flush on return? I'll have a think about this, it'd been a while and I'll need to page this in again if I ever thought about that bit thoroughly, the separation already existed when I took over the code and I didn't see it raised as a concern on the first round of review which I had hoped had covered the big picture stuff. > 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? I think it's clear that we need all three flags, they're all orthogonal=20 enough - FOREIGN_FPSTATE says if the hardware state needs syncing and applies to both FPSIMD and SVE, SVE says if the task should execute SVE instructions without trapping and SVE_NEEDS_FLUSH says where to restore the SVE state from. I'm not really seeing any redundancy that we can eliminate. Having spent time looking at this the main issue and the main thing is to look at allowing TIF_SVE and TIF_SVE_NEEDS_FLUSH to be simultaneously set as you suggested above - the interaction with TIF_FOREIGN_FPSTATE is fairly simple so I don't see a big problem there. > > + /* 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? There was a performance concern raised about doing the capabilities check more than once in the same function, this was a refactor to pull the system_supports_sve() check out which looks a bit more fiddly. Previously there were two blocks which checked for system_supports_sve() and tested flags separately. > > + } 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). Yes, this was just about having a check on use as had been requested ince we're adding a check rather than the check actually being useful, it's easier to verify that the checks are all there. > > + if (system_supports_sve() && > > + test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) { > Why do we need to check system_supports_sve() here? I don't think we do, it's been there since the initial version and looked like a stylistic thing. I'll just drop it since it's a bit late to validate at this point anyway and if there were a situation where we had the flag but not the hardware we probably ought to be screaming about it rather than silently ignoring it as the code currently does. > > + /* 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. Yes, one of the previous review suggestions was to add such checks. I can either remove it again or put it behind a define/config option that enables debug checks like this - as you say this code is a bit complex so I do think there's value in having extra checks of things we know should be true in there as an option to help with validation. > > + /* > > + * 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 tas= k" > the tracee? Yes. --XWOWbaMNXpFDWE00 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl9o6vgACgkQJNaLcl1U h9B/twf+M/lpfFGywkhCLipRXwvnqtmvf2KhUnSoo/vc+3tfMXHsTB4KAYmUy1UD PY0yyi0RpjWPuyaAb9oFpAkKV1fTeHActmQrcNTFFvB74tl0mIStt9oLrhguMaA3 xBajYdJkiWhGvUJJDagsLnKGhs5U4zY5olYCqZ2iISiTZu12gotniDmnD8JomrBI AdUkpAieX8HllXOKu5QTMEUlWXV17Y0J8BLKQ90jC5PsJ3bPr/DDCz5/Zsi1ufgQ ZAE4rCf3jzynuKk6h6AuGjk7nI5k0Z6vtHGpwgPJldeHL+9rkl0uZPVs6GAmOmyt sX+bSh2rG8+gPjEEu0FK7gMytILvfA== =kHre -----END PGP SIGNATURE----- --XWOWbaMNXpFDWE00-- --===============8836323000706040071== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============8836323000706040071==--