From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org
Subject: Re: [PATCH 2/2] x86/sgx: Determine SECS at compile time in sgx_encl_eldu()
Date: Wed, 21 Aug 2019 17:55:51 -0700 [thread overview]
Message-ID: <20190822005551.GP29345@linux.intel.com> (raw)
In-Reply-To: <20190821232902.29476-3-jarkko.sakkinen@linux.intel.com>
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@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> 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
>
next prev parent reply other threads:[~2019-08-22 0:55 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-21 23:29 [PATCH 0/2] v22 fixes Jarkko Sakkinen
2019-08-21 23:29 ` [PATCH 1/2] x86/sgx: Remove duplicate check for entry->epc_page in sgx_encl_load_page() Jarkko Sakkinen
2019-08-22 0:33 ` Sean Christopherson
2019-08-22 14:45 ` Jarkko Sakkinen
2019-08-21 23:29 ` [PATCH 2/2] x86/sgx: Determine SECS at compile time in sgx_encl_eldu() Jarkko Sakkinen
2019-08-22 0:55 ` Sean Christopherson [this message]
2019-08-22 14:55 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190822005551.GP29345@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=linux-sgx@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).