| Message ID | 20170404072938.4800-4-kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >
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
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;
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(-)