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

From: "jarkko@kernel.org" <jarkko@kernel.org>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Chatre, Reinette" <reinette.chatre@intel.com>,
	"Dhanraj, Vijay" <vijay.dhanraj@intel.com>,
	"haitao.huang@linux.intel.com" <haitao.huang@linux.intel.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Subject: Re: [RFC PATCH v4 4/4] selftests/sgx: Add test for madvise(..., WILLNEED)
Date: Sat, 18 Feb 2023 00:29:18 +0200	[thread overview]
Message-ID: <Y+//vtDmqjZjrR97@kernel.org> (raw)
In-Reply-To: <a9003c4ed2fac5a736a7317a3d4614de26bfb464.camel@intel.com>

On Wed, Feb 15, 2023 at 08:46:05AM +0000, Huang, Kai wrote:
> 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?

I think I gave my feedback in my earlier comments so yeah I agree
with aligning anonymous semantics for sure.

BR, Jarkko