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 0F200C6379F for ; Wed, 22 Feb 2023 10:55:58 +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=uQtXtg0iUCfYGVC94eMgplYsg4q4AdvUH93EiidDbd4=; b=2UMVU/clvaN/Nt RvskQp9Yx/ptdPBXN9j/KfKY+0TbhhW4FOnLqQ1VrnAI3ZUpQMe9ImlZZwnHDxDr2pRUjJyTYlLxS ay0POlNdA3A/agYBTpKVeVsrCA3O7Z5ZJeY0NVcyRYV1ic2W7Y2u5Dx1IS4SUAV98Ri5Sh1sTnCKd hjA5Za5ltRkoNU3yXV85ydOl2xwH9EJzQ/s71d8YobQ2QOBoTyHdOZWPoBxEC7v2I2srmRKhdzxNq LTtgWyRBJShGG1tWSLxSS13VZ4wbuiAof3CHliMPpOkmbs08/B50APnp42GG8MzBZ9fD/Lp52JgWk wvmc8RdaDgL9xjIuQtDg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pUmlz-00C5bA-28; Wed, 22 Feb 2023 10:54:55 +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 1pUmlu-00C5Zi-43 for linux-arm-kernel@lists.infradead.org; Wed, 22 Feb 2023 10:54:53 +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 6EDF661309; Wed, 22 Feb 2023 10:54:49 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BFE7CC433EF; Wed, 22 Feb 2023 10:54:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1677063288; bh=OB4ejOBC6EggM7a8fScuh+tleH59xEENrIJiBE/NI7g=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=B1hOw2J2oh2RCBDgQEOjEmjPHHr3lNveump1fiJ3+mbxQobLJYTAokdpJtIqXZ2FK Xa1f2bG3e99dez9CzHLq/tePO+ax3UhSZ0lvQ1TzAzzLS5n1MG0P1kwWq5uKfK9xY7 pMsFeXEyvOsWV4RYxWDAmCOXwH7uvrj/PahKd9UUAsYGLXhKFG7HcBkM8g0CItsLnk QQTM8EPbIE84kXRsllKd+w/vNJJFx64Ddo9taHLiWSRHoK6JqIL8YQRO2896DuLguG Ov4oRoqfkOWXI/5CZdFQpyZjHACCgIZjmAyuteoP+IXleffI9Xv4OOeDuEn8/M3V6P 1FBlguQ+saA8Q== 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 1pUmlq-00CKfG-BZ; Wed, 22 Feb 2023 10:54:46 +0000 Date: Wed, 22 Feb 2023 10:54:46 +0000 Message-ID: <86cz62x955.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 , dwmw2@infradead.org Subject: Re: [PATCH 05/16] KVM: arm64: timers: Convert per-vcpu virtual offset to a global value In-Reply-To: References: <20230216142123.2638675-1-maz@kernel.org> <20230216142123.2638675-6-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: 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, 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-20230222_025450_267955_138387A9 X-CRM114-Status: GOOD ( 42.46 ) 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 Wed, 22 Feb 2023 06:15:56 +0000, Reiji Watanabe wrote: > > Hi Marc, > > On Thu, Feb 16, 2023 at 6:21 AM Marc Zyngier wrote: > > > > Having a per-vcpu virtual offset is a pain. It needs to be synchronized > > on each update, and expands badly to a setup where different timers can > > have different offsets, or have composite offsets (as with NV). > > > > So let's start by replacing the use of the CNTVOFF_EL2 shadow register > > (which we want to reclaim for NV anyway), and make the virtual timer > > carry a pointer to a VM-wide offset. > > That's nice! > > > > > This simplifies the code significantly. > > > > Signed-off-by: Marc Zyngier > > --- > > arch/arm64/include/asm/kvm_host.h | 3 +++ > > arch/arm64/kvm/arch_timer.c | 45 +++++++------------------------ > > arch/arm64/kvm/hypercalls.c | 2 +- > > include/kvm/arm_arch_timer.h | 15 +++++++++++ > > 4 files changed, 29 insertions(+), 36 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 2a8175f38439..3adac0c5e175 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -193,6 +193,9 @@ struct kvm_arch { > > /* Interrupt controller */ > > struct vgic_dist vgic; > > > > + /* Timers */ > > + struct arch_timer_vm_offsets offsets; > > Nit: Perhaps using a more explicit name for the 'offsets' field might > be better than simply 'offsets', since it is a field of kvm_arch (not > a field of arch timer related struct). > (e.g. timer_offsets ?) Good point. Actually, maybe this should be a more generic structure (arch_timer_global, or something similar), containing the offset(s). I'll have a think. > > > + > > /* Mandated version of PSCI */ > > u32 psci_version; > > > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > > index 329a8444724f..1bb44f668608 100644 > > --- a/arch/arm64/kvm/arch_timer.c > > +++ b/arch/arm64/kvm/arch_timer.c > > @@ -84,14 +84,10 @@ u64 timer_get_cval(struct arch_timer_context *ctxt) > > > > static u64 timer_get_offset(struct arch_timer_context *ctxt) > > { > > - struct kvm_vcpu *vcpu = ctxt->vcpu; > > + if (ctxt->offset.vm_offset) > > + return *ctxt->offset.vm_offset; > > > > - switch(arch_timer_ctx_index(ctxt)) { > > - case TIMER_VTIMER: > > - return __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > > - default: > > - return 0; > > - } > > + return 0; > > } > > > > static void timer_set_ctl(struct arch_timer_context *ctxt, u32 ctl) > > @@ -128,15 +124,12 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval) > > > > static void timer_set_offset(struct arch_timer_context *ctxt, u64 offset) > > { > > - struct kvm_vcpu *vcpu = ctxt->vcpu; > > - > > - switch(arch_timer_ctx_index(ctxt)) { > > - case TIMER_VTIMER: > > - __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > > - break; > > - default: > > + if (!ctxt->offset.vm_offset) { > > WARN(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > > + return; > > } > > + > > + WRITE_ONCE(*ctxt->offset.vm_offset, offset); > > } > > > > u64 kvm_phys_timer_read(void) > > @@ -765,25 +758,6 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu) > > return 0; > > } > > > > -/* Make the updates of cntvoff for all vtimer contexts atomic */ > > -static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > > -{ > > - unsigned long i; > > - struct kvm *kvm = vcpu->kvm; > > - struct kvm_vcpu *tmp; > > - > > - mutex_lock(&kvm->lock); > > - kvm_for_each_vcpu(i, tmp, kvm) > > - timer_set_offset(vcpu_vtimer(tmp), cntvoff); > > - > > - /* > > - * When called from the vcpu create path, the CPU being created is not > > - * included in the loop above, so we just set it here as well. > > - */ > > - timer_set_offset(vcpu_vtimer(vcpu), cntvoff); > > - mutex_unlock(&kvm->lock); > > -} > > - > > void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > > { > > struct arch_timer_cpu *timer = vcpu_timer(vcpu); > > @@ -791,10 +765,11 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > > struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); > > > > vtimer->vcpu = vcpu; > > + vtimer->offset.vm_offset = &vcpu->kvm->arch.offsets.voffset; > > ptimer->vcpu = vcpu; > > > > /* Synchronize cntvoff across all vtimers of a VM. */ > > - update_vtimer_cntvoff(vcpu, kvm_phys_timer_read()); > > + timer_set_offset(vtimer, kvm_phys_timer_read()); > > timer_set_offset(ptimer, 0); > > > > hrtimer_init(&timer->bg_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_HARD); > > @@ -840,7 +815,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value) > > break; > > case KVM_REG_ARM_TIMER_CNT: > > timer = vcpu_vtimer(vcpu); > > - update_vtimer_cntvoff(vcpu, kvm_phys_timer_read() - value); > > + timer_set_offset(timer, kvm_phys_timer_read() - value); > > break; > > case KVM_REG_ARM_TIMER_CVAL: > > timer = vcpu_vtimer(vcpu); > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > > index 64c086c02c60..169d629f97cb 100644 > > --- a/arch/arm64/kvm/hypercalls.c > > +++ b/arch/arm64/kvm/hypercalls.c > > @@ -44,7 +44,7 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val) > > feature = smccc_get_arg1(vcpu); > > switch (feature) { > > case KVM_PTP_VIRT_COUNTER: > > - cycles = systime_snapshot.cycles - vcpu_read_sys_reg(vcpu, CNTVOFF_EL2); > > + cycles = systime_snapshot.cycles - vcpu->kvm->arch.offsets.voffset; > > break; > > case KVM_PTP_PHYS_COUNTER: > > cycles = systime_snapshot.cycles; > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > > index 76174f4fb646..41fda6255174 100644 > > --- a/include/kvm/arm_arch_timer.h > > +++ b/include/kvm/arm_arch_timer.h > > @@ -23,6 +23,19 @@ enum kvm_arch_timer_regs { > > TIMER_REG_CTL, > > }; > > > > +struct arch_timer_offset { > > + /* > > + * If set, pointer to one of the offsets in the kvm's offset > > + * structure. If NULL, assume a zero offset. > > + */ > > + u64 *vm_offset; > > +}; > > + > > +struct arch_timer_vm_offsets { > > + /* Offset applied to the virtual timer/counter */ > > + u64 voffset; > > +}; > > + > > struct arch_timer_context { > > struct kvm_vcpu *vcpu; > > > > @@ -33,6 +46,8 @@ struct arch_timer_context { > > struct hrtimer hrtimer; > > u64 ns_frac; > > > > + /* Offset for this counter/timer */ > > + struct arch_timer_offset offset; > > /* > > * We have multiple paths which can save/restore the timer state onto > > * the hardware, so we need some way of keeping track of where the > > -- > > 2.34.1 > > > > > > It appears that we can remove CNTVOFF_EL2 from the sysreg file. > Having said that, I don't think it matters anyway since the > following patch appears to use that again. Indeed, and this is why I didn't remove it the first place (we need it for NV). > > Reviewed-by: Reiji Watanabe 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