Re: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS wrappers

[Jarkko Sakkinen <jarkko@kernel.org>]

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