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 9D221C61DA3 for ; Fri, 24 Feb 2023 11:25:32 +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=5A7XPBn1lChb+C0tMjpZXe/cf9EsL20lTXZDvB82FzU=; b=dyn0DSK1oJGn3q uhFSzdZhXN4zfdFZrP2C+7nA0WBlnYKiwM1l0h4GP7aiw59LF1FxwZjz6gFokZqK6NE5/HEmbqyeH neSKbXnm9lGb/elGeBMUBYsVKoju1wou10rxOPfw7js6kXOZCGBWb3ffk/f1KKBvAqn0INgATBXq1 9wEUKtrLO4CutHKBFMkyLtKJsWGNw9C9fKieYSUcQUngl//egeQVnEMaTUWZGVpjGSqpnlMWss9v2 opw8qH43jpUl5XUdO9xCNy/5Y/SqwobSEZPEwGU01MDhLPR+3ONGMbargXKzSXIZ43puCUT5NsrTD MTJHYyQMhmFDNt2Ruqfg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pVWBj-002FZk-P9; Fri, 24 Feb 2023 11:24:31 +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 1pVWBg-002FYX-7C for linux-arm-kernel@lists.infradead.org; Fri, 24 Feb 2023 11:24:29 +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 B2BDE61828; Fri, 24 Feb 2023 11:24:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 254F0C433D2; Fri, 24 Feb 2023 11:24:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1677237867; bh=POLtIEWemq/X2ktJkENoM0iNTh4ccYZX11buWwrbHTs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=QthxYOP8LE68BGb+1ckD+bAQsV0YSJmv71uqunwEWCtPVBFdawjCl7fRe5QEjNZA9 aUU/KUAYfCbNswNAEwaHUfQRLX3gWbG1TemahUtr2WdgUpgCYH/Lb7Vg8kQjOHl18A ucbt6Zbm/X0xEvizIrmYnm6DJpOJeEIRGiw4ZamUMcUjn+qx0/hcfqN5rBAHRkvitT BbSFS+/doYG1t1U/sbi774n7DUci3jp+L/WimHBU57nAwMXPVb103FHJHg2Gs2nGZt 3WVYo7ogg3m+Tkjm7r7MqdNAq0WaxewpJc6OY/1zPAck0X4nRY5cNSreh6h9s+jCry Kckv3t2hv9uAg== 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 1pVWBc-00CsMI-Jw; Fri, 24 Feb 2023 11:24:24 +0000 Date: Fri, 24 Feb 2023 11:24:24 +0000 Message-ID: <86zg93wbkn.wl-maz@kernel.org> From: Marc Zyngier To: Colton Lewis Cc: 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, dwmw2@infradead.org Subject: Re: [PATCH 08/16] KVM: arm64: timers: Allow userspace to set the counter offsets In-Reply-To: References: <20230216142123.2638675-9-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/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: coltonlewis@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, 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-20230224_032428_356016_B323C8BC X-CRM114-Status: GOOD ( 35.08 ) 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 Thu, 23 Feb 2023 22:41:13 +0000, Colton Lewis wrote: > > Marc Zyngier writes: > > > Once this new API is used, there is no going back, and the counters > > cannot be written to to set the offsets implicitly (the writes > > are instead ignored). > > Why do this? I can't see a reason for disabling the other API the first > time this one is used. I can't see a reason not to. The new API is VM-wide. The old one operates on a per-vcpu basis. What sense does it make to accept something that directly conflicts with the previous actions from userspace? Once userspace has bought into the new API, it should use it consistently. The only reason we don't reject the write with an error is to allow userspace to keep using the vcpu register dump as an opaque object that it doesn't have to scan and amend. > > > In keeping with the architecture, the offsets are expressed as > > a delta that is substracted from the physical counter value. > ^ > nit: subtracted > > > +/* > > + * Counter/Timer offset structure. Describe the virtual/physical offsets. > > + * To be used with KVM_ARM_SET_CNT_OFFSETS. > > + */ > > +struct kvm_arm_counter_offsets { > > + __u64 virtual_offset; > > + __u64 physical_offset; > > + > > +#define KVM_COUNTER_SET_VOFFSET_FLAG (1UL << 0) > > +#define KVM_COUNTER_SET_POFFSET_FLAG (1UL << 1) > > + > > + __u64 flags; > > + __u64 reserved; > > +}; > > + > > It looks weird to have the #defines in the middle of the struct like > that. I think it would be easier to read with the #defines before the > struct. I do like it, as it perfectly shows in which context these #defines are valid. This is also a common idiom used all over the existing KVM code (just take a look at kvm_run for the canonical example). > > > @@ -852,9 +852,11 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > > ptimer->vcpu = vcpu; > > ptimer->offset.vm_offset = &vcpu->kvm->arch.offsets.poffset; > > > - /* Synchronize cntvoff across all vtimers of a VM. */ > > - timer_set_offset(vtimer, kvm_phys_timer_read()); > > - timer_set_offset(ptimer, 0); > > + /* Synchronize offsets across timers of a VM if not already provided */ > > + if (!test_bit(KVM_ARCH_FLAG_COUNTER_OFFSETS, &vcpu->kvm->arch.flags)) { > > + timer_set_offset(vtimer, kvm_phys_timer_read()); > > + timer_set_offset(ptimer, 0); > > + } > > > hrtimer_init(&timer->bg_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD); > > timer->bg_timer.function = kvm_bg_timer_expire; > > The code says "assign the offsets if the KVM_ARCH_FLAG_COUNTER_OFFSETS > flag is not on". The flag name is confusing and made it hard for me to > understand the intent. I think the intent is to only assign the offsets > if the user has not called the API to provide some offsets (that would > have been assigned in the API call along with flipping the flag > on). With that in mind, I would prefer the flag name reference the > user. KVM_ARCH_FLAG_USER_OFFSETS All offsets are provided by the user, no matter what API they used, so I don't think this adds much clarity. The real distinction is between the offsets being set by writing a vcpu attribute or a VM attribute. By this token, I'd suggest KVM_ARM_FLAG_VM_COUNTER_OFFSETS. Thoughts? 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