> -----Original Message----- > From: Paraschiv, Andra-Irina [mailto:andraprs@xxxxxxxxxx] > Sent: Thursday, September 16, 2021 1:47 AM > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > <longpeng2@xxxxxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; Gonglei (Arei) <arei.gonglei@xxxxxxxxxx>; Greg > KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Kamal Mostafa <kamal@xxxxxxxxxxxxx>; > Alexandru Vasile <lexnv@xxxxxxxxxx>; Alexandru Ciobotaru > <alcioa@xxxxxxxxxx>; Paolo Bonzini <pbonzini@xxxxxxxxxx>; Stefano Garzarella > <sgarzare@xxxxxxxxxx>; Stefan Hajnoczi <stefanha@xxxxxxxxxx>; Vitaly > Kuznetsov <vkuznets@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; > ne-devel-upstream@xxxxxxxxxx > 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@xxxxxxxxxx] > >> Sent: Wednesday, September 15, 2021 1:45 AM > >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > >> <longpeng2@xxxxxxxxxx> > >> Cc: linux-kernel@xxxxxxxxxxxxxxx; Gonglei (Arei) <arei.gonglei@xxxxxxxxxx>; > Greg > >> KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Kamal Mostafa <kamal@xxxxxxxxxxxxx>; > >> Alexandru Vasile <lexnv@xxxxxxxxxx>; Alexandru Ciobotaru > >> <alcioa@xxxxxxxxxx>; Paolo Bonzini <pbonzini@xxxxxxxxxx>; Stefano > Garzarella > >> <sgarzare@xxxxxxxxxx>; Stefan Hajnoczi <stefanha@xxxxxxxxxx>; Vitaly > >> Kuznetsov <vkuznets@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; > >> ne-devel-upstream@xxxxxxxxxx > >> 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.
