Re: [PATCH v3 01/28] x86/sgx: Store struct sgx_encl when allocating new VA pages

[Jarkko Sakkinen]

On Wed Jul 12, 2023 at 11:01 PM UTC, Haitao Huang wrote:
> In a later patch, when a cgroup has exceeded the max capacity for EPC pages
> and there are no more Enclave EPC pages associated with the cgroup that can
> be reclaimed, the only pages still associated with an enclave will be the
> unreclaimable Version Array (VA) pages or SECS pages, and the entire
> enclave will need to be killed to free up those pages.
>
> Currently, given an enclave pointer it is easy to find the associated VA
> pages and free them, however, OOM killing an enclave based on cgroup limits
> will require examining a cgroup's unreclaimable page list, and finding an
> enclave given a SECS page or a VA page. This will require a backpointer
> from a page to an enclave, including for VA pages.
>
> When allocating new Version Array (VA) pages, pass the struct sgx_encl of
> the enclave that is allocating the page. sgx_alloc_epc_page() will store
> this value in the owner field of the struct sgx_epc_page.  In a later
> patch, VA pages will be placed in an unreclaimable queue, and then when the
> cgroup max limit is reached and there are no more reclaimable pages and the
> enclave must be OOM killed, all the VA pages associated with that enclave
> can be uncharged and freed.
>
> To avoid casting needed to access the two types of owners: sgx_encl for VA
> pages, sgx_encl_page for other pages, replace 'owner' field in sgx_epc_page
> with a union of the two types.

I think the action taken is correct but the reasoning is a bit
convoluted.

Why not instead put something like:

"Because struct sgx_epc_page instances of VA pages are not owned by an
sgx_encl_page instance in the first place, mark their owner as sgx_encl,
in order to make it reachable from the unreclaimable list."

The code change itself, and rest of the paragraphs do look reasonable.

BR, Jarkko