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=-6.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,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 2E19EC4363D for ; Tue, 22 Sep 2020 14:05:05 +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 4F58520719 for ; Tue, 22 Sep 2020 14:05:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="DeerUAWN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F58520719 Authentication-Results: mail.kernel.org; dmarc=none (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=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=CVlRPsunmwYEiVxx5mXWLrGgaW031/T+n5MqW2I8I6E=; b=DeerUAWNNyvB6zsArLx16q0Hu 8NmHnckZvwuWNjn1IVC3AbvU/Fqyq2CyRXPoIx0VR+zU5DtsL4CJOG135YbVh86Fcf3TUoDC6vG1c eeg65bA7Kuli798PMI2ieEtzd+ASVhXhgTvWfunq0Z6wfHVzhT+jyBCxXJx93u2y1UK5K0SwGxlZS GSl1eRq/tnuk8havsuU+1ApMMSFa4WqgkHMSF0lSzXmonKPOCJd0chy/bsbpdQxsmSfMGo/d2RGst y9BBwDbgcIuHNy5SPL1Ixsscm2zNPlYwIBmuYXhCxRA7+383SdXGI3f5lDLAoDQwQ8j+rLYr6mKkl Ph0H+3xag==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKitG-0002Tv-0H; Tue, 22 Sep 2020 14:03:30 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKitC-0002TU-U6 for linux-arm-kernel@lists.infradead.org; Tue, 22 Sep 2020 14:03:27 +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 52AEC101E; Tue, 22 Sep 2020 07:03:25 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 36EEB3F718; Tue, 22 Sep 2020 07:03:24 -0700 (PDT) Date: Tue, 22 Sep 2020 15:03:21 +0100 From: Dave Martin To: Mark Brown Subject: Re: [PATCH v4 7/8] arm64/sve: Don't disable SVE on syscalls return Message-ID: <20200922140321.GD6642@arm.com> References: <20200828181155.17745-1-broonie@kernel.org> <20200828181155.17745-8-broonie@kernel.org> <20200921123625.GF2139@willie-the-truck> <20200921180337.GC4792@sirena.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200921180337.GC4792@sirena.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200922_100327_094588_37EC0B09 X-CRM114-Status: GOOD ( 51.69 ) 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 , Will Deacon , 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 Mon, Sep 21, 2020 at 07:03:37PM +0100, Mark Brown wrote: > 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 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? > > 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 > 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. Maybe just try to improve the documentation initially, and see where that gets us? > > > + /* 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. In the original SVE series, I included quite a lot of checks of this sort in order to minimise any overhead for the FPSIMD-only case. I don't know how much of a concern that is here. I guess this simply conforms to the same style. > > > > + /* 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. Seems reasonable. Having checks of this sort has proven quite valuable in the past for debugging, but we don't have to have them compiled in unconditionally. Partly, this serves as a statement that we're making an assumption that has to be enforced elsewhere. [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel