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@xxxxxxxxxxxxxxx>
Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
---
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
