Messages in this thread | | | Date | Mon, 6 Feb 2023 15:23:42 -0800 | Subject | Re: [PATCH v9 13/27] gunyah: vm_mgr: Add/remove user memory regions | From | Elliot Berman <> |
| |
On 2/6/2023 8:12 AM, Srinivas Kandagatla wrote: > > > On 20/01/2023 22:46, 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@quicinc.com> >> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> drivers/virt/gunyah/Makefile | 2 +- >> drivers/virt/gunyah/vm_mgr.c | 46 +++++++ >> drivers/virt/gunyah/vm_mgr.h | 28 +++- >> drivers/virt/gunyah/vm_mgr_mm.c | 223 ++++++++++++++++++++++++++++++++ >> include/uapi/linux/gunyah.h | 22 ++++ >> 5 files changed, 319 insertions(+), 2 deletions(-) >> create mode 100644 drivers/virt/gunyah/vm_mgr_mm.c >> >> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile >> index 03951cf82023..ff8bc4925392 100644 >> --- a/drivers/virt/gunyah/Makefile >> +++ b/drivers/virt/gunyah/Makefile >> @@ -2,5 +2,5 @@ >> obj-$(CONFIG_GUNYAH) += gunyah.o >> -gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o >> +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o >> obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o >> diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c >> index 0864dbd77e28..b847fde63333 100644 >> --- a/drivers/virt/gunyah/vm_mgr.c >> +++ b/drivers/virt/gunyah/vm_mgr.c >> @@ -35,14 +35,55 @@ static __must_check struct gunyah_vm >> *gunyah_vm_alloc(struct gh_rm_rpc *rm) >> ghvm->vmid = vmid; >> ghvm->rm = rm; >> + mutex_init(&ghvm->mm_lock); >> + INIT_LIST_HEAD(&ghvm->memory_mappings); >> + >> return ghvm; >> } >> static long gh_vm_ioctl(struct file *filp, unsigned int cmd, >> unsigned long arg) >> { >> + struct gunyah_vm *ghvm = filp->private_data; >> + void __user *argp = (void __user *)arg; >> long r; >> switch (cmd) { >> + case GH_VM_SET_USER_MEM_REGION: { >> + struct gunyah_vm_memory_mapping *mapping; >> + struct gh_userspace_memory_region region; >> + >> + r = -EFAULT; >> + if (copy_from_user(®ion, argp, sizeof(region))) >> + break; > Why not be explict about the error codes, do something like. > > if (copy_from_user(®ion, argp, sizeof(region))) > return -EFAULT; > > > setting r value everytime before starting any code is making the code > more reader unfriendly. > >
Done.
>> + >> + r = -EINVAL; >> + /* All other flag bits are reserved for future use */ >> + if (region.flags & ~(GH_MEM_ALLOW_READ | GH_MEM_ALLOW_WRITE | >> GH_MEM_ALLOW_EXEC | >> + GH_MEM_LENT)) >> + break; >> + >> + >> + if (region.memory_size) { > > This behaviour allocating memory in presense of valid memory_size and > finding memory in cases of zero size needs to be described properly in > the uapi so that the users are aware of this. >
This behavior is described in "Document Gunyah VM Manager": https://lore.kernel.org/all/20230120224627.4053418-20-quic_eberman@quicinc.com/
>> + r = 0; >> + mapping = gh_vm_mem_mapping_alloc(ghvm, ®ion); >> + if (IS_ERR(mapping)) { >> + r = PTR_ERR(mapping); >> + break; >> + } >> + } else { >> + mapping = gh_vm_mem_mapping_find(ghvm, region.label); >> + if (IS_ERR(mapping)) { >> + r = PTR_ERR(mapping); >> + break; >> + } >> + r = 0; >> + if (!mapping) >> + break; >> + gh_vm_mem_mapping_reclaim(ghvm, mapping); >> + kfree(mapping); >> + } >> + break; >> + } >> default: >> r = -ENOTTY; >> break; >> @@ -54,7 +95,12 @@ static long gh_vm_ioctl(struct file *filp, unsigned >> int cmd, unsigned long arg) >> static int gh_vm_release(struct inode *inode, struct file *filp) >> { >> struct gunyah_vm *ghvm = filp->private_data; >> + struct gunyah_vm_memory_mapping *mapping, *tmp; > Locking? >> + list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, >> list) { >> + gh_vm_mem_mapping_reclaim(ghvm, mapping); >> + kfree(mapping); >> + } >> put_gh_rm(ghvm->rm); >> kfree(ghvm); >> return 0; >> diff --git a/drivers/virt/gunyah/vm_mgr.h b/drivers/virt/gunyah/vm_mgr.h >> index e47f34de7f9e..6b38bf780f76 100644 >> --- a/drivers/virt/gunyah/vm_mgr.h >> +++ b/drivers/virt/gunyah/vm_mgr.h >> @@ -7,14 +7,40 @@ >> #define _GH_PRIV_VM_MGR_H >> #include <linux/gunyah_rsc_mgr.h> >> +#include <linux/list.h> >> +#include <linux/miscdevice.h> >> +#include <linux/mutex.h> >> #include <uapi/linux/gunyah.h> >> long gh_dev_vm_mgr_ioctl(struct gh_rm *rm, unsigned int cmd, >> unsigned long arg); >> +enum gunyah_vm_mem_share_type { >> + VM_MEM_SHARE, >> + VM_MEM_LEND, >> +}; >> + >> +struct gunyah_vm_memory_mapping { >> + struct list_head list; >> + enum gunyah_vm_mem_share_type share_type; >> + struct gh_rm_mem_parcel parcel; >> + >> + __u64 guest_phys_addr; >> + struct page **pages; >> + unsigned long npages; >> +}; >> + >> struct gunyah_vm { >> u16 vmid; >> - struct gh_rm_rpc *rm; >> + struct gh_rm *rm; >> + >> + struct mutex mm_lock; >> + struct list_head memory_mappings; >> }; >> +struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_alloc(struct >> gunyah_vm *ghvm, >> + struct gh_userspace_memory_region *region); >> +void gh_vm_mem_mapping_reclaim(struct gunyah_vm *ghvm, struct >> gunyah_vm_memory_mapping *mapping); >> +struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_find(struct >> gunyah_vm *ghvm, u32 label); >> + >> #endif >> diff --git a/drivers/virt/gunyah/vm_mgr_mm.c >> b/drivers/virt/gunyah/vm_mgr_mm.c >> new file mode 100644 >> index 000000000000..f2dbdb4ee8ab >> --- /dev/null >> +++ b/drivers/virt/gunyah/vm_mgr_mm.c >> @@ -0,0 +1,223 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All >> rights reserved. >> + */ >> + >> +#define pr_fmt(fmt) "gh_vm_mgr: " fmt >> + >> +#include <linux/gunyah_rsc_mgr.h> >> +#include <linux/mm.h> >> + >> +#include <uapi/linux/gunyah.h> >> + >> +#include "vm_mgr.h" >> + >> +static inline bool page_contiguous(phys_addr_t p, phys_addr_t t) >> +{ >> + return t - p == PAGE_SIZE; >> +} >> + >> +static struct gunyah_vm_memory_mapping >> *__gh_vm_mem_mapping_find(struct gunyah_vm *ghvm, u32 label) >> +{ >> + struct gunyah_vm_memory_mapping *mapping; >> + >> + > only one line.
Done.
>> + list_for_each_entry(mapping, &ghvm->memory_mappings, list) >> + if (mapping->parcel.label == label) >> + return mapping; >> + >> + return NULL; >> +} >> + >> +void gh_vm_mem_mapping_reclaim(struct gunyah_vm *ghvm, struct >> gunyah_vm_memory_mapping *mapping) >> +{ >> + int i, ret = 0; >> + >> + if (mapping->parcel.mem_handle != GH_MEM_HANDLE_INVAL) { >> + ret = gh_rm_mem_reclaim(ghvm->rm, &mapping->parcel); >> + if (ret) >> + pr_warn("Failed to reclaim memory parcel for label %d: >> %d\n", >> + mapping->parcel.label, ret); >> + } >> + >> + if (!ret) >> + for (i = 0; i < mapping->npages; i++) >> + unpin_user_page(mapping->pages[i]); >> + >> + kfree(mapping->pages); >> + kfree(mapping->parcel.acl_entries); >> + kfree(mapping->parcel.mem_entries); >> + >> + mutex_lock(&ghvm->mm_lock); >> + list_del(&mapping->list); >> + mutex_unlock(&ghvm->mm_lock); >> +} >> + >> +struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_find(struct >> gunyah_vm *ghvm, u32 label) >> +{ >> + struct gunyah_vm_memory_mapping *mapping; >> + int ret; >> + >> + ret = mutex_lock_interruptible(&ghvm->mm_lock); >> + if (ret) >> + return ERR_PTR(ret); >> + mapping = __gh_vm_mem_mapping_find(ghvm, label); >> + mutex_unlock(&ghvm->mm_lock); >> + return mapping ? : ERR_PTR(-ENODEV); >> +} >> + >> +struct gunyah_vm_memory_mapping *gh_vm_mem_mapping_alloc(struct >> gunyah_vm *ghvm, >> + struct gh_userspace_memory_region *region) >> +{ > Is this a static functoin or an exported symbol? >
Neither, it's used in vm_mgr.c.
>> + phys_addr_t curr_page, prev_page; >> + struct gunyah_vm_memory_mapping *mapping, *tmp_mapping; >> + struct gh_rm_mem_entry *mem_entries; >> + int i, j, pinned, ret = 0; >> + struct gh_rm_mem_parcel *parcel; >> + size_t entry_size; >> + u16 vmid; > + > Reverse christmas tree to sor local variables would be nice. >
Done, and checked throughout the series as well.
>> + if (!region->memory_size || !PAGE_ALIGNED(region->memory_size) || >> + !PAGE_ALIGNED(region->userspace_addr)) > Even this alignment needs some documentation. >
Documented now.
> Or why not just let the user only pass number of pages instead of size? > >
KVM, Nitro enclaves, and ACRN all give size directly and not as # of pages.
>> + return ERR_PTR(-EINVAL); >> + >> + if (!gh_api_has_feature(GH_API_FEATURE_MEMEXTENT)) >> + return ERR_PTR(-EOPNOTSUPP); > > We should proabably move this as very first check while handling this > IOCTL. >
Done.
> >> + >> + ret = mutex_lock_interruptible(&ghvm->mm_lock); >> + if (ret) >> + return ERR_PTR(ret); >> + mapping = __gh_vm_mem_mapping_find(ghvm, region->label); > > so label is unique and userspace proabably aware of this? > Can we have more than one userspace doing this? and if so how can it > ensure that each label is unique? >
The label is unique and userspace is aware of this. One userspace owns the VM FD.
> >> + if (mapping) { >> + mutex_unlock(&ghvm->mm_lock); >> + return ERR_PTR(-EEXIST); >> + } >> + >> + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); >> + if (!mapping) { >> + ret = -ENOMEM; >> + goto unlock; >> + } >> + >> + mapping->parcel.label = region->label; > >> + mapping->guest_phys_addr = region->guest_phys_addr; >> + mapping->npages = region->memory_size >> PAGE_SHIFT; >> + parcel = &mapping->parcel; >> + parcel->mem_handle = GH_MEM_HANDLE_INVAL; /* to be filled later >> by mem_share/mem_lend */ >> + parcel->mem_type = GH_RM_MEM_TYPE_NORMAL; >> + >> + /* Check for overlap */ >> + list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) { >> + if (!((mapping->guest_phys_addr + (mapping->npages << >> PAGE_SHIFT) <= >> + tmp_mapping->guest_phys_addr) || >> + (mapping->guest_phys_addr >= >> + tmp_mapping->guest_phys_addr + (tmp_mapping->npages << >> PAGE_SHIFT)))) { >> + ret = -EEXIST; >> + goto unlock; >> + } >> + } > This looks like we will loop every mappign for each allocation giving us > an O(n), How frequent and how many max mappings can be there in the system? >
In all our use cases so far, only a few (max 3) mappings are used. Gunyah and Linux would be similarly bounded by heap-based memory constraints when adding more memory mappings if you don't hit U32_MAX first.
This could be reworked into a rb tree, but I thought it better to use a simpler list-based implementation since it was easier to review and benefits are not seen when only a few mappings used.
[snip]
>> --- a/include/uapi/linux/gunyah.h >> +++ b/include/uapi/linux/gunyah.h >> @@ -20,4 +20,26 @@ >> */ >> #define GH_CREATE_VM _IO(GH_IOCTL_TYPE, 0x0) /* Returns a >> Gunyah VM fd */ >> +/* >> + * ioctls for VM fds >> + */ >> +struct gh_userspace_memory_region { > > This struct needs some kernedoc. >
Done, as well for the other UAPI structs added later in the series.
>> + __u32 label; > >> +#define GH_MEM_ALLOW_READ (1UL << 0) >> +#define GH_MEM_ALLOW_WRITE (1UL << 1) >> +#define GH_MEM_ALLOW_EXEC (1UL << 2) >> +/* >> + * The guest will be lent the memory instead of shared. >> + * In other words, the guest has exclusive access to the memory >> region and the host loses access. >> + */ >> +#define GH_MEM_LENT (1UL << 3) >> + __u32 flags; >> + __u64 guest_phys_addr; >> + __u64 memory_size; > if we are only expecting pages, this should probably be make explict by > using nr_pages instead of size > >> + __u64 userspace_addr; >> +}; >> + >> +#define GH_VM_SET_USER_MEM_REGION _IOW(GH_IOCTL_TYPE, 0x1, \ >> + struct gh_userspace_memory_region) >> + >> #endif
| |