[intel-sgx-kernel-dev,3/7] intel_sgx: expose sgx_pfn_to_epc_page
diff mbox

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

Commit Message

Kai Huang April 4, 2017, 7:29 a.m. UTC
And sgx_epc_page_to_pfn. They are equivalent to page_to_pfn, pfn_to_page for
normal memory. KVM needs to find 'struct sgx_epc_page' given particular PFN
upon EPC faulting from guest EPC access.

Current code allocates 'struct sgx_epc_page' one by one in which case we have
to manually compare physical address of each 'struct sgx_epc_page' in order to
find the right one given particular PFN. It is unefficient. Changing to allocate
one EPC page table for all 'struct sgx_epc_page' at once for each EPC bank
when EPC bank is initialized, so that we can find 'struct sgx_epc_page' quickly
given particular PFN in that bank. Consequently, pa member is removed from
'struct sgx_epc_page' as it is not necessary anymore.

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 arch/x86/include/asm/sgx.h                  |  7 +++
 drivers/platform/x86/intel_sgx.h            |  6 +--
 drivers/platform/x86/intel_sgx_ioctl.c      |  3 +-
 drivers/platform/x86/intel_sgx_page_cache.c | 45 ++++++++---------
 drivers/platform/x86/intel_sgx_util.c       | 77 +++++++++++++++++++++++++----
 5 files changed, 100 insertions(+), 38 deletions(-)

Comments

Jarkko Sakkinen April 4, 2017, 2:57 p.m. UTC | #1
On Tue, Apr 04, 2017 at 07:29:34PM +1200, Kai Huang wrote:
> And sgx_epc_page_to_pfn. They are equivalent to page_to_pfn, pfn_to_page for
> normal memory. KVM needs to find 'struct sgx_epc_page' given particular PFN
> upon EPC faulting from guest EPC access.
> 
> Current code allocates 'struct sgx_epc_page' one by one in which case we have
> to manually compare physical address of each 'struct sgx_epc_page' in order to
> find the right one given particular PFN. It is unefficient. Changing to allocate
> one EPC page table for all 'struct sgx_epc_page' at once for each EPC bank
> when EPC bank is initialized, so that we can find 'struct sgx_epc_page' quickly
> given particular PFN in that bank. Consequently, pa member is removed from
> 'struct sgx_epc_page' as it is not necessary anymore.
> 
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> ---
>  arch/x86/include/asm/sgx.h                  |  7 +++
>  drivers/platform/x86/intel_sgx.h            |  6 +--
>  drivers/platform/x86/intel_sgx_ioctl.c      |  3 +-
>  drivers/platform/x86/intel_sgx_page_cache.c | 45 ++++++++---------
>  drivers/platform/x86/intel_sgx_util.c       | 77 +++++++++++++++++++++++++----
>  5 files changed, 100 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 190e3ed..0e3fee1 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -363,4 +363,11 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *epc)
>  	return __encls_ret(EMODT, secinfo, epc, rdx);
>  }
>  
> +struct sgx_epc_page {
> +	struct list_head	free_list;
> +};

Why do you need a linked list of nothing??

This somehow indicates that this commit is not too well thought.

/Jarkko

> +
> +struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn);
> +unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry);
> +
>  #endif /* _ASM_X86_SGX_H */
> diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h
> index a98915a..da662d0 100644
> --- a/drivers/platform/x86/intel_sgx.h
> +++ b/drivers/platform/x86/intel_sgx.h
> @@ -74,11 +74,6 @@
>  #define SGX_EINIT_SLEEP_COUNT	50
>  #define SGX_EINIT_SLEEP_TIME	20
>  
> -struct sgx_epc_page {
> -	resource_size_t		pa;
> -	struct list_head	free_list;
> -};
> -
>  #define SGX_VA_SLOT_COUNT 512
>  
>  struct sgx_va_page {
> @@ -161,6 +156,7 @@ struct sgx_epc_bank {
>  #endif
>  	unsigned long start;
>  	unsigned long end;
> +	struct sgx_epc_page *epgtbl;
>  };
>  
>  extern struct workqueue_struct *sgx_add_page_wq;
> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
> index e0e2f14..3fc89ac 100644
> --- a/drivers/platform/x86/intel_sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> @@ -243,7 +243,8 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
>  		goto out;
>  	}
>  
> -	ret = vm_insert_pfn(vma, encl_page->addr, PFN_DOWN(epc_page->pa));
> +	ret = vm_insert_pfn(vma, encl_page->addr,
> +			sgx_epc_page_to_pfn(epc_page));
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index 2a277ba..8d323f4 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -422,9 +422,7 @@ int ksgxswapd(void *p)
>  
>  static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>  {
> -	unsigned long i, size;
> -	struct sgx_epc_page *new_epc_page, *entry;
> -	struct list_head *parser, *temp;
> +	unsigned long i, nrpages;
>  
>  	pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n",
>  			bank->start, bank->end);
> @@ -432,22 +430,24 @@ static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>  	if (!bank->start || !bank->end)
>  		return -ENODEV;
>  
> -	size = bank->end - bank->start;
> +	nrpages = (bank->end - bank->start) >> PAGE_SHIFT;
>  
>  #ifdef CONFIG_X86_64
> -	bank->mem = ioremap_cache(bank->start, size);
> +	bank->mem = ioremap_cache(bank->start, nrpages << PAGE_SHIFT);
>  	if (!bank->mem)
>  		return -ENOMEM;
>  #endif
>  
> -	for (i = 0; i < size; i += PAGE_SIZE) {
> -		new_epc_page = kzalloc(sizeof(*new_epc_page), GFP_KERNEL);
> -		if (!new_epc_page)
> -			goto err_freelist;
> -		new_epc_page->pa = bank->start + i;
> +	bank->epgtbl = alloc_pages_exact(sizeof(struct sgx_epc_page) * nrpages,
> +			GFP_KERNEL);
> +	if (!bank->epgtbl)
> +		goto err_iounmap;
> +
> +	for (i = 0; i < nrpages; i++) {
> +		struct sgx_epc_page *entry = bank->epgtbl + i;
>  
>  		spin_lock(&sgx_free_list_lock);
> -		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
> +		list_add_tail(&entry->free_list, &sgx_free_list);
>  		sgx_nr_total_epc_pages++;
>  		sgx_nr_free_pages++;
>  		spin_unlock(&sgx_free_list_lock);
> @@ -455,14 +455,7 @@ static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>  
>  	return 0;
>  
> -err_freelist:
> -	list_for_each_safe(parser, temp, &sgx_free_list) {
> -		spin_lock(&sgx_free_list_lock);
> -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> -		list_del(&entry->free_list);
> -		spin_unlock(&sgx_free_list_lock);
> -		kfree(entry);
> -	}
> +err_iounmap:
>  #ifdef CONFIG_X86_64
>  	iounmap(bank->mem);
>  #endif
> @@ -475,9 +468,13 @@ static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank)
>  		return;
>  
>  	/*
> -	 * Don't free 'struct sgx_epc_page' for EPC page in this bank. All
> -	 * 'struct sgx_epc_page' for all EPC pages will be destroyed together.
> +	 * 'struct sgx_epc_page' for all EPC pages in this bank have already
> +	 * been deleted from sgx_free_list. Just free the memory holding them.
>  	 */
> +	if (bank->epgtbl)
> +		free_pages_exact(bank->epgtbl, sizeof (struct sgx_epc_page) *
> +				((bank->end - bank->start) >> PAGE_SHIFT));
> +
>  #ifdef CONFIG_X86_64
>  	if (bank->mem)
>  		iounmap(bank->mem);
> @@ -516,11 +513,15 @@ void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks)
>  	if (ksgxswapd_tsk)
>  		kthread_stop(ksgxswapd_tsk);
>  
> +	/*
> +	 * To make sure, delete all 'struct sgx_epc_page' from sgx_free_list
> +	 * before calling sgx_epc_bank_teardown where memory holding them is
> +	 * freed.
> +	 */
>  	spin_lock(&sgx_free_list_lock);
>  	list_for_each_safe(parser, temp, &sgx_free_list) {
>  		entry = list_entry(parser, struct sgx_epc_page, free_list);
>  		list_del(&entry->free_list);
> -		kfree(entry);
>  	}
>  	spin_unlock(&sgx_free_list_lock);
>  
> diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c
> index 234a5fb..6d36b98 100644
> --- a/drivers/platform/x86/intel_sgx_util.c
> +++ b/drivers/platform/x86/intel_sgx_util.c
> @@ -63,22 +63,79 @@
>  #include <linux/shmem_fs.h>
>  #include <linux/sched/mm.h>
>  
> -void *sgx_get_epc_page(struct sgx_epc_page *entry)
> +static struct sgx_epc_bank *sgx_epc_pfn_to_bank(unsigned long pfn)
>  {
> -#ifdef CONFIG_X86_32
> -	return kmap_atomic_pfn(PFN_DOWN(entry->pa));
> -#else
>  	int i;
>  
>  	for (i = 0; i < sgx_nr_epc_banks; i++) {
> -		if (entry->pa < sgx_epc_banks[i].end &&
> -		    entry->pa >= sgx_epc_banks[i].start) {
> -			return sgx_epc_banks[i].mem +
> -				(entry->pa - sgx_epc_banks[i].start);
> -		}
> +		struct sgx_epc_bank *bank = sgx_epc_banks + i;
> +		if (pfn >= (bank->start >> PAGE_SHIFT) &&
> +				pfn < (bank->end >> PAGE_SHIFT))
> +			return bank;
>  	}
>  
>  	return NULL;
> +}
> +
> +static struct sgx_epc_bank *sgx_epc_page_to_bank(struct sgx_epc_page *entry)
> +{
> +	int i;
> +
> +	for (i = 0; i < sgx_nr_epc_banks; i++) {
> +		struct sgx_epc_bank *bank = sgx_epc_banks + i;
> +		unsigned long idx = entry - bank->epgtbl;
> +		if (idx < (bank->end - bank->start) >> PAGE_SHIFT)
> +			return bank;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * sgx_epc_pfn_to_page - get 'struct sgx_epc_page' given PFN
> + * @pfn:	physical frame number of EPC page
> + *
> + * Return: 'struct sgx_epc_page' for EPC page, or NULL if invalid PFN
> + */
> +struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn)
> +{
> +	struct sgx_epc_bank *bank = sgx_epc_pfn_to_bank(pfn);
> +
> +	if (!bank)
> +		return NULL;
> +
> +	return bank->epgtbl + (pfn - (bank->start >> PAGE_SHIFT));
> +}
> +EXPORT_SYMBOL_GPL(sgx_epc_pfn_to_page);
> +
> +/**
> + * sgx_epc_page_to_pfn - get PFN given 'struct sgx_epc_page'
> + * @entry:	'struct sgx_epc_page' for particular EPC page
> + *
> + * Return: PFN of EPC page, or 0 if invalid 'struct sgx_epc_page'
> + */
> +unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry)
> +{
> +	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
> +
> +	if (!bank)
> +		return 0;
> +
> +	return (bank->start >> PAGE_SHIFT) + (entry - bank->epgtbl);
> +}
> +EXPORT_SYMBOL_GPL(sgx_epc_page_to_pfn);
> +
> +void *sgx_get_epc_page(struct sgx_epc_page *entry)
> +{
> +#ifdef CONFIG_X86_32
> +	return kmap_atomic_pfn(sgx_epc_page_to_pfn(entry));
> +#else
> +	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
> +
> +	if (!bank)
> +		return NULL;
> +
> +	return bank->mem + ((entry - bank->epgtbl) << PAGE_SHIFT);
>  #endif
>  }
>  
> @@ -369,7 +426,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	if (rc)
>  		goto out;
>  
> -	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
> +	rc = vm_insert_pfn(vma, entry->addr, sgx_epc_page_to_pfn(epc_page));
>  	if (rc)
>  		goto out;
>  
> -- 
> 2.9.3
>
Kai Huang April 5, 2017, 12:11 a.m. UTC | #2
On 4/5/2017 2:57 AM, Jarkko Sakkinen wrote:
> On Tue, Apr 04, 2017 at 07:29:34PM +1200, Kai Huang wrote:
>> And sgx_epc_page_to_pfn. They are equivalent to page_to_pfn, pfn_to_page for
>> normal memory. KVM needs to find 'struct sgx_epc_page' given particular PFN
>> upon EPC faulting from guest EPC access.
>>
>> Current code allocates 'struct sgx_epc_page' one by one in which case we have
>> to manually compare physical address of each 'struct sgx_epc_page' in order to
>> find the right one given particular PFN. It is unefficient. Changing to allocate
>> one EPC page table for all 'struct sgx_epc_page' at once for each EPC bank
>> when EPC bank is initialized, so that we can find 'struct sgx_epc_page' quickly
>> given particular PFN in that bank. Consequently, pa member is removed from
>> 'struct sgx_epc_page' as it is not necessary anymore.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> ---
>>  arch/x86/include/asm/sgx.h                  |  7 +++
>>  drivers/platform/x86/intel_sgx.h            |  6 +--
>>  drivers/platform/x86/intel_sgx_ioctl.c      |  3 +-
>>  drivers/platform/x86/intel_sgx_page_cache.c | 45 ++++++++---------
>>  drivers/platform/x86/intel_sgx_util.c       | 77 +++++++++++++++++++++++++----
>>  5 files changed, 100 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
>> index 190e3ed..0e3fee1 100644
>> --- a/arch/x86/include/asm/sgx.h
>> +++ b/arch/x86/include/asm/sgx.h
>> @@ -363,4 +363,11 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *epc)
>>  	return __encls_ret(EMODT, secinfo, epc, rdx);
>>  }
>>
>> +struct sgx_epc_page {
>> +	struct list_head	free_list;
>> +};
>
> Why do you need a linked list of nothing??
>
> This somehow indicates that this commit is not too well thought.

I did this patch because currently I call sgx_epc_pfn_to_page 
occasionally in KVM, and I thought it would be nice to have 
sgx_epc_page_to_pfn and sgx_epc_pfn_to_page (like page_to_pfn, 
pfn_to_page for memory), just in case we will need them in future. I 
used to have member 'refcount' in 'struct sgx_epc_page' as well and have 
sgx_get{put}_epc_page to decrease/increase the refcount. But I recently 
changed the implementation so sgx_get{put}_epc_page are not necessary 
anymore so I removed 'refcount' in 'struct sgx_epc_page'.

It seems I can avoid sgx_epc_pfn_to_page as well if I do some change in 
current implementation, so if you don't like this patch I can drop it.

Thanks,
-Kai

>
> /Jarkko
>
>> +
>> +struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn);
>> +unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry);
>> +
>>  #endif /* _ASM_X86_SGX_H */
>> diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h
>> index a98915a..da662d0 100644
>> --- a/drivers/platform/x86/intel_sgx.h
>> +++ b/drivers/platform/x86/intel_sgx.h
>> @@ -74,11 +74,6 @@
>>  #define SGX_EINIT_SLEEP_COUNT	50
>>  #define SGX_EINIT_SLEEP_TIME	20
>>
>> -struct sgx_epc_page {
>> -	resource_size_t		pa;
>> -	struct list_head	free_list;
>> -};
>> -
>>  #define SGX_VA_SLOT_COUNT 512
>>
>>  struct sgx_va_page {
>> @@ -161,6 +156,7 @@ struct sgx_epc_bank {
>>  #endif
>>  	unsigned long start;
>>  	unsigned long end;
>> +	struct sgx_epc_page *epgtbl;
>>  };
>>
>>  extern struct workqueue_struct *sgx_add_page_wq;
>> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
>> index e0e2f14..3fc89ac 100644
>> --- a/drivers/platform/x86/intel_sgx_ioctl.c
>> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
>> @@ -243,7 +243,8 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
>>  		goto out;
>>  	}
>>
>> -	ret = vm_insert_pfn(vma, encl_page->addr, PFN_DOWN(epc_page->pa));
>> +	ret = vm_insert_pfn(vma, encl_page->addr,
>> +			sgx_epc_page_to_pfn(epc_page));
>>  	if (ret)
>>  		goto out;
>>
>> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
>> index 2a277ba..8d323f4 100644
>> --- a/drivers/platform/x86/intel_sgx_page_cache.c
>> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
>> @@ -422,9 +422,7 @@ int ksgxswapd(void *p)
>>
>>  static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>>  {
>> -	unsigned long i, size;
>> -	struct sgx_epc_page *new_epc_page, *entry;
>> -	struct list_head *parser, *temp;
>> +	unsigned long i, nrpages;
>>
>>  	pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n",
>>  			bank->start, bank->end);
>> @@ -432,22 +430,24 @@ static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>>  	if (!bank->start || !bank->end)
>>  		return -ENODEV;
>>
>> -	size = bank->end - bank->start;
>> +	nrpages = (bank->end - bank->start) >> PAGE_SHIFT;
>>
>>  #ifdef CONFIG_X86_64
>> -	bank->mem = ioremap_cache(bank->start, size);
>> +	bank->mem = ioremap_cache(bank->start, nrpages << PAGE_SHIFT);
>>  	if (!bank->mem)
>>  		return -ENOMEM;
>>  #endif
>>
>> -	for (i = 0; i < size; i += PAGE_SIZE) {
>> -		new_epc_page = kzalloc(sizeof(*new_epc_page), GFP_KERNEL);
>> -		if (!new_epc_page)
>> -			goto err_freelist;
>> -		new_epc_page->pa = bank->start + i;
>> +	bank->epgtbl = alloc_pages_exact(sizeof(struct sgx_epc_page) * nrpages,
>> +			GFP_KERNEL);
>> +	if (!bank->epgtbl)
>> +		goto err_iounmap;
>> +
>> +	for (i = 0; i < nrpages; i++) {
>> +		struct sgx_epc_page *entry = bank->epgtbl + i;
>>
>>  		spin_lock(&sgx_free_list_lock);
>> -		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
>> +		list_add_tail(&entry->free_list, &sgx_free_list);
>>  		sgx_nr_total_epc_pages++;
>>  		sgx_nr_free_pages++;
>>  		spin_unlock(&sgx_free_list_lock);
>> @@ -455,14 +455,7 @@ static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
>>
>>  	return 0;
>>
>> -err_freelist:
>> -	list_for_each_safe(parser, temp, &sgx_free_list) {
>> -		spin_lock(&sgx_free_list_lock);
>> -		entry = list_entry(parser, struct sgx_epc_page, free_list);
>> -		list_del(&entry->free_list);
>> -		spin_unlock(&sgx_free_list_lock);
>> -		kfree(entry);
>> -	}
>> +err_iounmap:
>>  #ifdef CONFIG_X86_64
>>  	iounmap(bank->mem);
>>  #endif
>> @@ -475,9 +468,13 @@ static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank)
>>  		return;
>>
>>  	/*
>> -	 * Don't free 'struct sgx_epc_page' for EPC page in this bank. All
>> -	 * 'struct sgx_epc_page' for all EPC pages will be destroyed together.
>> +	 * 'struct sgx_epc_page' for all EPC pages in this bank have already
>> +	 * been deleted from sgx_free_list. Just free the memory holding them.
>>  	 */
>> +	if (bank->epgtbl)
>> +		free_pages_exact(bank->epgtbl, sizeof (struct sgx_epc_page) *
>> +				((bank->end - bank->start) >> PAGE_SHIFT));
>> +
>>  #ifdef CONFIG_X86_64
>>  	if (bank->mem)
>>  		iounmap(bank->mem);
>> @@ -516,11 +513,15 @@ void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks)
>>  	if (ksgxswapd_tsk)
>>  		kthread_stop(ksgxswapd_tsk);
>>
>> +	/*
>> +	 * To make sure, delete all 'struct sgx_epc_page' from sgx_free_list
>> +	 * before calling sgx_epc_bank_teardown where memory holding them is
>> +	 * freed.
>> +	 */
>>  	spin_lock(&sgx_free_list_lock);
>>  	list_for_each_safe(parser, temp, &sgx_free_list) {
>>  		entry = list_entry(parser, struct sgx_epc_page, free_list);
>>  		list_del(&entry->free_list);
>> -		kfree(entry);
>>  	}
>>  	spin_unlock(&sgx_free_list_lock);
>>
>> diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c
>> index 234a5fb..6d36b98 100644
>> --- a/drivers/platform/x86/intel_sgx_util.c
>> +++ b/drivers/platform/x86/intel_sgx_util.c
>> @@ -63,22 +63,79 @@
>>  #include <linux/shmem_fs.h>
>>  #include <linux/sched/mm.h>
>>
>> -void *sgx_get_epc_page(struct sgx_epc_page *entry)
>> +static struct sgx_epc_bank *sgx_epc_pfn_to_bank(unsigned long pfn)
>>  {
>> -#ifdef CONFIG_X86_32
>> -	return kmap_atomic_pfn(PFN_DOWN(entry->pa));
>> -#else
>>  	int i;
>>
>>  	for (i = 0; i < sgx_nr_epc_banks; i++) {
>> -		if (entry->pa < sgx_epc_banks[i].end &&
>> -		    entry->pa >= sgx_epc_banks[i].start) {
>> -			return sgx_epc_banks[i].mem +
>> -				(entry->pa - sgx_epc_banks[i].start);
>> -		}
>> +		struct sgx_epc_bank *bank = sgx_epc_banks + i;
>> +		if (pfn >= (bank->start >> PAGE_SHIFT) &&
>> +				pfn < (bank->end >> PAGE_SHIFT))
>> +			return bank;
>>  	}
>>
>>  	return NULL;
>> +}
>> +
>> +static struct sgx_epc_bank *sgx_epc_page_to_bank(struct sgx_epc_page *entry)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < sgx_nr_epc_banks; i++) {
>> +		struct sgx_epc_bank *bank = sgx_epc_banks + i;
>> +		unsigned long idx = entry - bank->epgtbl;
>> +		if (idx < (bank->end - bank->start) >> PAGE_SHIFT)
>> +			return bank;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * sgx_epc_pfn_to_page - get 'struct sgx_epc_page' given PFN
>> + * @pfn:	physical frame number of EPC page
>> + *
>> + * Return: 'struct sgx_epc_page' for EPC page, or NULL if invalid PFN
>> + */
>> +struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn)
>> +{
>> +	struct sgx_epc_bank *bank = sgx_epc_pfn_to_bank(pfn);
>> +
>> +	if (!bank)
>> +		return NULL;
>> +
>> +	return bank->epgtbl + (pfn - (bank->start >> PAGE_SHIFT));
>> +}
>> +EXPORT_SYMBOL_GPL(sgx_epc_pfn_to_page);
>> +
>> +/**
>> + * sgx_epc_page_to_pfn - get PFN given 'struct sgx_epc_page'
>> + * @entry:	'struct sgx_epc_page' for particular EPC page
>> + *
>> + * Return: PFN of EPC page, or 0 if invalid 'struct sgx_epc_page'
>> + */
>> +unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry)
>> +{
>> +	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
>> +
>> +	if (!bank)
>> +		return 0;
>> +
>> +	return (bank->start >> PAGE_SHIFT) + (entry - bank->epgtbl);
>> +}
>> +EXPORT_SYMBOL_GPL(sgx_epc_page_to_pfn);
>> +
>> +void *sgx_get_epc_page(struct sgx_epc_page *entry)
>> +{
>> +#ifdef CONFIG_X86_32
>> +	return kmap_atomic_pfn(sgx_epc_page_to_pfn(entry));
>> +#else
>> +	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
>> +
>> +	if (!bank)
>> +		return NULL;
>> +
>> +	return bank->mem + ((entry - bank->epgtbl) << PAGE_SHIFT);
>>  #endif
>>  }
>>
>> @@ -369,7 +426,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>>  	if (rc)
>>  		goto out;
>>
>> -	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
>> +	rc = vm_insert_pfn(vma, entry->addr, sgx_epc_page_to_pfn(epc_page));
>>  	if (rc)
>>  		goto out;
>>
>> --
>> 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:36 a.m. UTC | #3
On Wed, Apr 05, 2017 at 12:11:29PM +1200, Huang, Kai wrote:
> 
> 
> On 4/5/2017 2:57 AM, Jarkko Sakkinen wrote:
> > On Tue, Apr 04, 2017 at 07:29:34PM +1200, Kai Huang wrote:
> > > And sgx_epc_page_to_pfn. They are equivalent to page_to_pfn, pfn_to_page for
> > > normal memory. KVM needs to find 'struct sgx_epc_page' given particular PFN
> > > upon EPC faulting from guest EPC access.
> > > 
> > > Current code allocates 'struct sgx_epc_page' one by one in which case we have
> > > to manually compare physical address of each 'struct sgx_epc_page' in order to
> > > find the right one given particular PFN. It is unefficient. Changing to allocate
> > > one EPC page table for all 'struct sgx_epc_page' at once for each EPC bank
> > > when EPC bank is initialized, so that we can find 'struct sgx_epc_page' quickly
> > > given particular PFN in that bank. Consequently, pa member is removed from
> > > 'struct sgx_epc_page' as it is not necessary anymore.
> > > 
> > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> > > ---
> > >  arch/x86/include/asm/sgx.h                  |  7 +++
> > >  drivers/platform/x86/intel_sgx.h            |  6 +--
> > >  drivers/platform/x86/intel_sgx_ioctl.c      |  3 +-
> > >  drivers/platform/x86/intel_sgx_page_cache.c | 45 ++++++++---------
> > >  drivers/platform/x86/intel_sgx_util.c       | 77 +++++++++++++++++++++++++----
> > >  5 files changed, 100 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > > index 190e3ed..0e3fee1 100644
> > > --- a/arch/x86/include/asm/sgx.h
> > > +++ b/arch/x86/include/asm/sgx.h
> > > @@ -363,4 +363,11 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *epc)
> > >  	return __encls_ret(EMODT, secinfo, epc, rdx);
> > >  }
> > > 
> > > +struct sgx_epc_page {
> > > +	struct list_head	free_list;
> > > +};
> > 
> > Why do you need a linked list of nothing??
> > 
> > This somehow indicates that this commit is not too well thought.
> 
> I did this patch because currently I call sgx_epc_pfn_to_page occasionally
> in KVM, and I thought it would be nice to have sgx_epc_page_to_pfn and
> sgx_epc_pfn_to_page (like page_to_pfn, pfn_to_page for memory), just in case
> we will need them in future. I used to have member 'refcount' in 'struct
> sgx_epc_page' as well and have sgx_get{put}_epc_page to decrease/increase
> the refcount. But I recently changed the implementation so
> sgx_get{put}_epc_page are not necessary anymore so I removed 'refcount' in
> 'struct sgx_epc_page'.
> 
> It seems I can avoid sgx_epc_pfn_to_page as well if I do some change in
> current implementation, so if you don't like this patch I can drop it.
> 
> Thanks,
> -Kai

Given this explanation is seems that you are pushing something that is
still at PoC level.

/Jarkko

Patch
diff mbox

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 190e3ed..0e3fee1 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -363,4 +363,11 @@  static inline int __emodt(struct sgx_secinfo *secinfo, void *epc)
 	return __encls_ret(EMODT, secinfo, epc, rdx);
 }
 
+struct sgx_epc_page {
+	struct list_head	free_list;
+};
+
+struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn);
+unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry);
+
 #endif /* _ASM_X86_SGX_H */
diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h
index a98915a..da662d0 100644
--- a/drivers/platform/x86/intel_sgx.h
+++ b/drivers/platform/x86/intel_sgx.h
@@ -74,11 +74,6 @@ 
 #define SGX_EINIT_SLEEP_COUNT	50
 #define SGX_EINIT_SLEEP_TIME	20
 
-struct sgx_epc_page {
-	resource_size_t		pa;
-	struct list_head	free_list;
-};
-
 #define SGX_VA_SLOT_COUNT 512
 
 struct sgx_va_page {
@@ -161,6 +156,7 @@  struct sgx_epc_bank {
 #endif
 	unsigned long start;
 	unsigned long end;
+	struct sgx_epc_page *epgtbl;
 };
 
 extern struct workqueue_struct *sgx_add_page_wq;
diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index e0e2f14..3fc89ac 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -243,7 +243,8 @@  static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
 		goto out;
 	}
 
-	ret = vm_insert_pfn(vma, encl_page->addr, PFN_DOWN(epc_page->pa));
+	ret = vm_insert_pfn(vma, encl_page->addr,
+			sgx_epc_page_to_pfn(epc_page));
 	if (ret)
 		goto out;
 
diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index 2a277ba..8d323f4 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -422,9 +422,7 @@  int ksgxswapd(void *p)
 
 static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
 {
-	unsigned long i, size;
-	struct sgx_epc_page *new_epc_page, *entry;
-	struct list_head *parser, *temp;
+	unsigned long i, nrpages;
 
 	pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n",
 			bank->start, bank->end);
@@ -432,22 +430,24 @@  static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
 	if (!bank->start || !bank->end)
 		return -ENODEV;
 
-	size = bank->end - bank->start;
+	nrpages = (bank->end - bank->start) >> PAGE_SHIFT;
 
 #ifdef CONFIG_X86_64
-	bank->mem = ioremap_cache(bank->start, size);
+	bank->mem = ioremap_cache(bank->start, nrpages << PAGE_SHIFT);
 	if (!bank->mem)
 		return -ENOMEM;
 #endif
 
-	for (i = 0; i < size; i += PAGE_SIZE) {
-		new_epc_page = kzalloc(sizeof(*new_epc_page), GFP_KERNEL);
-		if (!new_epc_page)
-			goto err_freelist;
-		new_epc_page->pa = bank->start + i;
+	bank->epgtbl = alloc_pages_exact(sizeof(struct sgx_epc_page) * nrpages,
+			GFP_KERNEL);
+	if (!bank->epgtbl)
+		goto err_iounmap;
+
+	for (i = 0; i < nrpages; i++) {
+		struct sgx_epc_page *entry = bank->epgtbl + i;
 
 		spin_lock(&sgx_free_list_lock);
-		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
+		list_add_tail(&entry->free_list, &sgx_free_list);
 		sgx_nr_total_epc_pages++;
 		sgx_nr_free_pages++;
 		spin_unlock(&sgx_free_list_lock);
@@ -455,14 +455,7 @@  static int sgx_epc_bank_init(struct sgx_epc_bank *bank)
 
 	return 0;
 
-err_freelist:
-	list_for_each_safe(parser, temp, &sgx_free_list) {
-		spin_lock(&sgx_free_list_lock);
-		entry = list_entry(parser, struct sgx_epc_page, free_list);
-		list_del(&entry->free_list);
-		spin_unlock(&sgx_free_list_lock);
-		kfree(entry);
-	}
+err_iounmap:
 #ifdef CONFIG_X86_64
 	iounmap(bank->mem);
 #endif
@@ -475,9 +468,13 @@  static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank)
 		return;
 
 	/*
-	 * Don't free 'struct sgx_epc_page' for EPC page in this bank. All
-	 * 'struct sgx_epc_page' for all EPC pages will be destroyed together.
+	 * 'struct sgx_epc_page' for all EPC pages in this bank have already
+	 * been deleted from sgx_free_list. Just free the memory holding them.
 	 */
+	if (bank->epgtbl)
+		free_pages_exact(bank->epgtbl, sizeof (struct sgx_epc_page) *
+				((bank->end - bank->start) >> PAGE_SHIFT));
+
 #ifdef CONFIG_X86_64
 	if (bank->mem)
 		iounmap(bank->mem);
@@ -516,11 +513,15 @@  void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks)
 	if (ksgxswapd_tsk)
 		kthread_stop(ksgxswapd_tsk);
 
+	/*
+	 * To make sure, delete all 'struct sgx_epc_page' from sgx_free_list
+	 * before calling sgx_epc_bank_teardown where memory holding them is
+	 * freed.
+	 */
 	spin_lock(&sgx_free_list_lock);
 	list_for_each_safe(parser, temp, &sgx_free_list) {
 		entry = list_entry(parser, struct sgx_epc_page, free_list);
 		list_del(&entry->free_list);
-		kfree(entry);
 	}
 	spin_unlock(&sgx_free_list_lock);
 
diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c
index 234a5fb..6d36b98 100644
--- a/drivers/platform/x86/intel_sgx_util.c
+++ b/drivers/platform/x86/intel_sgx_util.c
@@ -63,22 +63,79 @@ 
 #include <linux/shmem_fs.h>
 #include <linux/sched/mm.h>
 
-void *sgx_get_epc_page(struct sgx_epc_page *entry)
+static struct sgx_epc_bank *sgx_epc_pfn_to_bank(unsigned long pfn)
 {
-#ifdef CONFIG_X86_32
-	return kmap_atomic_pfn(PFN_DOWN(entry->pa));
-#else
 	int i;
 
 	for (i = 0; i < sgx_nr_epc_banks; i++) {
-		if (entry->pa < sgx_epc_banks[i].end &&
-		    entry->pa >= sgx_epc_banks[i].start) {
-			return sgx_epc_banks[i].mem +
-				(entry->pa - sgx_epc_banks[i].start);
-		}
+		struct sgx_epc_bank *bank = sgx_epc_banks + i;
+		if (pfn >= (bank->start >> PAGE_SHIFT) &&
+				pfn < (bank->end >> PAGE_SHIFT))
+			return bank;
 	}
 
 	return NULL;
+}
+
+static struct sgx_epc_bank *sgx_epc_page_to_bank(struct sgx_epc_page *entry)
+{
+	int i;
+
+	for (i = 0; i < sgx_nr_epc_banks; i++) {
+		struct sgx_epc_bank *bank = sgx_epc_banks + i;
+		unsigned long idx = entry - bank->epgtbl;
+		if (idx < (bank->end - bank->start) >> PAGE_SHIFT)
+			return bank;
+	}
+
+	return NULL;
+}
+
+/**
+ * sgx_epc_pfn_to_page - get 'struct sgx_epc_page' given PFN
+ * @pfn:	physical frame number of EPC page
+ *
+ * Return: 'struct sgx_epc_page' for EPC page, or NULL if invalid PFN
+ */
+struct sgx_epc_page *sgx_epc_pfn_to_page(unsigned long pfn)
+{
+	struct sgx_epc_bank *bank = sgx_epc_pfn_to_bank(pfn);
+
+	if (!bank)
+		return NULL;
+
+	return bank->epgtbl + (pfn - (bank->start >> PAGE_SHIFT));
+}
+EXPORT_SYMBOL_GPL(sgx_epc_pfn_to_page);
+
+/**
+ * sgx_epc_page_to_pfn - get PFN given 'struct sgx_epc_page'
+ * @entry:	'struct sgx_epc_page' for particular EPC page
+ *
+ * Return: PFN of EPC page, or 0 if invalid 'struct sgx_epc_page'
+ */
+unsigned long sgx_epc_page_to_pfn(struct sgx_epc_page *entry)
+{
+	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
+
+	if (!bank)
+		return 0;
+
+	return (bank->start >> PAGE_SHIFT) + (entry - bank->epgtbl);
+}
+EXPORT_SYMBOL_GPL(sgx_epc_page_to_pfn);
+
+void *sgx_get_epc_page(struct sgx_epc_page *entry)
+{
+#ifdef CONFIG_X86_32
+	return kmap_atomic_pfn(sgx_epc_page_to_pfn(entry));
+#else
+	struct sgx_epc_bank *bank = sgx_epc_page_to_bank(entry);
+
+	if (!bank)
+		return NULL;
+
+	return bank->mem + ((entry - bank->epgtbl) << PAGE_SHIFT);
 #endif
 }
 
@@ -369,7 +426,7 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 	if (rc)
 		goto out;
 
-	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
+	rc = vm_insert_pfn(vma, entry->addr, sgx_epc_page_to_pfn(epc_page));
 	if (rc)
 		goto out;