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

[PATCH v4 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 v3 -> v4:
  Patch 1:
    - move "#include <linux/range.h>" according to the alphabetical order. [Andra]
    - rename several variables, parameters, structures and functions.  [Andra]
    - add missing "Context" in the comments.  [Andra]
    - some other changes to makes the code much neater.  [Andra]
  Patch 2:
    - add missing "Context" in the comments.  [Andra]
    - move the comment in ne_merge_phys_contig_memory_regions() before
      the "if (...)". [Andra]
  Patch 3:
    - Nitro enclaves -> Nitro Enclaves   [Andra]
    - check the return code of "ne_misc_dev_test_init()"  [Andra]
    - GPL-2.0-or-later -> GPL-2.0  [Andra]
  Patch 4:
    - "int expect_num" -> "unsigned long  expect_num"  [Andra]
    - rename several variables and structures  [Andra]
    - invoke "kunit_kfree" to free the "regions"  [Andra]

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      | 173 ++++++++++++++++++-------
 drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 156 ++++++++++++++++++++++
 3 files changed, 294 insertions(+), 44 deletions(-)
 create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c

-- 
1.8.3.1

[PATCH v4 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 physically 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 v3 -> v4:
  - move "#include <linux/range.h>" according to the alphabetical order. [Andra]
  - rename several variables, parameters, structures and functions.  [Andra]
  - add missing "Context" in the comments.  [Andra]
  - some other changes to makes the code much neater.  [Andra]

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 | 83 ++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index e21e1e8..b7d2116 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -24,6 +24,7 @@
 #include <linux/nitro_enclaves.h>
 #include <linux/pci.h>
 #include <linux/poll.h>
+#include <linux/range.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <uapi/linux/vm_sockets.h>
@@ -126,6 +127,16 @@ struct ne_cpu_pool {
 static struct ne_cpu_pool ne_cpu_pool;
 
 /**
+ * struct ne_phys_contig_mem_regions - Physical contiguous memory regions
+ * @num:	The number of regions that currently has.
+ * @regions:	The array of physical memory regions.
+ */
+struct ne_phys_contig_mem_regions {
+	unsigned long num;
+	struct range  *regions;
+};
+
+/**
  * 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 physically contiguous.
+ * @phys_contig_regions : Private data associated with the contiguous physical memory regions.
+ * @page_paddr :          Physical start address of the region to be added.
+ * @page_size :           Length of the region to be added.
+ *
+ * Context: Process context. This function is called with the ne_enclave mutex held.
+ */
+static void
+ne_merge_phys_contig_memory_regions(struct ne_phys_contig_mem_regions *phys_contig_regions,
+				    u64 page_paddr, u64 page_size)
+{
+	unsigned long num = phys_contig_regions->num;
+
+	/* Physically contiguous, just merge */
+	if (num && (phys_contig_regions->regions[num - 1].end + 1) == page_paddr) {
+		phys_contig_regions->regions[num - 1].end += page_size;
+
+		return;
+	}
+
+	phys_contig_regions->regions[num].start = page_paddr;
+	phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
+	phys_contig_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,8 @@ 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 ne_phys_contig_mem_regions phys_contig_mem_regions = {};
 	int rc = -EINVAL;
 
 	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
@@ -866,9 +903,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) {
+	phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
+				sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
+	if (!phys_contig_mem_regions.regions) {
 		rc = -ENOMEM;
 
 		goto free_mem_region;
@@ -901,26 +938,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 +958,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_contig_mem_regions.num; i++) {
+		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 +986,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_contig_mem_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_contig_mem_regions.regions[i].start;
+		slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);
 
 		rc = ne_do_request(pdev, SLOT_ADD_MEM,
 				   &slot_add_mem_req, sizeof(slot_add_mem_req),
@@ -974,7 +1001,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_contig_mem_regions.regions);
 
 			/*
 			 * Exit here without put pages as memory regions may
@@ -987,7 +1014,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_contig_mem_regions.regions);
 
 	return 0;
 
@@ -995,7 +1022,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_contig_mem_regions.regions);
 	kfree(ne_mem_region->pages);
 	kfree(ne_mem_region);
 
-- 
1.8.3.1

[PATCH v4 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 v3 -> v4:
  - add missing "Context" in the comments.  [Andra]
  - move the comment in ne_merge_phys_contig_memory_regions() before
    the "if (...)". [Andra]

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 | 77 +++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index b7d2116..3741ae7 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -836,6 +836,37 @@ 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.
+ *
+ * Context: Process context. This function is called with the ne_enclave mutex held.
+ * 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 physically contiguous.
  * @phys_contig_regions : Private data associated with the contiguous physical memory regions.
@@ -843,23 +874,31 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
  * @page_size :           Length of the region to be added.
  *
  * Context: Process context. This function is called with the ne_enclave mutex held.
+ * Return:
+ * * 0 on success.
+ * * Negative return value on failure.
  */
-static void
+static int
 ne_merge_phys_contig_memory_regions(struct ne_phys_contig_mem_regions *phys_contig_regions,
 				    u64 page_paddr, u64 page_size)
 {
 	unsigned long num = phys_contig_regions->num;
+	int rc = 0;
+
+	rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
+	if (rc < 0)
+		return rc;
 
 	/* Physically contiguous, just merge */
 	if (num && (phys_contig_regions->regions[num - 1].end + 1) == page_paddr) {
 		phys_contig_regions->regions[num - 1].end += page_size;
-
-		return;
+	} else {
+		phys_contig_regions->regions[num].start = page_paddr;
+		phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
+		phys_contig_regions->num++;
 	}
 
-	phys_contig_regions->regions[num].start = page_paddr;
-	phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
-	phys_contig_regions->num++;
+	return 0;
 }
 
 /**
@@ -938,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]);
 
@@ -962,23 +1003,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
 		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,
-					    "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 v4 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 v3 -> v4:
  - Nitro enclaves -> Nitro Enclaves   [Andra]
  - check the return code of "ne_misc_dev_test_init()"  [Andra]
  - GPL-2.0-or-later -> GPL-2.0  [Andra]

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      | 31 ++++++++++++++++++++++++++
 drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 17 ++++++++++++++
 3 files changed, 57 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..a7e5020 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 3741ae7..ec46c12 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -1754,8 +1754,37 @@ 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)
 {
+	int rc = 0;
+
+	rc = ne_misc_dev_test_init();
+	if (rc < 0)
+		return rc;
+
 	mutex_init(&ne_cpu_pool.mutex);
 
 	return pci_register_driver(&ne_pci_driver);
@@ -1766,6 +1795,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..6862e99
--- /dev/null
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#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 v4 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 can build the test binary with the following configuration:
  CONFIG_KUNIT=y
  CONFIG_NITRO_ENCLAVES=m
  CONFIG_NITRO_ENCLAVES_MISC_DEV_TEST=y
and install the nitro_enclaves module to run the testcases.

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 v3 -> v4:
  - "int expect_num" -> "unsigned long  expect_num"  [Andra]
  - rename several variables and structures  [Andra]
  - invoke "kunit_kfree" to free the "regions"  [Andra]

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 | 139 +++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
index 6862e99..4648ec02 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
@@ -2,7 +2,146 @@
 
 #include <kunit/test.h>
 
+#define MAX_PHYS_REGIONS	16
+#define INVALID_VALUE		(~0ull)
+
+struct ne_phys_regions_test {
+	u64           paddr;
+	u64           size;
+	int           expect_rc;
+	unsigned long 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 ne_phys_contig_mem_regions is:
+	 *   num = 0
+	 *   regions = {}
+	 */
+	{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 ne_phys_contig_mem_regions is:
+	 *   num = 0
+	 *   regions = {}
+	 */
+	{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 ne_phys_contig_mem_regions is:
+	 *   num = 1
+	 *   regions = {
+	 *       {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 ne_phys_contig_mem_regions is:
+	 *   num = 2
+	 *   regions = {
+	 *       {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 ne_phys_contig_mem_regions is:
+	 *   num = 3
+	 *   regions = {
+	 *       {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 ne_phys_contig_mem_regions is:
+	 *   num = 3
+	 *   regions = {
+	 *       {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 ne_phys_contig_mem_regions is:
+	 *   num = 3
+	 *   regions = {
+	 *       {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 ne_phys_contig_mem_regions phys_contig_mem_regions = {};
+	int rc = 0;
+	int i = 0;
+
+	phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS,
+					sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
+	KUNIT_ASSERT_TRUE(test, phys_contig_mem_regions.regions != NULL);
+
+	for (i = 0; i < ARRAY_SIZE(phys_regions_test_cases); i++) {
+		struct ne_phys_regions_test *test_case = &phys_regions_test_cases[i];
+		unsigned long num = 0;
+
+		rc = ne_merge_phys_contig_memory_regions(&phys_contig_mem_regions,
+							 test_case->paddr, test_case->size);
+		KUNIT_EXPECT_EQ(test, rc, test_case->expect_rc);
+		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.num, test_case->expect_num);
+
+		if (test_case->expect_last_paddr == INVALID_VALUE)
+			continue;
+
+		num = phys_contig_mem_regions.num;
+		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.regions[num - 1].start,
+				test_case->expect_last_paddr);
+		KUNIT_EXPECT_EQ(test, range_len(&phys_contig_mem_regions.regions[num - 1]),
+				test_case->expect_last_size);
+	}
+
+	kunit_kfree(test, phys_contig_mem_regions.regions);
+}
+
 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 v4 1/4] nitro_enclaves: Merge contiguous physical memory regions

[Paraschiv, Andra-Irina]

On 03/11/2021 16:00, 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 physically 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 v3 -> v4:
>    - move "#include <linux/range.h>" according to the alphabetical order. [Andra]
>    - rename several variables, parameters, structures and functions.  [Andra]
>    - add missing "Context" in the comments.  [Andra]
>    - some other changes to makes the code much neater.  [Andra]
> 
> 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 | 83 ++++++++++++++++++++-----------
>   1 file changed, 55 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index e21e1e8..b7d2116 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -24,6 +24,7 @@
>   #include <linux/nitro_enclaves.h>
>   #include <linux/pci.h>
>   #include <linux/poll.h>
> +#include <linux/range.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
>   #include <uapi/linux/vm_sockets.h>
> @@ -126,6 +127,16 @@ struct ne_cpu_pool {
>   static struct ne_cpu_pool ne_cpu_pool;
>   
>   /**
> + * struct ne_phys_contig_mem_regions - Physical contiguous memory regions

“Physical contiguous memory regions” => “Contiguous physical memory 
regions.”

> + * @num:	The number of regions that currently has.
> + * @regions:	The array of physical memory regions.
> + */
> +struct ne_phys_contig_mem_regions {
> +	unsigned long num;
> +	struct range  *regions;
> +};
> +
> +/**
>    * 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 physically contiguous.
> + * @phys_contig_regions : Private data associated with the contiguous physical memory regions.
> + * @page_paddr :          Physical start address of the region to be added.
> + * @page_size :           Length of the region to be added.
> + *
> + * Context: Process context. This function is called with the ne_enclave mutex held.
> + */
> +static void
> +ne_merge_phys_contig_memory_regions(struct ne_phys_contig_mem_regions *phys_contig_regions,
> +				    u64 page_paddr, u64 page_size)
> +{
> +	unsigned long num = phys_contig_regions->num;
> +
> +	/* Physically contiguous, just merge */
> +	if (num && (phys_contig_regions->regions[num - 1].end + 1) == page_paddr) {
> +		phys_contig_regions->regions[num - 1].end += page_size;
> +
> +		return;
> +	}
> +
> +	phys_contig_regions->regions[num].start = page_paddr;
> +	phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
> +	phys_contig_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,8 @@ 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 ne_phys_contig_mem_regions phys_contig_mem_regions = {};
>   	int rc = -EINVAL;
>   
>   	rc = ne_sanity_check_user_mem_region(ne_enclave, mem_region);
> @@ -866,9 +903,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) {
> +	phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
> +				sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);

Please update the alignment as mentioned by "checkpatch":

CHECK: Alignment should match open parenthesis
#907: FILE: 
/home/ubuntu/linux/drivers/virt/nitro_enclaves/ne_misc_dev.c:907:
+	phys_contig_mem_regions.regions = kcalloc(max_nr_pages,
+				sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);

I think that should be similar to the "kcalloc" call that was before 
these changes e.g. "sizeof(*phys_contig_mem_regions.regions)" should be 
under "max_nr_pages" and "GFP_KERNEL" on the next line, the 3rd one.

Other than that, looks good to me.

Thanks,
Andra


> +	if (!phys_contig_mem_regions.regions) {
>   		rc = -ENOMEM;
>   
>   		goto free_mem_region;
> @@ -901,26 +938,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 +958,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_contig_mem_regions.num; i++) {
> +		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 +986,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_contig_mem_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_contig_mem_regions.regions[i].start;
> +		slot_add_mem_req.size = range_len(&phys_contig_mem_regions.regions[i]);
>   
>   		rc = ne_do_request(pdev, SLOT_ADD_MEM,
>   				   &slot_add_mem_req, sizeof(slot_add_mem_req),
> @@ -974,7 +1001,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_contig_mem_regions.regions);
>   
>   			/*
>   			 * Exit here without put pages as memory regions may
> @@ -987,7 +1014,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_contig_mem_regions.regions);
>   
>   	return 0;
>   
> @@ -995,7 +1022,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_contig_mem_regions.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 v4 2/4] nitro_enclaves: Sanity check physical memory regions during merging

[Paraschiv, Andra-Irina]

On 03/11/2021 16:00, 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 v3 -> v4:
>    - add missing "Context" in the comments.  [Andra]
>    - move the comment in ne_merge_phys_contig_memory_regions() before
>      the "if (...)". [Andra]
> 
> 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 | 77 +++++++++++++++++++++----------
>   1 file changed, 52 insertions(+), 25 deletions(-)

Reviewed-by: Andra Paraschiv <andraprs@amazon.com>

You can add in the v5 of the patch series this line after the 
"Signed-off-by" line, to track the already reviewed patches.

Thanks,
Andra

> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index b7d2116..3741ae7 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -836,6 +836,37 @@ 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.
> + *
> + * Context: Process context. This function is called with the ne_enclave mutex held.
> + * 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 physically contiguous.
>    * @phys_contig_regions : Private data associated with the contiguous physical memory regions.
> @@ -843,23 +874,31 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>    * @page_size :           Length of the region to be added.
>    *
>    * Context: Process context. This function is called with the ne_enclave mutex held.
> + * Return:
> + * * 0 on success.
> + * * Negative return value on failure.
>    */
> -static void
> +static int
>   ne_merge_phys_contig_memory_regions(struct ne_phys_contig_mem_regions *phys_contig_regions,
>                                      u64 page_paddr, u64 page_size)
>   {
>          unsigned long num = phys_contig_regions->num;
> +       int rc = 0;
> +
> +       rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
> +       if (rc < 0)
> +               return rc;
> 
>          /* Physically contiguous, just merge */
>          if (num && (phys_contig_regions->regions[num - 1].end + 1) == page_paddr) {
>                  phys_contig_regions->regions[num - 1].end += page_size;
> -
> -               return;
> +       } else {
> +               phys_contig_regions->regions[num].start = page_paddr;
> +               phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
> +               phys_contig_regions->num++;
>          }
> 
> -       phys_contig_regions->regions[num].start = page_paddr;
> -       phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
> -       phys_contig_regions->num++;
> +       return 0;
>   }
> 
>   /**
> @@ -938,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]);
> 
> @@ -962,23 +1003,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  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,
> -                                           "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 v4 3/4] nitro_enclaves: Add KUnit tests setup for the misc device functionality

[Paraschiv, Andra-Irina]

On 03/11/2021 16:00, 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 v3 -> v4:
>    - Nitro enclaves -> Nitro Enclaves   [Andra]
>    - check the return code of "ne_misc_dev_test_init()"  [Andra]
>    - GPL-2.0-or-later -> GPL-2.0  [Andra]
> 
> 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      | 31 ++++++++++++++++++++++++++
>   drivers/virt/nitro_enclaves/ne_misc_dev_test.c | 17 ++++++++++++++
>   3 files changed, 57 insertions(+)
>   create mode 100644 drivers/virt/nitro_enclaves/ne_misc_dev_test.c

Reviewed-by: Andra Paraschiv <andraprs@amazon.com>

Thanks,
Andra

> 
> diff --git a/drivers/virt/nitro_enclaves/Kconfig b/drivers/virt/nitro_enclaves/Kconfig
> index 8c9387a..a7e5020 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 3741ae7..ec46c12 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -1754,8 +1754,37 @@ 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)
>   {
> +       int rc = 0;
> +
> +       rc = ne_misc_dev_test_init();
> +       if (rc < 0)
> +               return rc;
> +
>          mutex_init(&ne_cpu_pool.mutex);
> 
>          return pci_register_driver(&ne_pci_driver);
> @@ -1766,6 +1795,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..6862e99
> --- /dev/null
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#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
> 



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

[Paraschiv, Andra-Irina]

On 03/11/2021 16:00, 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 can build the test binary with the following configuration:
>    CONFIG_KUNIT=y
>    CONFIG_NITRO_ENCLAVES=m
>    CONFIG_NITRO_ENCLAVES_MISC_DEV_TEST=y
> and install the nitro_enclaves module to run the testcases.
> 
> 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 v3 -> v4:
>    - "int expect_num" -> "unsigned long  expect_num"  [Andra]
>    - rename several variables and structures  [Andra]
>    - invoke "kunit_kfree" to free the "regions"  [Andra]
> 
> 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 | 139 +++++++++++++++++++++++++
>   1 file changed, 139 insertions(+)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> index 6862e99..4648ec02 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> @@ -2,7 +2,146 @@
>   
>   #include <kunit/test.h>
>   
> +#define MAX_PHYS_REGIONS	16
> +#define INVALID_VALUE		(~0ull)
> +
> +struct ne_phys_regions_test {
> +	u64           paddr;
> +	u64           size;
> +	int           expect_rc;
> +	unsigned long 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 ne_phys_contig_mem_regions is:
> +	 *   num = 0
> +	 *   regions = {}
> +	 */
> +	{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 ne_phys_contig_mem_regions is:
> +	 *   num = 0
> +	 *   regions = {}
> +	 */
> +	{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 ne_phys_contig_mem_regions is:
> +	 *   num = 1
> +	 *   regions = {
> +	 *       {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 ne_phys_contig_mem_regions is:
> +	 *   num = 2
> +	 *   regions = {
> +	 *       {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 ne_phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   regions = {
> +	 *       {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 ne_phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   regions = {
> +	 *       {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 ne_phys_contig_mem_regions is:
> +	 *   num = 3
> +	 *   regions = {
> +	 *       {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 ne_phys_contig_mem_regions phys_contig_mem_regions = {};
> +	int rc = 0;
> +	int i = 0;
> +
> +	phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS,
> +					sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
> +	KUNIT_ASSERT_TRUE(test, phys_contig_mem_regions.regions != NULL);

Please update the codebase as per these two "checkpatch" messages:

CHECK: Alignment should match open parenthesis
#118: FILE: 
/home/ubuntu/linux/drivers/virt/nitro_enclaves/ne_misc_dev_test.c:118:
+	phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS,
+					sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);


CHECK: Comparison to NULL could be written "phys_contig_mem_regions.regions"
#119: FILE: 
/home/ubuntu/linux/drivers/virt/nitro_enclaves/ne_misc_dev_test.c:119:
+	KUNIT_ASSERT_TRUE(test, phys_contig_mem_regions.regions != NULL);

The first one is similar to the alignment check from the first patch in 
this series. The second one is just "KUNIT_ASSERT_TRUE(test, 
phys_contig_mem_regions.regions);".

Other than that, looks good to me.

Thanks,
Andra

> +
> +	for (i = 0; i < ARRAY_SIZE(phys_regions_test_cases); i++) {
> +		struct ne_phys_regions_test *test_case = &phys_regions_test_cases[i];
> +		unsigned long num = 0;
> +
> +		rc = ne_merge_phys_contig_memory_regions(&phys_contig_mem_regions,
> +							 test_case->paddr, test_case->size);
> +		KUNIT_EXPECT_EQ(test, rc, test_case->expect_rc);
> +		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.num, test_case->expect_num);
> +
> +		if (test_case->expect_last_paddr == INVALID_VALUE)
> +			continue;
> +
> +		num = phys_contig_mem_regions.num;
> +		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.regions[num - 1].start,
> +				test_case->expect_last_paddr);
> +		KUNIT_EXPECT_EQ(test, range_len(&phys_contig_mem_regions.regions[num - 1]),
> +				test_case->expect_last_size);
> +	}
> +
> +	kunit_kfree(test, phys_contig_mem_regions.regions);
> +}
> +
>   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 v4 4/4] nitro_enclaves: Add KUnit tests for contiguous physical memory regions merging

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

> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@amazon.com]
> Sent: Sunday, November 7, 2021 9:20 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 v4 4/4] nitro_enclaves: Add KUnit tests for contiguous
> physical memory regions merging
> 
> 
> 
> On 03/11/2021 16:00, 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 can build the test binary with the following configuration:
> >    CONFIG_KUNIT=y
> >    CONFIG_NITRO_ENCLAVES=m
> >    CONFIG_NITRO_ENCLAVES_MISC_DEV_TEST=y
> > and install the nitro_enclaves module to run the testcases.
> >
> > 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 v3 -> v4:
> >    - "int expect_num" -> "unsigned long  expect_num"  [Andra]
> >    - rename several variables and structures  [Andra]
> >    - invoke "kunit_kfree" to free the "regions"  [Andra]
> >
> > 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 | 139
> +++++++++++++++++++++++++
> >   1 file changed, 139 insertions(+)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> > index 6862e99..4648ec02 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev_test.c
> > @@ -2,7 +2,146 @@
> >
> >   #include <kunit/test.h>
> >
> > +#define MAX_PHYS_REGIONS	16
> > +#define INVALID_VALUE		(~0ull)
> > +
> > +struct ne_phys_regions_test {
> > +	u64           paddr;
> > +	u64           size;
> > +	int           expect_rc;
> > +	unsigned long 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 ne_phys_contig_mem_regions is:
> > +	 *   num = 0
> > +	 *   regions = {}
> > +	 */
> > +	{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 ne_phys_contig_mem_regions is:
> > +	 *   num = 0
> > +	 *   regions = {}
> > +	 */
> > +	{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 ne_phys_contig_mem_regions is:
> > +	 *   num = 1
> > +	 *   regions = {
> > +	 *       {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 ne_phys_contig_mem_regions is:
> > +	 *   num = 2
> > +	 *   regions = {
> > +	 *       {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 ne_phys_contig_mem_regions is:
> > +	 *   num = 3
> > +	 *   regions = {
> > +	 *       {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 ne_phys_contig_mem_regions is:
> > +	 *   num = 3
> > +	 *   regions = {
> > +	 *       {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 ne_phys_contig_mem_regions is:
> > +	 *   num = 3
> > +	 *   regions = {
> > +	 *       {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 ne_phys_contig_mem_regions phys_contig_mem_regions = {};
> > +	int rc = 0;
> > +	int i = 0;
> > +
> > +	phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS,
> > +					sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
> > +	KUNIT_ASSERT_TRUE(test, phys_contig_mem_regions.regions != NULL);
> 
> Please update the codebase as per these two "checkpatch" messages:
> 
> CHECK: Alignment should match open parenthesis
> #118: FILE:
> /home/ubuntu/linux/drivers/virt/nitro_enclaves/ne_misc_dev_test.c:118:
> +	phys_contig_mem_regions.regions = kunit_kcalloc(test, MAX_PHYS_REGIONS,
> +					sizeof(*phys_contig_mem_regions.regions), GFP_KERNEL);
> 
> 
> CHECK: Comparison to NULL could be written "phys_contig_mem_regions.regions"
> #119: FILE:
> /home/ubuntu/linux/drivers/virt/nitro_enclaves/ne_misc_dev_test.c:119:
> +	KUNIT_ASSERT_TRUE(test, phys_contig_mem_regions.regions != NULL);
> 
> The first one is similar to the alignment check from the first patch in
> this series. The second one is just "KUNIT_ASSERT_TRUE(test,
> phys_contig_mem_regions.regions);".
> 

OK, will fix them (including the first patch) in the next version, Thanks.

> Other than that, looks good to me.
> 
> Thanks,
> Andra
> 
> > +
> > +	for (i = 0; i < ARRAY_SIZE(phys_regions_test_cases); i++) {
> > +		struct ne_phys_regions_test *test_case = &phys_regions_test_cases[i];
> > +		unsigned long num = 0;
> > +
> > +		rc = ne_merge_phys_contig_memory_regions(&phys_contig_mem_regions,
> > +							 test_case->paddr, test_case->size);
> > +		KUNIT_EXPECT_EQ(test, rc, test_case->expect_rc);
> > +		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.num,
> test_case->expect_num);
> > +
> > +		if (test_case->expect_last_paddr == INVALID_VALUE)
> > +			continue;
> > +
> > +		num = phys_contig_mem_regions.num;
> > +		KUNIT_EXPECT_EQ(test, phys_contig_mem_regions.regions[num - 1].start,
> > +				test_case->expect_last_paddr);
> > +		KUNIT_EXPECT_EQ(test, range_len(&phys_contig_mem_regions.regions[num
> - 1]),
> > +				test_case->expect_last_size);
> > +	}
> > +
> > +	kunit_kfree(test, phys_contig_mem_regions.regions);
> > +}
> > +
> >   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.