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

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

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

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