Re: [PATCH v30 08/20] x86/sgx: Add functions to allocate and free EPC pages

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, May 26, 2020 at 02:52:08PM +0200, Borislav Petkov wrote:
> On Fri, May 15, 2020 at 03:43:58AM +0300, Jarkko Sakkinen wrote:
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 38424c1e8341..60d82e7537c8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -13,6 +13,66 @@
> >  struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> >  int sgx_nr_epc_sections;
> >  
> > +static struct sgx_epc_page *__sgx_try_alloc_page(struct sgx_epc_section *section)
> > +{
> > +	struct sgx_epc_page *page;
> > +
> > +	if (list_empty(&section->page_list))
> > +		return NULL;
> > +
> > +	page = list_first_entry(&section->page_list, struct sgx_epc_page, list);
> > +	list_del_init(&page->list);
> > +	return page;
> > +}
> > +
> > +/**
> > + * sgx_try_alloc_page() - Allocate an EPC page
> 
> Uuh, this is confusing. Looking forward into the patchset, you guys have
> 
> sgx_alloc_page()
> sgx_alloc_va_page()
>
> and this here
> 
> sgx_try_alloc_page()
> 
> So this one here should be called sgx_alloc_epc_page() if we're going to
> differentiate *what* it is allocating.

No objection to the rename.

> But then looking at sgx_alloc_page():
> 
> + * sgx_alloc_page() - Allocate an EPC page
> ...
> 
> + struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)
> 
> this one returns a struct sgx_epc_page * too.
> 
> The former "allocates" from the EPC cache so I'm thinking former should
> not have "alloc" in its name at all. It should be called something like
> 
> sgx_get_epc_page()
> 
> or so.

> Now, looking at sgx_alloc_page(), it does call this one -
> sgx_try_alloc_page() to get a page from the page list but it
> does not allocate anything. The actual allocation happens in
> sgx_alloc_epc_section() which actually does the k*alloc().

Ah.  The Enclave Page Cache isn't actually a cache, and it's not a kernel
construct.  The EPC is really just Enclave Memory, but Intel really likes
three letter acronyms.  On current CPUs, the EPC is carved out of main
memory via range registers, i.e. it's a statically located and sized chunk
of DRAM with special access rules that are enforced by the CPU.  It's no
more of a cache than regular DRAM. 

In other words, sgx_alloc_epc_section() is poorly named.  It doesn't
actually allocate EPC, it allocates kernel structures to map and track EPC.
sgx_(un)map_epc_section() would be more accurate and would hopefully
alleviate some of the confusion.

> Which sounds to me like those functions should not use "alloc" and
> "free" in their names but "get" and "put" to denote that they're simply
> getting pages from lists and returning them back to those lists.
> 
> IMNSVHO.

A better analogy is k*alloc() == sgx_alloc_epc_page() and
__sgx_alloc_try_alloc_page() == alloc_pages_node().  EPC sections aren't
guaranteed to have a 1:1 relationship with NUMA nodes, but that holds true
for current platforms.

I have no objection to renaming __sgx_alloc_try_alloc_page() to something
like sgx_alloc_epc_page_section or whatever, but IMO using get/put will be
horrendously confusing.