Re: [PATCH 16/25] x86/sgx: Support modifying SGX page type

[Reinette Chatre]

Hi Jarkko,

On 12/4/2021 3:45 PM, Jarkko Sakkinen wrote:
> On Wed, Dec 01, 2021 at 11:23:14AM -0800, Reinette Chatre wrote:
> > Every enclave contains one or more Thread Control Structures (TCS). The
> > TCS contains meta-data used by the hardware to save and restore thread
> > specific information when entering/exiting the enclave. With SGX1 an
> > enclave needs to be created with enough TCSs to support the largest
> > number of threads expecting to use the enclave and enough enclave pages
> > to meet all its anticipated memory demands. In SGX1 all pages remain in
> > the enclave until the enclave is unloaded.
> >
> > Earlier changes added support for the SGX2 feature where pages can be
> > added dynamically to an initialized enclave.
>
> Please remove this paragraph, i.e. do not tie the commit order like
> this.

Will do.

> > SGX2 introduces a new function, ENCLS[EMODT], that is used to change
> > the type of an enclave page from a regular (SGX_PAGE_TYPE_REG) enclave
> > page to a TCS (SGX_PAGE_TYPE_TCS) page or change the type from a
> > regular (SGX_PAGE_TYPE_REG) or TCS (SGX_PAGE_TYPE_TCS)
> > page to a trimmed (SGX_PAGE_TYPE_TRIM) page (setting it up for later
> > removal).
> >
> > With the existing support of dynamically adding regular enclave pages
> > to an initialized enclave and changing the page type to TCS it is
> > possible to dynamically increase the number of threads supported by an
> > enclave.
> >
> > Changing the enclave page type to SGX_PAGE_TYPE_TRIM is the first step
> > of dynamically removing pages from an initialized enclave. The complete
> > page removal flow is:
> > 1) Change the type of the pages to be removed to SGX_PAGE_TYPE_TRIM
> >     using the ioctl introduced here.
> > 2) Approve the page removal by running ENCLU[EACCEPT] from within
> >     the enclave.
> > 3) Initiate actual page removal using the new ioctl introduced in the
> >     following patch.
> >
> > Support changing SGX enclave page types with a new ioctl. With this
>
> What is "a new ioctl"? Why not just write "Add <ioctl name>""?

I do so to reduce the changes required during the ioctl naming 
discussion churn.

> > ioctl the user specifies a page range and the enclave page type to be
> > applied to all pages in the provided range. The ioctl itself can return
> > an error code based on failures encountered by the OS. It is also
> > possible for SGX specific failures to be encountered.  Add a result
> > output parameter to communicate the SGX return code. It is
> > possible for the enclave page type change request to fail on any page
> > within the provided range. Support partial success by returning
> > the number of pages that were successfully changed.
> >
> > After the page type is changed to SGX_PAGE_TYPE_TRIM the page continues
> > to be accessible from the OS perspective with page table entries and
> > internal state. The page may be moved to swap. Any invalid access
> > (any access except ENCLU[EACCEPT]) will encounter a page fault with
> > SGX flag set in error code until the page is removed. Removal of
> > trimmed enclave pages on user request will be supported in following
> > patch. Trimmed enclave pages are also removed when enclave is unloaded.
> >
> > Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>
> This is lacking discussion of EPCM interaction, most importanly
> .MODIFY field of an EPCM entry.

I will add that. I have the same question here as in EAUG patch - would 
you like a duplicate description in this patch and a new patch that 
introduces just the __emodt() wrapper or would you be ok with all new 
wrappers introduced together and the detailed description of their 
hardware supported flows only present in the patch that uses those wrappers?


> > ---
> >   arch/x86/include/uapi/asm/sgx.h |  19 +++
> >   arch/x86/kernel/cpu/sgx/ioctl.c | 235 ++++++++++++++++++++++++++++++++
> >   2 files changed, 254 insertions(+)
> >
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index 24bebc31e336..f70caccd166c 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -31,6 +31,8 @@ enum sgx_page_flags {
> >   	_IO(SGX_MAGIC, 0x04)
> >   #define SGX_IOC_PAGE_MODP \
> >   	_IOWR(SGX_MAGIC, 0x05, struct sgx_page_modp)
> > +#define SGX_IOC_PAGE_MODT \
> > +	_IOWR(SGX_MAGIC, 0x06, struct sgx_page_modt)
>
> I'd suggest to change this as SGX_IOC_ENCLAVE_MODIFY_TYPE.

How about SGX_IOC_ENCLAVE_MOD_TYPE to be consistent with your earlier 
suggestion of SGX_IOC_ENCLAVE_MOD_PROTECTIONS ?

Reinette