Re: [PATCH v2 3/4] KVM: arm64: nv: Punt stage-2 recycling to a vCPU request

[Oliver Upton <oliver.upton@linux.dev>]

On Mon, Oct 07, 2024 at 08:06:30PM +0100, Marc Zyngier wrote:
> On Mon, 07 Oct 2024 17:42:54 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Currently, when a nested MMU is repurposed for some other MMU context,
> > KVM unmaps everything during vcpu_load() while holding the MMU lock for
> > write. This is quite a performance bottleneck for large nested VMs, as
> > all vCPU scheduling will spin until the unmap completes.
> > 
> > Start punting the MMU cleanup to a vCPU request, where it is then
> > possible to periodically release the MMU lock and CPU in the presence of
> > contention.
> > 
> > Ensure that no vCPU winds up using a stale MMU by tracking the pending
> > unmap on the S2 MMU itself and requesting an unmap on every vCPU that
> > finds it.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/include/asm/kvm_host.h   |  3 +++
> >  arch/arm64/include/asm/kvm_nested.h |  2 ++
> >  arch/arm64/kvm/arm.c                |  2 ++
> >  arch/arm64/kvm/nested.c             | 21 +++++++++++++++++++--
> >  4 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 329619c6fa96..16b4b0003e7d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -51,6 +51,7 @@
> >  #define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
> >  #define KVM_REQ_SUSPEND		KVM_ARCH_REQ(6)
> >  #define KVM_REQ_RESYNC_PMU_EL0	KVM_ARCH_REQ(7)
> > +#define KVM_REQ_NESTED_S2_UNMAP	KVM_ARCH_REQ(8)
> >  
> >  #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
> >  				     KVM_DIRTY_LOG_INITIALLY_SET)
> > @@ -216,6 +217,8 @@ struct kvm_s2_mmu {
> >  	 * >0: Somebody is actively using this.
> >  	 */
> >  	atomic_t refcnt;
> > +
> > +	bool	pending_unmap;
> 
> Nit: move this up next to nested_stage2_enabled, saving a grand total
> of 4 bytes...

/facepalm I had it that way in some WIP flavor of this series, looks
like I dropped that diff. Will do.

> >  	atomic_inc(&s2_mmu->refcnt);
> > +	if (s2_mmu->pending_unmap)
> > +		kvm_make_request(KVM_REQ_NESTED_S2_UNMAP, vcpu);
> > +
> 
> This bit is awesomely subtle. Any vcpu incrementing the refcount on an
> MMU being laundered gets to partake in the cleaning process. Without
> this, the second guy would get to use the MMU before it has been
> cleaned-up.
> 

How about:

	/*
	 * Set the vCPU request to perform an unmap, even if the pending
	 * unmap originates from another vCPU. This guarantees that the
	 * MMU has been completely unmapped before any vCPU actually
	 * uses it, and allows multiple vCPUs to lend a hand with
	 * completing the unmap.
	 */
	if (s2_mmu->pending_unmap)
		kvm_make_request(KVM_REQ_NESTED_S2_UNMAP, vcpu);

-- 
Thanks,
Oliver