Messages in this thread | | | From | "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)" <> | Subject | RE: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory regions | Date | Wed, 3 Nov 2021 13:54:18 +0000 |
| |
Hi Andra,
Sorry for the long delay.
I've read all your suggestions in each patch, there's no objection. I'll send v4 later, please review when you free, thanks.
> -----Original Message----- > From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com] > Sent: Friday, October 15, 2021 9:34 PM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@huawei.com> > Cc: Gonglei (Arei) <arei.gonglei@huawei.com>; gregkh@linuxfoundation.org; > kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com; > stefanha@redhat.com; vkuznets@redhat.com; linux-kernel@vger.kernel.org; > ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com > Subject: Re: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory > regions > > > > On 09/10/2021 04:32, Longpeng(Mike) wrote: > > > > From: Longpeng <longpeng2@huawei.com> > > > > There can be cases when there are more memory regions that need to be > > set for an enclave than the maximum supported number of memory regions > > per enclave. One example can be when the memory regions are backed by 2 > > MiB hugepages (the minimum supported hugepage size). > > > > Let's merge the adjacent regions if they are physical contiguous. This > > physical contiguous => physically contiguous > > > way the final number of memory regions is less than before merging and > > could potentially avoid reaching maximum. > > > > Signed-off-by: Longpeng <longpeng2@huawei.com> > > --- > > Changes v2 -> v3: > > - update the commit title and commit message. [Andra] > > - use 'struct range' to instead of 'struct phys_mem_region'. [Andra, Greg > KH] > > - add comments before the function definition. [Andra] > > - rename several variables, parameters and function. [Andra] > > --- > > drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 > +++++++++++++++++++++---------- > > 1 file changed, 55 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c > b/drivers/virt/nitro_enclaves/ne_misc_dev.c > > index e21e1e8..eea53e9 100644 > > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c > > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c > > @@ -26,6 +26,7 @@ > > #include <linux/poll.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > +#include <linux/range.h> > > Please keep the alphabetical order and move this before "#include > <linux/slab.h>." > > > #include <uapi/linux/vm_sockets.h> > > > > #include "ne_misc_dev.h" > > @@ -126,6 +127,16 @@ struct ne_cpu_pool { > > static struct ne_cpu_pool ne_cpu_pool; > > > > /** > > + * struct phys_contig_mem_regions - Physical contiguous memory regions > > Physical contiguous memory regions => Contiguous physical memory regions > > > + * @num: The number of regions that currently has. > > + * @region: The array of physical memory regions. > > + */ > > +struct phys_contig_mem_regions { > > + unsigned long num; > > + struct range region[0]; > > Should use "struct range *regions" instead, to keep a similar style with > regard to arrays throughout the code. > > Please align the fields name (e.g. [1]) so that it's easier to > distinguish between fields type and name. > > Let's also prefix with "ne" the data structure naming e.g. > ne_phys_contig_mem_regions. So that is similar to other data structures > defined in the NE kernel driver codebase. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri > vers/virt/nitro_enclaves/ne_misc_dev.c#n118 > > > +}; > > + > > +/** > > * ne_check_enclaves_created() - Verify if at least one enclave has been > created. > > * @void: No parameters provided. > > * > > @@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct > ne_enclave *ne_enclave, > > } > > > > /** > > + * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the > adjacent > > + * regions if they are physical > contiguous. > > physical contiguous => physically contiguous > > > + * @regions : Private data associated with the physical contiguous memory > regions. > > physical contiguous memory regions => contiguous physical memory regions > > @regions => @phys_contig_regions > > > + * @page_paddr: Physical start address of the region to be added. > > + * @page_size : Length of the region to be added. > > + * > > + * Return: > > + * * No return value. > > Please add "Context: Process context. This function is called with the > ne_enclave mutex held." before "Return", similar to other defined functions. > > Can remove these lines, as for now there is no return value: > > * Return: > * * No return value. > > And then can add this in the second patch in the series: > > * Context: Process context. This function is called with the ne_enclave > mutex held. (note: already available in the first patch) > * Return: (note: added in the second patch) > * * 0 ... > > > + */ > > +static void > > +ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions > *regions, > > + u64 page_paddr, u64 page_size) > > phys_contig_mem_regions => ne_phys_contig_mem_regions > *regions => *phys_contig_regions > > Can define something like this: > > unsigned long num = phys_contig_regions->num; > > and then use it inside the function, except the last line that > increments the num. > > > +{ > > + /* Physical contiguous, just merge */ > > Physical contiguous => Physically contiguous > > > + if (regions->num && > > + (regions->region[regions->num - 1].end + 1) == page_paddr) { > > e.g. phys_contig_regions->regions[num - 1] > > > + regions->region[regions->num - 1].end += page_size; > > + > > + return; > > + } > > + > > + regions->region[regions->num].start = page_paddr; > > + regions->region[regions->num].end = page_paddr + page_size - 1; > > + regions->num++; > > +} > > + > > +/** > > * ne_set_user_memory_region_ioctl() - Add user space memory region to the > slot > > * associated with the current enclave. > > * @ne_enclave : Private data associated with the current enclave. > > @@ -843,9 +881,9 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > unsigned long max_nr_pages = 0; > > unsigned long memory_size = 0; > > struct ne_mem_region *ne_mem_region = NULL; > > - unsigned long nr_phys_contig_mem_regions = 0; > > struct pci_dev *pdev = ne_devs.ne_pci_dev->pdev; > > - struct page **phys_contig_mem_regions = NULL; > > + struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; > > struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; => > struct ne_phys_contig_mem_regions phys_contig_mem_regions = {}; > > > + size_t size_to_alloc = 0; > > int rc = -EINVAL; > > > > rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region); > > @@ -866,8 +904,9 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > goto free_mem_region; > > } > > > > - phys_contig_mem_regions = kcalloc(max_nr_pages, > sizeof(*phys_contig_mem_regions), > > - GFP_KERNEL); > > + size_to_alloc = sizeof(*phys_contig_mem_regions) + > > + max_nr_pages * sizeof(struct range); > > + phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL); > > Then can alloc memory for the regions field. > > phys_contig_mem_regions.regions = kcalloc(max_nr_pages, > sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL); > > The "size_of_alloc" will not be needed in this case. > > > if (!phys_contig_mem_regions) { > > rc = -ENOMEM; > > > > @@ -901,26 +940,16 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > if (rc < 0) > > goto put_pages; > > > > - /* > > - * TODO: Update once handled non-contiguous memory regions > > - * received from user space or contiguous physical memory regions > > - * larger than 2 MiB e.g. 8 MiB. > > - */ > > - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; > > + > ne_merge_phys_contig_memory_regions(phys_contig_mem_regions, > > And can pass it here like this: &phys_contig_mem_regions. > > > + > page_to_phys(ne_mem_region->pages[i]), > > + > page_size(ne_mem_region->pages[i])); > > > > memory_size += page_size(ne_mem_region->pages[i]); > > > > ne_mem_region->nr_pages++; > > } while (memory_size < mem_region.memory_size); > > > > - /* > > - * TODO: Update once handled non-contiguous memory regions received > > - * from user space or contiguous physical memory regions larger than > > - * 2 MiB e.g. 8 MiB. > > - */ > > - nr_phys_contig_mem_regions = ne_mem_region->nr_pages; > > - > > - if ((ne_enclave->nr_mem_regions + nr_phys_contig_mem_regions) > > > + if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) > > > ne_enclave->max_mem_regions) { > > dev_err_ratelimited(ne_misc_dev.this_device, > > "Reached max memory regions %lld\n", > > @@ -931,9 +960,10 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > goto put_pages; > > } > > > > - for (i = 0; i < nr_phys_contig_mem_regions; i++) { > > - u64 phys_region_addr = > page_to_phys(phys_contig_mem_regions[i]); > > - u64 phys_region_size = page_size(phys_contig_mem_regions[i]); > > + for (i = 0; i < phys_contig_mem_regions->num; i++) { > > + struct range *range = phys_contig_mem_regions->region + i; > > + u64 phys_region_addr = range->start; > > + u64 phys_region_size = range_len(range); > > With the updated data structure field, could be: > > u64 phys_region_addr = phys_contig_mem_regions.regions[i].start; > u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]); > > > > > if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { > > dev_err_ratelimited(ne_misc_dev.this_device, > > @@ -959,13 +989,14 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > > > list_add(&ne_mem_region->mem_region_list_entry, > &ne_enclave->mem_regions_list); > > > > - for (i = 0; i < nr_phys_contig_mem_regions; i++) { > > + for (i = 0; i < phys_contig_mem_regions->num; i++) { > > struct ne_pci_dev_cmd_reply cmd_reply = {}; > > struct slot_add_mem_req slot_add_mem_req = {}; > > + struct range *range = phys_contig_mem_regions->region + i; > > > > slot_add_mem_req.slot_uid = ne_enclave->slot_uid; > > - slot_add_mem_req.paddr = > page_to_phys(phys_contig_mem_regions[i]); > > - slot_add_mem_req.size = > page_size(phys_contig_mem_regions[i]); > > + slot_add_mem_req.paddr = range->start; > > + slot_add_mem_req.size = range_len(range); > > Similarly here: > > slot_add_mem_req.paddr = phys_contig_mem_regions.regions[i].start; > slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]); > > Then remain the kfree paths for "phys_contig_mem_regions.regions" > instead of "phys_contig_mem_regions". > > Thanks, > Andra > > > > > rc = ne_do_request(pdev, SLOT_ADD_MEM, > > &slot_add_mem_req, > sizeof(slot_add_mem_req), > > -- > > 1.8.3.1 > > > > > > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar > Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. > Registration number J22/2621/2005.
| |