All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler
@ 2021-07-05 14:36 Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs Jarkko Sakkinen
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05 14:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Jarkko Sakkinen, Dave Hansen

Create a heap for the test enclave, which has the same size as all
available Enclave Page Cache (EPC) pages in the system. This will guarantee
that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
have been swapped out by the page reclaimer during the load time. Actually,
this adds a bit more stress than that since part of the EPC gets reserved
for the Version Array (VA) pages.

For each test, the page fault handler gets triggered in two occasions:

- When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
  page fault handler.
- During the execution, each page that is referenced gets swapped in
  by the page fault handler.

Jarkko Sakkinen (3):
  x86/sgx: Add sgx_nr_all_pages to the debugfs
  selftests/sgx: Assign source for each segment
  selftests/sgx: Trigger the reclaimer and #PF handler

Tianjia Zhang (1):
  selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c

 Documentation/x86/sgx.rst               |  9 +++++
 arch/x86/kernel/cpu/sgx/main.c          | 10 ++++-
 tools/testing/selftests/sgx/load.c      | 38 ++++++++++++++----
 tools/testing/selftests/sgx/main.c      | 42 +++++++++++++++++++-
 tools/testing/selftests/sgx/main.h      |  4 +-
 tools/testing/selftests/sgx/sigstruct.c | 53 +++++++++++++------------
 6 files changed, 120 insertions(+), 36 deletions(-)

-- 
2.32.0


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

* [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs
  2021-07-05 14:36 [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler Jarkko Sakkinen
@ 2021-07-05 14:36 ` Jarkko Sakkinen
  2021-07-06 14:56   ` Dave Hansen
  2021-07-05 14:36 ` [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05 14:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Jarkko Sakkinen, Dave Hansen, Thomas Gleixner, Ingo Molnar, x86,
	H. Peter Anvin, Jonathan Corbet, linux-kernel, linux-doc

Create /sys/kernel/debug/x86/sgx_nr_all_pages, which reports total
number of EPC pages available in the system.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 Documentation/x86/sgx.rst      |  9 +++++++++
 arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
index dd0ac96ff9ef..3a8fefdebee0 100644
--- a/Documentation/x86/sgx.rst
+++ b/Documentation/x86/sgx.rst
@@ -250,3 +250,12 @@ user wants to deploy SGX applications both on the host and in guests
 on the same machine, the user should reserve enough EPC (by taking out
 total virtual EPC size of all SGX VMs from the physical EPC size) for
 host SGX applications so they can run with acceptable performance.
+
+SGX debugging
+=============
+
+The total number of available EPC pages can observed by:
+
+  # mount -t debugfs debugfs /sys/kernel/debug
+  # cat /sys/kernel/debug/x86/sgx_nr_all_pages
+  <the number of all EPC pages available in the system>
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 63d3de02bbcc..43e4b6215e62 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*  Copyright(c) 2016-20 Intel Corporation. */
 
+#include <linux/debugfs.h>
 #include <linux/file.h>
 #include <linux/freezer.h>
 #include <linux/highmem.h>
@@ -28,7 +29,10 @@ static DECLARE_WAIT_QUEUE_HEAD(ksgxd_waitq);
 static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
-/* The free page list lock protected variables prepend the lock. */
+/* The number of EPC pages in total in all nodes. */
+static unsigned long sgx_nr_all_pages;
+
+/* The number of free EPC pages in all nodes. */
 static unsigned long sgx_nr_free_pages;
 
 /* Nodes with one or more EPC sections. */
@@ -656,6 +660,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 
+	sgx_nr_all_pages += nr_pages;
+
 	return true;
 }
 
@@ -823,6 +829,8 @@ static int __init sgx_init(void)
 	if (sgx_vepc_init() && ret)
 		goto err_provision;
 
+	debugfs_create_ulong("sgx_nr_all_pages", 0444, arch_debugfs_dir, &sgx_nr_all_pages);
+
 	return 0;
 
 err_provision:
-- 
2.32.0


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

* [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-05 14:36 [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs Jarkko Sakkinen
@ 2021-07-05 14:36 ` Jarkko Sakkinen
  2021-07-06 20:53   ` Dave Hansen
  2021-07-05 14:36 ` [PATCH 3/4] selftests/sgx: Assign source for each segment Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler Jarkko Sakkinen
  3 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05 14:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Tianjia Zhang, Jarkko Sakkinen, Dave Hansen, Jethro Beekman,
	Borislav Petkov, linux-kernel

From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>

Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated
length of Q1 and Q2 is less than 384 bytes, things will go wrong.

E.g. if Q2 is 383 bytes, then

1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2().
2. The entire sigstruct->q2 is reversed, which results it being
   256 * Q2, given that the last byte of sigstruct->q2 is added
   to before the bytes given by calc_q1q2().

Either change in key or measurement can trigger the bug. E.g. an unmeasured
heap could cause a devastating change in Q1 or Q2.

Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to
the caller.

Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
The original patch did a bad job explaining the code change but it
turned out making sense. I wrote a new description.

v3:
- Adjusted the fixes tag to refer to the correct commit

v2:
- Added a fixes tag.

 tools/testing/selftests/sgx/sigstruct.c | 41 +++++++++++++------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index dee7a3d6c5a5..92bbc5a15c39 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m,
 	return true;
 }
 
+static void reverse_bytes(void *data, int length)
+{
+	int i = 0;
+	int j = length - 1;
+	uint8_t temp;
+	uint8_t *ptr = data;
+
+	while (i < j) {
+		temp = ptr[i];
+		ptr[i] = ptr[j];
+		ptr[j] = temp;
+		i++;
+		j--;
+	}
+}
+
 static bool calc_q1q2(const uint8_t *s, const uint8_t *m, uint8_t *q1,
 		      uint8_t *q2)
 {
 	struct q1q2_ctx ctx;
+	int len;
 
 	if (!alloc_q1q2_ctx(s, m, &ctx)) {
 		fprintf(stderr, "Not enough memory for Q1Q2 calculation\n");
@@ -89,8 +106,10 @@ static bool calc_q1q2(const uint8_t *s, const uint8_t *m, uint8_t *q1,
 		goto out;
 	}
 
-	BN_bn2bin(ctx.q1, q1);
-	BN_bn2bin(ctx.q2, q2);
+	len = BN_bn2bin(ctx.q1, q1);
+	reverse_bytes(q1, len);
+	len = BN_bn2bin(ctx.q2, q2);
+	reverse_bytes(q2, len);
 
 	free_q1q2_ctx(&ctx);
 	return true;
@@ -152,22 +171,6 @@ static RSA *gen_sign_key(void)
 	return key;
 }
 
-static void reverse_bytes(void *data, int length)
-{
-	int i = 0;
-	int j = length - 1;
-	uint8_t temp;
-	uint8_t *ptr = data;
-
-	while (i < j) {
-		temp = ptr[i];
-		ptr[i] = ptr[j];
-		ptr[j] = temp;
-		i++;
-		j--;
-	}
-}
-
 enum mrtags {
 	MRECREATE = 0x0045544145524345,
 	MREADD = 0x0000000044444145,
@@ -367,8 +370,6 @@ bool encl_measure(struct encl *encl)
 	/* BE -> LE */
 	reverse_bytes(sigstruct->signature, SGX_MODULUS_SIZE);
 	reverse_bytes(sigstruct->modulus, SGX_MODULUS_SIZE);
-	reverse_bytes(sigstruct->q1, SGX_MODULUS_SIZE);
-	reverse_bytes(sigstruct->q2, SGX_MODULUS_SIZE);
 
 	EVP_MD_CTX_destroy(ctx);
 	RSA_free(key);
-- 
2.32.0


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

* [PATCH 3/4] selftests/sgx: Assign source for each segment
  2021-07-05 14:36 [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
@ 2021-07-05 14:36 ` Jarkko Sakkinen
  2021-07-05 14:36 ` [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler Jarkko Sakkinen
  3 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05 14:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Jarkko Sakkinen, Dave Hansen, linux-kernel

Define source per segment so that enclave pages can be added from different
sources, e.g. anonymous VMA for zero pages. In other words, add 'src' field
to struct encl_segment, and assign it to 'encl->src' for pages inherited
from the enclave binary.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/load.c      | 5 +++--
 tools/testing/selftests/sgx/main.h      | 1 +
 tools/testing/selftests/sgx/sigstruct.c | 8 ++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 00928be57fc4..9946fab2a3d6 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -107,7 +107,7 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
 	memset(&secinfo, 0, sizeof(secinfo));
 	secinfo.flags = seg->flags;
 
-	ioc.src = (uint64_t)encl->src + seg->offset;
+	ioc.src = (uint64_t)seg->src;
 	ioc.offset = seg->offset;
 	ioc.length = seg->size;
 	ioc.secinfo = (unsigned long)&secinfo;
@@ -226,6 +226,7 @@ bool encl_load(const char *path, struct encl *encl)
 
 		if (j == 0) {
 			src_offset = phdr->p_offset & PAGE_MASK;
+			encl->src = encl->bin + src_offset;
 
 			seg->prot = PROT_READ | PROT_WRITE;
 			seg->flags = SGX_PAGE_TYPE_TCS << 8;
@@ -238,13 +239,13 @@ 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;
+		seg->src = encl->src + seg->offset;
 
 		j++;
 	}
 
 	assert(j == encl->nr_segments);
 
-	encl->src = encl->bin + src_offset;
 	encl->src_size = encl->segment_tbl[j - 1].offset +
 			 encl->segment_tbl[j - 1].size;
 
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 68672fd86cf9..452d11dc4889 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -7,6 +7,7 @@
 #define MAIN_H
 
 struct encl_segment {
+	void *src;
 	off_t offset;
 	size_t size;
 	unsigned int prot;
diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index 92bbc5a15c39..202a96fd81bf 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -289,14 +289,14 @@ static bool mrenclave_eextend(EVP_MD_CTX *ctx, uint64_t offset,
 static bool mrenclave_segment(EVP_MD_CTX *ctx, struct encl *encl,
 			      struct encl_segment *seg)
 {
-	uint64_t end = seg->offset + seg->size;
+	uint64_t end = seg->size;
 	uint64_t offset;
 
-	for (offset = seg->offset; offset < end; offset += PAGE_SIZE) {
-		if (!mrenclave_eadd(ctx, offset, seg->flags))
+	for (offset = 0; offset < end; offset += PAGE_SIZE) {
+		if (!mrenclave_eadd(ctx, seg->offset + offset, seg->flags))
 			return false;
 
-		if (!mrenclave_eextend(ctx, offset, encl->src + offset))
+		if (!mrenclave_eextend(ctx, seg->offset + offset, seg->src + offset))
 			return false;
 	}
 
-- 
2.32.0


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

* [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-05 14:36 [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler Jarkko Sakkinen
                   ` (2 preceding siblings ...)
  2021-07-05 14:36 ` [PATCH 3/4] selftests/sgx: Assign source for each segment Jarkko Sakkinen
@ 2021-07-05 14:36 ` Jarkko Sakkinen
  2021-07-06 18:34   ` Reinette Chatre
  3 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05 14:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Jarkko Sakkinen, Dave Hansen, linux-kernel

Create a heap for the test enclave, which has the same size as all
available Enclave Page Cache (EPC) pages in the system. This will guarantee
that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
have been swapped out by the page reclaimer during the load time. Actually,
this adds a bit more stress than that since part of the EPC gets reserved
for the Version Array (VA) pages.

For each test, the page fault handler gets triggered in two occasions:

- When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
  page fault handler.
- During the execution, each page that is referenced gets swapped in
  by the page fault handler.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/load.c      | 33 +++++++++++++++----
 tools/testing/selftests/sgx/main.c      | 42 ++++++++++++++++++++++++-
 tools/testing/selftests/sgx/main.h      |  3 +-
 tools/testing/selftests/sgx/sigstruct.c |  6 ++--
 4 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 9946fab2a3d6..a312a132ac33 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -21,6 +21,8 @@
 
 void encl_delete(struct encl *encl)
 {
+	struct encl_segment *heap_seg = &encl->segment_tbl[encl->nr_segments - 1];
+
 	if (encl->encl_base)
 		munmap((void *)encl->encl_base, encl->encl_size);
 
@@ -30,6 +32,8 @@ void encl_delete(struct encl *encl)
 	if (encl->fd)
 		close(encl->fd);
 
+	munmap(heap_seg->src, heap_seg->size);
+
 	if (encl->segment_tbl)
 		free(encl->segment_tbl);
 
@@ -111,7 +115,10 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
 	ioc.offset = seg->offset;
 	ioc.length = seg->size;
 	ioc.secinfo = (unsigned long)&secinfo;
-	ioc.flags = SGX_PAGE_MEASURE;
+	if (seg->measure)
+		ioc.flags = SGX_PAGE_MEASURE;
+	else
+		ioc.flags = 0;
 
 	rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
 	if (rc < 0) {
@@ -124,9 +131,10 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
 
 
 
-bool encl_load(const char *path, struct encl *encl)
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
 {
 	const char device_path[] = "/dev/sgx_enclave";
+	struct encl_segment *seg;
 	Elf64_Phdr *phdr_tbl;
 	off_t src_offset;
 	Elf64_Ehdr *ehdr;
@@ -188,6 +196,8 @@ bool encl_load(const char *path, struct encl *encl)
 	ehdr = encl->bin;
 	phdr_tbl = encl->bin + ehdr->e_phoff;
 
+	encl->nr_segments = 1; /* one for the heap */
+
 	for (i = 0; i < ehdr->e_phnum; i++) {
 		Elf64_Phdr *phdr = &phdr_tbl[i];
 
@@ -203,7 +213,6 @@ bool encl_load(const char *path, struct encl *encl)
 	for (i = 0, j = 0; i < ehdr->e_phnum; i++) {
 		Elf64_Phdr *phdr = &phdr_tbl[i];
 		unsigned int flags = phdr->p_flags;
-		struct encl_segment *seg;
 
 		if (phdr->p_type != PT_LOAD)
 			continue;
@@ -240,14 +249,26 @@ 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;
 		seg->src = encl->src + seg->offset;
+		seg->measure = true;
 
 		j++;
 	}
 
-	assert(j == encl->nr_segments);
+	assert(j == encl->nr_segments - 1);
+
+	seg = &encl->segment_tbl[j];
+	seg->offset =  encl->segment_tbl[j - 1].offset + encl->segment_tbl[j - 1].size;
+	seg->size = heap_size;
+	seg->src = mmap(NULL, heap_size, PROT_READ | PROT_WRITE,
+			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+	seg->prot = PROT_READ | PROT_WRITE;
+	seg->flags = (SGX_PAGE_TYPE_REG << 8) | seg->prot;
+	seg->measure = false;
+
+	if (seg->src == MAP_FAILED)
+		goto err;
 
-	encl->src_size = encl->segment_tbl[j - 1].offset +
-			 encl->segment_tbl[j - 1].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; )
 		encl->encl_size <<= 1;
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index e252015e0c15..772ba1d72619 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -112,17 +112,57 @@ FIXTURE(enclave) {
 	struct sgx_enclave_run run;
 };
 
+#define SGX_NR_ALL_PAGES_PATH "/sys/kernel/debug/x86/sgx_nr_all_pages"
+
+static int sysfs_get_ulong(const char *path, unsigned long *value)
+{
+	struct stat sbuf;
+	ssize_t ret = 0;
+	char buf[128];
+	int fd;
+
+	ret = stat(path, &sbuf);
+	if (ret)
+		return ret;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	ret = read(fd, buf, sizeof(buf));
+	if (ret < 0)
+		goto out;
+
+	/* Clear the read bytes count. */
+	ret = 0;
+
+	errno = 0;
+	*value = strtoul(buf, NULL, 0);
+	if (errno)
+		ret = -1;
+
+out:
+	close(fd);
+	return ret;
+}
+
 FIXTURE_SETUP(enclave)
 {
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
+	unsigned long nr_all_pages;
 	struct vdso_symtab symtab;
 	struct encl_segment *seg;
 	char maps_line[256];
 	FILE *maps_file;
 	unsigned int i;
 	void *addr;
+	int ret;
+
+	ret = sysfs_get_ulong(SGX_NR_ALL_PAGES_PATH, &nr_all_pages);
+	if (ret)
+		ksft_exit_skip("Failed to read " SGX_NR_ALL_PAGES_PATH "\n");
 
-	if (!encl_load("test_encl.elf", &self->encl)) {
+	if (!encl_load("test_encl.elf", &self->encl, nr_all_pages * 4096)) {
 		encl_delete(&self->encl);
 		ksft_exit_skip("cannot load enclaves\n");
 	}
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 452d11dc4889..a286861dc289 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -12,6 +12,7 @@ struct encl_segment {
 	size_t size;
 	unsigned int prot;
 	unsigned int flags;
+	bool measure;
 };
 
 struct encl {
@@ -32,7 +33,7 @@ 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);
+bool encl_load(const char *path, struct encl *encl, unsigned long heap_size);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 
diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index 202a96fd81bf..50c5ab1aa6fa 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -296,8 +296,10 @@ static bool mrenclave_segment(EVP_MD_CTX *ctx, struct encl *encl,
 		if (!mrenclave_eadd(ctx, seg->offset + offset, seg->flags))
 			return false;
 
-		if (!mrenclave_eextend(ctx, seg->offset + offset, seg->src + offset))
-			return false;
+		if (seg->measure) {
+			if (!mrenclave_eextend(ctx, seg->offset + offset, seg->src + offset))
+				return false;
+		}
 	}
 
 	return true;
-- 
2.32.0


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

* Re: [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs
  2021-07-05 14:36 ` [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs Jarkko Sakkinen
@ 2021-07-06 14:56   ` Dave Hansen
  2021-07-06 15:39     ` Greg KH
  2021-07-06 22:08     ` Jarkko Sakkinen
  0 siblings, 2 replies; 20+ messages in thread
From: Dave Hansen @ 2021-07-06 14:56 UTC (permalink / raw)
  To: Jarkko Sakkinen, Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Dave Hansen, Thomas Gleixner, Ingo Molnar, x86, H. Peter Anvin,
	Jonathan Corbet, linux-kernel, linux-doc

On 7/5/21 7:36 AM, Jarkko Sakkinen wrote:
> Create /sys/kernel/debug/x86/sgx_nr_all_pages, which reports total
> number of EPC pages available in the system.
Could we flesh this out a bit, please?

What's the value here when userspace could just re-enumerate the EPC
size from CPUID?

I'd really appreciate if we could draw parallels between these additions
to the "SGX VM" and their analogs in the "core VM".  In this case, I
think the closest analog is probably "MemTotal" in /proc/meminfo.

Second, how is this going to be used?

Third, is this going to be the ABI forever?

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

* Re: [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs
  2021-07-06 14:56   ` Dave Hansen
@ 2021-07-06 15:39     ` Greg KH
  2021-07-06 22:08     ` Jarkko Sakkinen
  1 sibling, 0 replies; 20+ messages in thread
From: Greg KH @ 2021-07-06 15:39 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Jarkko Sakkinen, Shuah Khan, linux-kselftest, linux-sgx,
	Reinette Chatre, Borislav Petkov, Dave Hansen, Thomas Gleixner,
	Ingo Molnar, x86, H. Peter Anvin, Jonathan Corbet, linux-kernel,
	linux-doc

On Tue, Jul 06, 2021 at 07:56:40AM -0700, Dave Hansen wrote:
> On 7/5/21 7:36 AM, Jarkko Sakkinen wrote:
> > Create /sys/kernel/debug/x86/sgx_nr_all_pages, which reports total
> > number of EPC pages available in the system.
> Could we flesh this out a bit, please?
> 
> What's the value here when userspace could just re-enumerate the EPC
> size from CPUID?
> 
> I'd really appreciate if we could draw parallels between these additions
> to the "SGX VM" and their analogs in the "core VM".  In this case, I
> think the closest analog is probably "MemTotal" in /proc/meminfo.
> 
> Second, how is this going to be used?
> 
> Third, is this going to be the ABI forever?

debugfs is never a stable abi.  If it is being used as such, then the
kernel code is wrong.  This better just be debugging stuff only.

thanks,

greg k-h

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-05 14:36 ` [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler Jarkko Sakkinen
@ 2021-07-06 18:34   ` Reinette Chatre
  2021-07-06 23:50     ` Jarkko Sakkinen
  0 siblings, 1 reply; 20+ messages in thread
From: Reinette Chatre @ 2021-07-06 18:34 UTC (permalink / raw)
  To: Jarkko Sakkinen, Shuah Khan
  Cc: linux-kselftest, linux-sgx, Borislav Petkov, Dave Hansen, linux-kernel

Hi Jarkko,

On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
> Create a heap for the test enclave, which has the same size as all
> available Enclave Page Cache (EPC) pages in the system. This will guarantee
> that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
> have been swapped out by the page reclaimer during the load time. Actually,
> this adds a bit more stress than that since part of the EPC gets reserved
> for the Version Array (VA) pages.
> 
> For each test, the page fault handler gets triggered in two occasions:
> 
> - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
>    page fault handler.
> - During the execution, each page that is referenced gets swapped in
>    by the page fault handler.
> 

If I understand this correctly, all EPC pages are now being consumed 
during fixture setup and thus every SGX test, no matter how big or 
small, now becomes a stress test of the reclaimer instead of there being 
a unique reclaimer test. Since an enclave is set up and torn down for 
every test this seems like a significant addition. It also seems like 
this would impact future tests of dynamic page addition where not all 
scenarios could be tested with all EPC pages already consumed.

Reinette

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

* Re: [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-05 14:36 ` [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
@ 2021-07-06 20:53   ` Dave Hansen
  2021-07-06 23:52     ` Jarkko Sakkinen
  0 siblings, 1 reply; 20+ messages in thread
From: Dave Hansen @ 2021-07-06 20:53 UTC (permalink / raw)
  To: Jarkko Sakkinen, Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Tianjia Zhang, Dave Hansen, Jethro Beekman, Borislav Petkov,
	linux-kernel

On 7/5/21 7:36 AM, Jarkko Sakkinen wrote:
> From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> 
> Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated
> length of Q1 and Q2 is less than 384 bytes, things will go wrong.
> 
> E.g. if Q2 is 383 bytes, then
> 
> 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2().
> 2. The entire sigstruct->q2 is reversed, which results it being
>    256 * Q2, given that the last byte of sigstruct->q2 is added
>    to before the bytes given by calc_q1q2().
> 
> Either change in key or measurement can trigger the bug. E.g. an unmeasured
> heap could cause a devastating change in Q1 or Q2.
> 
> Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to
> the caller.
> 
> Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
> Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

This looks fine, but can I suggest a Subject: tweak?

	selftests/sgx: Fix calculations for sub-maximum field sizes

In any case:

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>

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

* Re: [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs
  2021-07-06 14:56   ` Dave Hansen
  2021-07-06 15:39     ` Greg KH
@ 2021-07-06 22:08     ` Jarkko Sakkinen
  1 sibling, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-06 22:08 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Reinette Chatre,
	Borislav Petkov, Dave Hansen, Thomas Gleixner, Ingo Molnar, x86,
	H. Peter Anvin, Jonathan Corbet, linux-kernel, linux-doc

On Tue, Jul 06, 2021 at 07:56:40AM -0700, Dave Hansen wrote:
> On 7/5/21 7:36 AM, Jarkko Sakkinen wrote:
> > Create /sys/kernel/debug/x86/sgx_nr_all_pages, which reports total
> > number of EPC pages available in the system.
> Could we flesh this out a bit, please?
> 
> What's the value here when userspace could just re-enumerate the EPC
> size from CPUID?

My thinking is that it is better to use "kernel synthesized" value for the
EPC size, because kernel controls the EPC.

> I'd really appreciate if we could draw parallels between these additions
> to the "SGX VM" and their analogs in the "core VM".  In this case, I
> think the closest analog is probably "MemTotal" in /proc/meminfo.

Would make sense.

> Second, how is this going to be used?

SGX kselftest creates a heap, of which size is the same as the total size
of the EPC reported by the kernel.

> Third, is this going to be the ABI forever?

AFAIK, debugfs is not part of the ABI.

/Jarkko

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-06 18:34   ` Reinette Chatre
@ 2021-07-06 23:50     ` Jarkko Sakkinen
  2021-07-07  0:10       ` Reinette Chatre
  0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-06 23:50 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
> > Create a heap for the test enclave, which has the same size as all
> > available Enclave Page Cache (EPC) pages in the system. This will guarantee
> > that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
> > have been swapped out by the page reclaimer during the load time. Actually,
> > this adds a bit more stress than that since part of the EPC gets reserved
> > for the Version Array (VA) pages.
> > 
> > For each test, the page fault handler gets triggered in two occasions:
> > 
> > - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
> >    page fault handler.
> > - During the execution, each page that is referenced gets swapped in
> >    by the page fault handler.
> > 
> 
> If I understand this correctly, all EPC pages are now being consumed during
> fixture setup and thus every SGX test, no matter how big or small, now
> becomes a stress test of the reclaimer instead of there being a unique
> reclaimer test. Since an enclave is set up and torn down for every test this
> seems like a significant addition. It also seems like this would impact
> future tests of dynamic page addition where not all scenarios could be
> tested with all EPC pages already consumed.
> 
> Reinette

Re-initializing the test enclave is mandatory thing to do for all tests
because it has an internals state.

/Jarkko

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

* Re: [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-06 20:53   ` Dave Hansen
@ 2021-07-06 23:52     ` Jarkko Sakkinen
  0 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-06 23:52 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Reinette Chatre,
	Borislav Petkov, Tianjia Zhang, Dave Hansen, Jethro Beekman,
	Borislav Petkov, linux-kernel

On Tue, Jul 06, 2021 at 01:53:20PM -0700, Dave Hansen wrote:
> On 7/5/21 7:36 AM, Jarkko Sakkinen wrote:
> > From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> > 
> > Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated
> > length of Q1 and Q2 is less than 384 bytes, things will go wrong.
> > 
> > E.g. if Q2 is 383 bytes, then
> > 
> > 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2().
> > 2. The entire sigstruct->q2 is reversed, which results it being
> >    256 * Q2, given that the last byte of sigstruct->q2 is added
> >    to before the bytes given by calc_q1q2().
> > 
> > Either change in key or measurement can trigger the bug. E.g. an unmeasured
> > heap could cause a devastating change in Q1 or Q2.
> > 
> > Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to
> > the caller.
> > 
> > Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
> > Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/
> > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> This looks fine, but can I suggest a Subject: tweak?
> 
> 	selftests/sgx: Fix calculations for sub-maximum field sizes

WFM

> 
> In any case:
> 
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>

Thank you.

/Jarkko

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-06 23:50     ` Jarkko Sakkinen
@ 2021-07-07  0:10       ` Reinette Chatre
  2021-07-07  9:17         ` Jarkko Sakkinen
  0 siblings, 1 reply; 20+ messages in thread
From: Reinette Chatre @ 2021-07-07  0:10 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

Hi Jarkko,

On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
> On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
>>> Create a heap for the test enclave, which has the same size as all
>>> available Enclave Page Cache (EPC) pages in the system. This will guarantee
>>> that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
>>> have been swapped out by the page reclaimer during the load time. Actually,
>>> this adds a bit more stress than that since part of the EPC gets reserved
>>> for the Version Array (VA) pages.
>>>
>>> For each test, the page fault handler gets triggered in two occasions:
>>>
>>> - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
>>>     page fault handler.
>>> - During the execution, each page that is referenced gets swapped in
>>>     by the page fault handler.
>>>
>>
>> If I understand this correctly, all EPC pages are now being consumed during
>> fixture setup and thus every SGX test, no matter how big or small, now
>> becomes a stress test of the reclaimer instead of there being a unique
>> reclaimer test. Since an enclave is set up and torn down for every test this
>> seems like a significant addition. It also seems like this would impact
>> future tests of dynamic page addition where not all scenarios could be
>> tested with all EPC pages already consumed.
>>
>> Reinette
> 
> Re-initializing the test enclave is mandatory thing to do for all tests
> because it has an internals state.
> 

Right, but not all tests require the same enclave. In kselftest 
terminology I think you are attempting to force all tests to depend on 
the same test fixture. Is it not possible to have a separate "reclaimer" 
test fixture that would build an enclave with a large heap and then have 
reclaimer tests that exercise it by being tests that are specific to 
this "reclaimer fixture"?

Reinette

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07  0:10       ` Reinette Chatre
@ 2021-07-07  9:17         ` Jarkko Sakkinen
  2021-07-07 15:02           ` Reinette Chatre
  0 siblings, 1 reply; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-07  9:17 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

On Tue, Jul 06, 2021 at 05:10:38PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
> > On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
> > > > Create a heap for the test enclave, which has the same size as all
> > > > available Enclave Page Cache (EPC) pages in the system. This will guarantee
> > > > that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
> > > > have been swapped out by the page reclaimer during the load time. Actually,
> > > > this adds a bit more stress than that since part of the EPC gets reserved
> > > > for the Version Array (VA) pages.
> > > > 
> > > > For each test, the page fault handler gets triggered in two occasions:
> > > > 
> > > > - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
> > > >     page fault handler.
> > > > - During the execution, each page that is referenced gets swapped in
> > > >     by the page fault handler.
> > > > 
> > > 
> > > If I understand this correctly, all EPC pages are now being consumed during
> > > fixture setup and thus every SGX test, no matter how big or small, now
> > > becomes a stress test of the reclaimer instead of there being a unique
> > > reclaimer test. Since an enclave is set up and torn down for every test this
> > > seems like a significant addition. It also seems like this would impact
> > > future tests of dynamic page addition where not all scenarios could be
> > > tested with all EPC pages already consumed.
> > > 
> > > Reinette
> > 
> > Re-initializing the test enclave is mandatory thing to do for all tests
> > because it has an internals state.
> > 
> 
> Right, but not all tests require the same enclave. In kselftest terminology
> I think you are attempting to force all tests to depend on the same test
> fixture. Is it not possible to have a separate "reclaimer" test fixture that
> would build an enclave with a large heap and then have reclaimer tests that
> exercise it by being tests that are specific to this "reclaimer fixture"?
> 
> Reinette

Why add that complexity?

/Jarkko

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07  9:17         ` Jarkko Sakkinen
@ 2021-07-07 15:02           ` Reinette Chatre
  2021-07-07 20:50             ` Jarkko Sakkinen
  0 siblings, 1 reply; 20+ messages in thread
From: Reinette Chatre @ 2021-07-07 15:02 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

Hi Jarkko,

On 7/7/2021 2:17 AM, Jarkko Sakkinen wrote:
> On Tue, Jul 06, 2021 at 05:10:38PM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
>>> On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
>>>> Hi Jarkko,
>>>>
>>>> On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
>>>>> Create a heap for the test enclave, which has the same size as all
>>>>> available Enclave Page Cache (EPC) pages in the system. This will guarantee
>>>>> that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
>>>>> have been swapped out by the page reclaimer during the load time. Actually,
>>>>> this adds a bit more stress than that since part of the EPC gets reserved
>>>>> for the Version Array (VA) pages.
>>>>>
>>>>> For each test, the page fault handler gets triggered in two occasions:
>>>>>
>>>>> - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
>>>>>      page fault handler.
>>>>> - During the execution, each page that is referenced gets swapped in
>>>>>      by the page fault handler.
>>>>>
>>>>
>>>> If I understand this correctly, all EPC pages are now being consumed during
>>>> fixture setup and thus every SGX test, no matter how big or small, now
>>>> becomes a stress test of the reclaimer instead of there being a unique
>>>> reclaimer test. Since an enclave is set up and torn down for every test this
>>>> seems like a significant addition. It also seems like this would impact
>>>> future tests of dynamic page addition where not all scenarios could be
>>>> tested with all EPC pages already consumed.
>>>>
>>>> Reinette
>>>
>>> Re-initializing the test enclave is mandatory thing to do for all tests
>>> because it has an internals state.
>>>
>>
>> Right, but not all tests require the same enclave. In kselftest terminology
>> I think you are attempting to force all tests to depend on the same test
>> fixture. Is it not possible to have a separate "reclaimer" test fixture that
>> would build an enclave with a large heap and then have reclaimer tests that
>> exercise it by being tests that are specific to this "reclaimer fixture"?
>>
>> Reinette
> 
> Why add that complexity?
> 

With this change every test is turned into a pseudo reclaimer test 
without there being any explicit testing (with pass/fail criteria) of 
reclaimer behavior. This is an expensive addition and reduces the 
scenarios that the tests can exercise.

Reinette

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07 15:02           ` Reinette Chatre
@ 2021-07-07 20:50             ` Jarkko Sakkinen
  2021-07-07 21:20               ` Reinette Chatre
  2021-07-07 21:20               ` Dave Hansen
  0 siblings, 2 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-07 20:50 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

On Wed, Jul 07, 2021 at 08:02:42AM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 7/7/2021 2:17 AM, Jarkko Sakkinen wrote:
> > On Tue, Jul 06, 2021 at 05:10:38PM -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
> > > > On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
> > > > > Hi Jarkko,
> > > > > 
> > > > > On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
> > > > > > Create a heap for the test enclave, which has the same size as all
> > > > > > available Enclave Page Cache (EPC) pages in the system. This will guarantee
> > > > > > that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
> > > > > > have been swapped out by the page reclaimer during the load time. Actually,
> > > > > > this adds a bit more stress than that since part of the EPC gets reserved
> > > > > > for the Version Array (VA) pages.
> > > > > > 
> > > > > > For each test, the page fault handler gets triggered in two occasions:
> > > > > > 
> > > > > > - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
> > > > > >      page fault handler.
> > > > > > - During the execution, each page that is referenced gets swapped in
> > > > > >      by the page fault handler.
> > > > > > 
> > > > > 
> > > > > If I understand this correctly, all EPC pages are now being consumed during
> > > > > fixture setup and thus every SGX test, no matter how big or small, now
> > > > > becomes a stress test of the reclaimer instead of there being a unique
> > > > > reclaimer test. Since an enclave is set up and torn down for every test this
> > > > > seems like a significant addition. It also seems like this would impact
> > > > > future tests of dynamic page addition where not all scenarios could be
> > > > > tested with all EPC pages already consumed.
> > > > > 
> > > > > Reinette
> > > > 
> > > > Re-initializing the test enclave is mandatory thing to do for all tests
> > > > because it has an internals state.
> > > > 
> > > 
> > > Right, but not all tests require the same enclave. In kselftest terminology
> > > I think you are attempting to force all tests to depend on the same test
> > > fixture. Is it not possible to have a separate "reclaimer" test fixture that
> > > would build an enclave with a large heap and then have reclaimer tests that
> > > exercise it by being tests that are specific to this "reclaimer fixture"?
> > > 
> > > Reinette
> > 
> > Why add that complexity?
> > 
> 
> With this change every test is turned into a pseudo reclaimer test without
> there being any explicit testing (with pass/fail criteria) of reclaimer
> behavior. This is an expensive addition and reduces the scenarios that the
> tests can exercise.
> 
> Reinette

There is consistent known behaviour how reclaimer and also the page fault
are exercised for each test. I think that is what matters most right now
that the basic behaviour of both the page reclaimer and page fault handler
gets exercised.

I don't understand the real-world gain of doing something factors more
complex than necessary at a particular point of time,  when you don't
really need to hang yourself into it forever.

This patch does increase the coverage in a deterministic manner to the code
paths that were not previously exercised, i.e. we know the code paths, and
could even calculate the exact number of times that they are triggered. And
without doing anything obscure. That's what matters to me.

/Jarkko

/Jarkko

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07 20:50             ` Jarkko Sakkinen
@ 2021-07-07 21:20               ` Reinette Chatre
  2021-07-09 16:22                 ` Jarkko Sakkinen
  2021-07-07 21:20               ` Dave Hansen
  1 sibling, 1 reply; 20+ messages in thread
From: Reinette Chatre @ 2021-07-07 21:20 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

Hi Jarkko,

On 7/7/2021 1:50 PM, Jarkko Sakkinen wrote:
> On Wed, Jul 07, 2021 at 08:02:42AM -0700, Reinette Chatre wrote:
>> Hi Jarkko,
>>
>> On 7/7/2021 2:17 AM, Jarkko Sakkinen wrote:
>>> On Tue, Jul 06, 2021 at 05:10:38PM -0700, Reinette Chatre wrote:
>>>> Hi Jarkko,
>>>>
>>>> On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
>>>>> On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
>>>>>> Hi Jarkko,
>>>>>>
>>>>>> On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
>>>>>>> Create a heap for the test enclave, which has the same size as all
>>>>>>> available Enclave Page Cache (EPC) pages in the system. This will guarantee
>>>>>>> that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
>>>>>>> have been swapped out by the page reclaimer during the load time. Actually,
>>>>>>> this adds a bit more stress than that since part of the EPC gets reserved
>>>>>>> for the Version Array (VA) pages.
>>>>>>>
>>>>>>> For each test, the page fault handler gets triggered in two occasions:
>>>>>>>
>>>>>>> - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
>>>>>>>       page fault handler.
>>>>>>> - During the execution, each page that is referenced gets swapped in
>>>>>>>       by the page fault handler.
>>>>>>>
>>>>>>
>>>>>> If I understand this correctly, all EPC pages are now being consumed during
>>>>>> fixture setup and thus every SGX test, no matter how big or small, now
>>>>>> becomes a stress test of the reclaimer instead of there being a unique
>>>>>> reclaimer test. Since an enclave is set up and torn down for every test this
>>>>>> seems like a significant addition. It also seems like this would impact
>>>>>> future tests of dynamic page addition where not all scenarios could be
>>>>>> tested with all EPC pages already consumed.
>>>>>>
>>>>>> Reinette
>>>>>
>>>>> Re-initializing the test enclave is mandatory thing to do for all tests
>>>>> because it has an internals state.
>>>>>
>>>>
>>>> Right, but not all tests require the same enclave. In kselftest terminology
>>>> I think you are attempting to force all tests to depend on the same test
>>>> fixture. Is it not possible to have a separate "reclaimer" test fixture that
>>>> would build an enclave with a large heap and then have reclaimer tests that
>>>> exercise it by being tests that are specific to this "reclaimer fixture"?
>>>>
>>>> Reinette
>>>
>>> Why add that complexity?
>>>
>>
>> With this change every test is turned into a pseudo reclaimer test without
>> there being any explicit testing (with pass/fail criteria) of reclaimer
>> behavior. This is an expensive addition and reduces the scenarios that the
>> tests can exercise.
>>
>> Reinette
> 
> There is consistent known behaviour how reclaimer and also the page fault
> are exercised for each test. I think that is what matters most right now
> that the basic behaviour of both the page reclaimer and page fault handler
> gets exercised.

I believe the basic behavior of page fault handler is currently 
exercised in each test, this is required.

> 
> I don't understand the real-world gain of doing something factors more
> complex than necessary at a particular point of time,  when you don't
> really need to hang yourself into it forever.

Your argument about "hang yourself into it forever" can go both ways - 
why should all tests now unnecessarily consume the entire EPC forever?

If I understand correctly adding a separate reclaimer test is not 
complex but would require refactoring code.

> This patch does increase the coverage in a deterministic manner to the code
> paths that were not previously exercised, i.e. we know the code paths, and
> could even calculate the exact number of times that they are triggered. And
> without doing anything obscure. That's what matters to me.

On the contrary this is indeed obfuscating the SGX tests: if an issue 
shows up in the reclaimer then all tests would fail. If there is a 
unique reclaimer test then that would help point to where the issue may be.

Reinette

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07 20:50             ` Jarkko Sakkinen
  2021-07-07 21:20               ` Reinette Chatre
@ 2021-07-07 21:20               ` Dave Hansen
  2021-07-09 16:25                 ` Jarkko Sakkinen
  1 sibling, 1 reply; 20+ messages in thread
From: Dave Hansen @ 2021-07-07 21:20 UTC (permalink / raw)
  To: Jarkko Sakkinen, Reinette Chatre
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

On 7/7/21 1:50 PM, Jarkko Sakkinen wrote:
> There is consistent known behaviour how reclaimer and also the page fault
> are exercised for each test. I think that is what matters most right now
> that the basic behaviour of both the page reclaimer and page fault handler
> gets exercised.

There's also a lot of value to ensuring that tests can run _quickly_.
If you have a test that fails one out of a million executions, it's a
lot easier find and debug if it takes 1 ms versus 10 seconds.

In other words, I think I'd prefer if we run two enclaves in each
execution of the selftest.  One can be as small as possible.  The other
can be the reclaim-triggering one.

That's good both for test coverage, and it makes it a *bit* more
straightforward to hack out the reclaim test if you need things to run
faster.

The pkeys selftest isn't a bad example here either.  It has a couple of
different "malloc()" options: THP, hugetlbfs, small-page mmap(), and a
bunch of tests it runs on each type.  As we add more SGX tests, we might
end up with "do reclaim" just being an option we pass.

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07 21:20               ` Reinette Chatre
@ 2021-07-09 16:22                 ` Jarkko Sakkinen
  0 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-09 16:22 UTC (permalink / raw)
  To: Reinette Chatre
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Borislav Petkov,
	Dave Hansen, linux-kernel

On Wed, Jul 07, 2021 at 02:20:04PM -0700, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 7/7/2021 1:50 PM, Jarkko Sakkinen wrote:
> > On Wed, Jul 07, 2021 at 08:02:42AM -0700, Reinette Chatre wrote:
> > > Hi Jarkko,
> > > 
> > > On 7/7/2021 2:17 AM, Jarkko Sakkinen wrote:
> > > > On Tue, Jul 06, 2021 at 05:10:38PM -0700, Reinette Chatre wrote:
> > > > > Hi Jarkko,
> > > > > 
> > > > > On 7/6/2021 4:50 PM, Jarkko Sakkinen wrote:
> > > > > > On Tue, Jul 06, 2021 at 11:34:54AM -0700, Reinette Chatre wrote:
> > > > > > > Hi Jarkko,
> > > > > > > 
> > > > > > > On 7/5/2021 7:36 AM, Jarkko Sakkinen wrote:
> > > > > > > > Create a heap for the test enclave, which has the same size as all
> > > > > > > > available Enclave Page Cache (EPC) pages in the system. This will guarantee
> > > > > > > > that all test_encl.elf pages *and* SGX Enclave Control Structure (SECS)
> > > > > > > > have been swapped out by the page reclaimer during the load time. Actually,
> > > > > > > > this adds a bit more stress than that since part of the EPC gets reserved
> > > > > > > > for the Version Array (VA) pages.
> > > > > > > > 
> > > > > > > > For each test, the page fault handler gets triggered in two occasions:
> > > > > > > > 
> > > > > > > > - When SGX_IOC_ENCLAVE_INIT is performed, SECS gets swapped in by the
> > > > > > > >       page fault handler.
> > > > > > > > - During the execution, each page that is referenced gets swapped in
> > > > > > > >       by the page fault handler.
> > > > > > > > 
> > > > > > > 
> > > > > > > If I understand this correctly, all EPC pages are now being consumed during
> > > > > > > fixture setup and thus every SGX test, no matter how big or small, now
> > > > > > > becomes a stress test of the reclaimer instead of there being a unique
> > > > > > > reclaimer test. Since an enclave is set up and torn down for every test this
> > > > > > > seems like a significant addition. It also seems like this would impact
> > > > > > > future tests of dynamic page addition where not all scenarios could be
> > > > > > > tested with all EPC pages already consumed.
> > > > > > > 
> > > > > > > Reinette
> > > > > > 
> > > > > > Re-initializing the test enclave is mandatory thing to do for all tests
> > > > > > because it has an internals state.
> > > > > > 
> > > > > 
> > > > > Right, but not all tests require the same enclave. In kselftest terminology
> > > > > I think you are attempting to force all tests to depend on the same test
> > > > > fixture. Is it not possible to have a separate "reclaimer" test fixture that
> > > > > would build an enclave with a large heap and then have reclaimer tests that
> > > > > exercise it by being tests that are specific to this "reclaimer fixture"?
> > > > > 
> > > > > Reinette
> > > > 
> > > > Why add that complexity?
> > > > 
> > > 
> > > With this change every test is turned into a pseudo reclaimer test without
> > > there being any explicit testing (with pass/fail criteria) of reclaimer
> > > behavior. This is an expensive addition and reduces the scenarios that the
> > > tests can exercise.
> > > 
> > > Reinette
> > 
> > There is consistent known behaviour how reclaimer and also the page fault
> > are exercised for each test. I think that is what matters most right now
> > that the basic behaviour of both the page reclaimer and page fault handler
> > gets exercised.
> 
> I believe the basic behavior of page fault handler is currently exercised in
> each test, this is required.

This not true. The current test does not exercise ELDU code path.

> 
> > 
> > I don't understand the real-world gain of doing something factors more
> > complex than necessary at a particular point of time,  when you don't
> > really need to hang yourself into it forever.
> 
> Your argument about "hang yourself into it forever" can go both ways - why
> should all tests now unnecessarily consume the entire EPC forever?
> 
> If I understand correctly adding a separate reclaimer test is not complex
> but would require refactoring code.

What does it matter anyway if code nees to be refactored?

> > This patch does increase the coverage in a deterministic manner to the code
> > paths that were not previously exercised, i.e. we know the code paths, and
> > could even calculate the exact number of times that they are triggered. And
> > without doing anything obscure. That's what matters to me.
> 
> On the contrary this is indeed obfuscating the SGX tests: if an issue shows
> up in the reclaimer then all tests would fail. If there is a unique
> reclaimer test then that would help point to where the issue may be.

I tend to disagree this. I'll add a separate reclaimer test if I need
to test something that this does not scale. It's an iterative process.

/Jarkko

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

* Re: [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler
  2021-07-07 21:20               ` Dave Hansen
@ 2021-07-09 16:25                 ` Jarkko Sakkinen
  0 siblings, 0 replies; 20+ messages in thread
From: Jarkko Sakkinen @ 2021-07-09 16:25 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Reinette Chatre, Shuah Khan, linux-kselftest, linux-sgx,
	Borislav Petkov, Dave Hansen, linux-kernel

On Wed, Jul 07, 2021 at 02:20:07PM -0700, Dave Hansen wrote:
> On 7/7/21 1:50 PM, Jarkko Sakkinen wrote:
> > There is consistent known behaviour how reclaimer and also the page fault
> > are exercised for each test. I think that is what matters most right now
> > that the basic behaviour of both the page reclaimer and page fault handler
> > gets exercised.
> 
> There's also a lot of value to ensuring that tests can run _quickly_.
> If you have a test that fails one out of a million executions, it's a
> lot easier find and debug if it takes 1 ms versus 10 seconds.
> 
> In other words, I think I'd prefer if we run two enclaves in each
> execution of the selftest.  One can be as small as possible.  The other
> can be the reclaim-triggering one.
> 
> That's good both for test coverage, and it makes it a *bit* more
> straightforward to hack out the reclaim test if you need things to run
> faster.
> 
> The pkeys selftest isn't a bad example here either.  It has a couple of
> different "malloc()" options: THP, hugetlbfs, small-page mmap(), and a
> bunch of tests it runs on each type.  As we add more SGX tests, we might
> end up with "do reclaim" just being an option we pass.

Even with large EPC's, the current test runs quite fast, because heap is
left unmeasured. It's the EEXTEND operations that would cause a major
slow-down.

I would go only to something "more complex" when the current test hits
the roof. I don't like to make code more complicated, when that does not
happen.

When there's no compatibility requirements, it's not hard to refactor it
later on.

/Jarkko

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

end of thread, other threads:[~2021-07-09 16:25 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 14:36 [PATCH 0/4] selftests/sgx: Trigger the page reclaimer and #PF handler Jarkko Sakkinen
2021-07-05 14:36 ` [PATCH 1/4] x86/sgx: Add sgx_nr_all_pages to the debugfs Jarkko Sakkinen
2021-07-06 14:56   ` Dave Hansen
2021-07-06 15:39     ` Greg KH
2021-07-06 22:08     ` Jarkko Sakkinen
2021-07-05 14:36 ` [PATCH 2/4] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
2021-07-06 20:53   ` Dave Hansen
2021-07-06 23:52     ` Jarkko Sakkinen
2021-07-05 14:36 ` [PATCH 3/4] selftests/sgx: Assign source for each segment Jarkko Sakkinen
2021-07-05 14:36 ` [PATCH 4/4] selftests/sgx: Trigger the reclaimer and #PF handler Jarkko Sakkinen
2021-07-06 18:34   ` Reinette Chatre
2021-07-06 23:50     ` Jarkko Sakkinen
2021-07-07  0:10       ` Reinette Chatre
2021-07-07  9:17         ` Jarkko Sakkinen
2021-07-07 15:02           ` Reinette Chatre
2021-07-07 20:50             ` Jarkko Sakkinen
2021-07-07 21:20               ` Reinette Chatre
2021-07-09 16:22                 ` Jarkko Sakkinen
2021-07-07 21:20               ` Dave Hansen
2021-07-09 16:25                 ` Jarkko Sakkinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.