On 3/15/21 12:14 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 15, 2021 at 09:03:21AM -0700, Dave Hansen wrote:
>> On 3/13/21 8:01 AM, Jarkko Sakkinen wrote:
>>> Reset initialized EPC pages in sgx_dirty_page_list to uninitialized state,
>>> and free them using sgx_free_epc_page(). Do two passes, as for SECS pages
>>> the first round can fail, if all child pages have not yet been removed.
>>> The driver puts all pages on startup first to sgx_dirty_page_list, as the
>>> initialization could be triggered by kexec(), meaning that pages have been
>>> reserved for active enclaves before the operation.
>>>
>>> The section local lists are redundant, as sgx_free_epc_page() figures
>>> out the correction by using epc_page->section.
>>
>> During normal runtime, the "ksgxd" daemon behaves like a version of
>> kswapd just for SGX. But, its first job is to initialize enclave
>> memory. This is done in a a separate thread because this initialization
>> can be quite slow.
>>
>> Currently, the SGX boot code places each enclave page on a
>> sgx_section-local list (init_laundry_list). Once it starts up, the
>> ksgxd code walks over that list and populates the actual SGX page allocator.
>>
>> However, the per-section structures are going away to make way for the
>> SGX NUMA allocator. There's also little need to have a per-section
>> structure; the enclave pages are all treated identically, and they can
>> be placed on the correct allocator list from metadata stoered in the
>> enclave page itself.
>>
> Is this a suggestion how to rephrase the commit message? :-)
Yep.
>>> static int ksgxd(void *p)
>>> {
>>> - int i;
>>> + struct sgx_epc_page *page;
>>> + LIST_HEAD(dirty);
>>> + int i, ret;
>>>
>>> set_freezable();
>>>
>>> /*
>>> - * Sanitize pages in order to recover from kexec(). The 2nd pass is
>>> - * required for SECS pages, whose child pages blocked EREMOVE.
>>> + * Reset initialized EPC pages in sgx_dirty_page_list to uninitialized state,
>>> + * and free them using sgx_free_epc_page().
>>
>> I'm not a fan of comments that tell us verbatim what the code does.
>
> So, what are you suggesting? Remove the whole comment or parts of it? I'm
> presuming that you suggest removing the "top-level" part.
Just please make it more relevant and informative. I can tell this is
processing 'sgx_dirty_page_list' and calling sgx_free_epc_page() from
the code. I don't need a comment to tell me that.
A better comment would say:
Take enclave pages from the init code, ensure they are clean,
and free the back into the main SGX page allocator
That tells what *role* this code plays (connecting init code to the
allocator) in a way that just verbatim telling what code is called does not.
