Re: [PATCH 5/6] selftests/sgx: retry the ioctls returned with EAGAIN

[Jarkko Sakkinen]

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@xxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxx>
> > Tested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > ---
> >  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