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 68CDCC43334 for ; Wed, 20 Jul 2022 09:41:07 +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:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=5v1MTpyqwk4p/58Lz68chcZVUEaGW4oRGPBbPMElFhU=; b=Y345yiAHO+1H5a LFFv/xszy++aZ+TryzSXGwsZuRjj4Jx5pu2UieUUi4r850++zshna/4r6S7h+r7P4EktRnBYAwjF0 fcV2ILf4uFZ3veZWV0zdk1eBeRHo13D+lGEC2zgYyoj8lNLv+8lzcUUfucilrvk2DC9TCNS5X8k+f qQEOvjvB8lqnxjSeE9baMdvyg+njBkG59VSi9VNChl16Ynn89Tqja5Nc/7WQC4jaE9lv5nl+g+e6a /JuWTRCn7LKTJG0pFh+DE0WihUsSfSVirrmYbnxwzs1CkbrKwr1rVu+HMUSUaWkgcwjXZ5Z1UlicZ LBmalaOgFfe0v+V1K1Kg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oE6Bd-003Pme-La; Wed, 20 Jul 2022 09:40:09 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oE6Ba-003PkJ-Ll for linux-arm-kernel@lists.infradead.org; Wed, 20 Jul 2022 09:40:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 3E6806126A; Wed, 20 Jul 2022 09:40:06 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A128EC3411E; Wed, 20 Jul 2022 09:40:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1658310005; bh=uYi+PMhnThD8EyyCoIPSbOJSDFHGdViWegxE4PAv+8Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=JnINEN9w9SZScAT4cLV6//dnnokhYjdlMiskXwfjm6l0RSn/PC5g46V84GohO/WlF b6bocDgpqAI3RuQU/jRn4BRTaJDLR5gIzj4PnE885nmFb/sSTujjblSMnx6aA+ssGx hLr4fk4ATnGmvQ2iylxJjq1IKNPHnqQ2vuUsIqiUC2HkY8JaViYfX9fXDrPLSSIls4 p45VHi6VF8D0T5ndaxTQi6ONgTTm0hgAaZXp/JUtaTHUn/uRwgmrmGoI8lZXB6S3Lk QRRSs2g/3G7kxPjDqR9tmvQ5fVpqO4mbjO/A3L7Xj2LvlKXAWexciuaQFw7TwXdp0I zc6lEtZUW88bQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oE6BX-008kAo-Ey; Wed, 20 Jul 2022 10:40:03 +0100 Date: Wed, 20 Jul 2022 10:40:03 +0100 Message-ID: <877d48yvf0.wl-maz@kernel.org> From: Marc Zyngier To: Mark Brown Cc: Catalin Marinas , Will Deacon , Zhang Lei , James Morse , Alexandru Elisei , Andre Przywara , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE In-Reply-To: References: <20220620124158.482039-1-broonie@kernel.org> <20220620124158.482039-3-broonie@kernel.org> <87ilo4kmvx.wl-maz@kernel.org> <87h73nlnw0.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: broonie@kernel.org, catalin.marinas@arm.com, will@kernel.org, zhang.lei@jp.fujitsu.com, james.morse@arm.com, alexandru.elisei@arm.com, andre.przywara@arm.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220720_024006_806860_9125B90A X-CRM114-Status: GOOD ( 42.78 ) 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 Mon, 11 Jul 2022 16:53:43 +0100, Mark Brown wrote: > > On Mon, Jul 11, 2022 at 03:33:51PM +0100, Marc Zyngier wrote: > > Mark Brown wrote: > > > On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote: > > > > Mark Brown wrote: > > > > > > + enum fp_state *type; > > > > > For consistency: s/type/fp_type/ ? > > > > Sure if nobody else wants a different bikeshed. It really needs a > > > longer name like fp_state_t or something but that had it's own problems > > > with non-idiomaticness. > > > I'm not talking about the name of the type, but about the name of the > > member in the struct fpsimd_last_state_struct. I'd like it to be > > homogeneous to the name you use in struct kvm_vcpu_arch. 'type' is way > > Ah, sure I can do that. I had thought this being in the FP last state > structure made things clear here. > > > > > > - thread_sm_enabled(&task->thread)) > > > > > + thread_sm_enabled(&task->thread)) { > > > > > sve_to_fpsimd(task); > > > > > + task->thread.fp_type = FP_STATE_FPSIMD; > > > > > Can you move this assignment into the sve_to_fpsimd() helper? > > > > There are cases where we want a FPSIMD version of the state for > > > reading but don't want to affect the actual state of the process > > > (eg, if someone reads the FPSIMD registers via ptrace) so we don't > > > want to change the active register state just because we converted > > > it. Adding another API that does the convert and update didn't feel > > > like it was helping since you then have to remember which API does > > > what and we already have lots of similarly named functions for > > > slightly different contexts. > > > I still think the state conversion should be self contained. > > Sprinkling this context tracking is bound to end-up with a bug, while > > documenting what is to be used when, or with a helper named > > explicitly enough ("extract_fp_from_sve()" springs to mind) for > > ptrace. > > My experience trying to follow and update this code has been that > layering on more helpers just shifts the potential for bugs around - > it's easy to have the calling context using the wrong helper and looking > correct, or to spend time cross checking if the helper in a particular > context is the right one. Sometimes this happens because something > about the calling context changed rather than due to writing a new use. > Yes, someone might forget to update the state type but my experience > with this code is that it's a lot easier to spot "this is writing new > state, did it update the state type?" than "this is writing new state, > did it call the helper that implicitly updates the state type or the > other one?". My experience in maintaining the KVM code is that the least state leaks outside of this sort of helpers, the least problematic they are. I'd rather have multiple helpers that have different *documented* behaviours than expecting the random hacker to know (or in this case, *guess*) when or not to add some extra state-twiddling. It also makes the code far more readable because it is self-contained. If this series is supposed to help making things more maintainable, then this is one way to do it. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel