Messages in this thread | | | Date | Mon, 6 Dec 2021 13:13:49 -0800 | Subject | Re: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS wrappers | From | Reinette Chatre <> |
| |
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
| |