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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, 2021-12-08 at 14:11 -0800, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 11/30/2021 9:50 AM, Jarkko Sakkinen wrote:
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> ...
> 
> > +.TH SGX 7 2021\-02\-02 "Linux" "Linux Programmer's Manual"
> > +.PP
> > +sgx - overview of Software Guard eXtensions
> 
> Is the "overview of" text necessary?

It's more or less a convention:

$ git grep "overview of" man7 | wc -l
29

> > +.SH SYNOPSIS
> > +.EX
> > +.B #include <asm/sgx.h>
> > +.PP
> > +.IB enclave " = open(""/dev/sgx_enclave", " O_RDWR);"
> 
> I view the man page output using "man -l man7/sgx.7" and when I do so 
> the above line is unbalanced: "enclave" and (unexpectedly) the comma are 
> underlined and the line is displayed with a single instance of a double 
> quote: enclave = open("/dev/sgx_enclave, O_RDWR);

After some trial and error, and looking at symlink.7, this seems to
fix it:

-.IB enclave " = open(""/dev/sgx_enclave", " O_RDWR);"
+.IB enclave " = open(""/dev/sgx_enclave"", O_RDWR);"

Does this fix for you?

> > +.EE
> > +.SH DESCRIPTION
> > +Intel Software Guard eXtensions (SGX) allow applications to host
> > +enclaves,
> > +protected executable objects in memory.
> > +.PP
> > +Enclaves are blobs of executable code,
> > +running inside a CPU enforced container,
> > +which is mapped to the process address space.
> > +They are represented as the instances of
> > +.I /dev/sgx_enclave.

.IR /dev/sgx_enclave .

> > +They have a fixed set of entry points,
> > +defined when the enclave is built.
> > +.PP
> > +SGX can be only available if the kernel is configured and built with the
> 
> can be only -> can only be?

Agreed, I'll fix this.

> > +.B CONFIG_X86_SGX
> > +option.
> > +If CPU, BIOS and kernel have SGX enabled,
> > +.I sgx
> > +appears in the
> > +.I flags
> > +field of
> > +.IR /proc/cpuinfo .
> > +.PP
> > +If SGX appears not to be available,
> > +ensure that SGX is enabled in the BIOS.
> > +If a BIOS presents a choice between
> > +.I Enabled
> > +and
> > +.I Software Enabled
> > +modes for SGX,
> > +choose
> > +.I Enabled.
> 
> Earlier there is the underlined "/proc/cpuinfo" text followed by a 
> period and here the "Enabled" text is followed by a period. In this 
> instance the period is also underlined while previously it is not. 
> Looking at some other man pages it seems that the custom is that the 
> period should not be underlined and I will continue to point out 
> instances I noticed where this is not the case.

This most related to my very weak understanding of troff syntax.

I changed it to:

-.I Enabled.
+.IR Enabled .

This does seem to fix the issue, and aligns with this:

https://www.gnu.org/software/groff/manual/html_node/Man-font-macros.html

> > +.PP
> > +.SS Memory mapping
> > +The file descriptor for an enclave can be shared among multiple processes.
> > +An enclave is required by the CPU to be placed to an address,
> > +which is a multiple of its size.
> > +An address range containing a reasonable base address can be probed with an anonymous
> > +.BR mmap(2)
> > +call:
> > +.PP
> > +.EX
> > +void *area = mmap(NULL, size * 2, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS,
> > +                  -1, 0);
> > +
> > +void *base = ((uint64_t)area + size - 1) & ~(size - 1);
> > +.EE
> > +.PP
> > +The enclave file descriptor itself can be then mapped with the
> > +.BR MAP_FIXED
> > +flag set to the carved out memory.
> > +.SS Construction
> > +An enclave instance is created by opening
> > +.I /dev/sgx_enclave.
> 
> Underlined period above.

Thanks. I spotted similar error also early in the text.

> > +Its contents are populated with the
> > +.BR ioctl (2)
> > +interface in
> > +.IR <asm/sgx.h>:
> 
> Here also, should the above colon perhaps not be underlined?

Yeah, similarly:

.IR <asm/sgx.h>:
+.IR <asm/sgx.h> :

> > +.TP
> > +.IB SGX_IOC_ENCLAVE_CREATE
> > +Create SGX Enclave Control Structure (SECS) for the enclave.
> > +SECS is a hardware defined structure,
> > +which contains the global properties of an enclave.
> > +.IB SGX_IOC_ENCLAVE_CREATE
> > +is a one-shot call that fixes enclave's address and
> 
> fixes enclave's -> fixes the enclave's ?

Ack.

> > +size for the rest of its life-cycle.
> > +.TP
> > +.IB SGX_IOC_ENCLAVE_ADD_PAGES
> > +Fill a range of the enclaves pages with the caller provided data and protection bits.
> 
> Should this be "the enclave's pages"?

I think so.

> > +Memory mappings of the enclave can only have protection bits set,
> > +which are defined in this ioctl.
> 
> This is a bit hard to understand. How about "Memory mappings of the 
> enclave can only set protection bits that are defined in this ioctl."

Changed, thanks.

> > +The pages added are either regular memory pages for code and data
> 
> regular memory pages -> regular pages?

Changed.

> 
> ,
> > +or thread control structures (TCS).
> > +The latter define the entry points to the enclave,
> > +which can be entered after the initialization.
> > +.TP
> > +.IB SGX_IOC_ENCLAVE_INIT
> > +Initialize the enclave for the run-time.
> > +After a successful initialization,
> > +no new pages can be added to the enclave.
> > +.SS Invocation
> > +Thread control structure (TCS) page are the entry points to the enclave,
> 
> page are -> pages are ?

Thanks, good catch.

> > +which further define an offset inside the enclave where the execution begins.
> > +The entry points are invoked with
> > +.I __vdso_sgx_enter_enclave.
> 
> Underlined period above.

.I __vdso_sgx_enter_enclave.
+.IR __vdso_sgx_enter_enclave .

> > +The prototype for the vDSO is defined by
> > +.BR vdso_sgx_enter_enclave_t
> > +in
> > +.IR <asm/sgx.h>.
> 
> Same above with the underlining of "."
> 
> > +.SS Permissions
> > +.PP
> > +During the build process each enclave page is assigned protection bits,
> > +as part of
> > +.BR ioctl(SGX_IOC_ENCLAVE_ADD_PAGES).
> > +These protections are also the maximum protections to which the page can be be mapped.
> 
> to which -> with which ?

Ack.

> 
> > +If
> > +.BR mmap (2)_
> 
> Unexpected "_" above

Thanks.

> 
> > +is called with higher protections than those defined during the build,
> > +it will return
> > +.B -EACCES.
> > +If
> > +.BR ioctl(SGX_IOC_ENCLAVE_ADD_PAGES)
> > +is called after
> > +.BR mmap (2)
> > +with lower protections,
> > +the caller receives
> > +.BR SIGBUS,
> > +once it accesses the page for the first time.
> > +.SH VERSIONS
> > +The SGX feature was added in Linux 5.11.
> 
> This does not document the SGX_IOC_VEPC_REMOVE ioctl that was added in 
> v5.16. How do you envision additions to this page as new features are 
> added to the Linux support of SGX?

I started this before any of KVM stuff was in upstream. It'd be better
to get the basic ioctl's done first. I cannot really give estimate for
vepc at this point.

For future features (e.g. SGX2), the expectation is that the feature is
supported by an associated man page update.

> > +.SH SEE ALSO
> > +.BR ioctl (2),
> > +.BR mmap (2),
> > +.BR mprotect (2)
> > \ No newline at end of file
> > 
> 
> Reinette

Thanks for valuable feedback.

/Jarkko