Add functions for allocating page from Enclave Page Cache (EPC). A page is
allocated by going through the EPC sections and returning the first free
page.
When a page is freed, it might have a valid state, which means that the
callee has assigned it to an enclave, which are protected memory ares used
to run code protected from outside access. The page is returned back to the
invalid state with ENCLS[EREMOVE] [1].
[1] Intel SDM: 40.3 INTEL® SGX SYSTEM LEAF FUNCTION REFERENCE
Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
Acked-by: Jethro Beekman <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
arch/x86/kernel/cpu/sgx/main.c | 60 ++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 3 ++
2 files changed, 63 insertions(+)
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(§ion->page_list))
+ return NULL;
+
+ page = list_first_entry(§ion->page_list, struct sgx_epc_page, list);
+ list_del_init(&page->list);
+ return page;
+}
+
+/**
+ * sgx_try_alloc_page() - Allocate an EPC page
+ *
+ * Try to grab a page from the free EPC page list.
+ *
+ * Return:
+ * a pointer to a &struct sgx_epc_page instance,
+ * -errno on error
+ */
+struct sgx_epc_page *sgx_try_alloc_page(void)
+{
+ struct sgx_epc_section *section;
+ struct sgx_epc_page *page;
+ int i;
+
+ for (i = 0; i < sgx_nr_epc_sections; i++) {
+ section = &sgx_epc_sections[i];
+ spin_lock(§ion->lock);
+ page = __sgx_try_alloc_page(section);
+ spin_unlock(§ion->lock);
+
+ if (page)
+ return page;
+ }
+
+ return ERR_PTR(-ENOMEM);
+}
+
+/**
+ * sgx_free_page() - Free an EPC page
+ * @page: pointer a previously allocated EPC page
+ *
+ * EREMOVE an EPC page and insert it back to the list of free pages.
+ */
+void sgx_free_page(struct sgx_epc_page *page)
+{
+ struct sgx_epc_section *section = sgx_epc_section(page);
+ int ret;
+
+ ret = __eremove(sgx_epc_addr(page));
+ if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
+ return;
+
+ spin_lock(§ion->lock);
+ list_add_tail(&page->list, §ion->page_list);
+ spin_unlock(§ion->lock);
+}
+
static void __init sgx_free_epc_section(struct sgx_epc_section *section)
{
struct sgx_epc_page *page;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index aad30980be32..aa85f85412d8 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -67,4 +67,7 @@ extern struct task_struct *ksgxswapd_tsk;
bool __init sgx_page_reclaimer_init(void);
+struct sgx_epc_page *sgx_try_alloc_page(void);
+void sgx_free_page(struct sgx_epc_page *page);
+
#endif /* _X86_SGX_H */
--
2.25.1
On Fri, May 15, 2020 at 03:43:58AM +0300, Jarkko Sakkinen wrote:
> Add functions for allocating page from Enclave Page Cache (EPC). A page is
pages
> allocated by going through the EPC sections and returning the first free
> page.
>
> When a page is freed, it might have a valid state, which means that the
> callee has assigned it to an enclave, which are protected memory ares used
areas
although explaining what enclaves are has already happened so probably
not needed here too.
> to run code protected from outside access. The page is returned back to the
> invalid state with ENCLS[EREMOVE] [1].
>
> [1] Intel SDM: 40.3 INTEL® SGX SYSTEM LEAF FUNCTION REFERENCE
>
> Co-developed-by: Sean Christopherson <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> Acked-by: Jethro Beekman <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> arch/x86/kernel/cpu/sgx/main.c | 60 ++++++++++++++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/sgx.h | 3 ++
> 2 files changed, 63 insertions(+)
>
> 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(§ion->page_list))
> + return NULL;
> +
> + page = list_first_entry(§ion->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.
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().
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.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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(§ion->page_list))
> > + return NULL;
> > +
> > + page = list_first_entry(§ion->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.
On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> 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.
Makes sense.
> 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.
Ok. My only issue is that the naming nomenclature sounds strange and
confusing as it is. "try" in an "alloc" function is kinda tautological -
of course the function will try to do its best. :)
And there are three functions having "alloc" in the name so I can
imagine someone getting very confused when having to stare at that code.
So at least naming them in a way so that it is clear what kind of pages
they "allocate" - i.e., what they actually do - would be a step in the
right direction...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, May 27, 2020 at 10:46:38PM +0200, Borislav Petkov wrote:
> On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> > 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.
>
> Makes sense.
>
> > 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.
>
> Ok. My only issue is that the naming nomenclature sounds strange and
> confusing as it is. "try" in an "alloc" function is kinda tautological -
> of course the function will try to do its best. :)
Heh, so what you're saying is we should add __sgx_really_try_alloc_page()?
> And there are three functions having "alloc" in the name so I can
> imagine someone getting very confused when having to stare at that code.
>
> So at least naming them in a way so that it is clear what kind of pages
> they "allocate" - i.e., what they actually do - would be a step in the
> right direction...
Ya, and things will only get more confusing when actual NUMA awareness gets
thrown into the mix.
Jarkko, splicing in the NUMA awareness code, what do you think about:
sgx_alloc_epc_section -> sgx_map_epc_section
sgx_free_epc_section -> sgx_unmap_epc_section
sgx_alloc_page -> sgx_alloc_epc_page
sgx_free_page -> sgx_free_epc_page
sgx_try_alloc_page -> sgx_alloc_epc_page_node
__sgx_try_alloc_page -> sgx_alloc_epc_page_section
On Wed, May 27, 2020 at 10:46:38PM +0200, Borislav Petkov wrote:
> On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> > 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.
>
> Makes sense.
>
> > 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.
>
> Ok. My only issue is that the naming nomenclature sounds strange and
> confusing as it is. "try" in an "alloc" function is kinda tautological -
> of course the function will try to do its best. :)
>
> And there are three functions having "alloc" in the name so I can
> imagine someone getting very confused when having to stare at that code.
>
> So at least naming them in a way so that it is clear what kind of pages
> they "allocate" - i.e., what they actually do - would be a step in the
> right direction...
>
> Thx.
I also think sgx_get_epc_page() is also kind of confusing name. The
reason is that "get" can many things.
I'm not sure I follow fully Sean's reasoning but the way alloc is used
mostly in Linux is to ask through some API the used kernel memory
allocator to give memory for some kernel data structures.
Agreed that it is not the best match on what we are doing.
The first common verb that comes to my mind about the action taken is
grabbing. Attached a patch reflecting this idea that I'm ready to
squash.
/Jarkko
On Thu, May 28, 2020 at 04:23:19AM +0300, Jarkko Sakkinen wrote:
> On Wed, May 27, 2020 at 10:46:38PM +0200, Borislav Petkov wrote:
> > On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> > > 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.
...
> I'm not sure I follow fully Sean's reasoning but the way alloc is used
> mostly in Linux is to ask through some API the used kernel memory
> allocator to give memory for some kernel data structures.
Function names are usually some form of
<namespace>_<verb>_<object>
where 'object' is the target of the 'verb'. So sgx_alloc_epc_section()
is most likely going to be read as "SGX, allocate an EPC section". But
that code doesn't allocate an EPC section, it maps an EPC section, and on
success, adds the section's pages to the unsanitized list, i.e. what
effectively becomes the pool of EPC pages. The allocation part is a side
effect of how we track EPC pages, it's not the primary purpose of the
function.
Maybe sgx_add_epc_section() and sgx_remove_epc_section() would be better
than map/unmap?
Eliminating the misnamed sgx_alloc_epc_section() frees up the "alloc" verb
for use in the actual EPC page allocation paths, i.e. avoids having to
rename those to "grab". IMO, "alloc" is the best name as it most closely
aligns with the nomenclature for regular pages, e.g. "grab" is most often
used to elevate refcounts.
On Wed, May 27, 2020 at 05:52:17PM -0700, Sean Christopherson wrote:
> On Wed, May 27, 2020 at 10:46:38PM +0200, Borislav Petkov wrote:
> > On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> > > 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.
> >
> > Makes sense.
> >
> > > 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.
> >
> > Ok. My only issue is that the naming nomenclature sounds strange and
> > confusing as it is. "try" in an "alloc" function is kinda tautological -
> > of course the function will try to do its best. :)
>
> Heh, so what you're saying is we should add __sgx_really_try_alloc_page()?
>
> > And there are three functions having "alloc" in the name so I can
> > imagine someone getting very confused when having to stare at that code.
> >
> > So at least naming them in a way so that it is clear what kind of pages
> > they "allocate" - i.e., what they actually do - would be a step in the
> > right direction...
>
> Ya, and things will only get more confusing when actual NUMA awareness gets
> thrown into the mix.
>
> Jarkko, splicing in the NUMA awareness code, what do you think about:
>
> sgx_alloc_epc_section -> sgx_map_epc_section
> sgx_free_epc_section -> sgx_unmap_epc_section
Here alloc makes sense because memory gets allocated for the data
structures.
> sgx_alloc_page -> sgx_alloc_epc_page
> sgx_free_page -> sgx_free_epc_page
>
> sgx_try_alloc_page -> sgx_alloc_epc_page_node
> __sgx_try_alloc_page -> sgx_alloc_epc_page_section
I'm going with sgx_grab_page() and sgx_try_grab_page().
/Jarkko
On Wed, May 27, 2020 at 06:36:18PM -0700, Sean Christopherson wrote:
> On Thu, May 28, 2020 at 04:23:19AM +0300, Jarkko Sakkinen wrote:
> > On Wed, May 27, 2020 at 10:46:38PM +0200, Borislav Petkov wrote:
> > > On Tue, May 26, 2020 at 09:21:11PM -0700, Sean Christopherson wrote:
> > > > 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.
>
> ...
>
> > I'm not sure I follow fully Sean's reasoning but the way alloc is used
> > mostly in Linux is to ask through some API the used kernel memory
> > allocator to give memory for some kernel data structures.
>
> Function names are usually some form of
>
> <namespace>_<verb>_<object>
>
> where 'object' is the target of the 'verb'. So sgx_alloc_epc_section()
> is most likely going to be read as "SGX, allocate an EPC section". But
> that code doesn't allocate an EPC section, it maps an EPC section, and on
> success, adds the section's pages to the unsanitized list, i.e. what
> effectively becomes the pool of EPC pages. The allocation part is a side
> effect of how we track EPC pages, it's not the primary purpose of the
> function.
>
> Maybe sgx_add_epc_section() and sgx_remove_epc_section() would be better
> than map/unmap?
>
> Eliminating the misnamed sgx_alloc_epc_section() frees up the "alloc" verb
> for use in the actual EPC page allocation paths, i.e. avoids having to
> rename those to "grab". IMO, "alloc" is the best name as it most closely
> aligns with the nomenclature for regular pages, e.g. "grab" is most often
> used to elevate refcounts.
I'm thinking that you are over-engineering something this :-) Naming is
never perfect.
But I do get the original comment about sgx_alloc_page().
/Jarkko
Lemme reply to all mails with one. :-)
I think Sean almost had it:
> sgx_alloc_epc_section -> sgx_map_epc_section
> sgx_free_epc_section -> sgx_unmap_epc_section
Or even
sgx_setup_epc_section()
sgx_free_epc_section()
> sgx_alloc_page -> sgx_alloc_epc_page
> sgx_free_page -> sgx_free_epc_page
>
> sgx_try_alloc_page -> sgx_alloc_epc_page_node
> __sgx_try_alloc_page -> sgx_alloc_epc_page_section
And except those last two. Those are allocating a page from the EPC
sections so I'd call them:
sgx_try_alloc_page -> sgx_alloc_epc_page_section
__sgx_try_alloc_page -> __sgx_alloc_epc_page_section
former doing the loop, latter doing the per-section list games.
> I'm not sure I follow fully Sean's reasoning but the way alloc is used
> mostly in Linux is to ask through some API the used kernel memory
> allocator to give memory for some kernel data structures.
>
> Agreed that it is not the best match on what we are doing.
Yes, ok, let's stay with "alloc". Agreed.
> I'm going with sgx_grab_page() and sgx_try_grab_page().
And let's simply forget the "try" - all the functions that can fail and
return an error code, they all try. :-)
> So sgx_alloc_epc_section() is most likely going to be read as "SGX,
> allocate an EPC section".
Or, allocate *from* the EPC section if it returns a pointer to a page
and the comment above it says so...
So to sum up:
* sgx_{alloc,setup}_epc_section - sets up the EPC section and the pages
which belong to it.
* sgx_alloc_page - allocates an EPC page
* sgx_alloc_epc_page_section - allocates a page from any EPC section
I think that makes it pretty clear what each function does...
> I'm thinking that you are over-engineering something this :-) Naming is
> never perfect.
I know, but naming stuff right will pay off later.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> * sgx_alloc_page - allocates an EPC page
Did you want this to be sgx_alloc_epc_page?
On Thu, May 28, 2020 at 10:19:37AM -0700, Sean Christopherson wrote:
> On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> > * sgx_alloc_page - allocates an EPC page
>
> Did you want this to be sgx_alloc_epc_page?
Whatever you guys prefer. I'd use "sgx_alloc_page" because it returns
struct sgx_epc_page * but having "epc" in the name makes it even more
explicit so either is just fine.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, May 28, 2020 at 07:27:24PM +0200, Borislav Petkov wrote:
> On Thu, May 28, 2020 at 10:19:37AM -0700, Sean Christopherson wrote:
> > On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> > > * sgx_alloc_page - allocates an EPC page
> >
> > Did you want this to be sgx_alloc_epc_page?
>
> Whatever you guys prefer. I'd use "sgx_alloc_page" because it returns
> struct sgx_epc_page * but having "epc" in the name makes it even more
> explicit so either is just fine.
sgx_alloc_page() works for me, I just wanted to make sure it wasn't a typo.
Thanks!
On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> Lemme reply to all mails with one. :-)
>
> I think Sean almost had it:
>
> > sgx_alloc_epc_section -> sgx_map_epc_section
> > sgx_free_epc_section -> sgx_unmap_epc_section
>
> Or even
>
> sgx_setup_epc_section()
> sgx_free_epc_section()
I like these. Lets lock in.
> And except those last two. Those are allocating a page from the EPC
> sections so I'd call them:
>
> sgx_try_alloc_page -> sgx_alloc_epc_page_section
> __sgx_try_alloc_page -> __sgx_alloc_epc_page_section
>
> former doing the loop, latter doing the per-section list games.
sgx_alloc_epc_page_section() is a bit nasty and long name to use for
grabbing a page. And even the documentation spoke about grabbing before
this naming discussion. I think it is a great description what is going
on. Everytime I talk about the subject I talk about grabbing.
Lets just say that your suggestion, I could not use in a conference
talk as a verb when I describe what is going on. That function
signature does not fit to my mouth :-) I would talk about
grabbing a page.
This how I refactored yesterday (is in my GIT tree already):
/**
* sgx_grab_page() - Grab a free EPC page
* @owner: the owner of the EPC page
* @reclaim: reclaim pages if necessary
*
* Iterate through EPC sections and borrow a free EPC page to the caller. When a
* page is no longer needed it must be released with sgx_free_page(). If
* @reclaim is set to true, directly reclaim pages when we are out of pages. No
* mm's can be locked when @reclaim is set to true.
*
* Finally, wake up ksgxswapd when the number of pages goes below the watermark
* before returning back to the caller.
*
* Return:
* an EPC page,
* -errno on error
*/
I also rewrote the kdoc.
I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
/Jarkko
On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> > Lemme reply to all mails with one. :-)
> > And except those last two. Those are allocating a page from the EPC
> > sections so I'd call them:
> >
> > sgx_try_alloc_page -> sgx_alloc_epc_page_section
> > __sgx_try_alloc_page -> __sgx_alloc_epc_page_section
> >
> > former doing the loop, latter doing the per-section list games.
>
> sgx_alloc_epc_page_section() is a bit nasty and long name to use for
> grabbing a page. And even the documentation spoke about grabbing before
> this naming discussion. I think it is a great description what is going
> on. Everytime I talk about the subject I talk about grabbing.
>
> Lets just say that your suggestion, I could not use in a conference
> talk as a verb when I describe what is going on. That function
> signature does not fit to my mouth :-) I would talk about
> grabbing a page.
"allocate an EPC page from the specified section"
It also works if/when we add NUMA awareness, e.g. sgx_alloc_epc_page_node()
means "allocate an EPC page from the specified node". Note that I'm not
inventing these from scratch, simply stealing them from alloc_pages() and
alloc_pages_node(). The section thing is unique to SGX, but the underlying
concept is the same.
> This how I refactored yesterday (is in my GIT tree already):
>
> /**
> * sgx_grab_page() - Grab a free EPC page
> * @owner: the owner of the EPC page
> * @reclaim: reclaim pages if necessary
> *
> * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> * page is no longer needed it must be released with sgx_free_page(). If
> * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> * mm's can be locked when @reclaim is set to true.
> *
> * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> * before returning back to the caller.
> *
> * Return:
> * an EPC page,
> * -errno on error
> */
>
> I also rewrote the kdoc.
>
> I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
FWIW, I really, really dislike "grab". The nomenclature for normal memory
and pages uses "alloc" when taking a page off a free list, and "grab" when
elevating the refcount. I don't understand the motivation for diverging
from that. SGX is weird enough as is, using names that don't align with
exist norms will only serve to further obfuscate the code.
On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> > > Lemme reply to all mails with one. :-)
> > > And except those last two. Those are allocating a page from the EPC
> > > sections so I'd call them:
> > >
> > > sgx_try_alloc_page -> sgx_alloc_epc_page_section
> > > __sgx_try_alloc_page -> __sgx_alloc_epc_page_section
> > >
> > > former doing the loop, latter doing the per-section list games.
> >
> > sgx_alloc_epc_page_section() is a bit nasty and long name to use for
> > grabbing a page. And even the documentation spoke about grabbing before
> > this naming discussion. I think it is a great description what is going
> > on. Everytime I talk about the subject I talk about grabbing.
> > Lets just say that your suggestion, I could not use in a conference
> > talk as a verb when I describe what is going on. That function
> > signature does not fit to my mouth :-) I would talk about
> > grabbing a page.
>
> "allocate an EPC page from the specified section"
>
> It also works if/when we add NUMA awareness, e.g. sgx_alloc_epc_page_node()
> means "allocate an EPC page from the specified node". Note that I'm not
> inventing these from scratch, simply stealing them from alloc_pages() and
> alloc_pages_node(). The section thing is unique to SGX, but the underlying
> concept is the same.
Then it should be sgx_alloc_epc_page_from_section() if you go with that.
Otherwise it is mixes too much with the section. I did read these mails
first quickly and first thought that functions were doing something with
sgx_epc_section and not with pages.
Only with a deeper look that it's the name for allocating a page.
I think both names are waste of screen estate. Too long.
> > * sgx_grab_page() - Grab a free EPC page
> > * @owner: the owner of the EPC page
> > * @reclaim: reclaim pages if necessary
> > *
> > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > * page is no longer needed it must be released with sgx_free_page(). If
> > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > * mm's can be locked when @reclaim is set to true.
> > *
> > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > * before returning back to the caller.
> > *
> > * Return:
> > * an EPC page,
> > * -errno on error
> > */
> >
> > I also rewrote the kdoc.
> >
> > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
>
> FWIW, I really, really dislike "grab". The nomenclature for normal memory
> and pages uses "alloc" when taking a page off a free list, and "grab" when
> elevating the refcount. I don't understand the motivation for diverging
> from that. SGX is weird enough as is, using names that don't align with
> exist norms will only serve to further obfuscate the code.
OK, what would be a better name then? The semantics are not standard
memory allocation semantics in the first place. And kdoc in v30 speaks
about grabbing.
/Jarkko
On Fri, May 29, 2020 at 06:28:16AM +0300, Jarkko Sakkinen wrote:
> On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> > On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > > * sgx_grab_page() - Grab a free EPC page
> > > * @owner: the owner of the EPC page
> > > * @reclaim: reclaim pages if necessary
> > > *
> > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > > * page is no longer needed it must be released with sgx_free_page(). If
> > > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > > * mm's can be locked when @reclaim is set to true.
> > > *
> > > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > > * before returning back to the caller.
> > > *
> > > * Return:
> > > * an EPC page,
> > > * -errno on error
> > > */
> > >
> > > I also rewrote the kdoc.
> > >
> > > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
> >
> > FWIW, I really, really dislike "grab". The nomenclature for normal memory
> > and pages uses "alloc" when taking a page off a free list, and "grab" when
> > elevating the refcount. I don't understand the motivation for diverging
> > from that. SGX is weird enough as is, using names that don't align with
> > exist norms will only serve to further obfuscate the code.
>
> OK, what would be a better name then? The semantics are not standard
> memory allocation semantics in the first place. And kdoc in v30 speaks
> about grabbing.
In what way are they not standard allocation semantics? sgx_alloc_page()
is an API to allocate (EPC) memory on-demand, sgx_free_page() is its partner
to free that memory when it is no longer needed. There are many different
ways to manage and allocate memory, but the basic premise is the same for
all and no different than what we're doing.
On Fri, May 29, 2020 at 06:28:28AM +0300, Jarkko Sakkinen wrote:
> On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> > On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, May 28, 2020 at 07:16:35PM +0200, Borislav Petkov wrote:
> > > > Lemme reply to all mails with one. :-)
> > > > And except those last two. Those are allocating a page from the EPC
> > > > sections so I'd call them:
> > > >
> > > > sgx_try_alloc_page -> sgx_alloc_epc_page_section
> > > > __sgx_try_alloc_page -> __sgx_alloc_epc_page_section
> > > >
> > > > former doing the loop, latter doing the per-section list games.
> > >
> > > sgx_alloc_epc_page_section() is a bit nasty and long name to use for
> > > grabbing a page. And even the documentation spoke about grabbing before
> > > this naming discussion. I think it is a great description what is going
> > > on. Everytime I talk about the subject I talk about grabbing.
> > > Lets just say that your suggestion, I could not use in a conference
> > > talk as a verb when I describe what is going on. That function > > > signature does not fit to my mouth :-) I would talk about
> > > grabbing a page.
> >
> > "allocate an EPC page from the specified section"
> >
> > It also works if/when we add NUMA awareness, e.g. sgx_alloc_epc_page_node()
> > means "allocate an EPC page from the specified node". Note that I'm not
> > inventing these from scratch, simply stealing them from alloc_pages() and
> > alloc_pages_node(). The section thing is unique to SGX, but the underlying
> > concept is the same.
>
> Then it should be sgx_alloc_epc_page_from_section() if you go with that.
> Otherwise it is mixes too much with the section. I did read these mails
> first quickly and first thought that functions were doing something with
> sgx_epc_section and not with pages.
>
> Only with a deeper look that it's the name for allocating a page.
>
> I think both names are waste of screen estate. Too long.
>
> > > * sgx_grab_page() - Grab a free EPC page
> > > * @owner: the owner of the EPC page
> > > * @reclaim: reclaim pages if necessary
> > > *
> > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > > * page is no longer needed it must be released with sgx_free_page(). If
> > > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > > * mm's can be locked when @reclaim is set to true.
> > > *
> > > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > > * before returning back to the caller.
> > > *
> > > * Return:
> > > * an EPC page,
> > > * -errno on error
> > > */
> > >
> > > I also rewrote the kdoc.
> > >
> > > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
> >
> > FWIW, I really, really dislike "grab". The nomenclature for normal memory
> > and pages uses "alloc" when taking a page off a free list, and "grab" when
> > elevating the refcount. I don't understand the motivation for diverging
> > from that. SGX is weird enough as is, using names that don't align with
> > exist norms will only serve to further obfuscate the code.
>
> OK, what would be a better name then? The semantics are not standard
> memory allocation semantics in the first place. And kdoc in v30 speaks
> about grabbing.
I can live with sgx_alloc_epc_page() or sgx_alloc_epc_page_from_section()
although I'd prefer the shorter form.
/Jarkko
On Thu, May 28, 2020 at 08:37:16PM -0700, Sean Christopherson wrote:
> On Fri, May 29, 2020 at 06:28:16AM +0300, Jarkko Sakkinen wrote:
> > On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> > > On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > > > * sgx_grab_page() - Grab a free EPC page
> > > > * @owner: the owner of the EPC page
> > > > * @reclaim: reclaim pages if necessary
> > > > *
> > > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > > > * page is no longer needed it must be released with sgx_free_page(). If
> > > > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > > > * mm's can be locked when @reclaim is set to true.
> > > > *
> > > > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > > > * before returning back to the caller.
> > > > *
> > > > * Return:
> > > > * an EPC page,
> > > > * -errno on error
> > > > */
> > > >
> > > > I also rewrote the kdoc.
> > > >
> > > > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
> > >
> > > FWIW, I really, really dislike "grab". The nomenclature for normal memory
> > > and pages uses "alloc" when taking a page off a free list, and "grab" when
> > > elevating the refcount. I don't understand the motivation for diverging
> > > from that. SGX is weird enough as is, using names that don't align with
> > > exist norms will only serve to further obfuscate the code.
> >
> > OK, what would be a better name then? The semantics are not standard
> > memory allocation semantics in the first place. And kdoc in v30 speaks
> > about grabbing.
>
> In what way are they not standard allocation semantics? sgx_alloc_page()
> is an API to allocate (EPC) memory on-demand, sgx_free_page() is its partner
> to free that memory when it is no longer needed. There are many different
> ways to manage and allocate memory, but the basic premise is the same for
> all and no different than what we're doing.
I'll go with sgx_alloc_epc_page(). It is more precise name.
It's not as precise as sgx_alloc_epc_page_from_section but is a great
compromise between scree estate and clarity of expression :-)
/Jarkko
On Fri, May 29, 2020 at 11:13:09AM +0300, Jarkko Sakkinen wrote:
> On Thu, May 28, 2020 at 08:37:16PM -0700, Sean Christopherson wrote:
> > On Fri, May 29, 2020 at 06:28:16AM +0300, Jarkko Sakkinen wrote:
> > > On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> > > > On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > > > > * sgx_grab_page() - Grab a free EPC page
> > > > > * @owner: the owner of the EPC page
> > > > > * @reclaim: reclaim pages if necessary
> > > > > *
> > > > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > > > > * page is no longer needed it must be released with sgx_free_page(). If
> > > > > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > > > > * mm's can be locked when @reclaim is set to true.
> > > > > *
> > > > > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > > > > * before returning back to the caller.
> > > > > *
> > > > > * Return:
> > > > > * an EPC page,
> > > > > * -errno on error
> > > > > */
> > > > >
> > > > > I also rewrote the kdoc.
> > > > >
> > > > > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
> > > >
> > > > FWIW, I really, really dislike "grab". The nomenclature for normal memory
> > > > and pages uses "alloc" when taking a page off a free list, and "grab" when
> > > > elevating the refcount. I don't understand the motivation for diverging
> > > > from that. SGX is weird enough as is, using names that don't align with
> > > > exist norms will only serve to further obfuscate the code.
> > >
> > > OK, what would be a better name then? The semantics are not standard
> > > memory allocation semantics in the first place. And kdoc in v30 speaks
> > > about grabbing.
> >
> > In what way are they not standard allocation semantics? sgx_alloc_page()
> > is an API to allocate (EPC) memory on-demand, sgx_free_page() is its partner
> > to free that memory when it is no longer needed. There are many different
> > ways to manage and allocate memory, but the basic premise is the same for
> > all and no different than what we're doing.
>
> I end up to (ignoring unchanged names):
>
> - __sgx_alloc_epc_page_from_section() (static)
> - __sgx_alloc_epc_page()
> - sgx_alloc_epc_page()
> - sgx_setup_epc_section()
OK, also sgx_free_epc_page (not sgx_free_page() as before).
/Jarkko
On Thu, May 28, 2020 at 08:37:16PM -0700, Sean Christopherson wrote:
> On Fri, May 29, 2020 at 06:28:16AM +0300, Jarkko Sakkinen wrote:
> > On Thu, May 28, 2020 at 12:59:17PM -0700, Sean Christopherson wrote:
> > > On Thu, May 28, 2020 at 10:07:18PM +0300, Jarkko Sakkinen wrote:
> > > > * sgx_grab_page() - Grab a free EPC page
> > > > * @owner: the owner of the EPC page
> > > > * @reclaim: reclaim pages if necessary
> > > > *
> > > > * Iterate through EPC sections and borrow a free EPC page to the caller. When a
> > > > * page is no longer needed it must be released with sgx_free_page(). If
> > > > * @reclaim is set to true, directly reclaim pages when we are out of pages. No
> > > > * mm's can be locked when @reclaim is set to true.
> > > > *
> > > > * Finally, wake up ksgxswapd when the number of pages goes below the watermark
> > > > * before returning back to the caller.
> > > > *
> > > > * Return:
> > > > * an EPC page,
> > > > * -errno on error
> > > > */
> > > >
> > > > I also rewrote the kdoc.
> > > >
> > > > I do agree that sgx_try_grab_page() should be renamed as __sgx_grab_page().
> > >
> > > FWIW, I really, really dislike "grab". The nomenclature for normal memory
> > > and pages uses "alloc" when taking a page off a free list, and "grab" when
> > > elevating the refcount. I don't understand the motivation for diverging
> > > from that. SGX is weird enough as is, using names that don't align with
> > > exist norms will only serve to further obfuscate the code.
> >
> > OK, what would be a better name then? The semantics are not standard
> > memory allocation semantics in the first place. And kdoc in v30 speaks
> > about grabbing.
>
> In what way are they not standard allocation semantics? sgx_alloc_page()
> is an API to allocate (EPC) memory on-demand, sgx_free_page() is its partner
> to free that memory when it is no longer needed. There are many different
> ways to manage and allocate memory, but the basic premise is the same for
> all and no different than what we're doing.
I end up to (ignoring unchanged names):
- __sgx_alloc_epc_page_from_section() (static)
- __sgx_alloc_epc_page()
- sgx_alloc_epc_page()
- sgx_setup_epc_section()
/Jarkko