| Message ID | 20191009044241.3591-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86/sgx: Improve add pages ioctl | expand |
On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add > multiple pages to an enclave in a single syscall. Also provide a flag > that allows replicating a single source page to multiple target pages so > that userspace doesn't need to allocate a giant chunk of memory when > initializing things like the enlave's .bss, heap, etc... > > People that actually develop runtimes, please weigh in. Jarkko also > suggested going with a fully flexible ioctl, e.g. essentially creating an > array of the existing struct so that mrmask and/or secinfo can be unique > per page. AFAICT that's overkill and more cumbersome to use as it forces > userspace to allocate the full array. My understanding is that the > majority of enclaves will have contiguous blocks of pages with identical > mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less > flexible but easier-in-theory to use approach proposed here. > We think using the same mask for all pages (solution in this patch set) is reasonable. Although it seems odd that all pages would apply the same mask, this allows enough flexibility we can foresee. Another option acceptable to us (Intel SGX runtime) is to change it to a flag and have bit zero define whether the whole page is measured via EEXTEND. This is simpler and allows other bits reserved for future usages. However, it would fail any SGX runtime that is measuring partial page for optimization purposes.
+cc Jethro and Greg On Wed, Oct 09, 2019 at 10:28:36PM -0500, Haitao Huang wrote: > On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > >Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add > >multiple pages to an enclave in a single syscall. Also provide a flag > >that allows replicating a single source page to multiple target pages so > >that userspace doesn't need to allocate a giant chunk of memory when > >initializing things like the enlave's .bss, heap, etc... > > > >People that actually develop runtimes, please weigh in. Jarkko also > >suggested going with a fully flexible ioctl, e.g. essentially creating an > >array of the existing struct so that mrmask and/or secinfo can be unique > >per page. AFAICT that's overkill and more cumbersome to use as it forces > >userspace to allocate the full array. My understanding is that the > >majority of enclaves will have contiguous blocks of pages with identical > >mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less > >flexible but easier-in-theory to use approach proposed here. > > > We think using the same mask for all pages (solution in this patch set) is > reasonable. Although it seems odd that all pages would apply the same mask, > this allows enough flexibility we can foresee. Jethro, last time I brought this up you mentioned that it'd be nice to have an array of pages[*] instead of the repeat-for-each-page concept. Is there a use case where taking an array would provide a substantial benefit to userspace? Taking an array has downsides, and I think would actually be worse for the vast majority of use cases. E.g. for a contiguous chunk with the repeater approach struct sgx_enclave_add_pages ioc; struct sgx_secinfo secinfo; memset(&secinfo, 0, sizeof(secinfo)); secinfo.flags = sec_flags; ioc.secinfo = (unsigned long)&secinfo; ioc.mrmask = 0xFFFF; ioc.offset = offset; ioc.src = (uint64_t)data; ioc.nr_pages = nr_pages; ioc.flags = misc_flags; memset(ioc.reserved, 0, sizeof(ioc.reserved)); vs. something like this for the array approach struct sgx_enclave_add_pages ioc; unsigned long i; array = malloc(sizeof(struct sgx_enclave_add_page_entry) * nr_pages); for (i = 0; i < nr_pages; i++) { memset(entries[i].secinfo, 0, sizeof(secinfo)); entries[i].secinfo.flags = sec_flags; entries[i].mrmask = 0xFFFF; entries[i].offset = offset; entries[i].src = (uint64_t)data; entries[i].flags = misc_flags; memset(entries[i].reserved, 0, sizeof(entries[i].reserved)); } ioc.nr_pages = nr_pages; ioc.page_array = (uint64_t)array; memset(ioc.reserved, 0, sizeof(ioc.reserved)); The loop is mildly annoying, but the real killer is the array allocation. SECINFO is 64 bytes, which means each entry is 88 bytes or more, e.g. around 180kb to add an 8mb chunk of .bss or heap. My intention is/was for the multi-page add to be an opportunistic optimization, not a way to add all enclave pages in a single ioctl. [*] https://patchwork.kernel.org/patch/10977721/#22699225 > Another option acceptable to us (Intel SGX runtime) is to change it to a > flag and have bit zero define whether the whole page is measured via > EEXTEND. This is simpler and allows other bits reserved for future usages. > However, it would fail any SGX runtime that is measuring partial page for > optimization purposes. This can be an orthogonal change. I agree it makes sense to drop mrmask and instead have a SGX_ADD_PAGES_MEASURED flag to cover the whole page. Hiding the 256-byte granualarity from userspace is a good idea as it's not intrinsically tied to the SGX architecture and exists only because of latency requirements. And most of the kernel interfaces work on 4k granularity.
On Fri, Oct 11, 2019 at 07:37:25AM -0700, Sean Christopherson wrote: > +cc Jethro and Greg Good morning, I hope everyone is having or has had a pleasant weekend. > On Wed, Oct 09, 2019 at 10:28:36PM -0500, Haitao Huang wrote: > > On Tue, 08 Oct 2019 23:42:34 -0500, Sean Christopherson > > <sean.j.christopherson@intel.com> wrote: > > > > >Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add > > >multiple pages to an enclave in a single syscall. Also provide a flag > > >that allows replicating a single source page to multiple target pages so > > >that userspace doesn't need to allocate a giant chunk of memory when > > >initializing things like the enlave's .bss, heap, etc... > > > > > >People that actually develop runtimes, please weigh in. Jarkko also > > >suggested going with a fully flexible ioctl, e.g. essentially creating an > > >array of the existing struct so that mrmask and/or secinfo can be unique > > >per page. AFAICT that's overkill and more cumbersome to use as it forces > > >userspace to allocate the full array. My understanding is that the > > >majority of enclaves will have contiguous blocks of pages with identical > > >mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less > > >flexible but easier-in-theory to use approach proposed here. > > > > > We think using the same mask for all pages (solution in this patch set) is > > reasonable. Although it seems odd that all pages would apply the same mask, > > this allows enough flexibility we can foresee. > Jethro, last time I brought this up you mentioned that it'd be nice to > have an array of pages[*] instead of the repeat-for-each-page concept. > Is there a use case where taking an array would provide a substantial > benefit to userspace? Taking an array has downsides, and I think would > actually be worse for the vast majority of use cases. > > ... [ Example code removed ] ... > > The loop is mildly annoying, but the real killer is the array allocation. > SECINFO is 64 bytes, which means each entry is 88 bytes or more, e.g. around > 180kb to add an 8mb chunk of .bss or heap. > > My intention is/was for the multi-page add to be an opportunistic > optimization, not a way to add all enclave pages in a single ioctl. > > [*] https://patchwork.kernel.org/patch/10977721/#22699225 The simpler the better from our perspective. We would use the 'one-shot' method to initialize a block of pages with a set of common characteristics. Essentially constructing an image of enclave page characteristics in userspace in order to load an enclave image isn't something that we would envision doing. > > Another option acceptable to us (Intel SGX runtime) is to change it to a > > flag and have bit zero define whether the whole page is measured via > > EEXTEND. This is simpler and allows other bits reserved for future usages. > > However, it would fail any SGX runtime that is measuring partial page for > > optimization purposes. > This can be an orthogonal change. I agree it makes sense to drop > mrmask and instead have a SGX_ADD_PAGES_MEASURED flag to cover the > whole page. Hiding the 256-byte granualarity from userspace is a > good idea as it's not intrinsically tied to the SGX architecture and > exists only because of latency requirements. And most of the kernel > interfaces work on 4k granularity. Specifying the ability to measure an entire page is also a straight forward simplicity optimization that we would embrace. Have a good week. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker IDfusion, LLC SGX secured infrastructure and 4206 N. 19th Ave. autonomously self-defensive platforms. Fargo, ND 58102 PH: 701-281-1686 EMAIL: greg@idfusion.net ------------------------------------------------------------------------------ "Thank heaven for startups; without them we'd never have any advances." -- Seymour Cray
Enhance the SGX_IOC_ENCLAVE_ADD_PAGE{S} ioctl so that userspace can add multiple pages to an enclave in a single syscall. Also provide a flag that allows replicating a single source page to multiple target pages so that userspace doesn't need to allocate a giant chunk of memory when initializing things like the enlave's .bss, heap, etc... People that actually develop runtimes, please weigh in. Jarkko also suggested going with a fully flexible ioctl, e.g. essentially creating an array of the existing struct so that mrmask and/or secinfo can be unique per page. AFAICT that's overkill and more cumbersome to use as it forces userspace to allocate the full array. My understanding is that the majority of enclaves will have contiguous blocks of pages with identical mrmask and secinfo, e.g. code segments, ro data, etc..., thus the less flexible but easier-in-theory to use approach proposed here. Sean Christopherson (7): x86/sgx: Modify ADD_PAGE ioctl to take offset instead of full address selftests/x86/sgx: Update test to account for ADD_PAGE change x86/sgx: Tweak ADD_PAGE ioctl to allow adding multiple pages selftests/x86/sgx: Update enclave build flow to do multi-page add x86/sgx: Add a flag to ADD_PAGES to allow replicating the source page selftests/x86/sgx: Update selftest to account for ADD_PAGES flag selftests/x86/sgx: Add test coverage for reclaim and replicate arch/x86/include/uapi/asm/sgx.h | 25 +++++--- arch/x86/kernel/cpu/sgx/ioctl.c | 77 +++++++++++++++++------ tools/testing/selftests/x86/sgx/defines.h | 28 +++++++++ tools/testing/selftests/x86/sgx/main.c | 40 ++++++------ tools/testing/selftests/x86/sgx/sgxsign.c | 20 +++++- 5 files changed, 140 insertions(+), 50 deletions(-)