* [PATCH] nitro_enclaves: merge contiguous physical memory regions @ 2021-09-14 3:14 Longpeng(Mike) 2021-09-14 17:45 ` Paraschiv, Andra-Irina 0 siblings, 1 reply; 5+ messages in thread From: Longpeng(Mike) @ 2021-09-14 3:14 UTC (permalink / raw) To: andraprs, lexnv, alcioa; +Cc: linux-kernel, arei.gonglei, Longpeng(Mike) There maybe too many physical memory regions if the memory regions backend with 2M hugetlb pages. Let's merge the adjacent regions if they are physical contiguous. Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- drivers/virt/nitro_enclaves/ne_misc_dev.c | 64 +++++++++++++++++-------------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c index e21e1e8..2920f26 100644 --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c @@ -824,6 +824,11 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave, return 0; } +struct phys_contig_mem_region { + u64 paddr; + u64 size; +}; + /** * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot * associated with the current enclave. @@ -843,9 +848,10 @@ 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_region *phys_regions = NULL; + unsigned long nr_phys_regions = 0; + u64 prev_phys_region_end; int rc = -EINVAL; rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region); @@ -866,9 +872,8 @@ 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); - if (!phys_contig_mem_regions) { + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions), GFP_KERNEL); + if (!phys_regions) { rc = -ENOMEM; goto free_mem_region; @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, /* * 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. + * received from user space. */ - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; + if (nr_phys_regions && + prev_phys_region_end == page_to_phys(ne_mem_region->pages[i])) + phys_regions[nr_phys_regions - 1].size += + page_size(ne_mem_region->pages[i]); + else { + phys_regions[nr_phys_regions].paddr = + page_to_phys(ne_mem_region->pages[i]); + phys_regions[nr_phys_regions].size = + page_size(ne_mem_region->pages[i]); + nr_phys_regions++; + } + + prev_phys_region_end = phys_regions[nr_phys_regions - 1].paddr + + phys_regions[nr_phys_regions - 1].size; 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) > - ne_enclave->max_mem_regions) { + if ((ne_enclave->nr_mem_regions + nr_phys_regions) > ne_enclave->max_mem_regions) { dev_err_ratelimited(ne_misc_dev.this_device, "Reached max memory regions %lld\n", ne_enclave->max_mem_regions); @@ -931,11 +940,8 @@ 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]); - - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { + for (i = 0; i < nr_phys_regions; i++) { + if (phys_regions[i].size & (NE_MIN_MEM_REGION_SIZE - 1)) { dev_err_ratelimited(ne_misc_dev.this_device, "Physical mem region size is not multiple of 2 MiB\n"); @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, goto put_pages; } - if (!IS_ALIGNED(phys_region_addr, NE_MIN_MEM_REGION_SIZE)) { + if (!IS_ALIGNED(phys_regions[i].paddr, NE_MIN_MEM_REGION_SIZE)) { dev_err_ratelimited(ne_misc_dev.this_device, "Physical mem region address is not 2 MiB aligned\n"); @@ -959,13 +965,13 @@ 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 < nr_phys_regions; i++) { struct ne_pci_dev_cmd_reply cmd_reply = {}; struct slot_add_mem_req slot_add_mem_req = {}; 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 = phys_regions[i].paddr; + slot_add_mem_req.size = phys_regions[i].size; rc = ne_do_request(pdev, SLOT_ADD_MEM, &slot_add_mem_req, sizeof(slot_add_mem_req), @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, dev_err_ratelimited(ne_misc_dev.this_device, "Error in slot add mem [rc=%d]\n", rc); - kfree(phys_contig_mem_regions); + kfree(phys_regions); /* * Exit here without put pages as memory regions may @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, ne_enclave->nr_mem_regions++; } - kfree(phys_contig_mem_regions); + kfree(phys_regions); return 0; @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, for (i = 0; i < ne_mem_region->nr_pages; i++) put_page(ne_mem_region->pages[i]); free_mem_region: - kfree(phys_contig_mem_regions); + kfree(phys_regions); kfree(ne_mem_region->pages); kfree(ne_mem_region); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions 2021-09-14 3:14 [PATCH] nitro_enclaves: merge contiguous physical memory regions Longpeng(Mike) @ 2021-09-14 17:45 ` Paraschiv, Andra-Irina 2021-09-15 7:28 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 5+ messages in thread From: Paraschiv, Andra-Irina @ 2021-09-14 17:45 UTC (permalink / raw) To: Longpeng(Mike) Cc: linux-kernel, arei.gonglei, Greg KH, Kamal Mostafa, Alexandru Vasile, Alexandru Ciobotaru, Paolo Bonzini, Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov, kvm, ne-devel-upstream On 14/09/2021 06:14, Longpeng(Mike) wrote: > Thanks for the patch. > There maybe too many physical memory regions if the memory regions > backend with 2M hugetlb pages. Could update something along these lines: 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. Let's merge the adjacent regions if they are physical contiguous. This way the final number of memory regions is less than before merging and could potentially avoid reaching maximum. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > drivers/virt/nitro_enclaves/ne_misc_dev.c | 64 +++++++++++++++++-------------- > 1 file changed, 35 insertions(+), 29 deletions(-) Let's see the current state of the NE user space tooling and how it interacts with the NE kernel driver. The Nitro CLI tooling sets one hugepage per memory region [1]. The NE sample from the upstream Linux kernel does the same [2]. We need to have a way to exercise this code path of merging memory regions and validate it. Let's have, for example, an update for the NE sample, if you can please add as an additional commit in the next revision. There could be an option e.g. to have both 2 MiB and 8 MiB memory regions, backed by 2 MiB hugepages. For example: /** - * NE_MIN_MEM_REGION_SIZE - Minimum size of a memory region - 2 MiB. + * NE_2MB_MEM_REGION_SIZE - Size of a memory region - 2 MiB. */ -#define NE_MIN_MEM_REGION_SIZE (2 * 1024 * 1024) +#define NE_2MB_MEM_REGION_SIZE (2 * 1024 * 1024) /** - * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 MiB set for + * NE_8MB_MEM_REGION_SIZE - Size of a memory region - 8 MiB. + */ +#define NE_8MB_MEM_REGION_SIZE (8 * 1024 * 1024) + +/** + * NE_DEFAULT_NR_2MB_MEM_REGIONS - Default number of memory regions of 2 MiB set for + * an enclave. + */ +#define NE_DEFAULT_NR_2MB_MEM_REGIONS (128) + +/** + * NE_DEFAULT_NR_8MB_MEM_REGIONS - Default number of memory regions of 8 MiB set for + * an enclave. + */ +#define NE_DEFAULT_NR_8MB_MEM_REGIONS (32) + +/** + * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 MiB and 8 MiB set for * an enclave. */ -#define NE_DEFAULT_NR_MEM_REGIONS (256) +#define NE_DEFAULT_NR_MEM_REGIONS (NE_DEFAULT_NR_2MB_MEM_REGIONS + NE_DEFAULT_NR_8MB_MEM_REGIONS) Then the logic from [2] can be updated to allocate the first part, the 2 MiB memory regions, then the second part, the 8 MiB memory regions. Open for other options as well to cover the validation for the regions merging case. > > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c > index e21e1e8..2920f26 100644 > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c > @@ -824,6 +824,11 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave, > return 0; > } > > +struct phys_contig_mem_region { > + u64 paddr; > + u64 size; > +}; > + > /** > * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot > * associated with the current enclave. > @@ -843,9 +848,10 @@ 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_region *phys_regions = NULL; > + unsigned long nr_phys_regions = 0; > + u64 prev_phys_region_end; It's indeed initialized later on and it's not used till then, but let's just have an init to 0 here as well. > int rc = -EINVAL; > > rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region); > @@ -866,9 +872,8 @@ 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); > - if (!phys_contig_mem_regions) { > + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions), GFP_KERNEL); > + if (!phys_regions) { > rc = -ENOMEM; > > goto free_mem_region; > @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, > > /* > * 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. > + * received from user space. > */ Can remove this TODO completely, similar as below. > - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; > + if (nr_phys_regions && > + prev_phys_region_end == page_to_phys(ne_mem_region->pages[i])) > + phys_regions[nr_phys_regions - 1].size += > + page_size(ne_mem_region->pages[i]); Please add parantheses here as well. if ... { } else { } > + else { > + phys_regions[nr_phys_regions].paddr = > + page_to_phys(ne_mem_region->pages[i]); > + phys_regions[nr_phys_regions].size = > + page_size(ne_mem_region->pages[i]); > + nr_phys_regions++; > + } > + > + prev_phys_region_end = phys_regions[nr_phys_regions - 1].paddr + > + phys_regions[nr_phys_regions - 1].size; > > 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) > > - ne_enclave->max_mem_regions) { > + if ((ne_enclave->nr_mem_regions + nr_phys_regions) > ne_enclave->max_mem_regions) { > dev_err_ratelimited(ne_misc_dev.this_device, > "Reached max memory regions %lld\n", > ne_enclave->max_mem_regions); > @@ -931,11 +940,8 @@ 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]); > - > - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { > + for (i = 0; i < nr_phys_regions; i++) { > + if (phys_regions[i].size & (NE_MIN_MEM_REGION_SIZE - 1)) { > dev_err_ratelimited(ne_misc_dev.this_device, > "Physical mem region size is not multiple of 2 MiB\n"); > > @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, > goto put_pages; > } > > - if (!IS_ALIGNED(phys_region_addr, NE_MIN_MEM_REGION_SIZE)) { > + if (!IS_ALIGNED(phys_regions[i].paddr, NE_MIN_MEM_REGION_SIZE)) { > dev_err_ratelimited(ne_misc_dev.this_device, > "Physical mem region address is not 2 MiB aligned\n"); > > @@ -959,13 +965,13 @@ 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 < nr_phys_regions; i++) { > struct ne_pci_dev_cmd_reply cmd_reply = {}; > struct slot_add_mem_req slot_add_mem_req = {}; > > 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 = phys_regions[i].paddr; > + slot_add_mem_req.size = phys_regions[i].size; > > rc = ne_do_request(pdev, SLOT_ADD_MEM, > &slot_add_mem_req, sizeof(slot_add_mem_req), > @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, > dev_err_ratelimited(ne_misc_dev.this_device, > "Error in slot add mem [rc=%d]\n", rc); > > - kfree(phys_contig_mem_regions); > + kfree(phys_regions); > > /* > * Exit here without put pages as memory regions may > @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, > ne_enclave->nr_mem_regions++; > } > > - kfree(phys_contig_mem_regions); > + kfree(phys_regions); > > return 0; > > @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave, > for (i = 0; i < ne_mem_region->nr_pages; i++) > put_page(ne_mem_region->pages[i]); > free_mem_region: > - kfree(phys_contig_mem_regions); > + kfree(phys_regions); > kfree(ne_mem_region->pages); > kfree(ne_mem_region); > > -- > 1.8.3.1 > Please also add the changelog after the "---" line in the patches so that we can track changes between revisions. Thanks, Andra [1] https://github.com/aws/aws-nitro-enclaves-cli/blob/main/src/enclave_proc/resource_manager.rs#L211 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/nitro_enclaves/ne_ioctl_sample.c#n817 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] nitro_enclaves: merge contiguous physical memory regions 2021-09-14 17:45 ` Paraschiv, Andra-Irina @ 2021-09-15 7:28 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2021-09-15 17:46 ` Paraschiv, Andra-Irina 0 siblings, 1 reply; 5+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2021-09-15 7:28 UTC (permalink / raw) To: Paraschiv, Andra-Irina Cc: linux-kernel, Gonglei (Arei), Greg KH, Kamal Mostafa, Alexandru Vasile, Alexandru Ciobotaru, Paolo Bonzini, Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov, kvm, ne-devel-upstream > -----Original Message----- > From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com] > Sent: Wednesday, September 15, 2021 1:45 AM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@huawei.com> > Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>; Greg > KH <gregkh@linuxfoundation.org>; Kamal Mostafa <kamal@canonical.com>; > Alexandru Vasile <lexnv@amazon.com>; Alexandru Ciobotaru > <alcioa@amazon.com>; Paolo Bonzini <pbonzini@redhat.com>; Stefano Garzarella > <sgarzare@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Vitaly > Kuznetsov <vkuznets@redhat.com>; kvm@vger.kernel.org; > ne-devel-upstream@amazon.com > Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions > > > > On 14/09/2021 06:14, Longpeng(Mike) wrote: > > > > Thanks for the patch. > > > There maybe too many physical memory regions if the memory regions > > backend with 2M hugetlb pages. > > Could update something along these lines: > > 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. > > Let's merge the adjacent regions if they are physical contiguous. This > way the final number of memory regions is less than before merging and > could potentially avoid reaching maximum. > Okay, it looks much better, thanks. > > > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > > --- > > drivers/virt/nitro_enclaves/ne_misc_dev.c | 64 +++++++++++++++++-------------- > > 1 file changed, 35 insertions(+), 29 deletions(-) > > Let's see the current state of the NE user space tooling and how it > interacts with the NE kernel driver. > > The Nitro CLI tooling sets one hugepage per memory region [1]. The NE > sample from the upstream Linux kernel does the same [2]. > > We need to have a way to exercise this code path of merging memory > regions and validate it. Let's have, for example, an update for the NE > sample, if you can please add as an additional commit in the next revision. > > There could be an option e.g. to have both 2 MiB and 8 MiB memory > regions, backed by 2 MiB hugepages. For example: > > > /** > - * NE_MIN_MEM_REGION_SIZE - Minimum size of a memory region - 2 MiB. > + * NE_2MB_MEM_REGION_SIZE - Size of a memory region - 2 MiB. > */ > -#define NE_MIN_MEM_REGION_SIZE (2 * 1024 * 1024) > +#define NE_2MB_MEM_REGION_SIZE (2 * 1024 * 1024) > > /** > - * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 > MiB set for > + * NE_8MB_MEM_REGION_SIZE - Size of a memory region - 8 MiB. > + */ > +#define NE_8MB_MEM_REGION_SIZE (8 * 1024 * 1024) > + > +/** > + * NE_DEFAULT_NR_2MB_MEM_REGIONS - Default number of memory regions of > 2 MiB set for > + * an enclave. > + */ > +#define NE_DEFAULT_NR_2MB_MEM_REGIONS (128) > + > +/** > + * NE_DEFAULT_NR_8MB_MEM_REGIONS - Default number of memory regions of > 8 MiB set for > + * an enclave. > + */ > +#define NE_DEFAULT_NR_8MB_MEM_REGIONS (32) > + > +/** > + * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 > MiB and 8 MiB set for > * an enclave. > */ > -#define NE_DEFAULT_NR_MEM_REGIONS (256) > +#define NE_DEFAULT_NR_MEM_REGIONS > (NE_DEFAULT_NR_2MB_MEM_REGIONS + > NE_DEFAULT_NR_8MB_MEM_REGIONS) > > > > Then the logic from [2] can be updated to allocate the first part, the 2 > MiB memory regions, then the second part, the 8 MiB memory regions. > > Open for other options as well to cover the validation for the regions > merging case. > Okay, will do in the next version. By the way, I have another question here. The nitro_enclaves.ko consists of two parts: the misc part and the pci_dev part. The main logic is in the misc part, the pci_dev part is responsible for the device management and command routing. So how about to decouple these two parts, e.g. split the nitro_enclaves.ko into ne_ioctl.ko and ne_pdev.ko ? In this way, we can allow other modules to connect with ne_ioctl.ko: ne_ioctl.ko ---/ | --\ ---/ | -----\ ---/ | ----\ ne_pdev.ko test_pdev.ko others... With test_pdev.ko, we can validate the core logic of the ne_ioctl.ko in local. What's more, other vendor's device could also support software enclave feature if they follow the specification. > > > > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c > b/drivers/virt/nitro_enclaves/ne_misc_dev.c > > index e21e1e8..2920f26 100644 > > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c > > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c > > @@ -824,6 +824,11 @@ static int > ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave, > > return 0; > > } > > > > +struct phys_contig_mem_region { > > + u64 paddr; > > + u64 size; > > +}; > > + > > /** > > * ne_set_user_memory_region_ioctl() - Add user space memory region to > the slot > > * associated with the current > enclave. > > @@ -843,9 +848,10 @@ 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_region *phys_regions = NULL; > > + unsigned long nr_phys_regions = 0; > > + u64 prev_phys_region_end; > > It's indeed initialized later on and it's not used till then, but let's > just have an init to 0 here as well. > Okay. > > int rc = -EINVAL; > > > > rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region); > > @@ -866,9 +872,8 @@ 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); > > - if (!phys_contig_mem_regions) { > > + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions), > GFP_KERNEL); > > + if (!phys_regions) { > > rc = -ENOMEM; > > > > goto free_mem_region; > > @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > > > /* > > * 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. > > + * received from user space. > > */ > > Can remove this TODO completely, similar as below. > Okay. > > - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; > > + if (nr_phys_regions && > > + prev_phys_region_end == > page_to_phys(ne_mem_region->pages[i])) > > + phys_regions[nr_phys_regions - 1].size += > > + > page_size(ne_mem_region->pages[i]); > > Please add parantheses here as well. > > if ... { > > > } else { > > } > > > + else { > > + phys_regions[nr_phys_regions].paddr = > > + > page_to_phys(ne_mem_region->pages[i]); > > + phys_regions[nr_phys_regions].size = > > + > page_size(ne_mem_region->pages[i]); > > + nr_phys_regions++; > > + } > > + > > + prev_phys_region_end = phys_regions[nr_phys_regions - > 1].paddr + > > + phys_regions[nr_phys_regions - > 1].size; > > > > 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) > > > - ne_enclave->max_mem_regions) { > > + if ((ne_enclave->nr_mem_regions + nr_phys_regions) > > ne_enclave->max_mem_regions) { > > dev_err_ratelimited(ne_misc_dev.this_device, > > "Reached max memory > regions %lld\n", > > ne_enclave->max_mem_regions); > > @@ -931,11 +940,8 @@ 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]); > > - > > - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { > > + for (i = 0; i < nr_phys_regions; i++) { > > + if (phys_regions[i].size & (NE_MIN_MEM_REGION_SIZE - 1)) { > > dev_err_ratelimited(ne_misc_dev.this_device, > > "Physical mem region > size is not multiple of 2 MiB\n"); > > > > @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > goto put_pages; > > } > > > > - if (!IS_ALIGNED(phys_region_addr, > NE_MIN_MEM_REGION_SIZE)) { > > + if (!IS_ALIGNED(phys_regions[i].paddr, > NE_MIN_MEM_REGION_SIZE)) { > > dev_err_ratelimited(ne_misc_dev.this_device, > > "Physical mem region > address is not 2 MiB aligned\n"); > > > > @@ -959,13 +965,13 @@ 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 < nr_phys_regions; i++) { > > struct ne_pci_dev_cmd_reply cmd_reply = {}; > > struct slot_add_mem_req slot_add_mem_req = {}; > > > > 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 = phys_regions[i].paddr; > > + slot_add_mem_req.size = phys_regions[i].size; > > > > rc = ne_do_request(pdev, SLOT_ADD_MEM, > > &slot_add_mem_req, > sizeof(slot_add_mem_req), > > @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > dev_err_ratelimited(ne_misc_dev.this_device, > > "Error in slot add mem > [rc=%d]\n", rc); > > > > - kfree(phys_contig_mem_regions); > > + kfree(phys_regions); > > > > /* > > * Exit here without put pages as memory regions > may > > @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > ne_enclave->nr_mem_regions++; > > } > > > > - kfree(phys_contig_mem_regions); > > + kfree(phys_regions); > > > > return 0; > > > > @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct > ne_enclave *ne_enclave, > > for (i = 0; i < ne_mem_region->nr_pages; i++) > > put_page(ne_mem_region->pages[i]); > > free_mem_region: > > - kfree(phys_contig_mem_regions); > > + kfree(phys_regions); > > kfree(ne_mem_region->pages); > > kfree(ne_mem_region); > > > > -- > > 1.8.3.1 > > > > Please also add the changelog after the "---" line in the patches so > that we can track changes between revisions. > Got it, thanks. > Thanks, > Andra > > [1] > https://github.com/aws/aws-nitro-enclaves-cli/blob/main/src/enclave_proc/resour > ce_manager.rs#L211 > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/nitr > o_enclaves/ne_ioctl_sample.c#n817 > > > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions 2021-09-15 7:28 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2021-09-15 17:46 ` Paraschiv, Andra-Irina 2021-09-16 2:26 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 1 reply; 5+ messages in thread From: Paraschiv, Andra-Irina @ 2021-09-15 17:46 UTC (permalink / raw) To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) Cc: linux-kernel, Gonglei (Arei), Greg KH, Kamal Mostafa, Alexandru Vasile, Alexandru Ciobotaru, Paolo Bonzini, Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov, kvm, ne-devel-upstream On 15/09/2021 10:28, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: >> -----Original Message----- >> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com] >> Sent: Wednesday, September 15, 2021 1:45 AM >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) >> <longpeng2@huawei.com> >> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>; Greg >> KH <gregkh@linuxfoundation.org>; Kamal Mostafa <kamal@canonical.com>; >> Alexandru Vasile <lexnv@amazon.com>; Alexandru Ciobotaru >> <alcioa@amazon.com>; Paolo Bonzini <pbonzini@redhat.com>; Stefano Garzarella >> <sgarzare@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Vitaly >> Kuznetsov <vkuznets@redhat.com>; kvm@vger.kernel.org; >> ne-devel-upstream@amazon.com >> Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions >> >> >> >> On 14/09/2021 06:14, Longpeng(Mike) wrote: >>> >> >> Thanks for the patch. >> >>> There maybe too many physical memory regions if the memory regions >>> backend with 2M hugetlb pages. >> >> Could update something along these lines: >> >> 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. >> >> Let's merge the adjacent regions if they are physical contiguous. This >> way the final number of memory regions is less than before merging and >> could potentially avoid reaching maximum. >> > > Okay, it looks much better, thanks. > >>> >>> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >>> --- >>> drivers/virt/nitro_enclaves/ne_misc_dev.c | 64 +++++++++++++++++-------------- >>> 1 file changed, 35 insertions(+), 29 deletions(-) >> >> Let's see the current state of the NE user space tooling and how it >> interacts with the NE kernel driver. >> >> The Nitro CLI tooling sets one hugepage per memory region [1]. The NE >> sample from the upstream Linux kernel does the same [2]. >> >> We need to have a way to exercise this code path of merging memory >> regions and validate it. Let's have, for example, an update for the NE >> sample, if you can please add as an additional commit in the next revision. >> >> There could be an option e.g. to have both 2 MiB and 8 MiB memory >> regions, backed by 2 MiB hugepages. For example: >> >> >> /** >> - * NE_MIN_MEM_REGION_SIZE - Minimum size of a memory region - 2 MiB. >> + * NE_2MB_MEM_REGION_SIZE - Size of a memory region - 2 MiB. >> */ >> -#define NE_MIN_MEM_REGION_SIZE (2 * 1024 * 1024) >> +#define NE_2MB_MEM_REGION_SIZE (2 * 1024 * 1024) >> >> /** >> - * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 >> MiB set for >> + * NE_8MB_MEM_REGION_SIZE - Size of a memory region - 8 MiB. >> + */ >> +#define NE_8MB_MEM_REGION_SIZE (8 * 1024 * 1024) >> + >> +/** >> + * NE_DEFAULT_NR_2MB_MEM_REGIONS - Default number of memory regions of >> 2 MiB set for >> + * an enclave. >> + */ >> +#define NE_DEFAULT_NR_2MB_MEM_REGIONS (128) >> + >> +/** >> + * NE_DEFAULT_NR_8MB_MEM_REGIONS - Default number of memory regions of >> 8 MiB set for >> + * an enclave. >> + */ >> +#define NE_DEFAULT_NR_8MB_MEM_REGIONS (32) >> + >> +/** >> + * NE_DEFAULT_NR_MEM_REGIONS - Default number of memory regions of 2 >> MiB and 8 MiB set for >> * an enclave. >> */ >> -#define NE_DEFAULT_NR_MEM_REGIONS (256) >> +#define NE_DEFAULT_NR_MEM_REGIONS >> (NE_DEFAULT_NR_2MB_MEM_REGIONS + >> NE_DEFAULT_NR_8MB_MEM_REGIONS) >> >> >> >> Then the logic from [2] can be updated to allocate the first part, the 2 >> MiB memory regions, then the second part, the 8 MiB memory regions. >> >> Open for other options as well to cover the validation for the regions >> merging case. >> > > Okay, will do in the next version. > > By the way, I have another question here. > > The nitro_enclaves.ko consists of two parts: the misc part and the pci_dev part. > The main logic is in the misc part, the pci_dev part is responsible for the > device management and command routing. > > So how about to decouple these two parts, e.g. split the nitro_enclaves.ko into > ne_ioctl.ko and ne_pdev.ko ? In this way, we can allow other modules to connect > with ne_ioctl.ko: > > ne_ioctl.ko > ---/ | --\ > ---/ | -----\ > ---/ | ----\ > ne_pdev.ko test_pdev.ko others... > > > With test_pdev.ko, we can validate the core logic of the ne_ioctl.ko in local. > What's more, other vendor's device could also support software enclave feature > if they follow the specification. During the discussions for the initial merge there has been specific feedback to make the misc functionality available only if the PCI device is present, as they are coming altogether. In addition, there are userspace tools, configurations, setups available in different environments, that rely on the current structure of the NE kernel driver, and changes would break the compatibility. Thanks, Andra > >>> >>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c >> b/drivers/virt/nitro_enclaves/ne_misc_dev.c >>> index e21e1e8..2920f26 100644 >>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c >>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c >>> @@ -824,6 +824,11 @@ static int >> ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave, >>> return 0; >>> } >>> >>> +struct phys_contig_mem_region { >>> + u64 paddr; >>> + u64 size; >>> +}; >>> + >>> /** >>> * ne_set_user_memory_region_ioctl() - Add user space memory region to >> the slot >>> * associated with the current >> enclave. >>> @@ -843,9 +848,10 @@ 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_region *phys_regions = NULL; >>> + unsigned long nr_phys_regions = 0; >>> + u64 prev_phys_region_end; >> >> It's indeed initialized later on and it's not used till then, but let's >> just have an init to 0 here as well. >> > > Okay. > >>> int rc = -EINVAL; >>> >>> rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region); >>> @@ -866,9 +872,8 @@ 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); >>> - if (!phys_contig_mem_regions) { >>> + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions), >> GFP_KERNEL); >>> + if (!phys_regions) { >>> rc = -ENOMEM; >>> >>> goto free_mem_region; >>> @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct >> ne_enclave *ne_enclave, >>> >>> /* >>> * 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. >>> + * received from user space. >>> */ >> >> Can remove this TODO completely, similar as below. >> > > Okay. > >>> - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; >>> + if (nr_phys_regions && >>> + prev_phys_region_end == >> page_to_phys(ne_mem_region->pages[i])) >>> + phys_regions[nr_phys_regions - 1].size += >>> + >> page_size(ne_mem_region->pages[i]); >> >> Please add parantheses here as well. >> >> if ... { >> >> >> } else { >> >> } >> >>> + else { >>> + phys_regions[nr_phys_regions].paddr = >>> + >> page_to_phys(ne_mem_region->pages[i]); >>> + phys_regions[nr_phys_regions].size = >>> + >> page_size(ne_mem_region->pages[i]); >>> + nr_phys_regions++; >>> + } >>> + >>> + prev_phys_region_end = phys_regions[nr_phys_regions - >> 1].paddr + >>> + phys_regions[nr_phys_regions - >> 1].size; >>> >>> 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) > >>> - ne_enclave->max_mem_regions) { >>> + if ((ne_enclave->nr_mem_regions + nr_phys_regions) > >> ne_enclave->max_mem_regions) { >>> dev_err_ratelimited(ne_misc_dev.this_device, >>> "Reached max memory >> regions %lld\n", >>> ne_enclave->max_mem_regions); >>> @@ -931,11 +940,8 @@ 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]); >>> - >>> - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { >>> + for (i = 0; i < nr_phys_regions; i++) { >>> + if (phys_regions[i].size & (NE_MIN_MEM_REGION_SIZE - 1)) { >>> dev_err_ratelimited(ne_misc_dev.this_device, >>> "Physical mem region >> size is not multiple of 2 MiB\n"); >>> >>> @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct >> ne_enclave *ne_enclave, >>> goto put_pages; >>> } >>> >>> - if (!IS_ALIGNED(phys_region_addr, >> NE_MIN_MEM_REGION_SIZE)) { >>> + if (!IS_ALIGNED(phys_regions[i].paddr, >> NE_MIN_MEM_REGION_SIZE)) { >>> dev_err_ratelimited(ne_misc_dev.this_device, >>> "Physical mem region >> address is not 2 MiB aligned\n"); >>> >>> @@ -959,13 +965,13 @@ 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 < nr_phys_regions; i++) { >>> struct ne_pci_dev_cmd_reply cmd_reply = {}; >>> struct slot_add_mem_req slot_add_mem_req = {}; >>> >>> 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 = phys_regions[i].paddr; >>> + slot_add_mem_req.size = phys_regions[i].size; >>> >>> rc = ne_do_request(pdev, SLOT_ADD_MEM, >>> &slot_add_mem_req, >> sizeof(slot_add_mem_req), >>> @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct >> ne_enclave *ne_enclave, >>> dev_err_ratelimited(ne_misc_dev.this_device, >>> "Error in slot add mem >> [rc=%d]\n", rc); >>> >>> - kfree(phys_contig_mem_regions); >>> + kfree(phys_regions); >>> >>> /* >>> * Exit here without put pages as memory regions >> may >>> @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct >> ne_enclave *ne_enclave, >>> ne_enclave->nr_mem_regions++; >>> } >>> >>> - kfree(phys_contig_mem_regions); >>> + kfree(phys_regions); >>> >>> return 0; >>> >>> @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct >> ne_enclave *ne_enclave, >>> for (i = 0; i < ne_mem_region->nr_pages; i++) >>> put_page(ne_mem_region->pages[i]); >>> free_mem_region: >>> - kfree(phys_contig_mem_regions); >>> + kfree(phys_regions); >>> kfree(ne_mem_region->pages); >>> kfree(ne_mem_region); >>> >>> -- >>> 1.8.3.1 >>> >> >> Please also add the changelog after the "---" line in the patches so >> that we can track changes between revisions. >> > > Got it, thanks. > >> Thanks, >> Andra >> >> [1] >> https://github.com/aws/aws-nitro-enclaves-cli/blob/main/src/enclave_proc/resour >> ce_manager.rs#L211 >> [2] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/nitr >> o_enclaves/ne_ioctl_sample.c#n817 >> >> >> >> 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. 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] nitro_enclaves: merge contiguous physical memory regions 2021-09-15 17:46 ` Paraschiv, Andra-Irina @ 2021-09-16 2:26 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 0 siblings, 0 replies; 5+ messages in thread From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2021-09-16 2:26 UTC (permalink / raw) To: Paraschiv, Andra-Irina Cc: linux-kernel, Gonglei (Arei), Greg KH, Kamal Mostafa, Alexandru Vasile, Alexandru Ciobotaru, Paolo Bonzini, Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov, kvm, ne-devel-upstream > -----Original Message----- > From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com] > Sent: Thursday, September 16, 2021 1:47 AM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@huawei.com> > Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>; Greg > KH <gregkh@linuxfoundation.org>; Kamal Mostafa <kamal@canonical.com>; > Alexandru Vasile <lexnv@amazon.com>; Alexandru Ciobotaru > <alcioa@amazon.com>; Paolo Bonzini <pbonzini@redhat.com>; Stefano Garzarella > <sgarzare@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Vitaly > Kuznetsov <vkuznets@redhat.com>; kvm@vger.kernel.org; > ne-devel-upstream@amazon.com > Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions > > > > On 15/09/2021 10:28, Longpeng (Mike, Cloud Infrastructure Service > Product Dept.) wrote: > >> -----Original Message----- > >> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com] > >> Sent: Wednesday, September 15, 2021 1:45 AM > >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > >> <longpeng2@huawei.com> > >> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>; > Greg > >> KH <gregkh@linuxfoundation.org>; Kamal Mostafa <kamal@canonical.com>; > >> Alexandru Vasile <lexnv@amazon.com>; Alexandru Ciobotaru > >> <alcioa@amazon.com>; Paolo Bonzini <pbonzini@redhat.com>; Stefano > Garzarella > >> <sgarzare@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Vitaly > >> Kuznetsov <vkuznets@redhat.com>; kvm@vger.kernel.org; > >> ne-devel-upstream@amazon.com > >> Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions > >> > >> [snip] > >> Then the logic from [2] can be updated to allocate the first part, the 2 > >> MiB memory regions, then the second part, the 8 MiB memory regions. > >> > >> Open for other options as well to cover the validation for the regions > >> merging case. > >> > > > > Okay, will do in the next version. > > > > By the way, I have another question here. > > > > The nitro_enclaves.ko consists of two parts: the misc part and the pci_dev part. > > The main logic is in the misc part, the pci_dev part is responsible for the > > device management and command routing. > > > > So how about to decouple these two parts, e.g. split the nitro_enclaves.ko into > > ne_ioctl.ko and ne_pdev.ko ? In this way, we can allow other modules to > connect > > with ne_ioctl.ko: > > > > ne_ioctl.ko > > ---/ | --\ > > ---/ | -----\ > > ---/ | ----\ > > ne_pdev.ko test_pdev.ko others... > > > > > > With test_pdev.ko, we can validate the core logic of the ne_ioctl.ko in local. > > What's more, other vendor's device could also support software enclave feature > > if they follow the specification. > > > During the discussions for the initial merge there has been specific > feedback to make the misc functionality available only if the PCI device > is present, as they are coming altogether. In addition, there are > userspace tools, configurations, setups available in different > environments, that rely on the current structure of the NE kernel > driver, and changes would break the compatibility. > Got it, thanks :) I have an alternative to support to validate the misc functionality in local, I'll send it together in the next version. > Thanks, > Andra > > > > > >>> > >>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c > >> b/drivers/virt/nitro_enclaves/ne_misc_dev.c > >>> index e21e1e8..2920f26 100644 > >>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c > >>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c > >>> @@ -824,6 +824,11 @@ static int > >> ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave, > >>> return 0; > >>> } > >>> > >>> +struct phys_contig_mem_region { > >>> + u64 paddr; > >>> + u64 size; > >>> +}; > >>> + > >>> /** > >>> * ne_set_user_memory_region_ioctl() - Add user space memory region to > >> the slot > >>> * associated with the current > >> enclave. > >>> @@ -843,9 +848,10 @@ 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_region *phys_regions = NULL; > >>> + unsigned long nr_phys_regions = 0; > >>> + u64 prev_phys_region_end; > >> > >> It's indeed initialized later on and it's not used till then, but let's > >> just have an init to 0 here as well. > >> > > > > Okay. > > > >>> int rc = -EINVAL; > >>> > >>> rc = ne_sanity_check_user_mem_region(ne_enclave, > mem_region); > >>> @@ -866,9 +872,8 @@ 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); > >>> - if (!phys_contig_mem_regions) { > >>> + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions), > >> GFP_KERNEL); > >>> + if (!phys_regions) { > >>> rc = -ENOMEM; > >>> > >>> goto free_mem_region; > >>> @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct > >> ne_enclave *ne_enclave, > >>> > >>> /* > >>> * 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. > >>> + * received from user space. > >>> */ > >> > >> Can remove this TODO completely, similar as below. > >> > > > > Okay. > > > >>> - phys_contig_mem_regions[i] = ne_mem_region->pages[i]; > >>> + if (nr_phys_regions && > >>> + prev_phys_region_end == > >> page_to_phys(ne_mem_region->pages[i])) > >>> + phys_regions[nr_phys_regions - 1].size += > >>> + > >> page_size(ne_mem_region->pages[i]); > >> > >> Please add parantheses here as well. > >> > >> if ... { > >> > >> > >> } else { > >> > >> } > >> > >>> + else { > >>> + phys_regions[nr_phys_regions].paddr = > >>> + > >> page_to_phys(ne_mem_region->pages[i]); > >>> + phys_regions[nr_phys_regions].size = > >>> + > >> page_size(ne_mem_region->pages[i]); > >>> + nr_phys_regions++; > >>> + } > >>> + > >>> + prev_phys_region_end = phys_regions[nr_phys_regions - > >> 1].paddr + > >>> + > phys_regions[nr_phys_regions - > >> 1].size; > >>> > >>> 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) > > >>> - ne_enclave->max_mem_regions) { > >>> + if ((ne_enclave->nr_mem_regions + nr_phys_regions) > > >> ne_enclave->max_mem_regions) { > >>> dev_err_ratelimited(ne_misc_dev.this_device, > >>> "Reached max memory > >> regions %lld\n", > >>> > ne_enclave->max_mem_regions); > >>> @@ -931,11 +940,8 @@ 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]); > >>> - > >>> - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) { > >>> + for (i = 0; i < nr_phys_regions; i++) { > >>> + if (phys_regions[i].size & (NE_MIN_MEM_REGION_SIZE - 1)) > { > >>> > dev_err_ratelimited(ne_misc_dev.this_device, > >>> "Physical mem region > >> size is not multiple of 2 MiB\n"); > >>> > >>> @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct > >> ne_enclave *ne_enclave, > >>> goto put_pages; > >>> } > >>> > >>> - if (!IS_ALIGNED(phys_region_addr, > >> NE_MIN_MEM_REGION_SIZE)) { > >>> + if (!IS_ALIGNED(phys_regions[i].paddr, > >> NE_MIN_MEM_REGION_SIZE)) { > >>> > dev_err_ratelimited(ne_misc_dev.this_device, > >>> "Physical mem region > >> address is not 2 MiB aligned\n"); > >>> > >>> @@ -959,13 +965,13 @@ 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 < nr_phys_regions; i++) { > >>> struct ne_pci_dev_cmd_reply cmd_reply = {}; > >>> struct slot_add_mem_req slot_add_mem_req = {}; > >>> > >>> 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 = phys_regions[i].paddr; > >>> + slot_add_mem_req.size = phys_regions[i].size; > >>> > >>> rc = ne_do_request(pdev, SLOT_ADD_MEM, > >>> &slot_add_mem_req, > >> sizeof(slot_add_mem_req), > >>> @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct > >> ne_enclave *ne_enclave, > >>> > dev_err_ratelimited(ne_misc_dev.this_device, > >>> "Error in slot add > mem > >> [rc=%d]\n", rc); > >>> > >>> - kfree(phys_contig_mem_regions); > >>> + kfree(phys_regions); > >>> > >>> /* > >>> * Exit here without put pages as memory > regions > >> may > >>> @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct > >> ne_enclave *ne_enclave, > >>> ne_enclave->nr_mem_regions++; > >>> } > >>> > >>> - kfree(phys_contig_mem_regions); > >>> + kfree(phys_regions); > >>> > >>> return 0; > >>> > >>> @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct > >> ne_enclave *ne_enclave, > >>> for (i = 0; i < ne_mem_region->nr_pages; i++) > >>> put_page(ne_mem_region->pages[i]); > >>> free_mem_region: > >>> - kfree(phys_contig_mem_regions); > >>> + kfree(phys_regions); > >>> kfree(ne_mem_region->pages); > >>> kfree(ne_mem_region); > >>> > >>> -- > >>> 1.8.3.1 > >>> > >> > >> Please also add the changelog after the "---" line in the patches so > >> that we can track changes between revisions. > >> > > > > Got it, thanks. > > > >> Thanks, > >> Andra > >> > >> [1] > >> > https://github.com/aws/aws-nitro-enclaves-cli/blob/main/src/enclave_proc/resour > >> ce_manager.rs#L211 > >> [2] > >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/nitr > >> o_enclaves/ne_ioctl_sample.c#n817 > >> > >> > >> > >> 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. > > > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-16 2:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 3:14 [PATCH] nitro_enclaves: merge contiguous physical memory regions Longpeng(Mike) 2021-09-14 17:45 ` Paraschiv, Andra-Irina 2021-09-15 7:28 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.) 2021-09-15 17:46 ` Paraschiv, Andra-Irina 2021-09-16 2:26 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).