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

[Marc Zyngier <maz@kernel.org>]

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...

>  };
>  
>  struct kvm_arch_memory_slot {
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index e74b90dcfac4..233e65522716 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -78,6 +78,8 @@ extern void kvm_s2_mmu_iterate_by_vmid(struct kvm *kvm, u16 vmid,
>  extern void kvm_vcpu_load_hw_mmu(struct kvm_vcpu *vcpu);
>  extern void kvm_vcpu_put_hw_mmu(struct kvm_vcpu *vcpu);
>  
> +extern void check_nested_vcpu_requests(struct kvm_vcpu *vcpu);
> +
>  struct kvm_s2_trans {
>  	phys_addr_t output;
>  	unsigned long block_size;
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a0d01c46e408..c7b659604bae 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1031,6 +1031,8 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
>  
>  		if (kvm_dirty_ring_check_request(vcpu))
>  			return 0;
> +
> +		check_nested_vcpu_requests(vcpu);
>  	}
>  
>  	return 1;
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 58d3b998793a..71b96eaf6632 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -632,9 +632,9 @@ static struct kvm_s2_mmu *get_s2_mmu_nested(struct kvm_vcpu *vcpu)
>  	/* Set the scene for the next search */
>  	kvm->arch.nested_mmus_next = (i + 1) % kvm->arch.nested_mmus_size;
>  
> -	/* Clear the old state */
> +	/* Make sure we don't forget to do the laundry */
>  	if (kvm_s2_mmu_valid(s2_mmu))
> -		kvm_stage2_unmap_range(s2_mmu, 0, kvm_phys_size(s2_mmu), false);
> +		s2_mmu->pending_unmap = true;
>  
>  	/*
>  	 * The virtual VMID (modulo CnP) will be used as a key when matching
> @@ -650,6 +650,9 @@ static struct kvm_s2_mmu *get_s2_mmu_nested(struct kvm_vcpu *vcpu)
>  
>  out:
>  	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.

If you don't mind, I'd like to add a comment to that effect, so that
our future selves remember what this is about.

And move the pending_unmap field in the process.

Thanks,

	M.

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