From: Haitao Huang <[email protected]>
For EMODT and EREMOVE ioctls with a large range, kernel
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
using the byte count returned in each iteration.
Signed-off-by: Haitao Huang <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
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;
/*
* 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)
--
2.37.2
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 <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Jarkko Sakkinen <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Tested-by: Jarkko Sakkinen <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > 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
On 8/30/22 19:31, Jarkko Sakkinen wrote:
>> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
>>> From: Haitao Huang <[email protected]>
>>>
>>> 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().
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 <[email protected]>
>>>
>>> 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 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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>>
> >>> 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