[PATCH v2 0/4] merge contiguous physical memory regions

[PATCH v2 0/4] merge contiguous physical memory regions

[Longpeng(Mike)]

Hi guys,

This patchset try to merge the contiguous physical memory regions when
set user memory regions, you can see message in PATCH 1 for details.
Please review when you free, thank!

Changes v1 -> v2:
  - update the commit message as Andra's suggestion  [Andra]
  - remove TODO completely in ne_set_user_memory_region_ioctl  [Andra]
  - extract the physical memory regions setup into individual
    function
  - add kunit tests  [Andra]

Longpeng(Mike) (4):
  nitro_enclaves: merge contiguous physical memory regions
  nitro_enclaves: sanity check the physical region during setting
  nitro_enclaves: add test framework for the misc functionality
  nitro_enclaves: add kunit tests for physical contiguous region merging

 drivers/virt/nitro_enclaves/Kconfig        |   8 ++
 drivers/virt/nitro_enclaves/ne_misc_dev.c  | 160 ++++++++++++++++++++---------
 drivers/virt/nitro_enclaves/ne_misc_test.c |  63 ++++++++++++
 3 files changed, 182 insertions(+), 49 deletions(-)
 create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c

-- 
1.8.3.1

[PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

[Longpeng(Mike)]

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. This
way the final number of memory regions is less than before merging and
could potentially avoid reaching maximum.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/virt/nitro_enclaves/ne_misc_dev.c | 87 ++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 29 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index e21e1e8..a4776fc 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -126,6 +126,26 @@ struct ne_cpu_pool {
 static struct ne_cpu_pool ne_cpu_pool;
 
 /**
+ * struct phys_mem_region - Physical memory region
+ * @paddr:	The start physical address of the region.
+ * @size:	The sizeof of the region.
+ */
+struct phys_mem_region {
+	u64 paddr;
+	u64 size;
+};
+
+/**
+ * struct phys_contig_mem_region - Physical contiguous memory regions
+ * @num:	The number of regions that currently has.
+ * @region:	The array of physical memory regions.
+ */
+struct phys_contig_mem_region {
+	unsigned long num;
+	struct phys_mem_region region[0];
+};
+
+/**
  * ne_check_enclaves_created() - Verify if at least one enclave has been created.
  * @void:	No parameters provided.
  *
@@ -824,6 +844,27 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
 	return 0;
 }
 
+static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
+				      u64 paddr, u64 size)
+{
+	u64 prev_phys_region_end = 0;
+
+	if (regions->num) {
+		prev_phys_region_end = regions->region[regions->num - 1].paddr +
+					regions->region[regions->num - 1].size;
+
+		/* Physical contiguous, just merge */
+		if (prev_phys_region_end == paddr) {
+			regions->region[regions->num - 1].size += size;
+			return;
+		}
+	}
+
+	regions->region[regions->num].paddr = paddr;
+	regions->region[regions->num].size = size;
+	regions->num++;
+}
+
 /**
  * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot
  *				       associated with the current enclave.
@@ -843,9 +884,9 @@ 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;
+	size_t size_to_alloc = 0;
 	int rc = -EINVAL;
 
 	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
@@ -866,9 +907,9 @@ 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) {
+	size_to_alloc = sizeof(*phys_regions) + max_nr_pages * sizeof(struct phys_mem_region);
+	phys_regions = kzalloc(size_to_alloc, GFP_KERNEL);
+	if (!phys_regions) {
 		rc = -ENOMEM;
 
 		goto free_mem_region;
@@ -901,27 +942,15 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		if (rc < 0)
 			goto put_pages;
 
-		/*
-		 * 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.
-		 */
-		phys_contig_mem_regions[i] = ne_mem_region->pages[i];
+		ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
+					  page_size(ne_mem_region->pages[i]));
 
 		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 + phys_regions->num) > 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,9 +960,9 @@ 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]);
+	for (i = 0; i < phys_regions->num; i++) {
+		u64 phys_region_addr = phys_regions->region[i].paddr;
+		u64 phys_region_size = phys_regions->region[i].size;
 
 		if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
 			dev_err_ratelimited(ne_misc_dev.this_device,
@@ -959,13 +988,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 < phys_regions->num; 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->region[i].paddr;
+		slot_add_mem_req.size = phys_regions->region[i].size;
 
 		rc = ne_do_request(pdev, SLOT_ADD_MEM,
 				   &slot_add_mem_req, sizeof(slot_add_mem_req),
@@ -974,7 +1003,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 +1016,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 +1024,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

[PATCH v2 2/4] nitro_enclaves: sanity check the physical region during setting

[Longpeng(Mike)]

Sanity check the physical region before add it to the array, this makes
the code more testable, thus we can test the physical region setup logic
individually.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/virt/nitro_enclaves/ne_misc_dev.c | 62 +++++++++++++++++--------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index a4776fc..d551b88 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -844,10 +844,28 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
 	return 0;
 }
 
-static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
-				      u64 paddr, u64 size)
+static inline int ne_sanity_check_phys_mem_region(u64 paddr, u64 size)
+{
+	if (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");
+		return -EINVAL;
+	}
+
+	if (!IS_ALIGNED(paddr, NE_MIN_MEM_REGION_SIZE)) {
+		dev_err_ratelimited(ne_misc_dev.this_device,
+				    "Physical mem region address is not 2 MiB aligned\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
+				     u64 paddr, u64 size)
 {
 	u64 prev_phys_region_end = 0;
+	int rc = 0;
 
 	if (regions->num) {
 		prev_phys_region_end = regions->region[regions->num - 1].paddr +
@@ -855,14 +873,23 @@ static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
 
 		/* Physical contiguous, just merge */
 		if (prev_phys_region_end == paddr) {
+			rc = ne_sanity_check_phys_mem_region(paddr, size);
+			if (rc < 0)
+				return rc;
+
 			regions->region[regions->num - 1].size += size;
-			return;
+			return 0;
 		}
 	}
 
+	rc = ne_sanity_check_phys_mem_region(paddr, size);
+	if (rc < 0)
+		return rc;
+
 	regions->region[regions->num].paddr = paddr;
 	regions->region[regions->num].size = size;
 	regions->num++;
+	return 0;
 }
 
 /**
@@ -942,8 +969,10 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		if (rc < 0)
 			goto put_pages;
 
-		ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
-					  page_size(ne_mem_region->pages[i]));
+		rc = ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
+					       page_size(ne_mem_region->pages[i]));
+		if (rc < 0)
+			goto put_pages;
 
 		memory_size += page_size(ne_mem_region->pages[i]);
 
@@ -960,29 +989,6 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		goto put_pages;
 	}
 
-	for (i = 0; i < phys_regions->num; i++) {
-		u64 phys_region_addr = phys_regions->region[i].paddr;
-		u64 phys_region_size = phys_regions->region[i].size;
-
-		if (phys_region_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");
-
-			rc = -EINVAL;
-
-			goto put_pages;
-		}
-
-		if (!IS_ALIGNED(phys_region_addr, NE_MIN_MEM_REGION_SIZE)) {
-			dev_err_ratelimited(ne_misc_dev.this_device,
-					    "Physical mem region address is not 2 MiB aligned\n");
-
-			rc = -EINVAL;
-
-			goto put_pages;
-		}
-	}
-
 	ne_mem_region->memory_size = mem_region.memory_size;
 	ne_mem_region->userspace_addr = mem_region.userspace_addr;
 
-- 
1.8.3.1

[PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

[Longpeng(Mike)]

Add test framework for the misc functionality.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
 drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27 +++++++++++++++++++++++++++
 drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c

diff --git a/drivers/virt/nitro_enclaves/Kconfig b/drivers/virt/nitro_enclaves/Kconfig
index 8c9387a..24c54da 100644
--- a/drivers/virt/nitro_enclaves/Kconfig
+++ b/drivers/virt/nitro_enclaves/Kconfig
@@ -18,3 +18,11 @@ config NITRO_ENCLAVES
 
 	  To compile this driver as a module, choose M here.
 	  The module will be called nitro_enclaves.
+
+config NITRO_ENCLAVES_MISC_TEST
+	bool "Tests for the misc functionality of Nitro enclaves"
+	depends on NITRO_ENCLAVES && KUNIT=y
+	help
+	  Enable KUnit tests for the misc functionality of Nitro Enclaves. Select
+	  this option only if you will boot the kernel for the purpose of running
+	  unit tests (e.g. under UML or qemu). If unsure, say N.
diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index d551b88..0131e1b 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -1735,8 +1735,33 @@ static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return 0;
 }
 
+#if defined(CONFIG_NITRO_ENCLAVES_MISC_TEST)
+#include "ne_misc_test.c"
+
+static inline int ne_misc_test_init(void)
+{
+	return __kunit_test_suites_init(ne_misc_test_suites);
+}
+
+static inline void ne_misc_test_exit(void)
+{
+	__kunit_test_suites_exit(ne_misc_test_suites);
+}
+#else
+static inline int ne_misc_test_init(void)
+{
+	return 0;
+}
+
+static inline void ne_misc_test_exit(void)
+{
+}
+#endif
+
 static int __init ne_init(void)
 {
+	ne_misc_test_init();
+
 	mutex_init(&ne_cpu_pool.mutex);
 
 	return pci_register_driver(&ne_pci_driver);
@@ -1747,6 +1772,8 @@ static void __exit ne_exit(void)
 	pci_unregister_driver(&ne_pci_driver);
 
 	ne_teardown_cpu_pool();
+
+	ne_misc_test_exit();
 }
 
 module_init(ne_init);
diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c b/drivers/virt/nitro_enclaves/ne_misc_test.c
new file mode 100644
index 0000000..3426c35
--- /dev/null
+++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <kunit/test.h>
+
+static struct kunit_case ne_misc_test_cases[] = {
+	{}
+};
+
+static struct kunit_suite ne_misc_test_suite = {
+	.name = "ne_misc_test",
+	.test_cases = ne_misc_test_cases,
+};
+
+static struct kunit_suite *ne_misc_test_suites[] = {
+	&ne_misc_test_suite,
+	NULL
+};
-- 
1.8.3.1

[PATCH v2 4/4] nitro_enclaves: add kunit tests for physical contiguous region merging

[Longpeng(Mike)]

Add kunit tests for the physical contiguous memory region merging
functionality.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 drivers/virt/nitro_enclaves/ne_misc_test.c | 46 ++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c b/drivers/virt/nitro_enclaves/ne_misc_test.c
index 3426c35..8532bec 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_test.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
@@ -2,7 +2,53 @@
 
 #include <kunit/test.h>
 
+#define MAX_PHYS_REGIONS	16
+
+struct phys_regions_test {
+	u64 paddr;
+	u64 size;
+	int expect_rc;
+	unsigned long expect_num;
+	u64 expect_last_paddr;
+	u64 expect_last_size;
+} phys_regions_testcases[] = {
+	{0x1000, 0x200000, -EINVAL, 0, ~0, ~0},
+	{0x200000, 0x1000, -EINVAL, 0, ~0, ~0},
+	{0x200000, 0x200000, 0, 1, 0x200000, 0x200000},
+	{0x0, 0x200000, 0, 2, 0x0, 0x200000},
+	{0x600000, 0x400000, 0, 3, 0x600000, 0x400000},
+	{0xa00000, 0x400000, 0, 3, 0x600000, 0x800000},
+	{0x1000, 0x200000, -EINVAL, 3, 0x600000, 0x800000},
+};
+
+static void ne_misc_test_set_phys_region(struct kunit *test)
+{
+	struct phys_contig_mem_region *regions;
+	size_t sz;
+	int i, rc;
+
+	sz = sizeof(*regions) + MAX_PHYS_REGIONS * sizeof(struct phys_mem_region);
+	regions = kunit_kzalloc(test, sz, GFP_KERNEL);
+	KUNIT_ASSERT_TRUE(test, regions != NULL);
+
+	for (i = 0; i < ARRAY_SIZE(phys_regions_testcases); i++) {
+		rc = ne_add_phys_memory_region(regions, phys_regions_testcases[i].paddr,
+					       phys_regions_testcases[i].size);
+		KUNIT_EXPECT_EQ(test, rc, phys_regions_testcases[i].expect_rc);
+		KUNIT_EXPECT_EQ(test, regions->num, phys_regions_testcases[i].expect_num);
+
+		if (phys_regions_testcases[i].expect_last_paddr == ~0ul)
+			continue;
+
+		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].paddr,
+				phys_regions_testcases[i].expect_last_paddr);
+		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].size,
+				phys_regions_testcases[i].expect_last_size);
+	}
+}
+
 static struct kunit_case ne_misc_test_cases[] = {
+	KUNIT_CASE(ne_misc_test_set_phys_region),
 	{}
 };
 
-- 
1.8.3.1

Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

[Greg KH]

On Tue, Sep 21, 2021 at 11:10:36PM +0800, Longpeng(Mike) wrote:
> 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. This
> way the final number of memory regions is less than before merging and
> could potentially avoid reaching maximum.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>

I need a real (i.e. legal) name for a signed-off-by line please.

> ---
>  drivers/virt/nitro_enclaves/ne_misc_dev.c | 87 ++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index e21e1e8..a4776fc 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -126,6 +126,26 @@ struct ne_cpu_pool {
>  static struct ne_cpu_pool ne_cpu_pool;
>  
>  /**
> + * struct phys_mem_region - Physical memory region
> + * @paddr:	The start physical address of the region.
> + * @size:	The sizeof of the region.
> + */
> +struct phys_mem_region {
> +	u64 paddr;
> +	u64 size;
> +};
> +
> +/**
> + * struct phys_contig_mem_region - Physical contiguous memory regions
> + * @num:	The number of regions that currently has.
> + * @region:	The array of physical memory regions.
> + */
> +struct phys_contig_mem_region {
> +	unsigned long num;
> +	struct phys_mem_region region[0];
> +};

Why create your own structures and not use the in-kernel ones for stuff
like this?  What is wrong with the existing memory region or resource
handling logic?

thanks,

greg k-h

Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

[Greg KH]

On Tue, Sep 21, 2021 at 11:10:36PM +0800, Longpeng(Mike) wrote:
> 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. This
> way the final number of memory regions is less than before merging and
> could potentially avoid reaching maximum.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  drivers/virt/nitro_enclaves/ne_misc_dev.c | 87 ++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 29 deletions(-)
> 

What changed from v1?

Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

[Greg KH]

On Tue, Sep 21, 2021 at 11:10:38PM +0800, Longpeng(Mike) wrote:
> Add test framework for the misc functionality.

What is "the misc functionality"?

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
>  drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27 +++++++++++++++++++++++++++
>  drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
>  3 files changed, 52 insertions(+)
>  create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c

What changed from v1?

thanks,

greg k-h

RE: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

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

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, September 21, 2021 11:21 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: andraprs@amazon.com; lexnv@amazon.com; alcioa@amazon.com;
> linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
> stefanha@redhat.com; vkuznets@redhat.com; ne-devel-upstream@amazon.com
> Subject: Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory
> regions
> 
> On Tue, Sep 21, 2021 at 11:10:36PM +0800, Longpeng(Mike) wrote:
> > 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. This
> > way the final number of memory regions is less than before merging and
> > could potentially avoid reaching maximum.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  drivers/virt/nitro_enclaves/ne_misc_dev.c | 87
> ++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 29 deletions(-)
> >
> 
> What changed from v1?

It seems you missed the cover letter ?

RE: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

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

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, September 21, 2021 11:21 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: andraprs@amazon.com; lexnv@amazon.com; alcioa@amazon.com;
> linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
> stefanha@redhat.com; vkuznets@redhat.com; ne-devel-upstream@amazon.com
> Subject: Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc
> functionality
> 
> On Tue, Sep 21, 2021 at 11:10:38PM +0800, Longpeng(Mike) wrote:
> > Add test framework for the misc functionality.
> 
> What is "the misc functionality"?
> 

The functionalities provided in the ne_misc_dev.c

> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
> >  drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27
> +++++++++++++++++++++++++++
> >  drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
> >  3 files changed, 52 insertions(+)
> >  create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c
> 
> What changed from v1?
> 

The unit tests are new added in v2, described in the cover letter.

> thanks,
> 
> greg k-h

RE: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

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

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, September 21, 2021 11:20 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: andraprs@amazon.com; lexnv@amazon.com; alcioa@amazon.com;
> linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
> stefanha@redhat.com; vkuznets@redhat.com; ne-devel-upstream@amazon.com
> Subject: Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory
> regions
> 
> On Tue, Sep 21, 2021 at 11:10:36PM +0800, Longpeng(Mike) wrote:
> > 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. This
> > way the final number of memory regions is less than before merging and
> > could potentially avoid reaching maximum.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> 
> I need a real (i.e. legal) name for a signed-off-by line please.
> 

Okay.

> > ---
> >  drivers/virt/nitro_enclaves/ne_misc_dev.c | 87
> ++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index e21e1e8..a4776fc 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -126,6 +126,26 @@ struct ne_cpu_pool {
> >  static struct ne_cpu_pool ne_cpu_pool;
> >
> >  /**
> > + * struct phys_mem_region - Physical memory region
> > + * @paddr:	The start physical address of the region.
> > + * @size:	The sizeof of the region.
> > + */
> > +struct phys_mem_region {
> > +	u64 paddr;
> > +	u64 size;
> > +};
> > +
> > +/**
> > + * struct phys_contig_mem_region - Physical contiguous memory regions
> > + * @num:	The number of regions that currently has.
> > + * @region:	The array of physical memory regions.
> > + */
> > +struct phys_contig_mem_region {
> > +	unsigned long num;
> > +	struct phys_mem_region region[0];
> > +};
> 
> Why create your own structures and not use the in-kernel ones for stuff
> like this?  What is wrong with the existing memory region or resource
> handling logic?
> 

This patch only optimizes the physical memory regions handling logic of 
the Nitro Enclaves driver, not the common resource management in kernel. 
The physical memory regions are need to send to the hardware later.

Thanks for your suggestion, maybe we can use 'struct range' instead of 
'struct phys_mem_region'.

> thanks,
> 
> greg k-h

Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

[Greg KH]

On Wed, Sep 22, 2021 at 12:33:19AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, September 21, 2021 11:21 PM
> > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> > <longpeng2@huawei.com>
> > Cc: andraprs@amazon.com; lexnv@amazon.com; alcioa@amazon.com;
> > linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> > kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
> > stefanha@redhat.com; vkuznets@redhat.com; ne-devel-upstream@amazon.com
> > Subject: Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc
> > functionality
> > 
> > On Tue, Sep 21, 2021 at 11:10:38PM +0800, Longpeng(Mike) wrote:
> > > Add test framework for the misc functionality.
> > 
> > What is "the misc functionality"?
> > 
> 
> The functionalities provided in the ne_misc_dev.c

You need to be more verbose here in this commit message please, so that
everyone can properly understand what is going on.

> 
> > >
> > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > > ---
> > >  drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
> > >  drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27
> > +++++++++++++++++++++++++++
> > >  drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
> > >  3 files changed, 52 insertions(+)
> > >  create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c
> > 
> > What changed from v1?
> > 
> 
> The unit tests are new added in v2, described in the cover letter.

Ah, the cover letter showed up _after_ these commits did :(

thanks,

greg k-h

Re: [PATCH v2 0/4] merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> 
> Hi guys,
> 
> This patchset try to merge the contiguous physical memory regions when
> set user memory regions, you can see message in PATCH 1 for details.
> Please review when you free, thank!
> 
> Changes v1 -> v2:
>    - update the commit message as Andra's suggestion  [Andra]
>    - remove TODO completely in ne_set_user_memory_region_ioctl  [Andra]
>    - extract the physical memory regions setup into individual
>      function
>    - add kunit tests  [Andra]

Thank you for the patch series. I'll review the patches during this 
week. I had a couple of days off.

Thanks,
Andra

> 
> Longpeng(Mike) (4):
>    nitro_enclaves: merge contiguous physical memory regions
>    nitro_enclaves: sanity check the physical region during setting
>    nitro_enclaves: add test framework for the misc functionality
>    nitro_enclaves: add kunit tests for physical contiguous region merging
> 
>   drivers/virt/nitro_enclaves/Kconfig        |   8 ++
>   drivers/virt/nitro_enclaves/ne_misc_dev.c  | 160 ++++++++++++++++++++---------
>   drivers/virt/nitro_enclaves/ne_misc_test.c |  63 ++++++++++++
>   3 files changed, 182 insertions(+), 49 deletions(-)
>   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c
> 
> --
> 1.8.3.1
> 



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.

Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> 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. This
> way the final number of memory regions is less than before merging and
> could potentially avoid reaching maximum.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---

Please add the changelog for each of the patches in the series, in 
addition to the one in the cover letter.

Also please start the commit titles for all the patches with upper-case 
letter e.g. nitro_enclaves: Merge contiguous physical memory regions

>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 87 ++++++++++++++++++++-----------
>   1 file changed, 58 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index e21e1e8..a4776fc 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -126,6 +126,26 @@ struct ne_cpu_pool {
>   static struct ne_cpu_pool ne_cpu_pool;
>   
>   /**
> + * struct phys_mem_region - Physical memory region
> + * @paddr:	The start physical address of the region.
> + * @size:	The sizeof of the region.
> + */
> +struct phys_mem_region {
> +	u64 paddr;
> +	u64 size;
> +};
> +
> +/**
> + * struct phys_contig_mem_region - Physical contiguous memory regions
> + * @num:	The number of regions that currently has.
> + * @region:	The array of physical memory regions.
> + */
> +struct phys_contig_mem_region {
> +	unsigned long num;
> +	struct phys_mem_region region[0];
> +};

Let's have a single data structure here instead of two, since they are 
not used separately, they come altogether. For example:

struct ne_phys_contig_mem_regions {
         unsigned long   num;
         u64             *paddr;
         u64             *size;
};

And then can allocate memory for "paddr" and "size" using "max_nr_pages" 
from the "ne_set_user_memory_region_ioctl" logic.

> +
> +/**
>    * ne_check_enclaves_created() - Verify if at least one enclave has been created.
>    * @void:	No parameters provided.
>    *
> @@ -824,6 +844,27 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>   	return 0;
>   }
>   
> +static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> +				      u64 paddr, u64 size)

Please add a comment before the function signature, similar to the other 
existing functions, including a brief description of the function, the 
parameters and the return value.

Could be "ne_merge_phys_contig_memory_regions" and specifically mention 
"page_paddr" and "page_size", instead of "paddr" and "size".

> +{
> +	u64 prev_phys_region_end = 0;
> +
> +	if (regions->num) {
> +		prev_phys_region_end = regions->region[regions->num - 1].paddr +
> +					regions->region[regions->num - 1].size;
> +
> +		/* Physical contiguous, just merge */
> +		if (prev_phys_region_end == paddr) {
> +			regions->region[regions->num - 1].size += size;
> +			return;
> +		}
> +	}
> +
> +	regions->region[regions->num].paddr = paddr;
> +	regions->region[regions->num].size = size;
> +	regions->num++;
> +}
> +
>   /**
>    * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot
>    *				       associated with the current enclave.
> @@ -843,9 +884,9 @@ 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;

"phys_contig_mem_regions" instead of "phys_regions", to be more specific.

Thanks,
Andra

> +	size_t size_to_alloc = 0;
>   	int rc = -EINVAL;
>   
>   	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> @@ -866,9 +907,9 @@ 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) {
> +	size_to_alloc = sizeof(*phys_regions) + max_nr_pages * sizeof(struct phys_mem_region);
> +	phys_regions = kzalloc(size_to_alloc, GFP_KERNEL);
> +	if (!phys_regions) {
>   		rc = -ENOMEM;
>   
>   		goto free_mem_region;
> @@ -901,27 +942,15 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>   		if (rc < 0)
>   			goto put_pages;
>   
> -		/*
> -		 * 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.
> -		 */
> -		phys_contig_mem_regions[i] = ne_mem_region->pages[i];
> +		ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
> +					  page_size(ne_mem_region->pages[i]));
>   
>   		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 + phys_regions->num) > 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,9 +960,9 @@ 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]);
> +	for (i = 0; i < phys_regions->num; i++) {
> +		u64 phys_region_addr = phys_regions->region[i].paddr;
> +		u64 phys_region_size = phys_regions->region[i].size;
>   
>   		if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
>   			dev_err_ratelimited(ne_misc_dev.this_device,
> @@ -959,13 +988,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 < phys_regions->num; 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->region[i].paddr;
> +		slot_add_mem_req.size = phys_regions->region[i].size;
>   
>   		rc = ne_do_request(pdev, SLOT_ADD_MEM,
>   				   &slot_add_mem_req, sizeof(slot_add_mem_req),
> @@ -974,7 +1003,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 +1016,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 +1024,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);
>   
> 



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.

Re: [PATCH v2 2/4] nitro_enclaves: sanity check the physical region during setting

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> 
> Sanity check the physical region before add it to the array, this makes
> the code more testable, thus we can test the physical region setup logic
> individually.

Could have the following for the title:

nitro_enclaves: Sanity check physical memory regions during merging

Then the commit message body could be:

Sanity check the physical memory regions during the merge of contiguous 
regions. Thus we can test the physical memory regions setup logic 
individually, including the error cases coming from the sanity checks.

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 62 +++++++++++++++++--------------
>   1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index a4776fc..d551b88 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -844,10 +844,28 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>          return 0;
>   }
> 
> -static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> -                                     u64 paddr, u64 size)
> +static inline int ne_sanity_check_phys_mem_region(u64 paddr, u64 size)

Please add a comment including a brief description for the function, the 
parameters and the return value, as there are available for the already 
existing functions. Also could be more specific and mention 
"phys_mem_region_paddr" and "phys_mem_region_size" for the parameters.

No need for "inline" here, there are a couple of sanity checks, e.g. not 
a very simple, one line logic.

> +{
> +       if (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");
> +               return -EINVAL;
> +       }
> +
> +       if (!IS_ALIGNED(paddr, NE_MIN_MEM_REGION_SIZE)) {
> +               dev_err_ratelimited(ne_misc_dev.this_device,
> +                                   "Physical mem region address is not 2 MiB aligned\n");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> +                                    u64 paddr, u64 size)
>   {
>          u64 prev_phys_region_end = 0;
> +       int rc = 0;
> 
>          if (regions->num) {
>                  prev_phys_region_end = regions->region[regions->num - 1].paddr +
> @@ -855,14 +873,23 @@ static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> 
>                  /* Physical contiguous, just merge */
>                  if (prev_phys_region_end == paddr) {
> +                       rc = ne_sanity_check_phys_mem_region(paddr, size);
> +                       if (rc < 0)
> +                               return rc;
> +
>                          regions->region[regions->num - 1].size += size;
> -                       return;
> +                       return 0;

Can leave a blank line before return.

>                  }
>          }
> 
> +       rc = ne_sanity_check_phys_mem_region(paddr, size);
> +       if (rc < 0)
> +               return rc;


This check can be moved at the beginning of the function, before the 
initial "if" code block. Then can remove the check from the 
"prev_phys_region_end" check block above.

> +
>          regions->region[regions->num].paddr = paddr;
>          regions->region[regions->num].size = size;
>          regions->num++;
> +       return 0;


Can leave a blank line before return.

>   }
> 
>   /**
> @@ -942,8 +969,10 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  if (rc < 0)
>                          goto put_pages;
> 
> -               ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
> -                                         page_size(ne_mem_region->pages[i]));
> +               rc = ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
> +                                              page_size(ne_mem_region->pages[i]));
> +               if (rc < 0)
> +                       goto put_pages;
> 
>                  memory_size += page_size(ne_mem_region->pages[i]);
> 
> @@ -960,29 +989,6 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  goto put_pages;
>          }
> 
> -       for (i = 0; i < phys_regions->num; i++) {
> -               u64 phys_region_addr = phys_regions->region[i].paddr;
> -               u64 phys_region_size = phys_regions->region[i].size;
> -
> -               if (phys_region_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");
> -
> -                       rc = -EINVAL;
> -
> -                       goto put_pages;
> -               }
> -
> -               if (!IS_ALIGNED(phys_region_addr, NE_MIN_MEM_REGION_SIZE)) {
> -                       dev_err_ratelimited(ne_misc_dev.this_device,
> -                                           "Physical mem region address is not 2 MiB aligned\n");
> -
> -                       rc = -EINVAL;
> -
> -                       goto put_pages;
> -               }
> -       }
> -

Should also call the check function here, after the merge of physical 
contiguous memory regions has been completed, for double checking.

Thanks,
Andra

>          ne_mem_region->memory_size = mem_region.memory_size;
>          ne_mem_region->userspace_addr = mem_region.userspace_addr;
> 
> --
> 1.8.3.1
> 



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.

Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> Add test framework for the misc functionality.

Let's add more specifics here.

nitro_enclaves: Add KUnit tests setup for the misc device functionality

Add the initial setup for the KUnit tests that will target the Nitro 
Enclaves misc device functionality.

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>   drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
>   drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27 +++++++++++++++++++++++++++
>   drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
>   3 files changed, 52 insertions(+)
>   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c

Please modify in all places where necessary to mention Nitro Enclaves 
"misc device", instead of just "misc", to be more specific.

For example, here can be "ne_misc_dev_test.c".

> 
> diff --git a/drivers/virt/nitro_enclaves/Kconfig b/drivers/virt/nitro_enclaves/Kconfig
> index 8c9387a..24c54da 100644
> --- a/drivers/virt/nitro_enclaves/Kconfig
> +++ b/drivers/virt/nitro_enclaves/Kconfig
> @@ -18,3 +18,11 @@ config NITRO_ENCLAVES
>   
>   	  To compile this driver as a module, choose M here.
>   	  The module will be called nitro_enclaves.
> +
> +config NITRO_ENCLAVES_MISC_TEST

NITRO_ENCLAVES_MISC_DEV_TEST

> +	bool "Tests for the misc functionality of Nitro enclaves"

misc device functionality of the Nitro Enclaves

> +	depends on NITRO_ENCLAVES && KUNIT=y
> +	help
> +	  Enable KUnit tests for the misc functionality of Nitro Enclaves. Select

misc device functionality of the Nitro Enclaves

> +	  this option only if you will boot the kernel for the purpose of running
> +	  unit tests (e.g. under UML or qemu). If unsure, say N.
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index d551b88..0131e1b 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -1735,8 +1735,33 @@ static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   	return 0;
>   }
>   
> +#if defined(CONFIG_NITRO_ENCLAVES_MISC_TEST)
> +#include "ne_misc_test.c"
> +
> +static inline int ne_misc_test_init(void)
> +{
> +	return __kunit_test_suites_init(ne_misc_test_suites);
> +}
> +
> +static inline void ne_misc_test_exit(void)
> +{
> +	__kunit_test_suites_exit(ne_misc_test_suites);
> +}
> +#else
> +static inline int ne_misc_test_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline void ne_misc_test_exit(void)
> +{
> +}
> +#endif

s/misc/misc_dev/g

Why are these needed? Can't the test suite be setup using 
"kunit_test_suite" as in the KUnit documentation example [1]?

Wouldn't be necessary to conditionally compile the ne_misc_dev_test, 
based on the kernel config above?

[1] 
https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#writing-your-first-test

> +
>   static int __init ne_init(void)
>   {
> +	ne_misc_test_init();
> +
>   	mutex_init(&ne_cpu_pool.mutex);
>   
>   	return pci_register_driver(&ne_pci_driver);
> @@ -1747,6 +1772,8 @@ static void __exit ne_exit(void)
>   	pci_unregister_driver(&ne_pci_driver);
>   
>   	ne_teardown_cpu_pool();
> +
> +	ne_misc_test_exit();
>   }
>   
>   module_init(ne_init);
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c b/drivers/virt/nitro_enclaves/ne_misc_test.c
> new file mode 100644
> index 0000000..3426c35
> --- /dev/null
> +++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <kunit/test.h>
> +
> +static struct kunit_case ne_misc_test_cases[] = {
> +	{}
> +};
> +
> +static struct kunit_suite ne_misc_test_suite = {
> +	.name = "ne_misc_test",
> +	.test_cases = ne_misc_test_cases,
> +};
> +
> +static struct kunit_suite *ne_misc_test_suites[] = {
> +	&ne_misc_test_suite,
> +	NULL
> +};
> 

Can replace "ne_misc" with "ne_misc_dev".

Thanks,
Andra



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.

Re: [PATCH v2 4/4] nitro_enclaves: add kunit tests for physical contiguous region merging

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> Add kunit tests for the physical contiguous memory region merging
> functionality.

nitro_enclaves: Add KUnit tests for contiguous physical memory regions 
merging

Add KUnit tests for the contiguous physical memory regions merging 
functionality from the Nitro Enclaves misc device logic.

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>   drivers/virt/nitro_enclaves/ne_misc_test.c | 46 ++++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c b/drivers/virt/nitro_enclaves/ne_misc_test.c
> index 3426c35..8532bec 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_test.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
> @@ -2,7 +2,53 @@
>   
>   #include <kunit/test.h>
>   
> +#define MAX_PHYS_REGIONS	16
> +
> +struct phys_regions_test {
> +	u64 paddr;
> +	u64 size;
> +	int expect_rc;
> +	unsigned long expect_num;
> +	u64 expect_last_paddr;
> +	u64 expect_last_size;

Please align the fields in the data structure so that's easier to 
visualize them.

u64              paddr;
u64              size;
int              expect_rc;
unsigned long    expect_num;
............

> +} phys_regions_testcases[] = {

phys_regions_test_cases

> +	{0x1000, 0x200000, -EINVAL, 0, ~0, ~0},
> +	{0x200000, 0x1000, -EINVAL, 0, ~0, ~0},
> +	{0x200000, 0x200000, 0, 1, 0x200000, 0x200000},
> +	{0x0, 0x200000, 0, 2, 0x0, 0x200000},
> +	{0x600000, 0x400000, 0, 3, 0x600000, 0x400000},
> +	{0xa00000, 0x400000, 0, 3, 0x600000, 0x800000},
> +	{0x1000, 0x200000, -EINVAL, 3, 0x600000, 0x800000},

Please add a comment before each of the above test cases, including a 
brief description of what each of them intends to test. So that it's 
easier to relate to the numbers.

> +};
> +
> +static void ne_misc_test_set_phys_region(struct kunit *test)

ne_misc_dev_test_merge_phys_contig_memory_regions

> +{
> +	struct phys_contig_mem_region *regions;
> +	size_t sz;
> +	int i, rc;

Please initialize the variables.

Thanks,
Andra

> +
> +	sz = sizeof(*regions) + MAX_PHYS_REGIONS * sizeof(struct phys_mem_region);
> +	regions = kunit_kzalloc(test, sz, GFP_KERNEL);
> +	KUNIT_ASSERT_TRUE(test, regions != NULL);
> +
> +	for (i = 0; i < ARRAY_SIZE(phys_regions_testcases); i++) {
> +		rc = ne_add_phys_memory_region(regions, phys_regions_testcases[i].paddr,
> +					       phys_regions_testcases[i].size);
> +		KUNIT_EXPECT_EQ(test, rc, phys_regions_testcases[i].expect_rc);
> +		KUNIT_EXPECT_EQ(test, regions->num, phys_regions_testcases[i].expect_num);
> +
> +		if (phys_regions_testcases[i].expect_last_paddr == ~0ul)
> +			continue;
> +
> +		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].paddr,
> +				phys_regions_testcases[i].expect_last_paddr);
> +		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].size,
> +				phys_regions_testcases[i].expect_last_size);
> +	}
> +}
> +
>   static struct kunit_case ne_misc_test_cases[] = {
> +	KUNIT_CASE(ne_misc_test_set_phys_region),
>   	{}
>   };
>   
> 



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.

RE: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

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

Hi Andra,

> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
> Sent: Sunday, October 3, 2021 9:01 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> gregkh@linuxfoundation.org; kamal@canonical.com; pbonzini@redhat.com;
> sgarzare@redhat.com; stefanha@redhat.com; vkuznets@redhat.com;
> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
> Subject: Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory
> regions
> 
> 
> 
> On 21/09/2021 18:10, Longpeng(Mike) wrote:
> > 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. This
> > way the final number of memory regions is less than before merging and
> > could potentially avoid reaching maximum.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> 
> Please add the changelog for each of the patches in the series, in
> addition to the one in the cover letter.
> 
> Also please start the commit titles for all the patches with upper-case
> letter e.g. nitro_enclaves: Merge contiguous physical memory regions
> 

OK, thanks.

> >   drivers/virt/nitro_enclaves/ne_misc_dev.c | 87
> ++++++++++++++++++++-----------
> >   1 file changed, 58 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index e21e1e8..a4776fc 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -126,6 +126,26 @@ struct ne_cpu_pool {
> >   static struct ne_cpu_pool ne_cpu_pool;
> >
> >   /**
> > + * struct phys_mem_region - Physical memory region
> > + * @paddr:	The start physical address of the region.
> > + * @size:	The sizeof of the region.
> > + */
> > +struct phys_mem_region {
> > +	u64 paddr;
> > +	u64 size;
> > +};
> > +
> > +/**
> > + * struct phys_contig_mem_region - Physical contiguous memory regions
> > + * @num:	The number of regions that currently has.
> > + * @region:	The array of physical memory regions.
> > + */
> > +struct phys_contig_mem_region {
> > +	unsigned long num;
> > +	struct phys_mem_region region[0];
> > +};
> 
> Let's have a single data structure here instead of two, since they are
> not used separately, they come altogether. For example:
> 
> struct ne_phys_contig_mem_regions {
>          unsigned long   num;
>          u64             *paddr;
>          u64             *size;
> };
> 

I agree that the 'struct phys_mem_region' should be removed, but originally
I want to use 'struct range' instead, because 'paddr' and 'size' should be
used in pairs and we can see there're several cases in kernel that use 'range'
to manage their memory regions. Would this be acceptable to you ?

> And then can allocate memory for "paddr" and "size" using "max_nr_pages"
> from the "ne_set_user_memory_region_ioctl" logic.
> 
> > +
> > +/**
> >    * ne_check_enclaves_created() - Verify if at least one enclave has been
> created.
> >    * @void:	No parameters provided.
> >    *
> > @@ -824,6 +844,27 @@ static int ne_sanity_check_user_mem_region_page(struct
> ne_enclave *ne_enclave,
> >   	return 0;
> >   }
> >
> > +static void ne_add_phys_memory_region(struct phys_contig_mem_region
> *regions,
> > +				      u64 paddr, u64 size)
> 
> Please add a comment before the function signature, similar to the other
> existing functions, including a brief description of the function, the
> parameters and the return value.
> 
> Could be "ne_merge_phys_contig_memory_regions" and specifically mention	
> "page_paddr" and "page_size", instead of "paddr" and "size".
> 

OK, will do.

> > +{
> > +	u64 prev_phys_region_end = 0;
> > +
> > +	if (regions->num) {
> > +		prev_phys_region_end = regions->region[regions->num - 1].paddr +
> > +					regions->region[regions->num - 1].size;
> > +
> > +		/* Physical contiguous, just merge */
> > +		if (prev_phys_region_end == paddr) {
> > +			regions->region[regions->num - 1].size += size;
> > +			return;
> > +		}
> > +	}
> > +
> > +	regions->region[regions->num].paddr = paddr;
> > +	regions->region[regions->num].size = size;
> > +	regions->num++;
> > +}
> > +
> >   /**
> >    * ne_set_user_memory_region_ioctl() - Add user space memory region to the
> slot
> >    *				       associated with the current enclave.
> > @@ -843,9 +884,9 @@ 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;
> 
> "phys_contig_mem_regions" instead of "phys_regions", to be more specific.
> 

Will do, thanks.

> Thanks,
> Andra
> 
> > +	size_t size_to_alloc = 0;
> >   	int rc = -EINVAL;
> >
> >   	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> > @@ -866,9 +907,9 @@ 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) {
> > +	size_to_alloc = sizeof(*phys_regions) + max_nr_pages * sizeof(struct
> phys_mem_region);
> > +	phys_regions = kzalloc(size_to_alloc, GFP_KERNEL);
> > +	if (!phys_regions) {
> >   		rc = -ENOMEM;
> >
> >   		goto free_mem_region;
> > @@ -901,27 +942,15 @@ static int ne_set_user_memory_region_ioctl(struct
> ne_enclave *ne_enclave,
> >   		if (rc < 0)
> >   			goto put_pages;
> >
> > -		/*
> > -		 * 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.
> > -		 */
> > -		phys_contig_mem_regions[i] = ne_mem_region->pages[i];
> > +		ne_add_phys_memory_region(phys_regions,
> page_to_phys(ne_mem_region->pages[i]),
> > +					  page_size(ne_mem_region->pages[i]));
> >
> >   		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 + phys_regions->num) >
> 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,9 +960,9 @@ 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]);
> > +	for (i = 0; i < phys_regions->num; i++) {
> > +		u64 phys_region_addr = phys_regions->region[i].paddr;
> > +		u64 phys_region_size = phys_regions->region[i].size;
> >
> >   		if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> >   			dev_err_ratelimited(ne_misc_dev.this_device,
> > @@ -959,13 +988,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 < phys_regions->num; 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->region[i].paddr;
> > +		slot_add_mem_req.size = phys_regions->region[i].size;
> >
> >   		rc = ne_do_request(pdev, SLOT_ADD_MEM,
> >   				   &slot_add_mem_req, sizeof(slot_add_mem_req),
> > @@ -974,7 +1003,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 +1016,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 +1024,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);
> >
> >
> 
> 
> 
> 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.

Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 05/10/2021 16:54, Longpeng (Mike, Cloud Infrastructure Service 
Product Dept.) wrote:
> 
> Hi Andra,
> 
>> -----Original Message-----
>> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
>> Sent: Sunday, October 3, 2021 9:01 PM
>> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> <longpeng2@huawei.com>
>> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
>> gregkh@linuxfoundation.org; kamal@canonical.com; pbonzini@redhat.com;
>> sgarzare@redhat.com; stefanha@redhat.com; vkuznets@redhat.com;
>> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
>> Subject: Re: [PATCH v2 1/4] nitro_enclaves: merge contiguous physical memory
>> regions
>>
>>
>>
>> On 21/09/2021 18:10, Longpeng(Mike) wrote:
>>> 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. This
>>> way the final number of memory regions is less than before merging and
>>> could potentially avoid reaching maximum.
>>>
>>> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
>>> ---
>>
>> Please add the changelog for each of the patches in the series, in
>> addition to the one in the cover letter.
>>
>> Also please start the commit titles for all the patches with upper-case
>> letter e.g. nitro_enclaves: Merge contiguous physical memory regions
>>
> 
> OK, thanks.
> 
>>>    drivers/virt/nitro_enclaves/ne_misc_dev.c | 87
>> ++++++++++++++++++++-----------
>>>    1 file changed, 58 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> index e21e1e8..a4776fc 100644
>>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> @@ -126,6 +126,26 @@ struct ne_cpu_pool {
>>>    static struct ne_cpu_pool ne_cpu_pool;
>>>
>>>    /**
>>> + * struct phys_mem_region - Physical memory region
>>> + * @paddr: The start physical address of the region.
>>> + * @size:  The sizeof of the region.
>>> + */
>>> +struct phys_mem_region {
>>> +   u64 paddr;
>>> +   u64 size;
>>> +};
>>> +
>>> +/**
>>> + * struct phys_contig_mem_region - Physical contiguous memory regions
>>> + * @num:   The number of regions that currently has.
>>> + * @region:        The array of physical memory regions.
>>> + */
>>> +struct phys_contig_mem_region {
>>> +   unsigned long num;
>>> +   struct phys_mem_region region[0];
>>> +};
>>
>> Let's have a single data structure here instead of two, since they are
>> not used separately, they come altogether. For example:
>>
>> struct ne_phys_contig_mem_regions {
>>           unsigned long   num;
>>           u64             *paddr;
>>           u64             *size;
>> };
>>
> 
> I agree that the 'struct phys_mem_region' should be removed, but originally
> I want to use 'struct range' instead, because 'paddr' and 'size' should be
> used in pairs and we can see there're several cases in kernel that use 'range'
> to manage their memory regions. Would this be acceptable to you ?

If there is about this "struct range" [1], then yes, let's see how that 
would work. Then would be physical addresses ranges and could calculate 
the physical contig memory region size based on the range start and end.

Thanks,
Andra

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/range.h

> 
>> And then can allocate memory for "paddr" and "size" using "max_nr_pages"
>> from the "ne_set_user_memory_region_ioctl" logic.
>>
>>> +
>>> +/**
>>>     * ne_check_enclaves_created() - Verify if at least one enclave has been
>> created.
>>>     * @void: No parameters provided.
>>>     *
>>> @@ -824,6 +844,27 @@ static int ne_sanity_check_user_mem_region_page(struct
>> ne_enclave *ne_enclave,
>>>      return 0;
>>>    }
>>>
>>> +static void ne_add_phys_memory_region(struct phys_contig_mem_region
>> *regions,
>>> +                                 u64 paddr, u64 size)
>>
>> Please add a comment before the function signature, similar to the other
>> existing functions, including a brief description of the function, the
>> parameters and the return value.
>>
>> Could be "ne_merge_phys_contig_memory_regions" and specifically mention
>> "page_paddr" and "page_size", instead of "paddr" and "size".
>>
> 
> OK, will do.
> 
>>> +{
>>> +   u64 prev_phys_region_end = 0;
>>> +
>>> +   if (regions->num) {
>>> +           prev_phys_region_end = regions->region[regions->num - 1].paddr +
>>> +                                   regions->region[regions->num - 1].size;
>>> +
>>> +           /* Physical contiguous, just merge */
>>> +           if (prev_phys_region_end == paddr) {
>>> +                   regions->region[regions->num - 1].size += size;
>>> +                   return;
>>> +           }
>>> +   }
>>> +
>>> +   regions->region[regions->num].paddr = paddr;
>>> +   regions->region[regions->num].size = size;
>>> +   regions->num++;
>>> +}
>>> +
>>>    /**
>>>     * ne_set_user_memory_region_ioctl() - Add user space memory region to the
>> slot
>>>     *                                       associated with the current enclave.
>>> @@ -843,9 +884,9 @@ 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;
>>
>> "phys_contig_mem_regions" instead of "phys_regions", to be more specific.
>>
> 
> Will do, thanks.
> 
>> Thanks,
>> Andra
>>
>>> +   size_t size_to_alloc = 0;
>>>      int rc = -EINVAL;
>>>
>>>      rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
>>> @@ -866,9 +907,9 @@ 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) {
>>> +   size_to_alloc = sizeof(*phys_regions) + max_nr_pages * sizeof(struct
>> phys_mem_region);
>>> +   phys_regions = kzalloc(size_to_alloc, GFP_KERNEL);
>>> +   if (!phys_regions) {
>>>              rc = -ENOMEM;
>>>
>>>              goto free_mem_region;
>>> @@ -901,27 +942,15 @@ static int ne_set_user_memory_region_ioctl(struct
>> ne_enclave *ne_enclave,
>>>              if (rc < 0)
>>>                      goto put_pages;
>>>
>>> -           /*
>>> -            * 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.
>>> -            */
>>> -           phys_contig_mem_regions[i] = ne_mem_region->pages[i];
>>> +           ne_add_phys_memory_region(phys_regions,
>> page_to_phys(ne_mem_region->pages[i]),
>>> +                                     page_size(ne_mem_region->pages[i]));
>>>
>>>              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 + phys_regions->num) >
>> 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,9 +960,9 @@ 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]);
>>> +   for (i = 0; i < phys_regions->num; i++) {
>>> +           u64 phys_region_addr = phys_regions->region[i].paddr;
>>> +           u64 phys_region_size = phys_regions->region[i].size;
>>>
>>>              if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
>>>                      dev_err_ratelimited(ne_misc_dev.this_device,
>>> @@ -959,13 +988,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 < phys_regions->num; 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->region[i].paddr;
>>> +           slot_add_mem_req.size = phys_regions->region[i].size;
>>>
>>>              rc = ne_do_request(pdev, SLOT_ADD_MEM,
>>>                                 &slot_add_mem_req, sizeof(slot_add_mem_req),
>>> @@ -974,7 +1003,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 +1016,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 +1024,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);
>>>
>>>
>>
>>
>>
>> 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.

RE: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

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

> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
> Sent: Sunday, October 3, 2021 9:50 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
> gregkh@linuxfoundation.org; kamal@canonical.com; pbonzini@redhat.com;
> sgarzare@redhat.com; stefanha@redhat.com; vkuznets@redhat.com;
> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
> Subject: Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc
> functionality
> 
> 
> 
> On 21/09/2021 18:10, Longpeng(Mike) wrote:
> > Add test framework for the misc functionality.
> 
> Let's add more specifics here.
> 
> nitro_enclaves: Add KUnit tests setup for the misc device functionality
> 
> Add the initial setup for the KUnit tests that will target the Nitro
> Enclaves misc device functionality.
> 

OK, thanks.

> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >   drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
> >   drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27
> +++++++++++++++++++++++++++
> >   drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
> >   3 files changed, 52 insertions(+)
> >   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c
> 
> Please modify in all places where necessary to mention Nitro Enclaves
> "misc device", instead of just "misc", to be more specific.
> 
> For example, here can be "ne_misc_dev_test.c".
> 

OK.

> >
> > diff --git a/drivers/virt/nitro_enclaves/Kconfig
> b/drivers/virt/nitro_enclaves/Kconfig
> > index 8c9387a..24c54da 100644
> > --- a/drivers/virt/nitro_enclaves/Kconfig
> > +++ b/drivers/virt/nitro_enclaves/Kconfig
> > @@ -18,3 +18,11 @@ config NITRO_ENCLAVES
> >
> >   	  To compile this driver as a module, choose M here.
> >   	  The module will be called nitro_enclaves.
> > +
> > +config NITRO_ENCLAVES_MISC_TEST
> 
> NITRO_ENCLAVES_MISC_DEV_TEST
> 
> > +	bool "Tests for the misc functionality of Nitro enclaves"
> 
> misc device functionality of the Nitro Enclaves
> 
> > +	depends on NITRO_ENCLAVES && KUNIT=y
> > +	help
> > +	  Enable KUnit tests for the misc functionality of Nitro Enclaves. Select
> 
> misc device functionality of the Nitro Enclaves
> 

OK, thanks.

> > +	  this option only if you will boot the kernel for the purpose of running
> > +	  unit tests (e.g. under UML or qemu). If unsure, say N.
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index d551b88..0131e1b 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -1735,8 +1735,33 @@ static long ne_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> >   	return 0;
> >   }
> >
> > +#if defined(CONFIG_NITRO_ENCLAVES_MISC_TEST)
> > +#include "ne_misc_test.c"
> > +
> > +static inline int ne_misc_test_init(void)
> > +{
> > +	return __kunit_test_suites_init(ne_misc_test_suites);
> > +}
> > +
> > +static inline void ne_misc_test_exit(void)
> > +{
> > +	__kunit_test_suites_exit(ne_misc_test_suites);
> > +}
> > +#else
> > +static inline int ne_misc_test_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void ne_misc_test_exit(void)
> > +{
> > +}
> > +#endif
> 
> s/misc/misc_dev/g
> 
> Why are these needed? Can't the test suite be setup using
> "kunit_test_suite" as in the KUnit documentation example [1]?
> 
> Wouldn't be necessary to conditionally compile the ne_misc_dev_test,
> based on the kernel config above?
> 
> [1]
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#writing-y
> our-first-test
> 

Both of these two ways are supported in kernel, for example [2].

There are two reasons why I choose this way:
1. The functions (to test) in "ne_misc_dev.c" are 'static', we cannot invoke
    them in  "ne_misc_dev_test.c".
2. kunit_test_suite defines a module init function internal, and "ne_misc_dev.c"
    also defines one, so they cannot be compiled into one module.


[2]
https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/mmc/host/sdhci-of-aspeed.c#L612

> > +
> >   static int __init ne_init(void)
> >   {
> > +	ne_misc_test_init();
> > +
> >   	mutex_init(&ne_cpu_pool.mutex);
> >
> >   	return pci_register_driver(&ne_pci_driver);
> > @@ -1747,6 +1772,8 @@ static void __exit ne_exit(void)
> >   	pci_unregister_driver(&ne_pci_driver);
> >
> >   	ne_teardown_cpu_pool();
> > +
> > +	ne_misc_test_exit();
> >   }
> >
> >   module_init(ne_init);
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c
> b/drivers/virt/nitro_enclaves/ne_misc_test.c
> > new file mode 100644
> > index 0000000..3426c35
> > --- /dev/null
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <kunit/test.h>
> > +
> > +static struct kunit_case ne_misc_test_cases[] = {
> > +	{}
> > +};
> > +
> > +static struct kunit_suite ne_misc_test_suite = {
> > +	.name = "ne_misc_test",
> > +	.test_cases = ne_misc_test_cases,
> > +};
> > +
> > +static struct kunit_suite *ne_misc_test_suites[] = {
> > +	&ne_misc_test_suite,
> > +	NULL
> > +};
> >
> 
> Can replace "ne_misc" with "ne_misc_dev".
> 
> Thanks,
> Andra
> 
> 
> 
> 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.

Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc functionality

[Paraschiv, Andra-Irina]

On 07/10/2021 05:05, Longpeng (Mike, Cloud Infrastructure Service 
Product Dept.) wrote:
> 
> 
>> -----Original Message-----
>> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
>> Sent: Sunday, October 3, 2021 9:50 PM
>> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> <longpeng2@huawei.com>
>> Cc: linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>;
>> gregkh@linuxfoundation.org; kamal@canonical.com; pbonzini@redhat.com;
>> sgarzare@redhat.com; stefanha@redhat.com; vkuznets@redhat.com;
>> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
>> Subject: Re: [PATCH v2 3/4] nitro_enclaves: add test framework for the misc
>> functionality
>>
>>
>>
>> On 21/09/2021 18:10, Longpeng(Mike) wrote:
>>> Add test framework for the misc functionality.
>>
>> Let's add more specifics here.
>>
>> nitro_enclaves: Add KUnit tests setup for the misc device functionality
>>
>> Add the initial setup for the KUnit tests that will target the Nitro
>> Enclaves misc device functionality.
>>
> 
> OK, thanks.
> 
>>>
>>> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
>>> ---
>>>    drivers/virt/nitro_enclaves/Kconfig        |  8 ++++++++
>>>    drivers/virt/nitro_enclaves/ne_misc_dev.c  | 27
>> +++++++++++++++++++++++++++
>>>    drivers/virt/nitro_enclaves/ne_misc_test.c | 17 +++++++++++++++++
>>>    3 files changed, 52 insertions(+)
>>>    create mode 100644 drivers/virt/nitro_enclaves/ne_misc_test.c
>>
>> Please modify in all places where necessary to mention Nitro Enclaves
>> "misc device", instead of just "misc", to be more specific.
>>
>> For example, here can be "ne_misc_dev_test.c".
>>
> 
> OK.
> 
>>>
>>> diff --git a/drivers/virt/nitro_enclaves/Kconfig
>> b/drivers/virt/nitro_enclaves/Kconfig
>>> index 8c9387a..24c54da 100644
>>> --- a/drivers/virt/nitro_enclaves/Kconfig
>>> +++ b/drivers/virt/nitro_enclaves/Kconfig
>>> @@ -18,3 +18,11 @@ config NITRO_ENCLAVES
>>>
>>>    	  To compile this driver as a module, choose M here.
>>>    	  The module will be called nitro_enclaves.
>>> +
>>> +config NITRO_ENCLAVES_MISC_TEST
>>
>> NITRO_ENCLAVES_MISC_DEV_TEST
>>
>>> +	bool "Tests for the misc functionality of Nitro enclaves"
>>
>> misc device functionality of the Nitro Enclaves
>>
>>> +	depends on NITRO_ENCLAVES && KUNIT=y
>>> +	help
>>> +	  Enable KUnit tests for the misc functionality of Nitro Enclaves. Select
>>
>> misc device functionality of the Nitro Enclaves
>>
> 
> OK, thanks.
> 
>>> +	  this option only if you will boot the kernel for the purpose of running
>>> +	  unit tests (e.g. under UML or qemu). If unsure, say N.
>>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> index d551b88..0131e1b 100644
>>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> @@ -1735,8 +1735,33 @@ static long ne_ioctl(struct file *file, unsigned int
>> cmd, unsigned long arg)
>>>    	return 0;
>>>    }
>>>
>>> +#if defined(CONFIG_NITRO_ENCLAVES_MISC_TEST)
>>> +#include "ne_misc_test.c"
>>> +
>>> +static inline int ne_misc_test_init(void)
>>> +{
>>> +	return __kunit_test_suites_init(ne_misc_test_suites);
>>> +}
>>> +
>>> +static inline void ne_misc_test_exit(void)
>>> +{
>>> +	__kunit_test_suites_exit(ne_misc_test_suites);
>>> +}
>>> +#else
>>> +static inline int ne_misc_test_init(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static inline void ne_misc_test_exit(void)
>>> +{
>>> +}
>>> +#endif
>>
>> s/misc/misc_dev/g
>>
>> Why are these needed? Can't the test suite be setup using
>> "kunit_test_suite" as in the KUnit documentation example [1]?
>>
>> Wouldn't be necessary to conditionally compile the ne_misc_dev_test,
>> based on the kernel config above?
>>
>> [1]
>> https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#writing-y
>> our-first-test
>>
> 
> Both of these two ways are supported in kernel, for example [2].
> 
> There are two reasons why I choose this way:
> 1. The functions (to test) in "ne_misc_dev.c" are 'static', we cannot invoke
>      them in  "ne_misc_dev_test.c".
> 2. kunit_test_suite defines a module init function internal, and "ne_misc_dev.c"
>      also defines one, so they cannot be compiled into one module.


Thanks for the explanation.

Andra

> 
> 
> [2]
> https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/mmc/host/sdhci-of-aspeed.c#L612
> 
>>> +
>>>    static int __init ne_init(void)
>>>    {
>>> +	ne_misc_test_init();
>>> +
>>>    	mutex_init(&ne_cpu_pool.mutex);
>>>
>>>    	return pci_register_driver(&ne_pci_driver);
>>> @@ -1747,6 +1772,8 @@ static void __exit ne_exit(void)
>>>    	pci_unregister_driver(&ne_pci_driver);
>>>
>>>    	ne_teardown_cpu_pool();
>>> +
>>> +	ne_misc_test_exit();
>>>    }
>>>
>>>    module_init(ne_init);
>>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_test.c
>> b/drivers/virt/nitro_enclaves/ne_misc_test.c
>>> new file mode 100644
>>> index 0000000..3426c35
>>> --- /dev/null
>>> +++ b/drivers/virt/nitro_enclaves/ne_misc_test.c
>>> @@ -0,0 +1,17 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +#include <kunit/test.h>
>>> +
>>> +static struct kunit_case ne_misc_test_cases[] = {
>>> +	{}
>>> +};
>>> +
>>> +static struct kunit_suite ne_misc_test_suite = {
>>> +	.name = "ne_misc_test",
>>> +	.test_cases = ne_misc_test_cases,
>>> +};
>>> +
>>> +static struct kunit_suite *ne_misc_test_suites[] = {
>>> +	&ne_misc_test_suite,
>>> +	NULL
>>> +};
>>>
>>
>> Can replace "ne_misc" with "ne_misc_dev".
>>
>> Thanks,
>> Andra
>>
>>
>>
>> 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.