Re: [PATCH v2] sgx.7: New page with overview of Software Guard eXtensions (SGX)

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hi Jarko,

On 1/21/21 12:18 PM, Jarkko Sakkinen wrote:
> On Tue, Dec 22, 2020 at 07:53:24PM +0100, Michael Kerrisk (man-pages) wrote:
>> Hi Jarkko
>>
>> Thanks for revising the patch. I have many comments.
>> I must admit that I'm struggling to understand much here,
>> and so I'll probably have more comments on a future draft.
>> Could you please revise in the light of my comments
>> below (and hopefully the revisions will help me better
>> understand the topic when I look at the next draft).
> 
> I'm truly sorry of this incredibly long latency.

No problem. I appreciate your detailed notes below.

> I put the man page as my top priority up until it is good enough to be
> merged.
> 
>> On 12/22/20 1:41 AM, Jarkko Sakkinen wrote:
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>> v2:
>>> * Fixed the semantic newlines convention and various style errors etc.
>>>   that were reported by Alenjandro and Michael.

s/Alenjandro/Alejandro/  :-)

>>> * SGX was merged to v5.11.
>>
>> I think we better have a VERSIONS section in the page noting that this
>> feature is supported since Linux 5.11.
> 
> I added:
> 
> .SH VERSIONS
> The SGX feature was added in Linux 5.11.
> 
> I also changed the copyright year to 2021.
> 
>>> Link: https://lore.kernel.org/linux-sgx/f6eb74cf-0cb6-0549-9ed3-3e3b2af23ad1@gmail.com/
>>> Link: https://lore.kernel.org/linux-sgx/f6eb74cf-0cb6-0549-9ed3-3e3b2af23ad1@gmail.com/
>>>  man7/sgx.7 | 218 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 218 insertions(+)
>>>  create mode 100644 man7/sgx.7
>>>
>>> diff --git a/man7/sgx.7 b/man7/sgx.7
>>> new file mode 100644
>>> index 000000000..5e8d3d959
>>> --- /dev/null
>>> +++ b/man7/sgx.7
>>> @@ -0,0 +1,218 @@

[...]

>>> +Each of them can hold a single hardware thread at a time.
>>
>> "them" is unclear. Do you mean "Each of the entry points" 
>> or "Each enclave"?
> 
> TCS pages are a bit like locks. You reserve one and its held up until
> you leave the enclave address apce. It also tells you where to start
> execution.
> 
> I wrote a wrote a new paragraph that introduces ENCLU and tries
> to explain this in length in the v3 of this patch.

Okay.

>>> +While the enclave is loaded from a regular binary file,
>>> +only the threads inside the enclave can access its memory.
>>> +.PP
>>> +Although carved out of normal DRAM,
>>> +enclave memory is marked in the system memory map as reserved and is not
>>> +managed by the Linux memory manager.
>>> +There may be several regions spread across the system.
>>
>> I presume you mean "There may be several enclave regions"? I think it
>> would be clearer to say that.
> 
> Not sure.
> 
> So the thing is that there is reserved memory, consider it as a bit like
> VRAM. This memory can be oversubscribed. Then when you create an enclave
> you consume these pages. When running out of them, the kernel swaps pages
> from enclaves across the system currently based on a trivial FIFO policy.
> So these regions define kind of the memory pool for all enclaves running in
> the system.

SO, is there some suitable change for the manual page text?

>>> +Each contiguous region is called an Enclave Page Cache (EPC) section.
>>> +EPC sections are enumerated via CPUID instruction.
>>
>> BY "CPUID instruction" do you mean the interface described in the
>> cpuid(4) manual page? If yes, I think you better include a reference 
>> to that page.
> 
> Kernel uses a set of CPUID leaves to enumerate the available EPC.  The base
> leaf SGX specific functions is EAX=0x12, and enumeration leaves for EPC
> start from ECX=2 and onwards.
> 
> This CPUID is documented in the pages 313-14 of the Intel SDM:
> 
> https://software.intel.com/content/www/us/en/develop/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-2a-2b-2c-and-2d-instruction-set-reference-a-z.html
> 
> And its usage is implemented in sgx_page_cache_init() internal function:
> 
> https://elixir.bootlin.com/linux/v5.11-rc4/source/arch/x86/kernel/cpu/sgx/main.c#L664
> 
> I'll just remove that sentence. I don't think it's relevant here.
>
Okay.

>>> +These regions are encrypted when they leave the Last Level Cacche (LLC).
>>
>> Maybe better: s/These regions/EPC regions/ ?
>>
>> s/Cacche/Cache/
> 
> I changed this to
> 
> "The pages belonging to the EPC sections are encrypted when they leave the
> Last Level Cache (LLC)."

Okay.

[...]

>>> +.SS Enclave management
>>> +.PP
>>> +An enclave's life-cycle starts by opening
>>> +.I /dev/sgx_enclave.
>>
>> Remove the "." at the end of the preceding line.
> 
> Fixed.
> 
>>> +and ends once all the references to the opened file have been closed.
>>
>> I presume here that you mean to say that the lifecycle ends
>> when all duplicate file descriptors that refer to the open
>> file description (i.e., 'struct file') have been closed, right?
>> If that's correct, please modify the text. If it's not correct,
>> then I don't understand the text, and so some other fix is
>> probably needed.
> 
> I changed this to:
> 
> "and ends when all the file descriptors referring to the opened file
> have been closed."

Okay.

[...]

>> You suddenly use the term "ENCLS" here with no previous introduction or
>> definition.
> 
> It's a mnemonic for x86 opcode. Not exactly sure how to improve.

I think then it would be helpful to write something like "the x86
ENCLS opcode [or instruction]". That would help the less knowledgeable
reader orient themselves bit.

>>> +managing enclave memory,
>>> +and the ioctl interface provides a wrapper for it.
>>> +.PP
>>
>> [I find the next paragraph very hard to understand. So I'm going
>> to ask lots of silly questions...]
> 
> Thank you, I appreciate these questions. This is somewhat complicated
> topic, and when you've upstreamed a patch set literally for years, you
> become blind for many things.
> 
>>> +Enclave construction starts by calling
>>> +.B SGX_IOC_ENCLAVE_CREATE,
>>> +which takes in the initial version of SGX Enclave Control Structure
>>
>> What do you mean by "takes in"?
> 
> It's the 'src' field in struct sgx_enclave_create:
> 
> https://elixir.bootlin.com/linux/v5.11-rc4/source/arch/x86/include/uapi/asm/sgx.h
> 
> This address is given to ENCLS[ECREATE], which copies to an EPC
> page. It's the root of the enclave, not visible in the actual
> address space of the enclave. It contains data such as the base
> address and size of the enclave addree space.
> 
> I changed "takes in" to "copies".

Okay.

>>> +(SECS).
>>> +SGX Enclave Control Structure (SECS) contains the description of the
>>
>> s/SGX Enclave Control Structure (SECS)/The SECS/
>>
>> This all made weird because the current terminology includes
>> "Structure" in the name.
> 
> I agree. It's also asymmetrical to TCS. Either TCS should be
> STCS or SECS should be ECS. I'm just using the naming convetions
> from the Intel software developement manual.

I see :-/.

>> And yes, "the SECS" reads weirdly. What I'd really like to say
>> is "the SECS structure" or (even better) "the SEC structure".
>> Is either of those acceptable? (This would imply global changes 
>> in the following text.)
> 
> I'd still stick to the terminology that is common to what is used
> in the SDM and also in all the documentation, academic paper etc.
> Essentially, all the literature on SGX uses the same terminology.
> Drifting from that would be IMHO even more confusing.

Okay -- I'll see what I think of this when I review V3.


>>> +enclave.
>>> +The ioctl calls ENCLS[ECREATE] function,
>>
>> What is "ENCLS[ECREATE] function"? This needs some explanation.
> 
> We have ENCLS, which x86 opcode, and you EAX id's of various functions that
> is contains. One of them is ECREATE.
> 
> I rephased it as:
> 
> "The ioctl calls the ECREATE subfunction of ENCLS,"

Maybe s/ENCLS/the ENCLS opcode/?

[...]

>> But what is this "ENCLU[EGETKEY] function"? Where does it come from?
>> And what is ENCLU?  I think some more detail is needed here.
> 
> I refined this a lot for v3. I hope it makes a bit more sense. I introduce
> ENCLU early on in the when I talk about TCS in the new version of the
> patch.

Okay.

[...]

>>> +.PP
>>> +The vDSO function calling convention uses the standard RDI, RSI, RDX,
>>> +RCX, R8 and R9 registers.
>>> +This makes it possible to declare the vDSO as a C prototype,
>>> +but other than that there is no specific support for SystemV ABI.
>>
>> What do you mean by "SystemV ABI"?
> 
> I'm referring to the calling convention of x86-64 psABI.
> 
> I rephrased this as:
> 
> "but other than that there is no specific support for the x86-64
> calling convention,"

Okay.

>> Thanks,
>>
>> Michael
> 
> Thank you!

You're welcome. I doubt that I will truly understand this stuff 
by the time we're done, but I hope to help you beat the page into
better shape :-).

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/