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 mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id EC898C433EF for ; Mon, 11 Jul 2022 15:53:53 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 636744B7E2; Mon, 11 Jul 2022 11:53:53 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@kernel.org Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9oedTzjUV8i7; Mon, 11 Jul 2022 11:53:51 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E4DE74B947; Mon, 11 Jul 2022 11:53:51 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id C8FAB4B7E2 for ; Mon, 11 Jul 2022 11:53:50 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id aY9GAgxwH7sa for ; Mon, 11 Jul 2022 11:53:49 -0400 (EDT) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 8475C4B655 for ; Mon, 11 Jul 2022 11:53:49 -0400 (EDT) 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 7D89A6104F; Mon, 11 Jul 2022 15:53:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 60709C34115; Mon, 11 Jul 2022 15:53:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657554828; bh=x/+NZrQLovhCePrLFzt1Pdb82Zb5l1wuMFjM9DHfgmY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gnInIpun1pwBAmpjyvYNLaw5+bxpNSn7kzGJlBAlSvmjvF2c4PEagn5JscfL7wJBj KNOJ+Jlk7qHDeeor28oAZo2UWcif0mCjqbzWWN9orsgrefbr+JuW/1zAcwOaH98XTV f2gTWapSrLQraD6qP1NIuqVmEj2PJyCuNFnPhhIJs8IIa4NZV31QF2U1a/2ADMdnti pPFAQFXqsvUDajAzaN4+d2uQf3kDZ0b3X6zRcOn6OJXAsnZiMC6RQWtPbJkkvs9+9B heeYY9ARAaSPQn23wBZ7E9ZI9R30AE+316HIXiklV8k6hGeFtxNmruStuk7HerxCO5 Xg8v8L/SrQ4gQ== Date: Mon, 11 Jul 2022 16:53:43 +0100 From: Mark Brown To: Marc Zyngier Subject: Re: [PATCH v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE Message-ID: References: <20220620124158.482039-1-broonie@kernel.org> <20220620124158.482039-3-broonie@kernel.org> <87ilo4kmvx.wl-maz@kernel.org> <87h73nlnw0.wl-maz@kernel.org> MIME-Version: 1.0 In-Reply-To: <87h73nlnw0.wl-maz@kernel.org> X-Cookie: I am NOMAD! Cc: Catalin Marinas , Zhang Lei , Andre Przywara , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============2511590477898223236==" Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu --===============2511590477898223236== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="WrRSAYoHS7qTuBgz" Content-Disposition: inline --WrRSAYoHS7qTuBgz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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?". Since these callers are already explicitly peering into the data in one form or another (like reading or writing the actual register values, and including for some checking the type information) it seems reasonable for them to also be doing updates to the type selection explicitly. It does also make the error handling a little neater, if we are switching between state types then in the case of error we just leave things using the old, unmodified state. --WrRSAYoHS7qTuBgz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLMR4YACgkQJNaLcl1U h9CaxQf+MF1YRCkg87ipxPvssfDyCksfPEAgFg6Z+wbtMZQjaoxxrGc88Shl6l8f 3regxSfb4AN34gaZ9N4xbDN5hhkDo9aWHZSdvS/tZZpXutGLDyCjUcWDiui+J+q9 42HYQ5+0WXew++6U+hLFLW/oHSZfAOX+uWl66O9FaDOVD8jRXh/HlVt6A+0QBSeR QGY7eBNNayhrCVUyoKHn3APiyYxWvPDI92ku+2zxoM8UjPVkzkGUn4x6zG9VvUUX 7AnYkx/BlmbCR4maEjHX6Y2reTzL4Q5nYCyuOaAYPzZfBgiQl+LRrnGs528MeBlU GdADUJ6KhrAi4gP0PRnYMzIKzlBh1A== =1Z/E -----END PGP SIGNATURE----- --WrRSAYoHS7qTuBgz-- --===============2511590477898223236== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm --===============2511590477898223236==-- 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 660EDC433EF for ; Mon, 11 Jul 2022 15:55:09 +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-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-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=brwjV/sPnEfL3HdOxyjzQ7v08J8Zml9e/3SXzL2ovSM=; b=UWXUPIzq4UdzNk6WFgl19m+qsJ Fayk11OiGN3Ii9GbkAg8NlCtbkN3Sf4AtQcjvPo5266x7BXnwquMd8g25m3aVxK321vKuWfA377GF 6nadRQVhH85tgvn3KlziDyWCrxSxlkFG3rfa4NOkkc+QSY6s1+XmJtrk0Tj5xKLn+p5ZbPqg8AOJf BzNGM6xSa11l721+DKmPZn5RJxOIBOQwg8GnTBMBLVnG4TNrYd4gG470w7fXBH4FRpxUeEyKSDfHs jCXTophF8XWMdEuoxVi2RK1IiE0ggX6fz1Kix00xRz6Apb96g9NlFhPtsIiZR9j4yTnIgDXPsaErC ezkMY94A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oAvjQ-0031M2-Gc; Mon, 11 Jul 2022 15:53:56 +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 1oAvjM-0031KL-SX for linux-arm-kernel@lists.infradead.org; Mon, 11 Jul 2022 15:53:54 +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 7D89A6104F; Mon, 11 Jul 2022 15:53:48 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 60709C34115; Mon, 11 Jul 2022 15:53:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657554828; bh=x/+NZrQLovhCePrLFzt1Pdb82Zb5l1wuMFjM9DHfgmY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gnInIpun1pwBAmpjyvYNLaw5+bxpNSn7kzGJlBAlSvmjvF2c4PEagn5JscfL7wJBj KNOJ+Jlk7qHDeeor28oAZo2UWcif0mCjqbzWWN9orsgrefbr+JuW/1zAcwOaH98XTV f2gTWapSrLQraD6qP1NIuqVmEj2PJyCuNFnPhhIJs8IIa4NZV31QF2U1a/2ADMdnti pPFAQFXqsvUDajAzaN4+d2uQf3kDZ0b3X6zRcOn6OJXAsnZiMC6RQWtPbJkkvs9+9B heeYY9ARAaSPQn23wBZ7E9ZI9R30AE+316HIXiklV8k6hGeFtxNmruStuk7HerxCO5 Xg8v8L/SrQ4gQ== Date: Mon, 11 Jul 2022 16:53:43 +0100 From: Mark Brown To: Marc Zyngier 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 Message-ID: References: <20220620124158.482039-1-broonie@kernel.org> <20220620124158.482039-3-broonie@kernel.org> <87ilo4kmvx.wl-maz@kernel.org> <87h73nlnw0.wl-maz@kernel.org> MIME-Version: 1.0 In-Reply-To: <87h73nlnw0.wl-maz@kernel.org> X-Cookie: I am NOMAD! X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220711_085353_020640_12A3A46F X-CRM114-Status: GOOD ( 34.57 ) 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: multipart/mixed; boundary="===============3017977442781478630==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============3017977442781478630== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="WrRSAYoHS7qTuBgz" Content-Disposition: inline --WrRSAYoHS7qTuBgz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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?". Since these callers are already explicitly peering into the data in one form or another (like reading or writing the actual register values, and including for some checking the type information) it seems reasonable for them to also be doing updates to the type selection explicitly. It does also make the error handling a little neater, if we are switching between state types then in the case of error we just leave things using the old, unmodified state. --WrRSAYoHS7qTuBgz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLMR4YACgkQJNaLcl1U h9CaxQf+MF1YRCkg87ipxPvssfDyCksfPEAgFg6Z+wbtMZQjaoxxrGc88Shl6l8f 3regxSfb4AN34gaZ9N4xbDN5hhkDo9aWHZSdvS/tZZpXutGLDyCjUcWDiui+J+q9 42HYQ5+0WXew++6U+hLFLW/oHSZfAOX+uWl66O9FaDOVD8jRXh/HlVt6A+0QBSeR QGY7eBNNayhrCVUyoKHn3APiyYxWvPDI92ku+2zxoM8UjPVkzkGUn4x6zG9VvUUX 7AnYkx/BlmbCR4maEjHX6Y2reTzL4Q5nYCyuOaAYPzZfBgiQl+LRrnGs528MeBlU GdADUJ6KhrAi4gP0PRnYMzIKzlBh1A== =1Z/E -----END PGP SIGNATURE----- --WrRSAYoHS7qTuBgz-- --===============3017977442781478630== 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 --===============3017977442781478630==--