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 mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2360AC433F5 for ; Thu, 17 Mar 2022 11:34:45 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 6530249F3D; Thu, 17 Mar 2022 07:34:45 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@kernel.org Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Xk0KwKoqeIlE; Thu, 17 Mar 2022 07:34:44 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 04CCB49F28; Thu, 17 Mar 2022 07:34:44 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7AF7E49F1D for ; Thu, 17 Mar 2022 07:34:43 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OpDbmkMeF4fz for ; Thu, 17 Mar 2022 07:34:42 -0400 (EDT) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 1139349F07 for ; Thu, 17 Mar 2022 07:34:42 -0400 (EDT) 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 680AB618EE; Thu, 17 Mar 2022 11:34:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C9BAEC340EC; Thu, 17 Mar 2022 11:34:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1647516879; bh=ORov9gVmJ2HPUwjeBEPodpmnd8fTvKTLG5i+PtnN5NI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=YmcmrL5w+ZcF35uC0sBaHJZgEhB+AN3iQsaS9qUvyKo6gwUeeBoZtFMDq8S52jTGn s8HRIfP0+4XMYkWAawosJ8+HDOQLl3lPjqS761u8EL8BqZr4l6xG0AgAL3RideCxuu m4J+moGrVbHnMOs5nRBk3oQZm6SCtIsQmA/491vxuaDIBr1QXKw0LgemFOGdUI4llO tDTyq7a3N3JeAGaEYwEdJ/zHzoG3qL/2SbwZ8KxW0z9SfvQ3BDNcS98D8zmGxOIsFs rxzQWdp/F90dJyRQiSkOGsOH4GHCN7ocsyNARj32GZ8UEOaCRcd+341ZRj80rIJ6NR PpyLP476ObXNQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nUoOr-00F9vq-He; Thu, 17 Mar 2022 11:34:37 +0000 Date: Thu, 17 Mar 2022 11:34:37 +0000 Message-ID: <87tubwyfqq.wl-maz@kernel.org> From: Marc Zyngier To: Jing Zhang Subject: Re: [PATCH v1 1/2] KVM: arm64: Add arch specific exit reasons In-Reply-To: <20220317005630.3666572-2-jingzhangos@google.com> References: <20220317005630.3666572-1-jingzhangos@google.com> <20220317005630.3666572-2-jingzhangos@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: jingzhangos@google.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, will@kernel.org, pbonzini@redhat.com, dmatlack@google.com, seanjc@google.com, oupton@google.com, reijiw@google.com, ricarkol@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 Cc: KVM , David Matlack , Paolo Bonzini , Will Deacon , KVMARM X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Jing, On Thu, 17 Mar 2022 00:56:29 +0000, Jing Zhang wrote: > > Arch specific exit reasons have been available for other architectures. > Add arch specific exit reason support for ARM64, which would be used in > KVM stats for monitoring VCPU status. > > Signed-off-by: Jing Zhang > --- > arch/arm64/include/asm/kvm_emulate.h | 5 +++ > arch/arm64/include/asm/kvm_host.h | 33 +++++++++++++++ > arch/arm64/kvm/handle_exit.c | 62 +++++++++++++++++++++++++--- > arch/arm64/kvm/mmu.c | 4 ++ > arch/arm64/kvm/sys_regs.c | 6 +++ > 5 files changed, 105 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index d62405ce3e6d..f73c8d900642 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -321,6 +321,11 @@ static inline bool kvm_vcpu_trap_is_iabt(const struct kvm_vcpu *vcpu) > return kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_IABT_LOW; > } > > +static inline bool kvm_vcpu_trap_is_dabt(const struct kvm_vcpu *vcpu) > +{ > + return kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW; > +} > + > static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu) > { > return kvm_vcpu_trap_is_iabt(vcpu) && !kvm_vcpu_abt_iss1tw(vcpu); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 76f795b628f1..daa68b053bdc 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -282,6 +282,36 @@ struct vcpu_reset_state { > bool reset; > }; > > +enum arm_exit_reason { > + ARM_EXIT_UNKNOWN, > + ARM_EXIT_IRQ, > + ARM_EXIT_EL1_SERROR, > + ARM_EXIT_HYP_GONE, > + ARM_EXIT_IL, > + ARM_EXIT_WFI, > + ARM_EXIT_WFE, > + ARM_EXIT_CP15_32, > + ARM_EXIT_CP15_64, > + ARM_EXIT_CP14_32, > + ARM_EXIT_CP14_LS, > + ARM_EXIT_CP14_64, > + ARM_EXIT_HVC32, > + ARM_EXIT_SMC32, > + ARM_EXIT_HVC64, > + ARM_EXIT_SMC64, > + ARM_EXIT_SYS64, > + ARM_EXIT_SVE, > + ARM_EXIT_IABT_LOW, > + ARM_EXIT_DABT_LOW, > + ARM_EXIT_SOFTSTP_LOW, > + ARM_EXIT_WATCHPT_LOW, > + ARM_EXIT_BREAKPT_LOW, > + ARM_EXIT_BKPT32, > + ARM_EXIT_BRK64, > + ARM_EXIT_FP_ASIMD, > + ARM_EXIT_PAC, > +}; > + > struct kvm_vcpu_arch { > struct kvm_cpu_context ctxt; > void *sve_state; > @@ -382,6 +412,9 @@ struct kvm_vcpu_arch { > u64 last_steal; > gpa_t base; > } steal; > + > + /* Arch specific exit reason */ > + enum arm_exit_reason exit_reason; We already have a copy of ESR_EL2. Together with the exit code, this gives you everything you need. Why add another piece of state? [...] > @@ -135,6 +179,7 @@ static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu) > kvm_pr_unimpl("Unknown exception class: esr: %#08x -- %s\n", > esr, esr_get_class_string(esr)); > > + vcpu->arch.exit_reason = ARM_EXIT_UNKNOWN; If anything, this should say "either CPU out of spec, or KVM bug". And I don't see the point of tracking these. This should be reported in a completely different manner, because this has nothing to do with the normal exits a vcpu does. > @@ -250,6 +299,7 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index) > * EL2 has been reset to the hyp-stub. This happens when a guest > * is pre-empted by kvm_reboot()'s shutdown call. > */ > + vcpu->arch.exit_reason = ARM_EXIT_HYP_GONE; Same thing here: the machine is *rebooting*. Who cares? > run->exit_reason = KVM_EXIT_FAIL_ENTRY; > return 0; > case ARM_EXCEPTION_IL: > @@ -257,11 +307,13 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index) > * We attempted an illegal exception return. Guest state must > * have been corrupted somehow. Give up. > */ > + vcpu->arch.exit_reason = ARM_EXIT_IL; This is another reason why I dislike this patch. It mixes architectural state (ESR) and KVM gunk (exit code). Why not spit these two bits of information in the trace, and let whoever deals with it to infer what they want from it? > run->exit_reason = KVM_EXIT_FAIL_ENTRY; > return -EINVAL; > default: > kvm_pr_unimpl("Unsupported exception type: %d", > exception_index); > + vcpu->arch.exit_reason = ARM_EXIT_UNKNOWN; See? Now you have UNKNOWN covering two really different concepts. That's broken. Overall, this patch is reinventing the wheel (and slightly square one), and I don't see a good reason for the state duplication. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm