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 1A369C47258 for ; Tue, 23 Jan 2024 17:45: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=B2QlJnemokhsFDAudS1hX7i8d4L22GEDXVdVUlRBzd0=; b=XdMPahCLrevFAJ lsq3BQOEFXslzb6K6Qdfd/Xvl5jurCzZ8byWXEoEDnuFjVKxBTtpC95x9Ctr4cUjjWYSlVE684lri 3UiHCmLJtPw1cKR+KLK+fmfWQ1anpybz1klL8i9q8C6QLkWsyn6lbljgdv1Pjq/xH/lqvsUpJdVag pgL/i3A569IAmZPmr1/Bbvpnw3JC8+r4Q6DrX3lORHAacAoHf6weRjkNg9/VKm6tSV7OL8UvEJTC0 1cGWEjvJyAeN296+s6/8BUXsEUBGDXkQ7BNJvJXmQgvpsAFhiEW04NzfmrTHRO+Pt2dVHjJNiX7Pb p5yuXK8C9dRbCSrJrnBw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rSKpy-00HYkE-04; Tue, 23 Jan 2024 17:45:26 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rSKpr-00HYiL-2C for linux-arm-kernel@lists.infradead.org; Tue, 23 Jan 2024 17:45:21 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id E2E8FCE2DFC; Tue, 23 Jan 2024 17:45:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 16906C433C7; Tue, 23 Jan 2024 17:45:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706031917; bh=9b0JptKYUL2tLymXQ28zb0b6p+7g/64y/aIArNTTNxk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=CHztmlIWrxS7UyxoTJi24dT1YM4EnUNdgR9wYVJQoPXDvbDie92hkI8RaW1+8Fda/ Vx5ZuMUO9Lf05DIfFj0mjrvtLTupF6cCTrgfjRgNF5n5NyijpJ82opQR8SbXpuckEQ eHe853O8+9SP93aEDxpC7eDLlofxzIjPu6SlRUrss3iulNhIhoZnrIg5NK0gV6gWZE y68+HilHDPuG8Mcu3L2E5A8R1nlfrokEsiSU/Wl9KZYSTMWiQp/mUveEJuXWUaYXOq Nkry+wJT4RFhKbMNOpDzg5Xo4LFfKt+ppnI5gjahgvpAMRyftWHL6PHWQDBL+P9o8B Gw7nTkNQb8oOQ== 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 1rSKpm-00E4UN-M9; Tue, 23 Jan 2024 17:45:14 +0000 Date: Tue, 23 Jan 2024 17:45:14 +0000 Message-ID: <86h6j47xlh.wl-maz@kernel.org> From: Marc Zyngier To: Joey Gouly Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Catalin Marinas , Will Deacon , Mark Brown Subject: Re: [PATCH 10/25] KVM: arm64: nv: Turn encoding ranges into discrete XArray stores In-Reply-To: <20240123163725.GE1283334@e124191.cambridge.arm.com> References: <20240122201852.262057-1-maz@kernel.org> <20240122201852.262057-11-maz@kernel.org> <20240123163725.GE1283334@e124191.cambridge.arm.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.1 (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: joey.gouly@arm.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org, broonie@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-20240123_094520_069564_CBF4A5F3 X-CRM114-Status: GOOD ( 35.26 ) 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 Tue, 23 Jan 2024 16:37:25 +0000, Joey Gouly wrote: > > On Mon, Jan 22, 2024 at 08:18:37PM +0000, Marc Zyngier wrote: > > In order to be able to store different values for member of an > > encoding range, replace xa_store_range() calls with discrete > > xa_store() calls and an encoding iterator. > > > > We end-up using a bit more memory, but we gain some flexibility > > that we will make use of shortly. > > > > Signed-off-by: Marc Zyngier > > --- > > arch/arm64/kvm/emulate-nested.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c > > index ef46c2e45307..59622636b723 100644 > > --- a/arch/arm64/kvm/emulate-nested.c > > +++ b/arch/arm64/kvm/emulate-nested.c > > @@ -1757,6 +1757,28 @@ static __init void print_nv_trap_error(const struct encoding_to_trap_config *tc, > > err); > > } > > > > +static u32 encoding_next(u32 encoding) > > +{ > > + u8 op0, op1, crn, crm, op2; > > + > > + op0 = sys_reg_Op0(encoding); > > + op1 = sys_reg_Op1(encoding); > > + crn = sys_reg_CRn(encoding); > > + crm = sys_reg_CRm(encoding); > > + op2 = sys_reg_Op2(encoding); > > + > > + if (op2 < Op2_mask) > > + return sys_reg(op0, op1, crn, crm, op2 + 1); > > + if (crm < CRm_mask) > > + return sys_reg(op0, op1, crn, crm + 1, 0); > > + if (crn < CRn_mask) > > + return sys_reg(op0, op1, crn + 1, 0, 0); > > + if (op1 < Op1_mask) > > + return sys_reg(op0, op1 + 1, 0, 0, 0); > > + > > + return sys_reg(op0 + 1, 0, 0, 0, 0); > > +} > > I like this function, aesthetically pleasing! Glad you like the colour! :D > > > + > > int __init populate_nv_trap_config(void) > > { > > int ret = 0; > > @@ -1775,13 +1797,8 @@ int __init populate_nv_trap_config(void) > > ret = -EINVAL; > > } > > > > - if (cgt->encoding != cgt->end) { > > - prev = xa_store_range(&sr_forward_xa, > > - cgt->encoding, cgt->end, > > - xa_mk_value(cgt->tc.val), > > - GFP_KERNEL); > > - } else { > > - prev = xa_store(&sr_forward_xa, cgt->encoding, > > + for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) { > > + prev = xa_store(&sr_forward_xa, enc, > > xa_mk_value(cgt->tc.val), GFP_KERNEL); > > if (prev && !xa_is_err(prev)) { > > ret = -EINVAL; > > The error handling looks a bit weird here, the full context: > > for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) { > prev = xa_store(&sr_forward_xa, enc, > xa_mk_value(cgt->tc.val), GFP_KERNEL); > if (prev && !xa_is_err(prev)) { > ret = -EINVAL; > print_nv_trap_error(cgt, "Duplicate CGT", ret); > } > } > > if (xa_is_err(prev)) { > ret = xa_err(prev); > print_nv_trap_error(cgt, "Failed CGT insertion", ret); > } > > I would maybe expect some 'goto's after setting ret? It looks like the ret > would still be returned properly at the end of the function at least. We also > don't check the return value of xa_store() in the encoding_to_fgt loop further > down, which seems worse as that could affect VMs if some encodings failed to be > stored for some reason. The lack of goto is on purpose. Getting the tables right is tedious when you can't collect multiple errors at once and stop on the first one. Which is why I opted for this scheme where 'ret' only gets written on error. However, the error handling is pretty lax indeed. I have this in store, on top of the current patch: diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c index 5c0f81b6e55c..f2cf0fbf27eb 100644 --- a/arch/arm64/kvm/emulate-nested.c +++ b/arch/arm64/kvm/emulate-nested.c @@ -1795,11 +1795,11 @@ int __init populate_nv_trap_config(void) ret = -EINVAL; print_nv_trap_error(cgt, "Duplicate CGT", ret); } - } - if (xa_is_err(prev)) { - ret = xa_err(prev); - print_nv_trap_error(cgt, "Failed CGT insertion", ret); + if (xa_is_err(prev)) { + ret = xa_err(prev); + print_nv_trap_error(cgt, "Failed CGT insertion", ret); + } } } @@ -1812,6 +1812,7 @@ int __init populate_nv_trap_config(void) for (int i = 0; i < ARRAY_SIZE(encoding_to_fgt); i++) { const struct encoding_to_trap_config *fgt = &encoding_to_fgt[i]; union trap_config tc; + void *prev; if (fgt->tc.fgt >= __NR_FGT_GROUP_IDS__) { ret = -EINVAL; @@ -1826,8 +1827,13 @@ int __init populate_nv_trap_config(void) } tc.val |= fgt->tc.val; - xa_store(&sr_forward_xa, fgt->encoding, - xa_mk_value(tc.val), GFP_KERNEL); + prev = xa_store(&sr_forward_xa, fgt->encoding, + xa_mk_value(tc.val), GFP_KERNEL); + + if (xa_is_err(prev)) { + ret = xa_err(prev); + print_nv_trap_error(cgt, "Failed FGT insertion", ret); + } } kvm_info("nv: %ld fine grained trap handlers\n", Completely untested, of course. 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