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

["Paraschiv, Andra-Irina" <andraprs@xxxxxxxxxx>]

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