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 1AFB21879 for ; Wed, 31 May 2023 08:46:29 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BC363C4339B; Wed, 31 May 2023 08:46:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685522789; bh=JMuzqmUBJoA/r5J0yK+sZwXSCkPl1O+bxFArwcnvsW0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=fDIwkCIj5dA38T2YdLDzsOvIZHNL6F8EXwkkFaKnevX4oe6gQsFkc+LVPhVu2rpTE ZvHE8ISZCUhzsCQoyWDGDaboDQXQD+zyJIf4vNYmFvR7ra/26EuOo6BCBbZT6r82ob 2FlMJHlnifboxOAQZ+D0EfBUbBU/p1uUdA/Sc1rxjzE7GVyTT7qg8MKDTiQOyPF706 8/nRVcvAVD1hnFNWkjCoTB1WwhCdzLPOKfd7HsDdFCYZddmiFeiNyQBhAfRXx4wSXk ozzW9dKVH0Fc3kewf0I9870I7htmRc+rw8QhnJjI6NqxRXqQl0oKtey9ZRrnwa5wTS 2KId/sKFf7LTg== 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 1q4HTP-001Z0N-JX; Wed, 31 May 2023 09:46:27 +0100 Date: Wed, 31 May 2023 09:46:27 +0100 Message-ID: <86zg5kc2ho.wl-maz@kernel.org> From: Marc Zyngier To: Raghavendra Rao Ananta Cc: Oliver Upton , James Morse , Suzuki K Poulose , Ricardo Koller , Paolo Bonzini , Jing Zhang , Colton Lewis , linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH v4 3/6] KVM: arm64: Implement kvm_arch_flush_remote_tlbs_range() In-Reply-To: References: <20230519005231.3027912-1-rananta@google.com> <20230519005231.3027912-4-rananta@google.com> <87v8gbjkzn.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/28.2 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: rananta@google.com, oliver.upton@linux.dev, james.morse@arm.com, suzuki.poulose@arm.com, ricarkol@google.com, pbonzini@redhat.com, jingzhangos@google.com, coltonlewis@google.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Tue, 30 May 2023 22:22:23 +0100, Raghavendra Rao Ananta wrote: >=20 > On Mon, May 29, 2023 at 7:00=E2=80=AFAM Marc Zyngier wro= te: > > > > On Fri, 19 May 2023 01:52:28 +0100, > > Raghavendra Rao Ananta wrote: > > > > > > Implement kvm_arch_flush_remote_tlbs_range() for arm64 > > > to invalidate the given range in the TLB. > > > > > > Signed-off-by: Raghavendra Rao Ananta > > > --- > > > arch/arm64/include/asm/kvm_host.h | 3 +++ > > > arch/arm64/kvm/hyp/nvhe/tlb.c | 4 +--- > > > arch/arm64/kvm/mmu.c | 11 +++++++++++ > > > 3 files changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/a= sm/kvm_host.h > > > index 81ab41b84f436..343fb530eea9c 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -1081,6 +1081,9 @@ struct kvm *kvm_arch_alloc_vm(void); > > > #define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > > > int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > > > > > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS_RANGE > > > +int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t start_gf= n, u64 pages); > > > + > > > static inline bool kvm_vm_is_protected(struct kvm *kvm) > > > { > > > return false; > > > diff --git a/arch/arm64/kvm/hyp/nvhe/tlb.c b/arch/arm64/kvm/hyp/nvhe/= tlb.c > > > index d4ea549c4b5c4..d2c7c1bc6d441 100644 > > > --- a/arch/arm64/kvm/hyp/nvhe/tlb.c > > > +++ b/arch/arm64/kvm/hyp/nvhe/tlb.c > > > @@ -150,10 +150,8 @@ void __kvm_tlb_flush_vmid_range(struct kvm_s2_mm= u *mmu, > > > return; > > > } > > > > > > - dsb(ishst); > > > - > > > /* Switch to requested VMID */ > > > - __tlb_switch_to_guest(mmu, &cxt); > > > + __tlb_switch_to_guest(mmu, &cxt, false); > > > > This hunk is in the wrong patch, isn't it? > > > Ah, you are right. It should be part of the previous patch. I think I > introduced it accidentally when I rebased the series. I'll remove it > in the next spin. >=20 >=20 > > > > > > __flush_tlb_range_op(ipas2e1is, start, pages, stride, 0, 0, fal= se); > > > > > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > > index d0a0d3dca9316..e3673b4c10292 100644 > > > --- a/arch/arm64/kvm/mmu.c > > > +++ b/arch/arm64/kvm/mmu.c > > > @@ -92,6 +92,17 @@ int kvm_arch_flush_remote_tlbs(struct kvm *kvm) > > > return 0; > > > } > > > > > > +int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm, gfn_t start_gf= n, u64 pages) > > > +{ > > > + phys_addr_t start, end; > > > + > > > + start =3D start_gfn << PAGE_SHIFT; > > > + end =3D (start_gfn + pages) << PAGE_SHIFT; > > > + > > > + kvm_call_hyp(__kvm_tlb_flush_vmid_range, &kvm->arch.mmu, start,= end); > > > > So that's the point that I think is not right. It is the MMU code that > > should drive the invalidation method, and not the HYP code. The HYP > > code should be as dumb as possible, and the logic should be kept in > > the MMU code. > > > > So when a range invalidation is forwarded to HYP, it's a *valid* range > > invalidation. not something that can fallback to VMID-wide invalidation. > > > I'm guessing that you are referring to patch-2. Do you recommend > moving the 'pages >=3D MAX_TLBI_RANGE_PAGES' logic here and simply > return an error? How about for the other check: > system_supports_tlb_range()? > The idea was for __kvm_tlb_flush_vmid_range() to also implement a > fallback mechanism in case the system doesn't support the range-based > instructions. But if we end up calling __kvm_tlb_flush_vmid_range() > from multiple cases, we'd end up duplicating the checks. WDYT? My take is that there should be a single helper deciding to issue either a number of range-based TLBIs depending on start/end, or a single VMID-based TLBI. Having multiple calling sites is not a problem, and even if that code gets duplicated, big deal. But a hypercall that falls back to global invalidation based on a range evaluation error (more than MAX_TLBI_RANGE_PAGES) is papering over a latent bug. There should be no logic whatsoever in any of the two tlb.c files. Only a switch to the correct context, and the requested invalidation, which *must* be architecturally correct. Thanks, M. --=20 Without deviation from the norm, progress is not possible.