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 DF9D2ECAAD5 for ; Sat, 10 Sep 2022 10:37:53 +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=SdQmDN1vSdQIqZ1cgfe1UBxrqiqcyQChCCTeSCz8pio=; b=uTzbyUdLJeGaNF TtOGt1Bnqr+r2gKN8/lX78ZTcp1JHKEurulpFBHWsEpl9XqduXVcQqJoQ+xTk6Y/09iVnZI1Q/ncL B0mnlUzQEMnAS7QtqCIWeU4K3A63k6Al9QDrbeWtchQaBAUBn/w61/q2TzONtu2+ChqDCZMVzCBJY 3nm1a0YFo+m59rnRJpK/I+0z+d7sjF4dvh78jtbwGFH/XxNPSCCM5qHgvde8GVlxpbr+WsXWIQ1l6 UKdRD5NUTblls7ToR1pxPCA4aioDpTCgsUH7meeOEVCsLfpcmLazLPzjP4w1cW4Kna0O4TstTkvgb 44nvwzEW8ZO2hPW4sBtA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWxqR-009P5P-AU; Sat, 10 Sep 2022 10:36:15 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWxqO-009P4y-5e for linux-arm-kernel@lists.infradead.org; Sat, 10 Sep 2022 10:36:14 +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 ams.source.kernel.org (Postfix) with ESMTPS id 89871B807ED; Sat, 10 Sep 2022 10:36:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1E4E6C433D6; Sat, 10 Sep 2022 10:36:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662806168; bh=JcsNXXKqzhV/1uuRQUph15OvWzE1MZ/pE5WVAyByBWY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=B6koL3NK7jJp5wDsT4RHo3fSbIaKDPtS7lvpioPxRSq87vfpsEHmyb0xARzpVOtYl xEEP0DPi9EQUrwofWI/WfL1LySb17HFR6roLKwbzWPwllihHsVIoLytQREwsyUWGGK HAzhW/yN68QAAy209tO0eDytKCWmBsdJ7AcigG7CESu3PggrWFnD/FN566bwz/f1cQ 1yTezzc6zeBOa/fK2vHhXZ//x1RTeQt/FiE1A+Ed5mfrxHDevSFr8Y1IrQ1YRo21hV YHWxnniB45dbEAoXgMxtpr0hri5xxEunuG9O4tNN/gaCO5E7Q31nw3AT3gFmpE7RdL eL+2LSp4Jf3qg== 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 1oWxqH-009OIr-O2; Sat, 10 Sep 2022 11:36:05 +0100 Date: Sat, 10 Sep 2022 11:36:04 +0100 Message-ID: <875yhvqzxn.wl-maz@kernel.org> From: Marc Zyngier To: Reiji Watanabe Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 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: <20220909044636.1997755-2-reijiw@google.com> References: <20220909044636.1997755-1-reijiw@google.com> <20220909044636.1997755-2-reijiw@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/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_033612_546226_E119489A X-CRM114-Status: GOOD ( 38.87 ) 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 Fri, 09 Sep 2022 05:46:34 +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). > > Fix this by not setting PSTATE.SS on guest entry if the Software > Step state at the last exit was Active-pending. > > Fixes: 337b99bf7edf ("KVM: arm64: guest debug, add support for single-step") > Signed-off-by: Reiji Watanabe Now that I'm a bit more clued about what the architecture actually mandates, I can try and review this patch. > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/debug.c | 19 ++++++++++++++++++- > arch/arm64/kvm/guest.c | 1 + > arch/arm64/kvm/handle_exit.c | 2 ++ > 4 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e9c9388ccc02..4cf6eef02565 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -535,6 +535,9 @@ struct kvm_vcpu_arch { > #define IN_WFIT __vcpu_single_flag(sflags, BIT(3)) > /* vcpu system registers loaded on physical CPU */ > #define SYSREGS_ON_CPU __vcpu_single_flag(sflags, BIT(4)) > +/* Software step state is Active-pending */ > +#define DBG_SS_ACTIVE_PENDING __vcpu_single_flag(sflags, BIT(5)) > + > > /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */ > #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \ > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index 0b28d7db7c76..125cfb94b4ad 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -188,7 +188,16 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > * debugging the system. > */ > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > - *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + /* > + * If the software step state at the last guest exit > + * was Active-pending, we don't set DBG_SPSR_SS so > + * that the state is maintained (to not run another > + * single-step until the pending Software Step > + * exception is taken). > + */ > + if (!vcpu_get_flag(vcpu, DBG_SS_ACTIVE_PENDING)) > + *vcpu_cpsr(vcpu) |= DBG_SPSR_SS; > + > mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); > mdscr |= DBG_MDSCR_SS; > vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1); > @@ -279,6 +288,14 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > &vcpu->arch.debug_ptr->dbg_wcr[0], > &vcpu->arch.debug_ptr->dbg_wvr[0]); > } > + > + if ((vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) && > + !(*vcpu_cpsr(vcpu) & DBG_SPSR_SS)) > + /* > + * Mark the vcpu as ACTIVE_PENDING > + * until Software Step exception is confirmed. s/confirmed/taken/? This would match the comment in the previous hunk. > + */ > + vcpu_set_flag(vcpu, DBG_SS_ACTIVE_PENDING); > } > } > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index f802a3b3f8db..2ff13a3f8479 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -937,6 +937,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > } else { > /* If not enabled clear all flags */ > vcpu->guest_debug = 0; > + vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING); > } > > out: > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index bbe5b393d689..8e43b2668d67 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -154,6 +154,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > > if (ESR_ELx_EC(esr) == ESR_ELx_EC_WATCHPT_LOW) > run->debug.arch.far = vcpu->arch.fault.far_el2; > + else if (ESR_ELx_EC(esr) == ESR_ELx_EC_SOFTSTP_LOW) > + vcpu_clear_flag(vcpu, DBG_SS_ACTIVE_PENDING); Can we write this as a switch/case statement? > > return 0; > } I think we also need to do something if userspace decides to write to PSTATE as a result of a non-debug exit (such as a signal) when this DBG_SS_ACTIVE_PENDING is set. I came up with the following complicated, but not impossible scenario: - guest single step, PSTATE.SS=0 - exit due to interrupt - DBG_SS_ACTIVE_PENDING set - reenter guest - exit again due to another interrupt - exit to userspace due to signal pending - userspace writes PSTATE.SS=1 for no good reason - we now have an inconsistent state between PSTATE.SS and the vcpu flags My gut feeling is that we need something like the vcpu flag being set to !PSTATE.SS if written while debug is enabled. 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