Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: "Xing, Cedric" <cedric.xing@intel.com>
To: "Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	Jethro Beekman <jethro@fortanix.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	"Dr . Greg Wettstein" <greg@enjellic.com>
Subject: RE: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
Date: Thu, 13 Jun 2019 19:45:23 +0000
Message-ID: <960B34DE67B9E140824F1DCDEC400C0F65503CD4@ORSMSX116.amr.corp.intel.com> (raw)
In-Reply-To: <20190613165130.GB5850@linux.intel.com>

> From: Christopherson, Sean J
> Sent: Thursday, June 13, 2019 9:52 AM
> 
> On Thu, Jun 13, 2019 at 12:43:42AM +0000, Jethro Beekman wrote:
> > On 2019-06-05 12:48, Sean Christopherson wrote:
> > >...to improve performance when building enclaves by reducing the
> > >number of user<->system transitions.  Rather than provide arbitrary
> > >batching, e.g. with per-page SECINFO and mrmask, take advantage of
> > >the fact that any sane enclave will have large swaths of pages with
> > >identical properties, e.g. code vs. data sections.
> > >
> > >For simplicity and stability in the initial implementation, loop over
> > >the existing add page flow instead of taking a more agressive
> > >approach, which would require tracking transitions between VMAs and
> > >holding mmap_sem for an extended duration.
> > >
> > >On an error, update the userspace struct to reflect progress made,
> e.g.
> > >so that the ioctl can be re-invoked to finish adding pages after a
> > >non- fatal error.
> > >
> > >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > >---
> > >  Documentation/x86/sgx/3.API.rst        |   2 +-
> > >  arch/x86/include/uapi/asm/sgx.h        |  21 ++--
> > >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 128
> > >+++++++++++++++++--------
> > >  3 files changed, 98 insertions(+), 53 deletions(-)
> > >
> > >diff --git a/Documentation/x86/sgx/3.API.rst
> > >b/Documentation/x86/sgx/3.API.rst index b113aeb05f54..44550aa41073
> > >100644
> > >--- a/Documentation/x86/sgx/3.API.rst
> > >+++ b/Documentation/x86/sgx/3.API.rst
> > >@@ -22,6 +22,6 @@ controls the `PROVISON_KEY` attribute.
> > >  .. kernel-doc:: arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > >     :functions: sgx_ioc_enclave_create
> > >-               sgx_ioc_enclave_add_page
> > >+               sgx_ioc_enclave_add_region
> > >                 sgx_ioc_enclave_init
> > >                 sgx_ioc_enclave_set_attribute diff --git
> > >a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > >index 9ed690a38c70..30d114f6b3bd 100644
> > >--- a/arch/x86/include/uapi/asm/sgx.h
> > >+++ b/arch/x86/include/uapi/asm/sgx.h
> > >@@ -12,8 +12,8 @@
> > >  #define SGX_IOC_ENCLAVE_CREATE \
> > >  	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create) -#define
> > >SGX_IOC_ENCLAVE_ADD_PAGE \
> > >-	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
> > >+#define SGX_IOC_ENCLAVE_ADD_REGION \
> > >+	_IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_region)
> > >  #define SGX_IOC_ENCLAVE_INIT \
> > >  	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> > >  #define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \ @@ -32,21 +32,22 @@ struct
> > >sgx_enclave_create  {
> > >  };
> > >  /**
> > >- * struct sgx_enclave_add_page - parameter structure for the
> > >- *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> > >- * @addr:	address within the ELRANGE
> > >- * @src:	address for the page data
> > >- * @secinfo:	address for the SECINFO data
> > >- * @mrmask:	bitmask for the measured 256 byte chunks
> > >+ * struct sgx_enclave_add_region - parameter structure for the
> > >+ *                                 %SGX_IOC_ENCLAVE_ADD_REGION ioctl
> > >+ * @addr:	start address within the ELRANGE
> > >+ * @src:	start address for the pages' data
> > >+ * @size:	size of region, in bytes
> > >+ * @secinfo:	address of the SECINFO data (common to the entire
> region)
> > >+ * @mrmask:	bitmask of 256 byte chunks to measure (applied per 4k
> page)
> > >   */
> > >-struct sgx_enclave_add_page {
> > >+struct sgx_enclave_add_region {
> > >  	__u64	addr;
> > >  	__u64	src;
> > >+	__u64	size;
> > >  	__u64	secinfo;
> > >  	__u16	mrmask;
> >
> > Considering:
> >
> > 1. I might want to load multiple pages that are not consecutive in
> memory.
> > 2. Repeating mrmask (other than 0 or ~0) doesn't really make sense for
> > ranges.
> >
> > I'd be in favor of an approach that passes an array of
> > sgx_enclave_add_page instead.
> 
> I'm not opposed to taking an array.  The region approach seemed simpler
> at first glance, but that may not be the case, especially if we get rid
> of the workqueue.  I'll play around with it.

In practice, ELF segments are usually mapped continuously in memory. Other components of an enclave such as heaps and stack, are usually trunks of zeroes. I'd just stay simple. After all, there's no requirement that everything has to be done in a single ioctl.

  parent reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
2019-06-05 19:48 ` [PATCH 1/7] x86/sgx: Remove dead code to handle non-existent IOR ioctl Sean Christopherson
2019-06-05 19:48 ` [PATCH 2/7] x86/sgx: Remove unnecessary @cmd parameter from ioctl helpers Sean Christopherson
2019-06-05 19:48 ` [PATCH 3/7] x86/sgx: Let ioctl helpers do copy to/from user Sean Christopherson
2019-06-05 19:48 ` [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl() Sean Christopherson
2019-06-06 15:47   ` Jarkko Sakkinen
2019-06-13  0:43   ` Jethro Beekman
2019-06-13 16:51     ` Sean Christopherson
2019-06-13 19:05       ` Andy Lutomirski
2019-06-13 19:15         ` Sean Christopherson
2019-06-13 19:45       ` Xing, Cedric [this message]
2019-06-05 19:48 ` [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source Sean Christopherson
2019-06-06 17:20   ` Andy Lutomirski
2019-06-06 17:32     ` Sean Christopherson
2019-06-07 19:32       ` Andy Lutomirski
2019-06-10 17:06         ` Jarkko Sakkinen
2019-06-10 18:09         ` Xing, Cedric
2019-06-10 18:41           ` Sean Christopherson
2019-06-10 18:53         ` Sean Christopherson
2019-06-13  0:38           ` Jethro Beekman
2019-06-13 13:46             ` Sean Christopherson
2019-06-13 16:16               ` Andy Lutomirski
2019-06-13 16:54                 ` Sean Christopherson
2019-06-05 19:48 ` [PATCH 6/7] x86/sgx: Use the actual zero page as the source when adding zero pages Sean Christopherson
2019-06-05 19:48 ` [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed' Sean Christopherson
2019-06-05 19:59   ` Dave Hansen
2019-06-05 20:00     ` Andy Lutomirski
2019-06-12 15:14   ` Jarkko Sakkinen
2019-06-12 15:23     ` Sean Christopherson
2019-06-13  0:44       ` Jethro Beekman
2019-06-13 15:38       ` Jarkko Sakkinen
2019-06-12 15:16 ` [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Jarkko Sakkinen
2019-06-12 18:14   ` Jarkko Sakkinen

Reply instructions:

You may reply publically 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=960B34DE67B9E140824F1DCDEC400C0F65503CD4@ORSMSX116.amr.corp.intel.com \
    --to=cedric.xing@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=greg@enjellic.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=sean.j.christopherson@intel.com \
    /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

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/ public-inbox