linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
@ 2021-05-26 12:47 Jarkko Sakkinen
  2021-05-26 12:47 ` [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-05-26 12:47 UTC (permalink / raw)
  To: shuah
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
	Dave Hansen, linux-kernel

Rename symbols for better clarity:

* 'eenter' might be confused for directly calling ENCLU[EENTER].  It does
  not.  It calls into the VDSO, which actually has the EENTER instruction.
* 'sgx_call_vdso' is *only* used for entering the enclave.  It's not some
  generic SGX call into the VDSO.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---

v3:
Refine the commit message according to Dave's suggestions.

v2:
Refined the renames just a bit.

 tools/testing/selftests/sgx/call.S |  6 +++---
 tools/testing/selftests/sgx/main.c | 25 +++++++++++++------------
 tools/testing/selftests/sgx/main.h |  4 ++--
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/sgx/call.S b/tools/testing/selftests/sgx/call.S
index 4ecadc7490f4..b09a25890f3b 100644
--- a/tools/testing/selftests/sgx/call.S
+++ b/tools/testing/selftests/sgx/call.S
@@ -5,8 +5,8 @@
 
 	.text
 
-	.global sgx_call_vdso
-sgx_call_vdso:
+	.global sgx_enter_enclave
+sgx_enter_enclave:
 	.cfi_startproc
 	push	%r15
 	.cfi_adjust_cfa_offset	8
@@ -27,7 +27,7 @@ sgx_call_vdso:
 	.cfi_adjust_cfa_offset	8
 	push	0x38(%rsp)
 	.cfi_adjust_cfa_offset	8
-	call	*eenter(%rip)
+	call	*vdso_sgx_enter_enclave(%rip)
 	add	$0x10, %rsp
 	.cfi_adjust_cfa_offset	-0x10
 	pop	%rbx
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index d304a4044eb9..43da68388e25 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -21,7 +21,7 @@
 #include "../kselftest.h"
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
-vdso_sgx_enter_enclave_t eenter;
+vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
 
 struct vdso_symtab {
 	Elf64_Sym *elf_symtab;
@@ -149,7 +149,7 @@ int main(int argc, char *argv[])
 {
 	struct sgx_enclave_run run;
 	struct vdso_symtab symtab;
-	Elf64_Sym *eenter_sym;
+	Elf64_Sym *sgx_enter_enclave_sym;
 	uint64_t result = 0;
 	struct encl encl;
 	unsigned int i;
@@ -194,29 +194,30 @@ int main(int argc, char *argv[])
 	if (!vdso_get_symtab(addr, &symtab))
 		goto err;
 
-	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
-	if (!eenter_sym)
+	sgx_enter_enclave_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
+	if (!sgx_enter_enclave_sym)
 		goto err;
 
-	eenter = addr + eenter_sym->st_value;
+	vdso_sgx_enter_enclave = addr + sgx_enter_enclave_sym->st_value;
 
-	ret = sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
-	if (!report_results(&run, ret, result, "sgx_call_vdso"))
+	ret = sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER,
+					    NULL, NULL, &run);
+	if (!report_results(&run, ret, result, "sgx_enter_enclave_unclobbered"))
 		goto err;
 
 
 	/* Invoke the vDSO directly. */
 	result = 0;
-	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
-		     0, 0, &run);
-	if (!report_results(&run, ret, result, "eenter"))
+	ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
+				     0, EENTER, 0, 0, &run);
+	if (!report_results(&run, ret, result, "sgx_enter_enclave"))
 		goto err;
 
 	/* And with an exit handler. */
 	run.user_handler = (__u64)user_handler;
 	run.user_data = 0xdeadbeef;
-	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
-		     0, 0, &run);
+	ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
+				     0, EENTER, 0, 0, &run);
 	if (!report_results(&run, ret, result, "user_handler"))
 		goto err;
 
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 67211a708f04..68672fd86cf9 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -35,7 +35,7 @@ bool encl_load(const char *path, struct encl *encl);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 
-int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
-		  struct sgx_enclave_run *run);
+int sgx_enter_enclave(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
+		      struct sgx_enclave_run *run);
 
 #endif /* MAIN_H */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness
  2021-05-26 12:47 [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
@ 2021-05-26 12:47 ` Jarkko Sakkinen
  2021-06-07 16:35   ` Reinette Chatre
  2021-05-26 12:47 ` [PATCH v7 3/4] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-05-26 12:47 UTC (permalink / raw)
  To: shuah
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
	Dave Hansen, linux-kernel

Migrate to kselftest harness. Use a fixture test with enclave initialized
and de-initialized for each of the existing three tests, in other words:

1. One FIXTURE() for managing the enclave life-cycle.
2. Three TEST_F()'s, one for each test case.

Dump lines of /proc/self/maps matching "sgx" in FIXTURE_SETUP() as this
can be very useful debugging information later on.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---

v7:
* Implement encl_call() that wraps the logic of invoking the vDSO for the
  test enclave.

v6:
* Remove extra '\n' from TH_LOG() calls.

v5:
* Use TH_LOG() for printing enclave address ranges instead of printf(),
  based on Reinette's remark.

v4:
* Refine to take better use of the kselftest harness macros.
* Fix: TCS base address was not initialized for a run struct.

v3:
* Use helper macros.

v2:
* Add the missing string argument to ksft_test_result_pass() and
  ksft_test_result_fail() calls.

 tools/testing/selftests/sgx/load.c |   3 -
 tools/testing/selftests/sgx/main.c | 179 +++++++++++++++--------------
 2 files changed, 94 insertions(+), 88 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index f441ac34b4d4..00928be57fc4 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -239,9 +239,6 @@ bool encl_load(const char *path, struct encl *encl)
 		seg->offset = (phdr->p_offset & PAGE_MASK) - src_offset;
 		seg->size = (phdr->p_filesz + PAGE_SIZE - 1) & PAGE_MASK;
 
-		printf("0x%016lx 0x%016lx 0x%02x\n", seg->offset, seg->size,
-		       seg->prot);
-
 		j++;
 	}
 
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 43da68388e25..8694772c8fd9 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -17,8 +17,8 @@
 #include <sys/types.h>
 #include <sys/auxv.h>
 #include "defines.h"
+#include "../kselftest_harness.h"
 #include "main.h"
-#include "../kselftest.h"
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
@@ -107,85 +107,49 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
 	return NULL;
 }
 
-bool report_results(struct sgx_enclave_run *run, int ret, uint64_t result,
-		  const char *test)
-{
-	bool valid = true;
-
-	if (ret) {
-		printf("FAIL: %s() returned: %d\n", test, ret);
-		valid = false;
-	}
-
-	if (run->function != EEXIT) {
-		printf("FAIL: %s() function, expected: %u, got: %u\n", test, EEXIT,
-		       run->function);
-		valid = false;
-	}
-
-	if (result != MAGIC) {
-		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", test, MAGIC,
-		       result);
-		valid = false;
-	}
-
-	if (run->user_data) {
-		printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n",
-		       test, run->user_data);
-		valid = false;
-	}
-
-	return valid;
-}
-
-static int user_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
-			struct sgx_enclave_run *run)
-{
-	run->user_data = 0;
-	return 0;
-}
+FIXTURE(enclave) {
+	struct encl encl;
+	struct sgx_enclave_run run;
+};
 
-int main(int argc, char *argv[])
+FIXTURE_SETUP(enclave)
 {
-	struct sgx_enclave_run run;
+	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
-	Elf64_Sym *sgx_enter_enclave_sym;
-	uint64_t result = 0;
-	struct encl encl;
+	struct encl_segment *seg;
 	unsigned int i;
 	void *addr;
-	int ret;
 
-	memset(&run, 0, sizeof(run));
-
-	if (!encl_load("test_encl.elf", &encl)) {
-		encl_delete(&encl);
+	if (!encl_load("test_encl.elf", &self->encl)) {
+		encl_delete(&self->encl);
 		ksft_exit_skip("cannot load enclaves\n");
 	}
 
-	if (!encl_measure(&encl))
+	for (i = 0; i < self->encl.nr_segments; i++) {
+		seg = &self->encl.segment_tbl[i];
+
+		TH_LOG("0x%016lx 0x%016lx 0x%02x", seg->offset, seg->size, seg->prot);
+	}
+
+	if (!encl_measure(&self->encl))
 		goto err;
 
-	if (!encl_build(&encl))
+	if (!encl_build(&self->encl))
 		goto err;
 
 	/*
 	 * An enclave consumer only must do this.
 	 */
-	for (i = 0; i < encl.nr_segments; i++) {
-		struct encl_segment *seg = &encl.segment_tbl[i];
-
-		addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
-			    seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
-		if (addr == MAP_FAILED) {
-			perror("mmap() segment failed");
-			exit(KSFT_FAIL);
-		}
+	for (i = 0; i < self->encl.nr_segments; i++) {
+		struct encl_segment *seg = &self->encl.segment_tbl[i];
+
+		addr = mmap((void *)self->encl.encl_base + seg->offset, seg->size,
+			    seg->prot, MAP_SHARED | MAP_FIXED, self->encl.fd, 0);
+		EXPECT_NE(addr, MAP_FAILED);
+		if (addr == MAP_FAILED)
+			goto err;
 	}
 
-	memset(&run, 0, sizeof(run));
-	run.tcs = encl.encl_base;
-
 	/* Get vDSO base address */
 	addr = (void *)getauxval(AT_SYSINFO_EHDR);
 	if (!addr)
@@ -200,32 +164,77 @@ int main(int argc, char *argv[])
 
 	vdso_sgx_enter_enclave = addr + sgx_enter_enclave_sym->st_value;
 
-	ret = sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER,
-					    NULL, NULL, &run);
-	if (!report_results(&run, ret, result, "sgx_enter_enclave_unclobbered"))
-		goto err;
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
 
+err:
+	if (!sgx_enter_enclave_sym)
+		encl_delete(&self->encl);
 
-	/* Invoke the vDSO directly. */
-	result = 0;
-	ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
-				     0, EENTER, 0, 0, &run);
-	if (!report_results(&run, ret, result, "sgx_enter_enclave"))
-		goto err;
+	ASSERT_NE(sgx_enter_enclave_sym, NULL);
+}
 
-	/* And with an exit handler. */
-	run.user_handler = (__u64)user_handler;
-	run.user_data = 0xdeadbeef;
-	ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result,
-				     0, EENTER, 0, 0, &run);
-	if (!report_results(&run, ret, result, "user_handler"))
-		goto err;
+FIXTURE_TEARDOWN(enclave)
+{
+	encl_delete(&self->encl);
+}
 
-	printf("SUCCESS\n");
-	encl_delete(&encl);
-	exit(KSFT_PASS);
+static inline int encl_call(const void *in, void *out, struct sgx_enclave_run *run, bool clobbered)
+{
+	void *in2 = (void *)in;
+	int ret;
 
-err:
-	encl_delete(&encl);
-	exit(KSFT_FAIL);
+	if (clobbered)
+		ret = vdso_sgx_enter_enclave((unsigned long)in2, (unsigned long)out, 0,
+					     EENTER, 0, 0, run);
+	else
+		ret = sgx_enter_enclave(in2, out, 0, EENTER, NULL, NULL, run);
+
+	return ret;
+}
+
+TEST_F(enclave, unclobbered_vdso)
+{
+	uint64_t result = 0;
+
+	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, false), 0);
+
+	EXPECT_EQ(result, MAGIC);
+	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EQ(self->run.user_data, 0);
+}
+
+TEST_F(enclave, clobbered_vdso)
+{
+	uint64_t result = 0;
+
+	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, true), 0);
+
+	EXPECT_EQ(result, MAGIC);
+	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EQ(self->run.user_data, 0);
+}
+
+static int test_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
+			struct sgx_enclave_run *run)
+{
+	run->user_data = 0;
+
+	return 0;
 }
+
+TEST_F(enclave, clobbered_vdso_and_user_function)
+{
+	uint64_t result = 0;
+
+	self->run.user_handler = (__u64)test_handler;
+	self->run.user_data = 0xdeadbeef;
+
+	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, true), 0);
+
+	EXPECT_EQ(result, MAGIC);
+	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EQ(self->run.user_data, 0);
+}
+
+TEST_HARNESS_MAIN
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v7 3/4] selftests/sgx: Dump enclave memory map
  2021-05-26 12:47 [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
  2021-05-26 12:47 ` [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
@ 2021-05-26 12:47 ` Jarkko Sakkinen
  2021-05-26 12:47 ` [PATCH v7 4/4] selftests/sgx: Add EXPECT_EEXIT() macro Jarkko Sakkinen
  2021-06-07 16:28 ` [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Reinette Chatre
  3 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-05-26 12:47 UTC (permalink / raw)
  To: shuah
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
	Dave Hansen, linux-kernel

Often, it's useful to check whether /proc/self/maps looks sane when
dealing with memory mapped objects, especially when they are JIT'ish
dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
matching lines from the memory map in FIXTURE_SETUP().

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 8694772c8fd9..d658882b5dd2 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
 	struct encl_segment *seg;
+	char maps_line[256];
+	FILE *maps_file;
 	unsigned int i;
 	void *addr;
 
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
 
+	maps_file = fopen("/proc/self/maps", "r");
+	if (maps_file != NULL)  {
+		while (fgets(maps_line, sizeof(maps_line), maps_file) != NULL) {
+			maps_line[strlen(maps_line) - 1] = '\0';
+
+			if (strstr(maps_line, "/dev/sgx_enclave"))
+				TH_LOG("%s", maps_line);
+		}
+
+		fclose(maps_file);
+	}
+
 err:
 	if (!sgx_enter_enclave_sym)
 		encl_delete(&self->encl);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v7 4/4] selftests/sgx: Add EXPECT_EEXIT() macro
  2021-05-26 12:47 [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
  2021-05-26 12:47 ` [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
  2021-05-26 12:47 ` [PATCH v7 3/4] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
@ 2021-05-26 12:47 ` Jarkko Sakkinen
  2021-06-07 16:28 ` [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Reinette Chatre
  3 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-05-26 12:47 UTC (permalink / raw)
  To: shuah
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Jarkko Sakkinen,
	Dave Hansen, linux-kernel

Add EXPECT_EEXIT() macro, which will conditionally print the exception
information, in addition to

  EXPECT_EQ(self->run.function, EEXIT);

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/main.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index d658882b5dd2..86d4283f9c61 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -207,6 +207,14 @@ static inline int encl_call(const void *in, void *out, struct sgx_enclave_run *r
 	return ret;
 }
 
+#define EXPECT_EEXIT(run) \
+	do { \
+		EXPECT_EQ((run)->function, EEXIT); \
+		if ((run)->function != EEXIT) \
+			TH_LOG("0x%02x 0x%02x 0x%16llx", (run)->exception_vector, \
+			       (run)->exception_error_code, (run)->exception_addr); \
+	} while (0)
+
 TEST_F(enclave, unclobbered_vdso)
 {
 	uint64_t result = 0;
@@ -214,7 +222,7 @@ TEST_F(enclave, unclobbered_vdso)
 	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, false), 0);
 
 	EXPECT_EQ(result, MAGIC);
-	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EEXIT(&self->run);
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
@@ -225,7 +233,7 @@ TEST_F(enclave, clobbered_vdso)
 	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, true), 0);
 
 	EXPECT_EQ(result, MAGIC);
-	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EEXIT(&self->run);
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
@@ -247,7 +255,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function)
 	EXPECT_EQ(encl_call(&MAGIC, &result, &self->run, true), 0);
 
 	EXPECT_EQ(result, MAGIC);
-	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EEXIT(&self->run);
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
  2021-05-26 12:47 [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
                   ` (2 preceding siblings ...)
  2021-05-26 12:47 ` [PATCH v7 4/4] selftests/sgx: Add EXPECT_EEXIT() macro Jarkko Sakkinen
@ 2021-06-07 16:28 ` Reinette Chatre
  2021-06-09 13:00   ` Jarkko Sakkinen
  3 siblings, 1 reply; 9+ messages in thread
From: Reinette Chatre @ 2021-06-07 16:28 UTC (permalink / raw)
  To: Jarkko Sakkinen, shuah
  Cc: linux-kselftest, linux-sgx, Dave Hansen, linux-kernel

Hi Jarkko,

On 5/26/2021 5:47 AM, Jarkko Sakkinen wrote:
> diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
> index 67211a708f04..68672fd86cf9 100644
> --- a/tools/testing/selftests/sgx/main.h
> +++ b/tools/testing/selftests/sgx/main.h
> @@ -35,7 +35,7 @@ bool encl_load(const char *path, struct encl *encl);
>   bool encl_measure(struct encl *encl);
>   bool encl_build(struct encl *encl);
>   
> -int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
> -		  struct sgx_enclave_run *run);
> +int sgx_enter_enclave(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
> +		      struct sgx_enclave_run *run);
>   

Is there a reason why all registers except rdx are "void *"?

Reinette

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness
  2021-05-26 12:47 ` [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
@ 2021-06-07 16:35   ` Reinette Chatre
  2021-06-09 13:02     ` Jarkko Sakkinen
  0 siblings, 1 reply; 9+ messages in thread
From: Reinette Chatre @ 2021-06-07 16:35 UTC (permalink / raw)
  To: Jarkko Sakkinen, shuah
  Cc: linux-kselftest, linux-sgx, Dave Hansen, linux-kernel

Hi Jarkko,

On 5/26/2021 5:47 AM, Jarkko Sakkinen wrote:

...

> -	exit(KSFT_PASS);
> +static inline int encl_call(const void *in, void *out, struct sgx_enclave_run *run, bool clobbered)
> +{
> +	void *in2 = (void *)in;
> +	int ret;
>   
> -err:
> -	encl_delete(&encl);
> -	exit(KSFT_FAIL);
> +	if (clobbered)
> +		ret = vdso_sgx_enter_enclave((unsigned long)in2, (unsigned long)out, 0,
> +					     EENTER, 0, 0, run);
> +	else
> +		ret = sgx_enter_enclave(in2, out, 0, EENTER, NULL, NULL, run);
> +
> +	return ret;
> +}

I find this change unnecessary because it is very specific to the 
current test cases and limiting future test cases. From what I 
understand it attempts to create a generic "call into the enclave" 
wrapper but in doing so it constrains the call to use only two 
registers, assuming there will always and only be just an "in" and "out" 
parameter needed.

Reinette

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
  2021-06-07 16:28 ` [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Reinette Chatre
@ 2021-06-09 13:00   ` Jarkko Sakkinen
  2021-06-09 21:02     ` Reinette Chatre
  0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-06-09 13:00 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: shuah, linux-kselftest, linux-sgx, Dave Hansen, linux-kernel

On Mon, Jun 07, 2021 at 09:28:56AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 5/26/2021 5:47 AM, Jarkko Sakkinen wrote:
> > diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
> > index 67211a708f04..68672fd86cf9 100644
> > --- a/tools/testing/selftests/sgx/main.h
> > +++ b/tools/testing/selftests/sgx/main.h
> > @@ -35,7 +35,7 @@ bool encl_load(const char *path, struct encl *encl);
> >   bool encl_measure(struct encl *encl);
> >   bool encl_build(struct encl *encl);
> > -int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
> > -		  struct sgx_enclave_run *run);
> > +int sgx_enter_enclave(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
> > +		      struct sgx_enclave_run *run);
> 
> Is there a reason why all registers except rdx are "void *"?

Evolution? It's test code.

There's neither reason to change this for no reason.

/Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness
  2021-06-07 16:35   ` Reinette Chatre
@ 2021-06-09 13:02     ` Jarkko Sakkinen
  0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-06-09 13:02 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: shuah, linux-kselftest, linux-sgx, Dave Hansen, linux-kernel

On Mon, Jun 07, 2021 at 09:35:25AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 5/26/2021 5:47 AM, Jarkko Sakkinen wrote:
> 
> ...
> 
> > -	exit(KSFT_PASS);
> > +static inline int encl_call(const void *in, void *out, struct sgx_enclave_run *run, bool clobbered)
> > +{
> > +	void *in2 = (void *)in;
> > +	int ret;
> > -err:
> > -	encl_delete(&encl);
> > -	exit(KSFT_FAIL);
> > +	if (clobbered)
> > +		ret = vdso_sgx_enter_enclave((unsigned long)in2, (unsigned long)out, 0,
> > +					     EENTER, 0, 0, run);
> > +	else
> > +		ret = sgx_enter_enclave(in2, out, 0, EENTER, NULL, NULL, run);
> > +
> > +	return ret;
> > +}
> 
> I find this change unnecessary because it is very specific to the current
> test cases and limiting future test cases. From what I understand it
> attempts to create a generic "call into the enclave" wrapper but in doing so
> it constrains the call to use only two registers, assuming there will always
> and only be just an "in" and "out" parameter needed.
> 
> Reinette

Thank you, good catch, it should be a macro. I'll turn in such.

Other than that, it does not limit anything as we are talking neither API
or ABI.

/Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'
  2021-06-09 13:00   ` Jarkko Sakkinen
@ 2021-06-09 21:02     ` Reinette Chatre
  0 siblings, 0 replies; 9+ messages in thread
From: Reinette Chatre @ 2021-06-09 21:02 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: shuah, linux-kselftest, linux-sgx, Dave Hansen, linux-kernel

Hi Jarkko,

On 6/9/2021 6:00 AM, Jarkko Sakkinen wrote:
> On Mon, Jun 07, 2021 at 09:28:56AM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 5/26/2021 5:47 AM, Jarkko Sakkinen wrote:
>>> diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
>>> index 67211a708f04..68672fd86cf9 100644
>>> --- a/tools/testing/selftests/sgx/main.h
>>> +++ b/tools/testing/selftests/sgx/main.h
>>> @@ -35,7 +35,7 @@ bool encl_load(const char *path, struct encl *encl);
>>>    bool encl_measure(struct encl *encl);
>>>    bool encl_build(struct encl *encl);
>>> -int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
>>> -		  struct sgx_enclave_run *run);
>>> +int sgx_enter_enclave(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9,
>>> +		      struct sgx_enclave_run *run);
>>
>> Is there a reason why all registers except rdx are "void *"?
> 
> Evolution? It's test code.
> 
> There's neither reason to change this for no reason.
> 

One reason would be to make the code consistent. This would reduce 
confusion.

Reinette

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-09 21:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 12:47 [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Jarkko Sakkinen
2021-05-26 12:47 ` [PATCH v7 2/4] selftests/sgx: Migrate to kselftest harness Jarkko Sakkinen
2021-06-07 16:35   ` Reinette Chatre
2021-06-09 13:02     ` Jarkko Sakkinen
2021-05-26 12:47 ` [PATCH v7 3/4] selftests/sgx: Dump enclave memory map Jarkko Sakkinen
2021-05-26 12:47 ` [PATCH v7 4/4] selftests/sgx: Add EXPECT_EEXIT() macro Jarkko Sakkinen
2021-06-07 16:28 ` [PATCH v7 1/4] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' Reinette Chatre
2021-06-09 13:00   ` Jarkko Sakkinen
2021-06-09 21:02     ` Reinette Chatre

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).