Re: [PATCH v2 21/30] KVM: arm64: Kill topup_memcache from kvm_s2_fault

[Marc Zyngier <maz@kernel.org>]

On Fri, 27 Mar 2026 11:36:09 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> The topup_memcache field can be easily replaced by the equivalent
> conditions, and the resulting code is not much worse.
> 
> Tested-by: Fuad Tabba <tabba@google.com>
> Reviewed-by: Fuad Tabba <tabba@google.com>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/mmu.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index e8bda71e862b2..5b05caecdbd92 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1712,7 +1712,6 @@ static short kvm_s2_resolve_vma_size(const struct kvm_s2_fault_desc *s2fd,
>  
>  struct kvm_s2_fault {
>  	bool writable;
> -	bool topup_memcache;
>  	bool mte_allowed;
>  	bool is_vma_cacheable;
>  	bool s2_force_noncacheable;
> @@ -1983,9 +1982,8 @@ static int user_mem_abort(const struct kvm_s2_fault_desc *s2fd)
>  		.logging_active = logging_active,
>  		.force_pte = logging_active,
>  		.prot = KVM_PGTABLE_PROT_R,
> -		.topup_memcache = !perm_fault || (logging_active && kvm_is_write_fault(s2fd->vcpu)),
>  	};
> -	void *memcache;
> +	void *memcache = NULL;
>  	int ret;
>  
>  	/*
> @@ -1994,9 +1992,11 @@ static int user_mem_abort(const struct kvm_s2_fault_desc *s2fd)
>  	 * only exception to this is when dirty logging is enabled at runtime
>  	 * and a write fault needs to collapse a block entry into a table.
>  	 */
> -	ret = prepare_mmu_memcache(s2fd->vcpu, fault.topup_memcache, &memcache);
> -	if (ret)
> -		return ret;
> +	if (!perm_fault || (logging_active && kvm_is_write_fault(s2fd->vcpu))) {
> +		ret = prepare_mmu_memcache(s2fd->vcpu, true, &memcache);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * Let's check if we will get back a huge page backed by hugetlbfs, or

Sashiko has spotted [1] an interesting corner case here, which is that the
original code always initialises memcache to its correct value, while
we now only do it in a limited number of cases.

I'm proposing to restore the original behaviour by folding the
following change into this patch, splitting the retrieval of the
memcache pointer from the top-up and avoiding the ugly pointer
indirection:

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 1fe7182be45ac..03e1f389339c7 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1513,25 +1513,22 @@ static bool kvm_vma_is_cacheable(struct vm_area_struct *vma)
 	}
 }
 
-static int prepare_mmu_memcache(struct kvm_vcpu *vcpu, bool topup_memcache,
-				void **memcache)
+static void *get_mmu_memcache(struct kvm_vcpu *vcpu)
 {
-	int min_pages;
-
 	if (!is_protected_kvm_enabled())
-		*memcache = &vcpu->arch.mmu_page_cache;
+		return &vcpu->arch.mmu_page_cache;
 	else
-		*memcache = &vcpu->arch.pkvm_memcache;
-
-	if (!topup_memcache)
-		return 0;
+		return &vcpu->arch.pkvm_memcache;
+}
 
-	min_pages = kvm_mmu_cache_min_pages(vcpu->arch.hw_mmu);
+static int topup_mmu_memcache(struct kvm_vcpu *vcpu, void *memcache)
+{
+	int min_pages = kvm_mmu_cache_min_pages(vcpu->arch.hw_mmu);
 
 	if (!is_protected_kvm_enabled())
-		return kvm_mmu_topup_memory_cache(*memcache, min_pages);
+		return kvm_mmu_topup_memory_cache(memcache, min_pages);
 
-	return topup_hyp_memcache(*memcache, min_pages);
+	return topup_hyp_memcache(memcache, min_pages);
 }
 
 /*
@@ -1589,7 +1586,8 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
 	gfn_t gfn;
 	int ret;
 
-	ret = prepare_mmu_memcache(s2fd->vcpu, true, &memcache);
+	memcache = get_mmu_memcache(s2fd->vcpu);
+	ret = topup_mmu_memcache(s2fd->vcpu, memcache);
 	if (ret)
 		return ret;
 
@@ -1993,7 +1991,7 @@ static int user_mem_abort(const struct kvm_s2_fault_desc *s2fd)
 	bool perm_fault = kvm_vcpu_trap_is_permission_fault(s2fd->vcpu);
 	struct kvm_s2_fault_vma_info s2vi = {};
 	enum kvm_pgtable_prot prot;
-	void *memcache = NULL;
+	void *memcache;
 	int ret;
 
 	/*
@@ -2002,9 +2000,10 @@ static int user_mem_abort(const struct kvm_s2_fault_desc *s2fd)
 	 * only exception to this is when dirty logging is enabled at runtime
 	 * and a write fault needs to collapse a block entry into a table.
 	 */
+	memcache = get_mmu_memcache(s2fd->vcpu);
 	if (!perm_fault || (memslot_is_logging(s2fd->memslot) &&
 			    kvm_is_write_fault(s2fd->vcpu))) {
-		ret = prepare_mmu_memcache(s2fd->vcpu, true, &memcache);
+		ret = topup_mmu_memcache(s2fd->vcpu, memcache);
 		if (ret)
 			return ret;
 	}

The bot has also pointed out a couple of cases where memcache and
permission faults interact badly. I'll look into them separately, as
they predate this rework.

Thanks,

	M.

[1] https://sashiko.dev/#/patchset/20260327113618.4051534-1-maz%40kernel.org?patch=12134

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