[PATCH v5 1/2] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'

[PATCH v5 1/2] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso'

[Jarkko Sakkinen]

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

[PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Jarkko Sakkinen]

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.

This gives a leaps better reporting than before. Here's an example
transcript:

TAP version 13
1..3

ok 1 enclave.unclobbered_vdso

ok 2 enclave.clobbered_vdso

ok 3 enclave.clobbered_vdso_and_user_function

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

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 | 170 ++++++++++++++---------------
 2 files changed, 85 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..78b2c8b27e07 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\n", 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,68 @@ 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);
+	vdso_sgx_enter_enclave = NULL;
+}
 
-	printf("SUCCESS\n");
-	encl_delete(&encl);
-	exit(KSFT_PASS);
 
-err:
-	encl_delete(&encl);
-	exit(KSFT_FAIL);
+TEST_F(enclave, unclobbered_vdso)
+{
+	uint64_t result = 0;
+
+	EXPECT_EQ(sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &self->run), 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(vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, 0,
+					 EENTER, 0, 0, &self->run), 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(vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, 0,
+					 EENTER, 0, 0, &self->run), 0);
+
+	EXPECT_EQ(result, MAGIC);
+	EXPECT_EQ(self->run.function, EEXIT);
+	EXPECT_EQ(self->run.user_data, 0);
+}
+
+TEST_HARNESS_MAIN
-- 
2.31.1

Re: [PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Reinette Chatre]

Hi Jarkko,

On 5/12/2021 2:53 PM, Jarkko Sakkinen wrote:
> 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.
> 
> This gives a leaps better reporting than before. Here's an example
> transcript:
> 
> TAP version 13
> 1..3
> 
> ok 1 enclave.unclobbered_vdso
> 
> ok 2 enclave.clobbered_vdso
> 
> ok 3 enclave.clobbered_vdso_and_user_function
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> 
> v5:
> * Use TH_LOG() for printing enclave address ranges instead of printf(),
>    based on Reinette's remark.

Thank you for considering my feedback. The motivation for my comment was 
to consider how this test output will be parsed. If these tests will 
have their output parsed by automated systems then it needs to conform 
to the TAP13 format as supported by kselftest.

In your latest version the output printed during a successful test has 
been changed, using TH_LOG() as you noted. From what I can tell this is 
the only output addressed - failing tests continue to print error 
messages (perror, fprintf) without consideration of how they will be 
parsed. My apologies, I am not a kselftest expert to know what the best 
way for this integration is.

Reinette

Re: [PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Jarkko Sakkinen]

On Mon, May 17, 2021 at 10:03:42AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 5/12/2021 2:53 PM, Jarkko Sakkinen wrote:
> > 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.
> > 
> > This gives a leaps better reporting than before. Here's an example
> > transcript:
> > 
> > TAP version 13
> > 1..3
> > 
> > ok 1 enclave.unclobbered_vdso
> > 
> > ok 2 enclave.clobbered_vdso
> > 
> > ok 3 enclave.clobbered_vdso_and_user_function
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > 
> > v5:
> > * Use TH_LOG() for printing enclave address ranges instead of printf(),
> >    based on Reinette's remark.
> 
> Thank you for considering my feedback. The motivation for my comment was to
> consider how this test output will be parsed. If these tests will have their
> output parsed by automated systems then it needs to conform to the TAP13
> format as supported by kselftest.
> 
> In your latest version the output printed during a successful test has been
> changed, using TH_LOG() as you noted. From what I can tell this is the only
> output addressed - failing tests continue to print error messages (perror,
> fprintf) without consideration of how they will be parsed. My apologies, I
> am not a kselftest expert to know what the best way for this integration is.
> 
> Reinette

It's a valid question, yes.

The problem is that only main.c can use kselftest macros because
kselftest_harness.h pulls 

static int test_harness_run(int __attribute__((unused)) argc,
			    char __attribute__((unused)) **argv)

which will not end up having a call site (because there's no
"TEST_HARNESS_MAIN").

The whole logging thing in kselftest harness is a bit ambiguous.
Namely:

1. There's a macro TH_LOG() defined in kselftest_harness.h, which
   "internally" uses fprintf().
2. There's an inline function ksft_print_msg() in kselftest.h
   using vsprintf().

To add to that, kselftest_harness.h internally prints by using
ksft_print_msg(), and provides TH_LOG(), which does not use
ksft_print_msg().

I don't really get the logic in all this.

/Jarkko

Re: [PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Jarkko Sakkinen]

On Tue, May 18, 2021 at 08:49:00PM +0300, Jarkko Sakkinen wrote:
> On Mon, May 17, 2021 at 10:03:42AM -0700, Reinette Chatre wrote:
> > Hi Jarkko,
> > 
> > On 5/12/2021 2:53 PM, Jarkko Sakkinen wrote:
> > > 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.
> > > 
> > > This gives a leaps better reporting than before. Here's an example
> > > transcript:
> > > 
> > > TAP version 13
> > > 1..3
> > > 
> > > ok 1 enclave.unclobbered_vdso
> > > 
> > > ok 2 enclave.clobbered_vdso
> > > 
> > > ok 3 enclave.clobbered_vdso_and_user_function
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > 
> > > v5:
> > > * Use TH_LOG() for printing enclave address ranges instead of printf(),
> > >    based on Reinette's remark.
> > 
> > Thank you for considering my feedback. The motivation for my comment was to
> > consider how this test output will be parsed. If these tests will have their
> > output parsed by automated systems then it needs to conform to the TAP13
> > format as supported by kselftest.
> > 
> > In your latest version the output printed during a successful test has been
> > changed, using TH_LOG() as you noted. From what I can tell this is the only
> > output addressed - failing tests continue to print error messages (perror,
> > fprintf) without consideration of how they will be parsed. My apologies, I
> > am not a kselftest expert to know what the best way for this integration is.
> > 
> > Reinette
> 
> It's a valid question, yes.
> 
> The problem is that only main.c can use kselftest macros because
> kselftest_harness.h pulls 
> 
> static int test_harness_run(int __attribute__((unused)) argc,
> 			    char __attribute__((unused)) **argv)
> 
> which will not end up having a call site (because there's no
> "TEST_HARNESS_MAIN").
> 
> The whole logging thing in kselftest harness is a bit ambiguous.
> Namely:
> 
> 1. There's a macro TH_LOG() defined in kselftest_harness.h, which
>    "internally" uses fprintf().
> 2. There's an inline function ksft_print_msg() in kselftest.h
>    using vsprintf().
> 
> To add to that, kselftest_harness.h internally prints by using
> ksft_print_msg(), and provides TH_LOG(), which does not use
> ksft_print_msg().
> 
> I don't really get the logic in all this.

I tried to split TH_LOG() as separate entity but it's not possible, as the
macros access a static variable called '_metadata'.

I'm not exactly sure how to proceed from this, if we want to make logging
consistent.

I would personally suggest to leave the error messages intact in load.c,
because there is no way to make them consistent, except by removing them.

/Jarkko

Re: [PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Reinette Chatre]

Hi Jarkko,

On 5/18/2021 12:57 PM, Jarkko Sakkinen wrote:
> On Tue, May 18, 2021 at 08:49:00PM +0300, Jarkko Sakkinen wrote:
>> On Mon, May 17, 2021 at 10:03:42AM -0700, Reinette Chatre wrote:
>>> Hi Jarkko,
>>>
>>> On 5/12/2021 2:53 PM, Jarkko Sakkinen wrote:
>>>> 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.
>>>>
>>>> This gives a leaps better reporting than before. Here's an example
>>>> transcript:
>>>>
>>>> TAP version 13
>>>> 1..3
>>>>
>>>> ok 1 enclave.unclobbered_vdso
>>>>
>>>> ok 2 enclave.clobbered_vdso
>>>>
>>>> ok 3 enclave.clobbered_vdso_and_user_function
>>>>
>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>> ---
>>>>
>>>> v5:
>>>> * Use TH_LOG() for printing enclave address ranges instead of printf(),
>>>>     based on Reinette's remark.
>>>
>>> Thank you for considering my feedback. The motivation for my comment was to
>>> consider how this test output will be parsed. If these tests will have their
>>> output parsed by automated systems then it needs to conform to the TAP13
>>> format as supported by kselftest.
>>>
>>> In your latest version the output printed during a successful test has been
>>> changed, using TH_LOG() as you noted. From what I can tell this is the only
>>> output addressed - failing tests continue to print error messages (perror,
>>> fprintf) without consideration of how they will be parsed. My apologies, I
>>> am not a kselftest expert to know what the best way for this integration is.
>>>
>>> Reinette
>>
>> It's a valid question, yes.
>>
>> The problem is that only main.c can use kselftest macros because
>> kselftest_harness.h pulls
>>
>> static int test_harness_run(int __attribute__((unused)) argc,
>> 			    char __attribute__((unused)) **argv)
>>
>> which will not end up having a call site (because there's no
>> "TEST_HARNESS_MAIN").
>>
>> The whole logging thing in kselftest harness is a bit ambiguous.
>> Namely:
>>
>> 1. There's a macro TH_LOG() defined in kselftest_harness.h, which
>>     "internally" uses fprintf().
>> 2. There's an inline function ksft_print_msg() in kselftest.h
>>     using vsprintf().
>>
>> To add to that, kselftest_harness.h internally prints by using
>> ksft_print_msg(), and provides TH_LOG(), which does not use
>> ksft_print_msg().
>>
>> I don't really get the logic in all this.
> 
> I tried to split TH_LOG() as separate entity but it's not possible, as the
> macros access a static variable called '_metadata'.
> 
> I'm not exactly sure how to proceed from this, if we want to make logging
> consistent.
> 
> I would personally suggest to leave the error messages intact in load.c,
> because there is no way to make them consistent, except by removing them.


It is not clear to me why ksft_print_msg() cannot be used but an 
alternative to it may be to just prefix all existing diagnostic messages 
with "# ".

Reinette

Re: [PATCH v5 2/2] selftests/sgx: Migrate to kselftest harness

[Jarkko Sakkinen]

On Tue, May 18, 2021 at 01:07:16PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 5/18/2021 12:57 PM, Jarkko Sakkinen wrote:
> > On Tue, May 18, 2021 at 08:49:00PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, May 17, 2021 at 10:03:42AM -0700, Reinette Chatre wrote:
> > > > Hi Jarkko,
> > > > 
> > > > On 5/12/2021 2:53 PM, Jarkko Sakkinen wrote:
> > > > > 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.
> > > > > 
> > > > > This gives a leaps better reporting than before. Here's an example
> > > > > transcript:
> > > > > 
> > > > > TAP version 13
> > > > > 1..3
> > > > > 
> > > > > ok 1 enclave.unclobbered_vdso
> > > > > 
> > > > > ok 2 enclave.clobbered_vdso
> > > > > 
> > > > > ok 3 enclave.clobbered_vdso_and_user_function
> > > > > 
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > ---
> > > > > 
> > > > > v5:
> > > > > * Use TH_LOG() for printing enclave address ranges instead of printf(),
> > > > >     based on Reinette's remark.
> > > > 
> > > > Thank you for considering my feedback. The motivation for my comment was to
> > > > consider how this test output will be parsed. If these tests will have their
> > > > output parsed by automated systems then it needs to conform to the TAP13
> > > > format as supported by kselftest.
> > > > 
> > > > In your latest version the output printed during a successful test has been
> > > > changed, using TH_LOG() as you noted. From what I can tell this is the only
> > > > output addressed - failing tests continue to print error messages (perror,
> > > > fprintf) without consideration of how they will be parsed. My apologies, I
> > > > am not a kselftest expert to know what the best way for this integration is.
> > > > 
> > > > Reinette
> > > 
> > > It's a valid question, yes.
> > > 
> > > The problem is that only main.c can use kselftest macros because
> > > kselftest_harness.h pulls
> > > 
> > > static int test_harness_run(int __attribute__((unused)) argc,
> > > 			    char __attribute__((unused)) **argv)
> > > 
> > > which will not end up having a call site (because there's no
> > > "TEST_HARNESS_MAIN").
> > > 
> > > The whole logging thing in kselftest harness is a bit ambiguous.
> > > Namely:
> > > 
> > > 1. There's a macro TH_LOG() defined in kselftest_harness.h, which
> > >     "internally" uses fprintf().
> > > 2. There's an inline function ksft_print_msg() in kselftest.h
> > >     using vsprintf().
> > > 
> > > To add to that, kselftest_harness.h internally prints by using
> > > ksft_print_msg(), and provides TH_LOG(), which does not use
> > > ksft_print_msg().
> > > 
> > > I don't really get the logic in all this.
> > 
> > I tried to split TH_LOG() as separate entity but it's not possible, as the
> > macros access a static variable called '_metadata'.
> > 
> > I'm not exactly sure how to proceed from this, if we want to make logging
> > consistent.
> > 
> > I would personally suggest to leave the error messages intact in load.c,
> > because there is no way to make them consistent, except by removing them.
> 
> 
> It is not clear to me why ksft_print_msg() cannot be used but an alternative
> to it may be to just prefix all existing diagnostic messages with "# ".
> 
> Reinette

How is using ksft_print_msg() better than using fprintf()? It's as
incompatible with the logging used by fixtures, as is a raw fprintf().

What is the gain of doing that?

/Jarkko