From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 895081CF93; Wed, 31 Jul 2024 10:18:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722421089; cv=none; b=ELGxZz394OITFHKZ54GiLMbBdBkWYNIzZdDJjfNaD8iX97BA/HHEZtRkJcir7WsX7FEUKGNNXUTnPkuNSA5uRvYSyGh4PMWkKt4OOM0SxYc8BcyFgKEu5P2Zn/CA2Rh6g8NL/pciNS6Z5kNwiRUHYEC2X0pXADJUpQ0pSBh1Bnk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722421089; c=relaxed/simple; bh=0qWIYRSbQPUoLX7x3NLoVeGv/6AMKewMa0CxmLbwjGA=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=ulBYukdwXLnP9A06ytEdXtKiWgKFulMB+dR7uUMCVORnKYGtltp/hh3bXGw/s3IJBTnwLL5kDJPaVvs1DdY5vmLZhR3yjiKDY1tbTW3yV1DKUGlyFKaUfOAZkFRNt+NbqinpT6B/98fBRRa2OR8/3yT31lCf/fwS2SS9EEyAry4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=RtAhcwgf; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="RtAhcwgf" 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) Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: 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 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.