lkml.org 
[lkml]   [2021]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS wrappers
From
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

\
 
 \ /
  Last update: 2021-12-06 22:15    [W:0.166 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site