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

From: Reinette Chatre <reinette.chatre@intel.com>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: <dave.hansen@linux.intel.com>, <tglx@linutronix.de>,
	<bp@alien8.de>, <luto@kernel.org>, <mingo@redhat.com>,
	<linux-sgx@vger.kernel.org>, <x86@kernel.org>,
	<seanjc@google.com>, <kai.huang@intel.com>,
	<cathy.zhang@intel.com>, <cedric.xing@intel.com>,
	<haitao.huang@intel.com>, <mark.shanahan@intel.com>,
	<hpa@zytor.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 16/25] x86/sgx: Support modifying SGX page type
Date: Mon, 6 Dec 2021 13:48:31 -0800	[thread overview]
Message-ID: <dc74a876-54fe-5caa-6602-8887cfe34315@intel.com> (raw)
In-Reply-To: <Yav9g4+L8zg48DRf@iki.fi>

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@intel.com>
> 
> 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