Re: [RFC PATCH v4 4/4] selftests/sgx: Add test for madvise(..., WILLNEED)

["Huang, Kai" <kai.huang@intel.com>]

On Tue, 2023-02-14 at 22:42 -0600, Haitao Huang wrote:
> On Tue, 14 Feb 2023 20:38:29 -0600, Huang, Kai <kai.huang@intel.com> wrote:
> 
> > On Fri, 2023-01-27 at 20:55 -0800, Haitao Huang wrote:
> > > +/*
> > > + * Compare performance with and without madvise call before EACCEPT'ing
> > > + * different size of regions.
> > > + */
> > > +TEST_F_TIMEOUT(enclave, augment_via_madvise, TIMEOUT_DEFAULT)
> > > +{
> > > 
> > [...]
> > 
> > > +
> > > +	for (i = 0; i < self->encl.nr_segments; i++) {
> > > +		struct encl_segment *seg = &self->encl.segment_tbl[i];
> > > +
> > > +		total_size += seg->size;
> > > +	}
> > > +
> > > +	for (i = 1; i < 52 && advise_size < max_advise_size; i++) {
> > > +		addr = mmap((void *)self->encl.encl_base + total_size, advise_size,
> > > +			    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
> > > +					self->encl.fd, 0);
> > 
> > I see the problem now.  Here 'pgoff' is always 0.  I think this is wrong.
> > 
> > Shouldn't you use the actual offset relative to the file as pgoff, which  
> > is
> > 	
> > 	total_size >> PAGE_SHIFT
> > 
> > ?
> > 
> But that will be inconsistent with current usage. We have been using zero  
> offset always including these self tests. The offset is also redundant in  
> this case because it is totally defined by the address for a given enclave  
> fd.
> 
> 

From mmap() manpage:

       void *mmap(void *addr, size_t length, int prot, int flags,
                  int fd, off_t offset);

       The contents of a file mapping (as opposed to an anonymous
       mapping; see MAP_ANONYMOUS below), are initialized using length
       bytes starting at offset offset in the file (or other object)
       referred to by the file descriptor fd.  offset must be a multiple
       of the page size as returned by sysconf(_SC_PAGE_SIZE).

I think, conceptually, "always using 0 as offset to mmap() different offset of
enclave file" is wrong.  You never encountered any issue is because SGX driver
doesn't use vma->vm_pgoff as you mentioned.

I am not entire clear about SGX driver's history, so I am not sure SGX drvier,
by design, has "relaxed semantics" of the offset parameter of mmap(), for
instance, allow it to be any value.  But to me I see no reason SGX should have
such relaxed semantics.

If you follow mmap() semantics, you won't need to manually set vma->vm_pgoff in
sgx_mmap() (which is hacky IMHO) and everything just works.

Jarkko/Dave,

Do you have any comments?