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

Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-09-15T07:28:53Z

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