Re: [RFC PATCH 1/3] KVM: guest_memfd: Use memslot id to keep track of associated memslots

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Sean,

On Mon, Jul 06, 2026 at 02:46:03PM -0700, Sean Christopherson wrote:
> On Mon, Jul 06, 2026, Alexandru Elisei wrote:
> > On Mon, Jul 06, 2026 at 09:14:59AM +0200, David Hildenbrand wrote:
> > > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > > index db57c5766ab6..43ef8e908aaf 100644
> > > > --- a/virt/kvm/guest_memfd.c
> > > > +++ b/virt/kvm/guest_memfd.c
> > > > @@ -25,6 +25,7 @@ struct gmem_file {
> > > >  	struct kvm *kvm;
> > > >  	struct xarray bindings;
> > > >  	struct list_head entry;
> > > > +	bool found_memslot;	/* Used for balancing invalidations when punching a hole */
> > > 
> > > Probabably best to document what it means, not only what it is used for (and
> > > maybe document above the member if you end up with more text).
> > 
> > Sure, this is how I changed it:
> > 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 210bdd76f0aa..3cee64047bce 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -25,7 +25,13 @@ struct gmem_file {
> >         struct kvm *kvm;
> >         struct xarray bindings;
> >         struct list_head entry;
> > -       bool found_memslot;     /* Used for balancing invalidations when punching a hole */
> > +       /*
> > +        * Keeps track of whether memory has been unmapped during secondary MMU
> > +        * invalidation, to keep the invalidate start and end calls balanced.
> > +        *
> > +        * Accessed while holding the inode->i_mapping lock in exclusive mode.
> > +        */
> > +       bool memslot_invalidated;
> >  };
> > 
> > Hopefully the new name is better.
> 
> Why add a boolean?  It's not _needed_ to balance updates, or rather it shouldn't
> be needed.  filemap_invalidate_lock() prevents modifying bindings while an
> invalidation is in-progress.  It might be a performation optimization, but if so,
> it's a premature one and belongs in a separate patch.

It's not about bindings being modified. This is the race I am trying to avoid:

CPU0:							CPU1:

KVM_SET_USER_MEMORY_REGION(slot, memory_size=0)
kvm_replace_memslot(kvm, slot, invalid_slot)
kvm_swap_active_memslots(kvm)
kvm_replace_memslot(kvm, invalid_slot, NULL)
kvm_swap_active_memslots(kvm)
							fallocate(guest_memfd, FALLOC_FL_PUNCH_HOLE)
							  __kvm_gmem_invalidate_start()
							    slot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
							    if (slot == NULL) {
								    // Early exit, does not call kvm_mmu_invalidate_start(kvm)
								    return;
							    }
							  __kvm_gmem_invalidate_end()
							    // No corresponding kvm_mmu_invalidate_start(kvm) call
							    kvm_mmu_invalidate_end(kvm)
kvm_replace_memslot(kvm, invalid_slot, NULL);
kvm_free_memslot(kvm, slot)
  kvm_gmem_unbind(slot)

Does that make sense?

Thanks,
Alex