On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote: > Hi Vijay and Jarkko, > > On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: > > From: Vijay Dhanraj <vijay.dhanraj@xxxxxxxxx> > > > > Add a new test case which is same as augment_via_eaccept but adds a > > larger number of EPC pages to stress test EAUG via EACCEPT. > > > > Signed-off-by: Vijay Dhanraj <vijay.dhanraj@xxxxxxxxx> > > Co-developed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> > > Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> > > --- > > v2: > > - Addressed Reinette's feedback: > > https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@xxxxxxxxx/ > > --- > > tools/testing/selftests/sgx/load.c | 5 +- > > tools/testing/selftests/sgx/main.c | 141 +++++++++++++++++++++--- > > tools/testing/selftests/sgx/main.h | 3 +- > > tools/testing/selftests/sgx/sigstruct.c | 2 +- > > 4 files changed, 129 insertions(+), 22 deletions(-) > > There seems to be at least three patches merged into one here: > 1) Update SGX selftests to create enclaves with provided size dedicated > to EDMM (this change causes a lot of noise and distracts from the test > addition). > 2) The mrenclave_ecreate() fix (which is still incomplete). > 3) The actual test addition. I would agree on this on a kernel patch but not for kselftest patch. It does not really give useful value here. This adds a test and that is a good enough granularity in my opinion, unless some major architecture work is required as precursory. It is not the case here. > > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c > > index 94bdeac1cf04..7de1b15c90b1 100644 > > --- a/tools/testing/selftests/sgx/load.c > > +++ b/tools/testing/selftests/sgx/load.c > > @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol) > > return 0; > > } > > > > -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size) > > +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size, > > + unsigned long edmm_size) > > { > > checkpatch.pl informs about alignment issues above and also a few other places. Weird. I did run checkpatch through these. Will revisit. > > > const char device_path[] = "/dev/sgx_enclave"; > > struct encl_segment *seg; > > @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size) > > > > encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size; > > > > - for (encl->encl_size = 4096; encl->encl_size < encl->src_size; ) > > + for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;) > > encl->encl_size <<= 1; > > > > return true; > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c > > index 9820b3809c69..867e98502120 100644 > > --- a/tools/testing/selftests/sgx/main.c > > +++ b/tools/testing/selftests/sgx/main.c > > @@ -21,8 +21,15 @@ > > #include "../kselftest_harness.h" > > #include "main.h" > > > > +/* > > + * The size was chosen based on a bug report: > > + * https://lore.kernel.org/linux-sgx/DM8PR11MB55912A7F47A84EC9913A6352F6999@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > + */ > > +static const unsigned long EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L; // 8 GB > > +static const unsigned long TIMEOUT_LONG = 900; /* seconds */ > > It is interesting that in this small snippet there are three different > comment styles (multi-line comment preceding code, tail comment using /**/, > and tail comment using //) :) Oops. Will fix. > > > static const uint64_t MAGIC = 0x1122334455667788ULL; > > static const uint64_t MAGIC2 = 0x8877665544332211ULL; > > + > > Addition of empty line here. Will remove. > > > vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave; > > > > /* > > @@ -173,7 +180,8 @@ FIXTURE(enclave) { > > }; > > > > static bool setup_test_encl(unsigned long heap_size, struct encl *encl, > > - struct __test_metadata *_metadata) > > + struct __test_metadata *_metadata, > > + unsigned long edmm_size) > > { > > Elf64_Sym *sgx_enter_enclave_sym = NULL; > > struct vdso_symtab symtab; > > @@ -183,7 +191,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl, > > unsigned int i; > > void *addr; > > > > - if (!encl_load("test_encl.elf", encl, heap_size)) { > > + if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) { > > encl_delete(encl); > > TH_LOG("Failed to load the test enclave."); > > return false; > > @@ -284,7 +292,7 @@ TEST_F(enclave, unclobbered_vdso) > > struct encl_op_get_from_buf get_op; > > struct encl_op_put_to_buf put_op; > > > > - ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); > > + ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0)); > > > > memset(&self->run, 0, sizeof(self->run)); > > self->run.tcs = self->encl.encl_base; > > @@ -357,7 +365,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed) > > > > total_mem = get_total_epc_mem(); > > ASSERT_NE(total_mem, 0); > > - ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata)); > > + ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0)); > > > > The support for EDMM size and all the call site changes could be moved to a separate > patch to make the changes easier to parse. > > > ... > > > @@ -1210,6 +1218,103 @@ TEST_F(enclave, augment_via_eaccept) > > munmap(addr, PAGE_SIZE); > > } > > > > +/* > > + * Test for the addition of large number of pages to an initialized enclave via > > + * a pre-emptive run of EACCEPT on every page to be added. > > + */ > > +TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG) > > +{ > > + struct encl_op_get_from_addr get_addr_op; > > + struct encl_op_put_to_addr put_addr_op; > > + struct encl_op_eaccept eaccept_op; > > + size_t total_size = 0; > > + unsigned long i; > > + void *addr; > > + > > + if (!sgx2_supported()) > > + SKIP(return, "SGX2 not supported"); > > + > > + ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, > > + EDMM_SIZE_LONG)); > > + > > + memset(&self->run, 0, sizeof(self->run)); > > + self->run.tcs = self->encl.encl_base; > > + > > + for (i = 0; i < self->encl.nr_segments; i++) { > > + struct encl_segment *seg = &self->encl.segment_tbl[i]; > > + > > + total_size += seg->size; > > + } > > + > > + /* > > + * mmap() every page at end of existing enclave to be used for > > + * EDMM. > > + */ > > + addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG, > > + PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED, > > + self->encl.fd, 0); > > + EXPECT_NE(addr, MAP_FAILED); > > + > > + self->run.exception_vector = 0; > > + self->run.exception_error_code = 0; > > + self->run.exception_addr = 0; > > + > > + /* > > + * Run EACCEPT on new page to trigger the #PF->EAUG->EACCEPT(again > > + * without a #PF). All should be transparent to userspace. > > + */ > > s/on new page/on every page/ ? +1 > > > + eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING; > > + eaccept_op.ret = 0; > > + eaccept_op.header.type = ENCL_OP_EACCEPT; > > + > > + for (i = 0; i < EDMM_SIZE_LONG; i += 4096) { > > + eaccept_op.epc_addr = (uint64_t)(addr + i); > > + > > + EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0); > > + if (self->run.exception_vector == 14 && > > + self->run.exception_error_code == 4 && > > + self->run.exception_addr == self->encl.encl_base) { > > + munmap(addr, EDMM_SIZE_LONG); > > + SKIP(return, "Kernel does not support adding pages to initialized enclave"); > > + } > > + > > + EXPECT_EQ(self->run.exception_vector, 0); > > + EXPECT_EQ(self->run.exception_error_code, 0); > > + EXPECT_EQ(self->run.exception_addr, 0); > > + ASSERT_EQ(eaccept_op.ret, 0); > > + ASSERT_EQ(self->run.function, EEXIT); > > + } > > + > > I see that you removed all comments here after the discussion about what > comment would be appropriate. This may be ok but it does seem awkward that > there are two parts to this section and the second part still has a comment > while the first does not. How about the comment I provided in > https://lore.kernel.org/linux-sgx/69a5e350-ded9-30c9-dc41-d08c01dd05dc@xxxxxxxxx/ : > > /* > * Pool of pages were successfully added to enclave. Perform sanity > * check on first page of the pool only to ensure data can be written > * to and read from a dynamically added enclave page. > */ +1 > > > + put_addr_op.value = MAGIC; > > + put_addr_op.addr = (unsigned long)addr; > > + put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS; > > + > > + EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0); > > + > > + EXPECT_EEXIT(&self->run); > > + EXPECT_EQ(self->run.exception_vector, 0); > > + EXPECT_EQ(self->run.exception_error_code, 0); > > + EXPECT_EQ(self->run.exception_addr, 0); > > + > > + /* > > + * Read memory from newly added page that was just written to, > > + * confirming that data previously written (MAGIC) is present. > > + */ > > + get_addr_op.value = 0; > > + get_addr_op.addr = (unsigned long)addr; > > + get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS; > > + > > + EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0); > > + > > + EXPECT_EQ(get_addr_op.value, MAGIC); > > + EXPECT_EEXIT(&self->run); > > + EXPECT_EQ(self->run.exception_vector, 0); > > + EXPECT_EQ(self->run.exception_error_code, 0); > > + EXPECT_EQ(self->run.exception_addr, 0); > > + > > + munmap(addr, EDMM_SIZE_LONG); > > +} > > + > > /* > > * SGX2 page type modification test in two phases: > > * Phase 1: > > ... > > > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c > > index a07896a46364..decd767434d5 100644 > > --- a/tools/testing/selftests/sgx/sigstruct.c > > +++ b/tools/testing/selftests/sgx/sigstruct.c > > @@ -349,7 +349,7 @@ bool encl_measure(struct encl *encl) > > if (!ctx) > > goto err; > > > > - if (!mrenclave_ecreate(ctx, encl->src_size)) > > + if (!mrenclave_ecreate(ctx, encl->encl_size)) > > goto err; > > > > for (i = 0; i < encl->nr_segments; i++) { > > As I mentioned in feedback to previous version, even though > encl_size is now provided, this misses that mrenclave_ecreate() > continues to recompute it within. > > Reinette > I simply forgot this one, sorry. Will fix. BR, Jarkko
