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

[David Hildenbrand <david.hildenbrand@arm.com>]

On 7/2/26 16:29, Alexandru Elisei wrote:
> To enable memslot operations, KVM maintains two arrays of memslots, and an
> RCU pointer to the active (in use) array. Changes are made first to the
> inactive array, and the RCU pointer is updated to point to the inactive
> array, which becomes active.
> 
> The guest_memfd file maintains an xarray of pointers to memslots that use
> it as the memory provider. After the RCU pointer to the active memslots is
> updated and until SRCU is synchronized, readers can observe the old or the
> new value for the active array, and therefore the old or the new pointer
> for a given memslot.  For memslot creation or deletion that is not an issue
> for guest_memfd, as readers will either read the same memslot pointer saved
> by the guest_memfd file, or a non-existing memslot.
> 
> But when changing the flags for a memslot, readers can read two different
> and non-NULL memslot pointers. Since there is no easy way to ensure that
> the memslot pointer that the guest_memfd stores is consistent with both
> views at the same time, modify how the guest_memfd file keeps track of the
> associated memslots: instead of storing the pointer directly, store the
> memslot id and address space id (as_id), and use that to reach the memslot
> in the active list of memslots.
> 
> This only changes how guest_memfd keeps track of memslots, userspace is not
> allowed to make changes to a memslot yet.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  virt/kvm/guest_memfd.c | 95 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 15 deletions(-)
> 
> 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).

>  };
>  
>  struct gmem_inode {
> @@ -43,6 +44,29 @@ static __always_inline struct gmem_inode *GMEM_I(struct inode *inode)
>  #define kvm_gmem_for_each_file(f, inode) \
>  	list_for_each_entry(f, &GMEM_I(inode)->gmem_file_list, entry)
>  
> +static void *memslot_to_xa_value(struct kvm_memory_slot *slot)
> +{
> +	WARN_ON_ONCE(sizeof(slot->as_id) > 16);
> +	WARN_ON_ONCE(sizeof(slot->id) > 16);
> +	WARN_ON_ONCE(sizeof(slot->as_id) + sizeof(slot->id) > sizeof(unsigned long));

These can just be BUILD_BUG_ON() I suppose.

> +
> +	return xa_mk_value(((unsigned long)slot->as_id) << 16 | (unsigned long)slot->id);

The latter "(unsigned long)" should not be required.

> +}
> +
> +static struct kvm_memory_slot *xa_value_to_memslot(struct kvm *kvm, const void *entry)
> +{

The following can all be const.

> +	unsigned long full_id = xa_to_value(entry);
> +	u16 as_id = (full_id >> 16) & U16_MAX;

Why the "& U16_MAX" here?

(1) It's an u16

(2) memslot_to_xa_value() never stores anything in there.

> +	short id = full_id & U16_MAX;

Same here. And I wonder why you are not also using an u16 here.

> +
> +	/*
> +	 * Do not ignore KVM_MEMSLOT_INVALID memslots, as we want
> +	 * ->error_remove_folio(), when it races with memslot deletion, to have
> +	 *  unmapped the memory upon completion.
> +	 */
> +	return id_to_memslot(__kvm_memslots(kvm, as_id), id);
> +}
> +
>  /**
>   * folio_file_pfn - like folio_file_page, but return a pfn.
>   * @folio: The folio which contains this index.
> @@ -157,7 +181,7 @@ static enum kvm_gfn_range_filter kvm_gmem_get_invalidate_filter(struct inode *in
>  	return KVM_FILTER_PRIVATE;
>  }
>  
> -static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
> +static bool __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  					pgoff_t end,
>  					enum kvm_gfn_range_filter attr_filter)
>  {
> @@ -165,9 +189,15 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  	struct kvm_memory_slot *slot;
>  	struct kvm *kvm = f->kvm;
>  	unsigned long index;
> +	void *entry;
> +
> +	xa_for_each_range(&f->bindings, index, entry, start, end - 1) {
> +		pgoff_t pgoff;
>  
> -	xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
> -		pgoff_t pgoff = slot->gmem.pgoff;
> +		slot = xa_value_to_memslot(kvm, entry);

That now gets more expensive. id_to_memslot() uses a hashtable, but there is
certainly more pointer chasing going on now.

> +		if (!slot)
> +			continue;
> +		pgoff = slot->gmem.pgoff;
>  
>  		struct kvm_gfn_range gfn_range = {
>  			.start = slot->base_gfn + max(pgoff, start) - pgoff,
> @@ -192,6 +222,8 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  
>  	if (found_memslot)
>  		KVM_MMU_UNLOCK(kvm);
> +
> +	return found_memslot;
>  }
-- 
Cheers,

David