Messages in this thread | | | Date | Mon, 6 Dec 2021 13:48:31 -0800 | Subject | Re: [PATCH 16/25] x86/sgx: Support modifying SGX page type | From | Reinette Chatre <> |
| |
Hi Jarkko,
On 12/4/2021 3:45 PM, Jarkko Sakkinen wrote: > On Wed, Dec 01, 2021 at 11:23:14AM -0800, Reinette Chatre wrote: >> Every enclave contains one or more Thread Control Structures (TCS). The >> TCS contains meta-data used by the hardware to save and restore thread >> specific information when entering/exiting the enclave. With SGX1 an >> enclave needs to be created with enough TCSs to support the largest >> number of threads expecting to use the enclave and enough enclave pages >> to meet all its anticipated memory demands. In SGX1 all pages remain in >> the enclave until the enclave is unloaded. >> >> Earlier changes added support for the SGX2 feature where pages can be >> added dynamically to an initialized enclave. > > Please remove this paragraph, i.e. do not tie the commit order like > this.
Will do.
>> >> SGX2 introduces a new function, ENCLS[EMODT], that is used to change >> the type of an enclave page from a regular (SGX_PAGE_TYPE_REG) enclave >> page to a TCS (SGX_PAGE_TYPE_TCS) page or change the type from a >> regular (SGX_PAGE_TYPE_REG) or TCS (SGX_PAGE_TYPE_TCS) >> page to a trimmed (SGX_PAGE_TYPE_TRIM) page (setting it up for later >> removal). >> >> With the existing support of dynamically adding regular enclave pages >> to an initialized enclave and changing the page type to TCS it is >> possible to dynamically increase the number of threads supported by an >> enclave. >> >> Changing the enclave page type to SGX_PAGE_TYPE_TRIM is the first step >> of dynamically removing pages from an initialized enclave. The complete >> page removal flow is: >> 1) Change the type of the pages to be removed to SGX_PAGE_TYPE_TRIM >> using the ioctl introduced here. >> 2) Approve the page removal by running ENCLU[EACCEPT] from within >> the enclave. >> 3) Initiate actual page removal using the new ioctl introduced in the >> following patch. >> >> Support changing SGX enclave page types with a new ioctl. With this > > What is "a new ioctl"? Why not just write "Add <ioctl name>""?
I do so to reduce the changes required during the ioctl naming discussion churn.
>> ioctl the user specifies a page range and the enclave page type to be >> applied to all pages in the provided range. The ioctl itself can return >> an error code based on failures encountered by the OS. It is also >> possible for SGX specific failures to be encountered. Add a result >> output parameter to communicate the SGX return code. It is >> possible for the enclave page type change request to fail on any page >> within the provided range. Support partial success by returning >> the number of pages that were successfully changed. >> >> After the page type is changed to SGX_PAGE_TYPE_TRIM the page continues >> to be accessible from the OS perspective with page table entries and >> internal state. The page may be moved to swap. Any invalid access >> (any access except ENCLU[EACCEPT]) will encounter a page fault with >> SGX flag set in error code until the page is removed. Removal of >> trimmed enclave pages on user request will be supported in following >> patch. Trimmed enclave pages are also removed when enclave is unloaded. >> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > > This is lacking discussion of EPCM interaction, most importanly > .MODIFY field of an EPCM entry.
I will add that. I have the same question here as in EAUG patch - would you like a duplicate description in this patch and a new patch that introduces just the __emodt() wrapper or would you be ok with all new wrappers introduced together and the detailed description of their hardware supported flows only present in the patch that uses those wrappers?
>> --- >> arch/x86/include/uapi/asm/sgx.h | 19 +++ >> arch/x86/kernel/cpu/sgx/ioctl.c | 235 ++++++++++++++++++++++++++++++++ >> 2 files changed, 254 insertions(+) >> >> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h >> index 24bebc31e336..f70caccd166c 100644 >> --- a/arch/x86/include/uapi/asm/sgx.h >> +++ b/arch/x86/include/uapi/asm/sgx.h >> @@ -31,6 +31,8 @@ enum sgx_page_flags { >> _IO(SGX_MAGIC, 0x04) >> #define SGX_IOC_PAGE_MODP \ >> _IOWR(SGX_MAGIC, 0x05, struct sgx_page_modp) >> +#define SGX_IOC_PAGE_MODT \ >> + _IOWR(SGX_MAGIC, 0x06, struct sgx_page_modt) > > I'd suggest to change this as SGX_IOC_ENCLAVE_MODIFY_TYPE.
How about SGX_IOC_ENCLAVE_MOD_TYPE to be consistent with your earlier suggestion of SGX_IOC_ENCLAVE_MOD_PROTECTIONS ?
Reinette
| |