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 E3A33E77173 for ; Mon, 9 Dec 2024 10:20:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type: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=U+kFGG53tud4P1pGsRkfJqvfZuZnzwl8uZtzHlalhsE=; b=2gO35FYzKQoU4SrGVZjnIW1XD4 71Ox75xMbJ7bDpNp49hCJ5tCcde8aN8oK0PA9drXvqGkXKbnZGTR9bdDt59NpoQJVhKqPDnc1QOy2 OZcKao+FwvEWaz26lyF9SiyjBPvrQE+XVYS8Ql7Sdab7ZrdMTtCk4LcQDjinY9JcIoBv0NNAUrxAa Jn8lCSjRq7R/oSNSdBgpt07SkKz0QFHDhx1a6N8ujQEXyYm/Ut702Wt+r+2Hi+9awnRtNT9ZE+9Q3 wLz6wxRu7wz0JxwW+6UIJZiF1P4NaWJtt2Vx7DhrMfkx7j3bHWhLNxx4++8PUzmBN1CamCvHMoSRv DFBDnF0Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tKarq-00000007G8g-1nTa; Mon, 09 Dec 2024 10:19:54 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tKaTJ-00000007Ake-3AMv for linux-arm-kernel@lists.infradead.org; Mon, 09 Dec 2024 09:54:35 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 16A2FA4129C; Mon, 9 Dec 2024 09:52:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4E781C4CED1; Mon, 9 Dec 2024 09:54:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1733738072; bh=8KrOzz1PgbSErfhC2I3kQ/sYbxXPyl6RjlXIVILvOrI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ndjpzBnT83lfYQeQZCm6qfkczXzMeH7Q3bMaOjhcWbn0YGlmSsOGCwnVpY9rAK+ld R2VVnNr2zFwgWPZ+y98EZjBx3AAglqflxNZpODD4cr4v58E5TGU/cKEN388ajDIZEE VXV9LXTsXvNK0RMHoqhbCHXFFvJKP4V3GKRzxnzfO76csJMtY7BwjkkxEg4ult/NmQ juOXJr+3xJrttFZc0vV8HEcnwczLZ/bLtQVpr0nb2wLAFrCdqWpHnlPKR1Zgw7oPRi 3NAyyySso3/tVzBuQ+gBlqe2P7iv8spop1O/bL49nnjsGhBMAr33xAqRhfyxMT6frT N7pfEqMpro7tA== 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 1tKaTF-001oNM-SO; Mon, 09 Dec 2024 09:54:29 +0000 Date: Mon, 09 Dec 2024 09:54:29 +0000 Message-ID: <867c89tc4q.wl-maz@kernel.org> From: Marc Zyngier To: Ganapatrao Kulkarni Cc: kvmarm , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, oliver.upton@linux.dev, christoffer.dall@arm.com, suzuki.poulose@arm.com, will@kernel.org, catalin.marinas@arm.com, coltonlewis@google.com, joey.gouly@arm.com, yuzenghui@huawei.com, darren@os.amperecomputing.com, vishnu@os.amperecomputing.com Subject: Re: [PATCH] KVM: arm64: nv: Set ISTATUS for emulated timers, If timer expired In-Reply-To: <20241209053201.339939-1-gankulkarni@os.amperecomputing.com> References: <20241209053201.339939-1-gankulkarni@os.amperecomputing.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/29.4 (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=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: gankulkarni@os.amperecomputing.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, oliver.upton@linux.dev, christoffer.dall@arm.com, suzuki.poulose@arm.com, will@kernel.org, catalin.marinas@arm.com, coltonlewis@google.com, joey.gouly@arm.com, yuzenghui@huawei.com, darren@os.amperecomputing.com, vishnu@os.amperecomputing.com 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-20241209_015433_925789_8160C05D X-CRM114-Status: GOOD ( 36.41 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Ganapatrao, Did you notice that the Columbia list was killed over two years ago, as per ac107abef1976 ("KVM: arm64: Advertise new kvmarm mailing list")? Consider that script/get_maintainer.pl is your friend. Cc'ing the correct list instead, On Mon, 09 Dec 2024 05:32:01 +0000, Ganapatrao Kulkarni wrote: >=20 > During automated testing of Nested Virtualization using avocado-vt, Which is not merged upstream. So what branch are you using? Based on what kernel version? On what HW? With which virtualisation features? > it has been observed that during some boot test iterations, > the Guest-Hypervisor boot was getting crashed with a > synchronous exception while it is still booting EDK2. >=20 > The test is launching Multiple instances of Guest-Hypervisor boot Is the multiple instance aspect really relevant to the reproduction of the problem? > and while booting, QEMU monitor issued the command "info register" > at regular intervals to take a register dump. To execute this > command, QEMU stops the run and does the register read of various > registers. While resuming the run, the function kvm_arm_timer_write() > writes back the saved CNTV_CTL_EL0 register with ISTATUS cleared always It is userspace that causes this write-back, right? AFAICT, KVM never does that on its own. > and resulting in the loss of pending interrupt for emulated timers. How does a missing interrupt result in a synchronous exception in EDK2? In my experience, EDK2 panics if it sees spurious interrupts, not when it is missing interrupts (it just locks up, which is expected). > In hardware based timers, ISTATUS is a RO/WI bit and gets set by the > h/w, if the condition is still met. However, in Nested-Virtualization > case, the Guest-Hypervisor's=C2=A0EDK2 is using an emulated virtual timer > and losing ISTATUS state and the interrupt forever. Why is this specific to NV? Can't the same thing happen to the physical timer in a non-VHE configuration? >=20 > Adding fix in kvm_arm_timer_write to set ISTATUS for emulated > timers, if the timer expired already. >=20 > Fixes: 81dc9504a700 ("KVM: arm64: nv: timers: Support hyp timer emulation= ") Where is this coming from? This patch doesn't touch the code you are changing, so how can it be the source of your problems? As far as I can tell, this has been the case since 5c5196da4e966 ("KVM: arm/arm64: Support EL1 phys timer register access in set/get reg"). > Co-developed-by: Vishnu Pajjuri > Signed-off-by: Vishnu Pajjuri > Signed-off-by: Ganapatrao Kulkarni > --- > arch/arm64/kvm/arch_timer.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) >=20 > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index 1215df590418..aca58113d790 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -1199,7 +1199,16 @@ static void kvm_arm_timer_write(struct kvm_vcpu *v= cpu, > break; > =20 > case TIMER_REG_CTL: > - timer_set_ctl(timer, val & ~ARCH_TIMER_CTRL_IT_STAT); > + struct timer_map map; > + > + val &=3D ~ARCH_TIMER_CTRL_IT_STAT; > + get_timer_map(vcpu, &map); > + /* Set ISTATUS bit for emulated timers, if timer expired. */ > + if (timer =3D=3D map.emul_vtimer || timer =3D=3D map.emul_ptimer) { > + if (!kvm_timer_compute_delta(timer)) > + val |=3D ARCH_TIMER_CTRL_IT_STAT; > + } > + timer_set_ctl(timer, val); > break; This really looks awfully complicated, when it is only a matter of recomputing the interrupt state, something that is at the core of the timer emulation. Why can't the following work: diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c index 895f09658ef83..bd6efafbe7109 100644 --- a/arch/arm64/kvm/arch_timer.c +++ b/arch/arm64/kvm/arch_timer.c @@ -1298,13 +1298,17 @@ static void kvm_arm_timer_write(struct kvm_vcpu *vc= pu, enum kvm_arch_timer_regs treg, u64 val) { + unsigned long tmp =3D val; + switch (treg) { case TIMER_REG_TVAL: timer_set_cval(timer, kvm_phys_timer_read() - timer_get_offset(timer) + = (s32)val); break; =20 case TIMER_REG_CTL: - timer_set_ctl(timer, val & ~ARCH_TIMER_CTRL_IT_STAT); + __assign_bit(__ffs(ARCH_TIMER_CTRL_IT_STAT), &tmp, + kvm_timer_should_fire(timer)); + timer_set_ctl(timer, tmp); break; =20 case TIMER_REG_CVAL: But overall, this very much looks like it is only papering over the real issue, which is that the *emulation* should regenerate the pending bit, and not rely on the userspace interface. As far as I can tell, we already correctly compute the status bit on read (read_timer_ctl()), so the guest should always observe something consistent when it traps. We also *never* use the status bit as an input to the emulation, and always recompute it from scratch (it is only there for the benefit of the guest or userspace). So I can't see how upstream is broken in at the moment, and you need to explain how this actually triggers. Ideally, with a standalone reproducer or a selftest. Thanks, M. --=20 Without deviation from the norm, progress is not possible.