| Message ID | 20200515004410.723949-9-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX foundations | expand |
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 <sean.j.christopherson@intel.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Acked-by: Jethro Beekman <jethro@fortanix.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > 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.
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.
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.
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.
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 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
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
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 */