[Pal/Linux-SGX] EDMM hybrid allocation #1262
Conversation
To reduce the enclave exits, pre-allocate minimal heap from top as required by application and allocate the remaining dynamically using EDMM. As part of this commit, introduced a new manifest option `sgx.edmm_heap_prealloc_size` to pre-allocate amount of heap required by the application. Note: This change assumes there is a single heap region (also called "free") that is beneath all other statically allocated memory areas (manifest, ssa, tls. tcs, stack, sig_stack, pal). Sample memory regions with helloworld program 000000000fffc000-0000000010000000 manifest 000000000ff7c000-000000000fffc000 ssa 000000000ff6c000-000000000ff7c000 tcs 000000000ff5c000-000000000ff6c000 tls 000000000ff1c000-000000000ff5c000 stack 000000000fedc000-000000000ff1c000 stack 000000000fe9c000-000000000fedc000 stack 000000000fe5c000-000000000fe9c000 stack 000000000fe1c000-000000000fe5c000 stack 000000000fddc000-000000000fe1c000 stack 000000000fd9c000-000000000fddc000 stack 000000000fd5c000-000000000fd9c000 stack 000000000fd1c000-000000000fd5c000 stack 000000000fcdc000-000000000fd1c000 stack 000000000fc9c000-000000000fcdc000 stack 000000000fc5c000-000000000fc9c000 stack 000000000fc1c000-000000000fc5c000 stack 000000000fbdc000-000000000fc1c000 stack 000000000fb9c000-000000000fbdc000 stack 000000000fb5c000-000000000fb9c000 stack 000000000fb4c000-000000000fb5c000 sig_stack 000000000fb3c000-000000000fb4c000 sig_stack 000000000fb2c000-000000000fb3c000 sig_stack 000000000fb1c000-000000000fb2c000 sig_stack 000000000fb0c000-000000000fb1c000 sig_stack 000000000fafc000-000000000fb0c000 sig_stack 000000000faec000-000000000fafc000 sig_stack 000000000fadc000-000000000faec000 sig_stack 000000000facc000-000000000fadc000 sig_stack 000000000fabc000-000000000facc000 sig_stack 000000000faac000-000000000fabc000 sig_stack 000000000fa9c000-000000000faac000 sig_stack 000000000fa8c000-000000000fa9c000 sig_stack 000000000fa7c000-000000000fa8c000 sig_stack 000000000fa6c000-000000000fa7c000 sig_stack 000000000fa5c000-000000000fa6c000 sig_stack 000000000f9f5000-000000000fa5c000 pal 0000000000010000-000000000f9f5000 free Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: )
pal/src/host/linux-sgx/enclave_edmm.c line 82 at r1 (raw file):
/* Entire request is in pre-allocated range, so clear memory to mimic SGX2 allocation */ if (pre_allocated_count == original_count) { memset((void*)addr, 0, (pre_allocated_count * PAGE_SIZE));
SGX2 EAUG instruction zeroes EPC page and so added this memset to zero the memory for the pre-allocated heap which is not added dynamically.
python/graminelibos/sgx_sign.py line 306 at r1 (raw file):
free_areas = [] for area in areas: addr = area.addr + area.size
This seems to be a like a dead code and so I have added a new function check_memory_area_holes to check there are no holes between memory regions. If maintainers agree, I can remove this code either as part of this PR or can create a new PR.
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 50 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @vijaydhanraj)
a discussion (no related file):
Note: Currently the default value for sgx.edmm_heap_prealloc_size is set to zero. Based on experiments, need to set this to a reasonable default.
We should do this in some follow-up PR, after a lot of testing and deciding on the "good enough" value.
For now, having a default of 0 seems reasonable -- as if this feature is not enabled at all.
-- commits line 2 at r1:
Introduce .... We can fix it during final rebase.
-- commits line 2 at r1:
Also, do we want to mention hybrid here? The term is a bit vague. Maybe "EDMM pre-allocation of part of heap"?
-- commits line 8 at r1:
This needs slight rewording (remove the, minimal -> some amount of, introduced- > introduce, etc). This can be done during final rebase.
-- commits line 8 at r1:
I would also add a sentence: Currently the default value is zero. This may be changed in future commits, based on perf experiments.
-- commits line 12 at r1:
. -> ,
-- commits line 52 at r1:
This list doesn't belong to the commit message. You already have the note here, and the example is meaningless (it's enough to have it in the PR description). So we'll remove this during final rebase.
Documentation/manifest-syntax.rst line 486 at r1 (raw file):
Support for EDMM first appeared in Linux 6.0. EDMM Hybrid allocation (Experimental)
Hybrid -> hybrid (no need for capital letter)
Documentation/manifest-syntax.rst line 486 at r1 (raw file):
Support for EDMM first appeared in Linux 6.0. EDMM Hybrid allocation (Experimental)
Why do we mark it as experimental? I think we could remove this.
Documentation/manifest-syntax.rst line 495 at r1 (raw file):
When ``sgx.edmm_enable`` is enabled, users can precisely set the amount of heap to pre-allocate by setting the ``sgx.edmm_heap_prealloc_size``. For
by setting this option
Documentation/manifest-syntax.rst line 496 at r1 (raw file):
When ``sgx.edmm_enable`` is enabled, users can precisely set the amount of heap to pre-allocate by setting the ``sgx.edmm_heap_prealloc_size``. For example, when size is set to "64M" Gramine will pre-allocate top 64M of heap
Please remove top, this is an internal Gramine detail (that Gramine allocates heap pages starting from the top, so we need to also preallocate starting from the top here). Users don't need to know about this.
Documentation/manifest-syntax.rst line 497 at r1 (raw file):
to pre-allocate by setting the ``sgx.edmm_heap_prealloc_size``. For example, when size is set to "64M" Gramine will pre-allocate top 64M of heap pages rather than allocating dynamically and thus the name "Hybrid allocation".
I would remove this and thus the name .... This doesn't add any information.
Documentation/manifest-syntax.rst line 498 at r1 (raw file):
example, when size is set to "64M" Gramine will pre-allocate top 64M of heap pages rather than allocating dynamically and thus the name "Hybrid allocation". This may help to balance out the cost between load time and run time.
I think this sentence should be replaced with a more specific one: Higher values in this option improve run time but worsen startup time, and vice versa.
Documentation/manifest-syntax.rst line 841 at r1 (raw file):
large enough to hold the whole heap area. This option is invalid (i.e. must be ``false``) when only ``sgx.edmm_enable``is
Please add a space before is
Documentation/manifest-syntax.rst line 843 at r1 (raw file):
This option is invalid (i.e. must be ``false``) when only ``sgx.edmm_enable``is specified, as there are no heap pages to pre-fault. But if used together with ``sgx.edmm_heap_prealloc_size``, then the pre-allocated heap size will be
I would remove the word size here, as it reads like broken English to me.
Documentation/manifest-syntax.rst line 844 at r1 (raw file):
specified, as there are no heap pages to pre-fault. But if used together with ``sgx.edmm_heap_prealloc_size``, then the pre-allocated heap size will be pre-faulted.
I would add also (i.e., ... will also be pre-faulted.)
pal/src/host/linux-sgx/enclave_edmm.c line 35 at r1 (raw file):
/* Returns page count that is non overlapping with the pre-allocated heap. 0 means entire request * overlaps with the pre-allocated region. * Partial overlap illustration:
Rewording:
/* Updates page count such that the request is fully below the pre-allocated heap.
* If `count` is updated to 0, then the entire request overlaps with pre-allocated heap.
*
pal/src/host/linux-sgx/enclave_edmm.c line 45 at r1 (raw file):
addr <-----+ | | | +---------+
Here is my ASCII kung-fu:
+----------------------+ --> heap_max
| |
addr + size <-- | Pre-allocated heap |
| |
+----------------------+ --> edmm_heap_prealloc_start
| | (heap_max + edmm_heap_prealloc_size)
| Dynamically |
addr <-- | allocated heap |
| |
+----------------------+
pal/src/host/linux-sgx/enclave_edmm.c line 62 at r1 (raw file):
} else { /* No overlap: Return the original count */ }
I think this way is easier to read:
if (addr >= edmm_heap_prealloc_start) {
/* full overlap: entire request lies in the pre-allocated region */
*count = 0;
} else if (addr + size > edmm_heap_prealloc_start) {
/* partial overlap: update count to skip the pre-allocated region */
*count = (edmm_heap_prealloc_start - addr) / PAGE_SIZE;
} else {
/* no overlap: don't update count */
}
}
pal/src/host/linux-sgx/enclave_edmm.c line 73 at r1 (raw file):
} /* Update count with preallocated heap */
Remove this sentence, it's obvious what happens (from variable names and function names)
pal/src/host/linux-sgx/enclave_edmm.c line 78 at r1 (raw file):
exclude_preallocated_pages(addr, &count); size_t pre_allocated_count = original_count - count;
Please rename to preallocated_count
pal/src/host/linux-sgx/enclave_edmm.c line 84 at r1 (raw file):
memset((void*)addr, 0, (pre_allocated_count * PAGE_SIZE)); return 0; }
This is hard to read. This looks simpler:
if (count == 0) {
/* entire request is in pre-allocated range */
memset((void*)addr, 0, (preallocated_count * PAGE_SIZE));
return 0;
}
/* request is partially in pre-allocated range */
memset((void*)(addr + count * PAGE_SIZE), 0, pre_allocated_count * PAGE_SIZE);
pal/src/host/linux-sgx/enclave_edmm.c line 88 at r1 (raw file):
/* Partial request is part of the pre-allocated range, so just clear memory contents in * the pre-allocated range to mimic SGX2 allocation */ memset((void*)(addr + (count * PAGE_SIZE)), 0, (pre_allocated_count * PAGE_SIZE));
Too long comment and too many braces. See my code snippet above.
pal/src/host/linux-sgx/enclave_edmm.c line 138 at r1 (raw file):
int sgx_edmm_remove_pages(uint64_t addr, size_t count) { /* Update count with preallocated heap */
Please remove this comment
pal/src/host/linux-sgx/host_main.c line 719 at r1 (raw file):
} if (!enclave_info->edmm_enabled && enclave_info->edmm_heap_prealloc_size > 0) {
Why do you need these two checks here (in untrusted PAL) if you have the same checks in trusted PAL? I suggest to remove these two checks from here and keep them only in the trusted PAL.
pal/src/host/linux-sgx/host_main.c line 720 at r1 (raw file):
if (!enclave_info->edmm_enabled && enclave_info->edmm_heap_prealloc_size > 0) { log_error("sgx.edmm_heap_prealloc_size should be used along with sgx.edmm_enable!");
should -> must
along -> together
pal/src/host/linux-sgx/host_main.c line 720 at r1 (raw file):
if (!enclave_info->edmm_enabled && enclave_info->edmm_heap_prealloc_size > 0) { log_error("sgx.edmm_heap_prealloc_size should be used along with sgx.edmm_enable!");
Please put both manifest options in ''
pal/src/host/linux-sgx/host_main.c line 726 at r1 (raw file):
if (!IS_ALIGNED(enclave_info->edmm_heap_prealloc_size, g_page_size)) { log_error("edmm_heap_prealloc_size should be page aligned: %ld", g_page_size);
Please add 'sgx.'
pal/src/host/linux-sgx/host_main.c line 726 at r1 (raw file):
if (!IS_ALIGNED(enclave_info->edmm_heap_prealloc_size, g_page_size)) { log_error("edmm_heap_prealloc_size should be page aligned: %ld", g_page_size);
I see no point in printing the g_page_size variable.
Maybe just print a message with ...must be 4K (page) aligned?
pal/src/host/linux-sgx/pal_main.c line 532 at r1 (raw file):
static void do_preheat_enclave(void) { /* Heap allocation requests are serviced starting from highest heap address. So when * sgx.edmm_heap_prealloc_size is turned on preheat from the top of the heap until
Add a comma after turned on
pal/src/host/linux-sgx/pal_main.c line 582 at r1 (raw file):
if (!edmm_enabled && edmm_heap_prealloc_size > 0) { log_error("edmm_heap_prealloc_size should be used along with edmm_enabled!");
ditto
pal/src/host/linux-sgx/pal_main.c line 588 at r1 (raw file):
size_t total_heap_size = g_pal_linuxsgx_state.heap_max - g_pal_linuxsgx_state.heap_min; if (edmm_heap_prealloc_size > total_heap_size) { log_error("edmm_heap_prealloc_size should be less than total heap size 0x%lx",
ditto
pal/src/host/linux-sgx/pal_main.c line 775 at r1 (raw file):
if (g_pal_linuxsgx_state.edmm_enabled && !g_pal_linuxsgx_state.edmm_heap_prealloc_size) { log_error("'sgx.preheat_enclave' manifest option makes no sense with only EDMM enabled." " Need to enable edmm_heap_prealloc_size as well!");
Wrong indentation
pal/src/host/linux-sgx/pal_main.c line 775 at r1 (raw file):
if (g_pal_linuxsgx_state.edmm_enabled && !g_pal_linuxsgx_state.edmm_heap_prealloc_size) { log_error("'sgx.preheat_enclave' manifest option makes no sense with only EDMM enabled." " Need to enable edmm_heap_prealloc_size as well!");
I don't think you want users to force to enable this new manifest option (that's how your current sentence reads). Maybe simply:
'sgx.preheat_enclave' manifest option makes no sense with EDMM enabled
and 'sgx.edmm_heap_prealloc_size' equal to zero!
python/graminelibos/sgx_sign.py line 284 at r1 (raw file):
gen_area_content(attr, areas, enclave_base, enclave_heap_min) # Enclaves with EDMM do not add "free" memory at startup but if heap is pre-allocated
Please add a , before but
python/graminelibos/sgx_sign.py line 285 at r1 (raw file):
# Enclaves with EDMM do not add "free" memory at startup but if heap is pre-allocated # using manifest `sgx.edmm_heap_prealloc_size` add the "free" pre-allocated pages.
Remove manifest word
python/graminelibos/sgx_sign.py line 285 at r1 (raw file):
# Enclaves with EDMM do not add "free" memory at startup but if heap is pre-allocated # using manifest `sgx.edmm_heap_prealloc_size` add the "free" pre-allocated pages.
add -> then
python/graminelibos/sgx_sign.py line 286 at r1 (raw file):
# Enclaves with EDMM do not add "free" memory at startup but if heap is pre-allocated # using manifest `sgx.edmm_heap_prealloc_size` add the "free" pre-allocated pages. # PS: Assumption here is that we have a single heap region (called "free") that is beneath all
Remove PS:. Also, add an empty comment line to visually separate these two sentences.
python/graminelibos/sgx_sign.py line 291 at r1 (raw file):
free_preallocated = [] if (attr['edmm_heap_prealloc_size'] > 0 and last_populated_addr > enclave_heap_min):
Doesn't it fit into 100 chars per line? Also, no need for ()
python/graminelibos/sgx_sign.py line 291 at r1 (raw file):
free_preallocated = [] if (attr['edmm_heap_prealloc_size'] > 0 and last_populated_addr > enclave_heap_min):
Why do you compare last_populated_addr against enclave_heap_min? Shouldn't it be attr['edmm_heap_prealloc_size']?
python/graminelibos/sgx_sign.py line 292 at r1 (raw file):
if (attr['edmm_heap_prealloc_size'] > 0 and last_populated_addr > enclave_heap_min):
Please remove empty line
python/graminelibos/sgx_sign.py line 293 at r1 (raw file):
last_populated_addr > enclave_heap_min): flags = PAGEINFO_R | PAGEINFO_W | PAGEINFO_X | PAGEINFO_REG
Oops, this is a security problem! When we use sgx.edmm_heap_prealloc_size, the pre-allocated range necessarily has RWX permissions. We should at least mention it in the documentation... I have no ideas how to solve it (if at all possible).
I didn't think of it before, but it's interesting: with this hybrid allocation, we trade security for performance. We kinda "drag" the SGXv1 page-permissions problem into the top part of the heap here.
python/graminelibos/sgx_sign.py line 296 at r1 (raw file):
start_addr = last_populated_addr - attr['edmm_heap_prealloc_size'] if start_addr < enclave_heap_min: raise Exception(" sgx.edmm_heap_prealloc_size cannot be more than total heap size!")
I would re-rephase like: sgx.edmm_heap_prealloc_size is greater than total heap size!
python/graminelibos/sgx_sign.py line 306 at r1 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
This seems to be a like a dead code and so I have added a new function
check_memory_area_holesto check there are no holes between memory regions. If maintainers agree, I can remove this code either as part of this PR or can create a new PR.
I'd prefer a separate PR.
python/graminelibos/sgx_sign.py line 468 at r1 (raw file):
return 1 last_populated_addr = area.addr
This function is so small and simple -- I wouldn't use empty lines here
python/graminelibos/sgx_sign.py line 469 at r1 (raw file):
last_populated_addr = area.addr return 0
This should return True and False
python/graminelibos/sgx_sign.py line 489 at r1 (raw file):
if not attr['edmm_enable'] and attr['edmm_heap_prealloc_size'] > 0: raise Exception("sgx.edmm_heap_prealloc_size should be used along with sgx.edmm_enable!")
should -> must
along -> together
python/graminelibos/sgx_sign.py line 494 at r1 (raw file):
is_aligned(attr['edmm_heap_prealloc_size'], offs.PAGESIZE) == 0: raise Exception("sgx.edmm_heap_prealloc_size: {0} should be greater than or equal to 0!" .format(attr['edmm_heap_prealloc_size']))
I suggest to remove this whole IF. I don't think the Python code must have such checks, that's just too much hand-holding of manifest writers.
python/graminelibos/sgx_sign.py line 536 at r1 (raw file):
memory_areas = populate_memory_areas(attr, memory_areas, enclave_base, enclave_heap_min) # Ensure no holes in the populated memory areas
The comment looks redundant, please remove
python/graminelibos/sgx_sign.py line 539 at r1 (raw file):
memory_area_holes = check_memory_area_holes(attr, memory_areas, enclave_base) if memory_area_holes: raise Exception('Cannot have holes in memory areas!')
in enclave memory areas
There was a problem hiding this comment.
Reviewable status: 7 of 12 files reviewed, 50 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Also, do we want to mention
hybridhere? The term is a bit vague. Maybe "EDMM pre-allocation of part of heap"?
How about Introduce EDMM pre-allocated heap optimization?
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Introduce .... We can fix it during final rebase.
ok, thanks.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This needs slight rewording (remove
the,minimal->some amount of,introduced- >introduce, etc). This can be done during final rebase.
ok, thanks.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would also add a sentence:
Currently the default value is zero. This may be changed in future commits, based on perf experiments.
sure, can you please fix this as part of the final rebase?
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
.->,
Can you please fix this as part of the final rebase?
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This list doesn't belong to the commit message. You already have the note here, and the example is meaningless (it's enough to have it in the PR description). So we'll remove this during final rebase.
ok, thanks.
Documentation/manifest-syntax.rst line 486 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hybrid->hybrid(no need for capital letter)
Done but should we rename this EDMM pre-allocate heap?
Documentation/manifest-syntax.rst line 486 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we mark it as experimental? I think we could remove this.
Done.
Documentation/manifest-syntax.rst line 495 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
by setting this option
Done.
Documentation/manifest-syntax.rst line 496 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove
top, this is an internal Gramine detail (that Gramine allocates heap pages starting from the top, so we need to also preallocate starting from the top here). Users don't need to know about this.
Done.
Documentation/manifest-syntax.rst line 497 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would remove this
and thus the name .... This doesn't add any information.
Done.
Documentation/manifest-syntax.rst line 498 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this sentence should be replaced with a more specific one:
Higher values in this option improve run time but worsen startup time, and vice versa.
Done with minor rewording. The following is the current text,
When ``sgx.edmm_enable`` is enabled, users can precisely set the amount of heap to pre-allocate by setting this option. For example, when size is set to "64M" Gramine will pre-allocate 64M of heap pages to the enclave (similar to SGXv1) rather than allocating dynamically them. Higher values in this option may improve run time but could worsen startup time, and vice versa.
Documentation/manifest-syntax.rst line 841 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a space before
is
Done.
Documentation/manifest-syntax.rst line 843 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would remove the word
sizehere, as it reads like broken English to me.
Done.
Documentation/manifest-syntax.rst line 844 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would add
also(i.e.,... will also be pre-faulted.)
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 35 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Rewording:
/* Updates page count such that the request is fully below the pre-allocated heap. * If `count` is updated to 0, then the entire request overlaps with pre-allocated heap. *
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 45 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Here is my ASCII kung-fu:
+----------------------+ --> heap_max | | addr + size <-- | Pre-allocated heap | | | +----------------------+ --> edmm_heap_prealloc_start | | (heap_max + edmm_heap_prealloc_size) | Dynamically | addr <-- | allocated heap | | | +----------------------+
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this way is easier to read:
if (addr >= edmm_heap_prealloc_start) { /* full overlap: entire request lies in the pre-allocated region */ *count = 0; } else if (addr + size > edmm_heap_prealloc_start) { /* partial overlap: update count to skip the pre-allocated region */ *count = (edmm_heap_prealloc_start - addr) / PAGE_SIZE; } else { /* no overlap: don't update count */ } }
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 73 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove this sentence, it's obvious what happens (from variable names and function names)
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 78 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rename to
preallocated_count
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 84 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is hard to read. This looks simpler:
if (count == 0) { /* entire request is in pre-allocated range */ memset((void*)addr, 0, (preallocated_count * PAGE_SIZE)); return 0; } /* request is partially in pre-allocated range */ memset((void*)(addr + count * PAGE_SIZE), 0, pre_allocated_count * PAGE_SIZE);
OK, done.
pal/src/host/linux-sgx/enclave_edmm.c line 88 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Too long comment and too many braces. See my code snippet above.
Done.
pal/src/host/linux-sgx/enclave_edmm.c line 138 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove this comment
Done.
pal/src/host/linux-sgx/host_main.c line 719 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need these two checks here (in untrusted PAL) if you have the same checks in trusted PAL? I suggest to remove these two checks from here and keep them only in the trusted PAL.
Added the checks in the untrusted PAL because, it here that we EADD the pages to enclave (as part of initialize_enclave()). Any incorrect values may result in EADD failing and it may not be clear why it failed.
pal/src/host/linux-sgx/host_main.c line 720 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
should->must
along->together
Done.
Code quote:
sgx.edmm_heap_prealloc_size should be used along with sgx.edmm_enable!pal/src/host/linux-sgx/host_main.c line 720 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please put both manifest options in
''
Done.
pal/src/host/linux-sgx/host_main.c line 726 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add
'sgx.'
Done.
pal/src/host/linux-sgx/host_main.c line 726 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I see no point in printing the
g_page_sizevariable.Maybe just print a message with
...must be 4K (page) aligned?
Done.
pal/src/host/linux-sgx/pal_main.c line 532 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Add a comma after
turned on
Done.
pal/src/host/linux-sgx/pal_main.c line 582 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
pal/src/host/linux-sgx/pal_main.c line 588 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
pal/src/host/linux-sgx/pal_main.c line 775 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wrong indentation
Done.
pal/src/host/linux-sgx/pal_main.c line 775 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think you want users to force to enable this new manifest option (that's how your current sentence reads). Maybe simply:
'sgx.preheat_enclave' manifest option makes no sense with EDMM enabled and 'sgx.edmm_heap_prealloc_size' equal to zero!
Done.
python/graminelibos/sgx_sign.py line 284 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a
,beforebut
Done.
python/graminelibos/sgx_sign.py line 285 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove
manifestword
Done.
python/graminelibos/sgx_sign.py line 285 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
add->then
you mean to add then before add? Something like below?
# Enclaves with EDMM do not add "free" memory at startup, but if heap is pre-allocated
# using `sgx.edmm_heap_prealloc_size` then add the "free" pre-allocated pages.
python/graminelibos/sgx_sign.py line 286 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove
PS:. Also, add an empty comment line to visually separate these two sentences.
Done.
python/graminelibos/sgx_sign.py line 291 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Doesn't it fit into 100 chars per line? Also, no need for
()
Yes, it does. I have fixed it.
python/graminelibos/sgx_sign.py line 291 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you compare
last_populated_addragainstenclave_heap_min? Shouldn't it beattr['edmm_heap_prealloc_size']?
I adopted this check from https://github.com/gramineproject/gramine/blob/master/python/graminelibos/sgx_sign.py#L294. But I feel we should at least throw a warning. So have modified the code to be as below. Does this make sense?
if attr['edmm_enable']:
free_preallocated = []
- if (attr['edmm_heap_prealloc_size'] > 0 and
- last_populated_addr > enclave_heap_min):
+ if attr['edmm_heap_prealloc_size'] > 0:
+
+ if last_populated_addr < attr['edmm_heap_prealloc_size']:
+ raise Exception("Not enough space for edmm heap pre-allocation!")
+
+ if last_populated_addr < enclave_heap_min:
+ raise Exception("No space for heap!! Please check your manifest!")
flags = PAGEINFO_R | PAGEINFO_W | PAGEINFO_X | PAGEINFO_REG
start_addr = last_populated_addr - attr['edmm_heap_prealloc_size']
if start_addr < enclave_heap_min:
- raise Exception(" sgx.edmm_heap_prealloc_size cannot be more than total heap size!")
+ raise Exception("sgx.edmm_heap_prealloc_size cannot be more than total heap size!")
python/graminelibos/sgx_sign.py line 292 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove empty line
Done.
python/graminelibos/sgx_sign.py line 293 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Oops, this is a security problem! When we use
sgx.edmm_heap_prealloc_size, the pre-allocated range necessarily has RWX permissions. We should at least mention it in the documentation... I have no ideas how to solve it (if at all possible).I didn't think of it before, but it's interesting: with this hybrid allocation, we trade security for performance. We kinda "drag" the SGXv1 page-permissions problem into the top part of the heap here.
Yes true, we do bring in SGX1, page permission limitations.
python/graminelibos/sgx_sign.py line 296 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would re-rephase like:
sgx.edmm_heap_prealloc_size is greater than total heap size!
Done.
python/graminelibos/sgx_sign.py line 306 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'd prefer a separate PR.
OK, will do.
python/graminelibos/sgx_sign.py line 468 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This function is so small and simple -- I wouldn't use empty lines here
Done.
python/graminelibos/sgx_sign.py line 469 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should return
TrueandFalse
Done.
python/graminelibos/sgx_sign.py line 489 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
should->must
along->together
Done.
python/graminelibos/sgx_sign.py line 494 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I suggest to remove this whole IF. I don't think the Python code must have such checks, that's just too much hand-holding of manifest writers.
I have removed this and also helper function is_aligned
python/graminelibos/sgx_sign.py line 536 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The comment looks redundant, please remove
Done.
python/graminelibos/sgx_sign.py line 539 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
in enclave memory areas
Done.
There was a problem hiding this comment.
Reviewable status: 7 of 12 files reviewed, 54 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @vijaydhanraj)
Documentation/manifest-syntax.rst line 842 at r2 (raw file):
This option is invalid (i.e. must be ``false``) when only ``sgx.edmm_enable`` is specified, as there are no heap pages to pre-fault. But if used together with
But also when sgx.edmm_heap_prealloc_size is set to 0?
Code quote:
This option is invalid (i.e. must be ``false``) when only ``sgx.edmm_enable`` is
specified, as there are no heap pages to pre-fault. But if used together withpal/src/host/linux-sgx/enclave_edmm.c line 43 at r2 (raw file):
| | +----------------------+ --> edmm_heap_prealloc_start | | (heap_max + edmm_heap_prealloc_size)
Why not -?
Code quote:
+pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
/* Request is partially in pre-allocated range */ memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE); }
Can we simply do memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE);?
Code quote:
if (preallocated_count != 0) {
/* Entire request is in pre-allocated range */
if (count == 0) {
memset((void*)addr, 0, preallocated_count * PAGE_SIZE);
return 0;
}
/* Request is partially in pre-allocated range */
memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE);
}pal/src/host/linux-sgx/host_main.c line 499 at r2 (raw file):
} else { if (enclave->edmm_heap_prealloc_size > areas[i].size) { log_error("edmm_heap_prealloc_size should be less than total heap size 0x%lx",
-> must; or should we actually align with the "edmm_heap_prealloc_size is greater than total heap size!" pattern in all similar places, as suggested by @dimakuv in python/graminelibos/sgx_sign.py?
Code quote:
shouldpython/graminelibos/sgx_sign.py line 293 at r1 (raw file):
We should at least mention it in the documentation...
+1
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
Documentation/manifest-syntax.rst line 486 at r2 (raw file):
Support for EDMM first appeared in Linux 6.0. EDMM heap pre-allocate size
pre-allocate -> pre-allocated
Documentation/manifest-syntax.rst line 497 at r2 (raw file):
to pre-allocate by setting this option. For example, when size is set to "64M" Gramine will pre-allocate 64M of heap pages to the enclave (similar to SGXv1) rather than allocating dynamically them. Higher values in this option may
Better wording:
... For example, when size is set to "64M",
Gramine will pre-allocate 64M of enclave memory (with read-write-execute
permissions!). Higher values in this option may ...
pal/src/host/linux-sgx/enclave_edmm.c line 84 at r1 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
OK, done.
Why do you move the first comment out of the IF? I find it very counter-intuitive to read...
pal/src/host/linux-sgx/enclave_edmm.c line 35 at r2 (raw file):
/* Updates page count such that the request is fully below the pre-allocated * heap. If `count` is updated to 0, then the entire request overlaps with * pre-allocated heap.
This doesn't look 100-chars-per-line?
pal/src/host/linux-sgx/enclave_edmm.c line 43 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Why not
-?
Yes, should be -. It was my mistake in the other comment.
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Can we simply do
memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE);?
@kailun-qin We need to return 0 in the first case, and we need to continue with SGX2 flows in the second case. That's why this distinction. I agree that memset() could be done only once, but the IFs are still required.
pal/src/host/linux-sgx/host_main.c line 719 at r1 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Added the checks in the untrusted PAL because, it here that we
EADDthe pages to enclave (as part ofinitialize_enclave()). Any incorrect values may result inEADDfailing and it may not be clear why it failed.
Ah, I see your point.
pal/src/host/linux-sgx/host_main.c line 499 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
->
must; or should we actually align with the "edmm_heap_prealloc_size is greater than total heap size!" pattern in all similar places, as suggested by @dimakuv inpython/graminelibos/sgx_sign.py?
Agree with @kailun-qin, let's just reuse the same message everywhere
python/graminelibos/sgx_sign.py line 285 at r1 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
you mean to add
thenbeforeadd? Something like below?# Enclaves with EDMM do not add "free" memory at startup, but if heap is pre-allocated # using `sgx.edmm_heap_prealloc_size` then add the "free" pre-allocated pages.
Yes, sorry for confusion.
python/graminelibos/sgx_sign.py line 284 at r2 (raw file):
# # Assumption here is that we have a single heap region (called "free") that is beneath all # other statically allocated memory areas(manifest, ssa, tls. tcs, stack, sig_stack, pal).
tls. -> tls,
python/graminelibos/sgx_sign.py line 290 at r2 (raw file):
if last_populated_addr < attr['edmm_heap_prealloc_size']: raise Exception("Not enough space for edmm heap pre-allocation!")
We need to give some guidance to users what to do. Maybe add:
Increase 'sgx.enclave_size' or decrease 'sgx.edmm_heap_prealloc_size' in the manifest.
python/graminelibos/sgx_sign.py line 293 at r2 (raw file):
if last_populated_addr < enclave_heap_min: raise Exception("No space for heap!! Please check your manifest!")
No space left for the heap! Increase 'sgx.enclave_size' in the manifest.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @vijaydhanraj)
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
We need to return 0 in the first case, and we need to continue with SGX2 flows in the second case. That's why this distinction.> the IFs are still required
Yes, I understand - sorry that I didn't write down the complete version. I was actually thinking about sth. like below:
memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE);
if (count == 0)
return 0;
the IFs are still required
which memset() in any case but directly returns afterwards only if the entire request is in the pre-allocated range
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
We need to return 0 in the first case, and we need to continue with SGX2 flows in the second case. That's why this distinction.> the IFs are still required
Yes, I understand - sorry that I didn't write down the complete version. I was actually thinking about sth. like below:
memset((void*)(addr + count * PAGE_SIZE), 0, preallocated_count * PAGE_SIZE); if (count == 0) return 0;the IFs are still required
which
memset()in any case but directly returns afterwards only if the entire request is in the pre-allocated range
Yeah, I'd be fine with this change
There was a problem hiding this comment.
Reviewable status: 8 of 12 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
Documentation/manifest-syntax.rst line 486 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
pre-allocate->pre-allocated
Done.
Documentation/manifest-syntax.rst line 497 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Better wording:
... For example, when size is set to "64M", Gramine will pre-allocate 64M of enclave memory (with read-write-execute permissions!). Higher values in this option may ...
Done. I removed the ! as it sounds like an alarm and instead referenced the similarity to SGXv1.
Documentation/manifest-syntax.rst line 842 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
But also when
sgx.edmm_heap_prealloc_sizeis set to 0?
I don't think we need to explicitly callout as sgx.edmm_heap_prealloc_size = 0 means the feature is off and our text reads "when only sgx.edmm_enable is ....". So, looks fine to me but open to adding additional text as well if it isn't clear.
pal/src/host/linux-sgx/enclave_edmm.c line 84 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you move the first comment out of the IF? I find it very counter-intuitive to read...
Moved it inside the if statement.
pal/src/host/linux-sgx/enclave_edmm.c line 35 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This doesn't look 100-chars-per-line?
yes, re-wrapped it.
pal/src/host/linux-sgx/enclave_edmm.c line 43 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, should be
-. It was my mistake in the other comment.
Not sure but have replaced + with -. Is this what we want?
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yeah, I'd be fine with this change
Done.
pal/src/host/linux-sgx/host_main.c line 499 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Agree with @kailun-qin, let's just reuse the same message everywhere
Since heap size can vary, it will be better if we can print the actual heap size and tell the user to set a lesser value. So, the current print message may be better. I have changed even in sgx_sign.py file for uniformity. Let me know if we want to revert it.
python/graminelibos/sgx_sign.py line 284 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
tls.->tls,
Done.
python/graminelibos/sgx_sign.py line 290 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We need to give some guidance to users what to do. Maybe add:
Increase 'sgx.enclave_size' or decrease 'sgx.edmm_heap_prealloc_size' in the manifest.
Done.
python/graminelibos/sgx_sign.py line 293 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No space left for the heap! Increase 'sgx.enclave_size' in the manifest.
Done.
There was a problem hiding this comment.
Reviewable status: 8 of 12 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
pal/src/host/linux-sgx/enclave_edmm.c line 43 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Not sure but have replaced
+with-. Is this what we want?
Misunderstood the comment earlier and fixed the typo.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
Documentation/manifest-syntax.rst line 497 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Done. I removed the
!as it sounds like an alarm and instead referenced the similarity to SGXv1.
Yes, thanks, your variant reads much better.
Documentation/manifest-syntax.rst line 842 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
I don't think we need to explicitly callout as
sgx.edmm_heap_prealloc_size = 0means the feature is off and our text reads "when onlysgx.edmm_enableis ....". So, looks fine to me but open to adding additional text as well if it isn't clear.
I agree with Vijay, Kailun's proposed addition sounds not important (as it should be obvious to readers).
pal/src/host/linux-sgx/host_main.c line 499 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Since heap size can vary, it will be better if we can print the actual heap size and tell the user to set a lesser value. So, the current print message may be better. I have changed even in
sgx_sign.pyfile for uniformity. Let me know if we want to revert it.
Looks good to me.
pal/src/host/linux-sgx/host_main.c line 499 at r3 (raw file):
} else { if (enclave->edmm_heap_prealloc_size > areas[i].size) { log_error("'sgx.edmm_heap_prealloc_size' must be less than total heap size 0x%lx",
Please re-wrap to 100 chars per line
python/graminelibos/sgx_sign.py line 295 at r3 (raw file):
if last_populated_addr < enclave_heap_min: raise Exception("No space for heap!! Increase 'sgx.enclave_size' in the manifest.")
Please !! to ! (leave only one exclamatory mark)
There was a problem hiding this comment.
Reviewable status: 10 of 12 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
a discussion (no related file):
@dimakuv @kailun-qin if you are done with initial round of reviews for this PR, then can you please trigger EDMM CI pipeline with sgx.edmm_heap_prealloc_size say set to 64M?
If things look fine, we can start with performance evaluation.
pal/src/host/linux-sgx/host_main.c line 499 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please re-wrap to 100 chars per line
Done.
python/graminelibos/sgx_sign.py line 295 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please
!!to!(leave only one exclamatory mark)
Done.
There was a problem hiding this comment.
Reviewable status: 10 of 12 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @vijaydhanraj)
a discussion (no related file):
then can you please trigger EDMM CI pipeline
Sure
with sgx.edmm_heap_prealloc_size say set to 64M?
But I don't think we can trigger CI pipelines w/ a specific manifest option.
How to test this PR?
Regular EDMM CI pipeline with sgx.edmm_heap_prealloc_size.
If we'd like to cover/test this in EDMM CI, we may need some updates then.
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
How about
Introduce EDMM pre-allocated heap optimization?
Maybe "Introduce EDMM heap pre-allocation"?
Documentation/manifest-syntax.rst line 842 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with Vijay, Kailun's proposed addition sounds not important (as it should be obvious to readers).
Sure, I don't insist on this. Let me resolve then.
pal/src/host/linux-sgx/enclave_edmm.c line 43 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Misunderstood the comment earlier and fixed the typo.
Yep, I meant the - in the calculation of edmm_heap_prealloc_start. Sorry about the misunderstanding.
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Done.
I guess the IF for preallocated_count != 0 is even not needed. Anyway, I don't insist :).
pal/src/host/linux-sgx/host_main.c line 499 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Looks good to me.
Looks good to me, resolving.
python/graminelibos/sgx_sign.py line 466 at r4 (raw file):
def check_memory_area_holes(attr, areas, enclave_base):
What about check_non_contiguous_memory_areas()?
Code quote:
check_memory_area_holespython/graminelibos/sgx_sign.py line 536 at r4 (raw file):
memory_area_holes = check_memory_area_holes(attr, memory_areas, enclave_base) if memory_area_holes:
-->
if check_non_contiguous_memory_areas(...):
raise Exception('Cannot have holes in enclave memory areas!')
Code quote:
memory_area_holes = check_memory_area_holes(attr, memory_areas, enclave_base)
if memory_area_holes:python/graminelibos/sgx_sign.py line 537 at r4 (raw file):
memory_area_holes = check_memory_area_holes(attr, memory_areas, enclave_base) if memory_area_holes: raise Exception('Cannot have holes in enclave memory areas!')
between?
Code quote:
inThere was a problem hiding this comment.
Reviewable status: 10 of 12 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
a discussion (no related file):
Previously, kailun-qin (Kailun Qin) wrote…
then can you please trigger EDMM CI pipeline
Sure
with sgx.edmm_heap_prealloc_size say set to 64M?
But I don't think we can trigger CI pipelines w/ a specific manifest option.
How to test this PR?
Regular EDMM CI pipeline with sgx.edmm_heap_prealloc_size.If we'd like to cover/test this in EDMM CI, we may need some updates then.
I do have a patch where I have set sgx.edmm_heap_prealloc_size = 64M for all the PAL and LibOS regression test manifest templates. I can also set this manifest option in https://github.com/gramineproject/gramine/blob/master/libos/test/ltp/manifest.template. Would this help?
Previously, kailun-qin (Kailun Qin) wrote…
Maybe "Introduce EDMM heap pre-allocation"?
Yes, it looks good to me.
pal/src/host/linux-sgx/enclave_edmm.c line 87 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I guess the IF for
preallocated_count != 0is even not needed. Anyway, I don't insist :).
The preallocated_count != 0 check ensures that we do memset only on statically allocated memory (SGXv1). If we remove this check, we will end up doing memset even for dynamically allocated pages which might lead to a seg-fault.
python/graminelibos/sgx_sign.py line 466 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
What about
check_non_contiguous_memory_areas()?
If this is preferred, I can update the function.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
a discussion (no related file):
But I don't think we can trigger CI pipelines w/ a specific manifest option.
Yes, we can't do that.
LTP is not tested in SGX mode (only in direct mode). Thus it won't help.
You can add this option to LibOS regression's manifest.template common file. It should be protected with env.get('EDMM'), similarly to this line:
python/graminelibos/sgx_sign.py line 466 at r4 (raw file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
If this is preferred, I can update the function.
I don't care about the name, I think the current one is also fine.
python/graminelibos/sgx_sign.py line 536 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
-->
if check_non_contiguous_memory_areas(...): raise Exception('Cannot have holes in enclave memory areas!')
Yes, let's simplify as Kailun suggested (remove the temporary var memory_area_holes)
But now the function name will be ambigious. So I suggest to rename to has_memory_area_holes()
python/graminelibos/sgx_sign.py line 537 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
between?
+1 to between
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But I don't think we can trigger CI pipelines w/ a specific manifest option.
Yes, we can't do that.
LTP is not tested in SGX mode (only in
directmode). Thus it won't help.You can add this option to LibOS regression's
manifest.templatecommon file. It should be protected withenv.get('EDMM'), similarly to this line:
Something like this sgx.edmm_heap_prealloc_size = {{ "\"64M\"" if env.get('EDMM', '0') == '1' else "\"0\"" }}?
And why only manifest.template? This should be added for other *.template files in PAL and LibOS as well, right? If this is fine, I can create a temporary commit with this manifest option enabled.
python/graminelibos/sgx_sign.py line 466 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't care about the name, I think the current one is also fine.
Changed to has_memory_area_holes
python/graminelibos/sgx_sign.py line 536 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, let's simplify as Kailun suggested (remove the temporary var
memory_area_holes)But now the function name will be ambigious. So I suggest to rename to
has_memory_area_holes()
Done.
python/graminelibos/sgx_sign.py line 537 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 to
between
Done.
This commit enables `sgx.edmm_heap_prealloc_size` manifest option on CI platforms that support EDMM. Currently this option is set only for a subset of LibOS regression tests for verification. Signed-off-by: Vijay Dhanraj <vijay.dhanraj@intel.com>
There was a problem hiding this comment.
Reviewable status: 11 of 13 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
a discussion (no related file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
Something like this
sgx.edmm_heap_prealloc_size = {{ "\"64M\"" if env.get('EDMM', '0') == '1' else "\"0\"" }}?
And why onlymanifest.template? This should be added for other *.template files inPALandLibOSas well, right? If this is fine, I can create a temporary commit with this manifest option enabled.
Created a new commit to enable sgx.edmm_heap_prealloc_size in LibOS regression's manifest.template file.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
libos/test/regression/manifest.template line 27 at r5 (raw file):
sgx.debug = true sgx.edmm_enable = {{ 'true' if env.get('EDMM', '0') == '1' else 'false' }} sgx.edmm_heap_prealloc_size = {{ "\"64M\"" if env.get('EDMM', '0') == '1' else "\"0\"" }}
I guess you could move the " outside of the Jinja syntax, and then you wouldn't require this ugly escaping:
sgx.edmm_heap_prealloc_size = "{{ '64M' if env.get('EDMM', '0') == '1' else '0' }}"
Please check if it correctly renders, and if yes, replace as suggested.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
libos/test/regression/manifest.template line 27 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I guess you could move the
"outside of the Jinja syntax, and then you wouldn't require this ugly escaping:sgx.edmm_heap_prealloc_size = "{{ '64M' if env.get('EDMM', '0') == '1' else '0' }}"Please check if it correctly renders, and if yes, replace as suggested.
Looks much better, thanks!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @vijaydhanraj)
a discussion (no related file):
@vijaydhanraj Do we have plans to update the manifest template files for CI-Examples? As in add the following to all manifest template files: sgx.edmm_heap_prealloc_size = "{{ '64M' if env.get('EDMM', '0') == '1' else '0' }}"
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
a discussion (no related file):
Previously, jinengandhi-intel wrote…
@vijaydhanraj Do we have plans to update the manifest template files for CI-Examples? As in add the following to all manifest template files:
sgx.edmm_heap_prealloc_size = "{{ '64M' if env.get('EDMM', '0') == '1' else '0' }}"
@jinengandhi-intel I think it's too early for this, because we'll need to first figure out the best "preallocated size" value for each example. E.g., something like Helloworld example doesn't need this at all, but something like PyTorch or TensorFlow would probably need 256M or even more.
I would say adding this new option to our examples/workloads is a future task, after we find the best values.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @vijaydhanraj)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@jinengandhi-intel I think it's too early for this, because we'll need to first figure out the best "preallocated size" value for each example. E.g., something like Helloworld example doesn't need this at all, but something like PyTorch or TensorFlow would probably need
256Mor even more.I would say adding this new option to our examples/workloads is a future task, after we find the best values.
@vijaydhanraj would you be running these experiments or you need help from the team here? From what I understand we would need to check functionality as well as evaluate performance for all workloads that we currently have baselines for. Last release when we had enabled EDMM, perf with and without EDMM was almost the same for most workloads. With this PR we expect to see some perf improvements, please correct if I am wrong.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @jinengandhi-intel, and @kailun-qin)
a discussion (no related file):
@jinengandhi-intel I will need help with running these experiments from the team and yes, we need to evaluate both functionality and performance.
Last release when we had enabled EDMM, perf with and without EDMM was almost the same for most workloads. With this PR we expect to see some perf improvements, please correct if I am wrong.
We need to investigate 2 aspects of performance, 1) load time performance 2) runtime performance. With existing naive support, we would see improvement in load time, but the runtime performance may be impacted. This may be the reason why few workloads failed with timeout errors.
With this PR, we are trying to balance between load time and runtime. So, with higher sgx.edmm_heap_prealloc_size the load time will be impacted but the runtime will improve and vice versa. The goal of the performance evaluation would be to find a sweet spot between load time and runtime.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @vijaydhanraj)
a discussion (no related file):
Previously, vijaydhanraj (Vijay Dhanraj) wrote…
@jinengandhi-intel I will need help with running these experiments from the team and yes, we need to evaluate both functionality and performance.
Last release when we had enabled EDMM, perf with and without EDMM was almost the same for most workloads. With this PR we expect to see some perf improvements, please correct if I am wrong.
We need to investigate 2 aspects of performance, 1) load time performance 2) runtime performance. With existing naive support, we would see improvement in load time, but the runtime performance may be impacted. This may be the reason why few workloads failed with timeout errors.
With this PR, we are trying to balance between load time and runtime. So, with higher
sgx.edmm_heap_prealloc_sizethe load time will be impacted but the runtime will improve and vice versa. The goal of the performance evaluation would be to find a sweet spot between load time and runtime.
@vijaydhanraj To begin with I did some functional validation with the PR which included running LTP tests with Gramine SGX and EDMM enabled and all workloads from CI-Examples as well as in the examples repo. For all of the above tests the manifest file was configured with the default heap size of 64M and I didn't see any functional issues. Validation was done on Ubuntu22.04 as well as RHEL8. So from a functionality standpoint 64M size worked for me.
In parallel Vasanth from my team has started the perf runs but this will take time as there are many workloads and each of them needs to be run with different heap size to find the best configuration. Will update once we have some results to share.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @vijaydhanraj)
a discussion (no related file):
I would like to block this PR until we resolve a high-level discussion: given #1513, do we want this hacky hybrid-allocation approach? This approach has two issues:
- Security -- preallocated pages must have RWX permissions.
- Cleanliness -- this approach relies on the fact that initially used memory is allocated from the top of the enclave. This may bite us when we modify this assumption.
With #1513, I think we could re-use the lazy-allocation flag from there, to introduce a similar optimization to this hybrid-alloc, but using the same generic mechanism (the bitvector of committed pages) as in #1513.
I am very much in favor of re-using #1513 rather than this PR.
There was a problem hiding this comment.
It seems that the pre-allocated heap memory is only guaranteed to be used when ASLR is disabled in this approach? By default ASLR is enabled and the top address is changed and not the same as heap_max.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @vijaydhanraj)
Description of the changes
To reduce the enclave exits, pre-allocate minimal heap from top as required by application and allocate the remaining dynamically using EDMM. As part of this commit, introduced a new manifest option
sgx.edmm_heap_prealloc_sizeto pre-allocate amount of heap required by the application.Note: This change assumes there is a single heap region (also called "free") that is beneath all other statically allocated memory areas (manifest, ssa, tls. tcs, stack, sig_stack, pal).
Sample memory regions with
helloworldprogramFixes #1221
How to test this PR?
Regular EDMM CI pipeline with
sgx.edmm_heap_prealloc_size.Note: Currently the default value for
sgx.edmm_heap_prealloc_sizeis set to zero. Based on experiments, need to set this to a reasonable default.This change is