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 C62E2C04FF0 for ; Sat, 13 Apr 2024 10:19:56 +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=vhRFHfk0qIFksisg3ZjOwfMTR49DhfOTvujiMXTCO3c=; b=LYr4rfYCYNGvnG MrHMgF+zQHnrjfGBIcHykFqhCUZBtp/NjugYZFMh74M9R6pChyT+KMJRoXUGVvA7+Kc6E37JkvF7K EbM9Nk/bK92k5NbKEnDbh1UYjzfo50HlWI3KqBvAodXncg8Ig90se5/mZi2a3219F1bypfRcjW6Op xfM0IFvrwGtzeO0Y3Vv30QlfSkGTaHV3LzAvQCZpsnYYRp3CBe/hfRYLc/kLr+af0tblG2/aE8x1u ZQp1N7ACc4Ahud8Uh+0GX3/LyjhqWoRpySV523O07gwj1DQhq/Z8MN3S3qKaDVuTpBmZSTQRraSuE AmH2ze8v8UrnlBPZ/rsw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rvaU3-00000002w24-27AE; Sat, 13 Apr 2024 10:19:43 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rvaU1-00000002w1G-275R for linux-arm-kernel@lists.infradead.org; Sat, 13 Apr 2024 10:19:42 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id A4F5861656; Sat, 13 Apr 2024 10:19:39 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4C37BC4AF07; Sat, 13 Apr 2024 10:19:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1713003579; bh=9Hu40LkvlfQcCNkyBLs/RLhZHlI4yg/7v8dPJiVYyEs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=c29dNQ+yEubEhbE3XnXXfhoAGCvaWZ5K9uB9X21vxDxDhDmr4B+n0VE6ouSrj7cG1 pjpqVKhq7uzDJ3WOpuR2H7UCz2V0J4CPBEdUN09AHuQuV3VAXxI+oGYb/LOZMqQ/Q0 eZ5ZP3jd0EkZty/k32oHNI2pTXOM3ZM47DKIJRfetlxyNJXfgt2EXfVGpHHXf3WlM+ PWiZG9yn7GLfpOtiXXYA6zlCFf0813Lc39o8CyLCkaZde2t13pXUW9BWdzGVRUpAIQ L8FqQc5mvo+ewtlQtGXy7rGYpwNsHbgpdRc1xkgAlUDkv5ZOpmZJGWfZb4Y2RKPXRB W7XOKRXg7YY/A== 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 1rvaTx-0047xk-6h; Sat, 13 Apr 2024 11:19:37 +0100 Date: Sat, 13 Apr 2024 11:19:35 +0100 Message-ID: <86edb9sgy0.wl-maz@kernel.org> From: Marc Zyngier To: Sebastian Ott Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, Oliver Upton , James Morse , Suzuki K Poulose , Catalin Marinas , Will Deacon Subject: Re: [PATCH 3/4] KVM: arm64: add emulation for CTR_EL0 register In-Reply-To: <20240405120108.11844-4-sebott@redhat.com> References: <20240405120108.11844-1-sebott@redhat.com> <20240405120108.11844-4-sebott@redhat.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.2 (aarch64-unknown-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: sebott@redhat.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, oliver.upton@linux.dev, james.morse@arm.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org 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-20240413_031941_657062_581C63EF X-CRM114-Status: GOOD ( 30.92 ) 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, 05 Apr 2024 13:01:07 +0100, Sebastian Ott wrote: > > CTR_EL0 is currently handled as an invariant register, thus > guests will be presented with the host value of that register. > Add emulation for CTR_EL0 based on a per VM value. > > When CTR_EL0 is changed the reset function for CLIDR_EL1 is > called to make sure we present the guest with consistent > register values. Isn't that a change in the userspace ABI? You are now creating an explicit ordering between the write to CTR_EL0 and the rest of the cache hierarchy registers. It has the obvious capacity to lead to the wrong result in a silent way... > > Signed-off-by: Sebastian Ott > --- > arch/arm64/kvm/sys_regs.c | 72 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 64 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4d29b1a0842d..b0ba292259f9 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1874,6 +1874,55 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > return true; > } > > +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd) > +{ > + vcpu->kvm->arch.ctr_el0 = 0; > + return kvm_get_ctr_el0(vcpu->kvm); I'd expect the cached value to be reset instead of being set to 0. What are you achieving by this? > +} > + > +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 *val) > +{ > + *val = kvm_get_ctr_el0(vcpu->kvm); > + return 0; > +} > + > +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding); > + > +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 val) > +{ > + u64 host_val = read_sanitised_ftr_reg(SYS_CTR_EL0); > + u64 old_val = kvm_get_ctr_el0(vcpu->kvm); > + const struct sys_reg_desc *clidr_el1; > + int ret; > + > + if (val == old_val) > + return 0; > + > + if (kvm_vm_has_ran_once(vcpu->kvm)) > + return -EBUSY; > + > + mutex_lock(&vcpu->kvm->arch.config_lock); > + ret = arm64_check_features(vcpu, rd, val); > + if (ret) { > + mutex_unlock(&vcpu->kvm->arch.config_lock); > + return ret; > + } > + if (val != host_val) > + vcpu->kvm->arch.ctr_el0 = val; > + else > + vcpu->kvm->arch.ctr_el0 = 0; > + > + mutex_unlock(&vcpu->kvm->arch.config_lock); > + > + clidr_el1 = get_sys_reg_desc(SYS_CLIDR_EL1); > + if (clidr_el1) > + clidr_el1->reset(vcpu, clidr_el1); > + > + return 0; No check against what can be changed, and in what direction? You seem to be allowing a guest to migrate from a host with IDC==1 to one where IDC==0 (same for DIC). How can that work? Same for the cache lines, which can be larger on the target... How will the guest survive that? > +} > + > static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, > const struct sys_reg_desc *r) > { > @@ -2460,7 +2509,11 @@ static const struct sys_reg_desc sys_reg_descs[] = { > { SYS_DESC(SYS_CCSIDR2_EL1), undef_access }, > { SYS_DESC(SYS_SMIDR_EL1), undef_access }, > { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 }, > - { SYS_DESC(SYS_CTR_EL0), access_ctr }, > + { SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr, > + .get_user = get_ctr, .set_user = set_ctr, .val = (CTR_EL0_DIC_MASK | > + CTR_EL0_IDC_MASK | > + CTR_EL0_DminLine_MASK | > + CTR_EL0_IminLine_MASK)}, > { SYS_DESC(SYS_SVCR), undef_access }, > > { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr, > @@ -3623,6 +3676,13 @@ static bool index_to_params(u64 id, struct sys_reg_params *params) > } > } > > +static const struct sys_reg_desc *get_sys_reg_desc(u32 encoding) > +{ > + struct sys_reg_params params = encoding_to_params(encoding); > + > + return find_reg(¶ms, sys_reg_descs, ARRAY_SIZE(sys_reg_descs)); > +} > + > const struct sys_reg_desc *get_reg_by_id(u64 id, > const struct sys_reg_desc table[], > unsigned int num) > @@ -3676,18 +3736,11 @@ FUNCTION_INVARIANT(midr_el1) > FUNCTION_INVARIANT(revidr_el1) > FUNCTION_INVARIANT(aidr_el1) > > -static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) > -{ > - ((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0); > - return ((struct sys_reg_desc *)r)->val; > -} > - > /* ->val is filled in by kvm_sys_reg_table_init() */ > static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = { > { SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 }, > { SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 }, > { SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 }, > - { SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 }, > }; > > static int get_invariant_sys_reg(u64 id, u64 __user *uaddr) > @@ -4049,6 +4102,9 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > vcpu->arch.hcrx_el2 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2); > } > > + if (vcpu->kvm->arch.ctr_el0) > + vcpu->arch.hcr_el2 |= HCR_TID2; Why trap CTR_EL0 if the values are the same as the host? I really dislike the use of the value 0 as a such an indication. Why isn't this grouped with the traps in vcpu_reset_hcr()? Thanks, 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