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 343AFC433EF for ; Thu, 7 Apr 2022 20:14:14 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=u+m9kpuJohfZGjgWjhFGNN67g95MUif/zqOrk7gPrYc=; b=rDX5K62LGWCd7v XBfYTOMU5o15Se7fB0SQGjQdT0rzOC0YDbYjEC7n99aQf6o9mBDibnCmRggZ8ChCFOd02pQu9V6m5 Vrjhx37ilezx9gbw24sHIIrIl8zjRRXSMhAwgIpwgqL4cVesliT9/VfnTpD+roheDfXYyzoL6P/LF giNqb3WEJdhHCdq7cax074bDusu40ytUaQ9ZXwFYY7iRyW979q8YOERBWvoq7ZFGQa62JOvXbrlb8 FUNQaEdOlE8eibrB7Or/CDunPbhWhDCJ5cAKQcL71mDBAvBLhYDk2UtX4EdOG/HsT3hwa9c5B2Ooa /Ty8qtICaj2t3h01zcKA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncYUy-00DpQj-Ls; Thu, 07 Apr 2022 20:12:56 +0000 Received: from mail-io1-xd34.google.com ([2607:f8b0:4864:20::d34]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ncYUv-00DpPn-As for linux-arm-kernel@lists.infradead.org; Thu, 07 Apr 2022 20:12:54 +0000 Received: by mail-io1-xd34.google.com with SMTP id k25so8188994iok.8 for ; Thu, 07 Apr 2022 13:12:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=sJRTozuXZn5+Q6veTiT+vzqfmgVSoHWXITOn1w2myow=; b=RxWOQY6KdgZaMEREMmfvE+CHxxieSSZNnxM2rFqPA0wSomNEou/Wa41qEFdJrCgNBv mZMq+So5tvvc1z8AtFY0XOyYfDfkFyk/ATSy9gVS1xHEs08u2icaao6NvoGdifTFGX2K 4aRjMyFoIyU+SpQHQnHNigfrqzsuy/pkifoWHm5T22FWKP6G1zUU0qOztPNrj9xf4eWE MNLwoFjPwjyGXziejWrnvWsS/9RYLAEVsciBrKFO3dtGoj65Li7YCkGt1/583UJVvCNW Ypnx2L+220K4kKmx8apWVq6mw3fupO782neBDOLI74pZAr16sCL5vY2HcDUodmzC9QK5 E6/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=sJRTozuXZn5+Q6veTiT+vzqfmgVSoHWXITOn1w2myow=; b=qb2BdEUcMTh9uV5Fx9tgo9BsRx2ib8byQ3ximxcTtSAMeCzRkAOcr86CjSwkbTR5BM +rkD33hkLcTkAXv1H+VOCl3KleQW+tdBtxrNEnCbBBnBhWTGaGkU2Hb/2elKn2Mvhzmj 4QKpB/EF4viu/G9Bwa6kr7njV9mnSuVlwuoY9Tg7a+xbVWkkoLOony8q+zA5k12L5KLA UdpLI1Lhsl/yBOjVCZl763Qggi9zWEgnx4UHrZXu21xwPO/EJTVmIOhGaIOZObo42LvL kS+M8ta+wQ10Mo32yIYqLneo57WCSN1Arsrv/UOchzAGeKFOYtO60d4rFCzcEJxP4FNW sCGQ== X-Gm-Message-State: AOAM533RdMvE2oFs4S94TQU454gk3cHd4+oJoVjNcrL2bXfAPwJBJCWo gwF6ZfeLgHW5xbEh+Xo9oblZhw== X-Google-Smtp-Source: ABdhPJyqXm9YX9vTJiNnxSBcA8ph2PFU3mlGYEK10zOmON/8Ld6MPO1USArTjmiQeR6sNjczH9IqqA== X-Received: by 2002:a05:6638:358a:b0:323:cbda:73c0 with SMTP id v10-20020a056638358a00b00323cbda73c0mr8182773jal.136.1649362367188; Thu, 07 Apr 2022 13:12:47 -0700 (PDT) Received: from google.com (194.225.68.34.bc.googleusercontent.com. [34.68.225.194]) by smtp.gmail.com with ESMTPSA id y17-20020a92d0d1000000b002ca8027016bsm939371ila.45.2022.04.07.13.12.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Apr 2022 13:12:46 -0700 (PDT) Date: Thu, 7 Apr 2022 20:12:43 +0000 From: Oliver Upton To: Marc Zyngier Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, James Morse , Alexandru Elisei , Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, Peter Shier , Ricardo Koller , Reiji Watanabe Subject: Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents Message-ID: References: <20220401010832.3425787-1-oupton@google.com> <20220401010832.3425787-2-oupton@google.com> <87lewib68f.wl-maz@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87lewib68f.wl-maz@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220407_131253_416309_96B1D9C6 X-CRM114-Status: GOOD ( 33.69 ) 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 Marc, On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote: > > + /* > > + * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32. > > + * Avoid conflicting with future expansion of AArch64 feature registers > > + * and simply treat them as RAZ here. > > + */ > > + if (params->CRm > 3) > > + params->regval = 0; > > + else > > + ret = emulate_sys_reg(vcpu, params); > > + > > + vcpu_set_reg(vcpu, Rt, params->regval); > > It feels odd to update Rt without checking whether the read has > succeeded. In your case, this is harmless, but would break with the > approach I'm outlining below. > A total kludge to avoid yet another level of indentation :) I'll go about this the right way next spin. > > + return ret; > > +} > > + > > +/** > > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an > > + * AArch32 ID register. > > + * @params: the system register access parameters > > + * > > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The > > + * only register trapped in the CRm=0 range is CTR, which is already handled in > > + * the cp15 register table. > > There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0, > which prevents it from fitting in your scheme. > > > + */ > > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params) > > +{ > > + return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0; > > +} > > + > > /** > > * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access > > * @vcpu: The VCPU pointer > > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, > > params.Op1 = (esr >> 14) & 0x7; > > params.Op2 = (esr >> 17) & 0x7; > > > > + /* > > + * Certain AArch32 ID registers are handled by rerouting to the AArch64 > > + * system register table. > > + */ > > + if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(¶ms)) > > + return kvm_emulate_cp15_id_reg(vcpu, ¶ms); > > I think this is a bit ugly. We reach this point from a function that > was cp15-specific, and now we are reconstructing the context. I'd > rather this is moved to kvm_handle_cp15_32(), and treated there > (untested): > Completely agree, hoisting this would be much more elegant. > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 7b45c040cc27..a071d89ace92 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, > * @run: The kvm_run struct > */ > static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, > + struct sys_reg_params *params, > const struct sys_reg_desc *global, > size_t nr_global) > { > - struct sys_reg_params params; > - u32 esr = kvm_vcpu_get_esr(vcpu); > int Rt = kvm_vcpu_sys_get_rt(vcpu); > > - params.CRm = (esr >> 1) & 0xf; > - params.regval = vcpu_get_reg(vcpu, Rt); > - params.is_write = ((esr & 1) == 0); > - params.CRn = (esr >> 10) & 0xf; > - params.Op0 = 0; > - params.Op1 = (esr >> 14) & 0x7; > - params.Op2 = (esr >> 17) & 0x7; > + params->regval = vcpu_get_reg(vcpu, Rt); > > - if (!emulate_cp(vcpu, ¶ms, global, nr_global)) { > - if (!params.is_write) > - vcpu_set_reg(vcpu, Rt, params.regval); > + if (!emulate_cp(vcpu, params, global, nr_global)) { > + if (!params->is_write) > + vcpu_set_reg(vcpu, Rt, params->regval); > return 1; > } > > - unhandled_cp_access(vcpu, ¶ms); > + unhandled_cp_access(vcpu, params); > return 1; > } > > @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu) > > int kvm_handle_cp15_32(struct kvm_vcpu *vcpu) > { > - return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs)); > + struct sys_reg_params params; > + > + params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu)); > + > + if (params.Op1 == 0 && params.CRn == 0 && params.CRm) > + return kvm_emulate_cp15_id_reg(vcpu, ¶ms); > + > + return kvm_handle_cp_32(vcpu, ¶ms, cp15_regs, ARRAY_SIZE(cp15_regs)); > } > > int kvm_handle_cp14_64(struct kvm_vcpu *vcpu) > @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu) > > int kvm_handle_cp14_32(struct kvm_vcpu *vcpu) > { > - return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs)); > + struct sys_reg_params params; > + > + params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu)); > + > + return kvm_handle_cp_32(vcpu, ¶ms, cp14_regs, ARRAY_SIZE(cp14_regs)); > } > > static bool is_imp_def_sys_reg(struct sys_reg_params *params) > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h > index cc0cc95a0280..fd4b2bb8c782 100644 > --- a/arch/arm64/kvm/sys_regs.h > +++ b/arch/arm64/kvm/sys_regs.h > @@ -35,6 +35,13 @@ struct sys_reg_params { > .Op2 = ((esr) >> 17) & 0x7, \ > .is_write = !((esr) & 1) }) > > +#define esr_cp1x_32_to_params(esr) \ > + ((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7, \ > + .CRn = ((esr) >> 10) & 0xf, \ > + .CRm = ((esr) >> 1) & 0xf, \ > + .Op2 = ((esr) >> 17) & 0x7, \ > + .is_write = !((esr) & 1) }) > + > struct sys_reg_desc { > /* Sysreg string for debug */ > const char *name; > > > What do you think? Way better. Your suggested patch looks correct, I'll fold all of this together and test it out. Thanks for the suggestions :) -- Best, Oliver _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel