RE: [PATCH] nitro_enclaves: merge contiguous physical memory regions

["Longpeng (Mike, Cloud Infrastructure Service Product Dept.)" <longpeng2@huawei.com>]

> -----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.