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?
