Re: [PATCH v13 16/20] KVM: arm64: Handle guest_memfd-backed guest page faults

[Marc Zyngier <maz@kernel.org>]

On Fri, 11 Jul 2025 15:17:46 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> On Fri, 11 Jul 2025 at 14:50, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 11 Jul 2025 10:59:39 +0100,
> > "Roy, Patrick" <roypat@amazon.co.uk> wrote:
> > >
> > >
> > > Hi Fuad,
> > >
> > > On Wed, 2025-07-09 at 11:59 +0100, Fuad Tabba wrote:> -snip-
> > > > +#define KVM_PGTABLE_WALK_MEMABORT_FLAGS (KVM_PGTABLE_WALK_HANDLE_FAULT | KVM_PGTABLE_WALK_SHARED)
> > > > +
> > > > +static int gmem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> > > > +                     struct kvm_s2_trans *nested,
> > > > +                     struct kvm_memory_slot *memslot, bool is_perm)
> > > > +{
> > > > +       bool write_fault, exec_fault, writable;
> > > > +       enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_MEMABORT_FLAGS;
> > > > +       enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
> > > > +       struct kvm_pgtable *pgt = vcpu->arch.hw_mmu->pgt;
> > > > +       struct page *page;
> > > > +       struct kvm *kvm = vcpu->kvm;
> > > > +       void *memcache;
> > > > +       kvm_pfn_t pfn;
> > > > +       gfn_t gfn;
> > > > +       int ret;
> > > > +
> > > > +       ret = prepare_mmu_memcache(vcpu, true, &memcache);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       if (nested)
> > > > +               gfn = kvm_s2_trans_output(nested) >> PAGE_SHIFT;
> > > > +       else
> > > > +               gfn = fault_ipa >> PAGE_SHIFT;
> > > > +
> > > > +       write_fault = kvm_is_write_fault(vcpu);
> > > > +       exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
> > > > +
> > > > +       if (write_fault && exec_fault) {
> > > > +               kvm_err("Simultaneous write and execution fault\n");
> > > > +               return -EFAULT;
> > > > +       }
> > > > +
> > > > +       if (is_perm && !write_fault && !exec_fault) {
> > > > +               kvm_err("Unexpected L2 read permission error\n");
> > > > +               return -EFAULT;
> > > > +       }
> > > > +
> > > > +       ret = kvm_gmem_get_pfn(kvm, memslot, gfn, &pfn, &page, NULL);
> > > > +       if (ret) {
> > > > +               kvm_prepare_memory_fault_exit(vcpu, fault_ipa, PAGE_SIZE,
> > > > +                                             write_fault, exec_fault, false);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       writable = !(memslot->flags & KVM_MEM_READONLY);
> > > > +
> > > > +       if (nested)
> > > > +               adjust_nested_fault_perms(nested, &prot, &writable);
> > > > +
> > > > +       if (writable)
> > > > +               prot |= KVM_PGTABLE_PROT_W;
> > > > +
> > > > +       if (exec_fault ||
> > > > +           (cpus_have_final_cap(ARM64_HAS_CACHE_DIC) &&
> > > > +            (!nested || kvm_s2_trans_executable(nested))))
> > > > +               prot |= KVM_PGTABLE_PROT_X;
> > > > +
> > > > +       kvm_fault_lock(kvm);
> > >
> > > Doesn't this race with gmem invalidations (e.g. fallocate(PUNCH_HOLE))?
> > > E.g. if between kvm_gmem_get_pfn() above and this kvm_fault_lock() a
> > > gmem invalidation occurs, don't we end up with stage-2 page tables
> > > refering to a stale host page? In user_mem_abort() there's the "grab
> > > mmu_invalidate_seq before dropping mmap_lock and check it hasnt changed
> > > after grabbing mmu_lock" which prevents this, but I don't really see an
> > > equivalent here.
> >
> > Indeed. We have a similar construct in kvm_translate_vncr() as well,
> > and I'd definitely expect something of the sort 'round here. If for
> > some reason this is not needed, then a comment explaining why would be
> > welcome.
> >
> > But this brings me to another interesting bit: kvm_translate_vncr() is
> > another path that deals with a guest translation fault (despite being
> > caught as an EL2 S1 fault), and calls kvm_faultin_pfn(). What happens
> > when the backing store is gmem? Probably nothin
> 
> I'll add guest_memfd handling logic to kvm_translate_vncr().
> 
> > I don't immediately see why NV and gmem should be incompatible, so
> > something must be done on that front too (including the return to
> > userspace if the page is gone).
> 
> Should it return to userspace or go back to the guest?
> user_mem_abort() returns to the guest if the page disappears (I don't
> quite understand the rationale behind that, but it was a deliberate
> change [1]): on mmu_invalidate_retry() it sets ret to -EAGAIN [2],
> which gets flipped to 0 on returning from user_mem_abort() [3].

Outside of gmem, racing with an invalidation (resulting in -EAGAIN) is
never a problem. We just replay the faulting instruction.  Also,
kvm_faultin_pfn() never fails outside of error cases (guest accessing
non-memory, or writing to RO memory). So returning to the guest is
always the right thing to do, and userspace never needs to see any of
that (I ignore userfaultfd here, as that's a different matter).

With gmem, you don't really have a choice. Whoever is in charge of the
memory told you it can't get to it, and it's only fair to go back to
userspace for it to sort it out (if at all possible).

So when it comes to VNCR faults, the behaviour should be the same,
given that the faulting page *is* a guest page, even if this isn't a
stage-2 mapping that we are dealing with.

I'd expect something along the lines of the hack below, (completely
untested, as usual).

Thanks,

	M.

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index 5b191f4dc5668..98b1d6d4688a6 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -1172,8 +1172,9 @@ static u64 read_vncr_el2(struct kvm_vcpu *vcpu)
 	return (u64)sign_extend64(__vcpu_sys_reg(vcpu, VNCR_EL2), 48);
 }
 
-static int kvm_translate_vncr(struct kvm_vcpu *vcpu)
+static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *gmem)
 {
+	struct kvm_memory_slot *memslot;
 	bool write_fault, writable;
 	unsigned long mmu_seq;
 	struct vncr_tlb *vt;
@@ -1216,9 +1217,21 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu)
 	smp_rmb();
 
 	gfn = vt->wr.pa >> PAGE_SHIFT;
-	pfn = kvm_faultin_pfn(vcpu, gfn, write_fault, &writable, &page);
-	if (is_error_noslot_pfn(pfn) || (write_fault && !writable))
-		return -EFAULT;
+	memslot = gfn_to_memslot(vcpu->kvm, gfn);
+	*gmem = kvm_slot_has_gmem(memslot);
+	if (!*gmem) {
+		pfn = __kvm_faultin_pfn(memslot, gfn, write_fault ? FOLL_WRITE : 0,
+					&writable, &page);
+		if (is_error_noslot_pfn(pfn) || (write_fault && !writable))
+			return -EFAULT;
+	} else {
+		ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, &page, NULL);
+		if (ret) {
+			kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE,
+						      write_fault, false, false);
+			return ret;
+		}
+	}
 
 	scoped_guard(write_lock, &vcpu->kvm->mmu_lock) {
 		if (mmu_invalidate_retry(vcpu->kvm, mmu_seq))
@@ -1292,14 +1305,14 @@ int kvm_handle_vncr_abort(struct kvm_vcpu *vcpu)
 	if (esr_fsc_is_permission_fault(esr)) {
 		inject_vncr_perm(vcpu);
 	} else if (esr_fsc_is_translation_fault(esr)) {
-		bool valid;
+		bool valid, gmem = false;
 		int ret;
 
 		scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
 			valid = kvm_vncr_tlb_lookup(vcpu);
 
 		if (!valid)
-			ret = kvm_translate_vncr(vcpu);
+			ret = kvm_translate_vncr(vcpu, &gmem);
 		else
 			ret = -EPERM;
 
@@ -1309,6 +1322,14 @@ int kvm_handle_vncr_abort(struct kvm_vcpu *vcpu)
 			/* Let's try again... */
 			break;
 		case -EFAULT:
+		case -EIO:
+			/*
+			 * FIXME: Add whatever other error cases the
+			 * GMEM stuff can spit out.
+			 */
+			if (gmem)
+				return 0;
+			fallthrough;
 		case -EINVAL:
 		case -ENOENT:
 		case -EACCES:

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