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

From: Haitao Huang
Date: Tue Oct 03 2023 - 00:49:52 EST


On Wed, 27 Sep 2023 05:28:36 -0500, Huang, Kai <kai.huang@xxxxxxxxx> 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).
In my view, 4) is not done as long as there is not separate list to track it.
Maybe I should make it clear the "states" vs "tracking". States are just bits in the flags, "tracking" is done using the lists by ksgxd/cgroup. And this patch is really about "states"
Would that clarify the intention of the patch?

If we really want to do all these in one patch, then the changelog should at
least mention the justification of all of them.

But I don't see why 3) and 4) need to be done here. Instead, IMHO they should
be done in a separate patch, and do it after the unreclaimable list is
introduced (or you need to bring that patch forward).


For instance, ...

[snip]

+
+ /* Page is in use but tracked in an unreclaimable LRU list. These are
+ * only reclaimable when the whole enclave is OOM killed or the enclave
+ * is released, e.g., VA, SECS pages
+ * Becomes NOT_TRACKED after sgx_drop_epc()
+ */
+ SGX_EPC_PAGE_UNRECLAIMABLE = 3,

... We even don't have the unreclaimable LRU list yet. It's odd to have this
comment here.


Yeah, I should take out the mentioning of the LRUs from definitions of the states.

Thanks
Haitao