Re: [RFC PATCH v3 04/27] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

Paolo Bonzini
2021-02-02T18:04:48Z

On 27/01/21 02:25, Kai Huang wrote:
> On Tue, 26 Jan 2021 08:04:35 -0800 Dave Hansen wrote:
>> On 1/26/21 1:30 AM, Kai Huang wrote:
>>> From: Jarkko Sakkinen <jarkko@kernel.org>
>>>
>>> Encapsulate the snippet in sgx_free_epc_page() concerning EREMOVE to
>>> sgx_reset_epc_page(), which is a static helper function for
>>> sgx_encl_release(). It's the only function existing, which deals with
>>> initialized pages.
>>
>> Yikes. I have no idea what that is saying. Here's a rewrite:
>>
>> EREMOVE takes a pages and removes any association between that page and
>> an enclave. It must be run on a page before it can be added into
>> another enclave. Currently, EREMOVE is run as part of pages being freed
>> into the SGX page allocator. It is not expected to fail.
>>
>> KVM does not track how guest pages are used, which means that SGX
>> virtualization use of EREMOVE might fail.
>>
>> Break out the EREMOVE call from the SGX page allocator. This will allow
>> the SGX virtualization code to use the allocator directly. (SGX/KVM
>> will also introduce a more permissive EREMOVE helper).
>
> Thanks.
>
> Hi Jarkko,
>
> Do you want me to update your patch directly, or do you want to take the
> change, and send me the patch again?

I think you should treat all these 27 patches as yours now (including
removing them, reordering them, adjusting commit message etc.).

>> OK, so if you're going to say "the caller must put the page in
>> uninitialized state", let's also add a comment to the place that *DO*
>> that, like the shiny new sgx_reset_epc_page().
>
> Hi Dave,
>
> Sorry I am a little bit confused here. Do you mean we should add a comment in
> sgx_reset_epc_page() to say, for instance: sgx_free_epc_page() requires the EPC
> page already been EREMOVE'd?

I also don't understand Dave's comment. I would say

It's the caller's responsibility to make sure that the page is in
uninitialized state with EREMOVE (sgx_reset_epc_page), EWB etc. before
calling this function.

Paolo