Re: [PATCH v1 5/6] KVM: arm64: Use kvm_s2_fault_vma_info in gmem_abort()

[Marc Zyngier <maz@kernel.org>]

On Fri, 25 Sep 2026 18:36:42 +0100,
Wei-Lin Chang <weilin.chang@arm.com> wrote:
> 
> On Tue, Sep 22, 2026 at 02:08:20PM +0100, Vincent Donnefort wrote:
> > From: Fuad Tabba <fuad.tabba@linux.dev>
> > 
> > gmem_abort() maps at s2fd->fault_ipa, which HPFAR_EL2 holds at 4K
> > granularity whatever the page size. The generic page-table code aligns
> > it, but pkvm_pgtable_stage2_map() looks up existing mappings over
> > [addr, addr + size), so on a pKVM host with pages larger than 4K a
> > guest_memfd-backed guest that faults past the first 4K of a page can
> > find its neighbour's mapping, get -EAGAIN and take the same fault
> > forever.
> > 
> > Take the addresses from kvm_s2_fault_vma_info instead, as
> > user_mem_abort() does, and report the gfn kvm_gmem_get_pfn() failed on
> > in the memory fault exit. kvm_s2_fault_get_vma_info() itself isn't
> > called: a guest_memfd memslot's userspace_addr doesn't need to be
> > backed by a VMA.
> > 
> > Fixes: a7b57e0995927 ("KVM: arm64: Handle guest_memfd-backed guest page faults")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/r/20260913175105.A57AC1F000FF@smtp.kernel.org
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> > ---
> >  arch/arm64/kvm/mmu.c | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 5aac624b0442..ee0fcca838c1 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1953,11 +1953,9 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
> >  	enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_SHARED;
> >  	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
> >  	struct kvm_pgtable *pgt = s2fd->vcpu->arch.hw_mmu->pgt;
> > -	unsigned long mmu_seq;
> > -	struct page *page;
> > +	struct kvm_s2_fault_vma_info s2vi = {};
> 
> I'd say it's unfortunate this struct's name contains vma :P

If that was the only thing that is wrong in the MMU code, we'd be in a
pretty good position! ;-)

> 
> >  	struct kvm *kvm = s2fd->vcpu->kvm;
> >  	void *memcache = NULL;
> > -	kvm_pfn_t pfn;
> >  	gfn_t gfn;
> >  	int ret;
> >  
> > @@ -1968,23 +1966,22 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
> >  			return ret;
> >  	}
> >  
> > -	if (s2fd->nested)
> > -		gfn = kvm_s2_trans_output(s2fd->nested) >> PAGE_SHIFT;
> > -	else
> > -		gfn = s2fd->fault_ipa >> PAGE_SHIFT;
> > +	s2vi.vma_pagesize = PAGE_SIZE;
> > +	s2vi.gfn = ALIGN_DOWN(s2fd->fault_ipa, s2vi.vma_pagesize) >> PAGE_SHIFT;
> > +	gfn = get_canonical_gfn(s2fd, &s2vi);
> 
> Perhaps it is worth reserving plain "gfn" for the value we use as the
> IA of the (shadow) page table, and name this variable something like
> canonical_gfn. Similar to what kvm_s2_fault_map() does.

Maybe. We have gfn in the vncr fault handling code as well, but that
one is the result of a S1 walk, so maybe more obvious.

> 
> >  
> >  	write_fault = kvm_is_write_fault(s2fd->vcpu);
> >  	exec_fault = kvm_vcpu_trap_is_exec_fault(s2fd->vcpu);
> >  
> >  	VM_WARN_ON_ONCE(write_fault && exec_fault);
> >  
> > -	mmu_seq = kvm->mmu_invalidate_seq;
> > +	s2vi.mmu_seq = kvm->mmu_invalidate_seq;
> >  	/* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */
> >  	smp_rmb();
> >  
> > -	ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, gfn, &pfn, &page, NULL);
> > +	ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, gfn, &s2vi.pfn, &s2vi.page, NULL);
> >  	if (ret) {
> > -		kvm_prepare_memory_fault_exit(s2fd->vcpu, s2fd->fault_ipa, PAGE_SIZE,
> > +		kvm_prepare_memory_fault_exit(s2fd->vcpu, gfn_to_gpa(gfn), s2vi.vma_pagesize,
> 
> Huh, I guess you also fixed a bug here? s2fd->fault_ipa could be the
> nested ipa instead of the canonical ipa.

Yeah, it appears so. Worth capturing in Fixes: as well.

	M.

-- 
Jazz isn't dead. It just smells funny.