On Tue, 26 Jan 2021 08:04:35 -0800 Dave Hansen wrote:
> On 1/26/21 1:30 AM, Kai Huang wrote:
> > From: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> >
> > Encapsulate the snippet in sgx_free_epc_page() concerning EREMOVE to
> > sgx_reset_epc_page(), which is a static helper function for
> > sgx_encl_release(). It's the only function existing, which deals with
> > initialized pages.
>
> Yikes. I have no idea what that is saying. Here's a rewrite:
>
> EREMOVE takes a pages and removes any association between that page and
> an enclave. It must be run on a page before it can be added into
> another enclave. Currently, EREMOVE is run as part of pages being freed
> into the SGX page allocator. It is not expected to fail.
>
> KVM does not track how guest pages are used, which means that SGX
> virtualization use of EREMOVE might fail.
>
> Break out the EREMOVE call from the SGX page allocator. This will allow
> the SGX virtualization code to use the allocator directly. (SGX/KVM
> will also introduce a more permissive EREMOVE helper).
Thanks.
Hi Jarkko,
Do you want me to update your patch directly, or do you want to take the
change, and send me the patch again?
>
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index ee50a5010277..a78b71447771 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -389,6 +389,16 @@ const struct vm_operations_struct sgx_vm_ops = {
> > .access = sgx_vma_access,
> > };
> >
> > +
> > +static void sgx_reset_epc_page(struct sgx_epc_page *epc_page)
> > +{
> > + int ret;
> > +
> > + ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> > + if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > + return;
> > +}
> > +
> > /**
> > * sgx_encl_release - Destroy an enclave instance
> > * @kref: address of a kref inside &sgx_encl
> > @@ -412,6 +422,7 @@ void sgx_encl_release(struct kref *ref)
> > if (sgx_unmark_page_reclaimable(entry->epc_page))
> > continue;
> >
> > + sgx_reset_epc_page(entry->epc_page);
> > sgx_free_epc_page(entry->epc_page);
> > encl->secs_child_cnt--;
> > entry->epc_page = NULL;
> > @@ -423,6 +434,7 @@ void sgx_encl_release(struct kref *ref)
> > xa_destroy(&encl->page_array);
> >
> > if (!encl->secs_child_cnt && encl->secs.epc_page) {
> > + sgx_reset_epc_page(encl->secs.epc_page);
> > sgx_free_epc_page(encl->secs.epc_page);
> > encl->secs.epc_page = NULL;
> > }
> > @@ -431,6 +443,7 @@ void sgx_encl_release(struct kref *ref)
> > va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
> > list);
> > list_del(&va_page->list);
> > + sgx_reset_epc_page(va_page->epc_page);
> > sgx_free_epc_page(va_page->epc_page);
> > kfree(va_page);
> > }
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index f330abdb5bb1..21c2ffa13870 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -598,16 +598,14 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> > * sgx_free_epc_page() - Free an EPC page
> > * @page: an EPC page
> > *
> > - * Call EREMOVE for an EPC page and insert it back to the list of free pages.
> > + * Put the EPC page back to the list of free pages. It's the callers
>
> "caller's"
>
> > + * responsibility to make sure that the page is in uninitialized state In other
>
> Period after "state", please.
>
> > + * words, do EREMOVE, EWB or whatever operation is necessary before calling
> > + * this function.
> > */
>
> OK, so if you're going to say "the caller must put the page in
> uninitialized state", let's also add a comment to the place that *DO*
> that, like the shiny new sgx_reset_epc_page().
Hi Dave,
Sorry I am a little bit confused here. Do you mean we should add a comment in
sgx_reset_epc_page() to say, for instance: sgx_free_epc_page() requires the EPC
page already been EREMOVE'd?
