| Message ID | 20170404072938.4800-6-kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: > KVM needs to call the two to allocate and free EPC pages. Also removed > 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed > 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless > to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX > app. sgx_get{put}_tgid_ctx are introduced and called separately after > sgx_alloc{free}_page are called to reflect the original logic. Also > introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always > assumes EREMOVE to be not part of sgx_free_page. Probably a better way > is to remove EREMOVE from sgx_free_page and call it explicitly where it > is needed. > > Note that as there's no OOM of EPC in SGX driver, and we don't support > evicting EPC from KVM guests yet, therefore sgx_alloc_page will never > return if there's no enough EPC when creating KVM guest. This even applies > to host sgx app alone as well as there's no OOM for EPC now. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> Why you are packaging all of this into a single commit? Which of these changes are mandatory to continue with KVM? Please split. Your short summary is misleading right now. Essentially a good golde rule for commit scope is that if your detailed changes do not fall in the scope of your short summary you should probably split. PS. For those changes that actually just revert things like NO_EREMOVE could you first of clearly state that it is a revert and why you want it back. I removed it because it added unnecessary complexity with almost no gain. Even if the call is sometimes useless it still returns with success. For v1 driver I rather would not add it back. /Jarkko > --- > arch/x86/include/asm/sgx.h | 12 +++++++++ > drivers/platform/x86/intel_sgx.h | 14 ++-------- > drivers/platform/x86/intel_sgx_ioctl.c | 23 ++++++++++++----- > drivers/platform/x86/intel_sgx_page_cache.c | 36 ++++++++++++++------------ > drivers/platform/x86/intel_sgx_util.c | 40 +++++++++++++++++++++-------- > 5 files changed, 80 insertions(+), 45 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index fe1a2ba..063ff8f 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -372,4 +372,16 @@ unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry); > void *sgx_kmap_epc_page(struct sgx_epc_page *entry); > void sgx_kunmap_epc_page(void *epc_page_vaddr); > > +enum sgx_alloc_flags { > + SGX_ALLOC_ATOMIC = BIT(0), > +}; > + > +enum sgx_free_flags { > + SGX_FREE_RETURN_ERROR = BIT(0), > + SGX_FREE_NO_EREMOVE = BIT(1), > +}; > + > +struct sgx_epc_page *sgx_alloc_page(unsigned int flags); > +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags); > + > #endif /* _ASM_X86_SGX_H */ > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > index b9b81d6..f9fc708 100644 > --- a/drivers/platform/x86/intel_sgx.h > +++ b/drivers/platform/x86/intel_sgx.h > @@ -216,6 +216,8 @@ void sgx_unpin_mm(struct sgx_encl *encl); > void sgx_invalidate(struct sgx_encl *encl); > int sgx_find_encl(struct mm_struct *mm, unsigned long addr, > struct vm_area_struct **vma); > +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx); > +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx); > > enum sgx_fault_flags { > SGX_FAULT_RESERVE = BIT(0), > @@ -237,20 +239,8 @@ extern struct mutex sgx_tgid_ctx_mutex; > extern struct list_head sgx_tgid_ctx_list; > extern struct task_struct *ksgxswapd_tsk; > > -enum sgx_alloc_flags { > - SGX_ALLOC_ATOMIC = BIT(0), > -}; > - > -enum sgx_free_flags { > - SGX_FREE_RETURN_ERROR = BIT(0), > -}; > - > int ksgxswapd(void *p); > int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); > void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); > -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, > - unsigned int flags); > -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > - unsigned int flags); > > #endif /* __ARCH_X86_INTEL_SGX_H__ */ > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c > index a8729c3..5ff647f 100644 > --- a/drivers/platform/x86/intel_sgx_ioctl.c > +++ b/drivers/platform/x86/intel_sgx_ioctl.c > @@ -216,12 +216,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) > struct vm_area_struct *vma; > int ret; > > - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); > + epc_page = sgx_alloc_page(0); > if (IS_ERR(epc_page)) > return false; > > + sgx_get_tgid_ctx(encl->tgid_ctx); > + > if (!sgx_pin_mm(encl)) { > - sgx_free_page(epc_page, encl, 0); > + sgx_free_page(epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > return false; > } > > @@ -275,7 +278,8 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) > sgx_unpin_mm(encl); > return true; > out: > - sgx_free_page(epc_page, encl, 0); > + sgx_free_page(epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > mutex_unlock(&encl->lock); > sgx_unpin_mm(encl); > return false; > @@ -405,17 +409,19 @@ static int sgx_init_page(struct sgx_encl *encl, > if (!va_page) > return -ENOMEM; > > - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); > + epc_page = sgx_alloc_page(0); > if (IS_ERR(epc_page)) { > kfree(va_page); > return PTR_ERR(epc_page); > } > + sgx_get_tgid_ctx(encl->tgid_ctx); > > vaddr = sgx_kmap_epc_page(epc_page); > if (!vaddr) { > sgx_warn(encl, "kmap of a new VA page failed %d\n", > ret); > - sgx_free_page(epc_page, encl, 0); > + sgx_free_page(epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > kfree(va_page); > return -EFAULT; > } > @@ -425,7 +431,8 @@ static int sgx_init_page(struct sgx_encl *encl, > > if (ret) { > sgx_warn(encl, "EPA returned %d\n", ret); > - sgx_free_page(epc_page, encl, 0); > + sgx_free_page(epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > kfree(va_page); > return -EFAULT; > } > @@ -539,12 +546,14 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, > encl->backing = backing; > encl->pcmd = pcmd; > > - secs_epc = sgx_alloc_page(encl->tgid_ctx, 0); > + secs_epc = sgx_alloc_page(0); > if (IS_ERR(secs_epc)) { > ret = PTR_ERR(secs_epc); > secs_epc = NULL; > goto out; > } > + /* don't need to sgx_get_tgid_ctx(encl->tgid_ctx) as encl->tgid_ctx is > + * NULL now */ > > ret = sgx_add_to_tgid_ctx(encl); > if (ret) > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c > index 3f9ba9c..71569e7 100644 > --- a/drivers/platform/x86/intel_sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > @@ -330,7 +330,8 @@ static void sgx_evict_page(struct sgx_encl_page *entry, > struct sgx_encl *encl) > { > sgx_ewb(encl, entry); > - sgx_free_page(entry->epc_page, encl, 0); > + sgx_free_page(entry->epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > entry->epc_page = NULL; > entry->flags &= ~SGX_ENCL_PAGE_RESERVED; > } > @@ -554,18 +555,15 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void) > * > * Return: an EPC page or an error code > */ > -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, > - unsigned int flags) > +struct sgx_epc_page *sgx_alloc_page(unsigned int flags) > { > struct sgx_epc_page *entry; > > for ( ; ; ) { > entry = sgx_alloc_page_fast(); > - if (entry) { > - if (ctx) > - atomic_inc(&ctx->epc_cnt); > + if (entry) > break; > - } else if (flags & SGX_ALLOC_ATOMIC) { > + else if (flags & SGX_ALLOC_ATOMIC) { > entry = ERR_PTR(-EBUSY); > break; > } > @@ -584,6 +582,7 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, > > return entry; > } > +EXPORT_SYMBOL_GPL(sgx_alloc_page); > > /** > * sgx_free_page - free an EPC page > @@ -591,25 +590,29 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, > * @encl: the enclave who owns the EPC page > * @flags: free flags > */ > -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > - unsigned int flags) > +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags) > { > void *epc; > - int ret; > + int ret = 0; > > - epc = sgx_kmap_epc_page(entry); > - ret = __eremove(epc); > - sgx_kunmap_epc_page(epc); > + if (!(flags & SGX_FREE_NO_EREMOVE)) { > + epc = sgx_kmap_epc_page(entry); > + ret = __eremove(epc); > + sgx_kunmap_epc_page(epc); > + } > > if (ret) { > if (flags & SGX_FREE_RETURN_ERROR) > return ret; > > - sgx_crit(encl, "EREMOVE returned %d\n", ret); > + /* > + * sgx_crit is removed because it takes encl as parameter, > + * which sgx_free_page doesn't take anymore. > + * TODO: remove encl from sgx_{print} functions and call > + * sgx_crit again. > + */ > } > > - atomic_dec(&encl->tgid_ctx->epc_cnt); > - > spin_lock(&sgx_free_list_lock); > list_add(&entry->free_list, &sgx_free_list); > sgx_nr_free_pages++; > @@ -617,3 +620,4 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > > return 0; > } > +EXPORT_SYMBOL_GPL(sgx_free_page); > diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c > index c63193d..96b3770 100644 > --- a/drivers/platform/x86/intel_sgx_util.c > +++ b/drivers/platform/x86/intel_sgx_util.c > @@ -372,12 +372,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > goto out; > } > > - epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); > + epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); > if (IS_ERR(epc_page)) { > rc = PTR_ERR(epc_page); > epc_page = NULL; > goto out; > } > + sgx_get_tgid_ctx(encl->tgid_ctx); > > if (encl->flags & SGX_ENCL_DEAD) { > rc = -EFAULT; > @@ -406,12 +407,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > > /* If SECS is evicted then reload it first */ > if (encl->flags & SGX_ENCL_SECS_EVICTED) { > - secs_epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); > + secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); > if (IS_ERR(secs_epc_page)) { > rc = PTR_ERR(secs_epc_page); > secs_epc_page = NULL; > goto out; > } > + sgx_get_tgid_ctx(encl->tgid_ctx); > > rc = sgx_eldu(encl, &encl->secs_page, secs_epc_page, true); > if (rc) > @@ -446,10 +448,14 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, > list_add_tail(&entry->load_list, &encl->load_list); > out: > mutex_unlock(&encl->lock); > - if (epc_page) > - sgx_free_page(epc_page, encl, 0); > - if (secs_epc_page) > - sgx_free_page(secs_epc_page, encl, 0); > + if (epc_page) { > + sgx_free_page(epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > + } > + if (secs_epc_page) { > + sgx_free_page(secs_epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > + } > return rc ? ERR_PTR(rc) : entry; > } > > @@ -489,7 +495,8 @@ void sgx_encl_release(struct kref *ref) > entry = *slot; > if (entry->epc_page) { > list_del(&entry->load_list); > - sgx_free_page(entry->epc_page, encl, 0); > + sgx_free_page(entry->epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > } > radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT); > kfree(entry); > @@ -499,12 +506,15 @@ 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_free_page(va_page->epc_page, encl, 0); > + sgx_free_page(va_page->epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > kfree(va_page); > } > > - if (encl->secs_page.epc_page) > - sgx_free_page(encl->secs_page.epc_page, encl, 0); > + if (encl->secs_page.epc_page) { > + sgx_free_page(encl->secs_page.epc_page, 0); > + sgx_put_tgid_ctx(encl->tgid_ctx); > + } > > encl->secs_page.epc_page = NULL; > > @@ -519,3 +529,13 @@ void sgx_encl_release(struct kref *ref) > > kfree(encl); > } > + > +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx) > +{ > + atomic_inc(&ctx->epc_cnt); > +} > + > +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx) > +{ > + atomic_dec(&ctx->epc_cnt); > +} > -- > 2.9.3 >
On 4/5/2017 2:48 AM, Jarkko Sakkinen wrote: > On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: >> KVM needs to call the two to allocate and free EPC pages. Also removed >> 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed >> 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless >> to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX >> app. sgx_get{put}_tgid_ctx are introduced and called separately after >> sgx_alloc{free}_page are called to reflect the original logic. Also >> introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always >> assumes EREMOVE to be not part of sgx_free_page. Probably a better way >> is to remove EREMOVE from sgx_free_page and call it explicitly where it >> is needed. >> >> Note that as there's no OOM of EPC in SGX driver, and we don't support >> evicting EPC from KVM guests yet, therefore sgx_alloc_page will never >> return if there's no enough EPC when creating KVM guest. This even applies >> to host sgx app alone as well as there's no OOM for EPC now. >> >> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > Why you are packaging all of this into a single commit? Which of these > changes are mandatory to continue with KVM? > > Please split. Your short summary is misleading right now. Essentially > a good golde rule for commit scope is that if your detailed changes > do not fall in the scope of your short summary you should probably > split. Besides adding SGX_FREE_NO_EREMOVE can be split I don't see why others can be split. Probably I can split this patch into: - remove sgx_tgid_ctx * ctx from sgx_alloc_page, sgx_encl *encl from sgx_free_page, and introduce 'sgx_get{put}_tgid_ctx. The place where sgx_alloc{free}_page are called are changed accordingly of course. - expose sgx_alloc_page and sgx_free_page for KVM. I will drop adding SGX_FREE_NO_EREMOVE back. No problem. I just think probably it's better to move EREMOVE out from sgx_free_page at all, but it's your call. Let me know if you are happy with the split above. Btw if you really don't like changing sgx_alloc{free}_page interfaces, KVM can still call them by passing NULL for sgx_tgid_ctx *ctx and sgx_encl *encl, but they are just meaningless for KVM. People may ask, ex, why you always pass NULL for sgx_alloc{free}_page and what does that mean? > > PS. For those changes that actually just revert things like NO_EREMOVE > could you first of clearly state that it is a revert and why you want > it back. > > I removed it because it added unnecessary complexity with almost no gain. > Even if the call is sometimes useless it still returns with success. For > v1 driver I rather would not add it back. I have no idea of why it was there at the beginning and why it was reverted. I have no problem keeping it in current way. I just have to change KVM code to suit with it. Thanks, -Kai > > /Jarkko > >> --- >> arch/x86/include/asm/sgx.h | 12 +++++++++ >> drivers/platform/x86/intel_sgx.h | 14 ++-------- >> drivers/platform/x86/intel_sgx_ioctl.c | 23 ++++++++++++----- >> drivers/platform/x86/intel_sgx_page_cache.c | 36 ++++++++++++++------------ >> drivers/platform/x86/intel_sgx_util.c | 40 +++++++++++++++++++++-------- >> 5 files changed, 80 insertions(+), 45 deletions(-) >> >> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h >> index fe1a2ba..063ff8f 100644 >> --- a/arch/x86/include/asm/sgx.h >> +++ b/arch/x86/include/asm/sgx.h >> @@ -372,4 +372,16 @@ unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry); >> void *sgx_kmap_epc_page(struct sgx_epc_page *entry); >> void sgx_kunmap_epc_page(void *epc_page_vaddr); >> >> +enum sgx_alloc_flags { >> + SGX_ALLOC_ATOMIC = BIT(0), >> +}; >> + >> +enum sgx_free_flags { >> + SGX_FREE_RETURN_ERROR = BIT(0), >> + SGX_FREE_NO_EREMOVE = BIT(1), >> +}; >> + >> +struct sgx_epc_page *sgx_alloc_page(unsigned int flags); >> +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags); >> + >> #endif /* _ASM_X86_SGX_H */ >> diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h >> index b9b81d6..f9fc708 100644 >> --- a/drivers/platform/x86/intel_sgx.h >> +++ b/drivers/platform/x86/intel_sgx.h >> @@ -216,6 +216,8 @@ void sgx_unpin_mm(struct sgx_encl *encl); >> void sgx_invalidate(struct sgx_encl *encl); >> int sgx_find_encl(struct mm_struct *mm, unsigned long addr, >> struct vm_area_struct **vma); >> +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx); >> +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx); >> >> enum sgx_fault_flags { >> SGX_FAULT_RESERVE = BIT(0), >> @@ -237,20 +239,8 @@ extern struct mutex sgx_tgid_ctx_mutex; >> extern struct list_head sgx_tgid_ctx_list; >> extern struct task_struct *ksgxswapd_tsk; >> >> -enum sgx_alloc_flags { >> - SGX_ALLOC_ATOMIC = BIT(0), >> -}; >> - >> -enum sgx_free_flags { >> - SGX_FREE_RETURN_ERROR = BIT(0), >> -}; >> - >> int ksgxswapd(void *p); >> int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); >> void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); >> -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, >> - unsigned int flags); >> -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, >> - unsigned int flags); >> >> #endif /* __ARCH_X86_INTEL_SGX_H__ */ >> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c >> index a8729c3..5ff647f 100644 >> --- a/drivers/platform/x86/intel_sgx_ioctl.c >> +++ b/drivers/platform/x86/intel_sgx_ioctl.c >> @@ -216,12 +216,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) >> struct vm_area_struct *vma; >> int ret; >> >> - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); >> + epc_page = sgx_alloc_page(0); >> if (IS_ERR(epc_page)) >> return false; >> >> + sgx_get_tgid_ctx(encl->tgid_ctx); >> + >> if (!sgx_pin_mm(encl)) { >> - sgx_free_page(epc_page, encl, 0); >> + sgx_free_page(epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> return false; >> } >> >> @@ -275,7 +278,8 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) >> sgx_unpin_mm(encl); >> return true; >> out: >> - sgx_free_page(epc_page, encl, 0); >> + sgx_free_page(epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> mutex_unlock(&encl->lock); >> sgx_unpin_mm(encl); >> return false; >> @@ -405,17 +409,19 @@ static int sgx_init_page(struct sgx_encl *encl, >> if (!va_page) >> return -ENOMEM; >> >> - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); >> + epc_page = sgx_alloc_page(0); >> if (IS_ERR(epc_page)) { >> kfree(va_page); >> return PTR_ERR(epc_page); >> } >> + sgx_get_tgid_ctx(encl->tgid_ctx); >> >> vaddr = sgx_kmap_epc_page(epc_page); >> if (!vaddr) { >> sgx_warn(encl, "kmap of a new VA page failed %d\n", >> ret); >> - sgx_free_page(epc_page, encl, 0); >> + sgx_free_page(epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> kfree(va_page); >> return -EFAULT; >> } >> @@ -425,7 +431,8 @@ static int sgx_init_page(struct sgx_encl *encl, >> >> if (ret) { >> sgx_warn(encl, "EPA returned %d\n", ret); >> - sgx_free_page(epc_page, encl, 0); >> + sgx_free_page(epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> kfree(va_page); >> return -EFAULT; >> } >> @@ -539,12 +546,14 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, >> encl->backing = backing; >> encl->pcmd = pcmd; >> >> - secs_epc = sgx_alloc_page(encl->tgid_ctx, 0); >> + secs_epc = sgx_alloc_page(0); >> if (IS_ERR(secs_epc)) { >> ret = PTR_ERR(secs_epc); >> secs_epc = NULL; >> goto out; >> } >> + /* don't need to sgx_get_tgid_ctx(encl->tgid_ctx) as encl->tgid_ctx is >> + * NULL now */ >> >> ret = sgx_add_to_tgid_ctx(encl); >> if (ret) >> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c >> index 3f9ba9c..71569e7 100644 >> --- a/drivers/platform/x86/intel_sgx_page_cache.c >> +++ b/drivers/platform/x86/intel_sgx_page_cache.c >> @@ -330,7 +330,8 @@ static void sgx_evict_page(struct sgx_encl_page *entry, >> struct sgx_encl *encl) >> { >> sgx_ewb(encl, entry); >> - sgx_free_page(entry->epc_page, encl, 0); >> + sgx_free_page(entry->epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> entry->epc_page = NULL; >> entry->flags &= ~SGX_ENCL_PAGE_RESERVED; >> } >> @@ -554,18 +555,15 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void) >> * >> * Return: an EPC page or an error code >> */ >> -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, >> - unsigned int flags) >> +struct sgx_epc_page *sgx_alloc_page(unsigned int flags) >> { >> struct sgx_epc_page *entry; >> >> for ( ; ; ) { >> entry = sgx_alloc_page_fast(); >> - if (entry) { >> - if (ctx) >> - atomic_inc(&ctx->epc_cnt); >> + if (entry) >> break; >> - } else if (flags & SGX_ALLOC_ATOMIC) { >> + else if (flags & SGX_ALLOC_ATOMIC) { >> entry = ERR_PTR(-EBUSY); >> break; >> } >> @@ -584,6 +582,7 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, >> >> return entry; >> } >> +EXPORT_SYMBOL_GPL(sgx_alloc_page); >> >> /** >> * sgx_free_page - free an EPC page >> @@ -591,25 +590,29 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, >> * @encl: the enclave who owns the EPC page >> * @flags: free flags >> */ >> -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, >> - unsigned int flags) >> +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags) >> { >> void *epc; >> - int ret; >> + int ret = 0; >> >> - epc = sgx_kmap_epc_page(entry); >> - ret = __eremove(epc); >> - sgx_kunmap_epc_page(epc); >> + if (!(flags & SGX_FREE_NO_EREMOVE)) { >> + epc = sgx_kmap_epc_page(entry); >> + ret = __eremove(epc); >> + sgx_kunmap_epc_page(epc); >> + } >> >> if (ret) { >> if (flags & SGX_FREE_RETURN_ERROR) >> return ret; >> >> - sgx_crit(encl, "EREMOVE returned %d\n", ret); >> + /* >> + * sgx_crit is removed because it takes encl as parameter, >> + * which sgx_free_page doesn't take anymore. >> + * TODO: remove encl from sgx_{print} functions and call >> + * sgx_crit again. >> + */ >> } >> >> - atomic_dec(&encl->tgid_ctx->epc_cnt); >> - >> spin_lock(&sgx_free_list_lock); >> list_add(&entry->free_list, &sgx_free_list); >> sgx_nr_free_pages++; >> @@ -617,3 +620,4 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, >> >> return 0; >> } >> +EXPORT_SYMBOL_GPL(sgx_free_page); >> diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c >> index c63193d..96b3770 100644 >> --- a/drivers/platform/x86/intel_sgx_util.c >> +++ b/drivers/platform/x86/intel_sgx_util.c >> @@ -372,12 +372,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, >> goto out; >> } >> >> - epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); >> + epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); >> if (IS_ERR(epc_page)) { >> rc = PTR_ERR(epc_page); >> epc_page = NULL; >> goto out; >> } >> + sgx_get_tgid_ctx(encl->tgid_ctx); >> >> if (encl->flags & SGX_ENCL_DEAD) { >> rc = -EFAULT; >> @@ -406,12 +407,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, >> >> /* If SECS is evicted then reload it first */ >> if (encl->flags & SGX_ENCL_SECS_EVICTED) { >> - secs_epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); >> + secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); >> if (IS_ERR(secs_epc_page)) { >> rc = PTR_ERR(secs_epc_page); >> secs_epc_page = NULL; >> goto out; >> } >> + sgx_get_tgid_ctx(encl->tgid_ctx); >> >> rc = sgx_eldu(encl, &encl->secs_page, secs_epc_page, true); >> if (rc) >> @@ -446,10 +448,14 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, >> list_add_tail(&entry->load_list, &encl->load_list); >> out: >> mutex_unlock(&encl->lock); >> - if (epc_page) >> - sgx_free_page(epc_page, encl, 0); >> - if (secs_epc_page) >> - sgx_free_page(secs_epc_page, encl, 0); >> + if (epc_page) { >> + sgx_free_page(epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> + } >> + if (secs_epc_page) { >> + sgx_free_page(secs_epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> + } >> return rc ? ERR_PTR(rc) : entry; >> } >> >> @@ -489,7 +495,8 @@ void sgx_encl_release(struct kref *ref) >> entry = *slot; >> if (entry->epc_page) { >> list_del(&entry->load_list); >> - sgx_free_page(entry->epc_page, encl, 0); >> + sgx_free_page(entry->epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> } >> radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT); >> kfree(entry); >> @@ -499,12 +506,15 @@ 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_free_page(va_page->epc_page, encl, 0); >> + sgx_free_page(va_page->epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> kfree(va_page); >> } >> >> - if (encl->secs_page.epc_page) >> - sgx_free_page(encl->secs_page.epc_page, encl, 0); >> + if (encl->secs_page.epc_page) { >> + sgx_free_page(encl->secs_page.epc_page, 0); >> + sgx_put_tgid_ctx(encl->tgid_ctx); >> + } >> >> encl->secs_page.epc_page = NULL; >> >> @@ -519,3 +529,13 @@ void sgx_encl_release(struct kref *ref) >> >> kfree(encl); >> } >> + >> +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx) >> +{ >> + atomic_inc(&ctx->epc_cnt); >> +} >> + >> +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx) >> +{ >> + atomic_dec(&ctx->epc_cnt); >> +} >> -- >> 2.9.3 >> > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev >
On Wed, Apr 05, 2017 at 12:37:22PM +1200, Huang, Kai wrote: > > > On 4/5/2017 2:48 AM, Jarkko Sakkinen wrote: > > On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: > > > KVM needs to call the two to allocate and free EPC pages. Also removed > > > 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed > > > 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless > > > to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX > > > app. sgx_get{put}_tgid_ctx are introduced and called separately after > > > sgx_alloc{free}_page are called to reflect the original logic. Also > > > introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always > > > assumes EREMOVE to be not part of sgx_free_page. Probably a better way > > > is to remove EREMOVE from sgx_free_page and call it explicitly where it > > > is needed. > > > > > > Note that as there's no OOM of EPC in SGX driver, and we don't support > > > evicting EPC from KVM guests yet, therefore sgx_alloc_page will never > > > return if there's no enough EPC when creating KVM guest. This even applies > > > to host sgx app alone as well as there's no OOM for EPC now. > > > > > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > > > Why you are packaging all of this into a single commit? Which of these > > changes are mandatory to continue with KVM? > > > > Please split. Your short summary is misleading right now. Essentially > > a good golde rule for commit scope is that if your detailed changes > > do not fall in the scope of your short summary you should probably > > split. > > Besides adding SGX_FREE_NO_EREMOVE can be split I don't see why others can > be split. Probably I can split this patch into: > > - remove sgx_tgid_ctx * ctx from sgx_alloc_page, sgx_encl *encl from > sgx_free_page, and introduce 'sgx_get{put}_tgid_ctx. The place where > sgx_alloc{free}_page are called are changed accordingly of course. > > > > - expose sgx_alloc_page and sgx_free_page for KVM. > > I will drop adding SGX_FREE_NO_EREMOVE back. No problem. I just think > probably it's better to move EREMOVE out from sgx_free_page at all, but it's > your call. > > Let me know if you are happy with the split above. > > Btw if you really don't like changing sgx_alloc{free}_page interfaces, KVM > can still call them by passing NULL for sgx_tgid_ctx *ctx and sgx_encl > *encl, but they are just meaningless for KVM. People may ask, ex, why you > always pass NULL for sgx_alloc{free}_page and what does that mean? > > > > > PS. For those changes that actually just revert things like NO_EREMOVE > > could you first of clearly state that it is a revert and why you want > > it back. > > > > I removed it because it added unnecessary complexity with almost no gain. > > Even if the call is sometimes useless it still returns with success. For > > v1 driver I rather would not add it back. > > I have no idea of why it was there at the beginning and why it was reverted. > I have no problem keeping it in current way. I just have to change KVM code > to suit with it. > > Thanks, > -Kai Beginning it was there because well this has been an internal driver for a long time so there just exist some unnecessary cruft :-) Generally I think you should rethink this whole patch set. What I would propose to you is to create patches to improve multibank EPC. We could queue that after v1 driver has been upstreamed. It's real well scoped functionality. What do you think? That way you could get some of your code rather sooner than later to upstream. /Jarkko
On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: > KVM needs to call the two to allocate and free EPC pages. Also removed > 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed > 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless > to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX > app. sgx_get{put}_tgid_ctx are introduced and called separately after > sgx_alloc{free}_page are called to reflect the original logic. Also > introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always > assumes EREMOVE to be not part of sgx_free_page. Probably a better way > is to remove EREMOVE from sgx_free_page and call it explicitly where it > is needed. > > Note that as there's no OOM of EPC in SGX driver, and we don't support > evicting EPC from KVM guests yet, therefore sgx_alloc_page will never > return if there's no enough EPC when creating KVM guest. This even applies > to host sgx app alone as well as there's no OOM for EPC now. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> I'll give a detailed review to make my point better. > --- > arch/x86/include/asm/sgx.h | 12 +++++++++ > drivers/platform/x86/intel_sgx.h | 14 ++-------- > drivers/platform/x86/intel_sgx_ioctl.c | 23 ++++++++++++----- > drivers/platform/x86/intel_sgx_page_cache.c | 36 ++++++++++++++------------ > drivers/platform/x86/intel_sgx_util.c | 40 +++++++++++++++++++++-------- > 5 files changed, 80 insertions(+), 45 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index fe1a2ba..063ff8f 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -372,4 +372,16 @@ unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry); > void *sgx_kmap_epc_page(struct sgx_epc_page *entry); > void sgx_kunmap_epc_page(void *epc_page_vaddr); > > +enum sgx_alloc_flags { > + SGX_ALLOC_ATOMIC = BIT(0), > +}; > + > +enum sgx_free_flags { > + SGX_FREE_RETURN_ERROR = BIT(0), > + SGX_FREE_NO_EREMOVE = BIT(1), > +}; > + > +struct sgx_epc_page *sgx_alloc_page(unsigned int flags); > +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags); > + > #endif /* _ASM_X86_SGX_H */ > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > index b9b81d6..f9fc708 100644 > --- a/drivers/platform/x86/intel_sgx.h > +++ b/drivers/platform/x86/intel_sgx.h > @@ -216,6 +216,8 @@ void sgx_unpin_mm(struct sgx_encl *encl); > void sgx_invalidate(struct sgx_encl *encl); > int sgx_find_encl(struct mm_struct *mm, unsigned long addr, > struct vm_area_struct **vma); > +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx); > +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx); > > enum sgx_fault_flags { > SGX_FAULT_RESERVE = BIT(0), > @@ -237,20 +239,8 @@ extern struct mutex sgx_tgid_ctx_mutex; > extern struct list_head sgx_tgid_ctx_list; > extern struct task_struct *ksgxswapd_tsk; > > -enum sgx_alloc_flags { > - SGX_ALLOC_ATOMIC = BIT(0), > -}; > - > -enum sgx_free_flags { > - SGX_FREE_RETURN_ERROR = BIT(0), > -}; > - > int ksgxswapd(void *p); > int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); > void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); > -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, > - unsigned int flags); > -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > - unsigned int flags); > > #endif /* __ARCH_X86_INTEL_SGX_H__ */ > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c > index a8729c3..5ff647f 100644 > --- a/drivers/platform/x86/intel_sgx_ioctl.c > +++ b/drivers/platform/x86/intel_sgx_ioctl.c > @@ -216,12 +216,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) > struct vm_area_struct *vma; > int ret; > > - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); > + epc_page = sgx_alloc_page(0); The change of parameters and export should be in different commits. I cannot give reviewed-by to both at the same time. They require separate consideration. But already at this point I'll say that I won't accept patch removing tgid_ctx parameter. Just doing the export would have been fine. /Jarkko
On 4/5/2017 7:52 PM, Jarkko Sakkinen wrote: > On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: >> KVM needs to call the two to allocate and free EPC pages. Also removed >> 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed >> 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless >> to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX >> app. sgx_get{put}_tgid_ctx are introduced and called separately after >> sgx_alloc{free}_page are called to reflect the original logic. Also >> introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always >> assumes EREMOVE to be not part of sgx_free_page. Probably a better way >> is to remove EREMOVE from sgx_free_page and call it explicitly where it >> is needed. >> >> Note that as there's no OOM of EPC in SGX driver, and we don't support >> evicting EPC from KVM guests yet, therefore sgx_alloc_page will never >> return if there's no enough EPC when creating KVM guest. This even applies >> to host sgx app alone as well as there's no OOM for EPC now. >> >> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > I'll give a detailed review to make my point better. Thank you. Thanks, -Kai > >> --- >> arch/x86/include/asm/sgx.h | 12 +++++++++ >> drivers/platform/x86/intel_sgx.h | 14 ++-------- >> drivers/platform/x86/intel_sgx_ioctl.c | 23 ++++++++++++----- >> drivers/platform/x86/intel_sgx_page_cache.c | 36 ++++++++++++++------------ >> drivers/platform/x86/intel_sgx_util.c | 40 +++++++++++++++++++++-------- >> 5 files changed, 80 insertions(+), 45 deletions(-) >> >> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h >> index fe1a2ba..063ff8f 100644 >> --- a/arch/x86/include/asm/sgx.h >> +++ b/arch/x86/include/asm/sgx.h >> @@ -372,4 +372,16 @@ unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry); >> void *sgx_kmap_epc_page(struct sgx_epc_page *entry); >> void sgx_kunmap_epc_page(void *epc_page_vaddr); >> >> +enum sgx_alloc_flags { >> + SGX_ALLOC_ATOMIC = BIT(0), >> +}; >> + >> +enum sgx_free_flags { >> + SGX_FREE_RETURN_ERROR = BIT(0), >> + SGX_FREE_NO_EREMOVE = BIT(1), >> +}; >> + >> +struct sgx_epc_page *sgx_alloc_page(unsigned int flags); >> +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags); >> + >> #endif /* _ASM_X86_SGX_H */ >> diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h >> index b9b81d6..f9fc708 100644 >> --- a/drivers/platform/x86/intel_sgx.h >> +++ b/drivers/platform/x86/intel_sgx.h >> @@ -216,6 +216,8 @@ void sgx_unpin_mm(struct sgx_encl *encl); >> void sgx_invalidate(struct sgx_encl *encl); >> int sgx_find_encl(struct mm_struct *mm, unsigned long addr, >> struct vm_area_struct **vma); >> +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx); >> +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx); >> >> enum sgx_fault_flags { >> SGX_FAULT_RESERVE = BIT(0), >> @@ -237,20 +239,8 @@ extern struct mutex sgx_tgid_ctx_mutex; >> extern struct list_head sgx_tgid_ctx_list; >> extern struct task_struct *ksgxswapd_tsk; >> >> -enum sgx_alloc_flags { >> - SGX_ALLOC_ATOMIC = BIT(0), >> -}; >> - >> -enum sgx_free_flags { >> - SGX_FREE_RETURN_ERROR = BIT(0), >> -}; >> - >> int ksgxswapd(void *p); >> int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); >> void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); >> -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, >> - unsigned int flags); >> -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, >> - unsigned int flags); >> >> #endif /* __ARCH_X86_INTEL_SGX_H__ */ >> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c >> index a8729c3..5ff647f 100644 >> --- a/drivers/platform/x86/intel_sgx_ioctl.c >> +++ b/drivers/platform/x86/intel_sgx_ioctl.c >> @@ -216,12 +216,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) >> struct vm_area_struct *vma; >> int ret; >> >> - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); >> + epc_page = sgx_alloc_page(0); > > The change of parameters and export should be in different commits. I > cannot give reviewed-by to both at the same time. They require separate > consideration. > > But already at this point I'll say that I won't accept patch removing > tgid_ctx parameter. Just doing the export would have been fine. > > /Jarkko > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev >
On 4/5/2017 7:42 PM, Jarkko Sakkinen wrote: > On Wed, Apr 05, 2017 at 12:37:22PM +1200, Huang, Kai wrote: >> >> >> On 4/5/2017 2:48 AM, Jarkko Sakkinen wrote: >>> On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: >>>> KVM needs to call the two to allocate and free EPC pages. Also removed >>>> 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed >>>> 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless >>>> to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX >>>> app. sgx_get{put}_tgid_ctx are introduced and called separately after >>>> sgx_alloc{free}_page are called to reflect the original logic. Also >>>> introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always >>>> assumes EREMOVE to be not part of sgx_free_page. Probably a better way >>>> is to remove EREMOVE from sgx_free_page and call it explicitly where it >>>> is needed. >>>> >>>> Note that as there's no OOM of EPC in SGX driver, and we don't support >>>> evicting EPC from KVM guests yet, therefore sgx_alloc_page will never >>>> return if there's no enough EPC when creating KVM guest. This even applies >>>> to host sgx app alone as well as there's no OOM for EPC now. >>>> >>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >>> >>> Why you are packaging all of this into a single commit? Which of these >>> changes are mandatory to continue with KVM? >>> >>> Please split. Your short summary is misleading right now. Essentially >>> a good golde rule for commit scope is that if your detailed changes >>> do not fall in the scope of your short summary you should probably >>> split. >> >> Besides adding SGX_FREE_NO_EREMOVE can be split I don't see why others can >> be split. Probably I can split this patch into: >> >> - remove sgx_tgid_ctx * ctx from sgx_alloc_page, sgx_encl *encl from >> sgx_free_page, and introduce 'sgx_get{put}_tgid_ctx. The place where >> sgx_alloc{free}_page are called are changed accordingly of course. >> >> >> >> - expose sgx_alloc_page and sgx_free_page for KVM. >> >> I will drop adding SGX_FREE_NO_EREMOVE back. No problem. I just think >> probably it's better to move EREMOVE out from sgx_free_page at all, but it's >> your call. >> >> Let me know if you are happy with the split above. >> >> Btw if you really don't like changing sgx_alloc{free}_page interfaces, KVM >> can still call them by passing NULL for sgx_tgid_ctx *ctx and sgx_encl >> *encl, but they are just meaningless for KVM. People may ask, ex, why you >> always pass NULL for sgx_alloc{free}_page and what does that mean? >> >>> >>> PS. For those changes that actually just revert things like NO_EREMOVE >>> could you first of clearly state that it is a revert and why you want >>> it back. >>> >>> I removed it because it added unnecessary complexity with almost no gain. >>> Even if the call is sometimes useless it still returns with success. For >>> v1 driver I rather would not add it back. >> >> I have no idea of why it was there at the beginning and why it was reverted. >> I have no problem keeping it in current way. I just have to change KVM code >> to suit with it. >> >> Thanks, >> -Kai > > Beginning it was there because well this has been an internal driver for > a long time so there just exist some unnecessary cruft :-) > > Generally I think you should rethink this whole patch set. Sure. > > What I would propose to you is to create patches to improve multibank > EPC. We could queue that after v1 driver has been upstreamed. It's real > well scoped functionality. What do you think? Moving kthread creation out is simple and I think if you would handle it I'd rather leave it to you. Do you mean queue the patch to improve multibank EPC or queue patches to support 'unified EPC' for KVM? I thought in our last meeting we have agreed that the patches for support 'unified EPC' would be merged to your driver before you upstream it (only after you review and are happy with the patches, of course). I understand you don't want to make big changes before upstream but I think if the patches are just about to expose necessary EPC management functions (will send them out later) I think there won't be big risk. I personally will test on KVM side, but as I said I couldn't test on host side due to testsuite (although I am happy to) so if you can do some sanity test on your side I think there won't be big risk. I can send you how to test on KVM side as well if you are interested :) Thanks, -Kai > > That way you could get some of your code rather sooner than later to > upstream. > > /Jarkko >
On Thu, Apr 06, 2017 at 08:51:24PM +1200, Huang, Kai wrote: > > > On 4/5/2017 7:42 PM, Jarkko Sakkinen wrote: > > On Wed, Apr 05, 2017 at 12:37:22PM +1200, Huang, Kai wrote: > > > > > > > > > On 4/5/2017 2:48 AM, Jarkko Sakkinen wrote: > > > > On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: > > > > > KVM needs to call the two to allocate and free EPC pages. Also removed > > > > > 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed > > > > > 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless > > > > > to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX > > > > > app. sgx_get{put}_tgid_ctx are introduced and called separately after > > > > > sgx_alloc{free}_page are called to reflect the original logic. Also > > > > > introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always > > > > > assumes EREMOVE to be not part of sgx_free_page. Probably a better way > > > > > is to remove EREMOVE from sgx_free_page and call it explicitly where it > > > > > is needed. > > > > > > > > > > Note that as there's no OOM of EPC in SGX driver, and we don't support > > > > > evicting EPC from KVM guests yet, therefore sgx_alloc_page will never > > > > > return if there's no enough EPC when creating KVM guest. This even applies > > > > > to host sgx app alone as well as there's no OOM for EPC now. > > > > > > > > > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > > > > > > > Why you are packaging all of this into a single commit? Which of these > > > > changes are mandatory to continue with KVM? > > > > > > > > Please split. Your short summary is misleading right now. Essentially > > > > a good golde rule for commit scope is that if your detailed changes > > > > do not fall in the scope of your short summary you should probably > > > > split. > > > > > > Besides adding SGX_FREE_NO_EREMOVE can be split I don't see why others can > > > be split. Probably I can split this patch into: > > > > > > - remove sgx_tgid_ctx * ctx from sgx_alloc_page, sgx_encl *encl from > > > sgx_free_page, and introduce 'sgx_get{put}_tgid_ctx. The place where > > > sgx_alloc{free}_page are called are changed accordingly of course. > > > > > > > > > > > > - expose sgx_alloc_page and sgx_free_page for KVM. > > > > > > I will drop adding SGX_FREE_NO_EREMOVE back. No problem. I just think > > > probably it's better to move EREMOVE out from sgx_free_page at all, but it's > > > your call. > > > > > > Let me know if you are happy with the split above. > > > > > > Btw if you really don't like changing sgx_alloc{free}_page interfaces, KVM > > > can still call them by passing NULL for sgx_tgid_ctx *ctx and sgx_encl > > > *encl, but they are just meaningless for KVM. People may ask, ex, why you > > > always pass NULL for sgx_alloc{free}_page and what does that mean? > > > > > > > > > > > PS. For those changes that actually just revert things like NO_EREMOVE > > > > could you first of clearly state that it is a revert and why you want > > > > it back. > > > > > > > > I removed it because it added unnecessary complexity with almost no gain. > > > > Even if the call is sometimes useless it still returns with success. For > > > > v1 driver I rather would not add it back. > > > > > > I have no idea of why it was there at the beginning and why it was reverted. > > > I have no problem keeping it in current way. I just have to change KVM code > > > to suit with it. > > > > > > Thanks, > > > -Kai > > > > Beginning it was there because well this has been an internal driver for > > a long time so there just exist some unnecessary cruft :-) > > > > Generally I think you should rethink this whole patch set. > > Sure. > > > > > What I would propose to you is to create patches to improve multibank > > EPC. We could queue that after v1 driver has been upstreamed. It's real > > well scoped functionality. What do you think? > > Moving kthread creation out is simple and I think if you would handle it I'd > rather leave it to you. Do you mean queue the patch to improve multibank EPC > or queue patches to support 'unified EPC' for KVM? I thought in our last > meeting we have agreed that the patches for support 'unified EPC' would be > merged to your driver before you upstream it (only after you review and are > happy with the patches, of course). OK > > I understand you don't want to make big changes before upstream but I think > if the patches are just about to expose necessary EPC management functions > (will send them out later) I think there won't be big risk. I personally > will test on KVM side, but as I said I couldn't test on host side due to > testsuite (although I am happy to) so if you can do some sanity test on your > side I think there won't be big risk. I can send you how to test on KVM side > as well if you are interested :) Lets walk this with baby steps. If I first do the export and you give me feedback when you have kind of "lowest common denominator" interface to continue your work? > Thanks, > -Kai /Jarkko
On 4/7/2017 6:44 AM, Jarkko Sakkinen wrote: > On Thu, Apr 06, 2017 at 08:51:24PM +1200, Huang, Kai wrote: >> >> >> On 4/5/2017 7:42 PM, Jarkko Sakkinen wrote: >>> On Wed, Apr 05, 2017 at 12:37:22PM +1200, Huang, Kai wrote: >>>> >>>> >>>> On 4/5/2017 2:48 AM, Jarkko Sakkinen wrote: >>>>> On Tue, Apr 04, 2017 at 07:29:36PM +1200, Kai Huang wrote: >>>>>> KVM needs to call the two to allocate and free EPC pages. Also removed >>>>>> 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed >>>>>> 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless >>>>>> to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX >>>>>> app. sgx_get{put}_tgid_ctx are introduced and called separately after >>>>>> sgx_alloc{free}_page are called to reflect the original logic. Also >>>>>> introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always >>>>>> assumes EREMOVE to be not part of sgx_free_page. Probably a better way >>>>>> is to remove EREMOVE from sgx_free_page and call it explicitly where it >>>>>> is needed. >>>>>> >>>>>> Note that as there's no OOM of EPC in SGX driver, and we don't support >>>>>> evicting EPC from KVM guests yet, therefore sgx_alloc_page will never >>>>>> return if there's no enough EPC when creating KVM guest. This even applies >>>>>> to host sgx app alone as well as there's no OOM for EPC now. >>>>>> >>>>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >>>>> >>>>> Why you are packaging all of this into a single commit? Which of these >>>>> changes are mandatory to continue with KVM? >>>>> >>>>> Please split. Your short summary is misleading right now. Essentially >>>>> a good golde rule for commit scope is that if your detailed changes >>>>> do not fall in the scope of your short summary you should probably >>>>> split. >>>> >>>> Besides adding SGX_FREE_NO_EREMOVE can be split I don't see why others can >>>> be split. Probably I can split this patch into: >>>> >>>> - remove sgx_tgid_ctx * ctx from sgx_alloc_page, sgx_encl *encl from >>>> sgx_free_page, and introduce 'sgx_get{put}_tgid_ctx. The place where >>>> sgx_alloc{free}_page are called are changed accordingly of course. >>>> >>>> >>>> >>>> - expose sgx_alloc_page and sgx_free_page for KVM. >>>> >>>> I will drop adding SGX_FREE_NO_EREMOVE back. No problem. I just think >>>> probably it's better to move EREMOVE out from sgx_free_page at all, but it's >>>> your call. >>>> >>>> Let me know if you are happy with the split above. >>>> >>>> Btw if you really don't like changing sgx_alloc{free}_page interfaces, KVM >>>> can still call them by passing NULL for sgx_tgid_ctx *ctx and sgx_encl >>>> *encl, but they are just meaningless for KVM. People may ask, ex, why you >>>> always pass NULL for sgx_alloc{free}_page and what does that mean? >>>> >>>>> >>>>> PS. For those changes that actually just revert things like NO_EREMOVE >>>>> could you first of clearly state that it is a revert and why you want >>>>> it back. >>>>> >>>>> I removed it because it added unnecessary complexity with almost no gain. >>>>> Even if the call is sometimes useless it still returns with success. For >>>>> v1 driver I rather would not add it back. >>>> >>>> I have no idea of why it was there at the beginning and why it was reverted. >>>> I have no problem keeping it in current way. I just have to change KVM code >>>> to suit with it. >>>> >>>> Thanks, >>>> -Kai >>> >>> Beginning it was there because well this has been an internal driver for >>> a long time so there just exist some unnecessary cruft :-) >>> >>> Generally I think you should rethink this whole patch set. >> >> Sure. >> >>> >>> What I would propose to you is to create patches to improve multibank >>> EPC. We could queue that after v1 driver has been upstreamed. It's real >>> well scoped functionality. What do you think? >> >> Moving kthread creation out is simple and I think if you would handle it I'd >> rather leave it to you. Do you mean queue the patch to improve multibank EPC >> or queue patches to support 'unified EPC' for KVM? I thought in our last >> meeting we have agreed that the patches for support 'unified EPC' would be >> merged to your driver before you upstream it (only after you review and are >> happy with the patches, of course). > > OK Thank you. > >> >> I understand you don't want to make big changes before upstream but I think >> if the patches are just about to expose necessary EPC management functions >> (will send them out later) I think there won't be big risk. I personally >> will test on KVM side, but as I said I couldn't test on host side due to >> testsuite (although I am happy to) so if you can do some sanity test on your >> side I think there won't be big risk. I can send you how to test on KVM side >> as well if you are interested :) > > Lets walk this with baby steps. If I first do the export and you > give me feedback when you have kind of "lowest common denominator" > interface to continue your work? 4 interfaces are mandatory: - sgx_alloc_page, sgx_free_page - sgx_get_epc_page, sgx_put_epc_page For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' from sgx_free_page, if you agree. The reason is they are meaningless for KVM, and you can review this patch to get idea what needs to be changed accordingly after removing the two parameters from sgx_alloc_page and sgx_free_page. Please let me know if this is OK for you. Thanks, -Kai > >> Thanks, >> -Kai > > /Jarkko >
On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: > 4 interfaces are mandatory: > > - sgx_alloc_page, sgx_free_page > - sgx_get_epc_page, sgx_put_epc_page > > For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct > sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' from > sgx_free_page, if you agree. The reason is they are meaningless for KVM, and > you can review this patch to get idea what needs to be changed accordingly > after removing the two parameters from sgx_alloc_page and sgx_free_page. I'll update the driver code to make these available right on Monday. I'll try to get this done as my first thing on that day so that I don't slow you down. I disagree on your proposal. If you don't need them, pass NULL. /Jarkko
On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: > On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: >> 4 interfaces are mandatory: >> >> - sgx_alloc_page, sgx_free_page >> - sgx_get_epc_page, sgx_put_epc_page >> >> For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct >> sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' from >> sgx_free_page, if you agree. The reason is they are meaningless for KVM, and >> you can review this patch to get idea what needs to be changed accordingly >> after removing the two parameters from sgx_alloc_page and sgx_free_page. > > I'll update the driver code to make these available right on Monday. > I'll try to get this done as my first thing on that day so that I > don't slow you down. > Thanks for doing this. > I disagree on your proposal. If you don't need them, pass NULL. No problem. I'll change KVM code accordingly. Thanks, -Kai > > /Jarkko >
On 4/10/2017 11:36 AM, Huang, Kai wrote: > > > On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: >> On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: >>> 4 interfaces are mandatory: >>> >>> - sgx_alloc_page, sgx_free_page >>> - sgx_get_epc_page, sgx_put_epc_page >>> >>> For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct >>> sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' >>> from >>> sgx_free_page, if you agree. The reason is they are meaningless for >>> KVM, and >>> you can review this patch to get idea what needs to be changed >>> accordingly >>> after removing the two parameters from sgx_alloc_page and sgx_free_page. >> >> I'll update the driver code to make these available right on Monday. >> I'll try to get this done as my first thing on that day so that I >> don't slow you down. >> > > Thanks for doing this. A reminder that I forgot to mention: currently sgx_free_page always assumes 'struct sgx_encl *encl' to be valid, so you need to add additional check against NULL 'struct sgx_encl *encl' parameter. Thanks, -Kai > >> I disagree on your proposal. If you don't need them, pass NULL. > > No problem. I'll change KVM code accordingly. > > Thanks, > -Kai >> >> /Jarkko >> > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev >
On Mon, Apr 10, 2017 at 12:47:11PM +1200, Huang, Kai wrote: > > > On 4/10/2017 11:36 AM, Huang, Kai wrote: > > > > > > On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: > > > On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: > > > > 4 interfaces are mandatory: > > > > > > > > - sgx_alloc_page, sgx_free_page > > > > - sgx_get_epc_page, sgx_put_epc_page > > > > > > > > For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct > > > > sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' > > > > from > > > > sgx_free_page, if you agree. The reason is they are meaningless for > > > > KVM, and > > > > you can review this patch to get idea what needs to be changed > > > > accordingly > > > > after removing the two parameters from sgx_alloc_page and sgx_free_page. > > > > > > I'll update the driver code to make these available right on Monday. > > > I'll try to get this done as my first thing on that day so that I > > > don't slow you down. > > > > > > > Thanks for doing this. > > A reminder that I forgot to mention: currently sgx_free_page always assumes > 'struct sgx_encl *encl' to be valid, so you need to add additional check > against NULL 'struct sgx_encl *encl' parameter. > > Thanks, > -Kai No worries, I'll rather add check than turn the driver over :) /Jarkko
On 4/10/2017 9:57 PM, Jarkko Sakkinen wrote: > On Mon, Apr 10, 2017 at 12:47:11PM +1200, Huang, Kai wrote: >> >> >> On 4/10/2017 11:36 AM, Huang, Kai wrote: >>> >>> >>> On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: >>>> On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: >>>>> 4 interfaces are mandatory: >>>>> >>>>> - sgx_alloc_page, sgx_free_page >>>>> - sgx_get_epc_page, sgx_put_epc_page >>>>> >>>>> For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct >>>>> sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' >>>>> from >>>>> sgx_free_page, if you agree. The reason is they are meaningless for >>>>> KVM, and >>>>> you can review this patch to get idea what needs to be changed >>>>> accordingly >>>>> after removing the two parameters from sgx_alloc_page and sgx_free_page. >>>> >>>> I'll update the driver code to make these available right on Monday. >>>> I'll try to get this done as my first thing on that day so that I >>>> don't slow you down. >>>> >>> >>> Thanks for doing this. >> >> A reminder that I forgot to mention: currently sgx_free_page always assumes >> 'struct sgx_encl *encl' to be valid, so you need to add additional check >> against NULL 'struct sgx_encl *encl' parameter. >> >> Thanks, >> -Kai > > No worries, I'll rather add check than turn the driver over :) Understood. Thank you :) Thanks, -Kai > > /Jarkko >
On Tue, Apr 11, 2017 at 10:38:50AM +1200, Huang, Kai wrote: > > > On 4/10/2017 9:57 PM, Jarkko Sakkinen wrote: > > On Mon, Apr 10, 2017 at 12:47:11PM +1200, Huang, Kai wrote: > > > > > > > > > On 4/10/2017 11:36 AM, Huang, Kai wrote: > > > > > > > > > > > > On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: > > > > > On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: > > > > > > 4 interfaces are mandatory: > > > > > > > > > > > > - sgx_alloc_page, sgx_free_page > > > > > > - sgx_get_epc_page, sgx_put_epc_page > > > > > > > > > > > > For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct > > > > > > sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' > > > > > > from > > > > > > sgx_free_page, if you agree. The reason is they are meaningless for > > > > > > KVM, and > > > > > > you can review this patch to get idea what needs to be changed > > > > > > accordingly > > > > > > after removing the two parameters from sgx_alloc_page and sgx_free_page. > > > > > > > > > > I'll update the driver code to make these available right on Monday. > > > > > I'll try to get this done as my first thing on that day so that I > > > > > don't slow you down. > > > > > > > > > > > > > Thanks for doing this. > > > > > > A reminder that I forgot to mention: currently sgx_free_page always assumes > > > 'struct sgx_encl *encl' to be valid, so you need to add additional check > > > against NULL 'struct sgx_encl *encl' parameter. > > > > > > Thanks, > > > -Kai > > > > No worries, I'll rather add check than turn the driver over :) > Understood. Thank you :) > > Thanks, > -Kai > > > > /Jarkko I updated the driver. Is this what you want? /Jarkko
On 4/13/2017 8:38 AM, Jarkko Sakkinen wrote: > On Tue, Apr 11, 2017 at 10:38:50AM +1200, Huang, Kai wrote: >> >> >> On 4/10/2017 9:57 PM, Jarkko Sakkinen wrote: >>> On Mon, Apr 10, 2017 at 12:47:11PM +1200, Huang, Kai wrote: >>>> >>>> >>>> On 4/10/2017 11:36 AM, Huang, Kai wrote: >>>>> >>>>> >>>>> On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: >>>>>> On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: >>>>>>> 4 interfaces are mandatory: >>>>>>> >>>>>>> - sgx_alloc_page, sgx_free_page >>>>>>> - sgx_get_epc_page, sgx_put_epc_page >>>>>>> >>>>>>> For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct >>>>>>> sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' >>>>>>> from >>>>>>> sgx_free_page, if you agree. The reason is they are meaningless for >>>>>>> KVM, and >>>>>>> you can review this patch to get idea what needs to be changed >>>>>>> accordingly >>>>>>> after removing the two parameters from sgx_alloc_page and sgx_free_page. >>>>>> >>>>>> I'll update the driver code to make these available right on Monday. >>>>>> I'll try to get this done as my first thing on that day so that I >>>>>> don't slow you down. >>>>>> >>>>> >>>>> Thanks for doing this. >>>> >>>> A reminder that I forgot to mention: currently sgx_free_page always assumes >>>> 'struct sgx_encl *encl' to be valid, so you need to add additional check >>>> against NULL 'struct sgx_encl *encl' parameter. >>>> >>>> Thanks, >>>> -Kai >>> >>> No worries, I'll rather add check than turn the driver over :) >> Understood. Thank you :) >> >> Thanks, >> -Kai >>> >>> /Jarkko > > I updated the driver. Is this what you want? Hi Jarkko, Yes this is what I wanted. One minor thing is would you also put below to asm/sgx.h? enum sgx_alloc_flags { SGX_ALLOC_ATOMIC = BIT(0), }; Thanks, -Kai > > /Jarkko >
On Thu, Apr 13, 2017 at 11:00:09AM +1200, Huang, Kai wrote: > > > On 4/13/2017 8:38 AM, Jarkko Sakkinen wrote: > > On Tue, Apr 11, 2017 at 10:38:50AM +1200, Huang, Kai wrote: > > > > > > > > > On 4/10/2017 9:57 PM, Jarkko Sakkinen wrote: > > > > On Mon, Apr 10, 2017 at 12:47:11PM +1200, Huang, Kai wrote: > > > > > > > > > > > > > > > On 4/10/2017 11:36 AM, Huang, Kai wrote: > > > > > > > > > > > > > > > > > > On 4/8/2017 7:34 AM, Jarkko Sakkinen wrote: > > > > > > > On Fri, Apr 07, 2017 at 05:48:46PM +1200, Huang, Kai wrote: > > > > > > > > 4 interfaces are mandatory: > > > > > > > > > > > > > > > > - sgx_alloc_page, sgx_free_page > > > > > > > > - sgx_get_epc_page, sgx_put_epc_page > > > > > > > > > > > > > > > > For sgx_alloc_page, and sgx_free_page, I also want to remove 'struct > > > > > > > > sgx_tgid_ctx *ctx from sgx_alloc_page, and remove 'struct encl *encl' > > > > > > > > from > > > > > > > > sgx_free_page, if you agree. The reason is they are meaningless for > > > > > > > > KVM, and > > > > > > > > you can review this patch to get idea what needs to be changed > > > > > > > > accordingly > > > > > > > > after removing the two parameters from sgx_alloc_page and sgx_free_page. > > > > > > > > > > > > > > I'll update the driver code to make these available right on Monday. > > > > > > > I'll try to get this done as my first thing on that day so that I > > > > > > > don't slow you down. > > > > > > > > > > > > > > > > > > > Thanks for doing this. > > > > > > > > > > A reminder that I forgot to mention: currently sgx_free_page always assumes > > > > > 'struct sgx_encl *encl' to be valid, so you need to add additional check > > > > > against NULL 'struct sgx_encl *encl' parameter. > > > > > > > > > > Thanks, > > > > > -Kai > > > > > > > > No worries, I'll rather add check than turn the driver over :) > > > Understood. Thank you :) > > > > > > Thanks, > > > -Kai > > > > > > > > /Jarkko > > > > I updated the driver. Is this what you want? > > Hi Jarkko, > > Yes this is what I wanted. One minor thing is would you also put below to > asm/sgx.h? > > enum sgx_alloc_flags { > SGX_ALLOC_ATOMIC = BIT(0), > }; > > Thanks, > -Kai Yeah, sure. Just forgot to add it. /Jarkko
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index fe1a2ba..063ff8f 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -372,4 +372,16 @@ unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry); void *sgx_kmap_epc_page(struct sgx_epc_page *entry); void sgx_kunmap_epc_page(void *epc_page_vaddr); +enum sgx_alloc_flags { + SGX_ALLOC_ATOMIC = BIT(0), +}; + +enum sgx_free_flags { + SGX_FREE_RETURN_ERROR = BIT(0), + SGX_FREE_NO_EREMOVE = BIT(1), +}; + +struct sgx_epc_page *sgx_alloc_page(unsigned int flags); +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags); + #endif /* _ASM_X86_SGX_H */ diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index b9b81d6..f9fc708 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -216,6 +216,8 @@ void sgx_unpin_mm(struct sgx_encl *encl); void sgx_invalidate(struct sgx_encl *encl); int sgx_find_encl(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **vma); +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx); +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx); enum sgx_fault_flags { SGX_FAULT_RESERVE = BIT(0), @@ -237,20 +239,8 @@ extern struct mutex sgx_tgid_ctx_mutex; extern struct list_head sgx_tgid_ctx_list; extern struct task_struct *ksgxswapd_tsk; -enum sgx_alloc_flags { - SGX_ALLOC_ATOMIC = BIT(0), -}; - -enum sgx_free_flags { - SGX_FREE_RETURN_ERROR = BIT(0), -}; - int ksgxswapd(void *p); int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, - unsigned int flags); -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, - unsigned int flags); #endif /* __ARCH_X86_INTEL_SGX_H__ */ diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c index a8729c3..5ff647f 100644 --- a/drivers/platform/x86/intel_sgx_ioctl.c +++ b/drivers/platform/x86/intel_sgx_ioctl.c @@ -216,12 +216,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) struct vm_area_struct *vma; int ret; - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); + epc_page = sgx_alloc_page(0); if (IS_ERR(epc_page)) return false; + sgx_get_tgid_ctx(encl->tgid_ctx); + if (!sgx_pin_mm(encl)) { - sgx_free_page(epc_page, encl, 0); + sgx_free_page(epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); return false; } @@ -275,7 +278,8 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req) sgx_unpin_mm(encl); return true; out: - sgx_free_page(epc_page, encl, 0); + sgx_free_page(epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); mutex_unlock(&encl->lock); sgx_unpin_mm(encl); return false; @@ -405,17 +409,19 @@ static int sgx_init_page(struct sgx_encl *encl, if (!va_page) return -ENOMEM; - epc_page = sgx_alloc_page(encl->tgid_ctx, 0); + epc_page = sgx_alloc_page(0); if (IS_ERR(epc_page)) { kfree(va_page); return PTR_ERR(epc_page); } + sgx_get_tgid_ctx(encl->tgid_ctx); vaddr = sgx_kmap_epc_page(epc_page); if (!vaddr) { sgx_warn(encl, "kmap of a new VA page failed %d\n", ret); - sgx_free_page(epc_page, encl, 0); + sgx_free_page(epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); kfree(va_page); return -EFAULT; } @@ -425,7 +431,8 @@ static int sgx_init_page(struct sgx_encl *encl, if (ret) { sgx_warn(encl, "EPA returned %d\n", ret); - sgx_free_page(epc_page, encl, 0); + sgx_free_page(epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); kfree(va_page); return -EFAULT; } @@ -539,12 +546,14 @@ static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd, encl->backing = backing; encl->pcmd = pcmd; - secs_epc = sgx_alloc_page(encl->tgid_ctx, 0); + secs_epc = sgx_alloc_page(0); if (IS_ERR(secs_epc)) { ret = PTR_ERR(secs_epc); secs_epc = NULL; goto out; } + /* don't need to sgx_get_tgid_ctx(encl->tgid_ctx) as encl->tgid_ctx is + * NULL now */ ret = sgx_add_to_tgid_ctx(encl); if (ret) diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c index 3f9ba9c..71569e7 100644 --- a/drivers/platform/x86/intel_sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx_page_cache.c @@ -330,7 +330,8 @@ static void sgx_evict_page(struct sgx_encl_page *entry, struct sgx_encl *encl) { sgx_ewb(encl, entry); - sgx_free_page(entry->epc_page, encl, 0); + sgx_free_page(entry->epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); entry->epc_page = NULL; entry->flags &= ~SGX_ENCL_PAGE_RESERVED; } @@ -554,18 +555,15 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void) * * Return: an EPC page or an error code */ -struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, - unsigned int flags) +struct sgx_epc_page *sgx_alloc_page(unsigned int flags) { struct sgx_epc_page *entry; for ( ; ; ) { entry = sgx_alloc_page_fast(); - if (entry) { - if (ctx) - atomic_inc(&ctx->epc_cnt); + if (entry) break; - } else if (flags & SGX_ALLOC_ATOMIC) { + else if (flags & SGX_ALLOC_ATOMIC) { entry = ERR_PTR(-EBUSY); break; } @@ -584,6 +582,7 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, return entry; } +EXPORT_SYMBOL_GPL(sgx_alloc_page); /** * sgx_free_page - free an EPC page @@ -591,25 +590,29 @@ struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *ctx, * @encl: the enclave who owns the EPC page * @flags: free flags */ -int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, - unsigned int flags) +int sgx_free_page(struct sgx_epc_page *entry, unsigned int flags) { void *epc; - int ret; + int ret = 0; - epc = sgx_kmap_epc_page(entry); - ret = __eremove(epc); - sgx_kunmap_epc_page(epc); + if (!(flags & SGX_FREE_NO_EREMOVE)) { + epc = sgx_kmap_epc_page(entry); + ret = __eremove(epc); + sgx_kunmap_epc_page(epc); + } if (ret) { if (flags & SGX_FREE_RETURN_ERROR) return ret; - sgx_crit(encl, "EREMOVE returned %d\n", ret); + /* + * sgx_crit is removed because it takes encl as parameter, + * which sgx_free_page doesn't take anymore. + * TODO: remove encl from sgx_{print} functions and call + * sgx_crit again. + */ } - atomic_dec(&encl->tgid_ctx->epc_cnt); - spin_lock(&sgx_free_list_lock); list_add(&entry->free_list, &sgx_free_list); sgx_nr_free_pages++; @@ -617,3 +620,4 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, return 0; } +EXPORT_SYMBOL_GPL(sgx_free_page); diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c index c63193d..96b3770 100644 --- a/drivers/platform/x86/intel_sgx_util.c +++ b/drivers/platform/x86/intel_sgx_util.c @@ -372,12 +372,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, goto out; } - epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); + epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); if (IS_ERR(epc_page)) { rc = PTR_ERR(epc_page); epc_page = NULL; goto out; } + sgx_get_tgid_ctx(encl->tgid_ctx); if (encl->flags & SGX_ENCL_DEAD) { rc = -EFAULT; @@ -406,12 +407,13 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, /* If SECS is evicted then reload it first */ if (encl->flags & SGX_ENCL_SECS_EVICTED) { - secs_epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC); + secs_epc_page = sgx_alloc_page(SGX_ALLOC_ATOMIC); if (IS_ERR(secs_epc_page)) { rc = PTR_ERR(secs_epc_page); secs_epc_page = NULL; goto out; } + sgx_get_tgid_ctx(encl->tgid_ctx); rc = sgx_eldu(encl, &encl->secs_page, secs_epc_page, true); if (rc) @@ -446,10 +448,14 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma, list_add_tail(&entry->load_list, &encl->load_list); out: mutex_unlock(&encl->lock); - if (epc_page) - sgx_free_page(epc_page, encl, 0); - if (secs_epc_page) - sgx_free_page(secs_epc_page, encl, 0); + if (epc_page) { + sgx_free_page(epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); + } + if (secs_epc_page) { + sgx_free_page(secs_epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); + } return rc ? ERR_PTR(rc) : entry; } @@ -489,7 +495,8 @@ void sgx_encl_release(struct kref *ref) entry = *slot; if (entry->epc_page) { list_del(&entry->load_list); - sgx_free_page(entry->epc_page, encl, 0); + sgx_free_page(entry->epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); } radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT); kfree(entry); @@ -499,12 +506,15 @@ 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_free_page(va_page->epc_page, encl, 0); + sgx_free_page(va_page->epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); kfree(va_page); } - if (encl->secs_page.epc_page) - sgx_free_page(encl->secs_page.epc_page, encl, 0); + if (encl->secs_page.epc_page) { + sgx_free_page(encl->secs_page.epc_page, 0); + sgx_put_tgid_ctx(encl->tgid_ctx); + } encl->secs_page.epc_page = NULL; @@ -519,3 +529,13 @@ void sgx_encl_release(struct kref *ref) kfree(encl); } + +void sgx_get_tgid_ctx(struct sgx_tgid_ctx *ctx) +{ + atomic_inc(&ctx->epc_cnt); +} + +void sgx_put_tgid_ctx(struct sgx_tgid_ctx *ctx) +{ + atomic_dec(&ctx->epc_cnt); +}
KVM needs to call the two to allocate and free EPC pages. Also removed 'struct sgx_tgid_ctx *ctx' parameter from sgx_alloc_page and removed 'struct sgx_encl *encl' from sgx_free_page, because they are meaningless to KVM. Remove them to make sgx_alloc{free}_page independent of host SGX app. sgx_get{put}_tgid_ctx are introduced and called separately after sgx_alloc{free}_page are called to reflect the original logic. Also introduce SGX_FREE_NO_EREMOVE flag for sgx_free_page, because KVM always assumes EREMOVE to be not part of sgx_free_page. Probably a better way is to remove EREMOVE from sgx_free_page and call it explicitly where it is needed. Note that as there's no OOM of EPC in SGX driver, and we don't support evicting EPC from KVM guests yet, therefore sgx_alloc_page will never return if there's no enough EPC when creating KVM guest. This even applies to host sgx app alone as well as there's no OOM for EPC now. Signed-off-by: Kai Huang <kai.huang@linux.intel.com> --- arch/x86/include/asm/sgx.h | 12 +++++++++ drivers/platform/x86/intel_sgx.h | 14 ++-------- drivers/platform/x86/intel_sgx_ioctl.c | 23 ++++++++++++----- drivers/platform/x86/intel_sgx_page_cache.c | 36 ++++++++++++++------------ drivers/platform/x86/intel_sgx_util.c | 40 +++++++++++++++++++++-------- 5 files changed, 80 insertions(+), 45 deletions(-)