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

[Marc Zyngier <maz@kernel.org>]

On Mon, 14 Jul 2025 08:42:00 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Hi Marc,
> 
> 
> On Fri, 11 Jul 2025 at 17:38, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 09 Jul 2025 11:59:42 +0100,
> > Fuad Tabba <tabba@google.com> wrote:
> > >
> > > Add arm64 architecture support for handling guest page faults on memory
> > > slots backed by guest_memfd.
> > >
> > > This change introduces a new function, gmem_abort(), which encapsulates
> > > the fault handling logic specific to guest_memfd-backed memory. The
> > > kvm_handle_guest_abort() entry point is updated to dispatch to
> > > gmem_abort() when a fault occurs on a guest_memfd-backed memory slot (as
> > > determined by kvm_slot_has_gmem()).
> > >
> > > Until guest_memfd gains support for huge pages, the fault granule for
> > > these memory regions is restricted to PAGE_SIZE.
> > >
> > > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > > Reviewed-by: James Houghton <jthoughton@google.com>
> > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > ---
> > >  arch/arm64/kvm/mmu.c | 82 ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 79 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 58662e0ef13e..71f8b53683e7 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -1512,6 +1512,78 @@ static void adjust_nested_fault_perms(struct kvm_s2_trans *nested,
> > >       *prot |= kvm_encode_nested_level(nested);
> > >  }
> > >
> > > +#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;
> > > +     }
> >
> > I don't think we need to cargo-cult this stuff. This cannot happen
> > architecturally (data and instruction aborts are two different
> > exceptions, so you can't have both at the same time), and is only
> > there because we were young and foolish when we wrote this crap.
> >
> > Now that we (the royal We) are only foolish, we can save a few bits by
> > dropping it. Or turn it into a VM_BUG_ON() if you really want to keep
> > it.
> 
> Will do, but if you agree, I'll go with a VM_WARN_ON_ONCE() since
> VM_BUG_ON is going away [1][2]
> 
> [1] https://lore.kernel.org/all/b247be59-c76e-4eb8-8a6a-f0129e330b11@redhat.com/
> [2] https://lore.kernel.org/all/20250604140544.688711-1-david@redhat.com/T/#u

Ah, sure. We've never seen these anyway in any situation other than
"I've mutated this kernel so badly it's closer to a hamster".

Thanks,

	M.

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