| Message ID | 20220830031206.13449-6-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Test and fixes | expand |
Hi Haitao and Jarkko, selftests/sgx: Retry the ioctl()s returned with EAGAIN On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: > From: Haitao Huang <haitao.huang@linux.intel.com> > > For EMODT and EREMOVE ioctls with a large range, kernel ioctl()s? > may not finish in one shot and return EAGAIN error code > and count of bytes of EPC pages on that operations are > finished successfully. > > Change the unclobbered_vdso_oversubscribed_remove test > to rerun the ioctls in a loop, updating offset and length ioctl()s? > using the byte count returned in each iteration. > This is a valuable addition, thank you very much. > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > tools/testing/selftests/sgx/main.c | 39 +++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c > index 867e98502120..3268d8b01b0b 100644 > --- a/tools/testing/selftests/sgx/main.c > +++ b/tools/testing/selftests/sgx/main.c > @@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > unsigned long total_mem; > int ret, errno_save; > unsigned long addr; > - unsigned long i; > + unsigned long i, count; Reverse fir tree? > > /* > * Create enclave with additional heap that is as big as all > @@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > modt_ioc.offset = heap->offset; > modt_ioc.length = heap->size; > modt_ioc.page_type = SGX_PAGE_TYPE_TRIM; > - > + count = 0; > TH_LOG("Changing type of %zd bytes to trimmed may take a while ...", > heap->size); > - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > - errno_save = ret == -1 ? errno : 0; > + do { > + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > + errno_save = ret == -1 ? errno : 0; > + if (errno_save == EAGAIN) { > + count += modt_ioc.count; > + modt_ioc.offset += modt_ioc.count; > + modt_ioc.length -= modt_ioc.count; > + modt_ioc.result = 0; As part of the test I think it would be helpful to know if there was a result code in here. What do you think of failing the test if it is not zero? > + modt_ioc.count = 0; > + } else > + break; Watch out for unbalanced braces (also later in patch). This causes checkpatch.pl noise. > + } while (modt_ioc.length != 0); > > EXPECT_EQ(ret, 0); > EXPECT_EQ(errno_save, 0); > EXPECT_EQ(modt_ioc.result, 0); > - EXPECT_EQ(modt_ioc.count, heap->size); > + count += modt_ioc.count; > + EXPECT_EQ(count, heap->size); > > /* EACCEPT all removed pages. */ > addr = self->encl.encl_base + heap->offset; > @@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > remove_ioc.offset = heap->offset; > remove_ioc.length = heap->size; > - > + count = 0; > TH_LOG("Removing %zd bytes from enclave may take a while ...", > heap->size); > - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); > - errno_save = ret == -1 ? errno : 0; > + do { > + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); > + errno_save = ret == -1 ? errno : 0; > + if (errno_save == EAGAIN) { > + count += remove_ioc.count; > + remove_ioc.offset += remove_ioc.count; > + remove_ioc.length -= remove_ioc.count; > + remove_ioc.count = 0; > + } else > + break; > + } while (remove_ioc.length != 0); > > EXPECT_EQ(ret, 0); > EXPECT_EQ(errno_save, 0); > - EXPECT_EQ(remove_ioc.count, heap->size); > + count += remove_ioc.count; > + EXPECT_EQ(count, heap->size); > } > > TEST_F(enclave, clobbered_vdso) Reinette
On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote: > Hi Haitao and Jarkko, > > > selftests/sgx: Retry the ioctl()s returned with EAGAIN > > > On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: > > From: Haitao Huang <haitao.huang@linux.intel.com> > > > > For EMODT and EREMOVE ioctls with a large range, kernel > > ioctl()s? Ioctl is common enough to be considered as noun and is widely phrased like that in commit messages. I don't see any added clarity. > > > may not finish in one shot and return EAGAIN error code > > and count of bytes of EPC pages on that operations are > > finished successfully. > > > > Change the unclobbered_vdso_oversubscribed_remove test > > to rerun the ioctls in a loop, updating offset and length > > ioctl()s? > > > using the byte count returned in each iteration. > > > > This is a valuable addition, thank you very much. > > > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > tools/testing/selftests/sgx/main.c | 39 +++++++++++++++++++++++------- > > 1 file changed, 30 insertions(+), 9 deletions(-) > > > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c > > index 867e98502120..3268d8b01b0b 100644 > > --- a/tools/testing/selftests/sgx/main.c > > +++ b/tools/testing/selftests/sgx/main.c > > @@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > unsigned long total_mem; > > int ret, errno_save; > > unsigned long addr; > > - unsigned long i; > > + unsigned long i, count; > > Reverse fir tree? +1 > > > > > /* > > * Create enclave with additional heap that is as big as all > > @@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > modt_ioc.offset = heap->offset; > > modt_ioc.length = heap->size; > > modt_ioc.page_type = SGX_PAGE_TYPE_TRIM; > > - > > + count = 0; > > TH_LOG("Changing type of %zd bytes to trimmed may take a while ...", > > heap->size); > > - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > > - errno_save = ret == -1 ? errno : 0; > > + do { > > + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); > > + errno_save = ret == -1 ? errno : 0; > > + if (errno_save == EAGAIN) { > > + count += modt_ioc.count; > > + modt_ioc.offset += modt_ioc.count; > > + modt_ioc.length -= modt_ioc.count; > > + modt_ioc.result = 0; > > As part of the test I think it would be helpful to know if there was a result code > in here. What do you think of failing the test if it is not zero? I would not mind doing that. > > > + modt_ioc.count = 0; > > + } else > > + break; > > Watch out for unbalanced braces (also later in patch). This causes > checkpatch.pl noise. Again. I did run checkpatch to all of these. Will revisit. > > > + } while (modt_ioc.length != 0); > > > > EXPECT_EQ(ret, 0); > > EXPECT_EQ(errno_save, 0); > > EXPECT_EQ(modt_ioc.result, 0); > > - EXPECT_EQ(modt_ioc.count, heap->size); > > + count += modt_ioc.count; > > + EXPECT_EQ(count, heap->size); > > > > /* EACCEPT all removed pages. */ > > addr = self->encl.encl_base + heap->offset; > > @@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) > > > > remove_ioc.offset = heap->offset; > > remove_ioc.length = heap->size; > > - > > + count = 0; > > TH_LOG("Removing %zd bytes from enclave may take a while ...", > > heap->size); > > - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); > > - errno_save = ret == -1 ? errno : 0; > > + do { > > + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); > > + errno_save = ret == -1 ? errno : 0; > > + if (errno_save == EAGAIN) { > > + count += remove_ioc.count; > > + remove_ioc.offset += remove_ioc.count; > > + remove_ioc.length -= remove_ioc.count; > > + remove_ioc.count = 0; > > + } else > > + break; > > + } while (remove_ioc.length != 0); > > > > EXPECT_EQ(ret, 0); > > EXPECT_EQ(errno_save, 0); > > - EXPECT_EQ(remove_ioc.count, heap->size); > > + count += remove_ioc.count; > > + EXPECT_EQ(count, heap->size); > > } > > > > TEST_F(enclave, clobbered_vdso) > > Reinette BR, Jarkko
Hi Jarkko, On 8/30/2022 7:31 PM, Jarkko Sakkinen wrote: > On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote: >> Hi Haitao and Jarkko, >> >> >> selftests/sgx: Retry the ioctl()s returned with EAGAIN >> >> >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: >>> From: Haitao Huang <haitao.huang@linux.intel.com> >>> >>> For EMODT and EREMOVE ioctls with a large range, kernel >> >> ioctl()s? > > Ioctl is common enough to be considered as noun and is > widely phrased like that in commit messages. I don't > see any added clarity. ok. I was asked to make this change in the SGX2 patches and thought that I should propagate this advice :) >>> + modt_ioc.count = 0; >>> + } else >>> + break; >> >> Watch out for unbalanced braces (also later in patch). This causes >> checkpatch.pl noise. > > Again. I did run checkpatch to all of these. Will revisit. It looks like I see it because I use "checkpatch.pl --strict". Reinette
On 8/30/22 19:31, Jarkko Sakkinen wrote: >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: >>> From: Haitao Huang <haitao.huang@linux.intel.com> >>> >>> For EMODT and EREMOVE ioctls with a large range, kernel >> ioctl()s? > Ioctl is common enough to be considered as noun and is > widely phrased like that in commit messages. I don't > see any added clarity. I definitely prefer ioctl().
On Wed, Aug 31, 2022 at 11:09:21AM -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 8/30/2022 7:31 PM, Jarkko Sakkinen wrote: > > On Tue, Aug 30, 2022 at 03:56:29PM -0700, Reinette Chatre wrote: > >> Hi Haitao and Jarkko, > >> > >> > >> selftests/sgx: Retry the ioctl()s returned with EAGAIN > >> > >> > >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: > >>> From: Haitao Huang <haitao.huang@linux.intel.com> > >>> > >>> For EMODT and EREMOVE ioctls with a large range, kernel > >> > >> ioctl()s? > > > > Ioctl is common enough to be considered as noun and is > > widely phrased like that in commit messages. I don't > > see any added clarity. > > ok. I was asked to make this change in the SGX2 patches and > thought that I should propagate this advice :) I can use the other form too, np. > > >>> + modt_ioc.count = 0; > >>> + } else > >>> + break; > >> > >> Watch out for unbalanced braces (also later in patch). This causes > >> checkpatch.pl noise. > > > > Again. I did run checkpatch to all of these. Will revisit. > > It looks like I see it because I use "checkpatch.pl --strict". Thanks BTW for pointing this out :-) > Reinette BR, Jarkko
On Wed, Aug 31, 2022 at 11:14:02AM -0700, Dave Hansen wrote: > On 8/30/22 19:31, Jarkko Sakkinen wrote: > >> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote: > >>> From: Haitao Huang <haitao.huang@linux.intel.com> > >>> > >>> For EMODT and EREMOVE ioctls with a large range, kernel > >> ioctl()s? > > Ioctl is common enough to be considered as noun and is > > widely phrased like that in commit messages. I don't > > see any added clarity. > > I definitely prefer ioctl(). I can use it. BR, Jarkko
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 867e98502120..3268d8b01b0b 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -399,7 +399,7 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) unsigned long total_mem; int ret, errno_save; unsigned long addr; - unsigned long i; + unsigned long i, count; /* * Create enclave with additional heap that is as big as all @@ -461,16 +461,27 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) modt_ioc.offset = heap->offset; modt_ioc.length = heap->size; modt_ioc.page_type = SGX_PAGE_TYPE_TRIM; - + count = 0; TH_LOG("Changing type of %zd bytes to trimmed may take a while ...", heap->size); - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); - errno_save = ret == -1 ? errno : 0; + do { + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_MODIFY_TYPES, &modt_ioc); + errno_save = ret == -1 ? errno : 0; + if (errno_save == EAGAIN) { + count += modt_ioc.count; + modt_ioc.offset += modt_ioc.count; + modt_ioc.length -= modt_ioc.count; + modt_ioc.result = 0; + modt_ioc.count = 0; + } else + break; + } while (modt_ioc.length != 0); EXPECT_EQ(ret, 0); EXPECT_EQ(errno_save, 0); EXPECT_EQ(modt_ioc.result, 0); - EXPECT_EQ(modt_ioc.count, heap->size); + count += modt_ioc.count; + EXPECT_EQ(count, heap->size); /* EACCEPT all removed pages. */ addr = self->encl.encl_base + heap->offset; @@ -498,15 +509,25 @@ TEST_F_TIMEOUT(enclave, unclobbered_vdso_oversubscribed_remove, 900) remove_ioc.offset = heap->offset; remove_ioc.length = heap->size; - + count = 0; TH_LOG("Removing %zd bytes from enclave may take a while ...", heap->size); - ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); - errno_save = ret == -1 ? errno : 0; + do { + ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_REMOVE_PAGES, &remove_ioc); + errno_save = ret == -1 ? errno : 0; + if (errno_save == EAGAIN) { + count += remove_ioc.count; + remove_ioc.offset += remove_ioc.count; + remove_ioc.length -= remove_ioc.count; + remove_ioc.count = 0; + } else + break; + } while (remove_ioc.length != 0); EXPECT_EQ(ret, 0); EXPECT_EQ(errno_save, 0); - EXPECT_EQ(remove_ioc.count, heap->size); + count += remove_ioc.count; + EXPECT_EQ(count, heap->size); } TEST_F(enclave, clobbered_vdso)