[intel-sgx-kernel-dev,5/7] intel_sgx: expose sgx_alloc{free}_page
diff mbox

Message ID 20170404072938.4800-6-kai.huang@linux.intel.com
State New
Headers show

Commit Message

Kai Huang April 4, 2017, 7:29 a.m. UTC
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(-)

Comments

Jarkko Sakkinen April 4, 2017, 2:48 p.m. UTC | #1
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
>
Kai Huang April 5, 2017, 12:37 a.m. UTC | #2
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
>
Jarkko Sakkinen April 5, 2017, 7:42 a.m. UTC | #3
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
Jarkko Sakkinen April 5, 2017, 7:52 a.m. UTC | #4
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
Kai Huang April 5, 2017, 8:26 a.m. UTC | #5
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
>
Kai Huang April 6, 2017, 8:51 a.m. UTC | #6
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
>
Jarkko Sakkinen April 6, 2017, 6:44 p.m. UTC | #7
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
Kai Huang April 7, 2017, 5:48 a.m. UTC | #8
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
>
Jarkko Sakkinen April 7, 2017, 7:34 p.m. UTC | #9
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
Kai Huang April 9, 2017, 11:36 p.m. UTC | #10
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
>
Kai Huang April 10, 2017, 12:47 a.m. UTC | #11
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
>
Jarkko Sakkinen April 10, 2017, 9:57 a.m. UTC | #12
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
Kai Huang April 10, 2017, 10:38 p.m. UTC | #13
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
>
Jarkko Sakkinen April 12, 2017, 8:38 p.m. UTC | #14
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
Kai Huang April 12, 2017, 11 p.m. UTC | #15
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
>
Jarkko Sakkinen April 13, 2017, 12:06 p.m. UTC | #16
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

Patch
diff mbox

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);
+}