Re: [PATCH v13 10/24] gunyah: vm_mgr: Add/remove user memory regions

[Elliot Berman]

On 5/19/2023 4:59 AM, Will Deacon wrote:
> Hi Elliot,
>
> On Tue, May 09, 2023 at 01:47:47PM -0700, Elliot Berman wrote:
> > When launching a virtual machine, Gunyah userspace allocates memory for
> > the guest and informs Gunyah about these memory regions through
> > SET_USER_MEMORY_REGION ioctl.
> >
> > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@xxxxxxxxxxx>
> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@xxxxxxxxxxx>
> > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> > ---
> >   drivers/virt/gunyah/Makefile    |   2 +-
> >   drivers/virt/gunyah/vm_mgr.c    |  59 +++++++-
> >   drivers/virt/gunyah/vm_mgr.h    |  26 ++++
> >   drivers/virt/gunyah/vm_mgr_mm.c | 236 ++++++++++++++++++++++++++++++++
> >   include/uapi/linux/gunyah.h     |  37 +++++
> >   5 files changed, 356 insertions(+), 4 deletions(-)
> >   create mode 100644 drivers/virt/gunyah/vm_mgr_mm.c
>
> [...]
>
> > +int gh_vm_mem_alloc(struct gh_vm *ghvm, struct gh_userspace_memory_region *region)
> > +{
> > +	struct gh_vm_mem *mapping, *tmp_mapping;
> > +	struct page *curr_page, *prev_page;
> > +	struct gh_rm_mem_parcel *parcel;
> > +	int i, j, pinned, ret = 0;
> > +	unsigned int gup_flags;
> > +	size_t entry_size;
> > +	u16 vmid;
> > +
> > +	if (!region->memory_size || !PAGE_ALIGNED(region->memory_size) ||
> > +		!PAGE_ALIGNED(region->userspace_addr) ||
> > +		!PAGE_ALIGNED(region->guest_phys_addr))
> > +		return -EINVAL;
> > +
> > +	if (overflows_type(region->guest_phys_addr + region->memory_size, u64))
> > +		return -EOVERFLOW;
> > +
> > +	ret = mutex_lock_interruptible(&ghvm->mm_lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mapping = __gh_vm_mem_find_by_label(ghvm, region->label);
> > +	if (mapping) {
> > +		ret = -EEXIST;
> > +		goto unlock;
> > +	}
> > +
> > +	list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) {
> > +		if (gh_vm_mem_overlap(tmp_mapping, region->guest_phys_addr,
> > +					region->memory_size)) {
> > +			ret = -EEXIST;
> > +			goto unlock;
> > +		}
> > +	}
> > +
> > +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL_ACCOUNT);
> > +	if (!mapping) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	mapping->guest_phys_addr = region->guest_phys_addr;
> > +	mapping->npages = region->memory_size >> PAGE_SHIFT;
> > +	parcel = &mapping->parcel;
> > +	parcel->label = region->label;
> > +	parcel->mem_handle = GH_MEM_HANDLE_INVAL; /* to be filled later by mem_share/mem_lend */
> > +	parcel->mem_type = GH_RM_MEM_TYPE_NORMAL;
> > +
> > +	ret = account_locked_vm(ghvm->mm, mapping->npages, true);
> > +	if (ret)
> > +		goto free_mapping;
> > +
> > +	mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL_ACCOUNT);
> > +	if (!mapping->pages) {
> > +		ret = -ENOMEM;
> > +		mapping->npages = 0; /* update npages for reclaim */
> > +		goto unlock_pages;
> > +	}
> > +
> > +	gup_flags = FOLL_LONGTERM;
> > +	if (region->flags & GH_MEM_ALLOW_WRITE)
> > +		gup_flags |= FOLL_WRITE;
> > +
> > +	pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
> > +					gup_flags, mapping->pages);
> > +	if (pinned < 0) {
> > +		ret = pinned;
> > +		goto free_pages;
> > +	} else if (pinned != mapping->npages) {
> > +		ret = -EFAULT;
> > +		mapping->npages = pinned; /* update npages for reclaim */
> > +		goto unpin_pages;
> > +	}
>
> Sorry if I missed it, but I still don't see where you reject file mappings
> here.

Sure, I can reject file mappings. I didn't catch that was the ask 
previously and thought it was only a comment about behavior of file 
mappings.

> This is also the wrong interface for upstream. Please get involved with
> the fd-based guest memory discussions [1] and port your series to that.

The user interface design for *shared* memory aligns with 
KVM_SET_USER_MEMORY_REGION.

I understood we want to use restricted memfd for giving guest-private 
memory (Gunyah calls this "lending memory"). When I went through the 
changes, I gathered KVM is using restricted memfd only for guest-private 
memory and not for shared memory. Thus, I dropped support for lending 
memory to the guest VM and only retained the shared memory support in 
this series. I'd like to merge what we can today and introduce the 
guest-private memory support in tandem with the restricted memfd; I 
don't see much reason to delay the series.

I briefly evaluated and picked the arm64/pKVM support that Fuad shared 
[2] and found it should be fine for Gunyah. I did build-only at the 
time. I don't have any comments on the base restricted_memfd support and 
Fuad has not posted [2] on mailing lists yet as far as I can tell.

> This patch cannot be merged in its current form.

I am a little confused why the implementation to share memory with the 
VM is being rejected. Besides rejecting file mappings, any other changes 
needed to be accepted?

- Elliot

> Will
>
> [1] https://lore.kernel.org/kvm/20221202061347.1070246-1-chao.p.peng@xxxxxxxxxxxxxxx/
[2]: 
https://android-kvm.googlesource.com/linux/+/refs/heads/tabba/fdmem-v10-core