Messages in this thread | | | Subject | Re: [PATCH v5 09/18] x86/sgx: Store struct sgx_encl when allocating new VA pages | Date | Wed, 27 Sep 2023 10:35:24 -0500 | From | "Haitao Huang" <> |
| |
Hi Kai,
On Wed, 27 Sep 2023 06:14:20 -0500, Huang, Kai <kai.huang@intel.com> wrote:
> On Fri, 2023-09-22 at 20:06 -0700, Haitao Huang wrote: >> From: Sean Christopherson <sean.j.christopherson@intel.com> >> >> In a later patch, when a cgroup has exceeded the max capacity for EPC >> pages, it may need to identify and OOM kill a less active enclave to >> make room for other enclaves within the same group. Such a victim >> enclave would have no active pages other than the unreclaimable Version >> Array (VA) and SECS pages. Therefore, the cgroup needs examine its >> 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, >> which is not available for VA pages. >> >> Because struct sgx_epc_page instances of VA pages are not owned by an >> sgx_encl_page instance, mark their owner as sgx_encl: pass the struct >> sgx_encl of the enclave allocating the VA page to sgx_alloc_epc_page(), >> which 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 that can be examined by the cgroup to select the OOM >> killed enclave. >> >> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >> Co-developed-by: Kristen Carlson Accardi <kristen@linux.intel.com> >> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> >> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com> >> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> >> Cc: Sean Christopherson <seanjc@google.com> >> > > [...] > >> @@ -562,7 +562,7 @@ struct sgx_epc_page *sgx_alloc_epc_page(void >> *owner, bool reclaim) >> for ( ; ; ) { >> page = __sgx_alloc_epc_page(); >> if (!IS_ERR(page)) { >> - page->owner = owner; >> + page->encl_page = owner; > > Looks using 'encl_page' is arbitrary. > > Also actually for virtual EPC page the owner is set to the 'sgx_vepc' > instance. > >> break; >> } >> >> @@ -607,7 +607,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page) >> >> spin_lock(&node->lock); >> >> - page->owner = NULL; >> + page->encl_page = NULL; > > Ditto. > >> if (page->poison) >> list_add(&page->list, &node->sgx_poison_page_list); >> else >> @@ -642,7 +642,7 @@ static bool __init sgx_setup_epc_section(u64 >> phys_addr, u64 size, >> for (i = 0; i < nr_pages; i++) { >> section->pages[i].section = index; >> section->pages[i].flags = 0; >> - section->pages[i].owner = NULL; >> + section->pages[i].encl_page = NULL; >> section->pages[i].poison = 0; >> list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); >> } >> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h >> b/arch/x86/kernel/cpu/sgx/sgx.h >> index 764cec23f4e5..5110dd433b80 100644 >> --- a/arch/x86/kernel/cpu/sgx/sgx.h >> +++ b/arch/x86/kernel/cpu/sgx/sgx.h >> @@ -68,7 +68,12 @@ struct sgx_epc_page { >> unsigned int section; >> u16 flags; >> u16 poison; >> - struct sgx_encl_page *owner; >> + >> + /* Possible owner types */ >> + union { >> + struct sgx_encl_page *encl_page; >> + struct sgx_encl *encl; >> + }; > > Sadly for virtual EPC page the owner is set to the 'sgx_vepc' instance it > belongs to. > > Given how sgx_{alloc|free}_epc_page() arbitrarily uses encl_page, > perhaps we > should do below? > > union { > struct sgx_encl_page *encl_page; > struct sgx_encl *encl; > struct sgx_vepc *vepc; > void *owner; > }; > > And in sgx_{alloc|free}_epc_page() we can use 'owner' instead. >
As I mentioned in cover letter and change log in 11/18, this series does not track virtual EPC. We can add vepc field into the union in future if such tracking is needed. Don't think "void *owner" is needed though.
Thanks Haitao
| |