Encapsulted pinning of the backing page into sgx_eldu in order to drop duplicate code. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen(a)linux.intel.com&gt; --- drivers/platform/x86/intel_sgx_vma.c | 54 ++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index 11d1c3b..73ec032 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -93,25 +93,33 @@ static void sgx_vma_close(struct vm_area_struct *vma) kref_put(&encl->refcount, sgx_encl_release); } -static int do_eldu(struct sgx_encl *encl, - struct sgx_encl_page *encl_page, - struct sgx_epc_page *epc_page, - struct page *backing, - bool is_secs) +static int sgx_eldu(struct sgx_encl *encl, + struct sgx_encl_page *encl_page, + struct sgx_epc_page *epc_page, + bool is_secs) { + struct page *backing; struct sgx_page_info pginfo; void *secs_ptr = NULL; void *epc_ptr; void *va_ptr; int ret; - pginfo.srcpge = (unsigned long)kmap_atomic(backing); + backing = sgx_get_backing(encl, encl_page); + if (IS_ERR(backing)) { + ret = PTR_ERR(backing); + sgx_warn(encl, "pinning the backing page for ELDU failed with %d\n", + ret); + return ret; + } + if (!is_secs) secs_ptr = sgx_get_epc_page(encl->secs_page.epc_page); pginfo.secs = (unsigned long)secs_ptr; epc_ptr = sgx_get_epc_page(epc_page); va_ptr = sgx_get_epc_page(encl_page->va_page->epc_page); + pginfo.srcpge = (unsigned long)kmap_atomic(backing); pginfo.linaddr = is_secs ? 0 : encl_page->addr; pginfo.pcmd = (unsigned long)&encl_page->pcmd; @@ -121,16 +129,19 @@ static int do_eldu(struct sgx_encl *encl, (unsigned long)va_ptr + encl_page->va_offset); + kunmap_atomic((void *)(unsigned long)pginfo.srcpge); sgx_put_epc_page(va_ptr); sgx_put_epc_page(epc_ptr); if (!is_secs) sgx_put_epc_page(secs_ptr); - kunmap_atomic((void *)(unsigned long)pginfo.srcpge); - WARN_ON(ret); - if (ret) + sgx_put_backing(backing, false); + + if (ret) { + sgx_err(encl, "ELDU returned %d\n", ret); return -EFAULT; + } return 0; } @@ -142,7 +153,6 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, struct sgx_encl_page *entry; struct sgx_epc_page *epc_page = NULL; struct sgx_epc_page *secs_epc_page = NULL; - struct page *backing; unsigned int free_flags = SGX_FREE_SKIP_EREMOVE; int rc; @@ -201,15 +211,7 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, goto out; } - backing = sgx_get_backing(encl, &encl->secs_page); - if (IS_ERR(backing)) { - entry = (void *)backing; - goto out; - } - - rc = do_eldu(encl, &encl->secs_page, secs_epc_page, - backing, true /* is_secs */); - sgx_put_backing(backing, 0); + rc = sgx_eldu(encl, &encl->secs_page, secs_epc_page, true); if (rc) goto out; @@ -220,28 +222,20 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, secs_epc_page = NULL; } - backing = sgx_get_backing(encl, entry); - if (IS_ERR(backing)) { - entry = (void *)backing; - goto out; - } - - rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */); if (rc) { - sgx_put_backing(backing, 0); entry = ERR_PTR(rc); goto out; } - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); - sgx_put_backing(backing, 0); - + rc = sgx_eldu(encl, entry, epc_page, false /* is_secs */); if (rc) { free_flags = 0; entry = ERR_PTR(rc); goto out; } + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); + encl->secs_child_cnt++; entry->epc_page = epc_page; -- 2.9.3

Move PCMD's to a backing storage file in order to give more control to do swapping and discarding. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen(a)linux.intel.com&gt; --- drivers/platform/x86/intel_sgx.h | 5 ++++- drivers/platform/x86/intel_sgx_ioctl.c | 12 +++++++++++ drivers/platform/x86/intel_sgx_page_cache.c | 19 ++++++++++++++++-- drivers/platform/x86/intel_sgx_util.c | 30 ++++++++++++++++++++++++++++ drivers/platform/x86/intel_sgx_vma.c | 31 +++++++++++++++++++++-------- 5 files changed, 86 insertions(+), 11 deletions(-) diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index ed9e8e6..1d03606 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -115,7 +115,6 @@ struct sgx_encl_page { struct list_head load_list; struct sgx_va_page *va_page; unsigned int va_offset; - struct sgx_pcmd pcmd; }; struct sgx_tgid_ctx { @@ -141,6 +140,7 @@ struct sgx_encl { struct task_struct *owner; struct mm_struct *mm; struct file *backing; + struct file *pcmd; struct list_head load_list; struct kref refcount; unsigned long base; @@ -198,6 +198,9 @@ void sgx_put_epc_page(void *epc_page_vaddr); struct page *sgx_get_backing(struct sgx_encl *encl, struct sgx_encl_page *entry); void sgx_put_backing(struct page *backing, bool write); +struct page *sgx_get_pcmd(struct sgx_encl *encl, + struct sgx_encl_page *entry); +void sgx_put_pcmd(struct page *pcmd_page, bool write); void sgx_insert_pte(struct sgx_encl *encl, struct sgx_encl_page *encl_page, struct sgx_epc_page *epc_page, diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c index b78c552..ce6a020 100644 --- a/drivers/platform/x86/intel_sgx_ioctl.c +++ b/drivers/platform/x86/intel_sgx_ioctl.c @@ -480,6 +480,7 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, struct vm_area_struct *vma; void *secs_vaddr = NULL; struct file *backing; + struct file *pcmd; long ret; secs = kzalloc(sizeof(*secs), GFP_KERNEL); @@ -504,9 +505,19 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, goto out; } + pcmd = shmem_file_setup("dev/sgx", + ((secs->size >> PAGE_SHIFT) + 1) * 128, + VM_NORESERVE); + if (IS_ERR(pcmd)) { + fput(backing); + ret = PTR_ERR(pcmd); + goto out; + } + encl = kzalloc(sizeof(*encl), GFP_KERNEL); if (!encl) { fput(backing); + fput(pcmd); ret = -ENOMEM; goto out; } @@ -525,6 +536,7 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, encl->base = secs->base; encl->size = secs->size; encl->backing = backing; + encl->pcmd = pcmd; secs_epc = sgx_alloc_page(encl->tgid_ctx, 0); if (IS_ERR(secs_epc)) { diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c index d073057..56000fb 100644 --- a/drivers/platform/x86/intel_sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx_page_cache.c @@ -237,10 +237,14 @@ static int __sgx_ewb(struct sgx_encl *encl, { struct sgx_page_info pginfo; struct page *backing; + struct page *pcmd; + unsigned long pcmd_offset; void *epc; void *va; int ret; + pcmd_offset = ((encl_page->addr >> PAGE_SHIFT) & 31) * 128; + backing = sgx_get_backing(encl, encl_page); if (IS_ERR(backing)) { ret = PTR_ERR(backing); @@ -249,21 +253,32 @@ static int __sgx_ewb(struct sgx_encl *encl, return ret; } + pcmd = sgx_get_pcmd(encl, encl_page); + if (IS_ERR(pcmd)) { + ret = PTR_ERR(pcmd); + sgx_warn(encl, "pinning the pcmd page for EWB failed with %d\n", + ret); + goto out; + } + epc = sgx_get_epc_page(encl_page->epc_page); va = sgx_get_epc_page(encl_page->va_page->epc_page); pginfo.srcpge = (unsigned long)kmap_atomic(backing); - pginfo.pcmd = (unsigned long)&encl_page->pcmd; + pginfo.pcmd = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; pginfo.linaddr = 0; pginfo.secs = 0; ret = __ewb(&pginfo, epc, (void *)((unsigned long)va + encl_page->va_offset)); + kunmap_atomic((void *)(unsigned long)(pginfo.pcmd - pcmd_offset)); kunmap_atomic((void *)(unsigned long)pginfo.srcpge); sgx_put_epc_page(va); sgx_put_epc_page(epc); - sgx_put_backing(backing, true); + sgx_put_pcmd(pcmd, true); +out: + sgx_put_backing(backing, true); return ret; } diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c index 2c390c5..40f5839 100644 --- a/drivers/platform/x86/intel_sgx_util.c +++ b/drivers/platform/x86/intel_sgx_util.c @@ -105,6 +105,33 @@ void sgx_put_backing(struct page *backing_page, bool write) put_page(backing_page); } +struct page *sgx_get_pcmd(struct sgx_encl *encl, + struct sgx_encl_page *entry) +{ + struct page *pcmd; + struct inode *inode; + struct address_space *mapping; + gfp_t gfpmask; + pgoff_t index; + + inode = encl->pcmd->f_path.dentry->d_inode; + mapping = inode->i_mapping; + gfpmask = mapping_gfp_mask(mapping); + /* 32 PCMD's per page */ + index = (entry->addr - encl->base) >> (PAGE_SHIFT + 5); + pcmd = shmem_read_mapping_page_gfp(mapping, index, gfpmask); + + return pcmd; +} + +void sgx_put_pcmd(struct page *pcmd_page, bool write) +{ + if (write) + set_page_dirty(pcmd_page); + + put_page(pcmd_page); +} + struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr) { struct vm_area_struct *vma; @@ -245,5 +272,8 @@ void sgx_encl_release(struct kref *ref) if (encl->backing) fput(encl->backing); + if (encl->pcmd) + fput(encl->pcmd); + kfree(encl); } diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index 73ec032..3171348 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -99,12 +99,16 @@ static int sgx_eldu(struct sgx_encl *encl, bool is_secs) { struct page *backing; + struct page *pcmd; + unsigned long pcmd_offset; struct sgx_page_info pginfo; void *secs_ptr = NULL; void *epc_ptr; void *va_ptr; int ret; + pcmd_offset = ((encl_page->addr >> PAGE_SHIFT) & 31) * 128; + backing = sgx_get_backing(encl, encl_page); if (IS_ERR(backing)) { ret = PTR_ERR(backing); @@ -113,22 +117,36 @@ static int sgx_eldu(struct sgx_encl *encl, return ret; } + pcmd = sgx_get_pcmd(encl, encl_page); + if (IS_ERR(pcmd)) { + ret = PTR_ERR(pcmd); + sgx_warn(encl, "pinning the pcmd page for EWB failed with %d\n", + ret); + goto out; + } + if (!is_secs) secs_ptr = sgx_get_epc_page(encl->secs_page.epc_page); - pginfo.secs = (unsigned long)secs_ptr; epc_ptr = sgx_get_epc_page(epc_page); va_ptr = sgx_get_epc_page(encl_page->va_page->epc_page); pginfo.srcpge = (unsigned long)kmap_atomic(backing); + pginfo.srcpge = (unsigned long)kmap_atomic(backing); + pginfo.pcmd = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; pginfo.linaddr = is_secs ? 0 : encl_page->addr; - pginfo.pcmd = (unsigned long)&encl_page->pcmd; + pginfo.secs = (unsigned long)secs_ptr; ret = __eldu((unsigned long)&pginfo, (unsigned long)epc_ptr, (unsigned long)va_ptr + encl_page->va_offset); + if (ret) { + sgx_err(encl, "ELDU returned %d\n", ret); + ret = -EFAULT; + } + kunmap_atomic((void *)(unsigned long)(pginfo.pcmd - pcmd_offset)); kunmap_atomic((void *)(unsigned long)pginfo.srcpge); sgx_put_epc_page(va_ptr); sgx_put_epc_page(epc_ptr); @@ -136,13 +154,10 @@ static int sgx_eldu(struct sgx_encl *encl, if (!is_secs) sgx_put_epc_page(secs_ptr); - sgx_put_backing(backing, false); - - if (ret) { - sgx_err(encl, "ELDU returned %d\n", ret); - return -EFAULT; - } + sgx_put_pcmd(pcmd, true); +out: + sgx_put_backing(backing, 0); return 0; } -- 2.9.3

On Fri, Jan 20, 2017 at 09:53:17PM +0000, Christopherson, Sean J wrote: Sure, that would make sense. Whoops this must have happened when I rebased these patches to the latest master. Thanks for spotting this. With a bit reorganization the kernel memory consumption for each enclave page starts to become acceptable (4 qwords): struct sgx_encl_page { union { unsigned long addr; unsigned int flags; }; union { /* loaded */ struct { struct sgx_epc_page *epc_page; struct list_head load_list; }; /* 64 bytes */ /* swapped */ struct { struct sgx_va_page *va_page; unsigned int va_offset; }; /* 60 bytes */ }; }; For me more important is that SGX takes away minimum amount of system resources from other (non-SGX) tasks than the SGX performance itself. /Jarkko

Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com&gt; --- drivers/platform/x86/intel_sgx_vma.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index c0fb28f..7e53c4a 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -254,20 +254,19 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, secs_epc_page = NULL; } + rc = sgx_eldu(encl, entry, epc_page, false /* is_secs */); if (rc) { entry = ERR_PTR(rc); goto out; } + free_flags = 0; - rc = sgx_eldu(encl, entry, epc_page, false /* is_secs */); + rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); if (rc) { - free_flags = 0; entry = ERR_PTR(rc); goto out; } - rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa)); - encl->secs_child_cnt++; entry->epc_page = epc_page; -- 2.7.4

Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com&gt; --- drivers/platform/x86/intel_sgx_vma.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c index 7e53c4a..46b81da 100644 --- a/drivers/platform/x86/intel_sgx_vma.c +++ b/drivers/platform/x86/intel_sgx_vma.c @@ -148,8 +148,6 @@ static int sgx_eldu(struct sgx_encl *encl, epc_ptr = sgx_get_epc_page(epc_page); va_ptr = sgx_get_epc_page(encl_page->va_page->epc_page); pginfo.srcpge = (unsigned long)kmap_atomic(backing); - - pginfo.srcpge = (unsigned long)kmap_atomic(backing); pginfo.pcmd = (unsigned long)kmap_atomic(pcmd) + pcmd_offset; pginfo.linaddr = is_secs ? 0 : encl_page->addr; pginfo.secs = (unsigned long)secs_ptr; @@ -175,7 +173,7 @@ static int sgx_eldu(struct sgx_encl *encl, out: sgx_put_backing(backing, 0); - return 0; + return ret; } static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma, -- 2.7.4

Combine sgx_get/put_backing/pcmd functionality into common utility functions, wrapped by inline helpers for backing and pcmd. Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com&gt; --- drivers/platform/x86/intel_sgx.h | 31 +++++++++++++++++++----- drivers/platform/x86/intel_sgx_util.c | 45 +++++------------------------------ 2 files changed, 31 insertions(+), 45 deletions(-) diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index 1d03606..22bf3be 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -195,12 +195,8 @@ long sgx_compat_ioctl(struct file *filep, unsigned int cmd, unsigned long arg); int sgx_test_and_clear_young(struct sgx_encl_page *page, struct sgx_encl *encl); void *sgx_get_epc_page(struct sgx_epc_page *entry); void sgx_put_epc_page(void *epc_page_vaddr); -struct page *sgx_get_backing(struct sgx_encl *encl, - struct sgx_encl_page *entry); -void sgx_put_backing(struct page *backing, bool write); -struct page *sgx_get_pcmd(struct sgx_encl *encl, - struct sgx_encl_page *entry); -void sgx_put_pcmd(struct page *pcmd_page, bool write); +struct page *sgx_get_shmem(struct file *shmem_file, pgoff_t index); +void sgx_put_shmem(struct page *shmem_page, bool write); void sgx_insert_pte(struct sgx_encl *encl, struct sgx_encl_page *encl_page, struct sgx_epc_page *epc_page, @@ -217,6 +213,29 @@ int sgx_find_encl(struct mm_struct *mm, unsigned long addr, void sgx_encl_release(struct kref *ref); void sgx_tgid_ctx_release(struct kref *ref); +static inline struct page *sgx_get_backing(struct sgx_encl *encl, + struct sgx_encl_page *entry) +{ + pgoff_t index = (entry->addr - encl->base) >> PAGE_SHIFT; + return sgx_get_shmem(encl->backing, index); +} +static inline void sgx_put_backing(struct page *backing, bool write) +{ + return sgx_put_shmem(backing, write); +} +static inline struct page *sgx_get_pcmd(struct sgx_encl *encl, + struct sgx_encl_page *entry) +{ + /* 32 PCMD's per page */ + pgoff_t index = (entry->addr - encl->base) >> (PAGE_SHIFT + 5); + return sgx_get_shmem(encl->pcmd, index); +} +static inline void sgx_put_pcmd(struct page *pcmd, bool write) +{ + return sgx_put_shmem(pcmd, write); +} + + /* EPC page cache */ #define SGX_NR_LOW_EPC_PAGES_DEFAULT 32 diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c index 40f5839..c7bf5e1 100644 --- a/drivers/platform/x86/intel_sgx_util.c +++ b/drivers/platform/x86/intel_sgx_util.c @@ -78,58 +78,25 @@ void sgx_put_epc_page(void *epc_page_vaddr) #endif } -struct page *sgx_get_backing(struct sgx_encl *encl, - struct sgx_encl_page *entry) +struct page *sgx_get_shmem(struct file *shmem_file, pgoff_t index) { - struct page *backing; struct inode *inode; struct address_space *mapping; gfp_t gfpmask; - pgoff_t index; - inode = encl->backing->f_path.dentry->d_inode; + inode = shmem_file->f_path.dentry->d_inode; mapping = inode->i_mapping; gfpmask = mapping_gfp_mask(mapping); - index = (entry->addr - encl->base) >> PAGE_SHIFT; - backing = shmem_read_mapping_page_gfp(mapping, index, gfpmask); - - return backing; -} - -void sgx_put_backing(struct page *backing_page, bool write) -{ - if (write) - set_page_dirty(backing_page); - - put_page(backing_page); -} - -struct page *sgx_get_pcmd(struct sgx_encl *encl, - struct sgx_encl_page *entry) -{ - struct page *pcmd; - struct inode *inode; - struct address_space *mapping; - gfp_t gfpmask; - pgoff_t index; - - inode = encl->pcmd->f_path.dentry->d_inode; - mapping = inode->i_mapping; - gfpmask = mapping_gfp_mask(mapping); - /* 32 PCMD's per page */ - index = (entry->addr - encl->base) >> (PAGE_SHIFT + 5); - pcmd = shmem_read_mapping_page_gfp(mapping, index, gfpmask); - - return pcmd; + return shmem_read_mapping_page_gfp(mapping, index, gfpmask); } -void sgx_put_pcmd(struct page *pcmd_page, bool write) +void sgx_put_shmem(struct page *shmem_page, bool write) { if (write) - set_page_dirty(pcmd_page); + set_page_dirty(shmem_page); - put_page(pcmd_page); + put_page(shmem_page); } struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr) -- 2.7.4

On Sat, Jan 21, 2017 at 10:22:26PM +0200, Jarkko Sakkinen wrote: will rather have something like enum sgx_backing_flags { SGX_BACKING_PCMD = BIT(0), SGX_BACKING_WRITE = BIT(1), }; struct page *sgx_get_backing(struct sgx_encl *encl, struct sgx_encl_page *entry, unsigned int flags); void sgx_put_backing(struct page *backing_page, unsigned int flags); This decreases the number of functions to one where as the above increases it to three. /Jarkko