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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EB06C77B61 for ; Thu, 13 Apr 2023 12:47:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229708AbjDMMrx (ORCPT ); Thu, 13 Apr 2023 08:47:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51942 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229479AbjDMMrw (ORCPT ); Thu, 13 Apr 2023 08:47:52 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EACE3E46 for ; Thu, 13 Apr 2023 05:47:50 -0700 (PDT) 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 868E760C48 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) 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 Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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.