[PATCH v3 0/4] Merge contiguous physical memory regions

[PATCH v3 0/4] Merge contiguous physical memory regions

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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 v2 -> v3:
  Patch 1:
    - update the commit title and commit message.  [Andra]
    - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg KH]
    - add comments before the function definition.  [Andra]
    - rename several variables, parameters and function.  [Andra]
  Patch 2:
    - update the commit title and commit message.  [Andra]
    - add comments before the function definition.  [Andra]
    - remove 'inline' attribute of ne_sanity_check_phys_mem_region. [Andra]
    - leave a blank line before return.  [Andra]
    - move sanity check in ne_merge_phys_contig_memory_regions to
      the beginning of the function.  [Andra]
    - double sanity checking after the merge of physical contiguous
      memory regions has been completed.  [Andra]
  Patch 3:
    - update the commit title and commit message.  [Andra]
    - use "misc_dev"/"misc device"/"MISC_DEV" to be more specific.  [Andra]
  Patch 4:
    - update the commit title and commit message.  [Andra]
    - align the fileds in 'struct phys_regions_test'.  [Andra]
    - rename 'phys_regions_testcases' to 'phys_regions_test_cases'.  [Andra]
    - add comments before each test cases.  [Andra]
    - initialize the variables in ne_misc_dev_test_merge_phys_contig_memory_regions.  [Andra]

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 (4):
  nitro_enclaves: Merge contiguous physical memory regions
  nitro_enclaves: Sanity check physical memory regions during merging
  nitro_enclaves: Add KUnit tests setup for the misc device
    functionality
  nitro_enclaves: Add KUnit tests for contiguous physical memory regions
    merging

 drivers/virt/nitro_enclaves/Kconfig            |   9 ++
 drivers/virt/nitro_enclaves/ne_misc_dev.c      | 163 +++++++++++++++++++------
 drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 153 +++++++++++++++++++++++
 3 files changed, 285 insertions(+), 40 deletions(-)
 create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c

-- 
1.8.3.1

[PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory regions

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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 <longpeng2@huawei.com>
---
Changes v2 -> v3:
  - update the commit title and commit message.  [Andra]
  - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg KH]
  - add comments before the function definition.  [Andra]
  - rename several variables, parameters and function.  [Andra]
---
 drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 +++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index e21e1e8..eea53e9 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -26,6 +26,7 @@
 #include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/range.h>
 #include <uapi/linux/vm_sockets.h>
 
 #include "ne_misc_dev.h"
@@ -126,6 +127,16 @@ struct ne_cpu_pool {
 static struct ne_cpu_pool ne_cpu_pool;
 
 /**
+ * struct phys_contig_mem_regions - Physical contiguous memory regions
+ * @num:	The number of regions that currently has.
+ * @region:	The array of physical memory regions.
+ */
+struct phys_contig_mem_regions {
+	unsigned long num;
+	struct range region[0];
+};
+
+/**
  * ne_check_enclaves_created() - Verify if at least one enclave has been created.
  * @void:	No parameters provided.
  *
@@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
 }
 
 /**
+ * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the adjacent
+ *                                         regions if they are physical contiguous.
+ * @regions   : Private data associated with the physical contiguous memory regions.
+ * @page_paddr: Physical start address of the region to be added.
+ * @page_size : Length of the region to be added.
+ *
+ * Return:
+ * * No return value.
+ */
+static void
+ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions *regions,
+				    u64 page_paddr, u64 page_size)
+{
+	/* Physical contiguous, just merge */
+	if (regions->num &&
+	    (regions->region[regions->num - 1].end + 1) == page_paddr) {
+		regions->region[regions->num - 1].end += page_size;
+
+		return;
+	}
+
+	regions->region[regions->num].start = page_paddr;
+	regions->region[regions->num].end = page_paddr + page_size - 1;
+	regions->num++;
+}
+
+/**
  * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot
  *				       associated with the current enclave.
  * @ne_enclave :	Private data associated with the current enclave.
@@ -843,9 +881,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_regions *phys_contig_mem_regions = NULL;
+	size_t size_to_alloc = 0;
 	int rc = -EINVAL;
 
 	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
@@ -866,8 +904,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);
+	size_to_alloc = sizeof(*phys_contig_mem_regions) +
+			max_nr_pages * sizeof(struct range);
+	phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL);
 	if (!phys_contig_mem_regions) {
 		rc = -ENOMEM;
 
@@ -901,26 +940,16 @@ 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_merge_phys_contig_memory_regions(phys_contig_mem_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) >
+	if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) >
 	    ne_enclave->max_mem_regions) {
 		dev_err_ratelimited(ne_misc_dev.this_device,
 				    "Reached max memory regions %lld\n",
@@ -931,9 +960,10 @@ 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_contig_mem_regions->num; i++) {
+		struct range *range = phys_contig_mem_regions->region + i;
+		u64 phys_region_addr = range->start;
+		u64 phys_region_size = range_len(range);
 
 		if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
 			dev_err_ratelimited(ne_misc_dev.this_device,
@@ -959,13 +989,14 @@ 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_contig_mem_regions->num; i++) {
 		struct ne_pci_dev_cmd_reply cmd_reply = {};
 		struct slot_add_mem_req slot_add_mem_req = {};
+		struct range *range = phys_contig_mem_regions->region + i;
 
 		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 = range->start;
+		slot_add_mem_req.size = range_len(range);
 
 		rc = ne_do_request(pdev, SLOT_ADD_MEM,
 				   &slot_add_mem_req, sizeof(slot_add_mem_req),
-- 
1.8.3.1

[PATCH v3 2/4] nitro_enclaves: Sanity check physical memory regions during merging

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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 <longpeng2@huawei.com>
---
Changes v2 -> v3:
  - update the commit title and commit message.  [Andra]
  - add comments before the function definition.  [Andra]
  - remove 'inline' attribute of ne_sanity_check_phys_mem_region. [Andra]
  - leave a blank line before return.  [Andra]
  - move sanity check in ne_merge_phys_contig_memory_regions to
    the beginning of the function.  [Andra]
  - double sanity checking after the merge of physical contiguous
    memory regions has been completed.  [Andra]
---
 drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 ++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index eea53e9..a8fa56b 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -836,6 +836,36 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
 }
 
 /**
+ * ne_sanity_check_phys_mem_region() - Sanity check the start address and the size
+ *                                     of a physical memory region.
+ * @phys_mem_region_paddr : Physical start address of the region to be sanity checked.
+ * @phys_mem_region_size  : Length of the region to be sanity checked.
+ *
+ * Return:
+ * * 0 on success.
+ * * Negative return value on failure.
+ */
+static int ne_sanity_check_phys_mem_region(u64 phys_mem_region_paddr,
+					   u64 phys_mem_region_size)
+{
+	if (phys_mem_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");
+
+		return -EINVAL;
+	}
+
+	if (!IS_ALIGNED(phys_mem_region_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;
+}
+
+/**
  * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the adjacent
  *                                         regions if they are physical contiguous.
  * @regions   : Private data associated with the physical contiguous memory regions.
@@ -843,23 +873,30 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
  * @page_size : Length of the region to be added.
  *
  * Return:
- * * No return value.
+ * * 0 on success.
+ * * Negative return value on failure.
  */
-static void
+static int
 ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions *regions,
 				    u64 page_paddr, u64 page_size)
 {
-	/* Physical contiguous, just merge */
+	int rc = 0;
+
+	rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
+	if (rc < 0)
+		return rc;
+
 	if (regions->num &&
 	    (regions->region[regions->num - 1].end + 1) == page_paddr) {
+		/* Physical contiguous, just merge */
 		regions->region[regions->num - 1].end += page_size;
-
-		return;
+	} else {
+		regions->region[regions->num].start = page_paddr;
+		regions->region[regions->num].end = page_paddr + page_size - 1;
+		regions->num++;
 	}
 
-	regions->region[regions->num].start = page_paddr;
-	regions->region[regions->num].end = page_paddr + page_size - 1;
-	regions->num++;
+	return 0;
 }
 
 /**
@@ -940,9 +977,11 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		if (rc < 0)
 			goto put_pages;
 
-		ne_merge_phys_contig_memory_regions(phys_contig_mem_regions,
-						    page_to_phys(ne_mem_region->pages[i]),
-						    page_size(ne_mem_region->pages[i]));
+		rc = ne_merge_phys_contig_memory_regions(phys_contig_mem_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]);
 
@@ -965,23 +1004,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		u64 phys_region_addr = range->start;
 		u64 phys_region_size = range_len(range);
 
-		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;
-
+		rc = ne_sanity_check_phys_mem_region(phys_region_addr, phys_region_size);
+		if (rc < 0)
 			goto put_pages;
-		}
 	}
 
 	ne_mem_region->memory_size = mem_region.memory_size;
-- 
1.8.3.1

[PATCH v3 3/4] nitro_enclaves: Add KUnit tests setup for the misc device functionality

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
Changes v2 -> v3:
  - update the commit title and commit message.  [Andra]
  - use "misc_dev"/"misc device"/"MISC_DEV" to be more specific.  [Andra]
---
 drivers/virt/nitro_enclaves/Kconfig            |  9 +++++++++
 drivers/virt/nitro_enclaves/ne_misc_dev.c      | 27 ++++++++++++++++++++++++++
 drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 17 ++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c

diff --git a/drivers/virt/nitro_enclaves/Kconfig b/drivers/virt/nitro_enclaves/Kconfig
index 8c9387a..90802b1 100644
--- a/drivers/virt/nitro_enclaves/Kconfig
+++ b/drivers/virt/nitro_enclaves/Kconfig
@@ -18,3 +18,12 @@ config NITRO_ENCLAVES
 
 	  To compile this driver as a module, choose M here.
 	  The module will be called nitro_enclaves.
+
+config NITRO_ENCLAVES_MISC_DEV_TEST
+	bool "Tests for the misc device functionality of the Nitro enclaves"
+	depends on NITRO_ENCLAVES && KUNIT=y
+	help
+	  Enable KUnit tests for the misc device functionality of the 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 a8fa56b..f895fc3 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -1756,8 +1756,33 @@ static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return 0;
 }
 
+#if defined(CONFIG_NITRO_ENCLAVES_MISC_DEV_TEST)
+#include "ne_misc_dev_test.c"
+
+static inline int ne_misc_dev_test_init(void)
+{
+	return __kunit_test_suites_init(ne_misc_dev_test_suites);
+}
+
+static inline void ne_misc_dev_test_exit(void)
+{
+	__kunit_test_suites_exit(ne_misc_dev_test_suites);
+}
+#else
+static inline int ne_misc_dev_test_init(void)
+{
+	return 0;
+}
+
+static inline void ne_misc_dev_test_exit(void)
+{
+}
+#endif
+
 static int __init ne_init(void)
 {
+	ne_misc_dev_test_init();
+
 	mutex_init(&ne_cpu_pool.mutex);
 
 	return pci_register_driver(&ne_pci_driver);
@@ -1768,6 +1793,8 @@ static void __exit ne_exit(void)
 	pci_unregister_driver(&ne_pci_driver);
 
 	ne_teardown_cpu_pool();
+
+	ne_misc_dev_test_exit();
 }
 
 module_init(ne_init);
diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
new file mode 100644
index 0000000..bcb755e
--- /dev/null
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <kunit/test.h>
+
+static struct kunit_case ne_misc_dev_test_cases[] = {
+	{}
+};
+
+static struct kunit_suite ne_misc_dev_test_suite = {
+	.name = "ne_misc_dev_test",
+	.test_cases = ne_misc_dev_test_cases,
+};
+
+static struct kunit_suite *ne_misc_dev_test_suites[] = {
+	&ne_misc_dev_test_suite,
+	NULL
+};
-- 
1.8.3.1

[PATCH v3 4/4] nitro_enclaves: Add KUnit tests for contiguous physical memory regions merging

[Longpeng(Mike)]

From: Longpeng <longpeng2@huawei.com>

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

We'll see the following message using dmesg if everything goes well:

[...]     # Subtest: ne_misc_dev_test
[...]     1..1
[...] (NULL device *): Physical mem region address is not 2 MiB aligned
[...] (NULL device *): Physical mem region size is not multiple of 2 MiB
[...] (NULL device *): Physical mem region address is not 2 MiB aligned
[...]     ok 1 - ne_misc_dev_test_merge_phys_contig_memory_regions
[...] ok 1 - ne_misc_dev_test

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
Changes v2 -> v3:
  - update the commit title and commit message.  [Andra]
  - align the fileds in 'struct phys_regions_test'.  [Andra]
  - rename 'phys_regions_testcases' to 'phys_regions_test_cases'.  [Andra]
  - add comments before each test cases.  [Andra]
  - initialize the variables in ne_misc_dev_test_merge_phys_contig_memory_regions.  [Andra]
---
 drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 136 +++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
index bcb755e..7bd6b34 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
@@ -2,7 +2,143 @@
 
 #include <kunit/test.h>
 
+#define MAX_PHYS_REGIONS	16
+#define INVALID_VALUE		(~0ull)
+
+struct phys_regions_test {
+	u64 paddr;
+	u64 size;
+	int expect_rc;
+	int expect_num;
+	u64 expect_last_paddr;
+	u64 expect_last_size;
+} phys_regions_test_cases[] = {
+	/*
+	 * Add the region from 0x1000 to (0x1000 + 0x200000 - 1):
+	 *   Expected result:
+	 *       Failed, start address is not 2M-aligned
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 0
+	 *   region = {}
+	 */
+	{0x1000, 0x200000, -EINVAL, 0, INVALID_VALUE, INVALID_VALUE},
+
+	/*
+	 * Add the region from 0x200000 to (0x200000 + 0x1000 - 1):
+	 *   Expected result:
+	 *       Failed, size is not 2M-aligned
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 0
+	 *   region = {}
+	 */
+	{0x200000, 0x1000, -EINVAL, 0, INVALID_VALUE, INVALID_VALUE},
+
+	/*
+	 * Add the region from 0x200000 to (0x200000 + 0x200000 - 1):
+	 *   Expected result:
+	 *       Successful
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 1
+	 *   region = {
+	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
+	 *   }
+	 */
+	{0x200000, 0x200000, 0, 1, 0x200000, 0x200000},
+
+	/*
+	 * Add the region from 0x0 to (0x0 + 0x200000 - 1):
+	 *   Expected result:
+	 *       Successful
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 2
+	 *   region = {
+	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
+	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
+	 *   }
+	 */
+	{0x0, 0x200000, 0, 2, 0x0, 0x200000},
+
+	/*
+	 * Add the region from 0x600000 to (0x600000 + 0x400000 - 1):
+	 *   Expected result:
+	 *       Successful
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 3
+	 *   region = {
+	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
+	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
+	 *       {start=0x600000, end=0x9fffff}, // len=0x400000
+	 *   }
+	 */
+	{0x600000, 0x400000, 0, 3, 0x600000, 0x400000},
+
+	/*
+	 * Add the region from 0xa00000 to (0xa00000 + 0x400000 - 1):
+	 *   Expected result:
+	 *       Successful, merging case!
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 3
+	 *   region = {
+	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
+	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
+	 *       {start=0x600000, end=0xdfffff}, // len=0x800000
+	 *   }
+	 */
+	{0xa00000, 0x400000, 0, 3, 0x600000, 0x800000},
+
+	/*
+	 * Add the region from 0x1000 to (0x1000 + 0x200000 - 1):
+	 *   Expected result:
+	 *       Failed, start address is not 2M-aligned
+	 *
+	 * Now the instance of struct phys_contig_mem_regions is:
+	 *   num = 3
+	 *   region = {
+	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
+	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
+	 *       {start=0x600000, end=0xdfffff}, // len=0x800000
+	 *   }
+	 */
+	{0x1000, 0x200000, -EINVAL, 3, 0x600000, 0x800000},
+};
+
+static void ne_misc_dev_test_merge_phys_contig_memory_regions(struct kunit *test)
+{
+	struct phys_contig_mem_regions *regions;
+	size_t sz = 0;
+	int rc = 0;
+	int i = 0;
+
+	sz = sizeof(*regions) + MAX_PHYS_REGIONS * sizeof(struct range);
+	regions = kunit_kzalloc(test, sz, GFP_KERNEL);
+	KUNIT_ASSERT_TRUE(test, regions != NULL);
+
+	for (i = 0; i < ARRAY_SIZE(phys_regions_test_cases); i++) {
+		struct phys_regions_test *entry = phys_regions_test_cases + i;
+
+		rc = ne_merge_phys_contig_memory_regions(regions,
+							 entry->paddr, entry->size);
+		KUNIT_EXPECT_EQ(test, rc, entry->expect_rc);
+		KUNIT_EXPECT_EQ(test, regions->num, entry->expect_num);
+
+		if (entry->expect_last_paddr == INVALID_VALUE)
+			continue;
+
+		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].start,
+				entry->expect_last_paddr);
+		KUNIT_EXPECT_EQ(test, range_len(&regions->region[regions->num - 1]),
+				entry->expect_last_size);
+	}
+}
+
 static struct kunit_case ne_misc_dev_test_cases[] = {
+	KUNIT_CASE(ne_misc_dev_test_merge_phys_contig_memory_regions),
 	{}
 };
 
-- 
1.8.3.1

Re: [PATCH v3 0/4] Merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> 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 v2 -> v3:
>    Patch 1:
>      - update the commit title and commit message.  [Andra]
>      - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg KH]
>      - add comments before the function definition.  [Andra]
>      - rename several variables, parameters and function.  [Andra]
>    Patch 2:
>      - update the commit title and commit message.  [Andra]
>      - add comments before the function definition.  [Andra]
>      - remove 'inline' attribute of ne_sanity_check_phys_mem_region. [Andra]
>      - leave a blank line before return.  [Andra]
>      - move sanity check in ne_merge_phys_contig_memory_regions to
>        the beginning of the function.  [Andra]
>      - double sanity checking after the merge of physical contiguous
>        memory regions has been completed.  [Andra]
>    Patch 3:
>      - update the commit title and commit message.  [Andra]
>      - use "misc_dev"/"misc device"/"MISC_DEV" to be more specific.  [Andra]
>    Patch 4:
>      - update the commit title and commit message.  [Andra]
>      - align the fileds in 'struct phys_regions_test'.  [Andra]
>      - rename 'phys_regions_testcases' to 'phys_regions_test_cases'.  [Andra]
>      - add comments before each test cases.  [Andra]
>      - initialize the variables in ne_misc_dev_test_merge_phys_contig_memory_regions.  [Andra]
> 
> 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 (4):
>    nitro_enclaves: Merge contiguous physical memory regions
>    nitro_enclaves: Sanity check physical memory regions during merging
>    nitro_enclaves: Add KUnit tests setup for the misc device
>      functionality
>    nitro_enclaves: Add KUnit tests for contiguous physical memory regions
>      merging
> 
>   drivers/virt/nitro_enclaves/Kconfig            |   9 ++
>   drivers/virt/nitro_enclaves/ne_misc_dev.c      | 163 +++++++++++++++++++------
>   drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 153 +++++++++++++++++++++++
>   3 files changed, 285 insertions(+), 40 deletions(-)
>   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> 

Thank you. I'll go through them till the end of this week.

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 v3 1/4] nitro_enclaves: Merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> 
> From: Longpeng <longpeng2@huawei.com>
> 
> 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

physical contiguous => physically contiguous

> way the final number of memory regions is less than before merging and
> could potentially avoid reaching maximum.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> Changes v2 -> v3:
>    - update the commit title and commit message.  [Andra]
>    - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg KH]
>    - add comments before the function definition.  [Andra]
>    - rename several variables, parameters and function.  [Andra]
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 +++++++++++++++++++++----------
>   1 file changed, 55 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index e21e1e8..eea53e9 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -26,6 +26,7 @@
>   #include <linux/poll.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/range.h>

Please keep the alphabetical order and move this before "#include 
<linux/slab.h>."

>   #include <uapi/linux/vm_sockets.h>
> 
>   #include "ne_misc_dev.h"
> @@ -126,6 +127,16 @@ struct ne_cpu_pool {
>   static struct ne_cpu_pool ne_cpu_pool;
> 
>   /**
> + * struct phys_contig_mem_regions - Physical contiguous memory regions

Physical contiguous memory regions => Contiguous physical memory regions

> + * @num:       The number of regions that currently has.
> + * @region:    The array of physical memory regions.
> + */
> +struct phys_contig_mem_regions {
> +       unsigned long num;
> +       struct range region[0];

Should use "struct range *regions" instead, to keep a similar style with 
regard to arrays throughout the code.

Please align the fields name (e.g. [1]) so that it's easier to 
distinguish between fields type and name.

Let's also prefix with "ne" the data structure naming e.g. 
ne_phys_contig_mem_regions. So that is similar to other data structures 
defined in the NE kernel driver codebase.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/virt/nitro_enclaves/ne_misc_dev.c#n118

> +};
> +
> +/**
>    * ne_check_enclaves_created() - Verify if at least one enclave has been created.
>    * @void:      No parameters provided.
>    *
> @@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>   }
> 
>   /**
> + * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the adjacent
> + *                                         regions if they are physical contiguous.

physical contiguous => physically contiguous

> + * @regions   : Private data associated with the physical contiguous memory regions.

physical contiguous memory regions => contiguous physical memory regions

@regions => @phys_contig_regions

> + * @page_paddr: Physical start address of the region to be added.
> + * @page_size : Length of the region to be added.
> + *
> + * Return:
> + * * No return value.

Please add "Context: Process context. This function is called with the 
ne_enclave mutex held." before "Return", similar to other defined functions.

Can remove these lines, as for now there is no return value:

* Return:
* * No return value.

And then can add this in the second patch in the series:

* Context: Process context. This function is called with the ne_enclave 
mutex held. (note: already available in the first patch)
* Return: (note: added in the second patch)
* * 0 ...

> + */
> +static void
> +ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions *regions,
> +                                   u64 page_paddr, u64 page_size)

phys_contig_mem_regions => ne_phys_contig_mem_regions
*regions => *phys_contig_regions

Can define something like this:

unsigned long num = phys_contig_regions->num;

and then use it inside the function, except the last line that 
increments the num.

> +{
> +       /* Physical contiguous, just merge */

Physical contiguous => Physically contiguous

> +       if (regions->num &&
> +           (regions->region[regions->num - 1].end + 1) == page_paddr) {

e.g. phys_contig_regions->regions[num - 1]

> +               regions->region[regions->num - 1].end += page_size;
> +
> +               return;
> +       }
> +
> +       regions->region[regions->num].start = page_paddr;
> +       regions->region[regions->num].end = page_paddr + page_size - 1;
> +       regions->num++;
> +}
> +
> +/**
>    * ne_set_user_memory_region_ioctl() - Add user space memory region to the slot
>    *                                    associated with the current enclave.
>    * @ne_enclave :       Private data associated with the current enclave.
> @@ -843,9 +881,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_regions *phys_contig_mem_regions = NULL;

struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; => 
struct ne_phys_contig_mem_regions phys_contig_mem_regions = {};

> +       size_t size_to_alloc = 0;
>          int rc = -EINVAL;
> 
>          rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> @@ -866,8 +904,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);
> +       size_to_alloc = sizeof(*phys_contig_mem_regions) +
> +                       max_nr_pages * sizeof(struct range);
> +       phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL);

Then can alloc memory for the regions field.

phys_contig_mem_regions.regions = kcalloc(max_nr_pages, 
sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);

The "size_of_alloc" will not be needed in this case.

>          if (!phys_contig_mem_regions) {
>                  rc = -ENOMEM;
> 
> @@ -901,26 +940,16 @@ 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_merge_phys_contig_memory_regions(phys_contig_mem_regions,

And can pass it here like this: &phys_contig_mem_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) >
> +       if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) >
>              ne_enclave->max_mem_regions) {
>                  dev_err_ratelimited(ne_misc_dev.this_device,
>                                      "Reached max memory regions %lld\n",
> @@ -931,9 +960,10 @@ 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_contig_mem_regions->num; i++) {
> +               struct range *range = phys_contig_mem_regions->region + i;
> +               u64 phys_region_addr = range->start;
> +               u64 phys_region_size = range_len(range);

With the updated data structure field, could be:

u64 phys_region_addr = phys_contig_mem_regions.regions[i].start;
u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]);

> 
>                  if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
>                          dev_err_ratelimited(ne_misc_dev.this_device,
> @@ -959,13 +989,14 @@ 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_contig_mem_regions->num; i++) {
>                  struct ne_pci_dev_cmd_reply cmd_reply = {};
>                  struct slot_add_mem_req slot_add_mem_req = {};
> +               struct range *range = phys_contig_mem_regions->region + i;
> 
>                  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 = range->start;
> +               slot_add_mem_req.size = range_len(range);

Similarly here:

slot_add_mem_req.paddr = phys_contig_mem_regions.regions[i].start;
slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);

Then remain the kfree paths for "phys_contig_mem_regions.regions" 
instead of "phys_contig_mem_regions".

Thanks,
Andra

> 
>                  rc = ne_do_request(pdev, SLOT_ADD_MEM,
>                                     &slot_add_mem_req, sizeof(slot_add_mem_req),
> --
> 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 v3 2/4] nitro_enclaves: Sanity check physical memory regions during merging

[Paraschiv, Andra-Irina]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> 
> From: Longpeng <longpeng2@huawei.com>
> 
> 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 <longpeng2@huawei.com>
> ---
> Changes v2 -> v3:
>    - update the commit title and commit message.  [Andra]
>    - add comments before the function definition.  [Andra]
>    - remove 'inline' attribute of ne_sanity_check_phys_mem_region. [Andra]
>    - leave a blank line before return.  [Andra]
>    - move sanity check in ne_merge_phys_contig_memory_regions to
>      the beginning of the function.  [Andra]
>    - double sanity checking after the merge of physical contiguous
>      memory regions has been completed.  [Andra]
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 ++++++++++++++++++++-----------
>   1 file changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index eea53e9..a8fa56b 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -836,6 +836,36 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>   }
> 
>   /**
> + * ne_sanity_check_phys_mem_region() - Sanity check the start address and the size
> + *                                     of a physical memory region.
> + * @phys_mem_region_paddr : Physical start address of the region to be sanity checked.
> + * @phys_mem_region_size  : Length of the region to be sanity checked.
> + *
> + * Return:

Please add this just before "Return", similar to other defined functions 
(e.g. [1]):

* Context: Process context. This function is called with the ne_enclave 
mutex held.
* Return:
......

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/virt/nitro_enclaves/ne_misc_dev.c#n725

> + * * 0 on success.
> + * * Negative return value on failure.
> + */
> +static int ne_sanity_check_phys_mem_region(u64 phys_mem_region_paddr,
> +                                          u64 phys_mem_region_size)
> +{
> +       if (phys_mem_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");
> +
> +               return -EINVAL;
> +       }
> +
> +       if (!IS_ALIGNED(phys_mem_region_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;
> +}
> +
> +/**
>    * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the adjacent
>    *                                         regions if they are physical contiguous.
>    * @regions   : Private data associated with the physical contiguous memory regions.
> @@ -843,23 +873,30 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>    * @page_size : Length of the region to be added.
>    *
>    * Return:
> - * * No return value.
> + * * 0 on success.
> + * * Negative return value on failure.
>    */
> -static void
> +static int
>   ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions *regions,
>                                      u64 page_paddr, u64 page_size)
>   {
> -       /* Physical contiguous, just merge */
> +       int rc = 0;
> +
> +       rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
> +       if (rc < 0)
> +               return rc;
> +
>          if (regions->num &&
>              (regions->region[regions->num - 1].end + 1) == page_paddr) {
> +               /* Physical contiguous, just merge */

Can move this comment before the "if (...)" line.

Thanks,
Andra

>                  regions->region[regions->num - 1].end += page_size;
> -
> -               return;
> +       } else {
> +               regions->region[regions->num].start = page_paddr;
> +               regions->region[regions->num].end = page_paddr + page_size - 1;
> +               regions->num++;
>          }
> 
> -       regions->region[regions->num].start = page_paddr;
> -       regions->region[regions->num].end = page_paddr + page_size - 1;
> -       regions->num++;
> +       return 0;
>   }
> 
>   /**
> @@ -940,9 +977,11 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  if (rc < 0)
>                          goto put_pages;
> 
> -               ne_merge_phys_contig_memory_regions(phys_contig_mem_regions,
> -                                                   page_to_phys(ne_mem_region->pages[i]),
> -                                                   page_size(ne_mem_region->pages[i]));
> +               rc = ne_merge_phys_contig_memory_regions(phys_contig_mem_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]);
> 
> @@ -965,23 +1004,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  u64 phys_region_addr = range->start;
>                  u64 phys_region_size = range_len(range);
> 
> -               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;
> -
> +               rc = ne_sanity_check_phys_mem_region(phys_region_addr, phys_region_size);
> +               if (rc < 0)
>                          goto put_pages;
> -               }
>          }
> 
>          ne_mem_region->memory_size = mem_region.memory_size;
> --
> 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 v3 3/4] nitro_enclaves: Add KUnit tests setup for the misc device functionality

[Paraschiv, Andra-Irina]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> Add the initial setup for the KUnit tests that will target the Nitro
> Enclaves misc device functionality.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> Changes v2 -> v3:
>    - update the commit title and commit message.  [Andra]
>    - use "misc_dev"/"misc device"/"MISC_DEV" to be more specific.  [Andra]
> ---
>   drivers/virt/nitro_enclaves/Kconfig            |  9 +++++++++
>   drivers/virt/nitro_enclaves/ne_misc_dev.c      | 27 ++++++++++++++++++++++++++
>   drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 17 ++++++++++++++++
>   3 files changed, 53 insertions(+)
>   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> 
> diff --git a/drivers/virt/nitro_enclaves/Kconfig b/drivers/virt/nitro_enclaves/Kconfig
> index 8c9387a..90802b1 100644
> --- a/drivers/virt/nitro_enclaves/Kconfig
> +++ b/drivers/virt/nitro_enclaves/Kconfig
> @@ -18,3 +18,12 @@ config NITRO_ENCLAVES
>   
>   	  To compile this driver as a module, choose M here.
>   	  The module will be called nitro_enclaves.
> +
> +config NITRO_ENCLAVES_MISC_DEV_TEST
> +	bool "Tests for the misc device functionality of the Nitro enclaves"

Nitro enclaves => Nitro Enclaves

> +	depends on NITRO_ENCLAVES && KUNIT=y
> +	help
> +	  Enable KUnit tests for the misc device functionality of the 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 a8fa56b..f895fc3 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -1756,8 +1756,33 @@ static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   	return 0;
>   }
>   
> +#if defined(CONFIG_NITRO_ENCLAVES_MISC_DEV_TEST)
> +#include "ne_misc_dev_test.c"
> +
> +static inline int ne_misc_dev_test_init(void)
> +{
> +	return __kunit_test_suites_init(ne_misc_dev_test_suites);
> +}
> +
> +static inline void ne_misc_dev_test_exit(void)
> +{
> +	__kunit_test_suites_exit(ne_misc_dev_test_suites);
> +}
> +#else
> +static inline int ne_misc_dev_test_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline void ne_misc_dev_test_exit(void)
> +{
> +}
> +#endif
> +
>   static int __init ne_init(void)
>   {
> +	ne_misc_dev_test_init();

Let's have:

int rc = 0;

and then check the return code of "ne_misc_dev_test_init()".

> +
>   	mutex_init(&ne_cpu_pool.mutex);
>   
>   	return pci_register_driver(&ne_pci_driver);
> @@ -1768,6 +1793,8 @@ static void __exit ne_exit(void)
>   	pci_unregister_driver(&ne_pci_driver);
>   
>   	ne_teardown_cpu_pool();
> +
> +	ne_misc_dev_test_exit();
>   }
>   
>   module_init(ne_init);
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> new file mode 100644
> index 0000000..bcb755e
> --- /dev/null
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Let's keep the same marker as in the other files of the NE kernel driver 
codebase.

// SPDX-License-Identifier: GPL-2.0

Thanks,
Andra

> +
> +#include <kunit/test.h>
> +
> +static struct kunit_case ne_misc_dev_test_cases[] = {
> +	{}
> +};
> +
> +static struct kunit_suite ne_misc_dev_test_suite = {
> +	.name = "ne_misc_dev_test",
> +	.test_cases = ne_misc_dev_test_cases,
> +};
> +
> +static struct kunit_suite *ne_misc_dev_test_suites[] = {
> +	&ne_misc_dev_test_suite,
> +	NULL
> +};
> 



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 v3 4/4] nitro_enclaves: Add KUnit tests for contiguous physical memory regions merging

[Paraschiv, Andra-Irina]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> Add KUnit tests for the contiguous physical memory regions merging
> functionality from the Nitro Enclaves misc device logic.
> 
> We'll see the following message using dmesg if everything goes well:
> 
> [...]     # Subtest: ne_misc_dev_test
> [...]     1..1
> [...] (NULL device *): Physical mem region address is not 2 MiB aligned
> [...] (NULL device *): Physical mem region size is not multiple of 2 MiB
> [...] (NULL device *): Physical mem region address is not 2 MiB aligned
> [...]     ok 1 - ne_misc_dev_test_merge_phys_contig_memory_regions
> [...] ok 1 - ne_misc_dev_test
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> Changes v2 -> v3:
>    - update the commit title and commit message.  [Andra]
>    - align the fileds in 'struct phys_regions_test'.  [Andra]
>    - rename 'phys_regions_testcases' to 'phys_regions_test_cases'.  [Andra]
>    - add comments before each test cases.  [Andra]
>    - initialize the variables in ne_misc_dev_test_merge_phys_contig_memory_regions.  [Andra]
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 136 +++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> index bcb755e..7bd6b34 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> @@ -2,7 +2,143 @@
>   
>   #include <kunit/test.h>
>   
> +#define MAX_PHYS_REGIONS	16
> +#define INVALID_VALUE		(~0ull)
> +
> +struct phys_regions_test {
> +	u64 paddr;
> +	u64 size;
> +	int expect_rc;
> +	int expect_num;

Please keep the same field type as "num": "unsigned long".

And just align the field names as mentioned in the first patch of the 
series, so they are easily visualized.

> +	u64 expect_last_paddr;
> +	u64 expect_last_size;
> +} phys_regions_test_cases[] = {
> +	/*
> +	 * Add the region from 0x1000 to (0x1000 + 0x200000 - 1):
> +	 *   Expected result:
> +	 *       Failed, start address is not 2M-aligned
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 0
> +	 *   region = {}
> +	 */
> +	{0x1000, 0x200000, -EINVAL, 0, INVALID_VALUE, INVALID_VALUE},
> +
> +	/*
> +	 * Add the region from 0x200000 to (0x200000 + 0x1000 - 1):
> +	 *   Expected result:
> +	 *       Failed, size is not 2M-aligned
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 0
> +	 *   region = {}
> +	 */
> +	{0x200000, 0x1000, -EINVAL, 0, INVALID_VALUE, INVALID_VALUE},
> +
> +	/*
> +	 * Add the region from 0x200000 to (0x200000 + 0x200000 - 1):
> +	 *   Expected result:
> +	 *       Successful
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 1
> +	 *   region = {
> +	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
> +	 *   }
> +	 */
> +	{0x200000, 0x200000, 0, 1, 0x200000, 0x200000},
> +
> +	/*
> +	 * Add the region from 0x0 to (0x0 + 0x200000 - 1):
> +	 *   Expected result:
> +	 *       Successful
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 2
> +	 *   region = {
> +	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
> +	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
> +	 *   }
> +	 */
> +	{0x0, 0x200000, 0, 2, 0x0, 0x200000},
> +
> +	/*
> +	 * Add the region from 0x600000 to (0x600000 + 0x400000 - 1):
> +	 *   Expected result:
> +	 *       Successful
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   region = {
> +	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
> +	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
> +	 *       {start=0x600000, end=0x9fffff}, // len=0x400000
> +	 *   }
> +	 */
> +	{0x600000, 0x400000, 0, 3, 0x600000, 0x400000},
> +
> +	/*
> +	 * Add the region from 0xa00000 to (0xa00000 + 0x400000 - 1):
> +	 *   Expected result:
> +	 *       Successful, merging case!
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   region = {
> +	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
> +	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
> +	 *       {start=0x600000, end=0xdfffff}, // len=0x800000
> +	 *   }
> +	 */
> +	{0xa00000, 0x400000, 0, 3, 0x600000, 0x800000},
> +
> +	/*
> +	 * Add the region from 0x1000 to (0x1000 + 0x200000 - 1):
> +	 *   Expected result:
> +	 *       Failed, start address is not 2M-aligned
> +	 *
> +	 * Now the instance of struct phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   region = {
> +	 *       {start=0x200000, end=0x3fffff}, // len=0x200000
> +	 *       {start=0x0,      end=0x1fffff}, // len=0x200000
> +	 *       {start=0x600000, end=0xdfffff}, // len=0x800000
> +	 *   }
> +	 */
> +	{0x1000, 0x200000, -EINVAL, 3, 0x600000, 0x800000},

Nice, I like how the comments are structured.

Please also update from "region" to "regions" and "struct 
phys_contig_mem_regions" to "ne_phys_contig_mem_regions", after the 
suggested naming changes in the first patch of the series.

> +};
> +
> +static void ne_misc_dev_test_merge_phys_contig_memory_regions(struct kunit *test)
> +{
> +	struct phys_contig_mem_regions *regions;

struct phys_contig_mem_regions *regions; => struct 
ne_phys_contig_mem_regions phys_contig_mem_regions = {};

> +	size_t sz = 0;
> +	int rc = 0;
> +	int i = 0;
> +
> +	sz = sizeof(*regions) + MAX_PHYS_REGIONS * sizeof(struct range);
> +	regions = kunit_kzalloc(test, sz, GFP_KERNEL);

And then can alloc the necessary memory for the "regions" field:

phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS, 
sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);

> +	KUNIT_ASSERT_TRUE(test, regions != NULL);
> +
> +	for (i = 0; i < ARRAY_SIZE(phys_regions_test_cases); i++) {
> +		struct phys_regions_test *entry = phys_regions_test_cases + i;

Could rename "entry" to "test_case".

Can also add:

unsigned long num = 0;

> +
> +		rc = ne_merge_phys_contig_memory_regions(regions,
> +							 entry->paddr, entry->size);

And then can update it here and use it further on:

num = phys_contig_mem_regions.num;

> +		KUNIT_EXPECT_EQ(test, rc, entry->expect_rc);
> +		KUNIT_EXPECT_EQ(test, regions->num, entry->expect_num);
> +
> +		if (entry->expect_last_paddr == INVALID_VALUE)
> +			continue;
> +
> +		KUNIT_EXPECT_EQ(test, regions->region[regions->num - 1].start,
> +				entry->expect_last_paddr);
> +		KUNIT_EXPECT_EQ(test, range_len(&regions->region[regions->num - 1]),
> +				entry->expect_last_size);
> +	}

At the end need to also free the allocated memory for the "regions" 
field using "kunit_free" [1].

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/kunit/test.h#n631

Thanks,
Andra

> +}
> +
>   static struct kunit_case ne_misc_dev_test_cases[] = {
> +	KUNIT_CASE(ne_misc_dev_test_merge_phys_contig_memory_regions),
>   	{}
>   };
>   
> 



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 v3 1/4] nitro_enclaves: Merge contiguous physical memory regions

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

Hi Andra,

Sorry for the long delay.

I've read all your suggestions in each patch, there's no objection. I'll
send v4 later, please review when you free, thanks.

> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
> Sent: Friday, October 15, 2021 9:34 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: Gonglei (Arei) <arei.gonglei@huawei.com>; gregkh@linuxfoundation.org;
> kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
> stefanha@redhat.com; vkuznets@redhat.com; linux-kernel@vger.kernel.org;
> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
> Subject: Re: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory
> regions
> 
> 
> 
> On 09/10/2021 04:32, Longpeng(Mike) wrote:
> >
> > From: Longpeng <longpeng2@huawei.com>
> >
> > 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
> 
> physical contiguous => physically contiguous
> 
> > way the final number of memory regions is less than before merging and
> > could potentially avoid reaching maximum.
> >
> > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > ---
> > Changes v2 -> v3:
> >    - update the commit title and commit message.  [Andra]
> >    - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg
> KH]
> >    - add comments before the function definition.  [Andra]
> >    - rename several variables, parameters and function.  [Andra]
> > ---
> >   drivers/virt/nitro_enclaves/ne_misc_dev.c | 79
> +++++++++++++++++++++----------
> >   1 file changed, 55 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index e21e1e8..eea53e9 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -26,6 +26,7 @@
> >   #include <linux/poll.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > +#include <linux/range.h>
> 
> Please keep the alphabetical order and move this before "#include
> <linux/slab.h>."
> 
> >   #include <uapi/linux/vm_sockets.h>
> >
> >   #include "ne_misc_dev.h"
> > @@ -126,6 +127,16 @@ struct ne_cpu_pool {
> >   static struct ne_cpu_pool ne_cpu_pool;
> >
> >   /**
> > + * struct phys_contig_mem_regions - Physical contiguous memory regions
> 
> Physical contiguous memory regions => Contiguous physical memory regions
> 
> > + * @num:       The number of regions that currently has.
> > + * @region:    The array of physical memory regions.
> > + */
> > +struct phys_contig_mem_regions {
> > +       unsigned long num;
> > +       struct range region[0];
> 
> Should use "struct range *regions" instead, to keep a similar style with
> regard to arrays throughout the code.
> 
> Please align the fields name (e.g. [1]) so that it's easier to
> distinguish between fields type and name.
> 
> Let's also prefix with "ne" the data structure naming e.g.
> ne_phys_contig_mem_regions. So that is similar to other data structures
> defined in the NE kernel driver codebase.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri
> vers/virt/nitro_enclaves/ne_misc_dev.c#n118
> 
> > +};
> > +
> > +/**
> >    * ne_check_enclaves_created() - Verify if at least one enclave has been
> created.
> >    * @void:      No parameters provided.
> >    *
> > @@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct
> ne_enclave *ne_enclave,
> >   }
> >
> >   /**
> > + * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the
> adjacent
> > + *                                         regions if they are physical
> contiguous.
> 
> physical contiguous => physically contiguous
> 
> > + * @regions   : Private data associated with the physical contiguous memory
> regions.
> 
> physical contiguous memory regions => contiguous physical memory regions
> 
> @regions => @phys_contig_regions
> 
> > + * @page_paddr: Physical start address of the region to be added.
> > + * @page_size : Length of the region to be added.
> > + *
> > + * Return:
> > + * * No return value.
> 
> Please add "Context: Process context. This function is called with the
> ne_enclave mutex held." before "Return", similar to other defined functions.
> 
> Can remove these lines, as for now there is no return value:
> 
> * Return:
> * * No return value.
> 
> And then can add this in the second patch in the series:
> 
> * Context: Process context. This function is called with the ne_enclave
> mutex held. (note: already available in the first patch)
> * Return: (note: added in the second patch)
> * * 0 ...
> 
> > + */
> > +static void
> > +ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions
> *regions,
> > +                                   u64 page_paddr, u64 page_size)
> 
> phys_contig_mem_regions => ne_phys_contig_mem_regions
> *regions => *phys_contig_regions
> 
> Can define something like this:
> 
> unsigned long num = phys_contig_regions->num;
> 
> and then use it inside the function, except the last line that
> increments the num.
> 
> > +{
> > +       /* Physical contiguous, just merge */
> 
> Physical contiguous => Physically contiguous
> 
> > +       if (regions->num &&
> > +           (regions->region[regions->num - 1].end + 1) == page_paddr) {
> 
> e.g. phys_contig_regions->regions[num - 1]
> 
> > +               regions->region[regions->num - 1].end += page_size;
> > +
> > +               return;
> > +       }
> > +
> > +       regions->region[regions->num].start = page_paddr;
> > +       regions->region[regions->num].end = page_paddr + page_size - 1;
> > +       regions->num++;
> > +}
> > +
> > +/**
> >    * ne_set_user_memory_region_ioctl() - Add user space memory region to the
> slot
> >    *                                    associated with the current enclave.
> >    * @ne_enclave :       Private data associated with the current enclave.
> > @@ -843,9 +881,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_regions *phys_contig_mem_regions = NULL;
> 
> struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; =>
> struct ne_phys_contig_mem_regions phys_contig_mem_regions = {};
> 
> > +       size_t size_to_alloc = 0;
> >          int rc = -EINVAL;
> >
> >          rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> > @@ -866,8 +904,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);
> > +       size_to_alloc = sizeof(*phys_contig_mem_regions) +
> > +                       max_nr_pages * sizeof(struct range);
> > +       phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL);
> 
> Then can alloc memory for the regions field.
> 
> phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
> sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
> 
> The "size_of_alloc" will not be needed in this case.
> 
> >          if (!phys_contig_mem_regions) {
> >                  rc = -ENOMEM;
> >
> > @@ -901,26 +940,16 @@ 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_merge_phys_contig_memory_regions(phys_contig_mem_regions,
> 
> And can pass it here like this: &phys_contig_mem_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) >
> > +       if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) >
> >              ne_enclave->max_mem_regions) {
> >                  dev_err_ratelimited(ne_misc_dev.this_device,
> >                                      "Reached max memory regions %lld\n",
> > @@ -931,9 +960,10 @@ 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_contig_mem_regions->num; i++) {
> > +               struct range *range = phys_contig_mem_regions->region + i;
> > +               u64 phys_region_addr = range->start;
> > +               u64 phys_region_size = range_len(range);
> 
> With the updated data structure field, could be:
> 
> u64 phys_region_addr = phys_contig_mem_regions.regions[i].start;
> u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]);
> 
> >
> >                  if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> >                          dev_err_ratelimited(ne_misc_dev.this_device,
> > @@ -959,13 +989,14 @@ 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_contig_mem_regions->num; i++) {
> >                  struct ne_pci_dev_cmd_reply cmd_reply = {};
> >                  struct slot_add_mem_req slot_add_mem_req = {};
> > +               struct range *range = phys_contig_mem_regions->region + i;
> >
> >                  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 = range->start;
> > +               slot_add_mem_req.size = range_len(range);
> 
> Similarly here:
> 
> slot_add_mem_req.paddr = phys_contig_mem_regions.regions[i].start;
> slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);
> 
> Then remain the kfree paths for "phys_contig_mem_regions.regions"
> instead of "phys_contig_mem_regions".
> 
> Thanks,
> Andra
> 
> >
> >                  rc = ne_do_request(pdev, SLOT_ADD_MEM,
> >                                     &slot_add_mem_req,
> sizeof(slot_add_mem_req),
> > --
> > 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 v3 1/4] nitro_enclaves: Merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 03/11/2021 15:54, Longpeng (Mike, Cloud Infrastructure Service 
Product Dept.) wrote:
> 
> Hi Andra,
> 
> Sorry for the long delay.
> 
> I've read all your suggestions in each patch, there's no objection. I'll
> send v4 later, please review when you free, thanks.

Thank you, no problem. I'll review the patch series till the end of this 
week.

Andra

> 
>> -----Original Message-----
>> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
>> Sent: Friday, October 15, 2021 9:34 PM
>> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> <longpeng2@huawei.com>
>> Cc: Gonglei (Arei) <arei.gonglei@huawei.com>; gregkh@linuxfoundation.org;
>> kamal@canonical.com; pbonzini@redhat.com; sgarzare@redhat.com;
>> stefanha@redhat.com; vkuznets@redhat.com; linux-kernel@vger.kernel.org;
>> ne-devel-upstream@amazon.com; lexnv@amazon.com; alcioa@amazon.com
>> Subject: Re: [PATCH v3 1/4] nitro_enclaves: Merge contiguous physical memory
>> regions
>>
>>
>>
>> On 09/10/2021 04:32, Longpeng(Mike) wrote:
>>>
>>> From: Longpeng <longpeng2@huawei.com>
>>>
>>> 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
>>
>> physical contiguous => physically contiguous
>>
>>> way the final number of memory regions is less than before merging and
>>> could potentially avoid reaching maximum.
>>>
>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>> ---
>>> Changes v2 -> v3:
>>>     - update the commit title and commit message.  [Andra]
>>>     - use 'struct range' to instead of 'struct phys_mem_region'.  [Andra, Greg
>> KH]
>>>     - add comments before the function definition.  [Andra]
>>>     - rename several variables, parameters and function.  [Andra]
>>> ---
>>>    drivers/virt/nitro_enclaves/ne_misc_dev.c | 79
>> +++++++++++++++++++++----------
>>>    1 file changed, 55 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> index e21e1e8..eea53e9 100644
>>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
>>> @@ -26,6 +26,7 @@
>>>    #include <linux/poll.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/types.h>
>>> +#include <linux/range.h>
>>
>> Please keep the alphabetical order and move this before "#include
>> <linux/slab.h>."
>>
>>>    #include <uapi/linux/vm_sockets.h>
>>>
>>>    #include "ne_misc_dev.h"
>>> @@ -126,6 +127,16 @@ struct ne_cpu_pool {
>>>    static struct ne_cpu_pool ne_cpu_pool;
>>>
>>>    /**
>>> + * struct phys_contig_mem_regions - Physical contiguous memory regions
>>
>> Physical contiguous memory regions => Contiguous physical memory regions
>>
>>> + * @num:       The number of regions that currently has.
>>> + * @region:    The array of physical memory regions.
>>> + */
>>> +struct phys_contig_mem_regions {
>>> +       unsigned long num;
>>> +       struct range region[0];
>>
>> Should use "struct range *regions" instead, to keep a similar style with
>> regard to arrays throughout the code.
>>
>> Please align the fields name (e.g. [1]) so that it's easier to
>> distinguish between fields type and name.
>>
>> Let's also prefix with "ne" the data structure naming e.g.
>> ne_phys_contig_mem_regions. So that is similar to other data structures
>> defined in the NE kernel driver codebase.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri
>> vers/virt/nitro_enclaves/ne_misc_dev.c#n118
>>
>>> +};
>>> +
>>> +/**
>>>     * ne_check_enclaves_created() - Verify if at least one enclave has been
>> created.
>>>     * @void:      No parameters provided.
>>>     *
>>> @@ -825,6 +836,33 @@ static int ne_sanity_check_user_mem_region_page(struct
>> ne_enclave *ne_enclave,
>>>    }
>>>
>>>    /**
>>> + * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the
>> adjacent
>>> + *                                         regions if they are physical
>> contiguous.
>>
>> physical contiguous => physically contiguous
>>
>>> + * @regions   : Private data associated with the physical contiguous memory
>> regions.
>>
>> physical contiguous memory regions => contiguous physical memory regions
>>
>> @regions => @phys_contig_regions
>>
>>> + * @page_paddr: Physical start address of the region to be added.
>>> + * @page_size : Length of the region to be added.
>>> + *
>>> + * Return:
>>> + * * No return value.
>>
>> Please add "Context: Process context. This function is called with the
>> ne_enclave mutex held." before "Return", similar to other defined functions.
>>
>> Can remove these lines, as for now there is no return value:
>>
>> * Return:
>> * * No return value.
>>
>> And then can add this in the second patch in the series:
>>
>> * Context: Process context. This function is called with the ne_enclave
>> mutex held. (note: already available in the first patch)
>> * Return: (note: added in the second patch)
>> * * 0 ...
>>
>>> + */
>>> +static void
>>> +ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions
>> *regions,
>>> +                                   u64 page_paddr, u64 page_size)
>>
>> phys_contig_mem_regions => ne_phys_contig_mem_regions
>> *regions => *phys_contig_regions
>>
>> Can define something like this:
>>
>> unsigned long num = phys_contig_regions->num;
>>
>> and then use it inside the function, except the last line that
>> increments the num.
>>
>>> +{
>>> +       /* Physical contiguous, just merge */
>>
>> Physical contiguous => Physically contiguous
>>
>>> +       if (regions->num &&
>>> +           (regions->region[regions->num - 1].end + 1) == page_paddr) {
>>
>> e.g. phys_contig_regions->regions[num - 1]
>>
>>> +               regions->region[regions->num - 1].end += page_size;
>>> +
>>> +               return;
>>> +       }
>>> +
>>> +       regions->region[regions->num].start = page_paddr;
>>> +       regions->region[regions->num].end = page_paddr + page_size - 1;
>>> +       regions->num++;
>>> +}
>>> +
>>> +/**
>>>     * ne_set_user_memory_region_ioctl() - Add user space memory region to the
>> slot
>>>     *                                    associated with the current enclave.
>>>     * @ne_enclave :       Private data associated with the current enclave.
>>> @@ -843,9 +881,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_regions *phys_contig_mem_regions = NULL;
>>
>> struct phys_contig_mem_regions *phys_contig_mem_regions = NULL; =>
>> struct ne_phys_contig_mem_regions phys_contig_mem_regions = {};
>>
>>> +       size_t size_to_alloc = 0;
>>>           int rc = -EINVAL;
>>>
>>>           rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
>>> @@ -866,8 +904,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);
>>> +       size_to_alloc = sizeof(*phys_contig_mem_regions) +
>>> +                       max_nr_pages * sizeof(struct range);
>>> +       phys_contig_mem_regions = kzalloc(size_to_alloc, GFP_KERNEL);
>>
>> Then can alloc memory for the regions field.
>>
>> phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
>> sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
>>
>> The "size_of_alloc" will not be needed in this case.
>>
>>>           if (!phys_contig_mem_regions) {
>>>                   rc = -ENOMEM;
>>>
>>> @@ -901,26 +940,16 @@ 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_merge_phys_contig_memory_regions(phys_contig_mem_regions,
>>
>> And can pass it here like this: &phys_contig_mem_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) >
>>> +       if ((ne_enclave->nr_mem_regions + phys_contig_mem_regions->num) >
>>>               ne_enclave->max_mem_regions) {
>>>                   dev_err_ratelimited(ne_misc_dev.this_device,
>>>                                       "Reached max memory regions %lld\n",
>>> @@ -931,9 +960,10 @@ 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_contig_mem_regions->num; i++) {
>>> +               struct range *range = phys_contig_mem_regions->region + i;
>>> +               u64 phys_region_addr = range->start;
>>> +               u64 phys_region_size = range_len(range);
>>
>> With the updated data structure field, could be:
>>
>> u64 phys_region_addr = phys_contig_mem_regions.regions[i].start;
>> u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]);
>>
>>>
>>>                   if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
>>>                           dev_err_ratelimited(ne_misc_dev.this_device,
>>> @@ -959,13 +989,14 @@ 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_contig_mem_regions->num; i++) {
>>>                   struct ne_pci_dev_cmd_reply cmd_reply = {};
>>>                   struct slot_add_mem_req slot_add_mem_req = {};
>>> +               struct range *range = phys_contig_mem_regions->region + i;
>>>
>>>                   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 = range->start;
>>> +               slot_add_mem_req.size = range_len(range);
>>
>> Similarly here:
>>
>> slot_add_mem_req.paddr = phys_contig_mem_regions.regions[i].start;
>> slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);
>>
>> Then remain the kfree paths for "phys_contig_mem_regions.regions"
>> instead of "phys_contig_mem_regions".
>>
>> Thanks,
>> Andra
>>
>>>
>>>                   rc = ne_do_request(pdev, SLOT_ADD_MEM,
>>>                                      &slot_add_mem_req,
>> sizeof(slot_add_mem_req),
>>> --
>>> 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.



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.