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 8343CECAAD5 for ; Sat, 10 Sep 2022 10:00:05 +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=HHbuKkNu53cMaeDtiAw7xQKBXJqsdabp5t5WxpPpJ74=; b=1weeQglw8TIxsX wD/4HfBYVmwljujHzecpv3xIYKwgyuUWj+ami7kKmgrrrAXQrX6o44/WyZ3xzVMQj/prxKQvpTfhJ 4LekMcF0fkDedibfGxUtXMNbCXDJrB2gSGi19XYHnq79kjl1baY/oqIo7kajbPKt+dPfYY6YBomNZ NV786QPoz0eOzQQqFWsAhF5cocyrcK76kdJ2dmZ0+Q7iDTBcz97wvQzs42FbDivy2ERTNb5cv4KzC CIS6OIgoWE2UEKvVTaFTwRwmYTY4TjPH44WD/BDoTBJprMRbepcF6yGibDwHOZ77C77ZJMAHzmYbM BNHzpHKQJ9FG5o9Mxu/A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWxGR-0095nN-Ib; Sat, 10 Sep 2022 09:59:03 +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 1oWxGO-0095l1-Bu for linux-arm-kernel@lists.infradead.org; Sat, 10 Sep 2022 09:59:02 +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 B8269608D4; Sat, 10 Sep 2022 09:58:59 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 280BFC433D6; Sat, 10 Sep 2022 09:58:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662803939; bh=hXKRPt6Bl9LEQ7baOPFH3GB30UfeFbckby2U0muaeQA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=i0Du6sPDtqkY0OJacqT2yH3qRZHeE/+bD06pjNBSl6jJ03FdROXmpI800clbi2Ynz wHXCtmr9FoU6+vXu3TLXTpBstIB4WQupHTIpjcHp7jnM628G5MynrcDx0ydM7FK71b hHgzxkhzAgfGf6JvnMS6BUQdFh9PXeigkGzL/kBdKbv1H/no6c0VXCkVM1/FXpi+I7 jI+0YkQeWEPpGlU0G5NhXPrZ/DwU8z12xGguuXvYz3Fikr+rgfMVAg87Zaoa6LiqVy J3QJv7CQfattOe9nLVX8AXfwyRKCN6vN/wU0iP3c6M8/v8Sr0+ehL05rFB8vHEx/5e wQZmrsqX9imwA== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.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 1oWxGL-009O8U-34; Sat, 10 Sep 2022 10:58:57 +0100 Date: Sat, 10 Sep 2022 10:58:51 +0100 Message-ID: <877d2br1no.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Linux ARM , James Morse , Alexandru Elisei , Suzuki K Poulose , Paolo Bonzini , Ricardo Koller , Oliver Upton , Jing Zhang , Raghavendra Rao Anata Subject: Re: [PATCH 1/3] KVM: arm64: Don't set PSTATE.SS when Software Step state is Active-pending In-Reply-To: References: <20220909044636.1997755-1-reijiw@google.com> <20220909044636.1997755-2-reijiw@google.com> <87bkrora8b.wl-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/27.1 (x86_64-pc-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.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, pbonzini@redhat.com, ricarkol@google.com, oliver.upton@linux.dev, jingzhangos@google.com, rananta@google.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-20220910_025900_503314_0B9C513D X-CRM114-Status: GOOD ( 39.37 ) 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 Hi Reiji, On Sat, 10 Sep 2022 05:12:57 +0100, Reiji Watanabe wrote: > > > Currently, PSTATE.SS is set on every guest entry if single-step is > > > enabled for the vCPU by userspace. However, it could cause extra > > > single-step execution without returning to userspace, which shouldn't > > > be performed, if the Software Step state at the last guest exit was > > > Active-pending (i.e. the last exit was not triggered by Software Step > > > exception, but by an asynchronous exception after the single-step > > > execution is performed). > > > > For my own enlightenment, could you describe a sequence of events that > > leads to this issue? > > Here is an example of the sequences. > > [Usersace] > | - ioctl(SET_GUEST_DEBUG) > | - ioctl(KVM_RUN) (vCPU PC==X) > v > [KVM] > | - *vcpu_cpsr(vcpu) |= SPSR_SS; > | - mdscr |= DBG_MDSCR_SS; > | - VM Entry > v > [Guest] vCPU PC==X > | - Execute an instruction at PC==X. > | PC is updated with X+4, and PSTATE.SS is cleared. > | > | !! Asynchronous exception !! > v > [KVM] vCPU PC==X+4 > | - The kernel processes the async exception. > | - handle_exit() returns 1 (don't return to userspace) > | - *vcpu_cpsr(vcpu) |= SPSR_SS; > | - mdscr |= DBG_MDSCR_SS; > | - VM Entry > v > [Guest] vCPU PC==X+4 > | - Execute an instruction at PC==X+4. > | PC is updated with X+8, PSTATE.SS is cleared. > | > | !! Software Step Exception !! > v > [KVM] vCPU PC==X+8 > | - run->exit_reason = KVM_EXIT_DEBUG; > | - Return to userspace > v > [Userspace] > - Userspace sees PC==X+8 (Userspace didn't see PC==X+4). > OK, I think I get it now, and I got confused because of the naming which is similar to what we use for interrupts, but the semantics are very different (let's pretend that this is the cause of most of my stupid questions earlier...). The states are described as such: Active+non-Pending: MDSCR.SS=1, PSTATE.SS=1 Active+Pending: MDSCR.SS=1, PSTATE.SS=0 and it is the inversion of PSTATE.SS that got me. The pending state describe the state of the *exception*. Before executing the instruction, no exception is pending. Once executed, an exception is pending. Of course, if we get an interrupt right after a single step (and that the implementation prioritises them over synchronous exceptions), we exit because of the interrupt, and the bug you uncovered sends us back to Active+non-Pending, losing the SS exception. Boo. Your fix is to not set PSTATE.SS=1 until we have actually handled a debug exception. In the above example, this would result in: [Guest] vCPU PC==X | - Execute an instruction at PC==X. | PC is updated with X+4, and PSTATE.SS is cleared. | | !! Asynchronous exception !! v [KVM] vCPU PC==X+4 | - The kernel processes the async exception. | - handle_exit() returns 1 (don't return to userspace) | - vcpu_set_flag(vcpu, DBG_SS_ACTIVE_PENDING); | - mdscr |= DBG_MDSCR_SS; | - VM Entry v [Guest] vCPU PC==X+4 | - Pending SS exception | | !! Software Step Exception !! v [KVM] vCPU PC==X+4 | - run->exit_reason = KVM_EXIT_DEBUG; | - vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING); | - Return to userspace v [Userspace] - Userspace sees PC==X+4 It is amusing that we never saw that one before, but I guess the IMPDEF nature of the exception prioritisation caught us here. Also, VM debugging is a relatively rare use case. > > Now, where does the asynchronous exception comes into play? I found > > this intriguing remark in the ARM ARM: > > > > > > The Software Step exception has higher priority than all other types > > of synchronous exception. However, the prioritization of this > > exception with respect to any unmasked pending asynchronous exception > > is not defined by the architecture. > > > > > > Is this what you were referring to in the commit message? I think you > > need to spell it out for us, as I don't fully understand what you are > > fixing nor do I understand the gory details of single-stepping... > > Yes, that is what I was referring to. > In "Figure D2-3 Software step state machine" in Arm ARM (DDI 0487I.a), > since KVM currently sets PSTATE.SS to 1 on every Guest Entry (when > single-step is enabled by userspace), KVM always has the CPU in > "Inactive" (the second inactive state from the top) transition to > "Active-not-pending" on the Guest Entry. With this patch, KVM > have the CPU transitions to "Active-pending" if the state before > "Inactive" was "Active-pending", which indicates the step completed > but Software Step exception is not taken yet, so that Software > Step exception is taken before further single-step is executed. > > I'm sorry for the unclear explanation, and > I hope my comments clarify the problem I'm trying to fix. Please do not apologise! This is excellent work, and I'm really glad you got to the bottom of this. It'd be good to capture some of this discussion in the commit message though, as I'm pretty we will all blissfully forget all about it shortly! 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