Re: [PATCH v4 2/2] mm: guestmem: Convert address_space operations to guestmem library

[Mike Day <michael.day@xxxxxxx>]

On 11/21/24 10:43, Elliot Berman wrote:
> On Wed, Nov 20, 2024 at 10:12:08AM -0800, Elliot Berman wrote:
> > diff --git a/mm/guestmem.c b/mm/guestmem.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..19dd7e5d498f07577ec5cec5b52055f7435980f4
> > --- /dev/null
> > +++ b/mm/guestmem.c
> > @@ -0,0 +1,196 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * guestmem library
> > + *
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/fs.h>
> > +#include <linux/guestmem.h>
> > +#include <linux/mm.h>
> > +#include <linux/pagemap.h>
> > +
> > +struct guestmem {
> > +	const struct guestmem_ops *ops;
> > +};
> > +
> > +static inline struct guestmem *folio_to_guestmem(struct folio *folio)
> > +{
> > +	struct address_space *mapping = folio->mapping;
> > +
> > +	return mapping->i_private_data;
> > +}
> > +
> > +static inline bool __guestmem_release_folio(struct address_space *mapping,
> > +					    struct folio *folio)
> > +{
> > +	struct guestmem *gmem = mapping->i_private_data;
> > +	struct list_head *entry;
> > +
> > +	if (gmem->ops->release_folio) {
> > +		list_for_each(entry, &mapping->i_private_list) {
> > +			if (!gmem->ops->release_folio(entry, folio))
> > +				return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static inline int
> > +__guestmem_invalidate_begin(struct address_space *const mapping, pgoff_t start,
> > +			    pgoff_t end)
> > +{
> > +	struct guestmem *gmem = mapping->i_private_data;
> > +	struct list_head *entry;
> > +	int ret = 0;
> > +
> > +	list_for_each(entry, &mapping->i_private_list) {
> > +		ret = gmem->ops->invalidate_begin(entry, start, end);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline void
> > +__guestmem_invalidate_end(struct address_space *const mapping, pgoff_t start,
> > +			  pgoff_t end)
> > +{
> > +	struct guestmem *gmem = mapping->i_private_data;
> > +	struct list_head *entry;
> > +
> > +	if (gmem->ops->invalidate_end) {
> > +		list_for_each(entry, &mapping->i_private_list)
> > +			gmem->ops->invalidate_end(entry, start, end);
> > +	}
> > +}
> > +
> > +static void guestmem_free_folio(struct address_space *mapping,
> > +				struct folio *folio)
> > +{
> > +	WARN_ON_ONCE(!__guestmem_release_folio(mapping, folio));
> > +}
> > +
> > +static int guestmem_error_folio(struct address_space *mapping,
> > +				struct folio *folio)
> > +{
> > +	pgoff_t start, end;
> > +	int ret;
> > +
> > +	filemap_invalidate_lock_shared(mapping);
> > +
> > +	start = folio->index;
> > +	end = start + folio_nr_pages(folio);
> > +
> > +	ret = __guestmem_invalidate_begin(mapping, start, end);
> > +	if (ret)
> > +		goto out;
> > +
> > +	/*
> > +	 * Do not truncate the range, what action is taken in response to the
> > +	 * error is userspace's decision (assuming the architecture supports
> > +	 * gracefully handling memory errors).  If/when the guest attempts to
> > +	 * access a poisoned page, kvm_gmem_get_pfn() will return -EHWPOISON,
> > +	 * at which point KVM can either terminate the VM or propagate the
> > +	 * error to userspace.
> > +	 */
> > +
> > +	__guestmem_invalidate_end(mapping, start, end);
> > +
> > +out:
> > +	filemap_invalidate_unlock_shared(mapping);
> > +	return ret ? MF_DELAYED : MF_FAILED;
> > +}
> > +
> > +static int guestmem_migrate_folio(struct address_space *mapping,
> > +				  struct folio *dst, struct folio *src,
> > +				  enum migrate_mode mode)
> > +{
> > +	WARN_ON_ONCE(1);
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct address_space_operations guestmem_aops = {
> > +	.dirty_folio = noop_dirty_folio,
> > +	.free_folio = guestmem_free_folio,
> > +	.error_remove_folio = guestmem_error_folio,
> > +	.migrate_folio = guestmem_migrate_folio,
> > +};
> > +
> > +int guestmem_attach_mapping(struct address_space *mapping,
> > +			    const struct guestmem_ops *const ops,
> > +			    struct list_head *data)
> > +{
> > +	struct guestmem *gmem;
> > +
> > +	if (mapping->a_ops == &guestmem_aops) {
> > +		gmem = mapping->i_private_data;
> > +		if (gmem->ops != ops)
> > +			return -EINVAL;
> > +
> > +		goto add;
> > +	}
> > +
> > +	gmem = kzalloc(sizeof(*gmem), GFP_KERNEL);
> > +	if (!gmem)
> > +		return -ENOMEM;
> > +
> > +	gmem->ops = ops;
> > +
> > +	mapping->a_ops = &guestmem_aops;
> > +	mapping->i_private_data = gmem;
> > +
> > +	mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
> > +	mapping_set_inaccessible(mapping);
> > +	/* Unmovable mappings are supposed to be marked unevictable as well. */
> > +	WARN_ON_ONCE(!mapping_unevictable(mapping));
> > +
> > +add:
> > +	list_add(data, &mapping->i_private_list);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(guestmem_attach_mapping);
> > +
> > +void guestmem_detach_mapping(struct address_space *mapping,
> > +			     struct list_head *data)
> > +{
> > +	list_del(data);
> > +
> > +	if (list_empty(&mapping->i_private_list)) {
> > +		kfree(mapping->i_private_data);
>
> Mike was helping me test this out for SEV-SNP. They helped find a bug
> here. Right now, when the file closes, KVM calls
> guestmem_detach_mapping() which will uninstall the ops. When that
> happens, it's not necessary that all of the folios aren't removed from
> the filemap yet and so our free_folio() callback isn't invoked. This
> means that we skip updating the RMP entry back to shared/KVM-owned.
>
> There are a few approaches I could take:
>
> 1. Create a guestmem superblock so I can register guestmem-specific
>     destroy_inode() to do the kfree() above. This requires a lot of
>     boilerplate code, and I think it's not preferred approach.
> 2. Update how KVM tracks the memory so it is back in "shared" state when
>     the file closes. This requires some significant rework about the page
>     state compared to current guest_memfd. That rework might be useful
>     for the shared/private state machine.
> 3. Call truncate_inode_pages(mapping, 0) to force pages to be freed
>     here. It's might be possible that a page is allocated after this
>     point. In order for that to be a problem, KVM would need to update
>     RMP entry as guest-owned, and I don't believe that's possible after
>     the last guestmem_detach_mapping().
>
> My preference is to go with #3 as it was the most easy thing to do.

#3 is my preference as well. The semantics are that the guest is "closing" the gmem
object, which means all the memory is being released from the guest.

Mike
> > +		mapping->i_private_data = NULL;
> > +		mapping->a_ops = &empty_aops;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(guestmem_detach_mapping);
> > +
> > +struct folio *guestmem_grab_folio(struct address_space *mapping, pgoff_t index)
> > +{
> > +	/* TODO: Support huge pages. */
> > +	return filemap_grab_folio(mapping, index);
> > +}
> > +EXPORT_SYMBOL_GPL(guestmem_grab_folio);
> > +
> > +int guestmem_punch_hole(struct address_space *mapping, loff_t offset,
> > +			loff_t len)
> > +{
> > +	pgoff_t start = offset >> PAGE_SHIFT;
> > +	pgoff_t end = (offset + len) >> PAGE_SHIFT;
> > +	int ret;
> > +
> > +	filemap_invalidate_lock(mapping);
> > +	ret = __guestmem_invalidate_begin(mapping, start, end);
> > +	if (ret)
> > +		goto out;
> > +
> > +	truncate_inode_pages_range(mapping, offset, offset + len - 1);
> > +
> > +	__guestmem_invalidate_end(mapping, start, end);
> > +
> > +out:
> > +	filemap_invalidate_unlock(mapping);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(guestmem_punch_hole);
> > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> > index fd6a3010afa833e077623065b80bdbb5b1012250..1339098795d2e859b2ee0ef419b29045aedc8487 100644
> > --- a/virt/kvm/Kconfig
> > +++ b/virt/kvm/Kconfig
> > @@ -106,6 +106,7 @@ config KVM_GENERIC_MEMORY_ATTRIBUTES
> >   
> >   config KVM_PRIVATE_MEM
> >          select XARRAY_MULTI
> > +       select GUESTMEM
> >          bool
> >   
> >   config KVM_GENERIC_PRIVATE_MEM
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 24dcbad0cb76e353509cf4718837a1999f093414..edf57d5662cb8634bbd9ca3118b293c4f7ca229a 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -1,6 +1,7 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   #include <linux/backing-dev.h>
> >   #include <linux/falloc.h>
> > +#include <linux/guestmem.h>
> >   #include <linux/kvm_host.h>
> >   #include <linux/pagemap.h>
> >   #include <linux/anon_inodes.h>
> > @@ -98,8 +99,7 @@ static int kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
> >    */
> >   static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index)
> >   {
> > -	/* TODO: Support huge pages. */
> > -	return filemap_grab_folio(inode->i_mapping, index);
> > +	return guestmem_grab_folio(inode->i_mapping, index);
> >   }
> >   
> >   static void kvm_gmem_invalidate_begin(struct kvm_gmem *gmem, pgoff_t start,
> > @@ -151,28 +151,7 @@ static void kvm_gmem_invalidate_end(struct kvm_gmem *gmem, pgoff_t start,
> >   
> >   static long kvm_gmem_punch_hole(struct inode *inode, loff_t offset, loff_t len)
> >   {
> > -	struct list_head *gmem_list = &inode->i_mapping->i_private_list;
> > -	pgoff_t start = offset >> PAGE_SHIFT;
> > -	pgoff_t end = (offset + len) >> PAGE_SHIFT;
> > -	struct kvm_gmem *gmem;
> > -
> > -	/*
> > -	 * Bindings must be stable across invalidation to ensure the start+end
> > -	 * are balanced.
> > -	 */
> > -	filemap_invalidate_lock(inode->i_mapping);
> > -
> > -	list_for_each_entry(gmem, gmem_list, entry)
> > -		kvm_gmem_invalidate_begin(gmem, start, end);
> > -
> > -	truncate_inode_pages_range(inode->i_mapping, offset, offset + len - 1);
> > -
> > -	list_for_each_entry(gmem, gmem_list, entry)
> > -		kvm_gmem_invalidate_end(gmem, start, end);
> > -
> > -	filemap_invalidate_unlock(inode->i_mapping);
> > -
> > -	return 0;
> > +	return guestmem_punch_hole(inode->i_mapping, offset, len);
> >   }
> >   
> >   static long kvm_gmem_allocate(struct inode *inode, loff_t offset, loff_t len)
> > @@ -277,7 +256,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
> >   	kvm_gmem_invalidate_begin(gmem, 0, -1ul);
> >   	kvm_gmem_invalidate_end(gmem, 0, -1ul);
> >   
> > -	list_del(&gmem->entry);
> > +	guestmem_detach_mapping(inode->i_mapping, &gmem->entry);
> >   
> >   	filemap_invalidate_unlock(inode->i_mapping);
> >   
> > @@ -318,63 +297,42 @@ void kvm_gmem_init(struct module *module)
> >   	kvm_gmem_fops.owner = module;
> >   }
> >   
> > -static int kvm_gmem_migrate_folio(struct address_space *mapping,
> > -				  struct folio *dst, struct folio *src,
> > -				  enum migrate_mode mode)
> > +static int kvm_guestmem_invalidate_begin(struct list_head *entry, pgoff_t start,
> > +					 pgoff_t end)
> >   {
> > -	WARN_ON_ONCE(1);
> > -	return -EINVAL;
> > -}
> > +	struct kvm_gmem *gmem = container_of(entry, struct kvm_gmem, entry);
> >   
> > -static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *folio)
> > -{
> > -	struct list_head *gmem_list = &mapping->i_private_list;
> > -	struct kvm_gmem *gmem;
> > -	pgoff_t start, end;
> > -
> > -	filemap_invalidate_lock_shared(mapping);
> > -
> > -	start = folio->index;
> > -	end = start + folio_nr_pages(folio);
> > -
> > -	list_for_each_entry(gmem, gmem_list, entry)
> > -		kvm_gmem_invalidate_begin(gmem, start, end);
> > -
> > -	/*
> > -	 * Do not truncate the range, what action is taken in response to the
> > -	 * error is userspace's decision (assuming the architecture supports
> > -	 * gracefully handling memory errors).  If/when the guest attempts to
> > -	 * access a poisoned page, kvm_gmem_get_pfn() will return -EHWPOISON,
> > -	 * at which point KVM can either terminate the VM or propagate the
> > -	 * error to userspace.
> > -	 */
> > +	kvm_gmem_invalidate_begin(gmem, start, end);
> >   
> > -	list_for_each_entry(gmem, gmem_list, entry)
> > -		kvm_gmem_invalidate_end(gmem, start, end);
> > +	return 0;
> > +}
> >   
> > -	filemap_invalidate_unlock_shared(mapping);
> > +static void kvm_guestmem_invalidate_end(struct list_head *entry, pgoff_t start,
> > +					pgoff_t end)
> > +{
> > +	struct kvm_gmem *gmem = container_of(entry, struct kvm_gmem, entry);
> >   
> > -	return MF_DELAYED;
> > +	kvm_gmem_invalidate_end(gmem, start, end);
> >   }
> >   
> >   #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> > -static void kvm_gmem_free_folio(struct address_space *mapping,
> > -				struct folio *folio)
> > +static bool kvm_gmem_release_folio(struct list_head *entry, struct folio *folio)
> >   {
> >   	struct page *page = folio_page(folio, 0);
> >   	kvm_pfn_t pfn = page_to_pfn(page);
> >   	int order = folio_order(folio);
> >   
> >   	kvm_arch_gmem_invalidate(pfn, pfn + (1ul << order));
> > +
> > +	return true;
> >   }
> >   #endif
> >   
> > -static const struct address_space_operations kvm_gmem_aops = {
> > -	.dirty_folio = noop_dirty_folio,
> > -	.migrate_folio	= kvm_gmem_migrate_folio,
> > -	.error_remove_folio = kvm_gmem_error_folio,
> > +static const struct guestmem_ops kvm_guestmem_ops = {
> > +	.invalidate_begin = kvm_guestmem_invalidate_begin,
> > +	.invalidate_end = kvm_guestmem_invalidate_end,
> >   #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> > -	.free_folio = kvm_gmem_free_folio,
> > +	.release_folio = kvm_gmem_release_folio,
> >   #endif
> >   };
> >   
> > @@ -430,22 +388,22 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> >   
> >   	inode->i_private = (void *)(unsigned long)flags;
> >   	inode->i_op = &kvm_gmem_iops;
> > -	inode->i_mapping->a_ops = &kvm_gmem_aops;
> >   	inode->i_mode |= S_IFREG;
> >   	inode->i_size = size;
> > -	mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> > -	mapping_set_inaccessible(inode->i_mapping);
> > -	/* Unmovable mappings are supposed to be marked unevictable as well. */
> > -	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> > +	err = guestmem_attach_mapping(inode->i_mapping, &kvm_guestmem_ops,
> > +				      &gmem->entry);
> > +	if (err)
> > +		goto err_putfile;
> >   
> >   	kvm_get_kvm(kvm);
> >   	gmem->kvm = kvm;
> >   	xa_init(&gmem->bindings);
> > -	list_add(&gmem->entry, &inode->i_mapping->i_private_list);
> >   
> >   	fd_install(fd, file);
> >   	return fd;
> >   
> > +err_putfile:
> > +	fput(file);
> >   err_gmem:
> >   	kfree(gmem);
> >   err_fd:
> >
> > --
> > 2.34.1