Re: [PATCH v5 06/18] x86/sgx: Introduce EPC page states

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Tue, 03 Oct 2023 15:03:48 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Mon, 2023-10-02 at 23:49 -0500, Haitao Huang wrote:
>> On Wed, 27 Sep 2023 05:28:36 -0500, Huang, Kai <kai.huang@intel.com>  
>> wrote:
>>
>> > On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote:
>> > > Use the lower 3 bits in the flags field of sgx_epc_page struct to
>> > > track EPC states in its life cycle and define an enum for possible
>> > > states. More state(s) will be added later.
>> >
>> > This patch does more than what the changelog claims to do.  AFAICT it
>> > does
>> > below:
>> >
>> >  1) Use the lower 3 bits to track EPC page status
>> >  2) Rename SGX_EPC_PAGE_RECLAIMER_TRACKED to SGX_EPC_PAGE_RERCLAIMABLE
>> >  3) Introduce a new state SGX_EPC_PAGE_UNRECLAIMABLE
>> >  4) Track SECS and VA pages as SGX_EPC_PAGE_UNRECLAIMABLE
>> >
>> > The changelog only says 1) IIUC.
>> >
>> I don't quite get why you would view 3) as a separate item from 1).
>
> 1) is about using some method to track EPC page status, 3) is adding a  
> new
> state.
>
> Why cannot they be separated?
>
>> In my view, 4) is not done as long as there is not separate list to  
>> track
>> it.
>
> You are literally doing below:
>
> @@ -113,6 +113,9 @@ static int sgx_encl_create(struct sgx_encl *encl,  
> struct
> sgx_secs *secs)
>  	encl->attributes = secs->attributes;
>  	encl->attributes_mask = SGX_ATTR_UNPRIV_MASK;
> +	sgx_record_epc_page(encl->secs.epc_page,
> +			    SGX_EPC_PAGE_UNRECLAIMABLE);
> +
>
> Which obviously is tracking SECS as unreclaimable page here.
>
> The only thing you are not doing now is to put to the actual list, which  
> you
> introduced in a later patch.
>
> But why not just doing them together?
>
>
I see where the problem is now.  Initially these states are bit masks so  
UNTRACKED and UNRECLAIMABLE are all not masked (set zero). I'll change  
these "record" calls with UNTRACKED instead, and later replace with  
UNRECLAIMABLE when they are actually added to the list. So UNRECLAIMABLE  
state can also be delayed until that patch with the list added.

Thanks.
Haitao