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

[Fuad Tabba <tabba@google.com>]

Hi Marc,

On Fri, 11 Jul 2025 at 16:48, Marc Zyngier <maz@kernel.org> wrote:
>
> 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).

Makes sense.

> 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!
/fuad

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