| Message ID | fd9ab4d760a2ea7a42ab9e60b9e19b8620abe11d.1638381245.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx and selftests/sgx: Support SGX2 | expand |
On Wed, Dec 01, 2021 at 11:22:59AM -0800, Reinette Chatre wrote: > The SGX ENCLS instruction uses EAX to specify an SGX function and > may require additional registers, depending on the SGX function. > ENCLS invokes the specified privileged SGX function for managing > and debugging enclaves. Macros are used to wrap the ENCLS > functionality and several wrappers are used to wrap the macros to > make the different SGX functions accessible in the code. > > The wrappers of the supported SGX functions are cryptic. Add short > changelog descriptions of each to a comment. I think you are adding function descriptions. > Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > arch/x86/kernel/cpu/sgx/encls.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > index 9b204843b78d..241b766265d3 100644 > --- a/arch/x86/kernel/cpu/sgx/encls.h > +++ b/arch/x86/kernel/cpu/sgx/encls.h > @@ -162,57 +162,68 @@ static inline bool encls_failed(int ret) > ret; \ > }) > > +/* Create an SECS page in the Enclave Page Cache (EPC) */ > static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs) > { > return __encls_2(ECREATE, pginfo, secs); > } You have: * "Create an SECS page in the Enclave Page Cache (EPC)" * "Add a Version Array (VA) page to the Enclave Page Cache (EPC)" They should have similar descriptions, e.g. * "Initialize an EPC page into SGX Enclave Control Structure (SECS) page." * "Initialize an EPC page into Version Array (VA) page." > +/* Extend uninitialized enclave measurement */ > static inline int __eextend(void *secs, void *addr) > { > return __encls_2(EEXTEND, secs, addr); > } That description does not make __eextend any less cryptic. Something like this would be already more informative: /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */ This same remark applies to the rest of these comments. They should provide a clue what the wrapper does rather than an English open coded function name. /Jarkko
Hi Jarkko, On 12/4/2021 10:30 AM, Jarkko Sakkinen wrote: > On Wed, Dec 01, 2021 at 11:22:59AM -0800, Reinette Chatre wrote: >> The SGX ENCLS instruction uses EAX to specify an SGX function and >> may require additional registers, depending on the SGX function. >> ENCLS invokes the specified privileged SGX function for managing >> and debugging enclaves. Macros are used to wrap the ENCLS >> functionality and several wrappers are used to wrap the macros to >> make the different SGX functions accessible in the code. >> >> The wrappers of the supported SGX functions are cryptic. Add short >> changelog descriptions of each to a comment. > > I think you are adding function descriptions. Will change. > >> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> arch/x86/kernel/cpu/sgx/encls.h | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h >> index 9b204843b78d..241b766265d3 100644 >> --- a/arch/x86/kernel/cpu/sgx/encls.h >> +++ b/arch/x86/kernel/cpu/sgx/encls.h >> @@ -162,57 +162,68 @@ static inline bool encls_failed(int ret) >> ret; \ >> }) >> >> +/* Create an SECS page in the Enclave Page Cache (EPC) */ >> static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs) >> { >> return __encls_2(ECREATE, pginfo, secs); >> } > > You have: > > * "Create an SECS page in the Enclave Page Cache (EPC)" > * "Add a Version Array (VA) page to the Enclave Page Cache (EPC)" > > They should have similar descriptions, e.g. > > * "Initialize an EPC page into SGX Enclave Control Structure (SECS) page." > * "Initialize an EPC page into Version Array (VA) page." Will do. Did you intentionally omit the articles or would you be ok if I change it to: "Initialize an EPC page into an SGX Enclave Control Structure (SECS) page." "Initialize an EPC page into a Version Array (VA) page." I also notice that you prefer the comments to end with a period and I will do so for all in the next version. >> +/* Extend uninitialized enclave measurement */ >> static inline int __eextend(void *secs, void *addr) >> { >> return __encls_2(EEXTEND, secs, addr); >> } > > That description does not make __eextend any less cryptic. > > Something like this would be already more informative: > > /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */ Thank you, I will use this description. > > This same remark applies to the rest of these comments. They should > provide a clue what the wrapper does rather than an English open coded > function name. Please see below for another attempt that includes your proposed changes so far. What do you think? __ecreate(): /* Initialize an EPC page into an SGX Enclave Control Structure (SECS) page. */ __eextend(): /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */ __eadd(): /* Copy a source page from non-enclave memory into the EPC. */ __einit(): /* Finalize enclave build, initialize enclave for user code execution */ __eremove(): /* Disassociate EPC page from its enclave and mark it as unused. */ __edbgwr(): /* Copy data to an EPC page belonging to a debug enclave. */ __edbgrd(): /* Copy data from an EPC page belonging to a debug enclave. */ __etrack(): /* Track that software has completed the required TLB address clears. */ __eldu(): /* Load, verify, and unblock an Enclave Page Cache (EPC) page. */ __eblock(): /* Make EPC page inaccessible to enclave, ready to be written to memory. */ __epa(): /* Initialize an EPC page into a Version Array (VA) page. */ __ewb(): /* Invalidate an EPC page and write it out to main memory. */ Reinette
On Mon, 2021-12-06 at 13:13 -0800, Reinette Chatre wrote: > > * "Create an SECS page in the Enclave Page Cache (EPC)" > > * "Add a Version Array (VA) page to the Enclave Page Cache (EPC)" > > > > They should have similar descriptions, e.g. > > > > * "Initialize an EPC page into SGX Enclave Control Structure (SECS) page." > > * "Initialize an EPC page into Version Array (VA) page." > > Will do. Did you intentionally omit the articles or would you be ok if I > change it to: > > "Initialize an EPC page into an SGX Enclave Control Structure (SECS) page." > "Initialize an EPC page into a Version Array (VA) page." > > I also notice that you prefer the comments to end with a period and I > will do so for all in the next version. Looks fine to me. > > > +/* Extend uninitialized enclave measurement */ > > > static inline int __eextend(void *secs, void *addr) > > > { > > > return __encls_2(EEXTEND, secs, addr); > > > } > > > > That description does not make __eextend any less cryptic. > > > > Something like this would be already more informative: > > > > /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */ > > Thank you, I will use this description. > > > > > This same remark applies to the rest of these comments. They should > > provide a clue what the wrapper does rather than an English open coded > > function name. > > Please see below for another attempt that includes your proposed changes > so far. What do you think? > > __ecreate(): > /* Initialize an EPC page into an SGX Enclave Control Structure (SECS) > page. */ > > __eextend(): > /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */ > > __eadd(): > /* Copy a source page from non-enclave memory into the EPC. */ Perhaps: /* * Associate an EPC page to an enclave either as a REG or TCS page * populated with the provided data. */ This is more aligned with your description for __eremove(). > > __einit(): > /* Finalize enclave build, initialize enclave for user code execution */ > > __eremove(): > /* Disassociate EPC page from its enclave and mark it as unused. */ > > __edbgwr(): > /* Copy data to an EPC page belonging to a debug enclave. */ > > __edbgrd(): > /* Copy data from an EPC page belonging to a debug enclave. */ > > __etrack(): > /* Track that software has completed the required TLB address clears. */ > > __eldu(): > /* Load, verify, and unblock an Enclave Page Cache (EPC) page. */ > > __eblock(): > /* Make EPC page inaccessible to enclave, ready to be written to memory. */ > > __epa(): > /* Initialize an EPC page into a Version Array (VA) page. */ > > __ewb(): > /* Invalidate an EPC page and write it out to main memory. */ > > > Reinette /Jarkko
Hi Jarkko, On 12/10/2021 9:28 PM, Jarkko Sakkinen wrote: > On Mon, 2021-12-06 at 13:13 -0800, Reinette Chatre wrote: ... >> >> __eadd(): >> /* Copy a source page from non-enclave memory into the EPC. */ > > Perhaps: > > /* > * Associate an EPC page to an enclave either as a REG or TCS page > * populated with the provided data. > */ > > This is more aligned with your description for __eremove(). I was trying to keep the descriptions as concise one-liners. I'll use the text you provide if you are ok with its line length being an exception. ... >> >> __eremove(): >> /* Disassociate EPC page from its enclave and mark it as unused. */ ... Reinette
diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h index 9b204843b78d..241b766265d3 100644 --- a/arch/x86/kernel/cpu/sgx/encls.h +++ b/arch/x86/kernel/cpu/sgx/encls.h @@ -162,57 +162,68 @@ static inline bool encls_failed(int ret) ret; \ }) +/* Create an SECS page in the Enclave Page Cache (EPC) */ static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs) { return __encls_2(ECREATE, pginfo, secs); } +/* Extend uninitialized enclave measurement */ static inline int __eextend(void *secs, void *addr) { return __encls_2(EEXTEND, secs, addr); } +/* Add a page to an uninitialized enclave */ static inline int __eadd(struct sgx_pageinfo *pginfo, void *addr) { return __encls_2(EADD, pginfo, addr); } +/* Initialize an enclave for execution */ static inline int __einit(void *sigstruct, void *token, void *secs) { return __encls_ret_3(EINIT, sigstruct, secs, token); } +/* Remove a page from the Enclave Page Cache (EPC) */ static inline int __eremove(void *addr) { return __encls_ret_1(EREMOVE, addr); } +/* Write to a debug enclave */ static inline int __edbgwr(void *addr, unsigned long *data) { return __encls_2(EDGBWR, *data, addr); } +/* Read from a debug enclave */ static inline int __edbgrd(void *addr, unsigned long *data) { return __encls_1_1(EDGBRD, *data, addr); } +/* Track threads operating inside the enclave */ static inline int __etrack(void *addr) { return __encls_ret_1(ETRACK, addr); } +/* Load, verify, and unblock an Enclave Page Cache (EPC) page */ static inline int __eldu(struct sgx_pageinfo *pginfo, void *addr, void *va) { return __encls_ret_3(ELDU, pginfo, addr, va); } +/* Mark an Enclave Page Cache (EPC) page as blocked */ static inline int __eblock(void *addr) { return __encls_ret_1(EBLOCK, addr); } +/* Add a Version Array (VA) page to the Enclave Page Cache (EPC) */ static inline int __epa(void *addr) { unsigned long rbx = SGX_PAGE_TYPE_VA; @@ -220,6 +231,7 @@ static inline int __epa(void *addr) return __encls_2(EPA, rbx, addr); } +/* Invalidate an EPC page and write it out to main memory */ static inline int __ewb(struct sgx_pageinfo *pginfo, void *addr, void *va) {
The SGX ENCLS instruction uses EAX to specify an SGX function and may require additional registers, depending on the SGX function. ENCLS invokes the specified privileged SGX function for managing and debugging enclaves. Macros are used to wrap the ENCLS functionality and several wrappers are used to wrap the macros to make the different SGX functions accessible in the code. The wrappers of the supported SGX functions are cryptic. Add short changelog descriptions of each to a comment. Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- arch/x86/kernel/cpu/sgx/encls.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)