From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 482552F23 for ; Thu, 13 Apr 2023 12:47:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E407EC433D2; Thu, 13 Apr 2023 12:47:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681390069; bh=lYjq1mhm2KORM73zusnPCOsUTVos5st/Qc5aIpzBhas=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DRDNznaiCFTKJa2uiV9LQZZpUa/MHjZjxsD338y29ZGw5RLimFmhVztKVTdS9q15t IoiLPVDVKgJiEko2I4fzivMQF8NYFgSUIq85a/7Vn8tnK801zmehR/NspysLHr8xpT SVtcRc1sastFusHb9rcNBdEul0lKysIwBY80ITCEAK9f0b7pqM/RTpnb9a8dmfGFMQ DT//tnmf9kXuer0qcZsh2bvDGPx4aIyYYerv286thq73Tb8lNwbmER2W9HWDAwSbSW mXi8o8Savm7V+L4AdxAoLSm8KH40cH4XOzdh5jblVMdt/vYHNEIfUwBTqwhnADeV4V S2MTaIdS/gMfw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.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 1pmwMd-0086cc-K7; Thu, 13 Apr 2023 13:47:47 +0100 Date: Thu, 13 Apr 2023 13:47:47 +0100 Message-ID: <86h6tkkky4.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Ricardo Koller , Simon Veith , Colton Lewis , Joey Gouly , dwmw2@infradead.org Subject: Re: [PATCH v4 05/20] KVM: arm64: timers: Allow physical offset without CNTPOFF_EL2 In-Reply-To: <20230410153441.vddskgxu2zzsi7bq@google.com> References: <20230330174800.2677007-1-maz@kernel.org> <20230330174800.2677007-6-maz@kernel.org> <20230410153441.vddskgxu2zzsi7bq@google.com> 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/28.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: reijiw@google.com, kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, ricarkol@google.com, sveith@amazon.de, coltonlewis@google.com, joey.gouly@arm.com, dwmw2@infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Mon, 10 Apr 2023 16:34:41 +0100, Reiji Watanabe wrote: > > Hi Marc, > > On Thu, Mar 30, 2023 at 06:47:45PM +0100, Marc Zyngier wrote: [...] > > + assign_clear_set_bit(tpt, CNTHCTL_EL1PCEN << 10, set, clr); > > + assign_clear_set_bit(tpc, CNTHCTL_EL1PCTEN << 10, set, clr); > > Nit: IMHO the way the code specifies the 'set' and 'clr' arguments for > the macro might be a bit confusing ('set' is for '_clr', and 'clr' is > for '_set')? I don't disagree, but we end-up with bits of different polarity once NV is fully in, see: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/arch/arm64/kvm/arch_timer.c?h=kvm-arm64/nv-6.4-WIP#n861 > Perhaps changing the parameter names of assign_clear_set_bit() like > below or flipping the condition (i.e. Specify !tpt or no_tpt instead > of tpt) might be less confusing? > > #define assign_clear_set_bit(_pred, _bit, _t_val, _f_val) \ > do { \ > if (_pred) \ > (_t_val) |= (_bit); \ > else \ > (_f_val) |= (_bit); \ > } while (0) See the pointer above. We need a good way to specify bits that have one polarity or another, and compute the result given the high-level constraints that are provided by the emulation code. So far, I haven't been able to work out something "nice". [...] > > + { SYS_DESC(SYS_CNTPCT_EL0), access_arch_timer }, > > + { SYS_DESC(SYS_CNTPCTSS_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_TVAL_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_CTL_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_CVAL_EL0), access_arch_timer }, > > @@ -2525,6 +2533,7 @@ static const struct sys_reg_desc cp15_64_regs[] = { > > { Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR0_EL1 }, > > { CP15_PMU_SYS_REG(DIRECT, 0, 0, 9, 0), .access = access_pmu_evcntr }, > > { Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */ > > + { SYS_DESC(SYS_AARCH32_CNTPCT), access_arch_timer }, > > Shouldn't KVM also emulate CNTPCTSS (Aarch32) when its trapping is > enabled on the host with ECV_CNTPOFF ? Oh, well spotted. I'll queue something on top of the series to that effect (I'd rather not respin it as it has been in -next for some time, and the merge window is approaching). > > > > { Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR1_EL1 }, > > { Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */ > > { Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */ > > -- > > 2.34.1 > > > > > > Nit: In emulating reading physical counter/timer for direct_ptimer > (poffset != 0 on VHE without ECV_CNTPOFF), it appears that > kvm_arm_timer_read_sysreg() unnecessarily calls > timer_{save,restore}_state(), and kvm_arm_timer_write_sysreg() > unnecessarily calls timer_save_state(). Couldn't we skip those > unnecessary calls ? (I didn't check all the following patches, but > the current code would be more convenient in the following patches ?) Well, it depends how you look at it. We still perform "some" level of emulation (such as offsetting CVAL), and it allows us to share some code with the full emulation. On top of that, we already fast-track CNTPCT_EL0, which is the main user, and has a visible benefit with NV. If anything, I'd rather add a similar fast-tracking for the read side of CNTP_CVAL_EL0 and CNTP_CTL_EL0. We could then leave that code for 32bit only, which nobody gives a toss about. What do you think? Thanks, M. -- Without deviation from the norm, progress is not possible. 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 07AA5C77B6E for ; Thu, 13 Apr 2023 12:48:46 +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=ULxabjvW96IdXRYtANIDJx5/FcNHYnrecVnaHtx9yCs=; b=ryN0Cqdsval7BN i4D9OLsxBZKoJXQAfRdtX9pyzK1LVXmHZzf/ORgghPuIb82gx2qzD5DkxtwpfWoXCvHOM2YhkstG9 wmxS2gJQBd7OCWV+CNxY0UP2jPsEJ4k3IVlP6MAp7tQmyPunvBLM8JfeBfkHZWFmpKAyLbxFay3fH U9SSmslBqOhIyaC7aTVkSE7u/Ue39KqXcB7/HJFRvPS585NA9ZK9xONk9QTGSIIeehqLE6dFgP5aY eMZgCj3wiJlO0KdloCwEHaj6+eOfKOrLZDzDs3ER7PJMipx5BfBB3Tz0vtGoaqZ0zkfiJBH+3EWGL 4kbxmig7pfzlc3Y6/AIw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pmwMk-006AEK-2e; Thu, 13 Apr 2023 12:47:54 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pmwMh-006ADO-0C for linux-arm-kernel@lists.infradead.org; Thu, 13 Apr 2023 12:47:52 +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 91AAA6118F; Thu, 13 Apr 2023 12:47:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E407EC433D2; Thu, 13 Apr 2023 12:47:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681390069; bh=lYjq1mhm2KORM73zusnPCOsUTVos5st/Qc5aIpzBhas=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DRDNznaiCFTKJa2uiV9LQZZpUa/MHjZjxsD338y29ZGw5RLimFmhVztKVTdS9q15t IoiLPVDVKgJiEko2I4fzivMQF8NYFgSUIq85a/7Vn8tnK801zmehR/NspysLHr8xpT SVtcRc1sastFusHb9rcNBdEul0lKysIwBY80ITCEAK9f0b7pqM/RTpnb9a8dmfGFMQ DT//tnmf9kXuer0qcZsh2bvDGPx4aIyYYerv286thq73Tb8lNwbmER2W9HWDAwSbSW mXi8o8Savm7V+L4AdxAoLSm8KH40cH4XOzdh5jblVMdt/vYHNEIfUwBTqwhnADeV4V S2MTaIdS/gMfw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.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 1pmwMd-0086cc-K7; Thu, 13 Apr 2023 13:47:47 +0100 Date: Thu, 13 Apr 2023 13:47:47 +0100 Message-ID: <86h6tkkky4.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Ricardo Koller , Simon Veith , Colton Lewis , Joey Gouly , dwmw2@infradead.org Subject: Re: [PATCH v4 05/20] KVM: arm64: timers: Allow physical offset without CNTPOFF_EL2 In-Reply-To: <20230410153441.vddskgxu2zzsi7bq@google.com> References: <20230330174800.2677007-1-maz@kernel.org> <20230330174800.2677007-6-maz@kernel.org> <20230410153441.vddskgxu2zzsi7bq@google.com> 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/28.2 (aarch64-unknown-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: reijiw@google.com, kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, ricarkol@google.com, sveith@amazon.de, coltonlewis@google.com, joey.gouly@arm.com, dwmw2@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-20230413_054751_180041_9AB6128D X-CRM114-Status: GOOD ( 28.26 ) 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, 10 Apr 2023 16:34:41 +0100, Reiji Watanabe wrote: > > Hi Marc, > > On Thu, Mar 30, 2023 at 06:47:45PM +0100, Marc Zyngier wrote: [...] > > + assign_clear_set_bit(tpt, CNTHCTL_EL1PCEN << 10, set, clr); > > + assign_clear_set_bit(tpc, CNTHCTL_EL1PCTEN << 10, set, clr); > > Nit: IMHO the way the code specifies the 'set' and 'clr' arguments for > the macro might be a bit confusing ('set' is for '_clr', and 'clr' is > for '_set')? I don't disagree, but we end-up with bits of different polarity once NV is fully in, see: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/arch/arm64/kvm/arch_timer.c?h=kvm-arm64/nv-6.4-WIP#n861 > Perhaps changing the parameter names of assign_clear_set_bit() like > below or flipping the condition (i.e. Specify !tpt or no_tpt instead > of tpt) might be less confusing? > > #define assign_clear_set_bit(_pred, _bit, _t_val, _f_val) \ > do { \ > if (_pred) \ > (_t_val) |= (_bit); \ > else \ > (_f_val) |= (_bit); \ > } while (0) See the pointer above. We need a good way to specify bits that have one polarity or another, and compute the result given the high-level constraints that are provided by the emulation code. So far, I haven't been able to work out something "nice". [...] > > + { SYS_DESC(SYS_CNTPCT_EL0), access_arch_timer }, > > + { SYS_DESC(SYS_CNTPCTSS_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_TVAL_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_CTL_EL0), access_arch_timer }, > > { SYS_DESC(SYS_CNTP_CVAL_EL0), access_arch_timer }, > > @@ -2525,6 +2533,7 @@ static const struct sys_reg_desc cp15_64_regs[] = { > > { Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR0_EL1 }, > > { CP15_PMU_SYS_REG(DIRECT, 0, 0, 9, 0), .access = access_pmu_evcntr }, > > { Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */ > > + { SYS_DESC(SYS_AARCH32_CNTPCT), access_arch_timer }, > > Shouldn't KVM also emulate CNTPCTSS (Aarch32) when its trapping is > enabled on the host with ECV_CNTPOFF ? Oh, well spotted. I'll queue something on top of the series to that effect (I'd rather not respin it as it has been in -next for some time, and the merge window is approaching). > > > > { Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR1_EL1 }, > > { Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */ > > { Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */ > > -- > > 2.34.1 > > > > > > Nit: In emulating reading physical counter/timer for direct_ptimer > (poffset != 0 on VHE without ECV_CNTPOFF), it appears that > kvm_arm_timer_read_sysreg() unnecessarily calls > timer_{save,restore}_state(), and kvm_arm_timer_write_sysreg() > unnecessarily calls timer_save_state(). Couldn't we skip those > unnecessary calls ? (I didn't check all the following patches, but > the current code would be more convenient in the following patches ?) Well, it depends how you look at it. We still perform "some" level of emulation (such as offsetting CVAL), and it allows us to share some code with the full emulation. On top of that, we already fast-track CNTPCT_EL0, which is the main user, and has a visible benefit with NV. If anything, I'd rather add a similar fast-tracking for the read side of CNTP_CVAL_EL0 and CNTP_CTL_EL0. We could then leave that code for 32bit only, which nobody gives a toss about. What do you think? Thanks, 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