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 722A8C3DA64 for ; Wed, 31 Jul 2024 11:30:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Kltrv4SFmYJwpvxk9cw+cBfX/Y1AQIT2pTvuDcYYLLk=; b=dz/YxcXwpqnItRFZvc0z37jGap TvlBt5Od8NyEWBDs9xcSCi6or2osOMKMS1SxwiA3K6uuRRGWKhYLwzhT1o7bJtonQqVi4lwhlxoin AsEImeKyK5JXdynE4fyWwVwtcKg5ZP8YyRFQiugHqFwOUMfTAerkXxYKrsmsb1DR9BQ8vBr5KQqcr BHu01YHQRwxRi14ElPnGX6IEZgR/fXB0fILul7JB7vVDKJCkPvC1cOkKo7WE2B6LRzP768DhqCWeB mucWq5EGjQB0sqqJajWFFEgQeLWjBXTUidBgoQjRwtFu12+VZSeAhfPe6NExnFEXLwY/CJEbzFVaT +zQca1ug==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZ7XZ-00000000uWb-2Bes; Wed, 31 Jul 2024 11:30:45 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sZ6PK-00000000hQz-09tu for linux-arm-kernel@lists.infradead.org; Wed, 31 Jul 2024 10:18:11 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 5BA9A62216; Wed, 31 Jul 2024 10:18:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0E7F0C116B1; Wed, 31 Jul 2024 10:18:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722421089; bh=0qWIYRSbQPUoLX7x3NLoVeGv/6AMKewMa0CxmLbwjGA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=RtAhcwgfOs1c+Wd4kjJEpRMtwSlM4r2m7nja3xFNeD2t5KCtoIbIlHJPNIJu5jtIE HKV9Pk7S8MhCPEWI0FgpuST0ElXcyzczs8aJ40F6lbBUPC8FwrRd9kaCp6jRKdRvfw l1NJxGWPPhGQf4vEPSjkK3VCssqgnt/SaX2tVtI3OWWsVZW3k8M+VsqfZ1URVbv6lr j055E6hjLylLkPL9Bb9QnJ6lxtaHq8NLVoAV+nSYvWA43Oh4jOhPVLvX8U3p3DaFBe RYlUnjSHE2g+ecamim2IuYHBSIRRhJJK2+EvgKRZ8gdPRhjKfRzvVKSRUoRdHi8OF6 Vyt3ai5En1f2A== 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 1sZ6PG-00Gx5v-VO; Wed, 31 Jul 2024 11:18:07 +0100 Date: Wed, 31 Jul 2024 11:18:06 +0100 Message-ID: <86ttg527c1.wl-maz@kernel.org> From: Marc Zyngier To: Alexandru Elisei Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Joey Gouly Subject: Re: [PATCH 10/12] KVM: arm64: nv: Add SW walker for AT S1 emulation In-Reply-To: References: <20240625133508.259829-1-maz@kernel.org> <20240708165800.1220065-1-maz@kernel.org> <86v80m0wlb.wl-maz@kernel.org> 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.3 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, joey.gouly@arm.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-20240731_031810_234185_4C4E9F49 X-CRM114-Status: GOOD ( 44.10 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 31 Jul 2024 10:53:14 +0100, Alexandru Elisei wrote: > > Hi, > > On Wed, Jul 31, 2024 at 09:55:28AM +0100, Marc Zyngier wrote: > > On Mon, 29 Jul 2024 16:26:00 +0100, > > Alexandru Elisei wrote: > > > > > > Hi Marc, > > > > > > On Mon, Jul 08, 2024 at 05:57:58PM +0100, Marc Zyngier wrote: > > > > In order to plug the brokenness of our current AT implementation, > > > > we need a SW walker that is going to... err.. walk the S1 tables > > > > and tell us what it finds. > > > > > > > > Of course, it builds on top of our S2 walker, and share similar > > > > concepts. The beauty of it is that since it uses kvm_read_guest(), > > > > it is able to bring back pages that have been otherwise evicted. > > > > > > > > This is then plugged in the two AT S1 emulation functions as > > > > a "slow path" fallback. I'm not sure it is that slow, but hey. > > > > > > > > Signed-off-by: Marc Zyngier > > > > --- > > > > arch/arm64/kvm/at.c | 538 ++++++++++++++++++++++++++++++++++++++++++-- > > > > 1 file changed, 520 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c > > > > index 71e3390b43b4c..8452273cbff6d 100644 > > > > --- a/arch/arm64/kvm/at.c > > > > +++ b/arch/arm64/kvm/at.c > > > > @@ -4,9 +4,305 @@ > > > > * Author: Jintack Lim > > > > */ > > > > > > > > +#include > > > > + > > > > +#include > > > > #include > > > > #include > > > > > > > > +struct s1_walk_info { > > > > + u64 baddr; > > > > + unsigned int max_oa_bits; > > > > + unsigned int pgshift; > > > > + unsigned int txsz; > > > > + int sl; > > > > + bool hpd; > > > > + bool be; > > > > + bool nvhe; > > > > + bool s2; > > > > +}; > > > > + > > > > +struct s1_walk_result { > > > > + union { > > > > + struct { > > > > + u64 desc; > > > > + u64 pa; > > > > + s8 level; > > > > + u8 APTable; > > > > + bool UXNTable; > > > > + bool PXNTable; > > > > + }; > > > > + struct { > > > > + u8 fst; > > > > + bool ptw; > > > > + bool s2; > > > > + }; > > > > + }; > > > > + bool failed; > > > > +}; > > > > + > > > > +static void fail_s1_walk(struct s1_walk_result *wr, u8 fst, bool ptw, bool s2) > > > > +{ > > > > + wr->fst = fst; > > > > + wr->ptw = ptw; > > > > + wr->s2 = s2; > > > > + wr->failed = true; > > > > +} > > > > + > > > > +#define S1_MMU_DISABLED (-127) > > > > + > > > > +static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi, > > > > + struct s1_walk_result *wr, const u64 va, const int el) > > > > +{ > > > > + u64 sctlr, tcr, tg, ps, ia_bits, ttbr; > > > > + unsigned int stride, x; > > > > + bool va55, tbi; > > > > + > > > > + wi->nvhe = el == 2 && !vcpu_el2_e2h_is_set(vcpu); > > > > > > Where 'el' is computed in handle_at_slow() as: > > > > > > /* > > > * We only get here from guest EL2, so the translation regime > > > * AT applies to is solely defined by {E2H,TGE}. > > > */ > > > el = (vcpu_el2_e2h_is_set(vcpu) && > > > vcpu_el2_tge_is_set(vcpu)) ? 2 : 1; > > > > > > I think 'nvhe' will always be false ('el' is 2 only when E2H is > > > set). > > > > Yeah, there is a number of problems here. el should depend on both the > > instruction (some are EL2-specific) and the HCR control bits. I'll > > tackle that now. > > Yeah, also noticed that how sctlr, tcr and ttbr are chosen in setup_s1_walk() > doesn't look quite right for the nvhe case. Are you sure? Assuming the 'el' value is correct (and I think I fixed that on my local branch), they seem correct to me (we check for va55 early in the function to avoid an later issue). Can you point out what exactly fails in that logic? > > > > > > I'm curious about what 'el' represents. The translation regime for the AT > > > instruction? > > > > Exactly that. > > Might I make a suggestion here? I was thinking about dropping the (el, wi-nvhe*) > tuple to represent the translation regime and have a wi->regime (or similar) to > unambiguously encode the regime. The value can be an enum with three values to > represent the three possible regimes (REGIME_EL10, REGIME_EL2, REGIME_EL20). I've been thinking of that, but I'm wondering whether that just results in pretty awful code in the end, because we go from 2 cases (el==1 or el==2) to 3. But most of the time, we don't care about the E2H=0 case, because we can handle it just like E2H=1. I'll give it a go and see what it looks like. Thanks, M. -- Without deviation from the norm, progress is not possible.