Re: [PATCH v2 2/5] KVM: arm64: nvhe: Synchronise with page table walker on TLBI

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2 2/5] KVM: arm64: nvhe: Synchronise with page table walker on TLBI
Date: Thu, 13 Apr 2023 08:37:23 +0100	[thread overview]
Message-ID: <86leiwkzbg.wl-maz@kernel.org> (raw)
In-Reply-To: <ZDdIMMv7jhmSqMNi@linux.dev>

On Thu, 13 Apr 2023 01:09:20 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Marc,
> 
> On Sat, Apr 08, 2023 at 05:04:24PM +0100, Marc Zyngier wrote:
> > A TLBI from EL2 impacting EL1 involves messing with the EL1&0
> > translation regime, and the page table walker may still be
> > performing speculative walks.
> > 
> > Piggyback on the existing DSBs to always have a DSB ISH that
> > will synchronise all load/store operations that the PTW may
> > still have.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/tlb.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/tlb.c b/arch/arm64/kvm/hyp/nvhe/tlb.c
> > index d296d617f589..e86dd04d49ff 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/tlb.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/tlb.c
> > @@ -17,6 +17,23 @@ struct tlb_inv_context {
> >  static void __tlb_switch_to_guest(struct kvm_s2_mmu *mmu,
> >  				  struct tlb_inv_context *cxt)
> >  {
> > +	/*
> > +	 * We have two requirements:
> > +	 *
> > +	 * - ensure that the page table updates are visible to all
> > +         *   CPUs, for which a dsb(ishst) is what we need
> > +	 *
> > +	 * - complete any speculative page table walk started before
> > +         *   we trapped to EL2 so that we can mess with the MM
> > +         *   registers out of context, for which dsb(nsh) is enough
> > +	 *
> > +	 * The composition of these two barriers is a dsb(ish). This
> > +	 * might be slightly over the top for non-shareable TLBIs, but
> > +	 * they are so vanishingly rare that it isn't worth the
> > +	 * complexity.
> > +	 */
> > +	dsb(ish);
> > +
> 
> Ricardo is carrying a patch for non-shareable TLBIs on permission
> relaxation [*], and he's found that it produces some rather desirable
> performance improvements. I appreciate the elegance of your approach,
> but given what's coming does it make sense to have the TLBI handlers
> continue to explicitly perform the appropriate DSB?

Ah, I forgot about my own patch! :D

Right, let me see if I can do something clever here...

Thanks,
	M.

-- 
Without deviation from the norm, progress is not possible.