Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

>
> [...snip...]
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index a13445c26d9d..d1e502d3246b 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -1003,73 +1003,61 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	return __kvm_gmem_create(kvm, size, flags);
>  }
>
> -int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> -				   unsigned int fd, uoff_t offset)
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> +				   const struct kvm_memory_slot *old,
> +				   struct kvm_memory_slot *new,
> +				   enum kvm_mr_change change)
>  {
> -	uoff_t size = slot->npages << PAGE_SHIFT;
>  	struct gmem_file *f;
>  	struct inode *inode;
> -	struct file *file;
>
> +	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(new->gmem.pgoff));
>
> -	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
> -	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
> -
> -	if (WARN_ON_ONCE(slot->flags & KVM_MEMSLOT_GMEM_ONLY))
> +	if (WARN_ON_ONCE(change != KVM_MR_CREATE))
>  		return -EINVAL;
>
> -	file = fget(fd);
> -	if (!file)
> -		return -EBADF;
> +	if (WARN_ON_ONCE(new->flags & KVM_MEMSLOT_GMEM_ONLY))
> +		return -EINVAL;
>
> -	if (file->f_op != &kvm_gmem_fops)
> -		goto err;
> +	if (new->gmem.file->f_op != &kvm_gmem_fops)
> +		return -EINVAL;
>
> -	f = file->private_data;
> +	f = new->gmem.file->private_data;
>  	if (f->kvm != kvm)
> -		goto err;
> +		return -EINVAL;
>
> -	inode = file_inode(file);
> +	inode = file_inode(new->gmem.file);
>
> -	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> -		goto err;
> +	if (new->gmem.pgoff + new->npages  > i_size_read(inode) >> PAGE_SHIFT)
> +		return -EINVAL;
>
>  	/*
>  	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
>  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot

Just noticed, this comment needs to be updated for the new function name.

>  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
>  	 */
> -	slot->gmem.file = file;
> -	slot->gmem.pgoff = offset >> PAGE_SHIFT;
>  	if (gmem_in_place_conversion || kvm_gmem_supports_mmap(inode))
> -		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> +		new->flags |= KVM_MEMSLOT_GMEM_ONLY;
>
> -	/*
> -	 * Gift the caller a reference to the file.  The reference will be
> -	 * dropped after bindings are established, or if installing the new
> -	 * memslot ultimately fails.
> -	 */
>  	return 0;
> -
> -err:
> -	fput(file);
> -	return -EINVAL;
>  }
>

Looks like const struct kvm_memory_slot *old is unused, though you
already told yourself that. :)

> -int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
> +int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
> +				  struct kvm_memory_slot *new,
> +				  enum kvm_mr_change change)
>  {
> -	struct gmem_file *f = slot->gmem.file->private_data;
> -	struct inode *inode = file_inode(slot->gmem.file);
> +	struct gmem_file *f = new->gmem.file->private_data;
> +	struct inode *inode = file_inode(new->gmem.file);
>  	unsigned long start, end;
>  	int r;
>
> -	if (WARN_ON_ONCE(slot->gmem.file->f_op != &kvm_gmem_fops))
> +	if (WARN_ON_ONCE(new->gmem.file->f_op != &kvm_gmem_fops))
>  		return -EIO;
>
>  	filemap_invalidate_lock(inode->i_mapping);
>
> -	start = slot->gmem.pgoff;
> -	end = start + slot->npages;
> +	start = new->gmem.pgoff;
> +	end = start + new->npages;
>
>  	if (!xa_empty(&f->bindings) &&
>  	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> @@ -1077,7 +1065,7 @@ int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
>  		return -EEXIST;
>  	}
>
> -	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
> +	r = xa_err(xa_store_range(&f->bindings, start, end - 1, new, GFP_KERNEL));
>  	if (r)
>  		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
>
> diff --git a/virt/kvm/guest_memfd.h b/virt/kvm/guest_memfd.h
> index 01bd359d27e3..63723ffee0a5 100644
> --- a/virt/kvm/guest_memfd.h
> +++ b/virt/kvm/guest_memfd.h
> @@ -8,9 +8,13 @@
>  int kvm_gmem_init(struct module *module);
>  void kvm_gmem_exit(void);
>  int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
> -int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> -				   unsigned int fd, uoff_t offset);
> -int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot);
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> +				   const struct kvm_memory_slot *old,
> +				   struct kvm_memory_slot *new,
> +				   enum kvm_mr_change change);
> +int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
> +				  struct kvm_memory_slot *new,
> +				  enum kvm_mr_change change);
>  void kvm_gmem_unbind(struct kvm_memory_slot *slot);
>  #else
>  static inline int kvm_gmem_init(struct module *module)
> @@ -20,15 +24,18 @@ static inline int kvm_gmem_init(struct module *module)
>  static inline void kvm_gmem_exit(void) {};
>
>  static inline int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> -						 struct kvm_memory_slot *slot,
> -						 unsigned int fd, uoff_t offset)
> +						 const struct kvm_memory_slot *old,
> +						 struct kvm_memory_slot *new,
> +						 enum kvm_mr_change change)
>  {
>  	WARN_ON_ONCE(1);
>  	return -EIO;
>  }
>
>  static inline int kvm_gmem_commit_memory_region(struct kvm *kvm,
> -						struct kvm_memory_slot *slot)
> +						struct kvm_memory_slot *old,
> +						struct kvm_memory_slot *new,
> +						enum kvm_mr_change change)
>  {
>  	WARN_ON_ONCE(1);
>  	return -EIO;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9fff2f4bf2f1..70d70042a9ce 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1681,6 +1681,12 @@ static int kvm_prepare_memory_region(struct kvm *kvm,
>  {
>  	int r;
>
> +	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> +		r = kvm_gmem_prepare_memory_region(kvm, old, new, change);
> +		if (r)
> +			return r;
> +	}
> +
>  	/*
>  	 * If dirty logging is disabled, nullify the bitmap; the old bitmap
>  	 * will be freed on "commit".  If logging is enabled in both old and
> @@ -1952,8 +1958,8 @@ static int kvm_set_memslot(struct kvm *kvm,
>  	if (r)
>  		goto err;
>
> -	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> -		r = kvm_gmem_commit_memory_region(kvm, new);
> +	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> +		r = kvm_gmem_commit_memory_region(kvm, old, new, change);
>  		if (r) {
>  			kvm_arch_free_memslot(kvm, new);
>  			kvm_destroy_dirty_bitmap(new);
> @@ -2133,11 +2139,16 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	new->npages = npages;
>  	new->flags = mem->flags;
>  	new->userspace_addr = mem->userspace_addr;
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
> -		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
> -						   mem->guest_memfd_offset);
> -		if (r)
> +	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +		new->gmem.file = fget(mem->guest_memfd);
> +		if (!new->gmem.file) {
> +			r = -EBADF;
>  			goto out;
> +		}
> +
> +		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> +#endif
>  	}
>
>  	r = kvm_set_memslot(kvm, old, new, change);
> @@ -2148,7 +2159,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	 * the file is closed before memslots are destroyed.
>  	 */
>  #ifdef CONFIG_KVM_GUEST_MEMFD
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
> +	if (new->gmem.file)
>  		fput(new->gmem.file);
>  #endif

And this LGTM too. Thanks!