On Thu, Aug 22, 2019 at 02:29:02AM +0300, Jarkko Sakkinen wrote:
> Using address resolution of any kind is obviously an overkill for
> anything that we know at compile time. Those sites should not hard
> bind how we store SECS.
>
> This commit adds @secs_child to sgx_encl_eldu() and sgx_encl_ewb()
> and replaces sgx_encl_get_index() with a macro SGX_ENCL_PAGE_INDEX()
Why use a macro instead of an inline function?
> The new macro assumes that it is operated on SECS children and it is a
> right call because it neither makes sense to bind large architectural
> decisions inside the smallest helpers (e.g. where and how we store SECS).
>
> Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 4 +--
> arch/x86/kernel/cpu/sgx/encl.c | 50 ++++++++++++--------------
> arch/x86/kernel/cpu/sgx/encl.h | 4 +--
> arch/x86/kernel/cpu/sgx/reclaim.c | 34 +++++++++++-------
> 4 files changed, 48 insertions(+), 44 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 9b784a061a47..fbf2aa9da5fc 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -82,7 +82,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
> {
> struct sgx_encl_page *encl_page = req->encl_page;
> struct sgx_encl *encl = req->encl;
> - unsigned long page_index = sgx_encl_get_index(encl_page);
> + unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> struct sgx_secinfo secinfo;
> struct sgx_pageinfo pginfo;
> struct page *backing;
> @@ -475,7 +475,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
> struct sgx_secinfo *secinfo,
> unsigned int mrmask)
> {
> - unsigned long page_index = sgx_encl_get_index(encl_page);
> + unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
> struct sgx_add_page_req *req = NULL;
> struct page *backing;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index d6397f7ef3b8..16d97198dafb 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -13,18 +13,26 @@
> #include "sgx.h"
>
> static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> - struct sgx_epc_page *epc_page)
> + struct sgx_epc_page *epc_page, bool secs_child)
secs_child isn't my first choice on names. I see "secs" and think "this
is an SECS page". I'd prefer is_secs, or maybe is_child?
> {
> unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page);
> struct sgx_encl *encl = encl_page->encl;
> - pgoff_t page_index = sgx_encl_get_index(encl_page);
> - pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
> - unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
> struct sgx_pageinfo pginfo;
> + unsigned long pcmd_offset;
> struct page *backing;
> + pgoff_t page_index;
> + pgoff_t pcmd_index;
> struct page *pcmd;
> int ret;
>
> + if (secs_child)
> + page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> + else
> + page_index = PFN_DOWN(encl->size);
> +
> + pcmd_index = sgx_pcmd_index(encl, page_index);
> + pcmd_offset = sgx_pcmd_offset(page_index);
> +
> backing = sgx_encl_get_backing_page(encl, page_index);
> if (IS_ERR(backing)) {
> ret = PTR_ERR(backing);
> @@ -40,8 +48,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
> pginfo.contents = (unsigned long)kmap_atomic(backing);
> pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset;
> - pginfo.secs = SGX_ENCL_PAGE_IS_SECS(encl_page) ? 0 :
> - (unsigned long)sgx_epc_addr(encl->secs.epc_page);
> +
> + if (secs_child)
> + pginfo.secs = (u64)sgx_epc_addr(encl->secs.epc_page);
> + else
> + pginfo.secs = 0;
Rather than pass a bool, we can pass 'struct sgx_epc_page *parent_secs'.
That'd avoid having to name a bool, and would clean up this type of code.
In general, I dislike boolean parameters where the caller uses true/false
as I can never remember the context of true/false.
>
> ret = __eldu(&pginfo, sgx_epc_addr(epc_page),
> sgx_epc_addr(encl_page->va_page->epc_page) + va_offset);
> @@ -64,7 +75,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> return ret;
> }
>
...
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -220,17 +220,26 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
> }
>
> static int __sgx_encl_ewb(struct sgx_encl *encl, struct sgx_epc_page *epc_page,
> - struct sgx_va_page *va_page, unsigned int va_offset)
> + struct sgx_va_page *va_page, unsigned int va_offset,
> + bool secs_child)
> {
> struct sgx_encl_page *encl_page = epc_page->owner;
> - pgoff_t page_index = sgx_encl_get_index(encl_page);
> - pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
> - unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
> struct sgx_pageinfo pginfo;
> + unsigned long pcmd_offset;
> struct page *backing;
> + pgoff_t page_index;
> + pgoff_t pcmd_index;
> struct page *pcmd;
> int ret;
>
> + if (secs_child)
> + page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> + else
> + page_index = PFN_DOWN(encl->size);
> +
> + pcmd_index = sgx_pcmd_index(encl, page_index);
> + pcmd_offset = sgx_pcmd_offset(page_index);
> +
> backing = sgx_encl_get_backing_page(encl, page_index);
> if (IS_ERR(backing)) {
> ret = PTR_ERR(backing);
> @@ -291,7 +300,7 @@ static const cpumask_t *sgx_encl_ewb_cpumask(struct sgx_encl *encl)
> return cpumask;
> }
>
> -static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> +static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool secs_child)
Same comments on the param as ELDU. The SECS pointer is needed for ETRACK,
so it's not completely extraneous.
> {
> struct sgx_encl_page *encl_page = epc_page->owner;
> struct sgx_encl *encl = encl_page->encl;
> @@ -308,7 +317,8 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> if (sgx_va_page_full(va_page))
> list_move_tail(&va_page->list, &encl->va_pages);
>
> - ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset);
> + ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset,
> + secs_child);
> if (ret == SGX_NOT_TRACKED) {
> ret = __etrack(sgx_epc_addr(encl->secs.epc_page));
> if (ret) {
> @@ -318,7 +328,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> }
>
> ret = __sgx_encl_ewb(encl, epc_page, va_page,
> - va_offset);
> + va_offset, secs_child);
> if (ret == SGX_NOT_TRACKED) {
> /*
> * Slow path, send IPIs to kick cpus out of the
> @@ -330,7 +340,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> on_each_cpu_mask(sgx_encl_ewb_cpumask(encl),
> sgx_ipi_cb, NULL, 1);
> ret = __sgx_encl_ewb(encl, epc_page, va_page,
> - va_offset);
> + va_offset, secs_child);
> }
> }
>
> @@ -340,12 +350,12 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
>
> encl_page->desc |= va_offset;
> encl_page->va_page = va_page;
> - } else if (!do_free) {
> + } else if (secs_child) {
> ret = __eremove(sgx_epc_addr(epc_page));
> WARN(ret, "EREMOVE returned %d\n", ret);
> }
>
> - if (do_free)
> + if (!secs_child)
> sgx_free_page(epc_page);
>
> encl_page->epc_page = NULL;
> @@ -358,11 +368,11 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page)
>
> mutex_lock(&encl->lock);
>
> - sgx_encl_ewb(epc_page, false);
> + sgx_encl_ewb(epc_page, true);
> encl->secs_child_cnt--;
> if (!encl->secs_child_cnt &&
> (encl->flags & (SGX_ENCL_DEAD | SGX_ENCL_INITIALIZED))) {
> - sgx_encl_ewb(encl->secs.epc_page, true);
> + sgx_encl_ewb(encl->secs.epc_page, false);
> }
>
> mutex_unlock(&encl->lock);
> --
> 2.20.1
>
