Message-ID: <b49ee68b-63c5-f8f8-ca2b-31663c3150f6@intel.com>
Date:   Mon, 6 Dec 2021 13:13:49 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.3.2
Subject: Re: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS
 wrappers
Content-Language: en-US
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>
References: <cover.1638381245.git.reinette.chatre@intel.com>
 <fd9ab4d760a2ea7a42ab9e60b9e19b8620abe11d.1638381245.git.reinette.chatre@intel.com>
 <YauzxOEmmpeGF/K3@iki.fi>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <YauzxOEmmpeGF/K3@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?akY1NDI4RWVwVXlTYmpWdHpTcXJkb05wWU53QmRpWERNZFZtS3ZwQ3lpN3dG?=
 =?utf-8?B?MXo4U3hmbWh4NVpQYUQvRlpTNkZOL1dBUHNIeHduM3lXMUtRL0VnMXhCZkxL?=
 =?utf-8?B?UUV2WTBYZGovWDVpV25xYXFaa2FNbGxTekw4UEduQUN6RjJoWVE4TkljS2dr?=
 =?utf-8?B?YW1EeXJVdEM0bFJ6K2poT1VrYU1HOFU2bFVJZVIxQ0FIYll4bkNEQ2YwWlBs?=
 =?utf-8?B?NUFzZTdpamJ0b01EaVlSL2JzMXJQcnZpVjJzVFhaM2NXQ2tDbWZtZFpMSXhq?=
 =?utf-8?B?QWdxRFczZkFkT0RPM1RhQTJKNGtsaFlFTXB6SndYa3B2L28rZlptS2xrQXZ5?=
 =?utf-8?B?M2h6ZjFGemI1d3FBemhSdlBUQmdNaklKdC9UeE5HM1ZWQUxlM2diaDVKZERV?=
 =?utf-8?B?dXU1U3hMbDhGSUNKOTFNamw1empveFBnRXdCU2pwcXVjUGNIOHpOTFZxS2JI?=
 =?utf-8?B?MHorZWlIL3p5OTJwQVl0Uy9sWXZHRzZMRFBaVktoREtpSmUySXVCZGZXaisy?=
 =?utf-8?B?Y0NlT3p5cWVsNWp4RFVDS0gzVDhxYVJiQVVsTjIvdlVGWUl6bTlGMjd0TEFM?=
 =?utf-8?B?Q3NyaWhjMUR5YlV0cEpUUlZpa0JvUllYeTltdGl3cFo5QVNZZVhnT1pyNTFH?=
 =?utf-8?B?QzFWaEN3M1RWNWplblI0RUJUUm9Ra1hrQXRHVkVVQ2pzck5WQnQ0TG5kZmll?=
 =?utf-8?B?Sy94MVdhSzJGZ1BkdXZ4K2U4Vk1BV1luVzNiUWdFclZsYkNEdW1qU2lpN1VH?=
 =?utf-8?B?TjNHVWVMWlVkeEU4Y1ZWSXU3SWlqdlN4RHhjci93SWlFNnZUZG90enRVZm5L?=
 =?utf-8?B?dm5HdnpkUlQrOG1YNHdPOW9BcGNUbGJsL2ttZWJQcWxNZy91OXgrT0F1cm1l?=
 =?utf-8?B?K1hBaklXdDd4dFo5eFNoRUtEaVVrb3Z3VTk0VjkxWE85Sno0Mm5VaWJwY1JD?=
 =?utf-8?B?czBIY0lNWVV6YTduQVNCNnZHS3BlejVxQmltVUcvV1lVODdNS1N2eGphTFYr?=
 =?utf-8?B?WUZSemN2VStzdzR3dzZVQnhobTBseTRjT2x4RjB1SG92d2FhNFZGRmd6cncz?=
 =?utf-8?B?QTJZc3JKV2lyNy9rbjZqa1pjN0tXTFQ3RU40Z1lCS0FNT2lvaDRkSzMzR3FB?=
 =?utf-8?B?cmZSZGM0a3VNTE90UWl4Z1ZrWmFxb3JSODFUYmF2UlJzNjd4eC9QMzl0QWlh?=
 =?utf-8?B?WDgxT2taSnhQUlpNZ1NhVmh2cGljZlZvbzNacGMzUCs3OWFuK1BFNGxRMW9p?=
 =?utf-8?B?RWtpeE9xYTZmdFIyckIybzdXeTdxSjVmRXk0R0s3QmZQbzI0NHl0TVhxMEQ4?=
 =?utf-8?B?OUd2ZTFTZzM3UXU0YkpzbmMxSkxuM1ZaYmhnMUFqZ3ZWZ2E4L1FKQXUyR0Rz?=
 =?utf-8?B?U1JCSUZENTZrUDdCbTVJY0gyZ3c5RmdGSnJRZlZ2TGw1bjltZmRORktySC9O?=
 =?utf-8?B?OGhseXk0NHJUZzNmalJsa3M4RThTem53b3BoYkhWcmx4eUEzZ2lCc2JqMHV0?=
 =?utf-8?B?Q2VicHhNLys1ZnNWVzF0OFdIYTNLdjB4QWlIaU4wMkhORGhhYVUwRXVGTXEw?=
 =?utf-8?B?WnJTQWVxNU1HQnBoeUZLeHVCNXJqaFZlTjZWbDI3dGlKUEp5L0thaVJJWk92?=
 =?utf-8?B?Nk8rdXlodDBaUWhiU3lheFVhNHMyK3Z3Wk1IUHJnOHVIUW5FNmRlQUMyMzBG?=
 =?utf-8?B?aDV6Vm9hQVpRSVc4UDhQS1Fra1pCY1V1RGZYRTFxNVFHZzJGL01EdnFuaUZF?=
 =?utf-8?B?VHo3alZrYmM4Z1pSbVIvQjlMUUw2RWxWU3ZnNENSUjUrVmFQZjBkNUNUdXpC?=
 =?utf-8?B?WUgzalg1TDQ4SWJnQ0c1LzI0WlBlb3BSSmhKTURKZmdrVFlwMTdSRlIvNHhq?=
 =?utf-8?B?VE5ldmRPWjhJeDNUV0ZaV2thR3ZjRCs4Yk5IaE1OWFViUDcvVkhNdzJveEM0?=
 =?utf-8?B?S1RKUElXRElwcnBoYlJ5V2dtbEhMck1HN0crOWNMMDZZVUQ4d3h6M2lLYXEy?=
 =?utf-8?B?UFJJd3Y5eWdzWk0yR0dwaDI1RGlSUit0SlcxcnJkUzFrWmZDTlAxSytqbGl5?=
 =?utf-8?B?OG9rTGRWUjUxc0d0TE1sTWFvR2Y4SWdVaU15aHFyT2FkUG5WeTNlV2ZZNVhS?=
 =?utf-8?B?eXJEZUQzWEtjS3FMK2pScUtONTB1Qzg5akd3SzJLT0NNYWxXSUlwUUp2Tnpn?=
 =?utf-8?B?Z2VHbURldlY1QlR6OHNMUWtJa3JSUFk3bWdPOUt3QjY4cGJXeHhMbjdoYTJv?=
 =?utf-8?Q?UDjxspHfr4DdxcPVLAUnnzlIqFy7NwqqW0hziKBSpk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4843954c-4078-4a9f-9cd8-08d9b8fd4ebd
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2021 21:13:54.2716
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: n4xJl3QjWYl5ngft4jfmgNFYI2SwcRlchCL5wqnrzkkRdCiCvIgg48ZoUeb9SHCnraWuztvqpAeq4E2tPUbnRAHfTELtfORG41xew30tO4A=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR11MB2674
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jarkko,

On 12/4/2021 10:30 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 01, 2021 at 11:22:59AM -0800, Reinette Chatre wrote:
>> The SGX ENCLS instruction uses EAX to specify an SGX function and
>> may require additional registers, depending on the SGX function.
>> ENCLS invokes the specified privileged SGX function for managing
>> and debugging enclaves. Macros are used to wrap the ENCLS
>> functionality and several wrappers are used to wrap the macros to
>> make the different SGX functions accessible in the code.
>>
>> The wrappers of the supported SGX functions are cryptic. Add short
>> changelog descriptions of each to a comment.
> 
> I think you are adding function descriptions.

Will change.

> 
>> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>>   arch/x86/kernel/cpu/sgx/encls.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
>> index 9b204843b78d..241b766265d3 100644
>> --- a/arch/x86/kernel/cpu/sgx/encls.h
>> +++ b/arch/x86/kernel/cpu/sgx/encls.h
>> @@ -162,57 +162,68 @@ static inline bool encls_failed(int ret)
>>   	ret;						\
>>   	})
>>   
>> +/* Create an SECS page in the Enclave Page Cache (EPC) */
>>   static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs)
>>   {
>>   	return __encls_2(ECREATE, pginfo, secs);
>>   }
> 
> You have:
> 
> * "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.

>> +/* 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. */

__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