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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux