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@xxxxxxxxx> 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
