[PATCH 3/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

[PATCH 3/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

[Jarkko Sakkinen]

From: Kristen Carlson Accardi <kristen@linux.intel.com>

OpenSSL 3.0 deprecates some of the functions used in the SGX
selftests, causing build errors on new distros. For now ignore
the warnings until support for the functions is no longer
available and mark FIXME so that it can be clear this should
be removed at some point.

Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/sigstruct.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index 50c5ab1aa6fa..a07896a46364 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -17,6 +17,12 @@
 #include "defines.h"
 #include "main.h"
 
+/*
+ * FIXME: OpenSSL 3.0 has deprecated some functions. For now just ignore
+ * the warnings.
+ */
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+
 struct q1q2_ctx {
 	BN_CTX *bn_ctx;
 	BIGNUM *m;
-- 
2.37.2

Re: [PATCH 3/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

[Reinette Chatre]

Hi Jarkko,

On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> OpenSSL 3.0 deprecates some of the functions used in the SGX
> selftests, causing build errors on new distros. For now ignore
> the warnings until support for the functions is no longer
> available and mark FIXME so that it can be clear this should
> be removed at some point.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---

This one can be dropped from this series. The fix already made it
into v6.0-rc3 via selftest.

Reinette

Re: [PATCH 3/6] selftests/sgx: Ignore OpenSSL 3.0 deprecated functions warning

[Jarkko Sakkinen]

On Tue, Aug 30, 2022 at 11:18:04AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > From: Kristen Carlson Accardi <kristen@linux.intel.com>
> > 
> > OpenSSL 3.0 deprecates some of the functions used in the SGX
> > selftests, causing build errors on new distros. For now ignore
> > the warnings until support for the functions is no longer
> > available and mark FIXME so that it can be clear this should
> > be removed at some point.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> 
> This one can be dropped from this series. The fix already made it
> into v6.0-rc3 via selftest.

ok

BR, Jarkko

[PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

From: Vijay Dhanraj <vijay.dhanraj@intel.com>

Add a new test case which is same as augment_via_eaccept but adds a
larger number of EPC pages to stress test EAUG via EACCEPT.

Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Addressed Reinette's feedback:
  https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
---
 tools/testing/selftests/sgx/load.c      |   5 +-
 tools/testing/selftests/sgx/main.c      | 141 +++++++++++++++++++++---
 tools/testing/selftests/sgx/main.h      |   3 +-
 tools/testing/selftests/sgx/sigstruct.c |   2 +-
 4 files changed, 129 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 94bdeac1cf04..7de1b15c90b1 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 	return 0;
 }
 
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+			   unsigned long edmm_size)
 {
 	const char device_path[] = "/dev/sgx_enclave";
 	struct encl_segment *seg;
@@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
 
 	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
 
-	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
+	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
 		encl->encl_size <<= 1;
 
 	return true;
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 9820b3809c69..867e98502120 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -21,8 +21,15 @@
 #include "../kselftest_harness.h"
 #include "main.h"
 
+/*
+ * The size was chosen based on a bug report:
+ * https://lore.kernel.org/linux-sgx/DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com/
+ */
+static const unsigned long EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L; // 8 GB
+static const unsigned long TIMEOUT_LONG = 900; /* seconds */
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 static const uint64_t MAGIC2 = 0x8877665544332211ULL;
+
 vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
 
 /*
@@ -173,7 +180,8 @@ FIXTURE(enclave) {
 };
 
 static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
-			    struct __test_metadata *_metadata)
+			    struct __test_metadata *_metadata,
+			    unsigned long edmm_size)
 {
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
@@ -183,7 +191,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	unsigned int i;
 	void *addr;
 
-	if (!encl_load("test_encl.elf", encl, heap_size)) {
+	if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) {
 		encl_delete(encl);
 		TH_LOG("Failed to load the test enclave.");
 		return false;
@@ -284,7 +292,7 @@ TEST_F(enclave, unclobbered_vdso)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -357,7 +365,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
 
 	total_mem = get_total_epc_mem();
 	ASSERT_NE(total_mem, 0);
-	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -401,7 +409,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	ASSERT_NE(total_mem, 0);
 	TH_LOG("Creating an enclave with %lu bytes heap may take a while ...",
 	       total_mem);
-	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
 
 	/*
 	 * Hardware (SGX2) and kernel support is needed for this test. Start
@@ -506,7 +514,7 @@ TEST_F(enclave, clobbered_vdso)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -542,7 +550,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function)
 	struct encl_op_get_from_buf get_op;
 	struct encl_op_put_to_buf put_op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -575,7 +583,7 @@ TEST_F(enclave, tcs_entry)
 {
 	struct encl_op_header op;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -620,7 +628,7 @@ TEST_F(enclave, pte_permissions)
 	unsigned long data_start;
 	int ret;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -722,7 +730,7 @@ TEST_F(enclave, tcs_permissions)
 	struct sgx_enclave_restrict_permissions ioc;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -785,7 +793,7 @@ TEST_F(enclave, epcm_permissions)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -986,7 +994,7 @@ TEST_F(enclave, augment)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1116,7 +1124,7 @@ TEST_F(enclave, augment_via_eaccept)
 	if (!sgx2_supported())
 		SKIP(return, "SGX2 not supported");
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1210,6 +1218,103 @@ TEST_F(enclave, augment_via_eaccept)
 	munmap(addr, PAGE_SIZE);
 }
 
+/*
+ * Test for the addition of large number of pages to an initialized enclave via
+ * a pre-emptive run of EACCEPT on every page to be added.
+ */
+TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG)
+{
+	struct encl_op_get_from_addr get_addr_op;
+	struct encl_op_put_to_addr put_addr_op;
+	struct encl_op_eaccept eaccept_op;
+	size_t total_size = 0;
+	unsigned long i;
+	void *addr;
+
+	if (!sgx2_supported())
+		SKIP(return, "SGX2 not supported");
+
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata,
+				    EDMM_SIZE_LONG));
+
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
+
+	for (i = 0; i < self->encl.nr_segments; i++) {
+		struct encl_segment *seg = &self->encl.segment_tbl[i];
+
+		total_size += seg->size;
+	}
+
+	/*
+	 * mmap() every page at end of existing enclave to be used for
+	 * EDMM.
+	 */
+	addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG,
+			PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
+			self->encl.fd, 0);
+	EXPECT_NE(addr, MAP_FAILED);
+
+	self->run.exception_vector = 0;
+	self->run.exception_error_code = 0;
+	self->run.exception_addr = 0;
+
+	/*
+	 * Run EACCEPT on new page to trigger the #PF->EAUG->EACCEPT(again
+	 * without a #PF). All should be transparent to userspace.
+	 */
+	eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
+	eaccept_op.ret = 0;
+	eaccept_op.header.type = ENCL_OP_EACCEPT;
+
+	for (i = 0; i < EDMM_SIZE_LONG; i += 4096) {
+		eaccept_op.epc_addr = (uint64_t)(addr + i);
+
+		EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
+		if (self->run.exception_vector == 14 &&
+			self->run.exception_error_code == 4 &&
+			self->run.exception_addr == self->encl.encl_base) {
+			munmap(addr, EDMM_SIZE_LONG);
+			SKIP(return, "Kernel does not support adding pages to initialized enclave");
+		}
+
+		EXPECT_EQ(self->run.exception_vector, 0);
+		EXPECT_EQ(self->run.exception_error_code, 0);
+		EXPECT_EQ(self->run.exception_addr, 0);
+		ASSERT_EQ(eaccept_op.ret, 0);
+		ASSERT_EQ(self->run.function, EEXIT);
+	}
+
+	put_addr_op.value = MAGIC;
+	put_addr_op.addr = (unsigned long)addr;
+	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
+
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	/*
+	 * Read memory from newly added page that was just written to,
+	 * confirming that data previously written (MAGIC) is present.
+	 */
+	get_addr_op.value = 0;
+	get_addr_op.addr = (unsigned long)addr;
+	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
+
+	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
+
+	EXPECT_EQ(get_addr_op.value, MAGIC);
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(self->run.exception_vector, 0);
+	EXPECT_EQ(self->run.exception_error_code, 0);
+	EXPECT_EQ(self->run.exception_addr, 0);
+
+	munmap(addr, EDMM_SIZE_LONG);
+}
+
 /*
  * SGX2 page type modification test in two phases:
  * Phase 1:
@@ -1238,7 +1343,7 @@ TEST_F(enclave, tcs_create)
 	int ret, i;
 
 	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl,
-				    _metadata));
+				    _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1568,7 +1673,7 @@ TEST_F(enclave, remove_added_page_no_eaccept)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1679,7 +1784,7 @@ TEST_F(enclave, remove_added_page_invalid_access)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1794,7 +1899,7 @@ TEST_F(enclave, remove_added_page_invalid_access_after_eaccept)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
@@ -1918,7 +2023,7 @@ TEST_F(enclave, remove_untouched_page)
 	unsigned long data_start;
 	int ret, errno_save;
 
-	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
 
 	/*
 	 * Hardware (SGX2) and kernel support is needed for this test. Start
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index fc585be97e2f..fe5d39ac0e1e 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -35,7 +35,8 @@ extern unsigned char sign_key[];
 extern unsigned char sign_key_end[];
 
 void encl_delete(struct encl *ctx);
-bool encl_load(const char *path, struct encl *encl, unsigned long heap_size);
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
+			   unsigned long edmm_size);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 uint64_t encl_get_entry(struct encl *encl, const char *symbol);
diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index a07896a46364..decd767434d5 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -349,7 +349,7 @@ bool encl_measure(struct encl *encl)
 	if (!ctx)
 		goto err;
 
-	if (!mrenclave_ecreate(ctx, encl->src_size))
+	if (!mrenclave_ecreate(ctx, encl->encl_size))
 		goto err;
 
 	for (i = 0; i < encl->nr_segments; i++) {
-- 
2.37.2

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Reinette Chatre]

Hi Vijay and Jarkko,

On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> 
> Add a new test case which is same as augment_via_eaccept but adds a
> larger number of EPC pages to stress test EAUG via EACCEPT.
> 
> Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v2:
> - Addressed Reinette's feedback:
>   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> ---
>  tools/testing/selftests/sgx/load.c      |   5 +-
>  tools/testing/selftests/sgx/main.c      | 141 +++++++++++++++++++++---
>  tools/testing/selftests/sgx/main.h      |   3 +-
>  tools/testing/selftests/sgx/sigstruct.c |   2 +-
>  4 files changed, 129 insertions(+), 22 deletions(-)

There seems to be at least three patches merged into one here:
1) Update SGX selftests to create enclaves with provided size dedicated
   to EDMM (this change causes a lot of noise and distracts from the test
   addition).
2) The mrenclave_ecreate() fix (which is still incomplete).
3) The actual test addition.


> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 94bdeac1cf04..7de1b15c90b1 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>  	return 0;
>  }
>  
> -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> +			   unsigned long edmm_size)
>  {

checkpatch.pl informs about alignment issues above and also a few other places.

>  	const char device_path[] = "/dev/sgx_enclave";
>  	struct encl_segment *seg;
> @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
>  
>  	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
>  
> -	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
> +	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
>  		encl->encl_size <<= 1;
>  
>  	return true;
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 9820b3809c69..867e98502120 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -21,8 +21,15 @@
>  #include "../kselftest_harness.h"
>  #include "main.h"
>  
> +/*
> + * The size was chosen based on a bug report:
> + * https://lore.kernel.org/linux-sgx/DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com/
> + */
> +static const unsigned long EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L; // 8 GB
> +static const unsigned long TIMEOUT_LONG = 900; /* seconds */

It is interesting that in this small snippet there are three different
comment styles (multi-line comment preceding code, tail comment using /**/,
and tail comment using //) :)

>  static const uint64_t MAGIC = 0x1122334455667788ULL;
>  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> +

Addition of empty line here.

>  vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
>  
>  /*
> @@ -173,7 +180,8 @@ FIXTURE(enclave) {
>  };
>  
>  static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
> -			    struct __test_metadata *_metadata)
> +			    struct __test_metadata *_metadata,
> +			    unsigned long edmm_size)
>  {
>  	Elf64_Sym *sgx_enter_enclave_sym = NULL;
>  	struct vdso_symtab symtab;
> @@ -183,7 +191,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  	unsigned int i;
>  	void *addr;
>  
> -	if (!encl_load("test_encl.elf", encl, heap_size)) {
> +	if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) {
>  		encl_delete(encl);
>  		TH_LOG("Failed to load the test enclave.");
>  		return false;
> @@ -284,7 +292,7 @@ TEST_F(enclave, unclobbered_vdso)
>  	struct encl_op_get_from_buf get_op;
>  	struct encl_op_put_to_buf put_op;
>  
> -	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
> +	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
>  
>  	memset(&self->run, 0, sizeof(self->run));
>  	self->run.tcs = self->encl.encl_base;
> @@ -357,7 +365,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
>  
>  	total_mem = get_total_epc_mem();
>  	ASSERT_NE(total_mem, 0);
> -	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
> +	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
>  

The support for EDMM size and all the call site changes could be moved to a separate
patch to make the changes easier to parse.


...

> @@ -1210,6 +1218,103 @@ TEST_F(enclave, augment_via_eaccept)
>  	munmap(addr, PAGE_SIZE);
>  }
>  
> +/*
> + * Test for the addition of large number of pages to an initialized enclave via
> + * a pre-emptive run of EACCEPT on every page to be added.
> + */
> +TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG)
> +{
> +	struct encl_op_get_from_addr get_addr_op;
> +	struct encl_op_put_to_addr put_addr_op;
> +	struct encl_op_eaccept eaccept_op;
> +	size_t total_size = 0;
> +	unsigned long i;
> +	void *addr;
> +
> +	if (!sgx2_supported())
> +		SKIP(return, "SGX2 not supported");
> +
> +	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata,
> +				    EDMM_SIZE_LONG));
> +
> +	memset(&self->run, 0, sizeof(self->run));
> +	self->run.tcs = self->encl.encl_base;
> +
> +	for (i = 0; i < self->encl.nr_segments; i++) {
> +		struct encl_segment *seg = &self->encl.segment_tbl[i];
> +
> +		total_size += seg->size;
> +	}
> +
> +	/*
> +	 * mmap() every page at end of existing enclave to be used for
> +	 * EDMM.
> +	 */
> +	addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG,
> +			PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
> +			self->encl.fd, 0);
> +	EXPECT_NE(addr, MAP_FAILED);
> +
> +	self->run.exception_vector = 0;
> +	self->run.exception_error_code = 0;
> +	self->run.exception_addr = 0;
> +
> +	/*
> +	 * Run EACCEPT on new page to trigger the #PF->EAUG->EACCEPT(again
> +	 * without a #PF). All should be transparent to userspace.
> +	 */

s/on new page/on every page/ ?

> +	eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
> +	eaccept_op.ret = 0;
> +	eaccept_op.header.type = ENCL_OP_EACCEPT;
> +
> +	for (i = 0; i < EDMM_SIZE_LONG; i += 4096) {
> +		eaccept_op.epc_addr = (uint64_t)(addr + i);
> +
> +		EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
> +		if (self->run.exception_vector == 14 &&
> +			self->run.exception_error_code == 4 &&
> +			self->run.exception_addr == self->encl.encl_base) {
> +			munmap(addr, EDMM_SIZE_LONG);
> +			SKIP(return, "Kernel does not support adding pages to initialized enclave");
> +		}
> +
> +		EXPECT_EQ(self->run.exception_vector, 0);
> +		EXPECT_EQ(self->run.exception_error_code, 0);
> +		EXPECT_EQ(self->run.exception_addr, 0);
> +		ASSERT_EQ(eaccept_op.ret, 0);
> +		ASSERT_EQ(self->run.function, EEXIT);
> +	}
> +

I see that you removed all comments here after the discussion about what
comment would be appropriate. This may be ok but it does seem awkward that
there are two parts to this section and the second part still has a comment
while the first does not. How about the comment I provided in
https://lore.kernel.org/linux-sgx/69a5e350-ded9-30c9-dc41-d08c01dd05dc@intel.com/ :

/*
 * Pool of pages were successfully added to enclave. Perform sanity
 * check on first page of the pool only to ensure data can be written
 * to and read from a dynamically added enclave page.
 */

> +	put_addr_op.value = MAGIC;
> +	put_addr_op.addr = (unsigned long)addr;
> +	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
> +
> +	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
> +
> +	EXPECT_EEXIT(&self->run);
> +	EXPECT_EQ(self->run.exception_vector, 0);
> +	EXPECT_EQ(self->run.exception_error_code, 0);
> +	EXPECT_EQ(self->run.exception_addr, 0);
> +
> +	/*
> +	 * Read memory from newly added page that was just written to,
> +	 * confirming that data previously written (MAGIC) is present.
> +	 */
> +	get_addr_op.value = 0;
> +	get_addr_op.addr = (unsigned long)addr;
> +	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
> +
> +	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
> +
> +	EXPECT_EQ(get_addr_op.value, MAGIC);
> +	EXPECT_EEXIT(&self->run);
> +	EXPECT_EQ(self->run.exception_vector, 0);
> +	EXPECT_EQ(self->run.exception_error_code, 0);
> +	EXPECT_EQ(self->run.exception_addr, 0);
> +
> +	munmap(addr, EDMM_SIZE_LONG);
> +}
> +
>  /*
>   * SGX2 page type modification test in two phases:
>   * Phase 1:

...

> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..decd767434d5 100644
> --- a/tools/testing/selftests/sgx/sigstruct.c
> +++ b/tools/testing/selftests/sgx/sigstruct.c
> @@ -349,7 +349,7 @@ bool encl_measure(struct encl *encl)
>  	if (!ctx)
>  		goto err;
>  
> -	if (!mrenclave_ecreate(ctx, encl->src_size))
> +	if (!mrenclave_ecreate(ctx, encl->encl_size))
>  		goto err;
>  
>  	for (i = 0; i < encl->nr_segments; i++) {

As I mentioned in feedback to previous version, even though
encl_size is now provided, this misses that mrenclave_ecreate()
continues to recompute it within.

Reinette

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
> Hi Vijay and Jarkko,
> 
> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > 
> > Add a new test case which is same as augment_via_eaccept but adds a
> > larger number of EPC pages to stress test EAUG via EACCEPT.
> > 
> > Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v2:
> > - Addressed Reinette's feedback:
> >   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> > ---
> >  tools/testing/selftests/sgx/load.c      |   5 +-
> >  tools/testing/selftests/sgx/main.c      | 141 +++++++++++++++++++++---
> >  tools/testing/selftests/sgx/main.h      |   3 +-
> >  tools/testing/selftests/sgx/sigstruct.c |   2 +-
> >  4 files changed, 129 insertions(+), 22 deletions(-)
> 
> There seems to be at least three patches merged into one here:
> 1) Update SGX selftests to create enclaves with provided size dedicated
>    to EDMM (this change causes a lot of noise and distracts from the test
>    addition).
> 2) The mrenclave_ecreate() fix (which is still incomplete).
> 3) The actual test addition.

I would agree on this on a kernel patch but not for kselftest patch. It
does not really give useful value here. This adds a test and that is a
good enough granularity in my opinion, unless some major architecture
work is required as precursory. It is not the case here.

> > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> > index 94bdeac1cf04..7de1b15c90b1 100644
> > --- a/tools/testing/selftests/sgx/load.c
> > +++ b/tools/testing/selftests/sgx/load.c
> > @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
> >  	return 0;
> >  }
> >  
> > -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> > +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> > +			   unsigned long edmm_size)
> >  {
> 
> checkpatch.pl informs about alignment issues above and also a few other places.

Weird. I did run checkpatch through these. Will revisit.

> 
> >  	const char device_path[] = "/dev/sgx_enclave";
> >  	struct encl_segment *seg;
> > @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> >  
> >  	encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
> >  
> > -	for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
> > +	for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
> >  		encl->encl_size <<= 1;
> >  
> >  	return true;
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 9820b3809c69..867e98502120 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -21,8 +21,15 @@
> >  #include "../kselftest_harness.h"
> >  #include "main.h"
> >  
> > +/*
> > + * The size was chosen based on a bug report:
> > + * https://lore.kernel.org/linux-sgx/DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com/
> > + */
> > +static const unsigned long EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L; // 8 GB
> > +static const unsigned long TIMEOUT_LONG = 900; /* seconds */
> 
> It is interesting that in this small snippet there are three different
> comment styles (multi-line comment preceding code, tail comment using /**/,
> and tail comment using //) :)

Oops. Will fix.

> 
> >  static const uint64_t MAGIC = 0x1122334455667788ULL;
> >  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> > +
> 
> Addition of empty line here.

Will remove.

> 
> >  vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
> >  
> >  /*
> > @@ -173,7 +180,8 @@ FIXTURE(enclave) {
> >  };
> >  
> >  static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
> > -			    struct __test_metadata *_metadata)
> > +			    struct __test_metadata *_metadata,
> > +			    unsigned long edmm_size)
> >  {
> >  	Elf64_Sym *sgx_enter_enclave_sym = NULL;
> >  	struct vdso_symtab symtab;
> > @@ -183,7 +191,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
> >  	unsigned int i;
> >  	void *addr;
> >  
> > -	if (!encl_load("test_encl.elf", encl, heap_size)) {
> > +	if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) {
> >  		encl_delete(encl);
> >  		TH_LOG("Failed to load the test enclave.");
> >  		return false;
> > @@ -284,7 +292,7 @@ TEST_F(enclave, unclobbered_vdso)
> >  	struct encl_op_get_from_buf get_op;
> >  	struct encl_op_put_to_buf put_op;
> >  
> > -	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
> > +	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
> >  
> >  	memset(&self->run, 0, sizeof(self->run));
> >  	self->run.tcs = self->encl.encl_base;
> > @@ -357,7 +365,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
> >  
> >  	total_mem = get_total_epc_mem();
> >  	ASSERT_NE(total_mem, 0);
> > -	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
> > +	ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
> >  
> 
> The support for EDMM size and all the call site changes could be moved to a separate
> patch to make the changes easier to parse.
> 
> 
> ...
> 
> > @@ -1210,6 +1218,103 @@ TEST_F(enclave, augment_via_eaccept)
> >  	munmap(addr, PAGE_SIZE);
> >  }
> >  
> > +/*
> > + * Test for the addition of large number of pages to an initialized enclave via
> > + * a pre-emptive run of EACCEPT on every page to be added.
> > + */
> > +TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG)
> > +{
> > +	struct encl_op_get_from_addr get_addr_op;
> > +	struct encl_op_put_to_addr put_addr_op;
> > +	struct encl_op_eaccept eaccept_op;
> > +	size_t total_size = 0;
> > +	unsigned long i;
> > +	void *addr;
> > +
> > +	if (!sgx2_supported())
> > +		SKIP(return, "SGX2 not supported");
> > +
> > +	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata,
> > +				    EDMM_SIZE_LONG));
> > +
> > +	memset(&self->run, 0, sizeof(self->run));
> > +	self->run.tcs = self->encl.encl_base;
> > +
> > +	for (i = 0; i < self->encl.nr_segments; i++) {
> > +		struct encl_segment *seg = &self->encl.segment_tbl[i];
> > +
> > +		total_size += seg->size;
> > +	}
> > +
> > +	/*
> > +	 * mmap() every page at end of existing enclave to be used for
> > +	 * EDMM.
> > +	 */
> > +	addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG,
> > +			PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
> > +			self->encl.fd, 0);
> > +	EXPECT_NE(addr, MAP_FAILED);
> > +
> > +	self->run.exception_vector = 0;
> > +	self->run.exception_error_code = 0;
> > +	self->run.exception_addr = 0;
> > +
> > +	/*
> > +	 * Run EACCEPT on new page to trigger the #PF->EAUG->EACCEPT(again
> > +	 * without a #PF). All should be transparent to userspace.
> > +	 */
> 
> s/on new page/on every page/ ?

+1

> 
> > +	eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
> > +	eaccept_op.ret = 0;
> > +	eaccept_op.header.type = ENCL_OP_EACCEPT;
> > +
> > +	for (i = 0; i < EDMM_SIZE_LONG; i += 4096) {
> > +		eaccept_op.epc_addr = (uint64_t)(addr + i);
> > +
> > +		EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
> > +		if (self->run.exception_vector == 14 &&
> > +			self->run.exception_error_code == 4 &&
> > +			self->run.exception_addr == self->encl.encl_base) {
> > +			munmap(addr, EDMM_SIZE_LONG);
> > +			SKIP(return, "Kernel does not support adding pages to initialized enclave");
> > +		}
> > +
> > +		EXPECT_EQ(self->run.exception_vector, 0);
> > +		EXPECT_EQ(self->run.exception_error_code, 0);
> > +		EXPECT_EQ(self->run.exception_addr, 0);
> > +		ASSERT_EQ(eaccept_op.ret, 0);
> > +		ASSERT_EQ(self->run.function, EEXIT);
> > +	}
> > +
> 
> I see that you removed all comments here after the discussion about what
> comment would be appropriate. This may be ok but it does seem awkward that
> there are two parts to this section and the second part still has a comment
> while the first does not. How about the comment I provided in
> https://lore.kernel.org/linux-sgx/69a5e350-ded9-30c9-dc41-d08c01dd05dc@intel.com/ :
> 
> /*
>  * Pool of pages were successfully added to enclave. Perform sanity
>  * check on first page of the pool only to ensure data can be written
>  * to and read from a dynamically added enclave page.
>  */

+1

> 
> > +	put_addr_op.value = MAGIC;
> > +	put_addr_op.addr = (unsigned long)addr;
> > +	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	/*
> > +	 * Read memory from newly added page that was just written to,
> > +	 * confirming that data previously written (MAGIC) is present.
> > +	 */
> > +	get_addr_op.value = 0;
> > +	get_addr_op.addr = (unsigned long)addr;
> > +	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EQ(get_addr_op.value, MAGIC);
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	munmap(addr, EDMM_SIZE_LONG);
> > +}
> > +
> >  /*
> >   * SGX2 page type modification test in two phases:
> >   * Phase 1:
> 
> ...
> 
> > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> > index a07896a46364..decd767434d5 100644
> > --- a/tools/testing/selftests/sgx/sigstruct.c
> > +++ b/tools/testing/selftests/sgx/sigstruct.c
> > @@ -349,7 +349,7 @@ bool encl_measure(struct encl *encl)
> >  	if (!ctx)
> >  		goto err;
> >  
> > -	if (!mrenclave_ecreate(ctx, encl->src_size))
> > +	if (!mrenclave_ecreate(ctx, encl->encl_size))
> >  		goto err;
> >  
> >  	for (i = 0; i < encl->nr_segments; i++) {
> 
> As I mentioned in feedback to previous version, even though
> encl_size is now provided, this misses that mrenclave_ecreate()
> continues to recompute it within.
> 
> Reinette
> 

I simply forgot this one, sorry. Will fix.

BR, Jarkko

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Reinette Chatre]

Hi Jarkko,

On 8/30/2022 7:28 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
>>> From: Vijay Dhanraj <vijay.dhanraj@intel.com>
>>>
>>> Add a new test case which is same as augment_via_eaccept but adds a
>>> larger number of EPC pages to stress test EAUG via EACCEPT.
>>>
>>> Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
>>> Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>> v2:
>>> - Addressed Reinette's feedback:
>>>   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
>>> ---
>>>  tools/testing/selftests/sgx/load.c      |   5 +-
>>>  tools/testing/selftests/sgx/main.c      | 141 +++++++++++++++++++++---
>>>  tools/testing/selftests/sgx/main.h      |   3 +-
>>>  tools/testing/selftests/sgx/sigstruct.c |   2 +-
>>>  4 files changed, 129 insertions(+), 22 deletions(-)
>>
>> There seems to be at least three patches merged into one here:
>> 1) Update SGX selftests to create enclaves with provided size dedicated
>>    to EDMM (this change causes a lot of noise and distracts from the test
>>    addition).
>> 2) The mrenclave_ecreate() fix (which is still incomplete).
>> 3) The actual test addition.
> 
> I would agree on this on a kernel patch but not for kselftest patch. It
> does not really give useful value here. This adds a test and that is a
> good enough granularity in my opinion, unless some major architecture
> work is required as precursory. It is not the case here.

I must say that for many good reasons this goes against one of the
fundamental rules of kernel patches: separate logical changes into
separate patches. This is your domain though so of course the work
within it follows your guidance and I will not pursue it further.

> 
>>> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
>>> index 94bdeac1cf04..7de1b15c90b1 100644
>>> --- a/tools/testing/selftests/sgx/load.c
>>> +++ b/tools/testing/selftests/sgx/load.c
>>> @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>>>  	return 0;
>>>  }
>>>  
>>> -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
>>> +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
>>> +			   unsigned long edmm_size)
>>>  {
>>
>> checkpatch.pl informs about alignment issues above and also a few other places.
> 
> Weird. I did run checkpatch through these. Will revisit.

I usually run checkpatch.pl with "--strict".

Reinette

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:09:02AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/30/2022 7:28 PM, Jarkko Sakkinen wrote:
> > On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
> >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> >>> From: Vijay Dhanraj <vijay.dhanraj@intel.com>
> >>>
> >>> Add a new test case which is same as augment_via_eaccept but adds a
> >>> larger number of EPC pages to stress test EAUG via EACCEPT.
> >>>
> >>> Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
> >>> Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>> ---
> >>> v2:
> >>> - Addressed Reinette's feedback:
> >>>   https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> >>> ---
> >>>  tools/testing/selftests/sgx/load.c      |   5 +-
> >>>  tools/testing/selftests/sgx/main.c      | 141 +++++++++++++++++++++---
> >>>  tools/testing/selftests/sgx/main.h      |   3 +-
> >>>  tools/testing/selftests/sgx/sigstruct.c |   2 +-
> >>>  4 files changed, 129 insertions(+), 22 deletions(-)
> >>
> >> There seems to be at least three patches merged into one here:
> >> 1) Update SGX selftests to create enclaves with provided size dedicated
> >>    to EDMM (this change causes a lot of noise and distracts from the test
> >>    addition).
> >> 2) The mrenclave_ecreate() fix (which is still incomplete).
> >> 3) The actual test addition.
> > 
> > I would agree on this on a kernel patch but not for kselftest patch. It
> > does not really give useful value here. This adds a test and that is a
> > good enough granularity in my opinion, unless some major architecture
> > work is required as precursory. It is not the case here.
> 
> I must say that for many good reasons this goes against one of the
> fundamental rules of kernel patches: separate logical changes into
> separate patches. This is your domain though so of course the work
> within it follows your guidance and I will not pursue it further.

I don't consider kselftest patch exactly same as kernel patch
but I can split this. What would be good enough?

> I usually run checkpatch.pl with "--strict".

I honestly did not know that this option was available :-)
First time I hear of it.

Thanks.

> 
> Reinette

BR, Jarkko

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Reinette Chatre]

Hi Jarkko,

On 9/1/2022 3:16 PM, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2022 at 11:09:02AM -0700, Reinette Chatre wrote:
>> On 8/30/2022 7:28 PM, Jarkko Sakkinen wrote:
>>> On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
>>>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:

>>>> There seems to be at least three patches merged into one here:
>>>> 1) Update SGX selftests to create enclaves with provided size dedicated
>>>>    to EDMM (this change causes a lot of noise and distracts from the test
>>>>    addition).
>>>> 2) The mrenclave_ecreate() fix (which is still incomplete).
>>>> 3) The actual test addition.
>>>
>>> I would agree on this on a kernel patch but not for kselftest patch. It
>>> does not really give useful value here. This adds a test and that is a
>>> good enough granularity in my opinion, unless some major architecture
>>> work is required as precursory. It is not the case here.
>>
>> I must say that for many good reasons this goes against one of the
>> fundamental rules of kernel patches: separate logical changes into
>> separate patches. This is your domain though so of course the work
>> within it follows your guidance and I will not pursue it further.
> 
> I don't consider kselftest patch exactly same as kernel patch

You are not alone.

> but I can split this. What would be good enough?

I identified three candidate patches in my original response that
is quoted above, but as I mentioned I understand the sentiment
and this is your domain so I will not insist on it.

Reinette

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Thu, Sep 01, 2022 at 04:11:34PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 9/1/2022 3:16 PM, Jarkko Sakkinen wrote:
> > On Wed, Aug 31, 2022 at 11:09:02AM -0700, Reinette Chatre wrote:
> >> On 8/30/2022 7:28 PM, Jarkko Sakkinen wrote:
> >>> On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
> >>>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> 
> >>>> There seems to be at least three patches merged into one here:
> >>>> 1) Update SGX selftests to create enclaves with provided size dedicated
> >>>>    to EDMM (this change causes a lot of noise and distracts from the test
> >>>>    addition).
> >>>> 2) The mrenclave_ecreate() fix (which is still incomplete).
> >>>> 3) The actual test addition.
> >>>
> >>> I would agree on this on a kernel patch but not for kselftest patch. It
> >>> does not really give useful value here. This adds a test and that is a
> >>> good enough granularity in my opinion, unless some major architecture
> >>> work is required as precursory. It is not the case here.
> >>
> >> I must say that for many good reasons this goes against one of the
> >> fundamental rules of kernel patches: separate logical changes into
> >> separate patches. This is your domain though so of course the work
> >> within it follows your guidance and I will not pursue it further.
> > 
> > I don't consider kselftest patch exactly same as kernel patch
> 
> You are not alone.
> 
> > but I can split this. What would be good enough?
> 
> I identified three candidate patches in my original response that
> is quoted above, but as I mentioned I understand the sentiment
> and this is your domain so I will not insist on it.

OK, fair enough, I'll rework on this. It's my domain but
at least my own aim is always only satisfy on consensus
:-)

BR, Jarkko

Re: [PATCH 4/6] selftests/sgx: Add SGX selftest augment_via_eaccept_long

[Jarkko Sakkinen]

On Fri, Sep 02, 2022 at 03:00:36AM +0300, Jarkko Sakkinen wrote:
> On Thu, Sep 01, 2022 at 04:11:34PM -0700, Reinette Chatre wrote:
> > Hi Jarkko,
> > 
> > On 9/1/2022 3:16 PM, Jarkko Sakkinen wrote:
> > > On Wed, Aug 31, 2022 at 11:09:02AM -0700, Reinette Chatre wrote:
> > >> On 8/30/2022 7:28 PM, Jarkko Sakkinen wrote:
> > >>> On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
> > >>>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > 
> > >>>> There seems to be at least three patches merged into one here:
> > >>>> 1) Update SGX selftests to create enclaves with provided size dedicated
> > >>>>    to EDMM (this change causes a lot of noise and distracts from the test
> > >>>>    addition).
> > >>>> 2) The mrenclave_ecreate() fix (which is still incomplete).
> > >>>> 3) The actual test addition.
> > >>>
> > >>> I would agree on this on a kernel patch but not for kselftest patch. It
> > >>> does not really give useful value here. This adds a test and that is a
> > >>> good enough granularity in my opinion, unless some major architecture
> > >>> work is required as precursory. It is not the case here.
> > >>
> > >> I must say that for many good reasons this goes against one of the
> > >> fundamental rules of kernel patches: separate logical changes into
> > >> separate patches. This is your domain though so of course the work
> > >> within it follows your guidance and I will not pursue it further.
> > > 
> > > I don't consider kselftest patch exactly same as kernel patch
> > 
> > You are not alone.
> > 
> > > but I can split this. What would be good enough?
> > 
> > I identified three candidate patches in my original response that
> > is quoted above, but as I mentioned I understand the sentiment
> > and this is your domain so I will not insist on it.
> 
> OK, fair enough, I'll rework on this. It's my domain but
> at least my own aim is always only satisfy on consensus
> :-)

I'll also split the patch set because this is not as urgent
as getting the fixes in. There will be separate fixes and
kselftest improvements patch sets.

BR, Jarkko

[PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

From: Haitao Huang <haitao.huang@linux.intel.com>

For EMODT and EREMOVE ioctls with a large range, kernel
may not finish in one shot and return EAGAIN error code
and count of bytes of EPC pages on that operations are
finished successfully.

Change the unclobbered_vdso_oversubscribed_remove test
to rerun the ioctls in a loop, updating offset and length
using the byte count returned in each iteration.

Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/main.c | 39 +++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 867e98502120..3268d8b01b0b 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	unsigned long total_mem;
 	int ret, errno_save;
 	unsigned long addr;
-	unsigned long i;
+	unsigned long i, count;
 
 	/*
 	 * Create enclave with additional heap that is as big as all
@@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 	modt_ioc.offset = heap->offset;
 	modt_ioc.length = heap->size;
 	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
-
+	count = 0;
 	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
 	       heap->size);
-	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
-	errno_save = ret == -1 ? errno : 0;
+	do {
+		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
+		errno_save = ret == -1 ? errno : 0;
+		if (errno_save == EAGAIN) {
+			count += modt_ioc.count;
+			modt_ioc.offset += modt_ioc.count;
+			modt_ioc.length -= modt_ioc.count;
+			modt_ioc.result = 0;
+			modt_ioc.count = 0;
+		} else
+			break;
+	} while (modt_ioc.length != 0);
 
 	EXPECT_EQ(ret, 0);
 	EXPECT_EQ(errno_save, 0);
 	EXPECT_EQ(modt_ioc.result, 0);
-	EXPECT_EQ(modt_ioc.count, heap->size);
+	count += modt_ioc.count;
+	EXPECT_EQ(count, heap->size);
 
 	/* EACCEPT all removed pages. */
 	addr = self->encl.encl_base + heap->offset;
@@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
 
 	remove_ioc.offset = heap->offset;
 	remove_ioc.length = heap->size;
-
+	count = 0;
 	TH_LOG("Removing %zd bytes from enclave may take a while ...",
 	       heap->size);
-	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
-	errno_save = ret == -1 ? errno : 0;
+	do {
+		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
+		errno_save = ret == -1 ? errno : 0;
+		if (errno_save == EAGAIN) {
+			count += remove_ioc.count;
+			remove_ioc.offset += remove_ioc.count;
+			remove_ioc.length -= remove_ioc.count;
+			remove_ioc.count = 0;
+		} else
+			break;
+	} while (remove_ioc.length != 0);
 
 	EXPECT_EQ(ret, 0);
 	EXPECT_EQ(errno_save, 0);
-	EXPECT_EQ(remove_ioc.count, heap->size);
+	count += remove_ioc.count;
+	EXPECT_EQ(count, heap->size);
 }
 
 TEST_F(enclave, clobbered_vdso)
-- 
2.37.2

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Reinette Chatre]

Hi Haitao and Jarkko,


selftests/sgx: Retry the ioctl()s returned with EAGAIN


On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> From: Haitao Huang <haitao.huang@linux.intel.com>
> 
> For EMODT and EREMOVE ioctls with a large range, kernel

ioctl()s?

> may not finish in one shot and return EAGAIN error code
> and count of bytes of EPC pages on that operations are
> finished successfully.
> 
> Change the unclobbered_vdso_oversubscribed_remove test
> to rerun the ioctls in a loop, updating offset and length

ioctl()s?

> using the byte count returned in each iteration.
> 

This is a valuable addition, thank you very much.

> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  tools/testing/selftests/sgx/main.c | 39 +++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 867e98502120..3268d8b01b0b 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	unsigned long total_mem;
>  	int ret, errno_save;
>  	unsigned long addr;
> -	unsigned long i;
> +	unsigned long i, count;

Reverse fir tree?

>  
>  	/*
>  	 * Create enclave with additional heap that is as big as all
> @@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  	modt_ioc.offset = heap->offset;
>  	modt_ioc.length = heap->size;
>  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> -
> +	count = 0;
>  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
>  	       heap->size);
> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> -	errno_save = ret == -1 ? errno : 0;
> +	do {
> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> +		errno_save = ret == -1 ? errno : 0;
> +		if (errno_save == EAGAIN) {
> +			count += modt_ioc.count;
> +			modt_ioc.offset += modt_ioc.count;
> +			modt_ioc.length -= modt_ioc.count;
> +			modt_ioc.result = 0;

As part of the test I think it would be helpful to know if there was a result code
in here. What do you think of failing the test if it is not zero?

> +			modt_ioc.count = 0;
> +		} else
> +			break;

Watch out for unbalanced braces (also later in patch). This causes
checkpatch.pl noise.

> +	} while (modt_ioc.length != 0);
>  
>  	EXPECT_EQ(ret, 0);
>  	EXPECT_EQ(errno_save, 0);
>  	EXPECT_EQ(modt_ioc.result, 0);
> -	EXPECT_EQ(modt_ioc.count, heap->size);
> +	count += modt_ioc.count;
> +	EXPECT_EQ(count, heap->size);
>  
>  	/* EACCEPT all removed pages. */
>  	addr = self->encl.encl_base + heap->offset;
> @@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
>  
>  	remove_ioc.offset = heap->offset;
>  	remove_ioc.length = heap->size;
> -
> +	count = 0;
>  	TH_LOG("Removing %zd bytes from enclave may take a while ...",
>  	       heap->size);
> -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
> -	errno_save = ret == -1 ? errno : 0;
> +	do {
> +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
> +		errno_save = ret == -1 ? errno : 0;
> +		if (errno_save == EAGAIN) {
> +			count += remove_ioc.count;
> +			remove_ioc.offset += remove_ioc.count;
> +			remove_ioc.length -= remove_ioc.count;
> +			remove_ioc.count = 0;
> +		} else
> +			break;
> +	} while (remove_ioc.length != 0);
>  
>  	EXPECT_EQ(ret, 0);
>  	EXPECT_EQ(errno_save, 0);
> -	EXPECT_EQ(remove_ioc.count, heap->size);
> +	count += remove_ioc.count;
> +	EXPECT_EQ(count, heap->size);
>  }
>  
>  TEST_F(enclave, clobbered_vdso)

Reinette

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote:
> Hi Haitao and Jarkko,
> 
> 
> selftests/sgx: Retry the ioctl()s returned with EAGAIN
> 
> 
> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > From: Haitao Huang <haitao.huang@linux.intel.com>
> > 
> > For EMODT and EREMOVE ioctls with a large range, kernel
> 
> ioctl()s?

Ioctl is common enough to be considered as noun and is
widely phrased like that in commit messages. I don't
see any added clarity.

> 
> > may not finish in one shot and return EAGAIN error code
> > and count of bytes of EPC pages on that operations are
> > finished successfully.
> > 
> > Change the unclobbered_vdso_oversubscribed_remove test
> > to rerun the ioctls in a loop, updating offset and length
> 
> ioctl()s?
> 
> > using the byte count returned in each iteration.
> > 
> 
> This is a valuable addition, thank you very much.
> 
> > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  tools/testing/selftests/sgx/main.c | 39 +++++++++++++++++++++++-------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 867e98502120..3268d8b01b0b 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >  	unsigned long total_mem;
> >  	int ret, errno_save;
> >  	unsigned long addr;
> > -	unsigned long i;
> > +	unsigned long i, count;
> 
> Reverse fir tree?

+1

> 
> >  
> >  	/*
> >  	 * Create enclave with additional heap that is as big as all
> > @@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >  	modt_ioc.offset = heap->offset;
> >  	modt_ioc.length = heap->size;
> >  	modt_ioc.page_type = SGX_PAGE_TYPE_TRIM;
> > -
> > +	count = 0;
> >  	TH_LOG("Changing type of %zd bytes to trimmed may take a while ...",
> >  	       heap->size);
> > -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > -	errno_save = ret == -1 ? errno : 0;
> > +	do {
> > +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc);
> > +		errno_save = ret == -1 ? errno : 0;
> > +		if (errno_save == EAGAIN) {
> > +			count += modt_ioc.count;
> > +			modt_ioc.offset += modt_ioc.count;
> > +			modt_ioc.length -= modt_ioc.count;
> > +			modt_ioc.result = 0;
> 
> As part of the test I think it would be helpful to know if there was a result code
> in here. What do you think of failing the test if it is not zero?

I would not mind doing that.

> 
> > +			modt_ioc.count = 0;
> > +		} else
> > +			break;
> 
> Watch out for unbalanced braces (also later in patch). This causes
> checkpatch.pl noise.

Again. I did run checkpatch to all of these. Will revisit.

> 
> > +	} while (modt_ioc.length != 0);
> >  
> >  	EXPECT_EQ(ret, 0);
> >  	EXPECT_EQ(errno_save, 0);
> >  	EXPECT_EQ(modt_ioc.result, 0);
> > -	EXPECT_EQ(modt_ioc.count, heap->size);
> > +	count += modt_ioc.count;
> > +	EXPECT_EQ(count, heap->size);
> >  
> >  	/* EACCEPT all removed pages. */
> >  	addr = self->encl.encl_base + heap->offset;
> > @@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900)
> >  
> >  	remove_ioc.offset = heap->offset;
> >  	remove_ioc.length = heap->size;
> > -
> > +	count = 0;
> >  	TH_LOG("Removing %zd bytes from enclave may take a while ...",
> >  	       heap->size);
> > -	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
> > -	errno_save = ret == -1 ? errno : 0;
> > +	do {
> > +		ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc);
> > +		errno_save = ret == -1 ? errno : 0;
> > +		if (errno_save == EAGAIN) {
> > +			count += remove_ioc.count;
> > +			remove_ioc.offset += remove_ioc.count;
> > +			remove_ioc.length -= remove_ioc.count;
> > +			remove_ioc.count = 0;
> > +		} else
> > +			break;
> > +	} while (remove_ioc.length != 0);
> >  
> >  	EXPECT_EQ(ret, 0);
> >  	EXPECT_EQ(errno_save, 0);
> > -	EXPECT_EQ(remove_ioc.count, heap->size);
> > +	count += remove_ioc.count;
> > +	EXPECT_EQ(count, heap->size);
> >  }
> >  
> >  TEST_F(enclave, clobbered_vdso)
> 
> Reinette


BR, Jarkko

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Reinette Chatre]

Hi Jarkko,

On 8/30/2022 7:31 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote:
>> Hi Haitao and Jarkko,
>>
>>
>> selftests/sgx: Retry the ioctl()s returned with EAGAIN
>>
>>
>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
>>> From: Haitao Huang <haitao.huang@linux.intel.com>
>>>
>>> For EMODT and EREMOVE ioctls with a large range, kernel
>>
>> ioctl()s?
> 
> Ioctl is common enough to be considered as noun and is
> widely phrased like that in commit messages. I don't
> see any added clarity.

ok. I was asked to make this change in the SGX2 patches and
thought that I should propagate this advice :)

>>> +			modt_ioc.count = 0;
>>> +		} else
>>> +			break;
>>
>> Watch out for unbalanced braces (also later in patch). This causes
>> checkpatch.pl noise.
> 
> Again. I did run checkpatch to all of these. Will revisit.

It looks like I see it because I use "checkpatch.pl --strict".

Reinette

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:09:21AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/30/2022 7:31 PM, Jarkko Sakkinen wrote:
> > On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote:
> >> Hi Haitao and Jarkko,
> >>
> >>
> >> selftests/sgx: Retry the ioctl()s returned with EAGAIN
> >>
> >>
> >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> >>> From: Haitao Huang <haitao.huang@linux.intel.com>
> >>>
> >>> For EMODT and EREMOVE ioctls with a large range, kernel
> >>
> >> ioctl()s?
> > 
> > Ioctl is common enough to be considered as noun and is
> > widely phrased like that in commit messages. I don't
> > see any added clarity.
> 
> ok. I was asked to make this change in the SGX2 patches and
> thought that I should propagate this advice :)

I can use the other form too, np.

> 
> >>> +			modt_ioc.count = 0;
> >>> +		} else
> >>> +			break;
> >>
> >> Watch out for unbalanced braces (also later in patch). This causes
> >> checkpatch.pl noise.
> > 
> > Again. I did run checkpatch to all of these. Will revisit.
> 
> It looks like I see it because I use "checkpatch.pl --strict".

Thanks BTW for pointing this out :-)

> Reinette

BR, Jarkko

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Dave Hansen]

On 8/30/22 19:31, Jarkko Sakkinen wrote:
>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
>>> From: Haitao Huang <haitao.huang@linux.intel.com>
>>>
>>> For EMODT and EREMOVE ioctls with a large range, kernel
>> ioctl()s?
> Ioctl is common enough to be considered as noun and is
> widely phrased like that in commit messages. I don't
> see any added clarity.

I definitely prefer ioctl().

Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:14:02AM -0700, Dave Hansen wrote:
> On 8/30/22 19:31, Jarkko Sakkinen wrote:
> >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> >>> From: Haitao Huang <haitao.huang@linux.intel.com>
> >>>
> >>> For EMODT and EREMOVE ioctls with a large range, kernel
> >> ioctl()s?
> > Ioctl is common enough to be considered as noun and is
> > widely phrased like that in commit messages. I don't
> > see any added clarity.
> 
> I definitely prefer ioctl().

I can use it.

BR, Jarkko

[PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/alloc-error.bt | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 tools/testing/selftests/sgx/alloc-error.bt

diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
new file mode 100644
index 000000000000..9268d50dea29
--- /dev/null
+++ b/tools/testing/selftests/sgx/alloc-error.bt
@@ -0,0 +1,7 @@
+kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
+		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
+}
+
+kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
+		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
+}
-- 
2.37.2

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Reinette Chatre]

Hi Jarkko,

On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  tools/testing/selftests/sgx/alloc-error.bt | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
> 
> diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> new file mode 100644
> index 000000000000..9268d50dea29
> --- /dev/null
> +++ b/tools/testing/selftests/sgx/alloc-error.bt
> @@ -0,0 +1,7 @@
> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> +}
> +
> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> +}


Could there be a snippet of comments in this new file to guide users
on how to use this script?

Reinette

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

On Tue, Aug 30, 2022 at 03:57:24PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  tools/testing/selftests/sgx/alloc-error.bt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
> > 
> > diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> > new file mode 100644
> > index 000000000000..9268d50dea29
> > --- /dev/null
> > +++ b/tools/testing/selftests/sgx/alloc-error.bt
> > @@ -0,0 +1,7 @@
> > +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> > +}
> > +
> > +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> > +}
> 
> 
> Could there be a snippet of comments in this new file to guide users
> on how to use this script?

Do not mean to be rude but I'm not sure what there is to guide but
I'm open for ideas.

BR, Jarkko

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Reinette Chatre]

Hi Jarkko,

On 8/30/2022 7:33 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 30, 2022 at 03:57:24PM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>>  tools/testing/selftests/sgx/alloc-error.bt | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
>>>
>>> diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
>>> new file mode 100644
>>> index 000000000000..9268d50dea29
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/sgx/alloc-error.bt
>>> @@ -0,0 +1,7 @@
>>> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
>>> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
>>> +}
>>> +
>>> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
>>> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
>>> +}
>>
>>
>> Could there be a snippet of comments in this new file to guide users
>> on how to use this script?
> 
> Do not mean to be rude but I'm not sure what there is to guide but
> I'm open for ideas.

How about something like below in comments as part of the script:

"bpftrace script using kretprobe to trace returns of some key functions
 in support of tracking allocation errors."

This is essentially in the subject line of the patch but that information
will be lost when somebody looks at the script and tries to figure
out what it is.

Reinette

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:10:20AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 8/30/2022 7:33 PM, Jarkko Sakkinen wrote:
> > On Tue, Aug 30, 2022 at 03:57:24PM -0700, Reinette Chatre wrote:
> >> Hi Jarkko,
> >>
> >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>> ---
> >>>  tools/testing/selftests/sgx/alloc-error.bt | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>  create mode 100644 tools/testing/selftests/sgx/alloc-error.bt
> >>>
> >>> diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> >>> new file mode 100644
> >>> index 000000000000..9268d50dea29
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/sgx/alloc-error.bt
> >>> @@ -0,0 +1,7 @@
> >>> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> >>> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> >>> +}
> >>> +
> >>> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> >>> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> >>> +}
> >>
> >>
> >> Could there be a snippet of comments in this new file to guide users
> >> on how to use this script?
> > 
> > Do not mean to be rude but I'm not sure what there is to guide but
> > I'm open for ideas.
> 
> How about something like below in comments as part of the script:
> 
> "bpftrace script using kretprobe to trace returns of some key functions
>  in support of tracking allocation errors."

I think comments that I put (before seeing) also
make it clear enough (not to say that what you
had not been a valid alternative).

BR, Jarkko

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Dave Hansen]

On 8/29/22 20:12, Jarkko Sakkinen wrote:
> diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> new file mode 100644
> index 000000000000..9268d50dea29
> --- /dev/null
> +++ b/tools/testing/selftests/sgx/alloc-error.bt
> @@ -0,0 +1,7 @@
> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> +}
> +
> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> +}

I guess this doesn't _hurt_, but it's also not exactly the easiest way
to get this done.  You need a whole bpf toolchain.  You could also just do:

	perf probe 'sgx_encl_page_alloc%return $retval'

Even *that* can be replicated in a few scant lines of shell code echoing
into /sys/kernel/debug/tracing.

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

On Wed, Aug 31, 2022 at 11:23:55AM -0700, Dave Hansen wrote:
> On 8/29/22 20:12, Jarkko Sakkinen wrote:
> > diff --git a/tools/testing/selftests/sgx/alloc-error.bt b/tools/testing/selftests/sgx/alloc-error.bt
> > new file mode 100644
> > index 000000000000..9268d50dea29
> > --- /dev/null
> > +++ b/tools/testing/selftests/sgx/alloc-error.bt
> > @@ -0,0 +1,7 @@
> > +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> > +}
> > +
> > +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> > +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> > +}
> 
> I guess this doesn't _hurt_, but it's also not exactly the easiest way
> to get this done.  You need a whole bpf toolchain.  You could also just do:
> 
> 	perf probe 'sgx_encl_page_alloc%return $retval'
> 
> Even *that* can be replicated in a few scant lines of shell code echoing
> into /sys/kernel/debug/tracing.

Thanks, I have not used perf that much. What if I replace
this with a shell script using perf? How do you use that
for two kretprobes?

BR, Jarkko

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Dave Hansen]

On 9/1/22 15:20, Jarkko Sakkinen wrote:
>>> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
>>> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
>>> +}
>>> +
>>> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
>>> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
>>> +}
>> I guess this doesn't _hurt_, but it's also not exactly the easiest way
>> to get this done.  You need a whole bpf toolchain.  You could also just do:
>>
>> 	perf probe 'sgx_encl_page_alloc%return $retval'
>>
>> Even *that* can be replicated in a few scant lines of shell code echoing
>> into /sys/kernel/debug/tracing.
> Thanks, I have not used perf that much. What if I replace
> this with a shell script using perf? How do you use that
> for two kretprobes?

The manpage is pretty good.

But, I'd proably be doing something along these lines:

	perf probe 'sgx_encl_page_alloc%return ret=$retval'
	perf record -e probe:sgx_encl_page_alloc -aR	\
		--filter='ret >= 0xwhatever' sleep 1
	perf script

There are probably shorter ways to do it, but I'm pretty sure that works.
	

Re: [PATCH 6/6] selftests/sgx: Add a bpftrace script for tracking allocation errors

[Jarkko Sakkinen]

On Thu, Sep 01, 2022 at 03:34:49PM -0700, Dave Hansen wrote:
> On 9/1/22 15:20, Jarkko Sakkinen wrote:
> >>> +kr:sgx_alloc_epc_page /(uint64)retval >= (uint64)(-4095)/ {
> >>> +		 printf("sgx_alloc_epc_page: retval=%d\n", (int64)retval);
> >>> +}
> >>> +
> >>> +kr:sgx_encl_page_alloc /(uint64)retval >= (uint64)(-4095)/ {
> >>> +		 printf("sgx_encl_page_alloc: retval=%d\n", (int64)retval);
> >>> +}
> >> I guess this doesn't _hurt_, but it's also not exactly the easiest way
> >> to get this done.  You need a whole bpf toolchain.  You could also just do:
> >>
> >> 	perf probe 'sgx_encl_page_alloc%return $retval'
> >>
> >> Even *that* can be replicated in a few scant lines of shell code echoing
> >> into /sys/kernel/debug/tracing.
> > Thanks, I have not used perf that much. What if I replace
> > this with a shell script using perf? How do you use that
> > for two kretprobes?
> 
> The manpage is pretty good.
> 
> But, I'd proably be doing something along these lines:
> 
> 	perf probe 'sgx_encl_page_alloc%return ret=$retval'
> 	perf record -e probe:sgx_encl_page_alloc -aR	\
> 		--filter='ret >= 0xwhatever' sleep 1
> 	perf script
> 
> There are probably shorter ways to do it, but I'm pretty sure that works.

Thanks, will give it a shot.

BR, Jarkko