linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl
@ 2019-06-05 19:48 Sean Christopherson
  2019-06-05 19:48 ` [PATCH 1/7] x86/sgx: Remove dead code to handle non-existent IOR ioctl Sean Christopherson
                   ` (7 more replies)
  0 siblings, 8 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

This series is intended to be included in v21 of Jarkko's SGX series and
applies on Jarkko's current master:

  dfc89a83b5bc ("docs: x86/sgx: Document the enclave API")

The primary goal of the series is to tweak the ioctl for adding pages to
an enclave so that it is somewhat extensible, e.g. add a flags field that
can be reused for access control integration and SGX2/EAUG, and add a size
field so that multiple pages can be added in a single call (batching EADD
has been mentioned at various times in the past).

The secondary goal is to improve the performance of building enclaves.
Handling multiple pages in a single call helps somewhat, but the real win
(for some enclaves) is using the kernel's zero page as the source for EADD
when possible.

Sean Christopherson (7):
  x86/sgx: Remove dead code to handle non-existent IOR ioctl
  x86/sgx: Remove unnecessary @cmd parameter from ioctl helpers
  x86/sgx: Let ioctl helpers do copy to/from user
  x86/sgx: Allow userspace to add multiple pages in single ioctl()
  x86/sgx: Add flag to zero added region instead of copying from source
  x86/sgx: Use the actual zero page as the source when adding zero pages
  x86/sgx: Add a reserved field to sgx_enclave_add_region to drop
    'packed'

 Documentation/x86/sgx/3.API.rst        |   2 +-
 arch/x86/include/uapi/asm/sgx.h        |  30 +--
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 252 +++++++++++++++----------
 3 files changed, 171 insertions(+), 113 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/7] x86/sgx: Remove dead code to handle non-existent IOR ioctl
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
@ 2019-06-05 19:48 ` Sean Christopherson
  2019-06-05 19:48 ` [PATCH 2/7] x86/sgx: Remove unnecessary @cmd parameter from ioctl helpers Sean Christopherson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

There are currently no SGX ioctls that require writing data back to
userspace.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index a27ec26a9350..cc057d487499 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -839,11 +839,5 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	if (copy_from_user(data, (void __user *)arg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	ret = handler(filep, cmd, (unsigned long)((void *)data));
-	if (!ret && (cmd & IOC_OUT)) {
-		if (copy_to_user((void __user *)arg, data, _IOC_SIZE(cmd)))
-			return -EFAULT;
-	}
-
-	return ret;
+	return handler(filep, cmd, (unsigned long)((void *)data));
 }
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 2/7] x86/sgx: Remove unnecessary @cmd parameter from ioctl helpers
  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 ` Sean Christopherson
  2019-06-05 19:48 ` [PATCH 3/7] x86/sgx: Let ioctl helpers do copy to/from user Sean Christopherson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

Each ioctl command is directly and uniquely associated with a handler,
i.e. the command is implied by the function itself.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index cc057d487499..a0742d18aa36 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -383,7 +383,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 /**
  * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
  * @filep:	open file to /dev/sgx
- * @cmd:	the command value
  * @arg:	pointer to an &sgx_enclave_create instance
  *
  * Allocate kernel data structures for a new enclave and execute ECREATE after
@@ -397,8 +396,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd,
-				   unsigned long arg)
+static long sgx_ioc_enclave_create(struct file *filep, unsigned long arg)
 {
 	struct sgx_enclave_create *createp = (struct sgx_enclave_create *)arg;
 	struct sgx_encl *encl = filep->private_data;
@@ -579,7 +577,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
  *
  * @filep:	open file to /dev/sgx
- * @cmd:	the command value
  * @arg:	pointer to an &sgx_enclave_add_page instance
  *
  * Add a page to an uninitialized enclave (EADD), and optionally extend the
@@ -593,8 +590,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
-				     unsigned long arg)
+static long sgx_ioc_enclave_add_page(struct file *filep, unsigned long arg)
 {
 	struct sgx_enclave_add_page *addp = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
@@ -721,7 +717,6 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
  *
  * @filep:	open file to /dev/sgx
- * @cmd:	the command value
  * @arg:	pointer to an &sgx_enclave_init instance
  *
  * Flush any outstanding enqueued EADD operations and perform EINIT.  The
@@ -733,8 +728,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  *   SGX error code on EINIT failure,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
-				 unsigned long arg)
+static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
 {
 	struct sgx_enclave_init *initp = (struct sgx_enclave_init *)arg;
 	struct sgx_encl *encl = filep->private_data;
@@ -770,7 +764,6 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
 /**
  * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
  * @filep:	open file to /dev/sgx
- * @cmd:	the command value
  * @arg:	pointer to a struct sgx_enclave_set_attribute instance
  *
  * Mark the enclave as being allowed to access a restricted attribute bit.
@@ -786,8 +779,7 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned int cmd,
  *
  * Return: 0 on success, -errno otherwise
  */
-static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
-					  unsigned long arg)
+static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned long arg)
 {
 	struct sgx_enclave_set_attribute *params = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
@@ -810,8 +802,7 @@ static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned int cmd,
 	return ret;
 }
 
-typedef long (*sgx_ioc_t)(struct file *filep, unsigned int cmd,
-			  unsigned long arg);
+typedef long (*sgx_ioc_t)(struct file *filep, unsigned long arg);
 
 long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
@@ -839,5 +830,5 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	if (copy_from_user(data, (void __user *)arg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	return handler(filep, cmd, (unsigned long)((void *)data));
+	return handler(filep, (unsigned long)((void *)data));
 }
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 3/7] x86/sgx: Let ioctl helpers do copy to/from user
  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 ` Sean Christopherson
  2019-06-05 19:48 ` [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl() Sean Christopherson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

A future patch will extend SGX_IOC_ENCLAVE_ADD_PAGE to allow userspace
to add multiple pages in a single ioctl.  Because the number of pages
could be quite high, it's possible the ioctl could be interrupted, in
which case the driver will need to "return" the number of pages that
have been queued, e.g. so that userspace can restart the ioctl in the
event that the signal wasn't fatal.

In short, SGX_IOC_ENCLAVE_ADD_PAGE will become a conditional _IOWR
and will only do copy_to_user() on a non-zero return value.  Rather than
implement funky logic, pass the raw userspace argument to the helpers
so that they may do copy_{to,from}_user() at will.  This kills
pre-copying the data, but the value added by pre-copy is tenuous, e.g.
it consolidates two lines of code but requires use of a fixed size
buffer as well as an indirect handler and thus retpoline.  And the
pre-copy may lead to reader confusion due to @arg being a user pointer
in some flows and a kernel pointer in others.  Lastly, in the event that
a pure _IOR ioctl is added, there won't be a need to update the existing
code (to skip copy_from_user()).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 73 ++++++++++++--------------
 1 file changed, 34 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index a0742d18aa36..d17c60dca114 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -383,7 +383,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 /**
  * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_create instance
+ * @arg:	userspace pointer to a struct sgx_enclave_create instance
  *
  * Allocate kernel data structures for a new enclave and execute ECREATE after
  * verifying the correctness of the provided SECS.
@@ -396,25 +396,27 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_create(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_create(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_create *createp = (struct sgx_enclave_create *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_create ecreate;
 	struct page *secs_page;
 	struct sgx_secs *secs;
 	int ret;
 
+	if (copy_from_user(&ecreate, arg, sizeof(ecreate)))
+		return -EFAULT;
+
 	secs_page = alloc_page(GFP_HIGHUSER);
 	if (!secs_page)
 		return -ENOMEM;
 
 	secs = kmap(secs_page);
-	if (copy_from_user(secs, (void __user *)createp->src, sizeof(*secs))) {
+	if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-
 	ret = sgx_encl_create(encl, secs);
 
 out:
@@ -577,7 +579,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
  *
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_add_page instance
+ * @arg:	userspace pointer to a struct sgx_enclave_add_page instance
  *
  * Add a page to an uninitialized enclave (EADD), and optionally extend the
  * enclave's measurement with the contents of the page (EEXTEND).  EADD and
@@ -590,16 +592,19 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
  *   0 on success,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_add_page(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_add_page *addp = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_add_page addp;
 	struct sgx_secinfo secinfo;
 	struct page *data_page;
 	void *data;
 	int ret;
 
-	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
+	if (copy_from_user(&addp, arg, sizeof(addp)))
+		return -EFAULT;
+
+	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
 			   sizeof(secinfo)))
 		return -EFAULT;
 
@@ -609,12 +614,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned long arg)
 
 	data = kmap(data_page);
 
-	if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) {
+	if (copy_from_user((void *)data, (void __user *)addp.src, PAGE_SIZE)) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-	ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask);
+	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask);
 	if (ret)
 		goto out;
 
@@ -717,7 +722,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
  *
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to an &sgx_enclave_init instance
+ * @arg:	userspace pointer to a struct sgx_enclave_init instance
  *
  * Flush any outstanding enqueued EADD operations and perform EINIT.  The
  * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
@@ -728,15 +733,18 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
  *   SGX error code on EINIT failure,
  *   -errno otherwise
  */
-static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_init(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_init *initp = (struct sgx_enclave_init *)arg;
 	struct sgx_encl *encl = filep->private_data;
 	struct sgx_einittoken *einittoken;
 	struct sgx_sigstruct *sigstruct;
+	struct sgx_enclave_init einit;
 	struct page *initp_page;
 	int ret;
 
+	if (copy_from_user(&einit, arg, sizeof(einit)))
+		return -EFAULT;
+
 	initp_page = alloc_page(GFP_HIGHUSER);
 	if (!initp_page)
 		return -ENOMEM;
@@ -746,13 +754,12 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
 		((unsigned long)sigstruct + PAGE_SIZE / 2);
 	memset(einittoken, 0, sizeof(*einittoken));
 
-	if (copy_from_user(sigstruct, (void __user *)initp->sigstruct,
+	if (copy_from_user(sigstruct, (void __user *)einit.sigstruct,
 			   sizeof(*sigstruct))) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-
 	ret = sgx_encl_init(encl, sigstruct, einittoken);
 
 out:
@@ -764,7 +771,7 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
 /**
  * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
  * @filep:	open file to /dev/sgx
- * @arg:	pointer to a struct sgx_enclave_set_attribute instance
+ * @arg:	userspace pointer to a struct sgx_enclave_set_attribute instance
  *
  * Mark the enclave as being allowed to access a restricted attribute bit.
  * The requested attribute is specified via the attribute_fd field in the
@@ -779,14 +786,17 @@ static long sgx_ioc_enclave_init(struct file *filep, unsigned long arg)
  *
  * Return: 0 on success, -errno otherwise
  */
-static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned long arg)
+static long sgx_ioc_enclave_set_attribute(struct file *filep, void __user *arg)
 {
-	struct sgx_enclave_set_attribute *params = (void *)arg;
 	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_set_attribute params;
 	struct file *attribute_file;
 	int ret;
 
-	attribute_file = fget(params->attribute_fd);
+	if (copy_from_user(&params, arg, sizeof(params)))
+		return -EFAULT;
+
+	attribute_file = fget(params.attribute_fd);
 	if (!attribute_file->f_op)
 		return -EINVAL;
 
@@ -802,33 +812,18 @@ static long sgx_ioc_enclave_set_attribute(struct file *filep, unsigned long arg)
 	return ret;
 }
 
-typedef long (*sgx_ioc_t)(struct file *filep, unsigned long arg);
-
 long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
-	char data[256];
-	sgx_ioc_t handler = NULL;
-	long ret;
-
 	switch (cmd) {
 	case SGX_IOC_ENCLAVE_CREATE:
-		handler = sgx_ioc_enclave_create;
-		break;
+		return sgx_ioc_enclave_create(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_ADD_PAGE:
-		handler = sgx_ioc_enclave_add_page;
-		break;
+		return sgx_ioc_enclave_add_page(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_INIT:
-		handler = sgx_ioc_enclave_init;
-		break;
+		return sgx_ioc_enclave_init(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
-		handler = sgx_ioc_enclave_set_attribute;
-		break;
+		return sgx_ioc_enclave_set_attribute(filep, (void __user *)arg);
 	default:
 		return -ENOIOCTLCMD;
 	}
-
-	if (copy_from_user(data, (void __user *)arg, _IOC_SIZE(cmd)))
-		return -EFAULT;
-
-	return handler(filep, (unsigned long)((void *)data));
 }
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
                   ` (2 preceding siblings ...)
  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 ` Sean Christopherson
  2019-06-06 15:47   ` Jarkko Sakkinen
  2019-06-13  0:43   ` Jethro Beekman
  2019-06-05 19:48 ` [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source Sean Christopherson
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

...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;
 } __attribute__((__packed__));
 
-
 /**
  * struct sgx_enclave_init - parameter structure for the
  *                           %SGX_IOC_ENCLAVE_INIT ioctl
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index d17c60dca114..b69350696b87 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -487,10 +487,9 @@ static int sgx_validate_tcs(struct sgx_encl *encl, struct sgx_tcs *tcs)
 	return 0;
 }
 
-static int __sgx_encl_add_page(struct sgx_encl *encl,
+static int sgx_encl_queue_page(struct sgx_encl *encl,
 			       struct sgx_encl_page *encl_page,
-			       void *data,
-			       struct sgx_secinfo *secinfo,
+			       void *data, struct sgx_secinfo *secinfo,
 			       unsigned int mrmask)
 {
 	unsigned long page_index = sgx_encl_get_index(encl, encl_page);
@@ -529,9 +528,9 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 	return 0;
 }
 
-static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
-			     void *data, struct sgx_secinfo *secinfo,
-			     unsigned int mrmask)
+static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
+			       void *data, struct sgx_secinfo *secinfo,
+			       unsigned int mrmask)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
@@ -563,7 +562,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto out;
 	}
 
-	ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask);
+	ret = sgx_encl_queue_page(encl, encl_page, data, secinfo, mrmask);
 	if (ret) {
 		radix_tree_delete(&encl_page->encl->page_tree,
 				  PFN_DOWN(encl_page->desc));
@@ -575,57 +574,102 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	return ret;
 }
 
-/**
- * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
- *
- * @filep:	open file to /dev/sgx
- * @arg:	userspace pointer to a struct sgx_enclave_add_page instance
- *
- * Add a page to an uninitialized enclave (EADD), and optionally extend the
- * enclave's measurement with the contents of the page (EEXTEND).  EADD and
- * EEXTEND are done asynchronously via worker threads.  A successful
- * sgx_ioc_enclave_add_page() only indicates the page has been added to the
- * work queue, it does not guarantee adding the page to the enclave will
- * succeed.
- *
- * Return:
- *   0 on success,
- *   -errno otherwise
- */
-static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
+static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
+			     unsigned long src, struct sgx_secinfo *secinfo,
+			     unsigned int mrmask)
 {
-	struct sgx_encl *encl = filep->private_data;
-	struct sgx_enclave_add_page addp;
-	struct sgx_secinfo secinfo;
 	struct page *data_page;
 	void *data;
 	int ret;
 
-	if (copy_from_user(&addp, arg, sizeof(addp)))
-		return -EFAULT;
-
-	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
-			   sizeof(secinfo)))
-		return -EFAULT;
-
 	data_page = alloc_page(GFP_HIGHUSER);
 	if (!data_page)
 		return -ENOMEM;
 
 	data = kmap(data_page);
 
-	if (copy_from_user((void *)data, (void __user *)addp.src, PAGE_SIZE)) {
+	if (copy_from_user((void *)data, (void __user *)src, PAGE_SIZE)) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask);
-	if (ret)
-		goto out;
-
+	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
 out:
 	kunmap(data_page);
 	__free_page(data_page);
+
+	return ret;
+}
+
+/**
+ * sgx_ioc_enclave_add_region - handler for %SGX_IOC_ENCLAVE_ADD_REGION
+ *
+ * @filep:	open file to /dev/sgx
+ * @arg:	userspace pointer to a struct sgx_enclave_add_region instance
+ *
+ * Add a range of pages to an uninitialized enclave (EADD), and optionally
+ * extend the enclave's measurement with the contents of the page (EEXTEND).
+ * The range of pages must be virtually contiguous.  The SECINFO and
+ * measurement maskare applied to all pages, i.e. pages with different
+ * properties must be added in separate calls.
+ *
+ * EADD and EEXTEND are done asynchronously via worker threads.  A successful
+ * sgx_ioc_enclave_add_region() only indicates the pages have been added to the
+ * work queue, it does not guarantee adding the pages to the enclave will
+ * succeed.
+ *
+ * On failure, @arg->addr, @arg->src and @arg->size are adjusted to reflect
+ * the remaining pages that need to be added to the enclave, i.e. userspace
+ * can re-invoke SGX_IOC_ENCLAVE_ADD_REGION using the same struct.
+ *
+ * Return:
+ *   0 on success,
+ *   -errno otherwise
+ */
+static long sgx_ioc_enclave_add_region(struct file *filep, void __user *arg)
+{
+	struct sgx_encl *encl = filep->private_data;
+	struct sgx_enclave_add_region region;
+	struct sgx_secinfo secinfo;
+	unsigned long i;
+	int ret;
+
+	if (copy_from_user(&region, arg, sizeof(region)))
+		return -EFAULT;
+
+	if (!IS_ALIGNED(region.addr, 4096) || !IS_ALIGNED(region.size, 4096))
+		return -EINVAL;
+
+	if (!region.size)
+		return 0;
+
+	if (copy_from_user(&secinfo, (void __user *)region.secinfo,
+			   sizeof(secinfo)))
+		return -EFAULT;
+
+	ret = 0;
+	for (i = 0; i < region.size; i += PAGE_SIZE) {
+		if (signal_pending(current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
+
+		if (need_resched())
+			cond_resched();
+
+		ret = sgx_encl_add_page(encl, region.addr + i, region.src + i,
+					&secinfo, region.mrmask);
+		if (ret)
+			break;
+	}
+	if (ret && i) {
+		region.addr += i;
+		region.src  += i;
+		region.size -= i;
+
+		if (copy_to_user(arg, &region, sizeof(region)))
+			return -EFAULT;
+	}
 	return ret;
 }
 
@@ -817,8 +861,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	switch (cmd) {
 	case SGX_IOC_ENCLAVE_CREATE:
 		return sgx_ioc_enclave_create(filep, (void __user *)arg);
-	case SGX_IOC_ENCLAVE_ADD_PAGE:
-		return sgx_ioc_enclave_add_page(filep, (void __user *)arg);
+	case SGX_IOC_ENCLAVE_ADD_REGION:
+		return sgx_ioc_enclave_add_region(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_INIT:
 		return sgx_ioc_enclave_init(filep, (void __user *)arg);
 	case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
                   ` (3 preceding siblings ...)
  2019-06-05 19:48 ` [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl() Sean Christopherson
@ 2019-06-05 19:48 ` Sean Christopherson
  2019-06-06 17:20   ` Andy Lutomirski
  2019-06-05 19:48 ` [PATCH 6/7] x86/sgx: Use the actual zero page as the source when adding zero pages Sean Christopherson
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

For some enclaves, e.g. an enclave with a small code footprint and a
large working set, the vast majority of pages added to the enclave are
zero pages.  Introduce a flag to denote such zero pages.  The major
benefit of the flag will be realized in a future patch to use Linux's
actual zero page as the source, as opposed to explicitly zeroing the
enclave's backing memory.

Use bit 8 for the SGX_ZERO_REGION flag to avoid an anticipated conflict
with passing PROT_{READ,WRITE,EXEC} in bits 2:0, and to leave room in
case there is a need for additional protection related bits.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        |  5 +++++
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 15 ++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 30d114f6b3bd..18204722f238 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -31,6 +31,9 @@ struct sgx_enclave_create  {
 	__u64	src;
 };
 
+/* Zero an added region instead of copying data from a source page. */
+#define SGX_ZERO_REGION	0x100
+
 /**
  * struct sgx_enclave_add_region - parameter structure for the
  *                                 %SGX_IOC_ENCLAVE_ADD_REGION ioctl
@@ -38,6 +41,7 @@ struct sgx_enclave_create  {
  * @src:	start address for the pages' data
  * @size:	size of region, in bytes
  * @secinfo:	address of the SECINFO data (common to the entire region)
+ * @flags:	miscellaneous flags
  * @mrmask:	bitmask of 256 byte chunks to measure (applied per 4k page)
  */
 struct sgx_enclave_add_region {
@@ -45,6 +49,7 @@ struct sgx_enclave_add_region {
 	__u64	src;
 	__u64	size;
 	__u64	secinfo;
+	__u32	flags;
 	__u16	mrmask;
 } __attribute__((__packed__));
 
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index b69350696b87..c35264ea0c93 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -459,6 +459,9 @@ static int sgx_validate_tcs(struct sgx_encl *encl, struct sgx_tcs *tcs)
 {
 	int i;
 
+	if (!tcs)
+		return -EINVAL;
+
 	if (tcs->flags & SGX_TCS_RESERVED_MASK)
 		return -EINVAL;
 
@@ -510,7 +513,10 @@ static int sgx_encl_queue_page(struct sgx_encl *encl,
 	}
 
 	backing_ptr = kmap(backing);
-	memcpy(backing_ptr, data, PAGE_SIZE);
+	if (data)
+		memcpy(backing_ptr, data, PAGE_SIZE);
+	else
+		memset(backing_ptr, 0, PAGE_SIZE);
 	kunmap(backing);
 	if (page_type == SGX_SECINFO_TCS)
 		encl_page->desc |= SGX_ENCL_PAGE_TCS;
@@ -576,12 +582,15 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			     unsigned long src, struct sgx_secinfo *secinfo,
-			     unsigned int mrmask)
+			     unsigned int mrmask, unsigned long flags)
 {
 	struct page *data_page;
 	void *data;
 	int ret;
 
+	if (flags & SGX_ZERO_REGION)
+		return __sgx_encl_add_page(encl, addr, NULL, secinfo, mrmask);
+
 	data_page = alloc_page(GFP_HIGHUSER);
 	if (!data_page)
 		return -ENOMEM;
@@ -658,7 +667,7 @@ static long sgx_ioc_enclave_add_region(struct file *filep, void __user *arg)
 			cond_resched();
 
 		ret = sgx_encl_add_page(encl, region.addr + i, region.src + i,
-					&secinfo, region.mrmask);
+					&secinfo, region.mrmask, region.flags);
 		if (ret)
 			break;
 	}
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 6/7] x86/sgx: Use the actual zero page as the source when adding zero pages
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
                   ` (4 preceding siblings ...)
  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-05 19:48 ` 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-12 15:16 ` [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Jarkko Sakkinen
  7 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

Using the zero page avoids dirtying the backing page, inserting TLB
entries, the cost of memset, etc...  For some enclaves, e.g. an enclave
with a small code footprint and a large working set, this results in a
20%+ reduction in enclave build time.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 50 ++++++++++++++++----------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index c35264ea0c93..e05a539e96fc 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -19,6 +19,7 @@ struct sgx_add_page_req {
 	struct sgx_secinfo secinfo;
 	unsigned long mrmask;
 	struct list_head list;
+	bool zero_page;
 };
 
 static int sgx_encl_grow(struct sgx_encl *encl)
@@ -76,6 +77,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
 	struct sgx_pageinfo pginfo;
 	struct page *backing;
 	unsigned long addr;
+	void *contents;
 	int ret;
 	int i;
 
@@ -84,9 +86,15 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
 
 	addr = SGX_ENCL_PAGE_ADDR(encl_page);
 
-	backing = sgx_encl_get_backing_page(encl, page_index);
-	if (IS_ERR(backing))
-		return false;
+	if (!req->zero_page) {
+		backing = sgx_encl_get_backing_page(encl, page_index);
+		if (IS_ERR(backing))
+			return false;
+		contents = kmap_atomic(backing);
+	} else {
+		backing = NULL;
+		contents = __va(page_to_pfn(ZERO_PAGE(0)) << PAGE_SHIFT);
+	}
 
 	/*
 	 * The SECINFO field must be 64-byte aligned, copy it to a local
@@ -99,11 +107,13 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
 	pginfo.secs = (unsigned long)sgx_epc_addr(encl->secs.epc_page);
 	pginfo.addr = addr;
 	pginfo.metadata = (unsigned long)&secinfo;
-	pginfo.contents = (unsigned long)kmap_atomic(backing);
+	pginfo.contents = (unsigned long)contents;
 	ret = __eadd(&pginfo, sgx_epc_addr(epc_page));
-	kunmap_atomic((void *)(unsigned long)pginfo.contents);
 
-	put_page(backing);
+	if (backing) {
+		kunmap_atomic(contents);
+		put_page(backing);
+	}
 
 	if (ret) {
 		if (encls_failed(ret))
@@ -506,18 +516,20 @@ static int sgx_encl_queue_page(struct sgx_encl *encl,
 	if (!req)
 		return -ENOMEM;
 
-	backing = sgx_encl_get_backing_page(encl, page_index);
-	if (IS_ERR(backing)) {
-		kfree(req);
-		return PTR_ERR(backing);
-	}
+	if (data) {
+		backing = sgx_encl_get_backing_page(encl, page_index);
+		if (IS_ERR(backing)) {
+			kfree(req);
+			return PTR_ERR(backing);
+		}
 
-	backing_ptr = kmap(backing);
-	if (data)
+		backing_ptr = kmap(backing);
 		memcpy(backing_ptr, data, PAGE_SIZE);
-	else
-		memset(backing_ptr, 0, PAGE_SIZE);
-	kunmap(backing);
+		kunmap(backing);
+	} else {
+		backing = NULL;
+		req->zero_page = true;
+	}
 	if (page_type == SGX_SECINFO_TCS)
 		encl_page->desc |= SGX_ENCL_PAGE_TCS;
 	memcpy(&req->secinfo, secinfo, sizeof(*secinfo));
@@ -529,8 +541,10 @@ static int sgx_encl_queue_page(struct sgx_encl *encl,
 	list_add_tail(&req->list, &encl->add_page_reqs);
 	if (empty)
 		queue_work(sgx_encl_wq, &encl->work);
-	set_page_dirty(backing);
-	put_page(backing);
+	if (backing) {
+		set_page_dirty(backing);
+		put_page(backing);
+	}
 	return 0;
 }
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
                   ` (5 preceding siblings ...)
  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 ` Sean Christopherson
  2019-06-05 19:59   ` Dave Hansen
  2019-06-12 15:14   ` Jarkko Sakkinen
  2019-06-12 15:16 ` [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Jarkko Sakkinen
  7 siblings, 2 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-05 19:48 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

A reserved field could prove useful in the future, and packed structs
can result in inefficiencies due to its impact on alignment.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        | 4 +++-
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 18204722f238..e2d9b3d512ef 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -43,6 +43,7 @@ struct sgx_enclave_create  {
  * @secinfo:	address of the SECINFO data (common to the entire region)
  * @flags:	miscellaneous flags
  * @mrmask:	bitmask of 256 byte chunks to measure (applied per 4k page)
+ * @reserved:	reserved for future use, must be zero
  */
 struct sgx_enclave_add_region {
 	__u64	addr;
@@ -51,7 +52,8 @@ struct sgx_enclave_add_region {
 	__u64	secinfo;
 	__u32	flags;
 	__u16	mrmask;
-} __attribute__((__packed__));
+	__u16	reserved;
+};
 
 /**
  * struct sgx_enclave_init - parameter structure for the
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index e05a539e96fc..bae5f3155376 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -663,6 +663,9 @@ static long sgx_ioc_enclave_add_region(struct file *filep, void __user *arg)
 	if (!IS_ALIGNED(region.addr, 4096) || !IS_ALIGNED(region.size, 4096))
 		return -EINVAL;
 
+	if (region.reserved)
+		return -EINVAL;
+
 	if (!region.size)
 		return 0;
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  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
  1 sibling, 1 reply; 33+ messages in thread
From: Dave Hansen @ 2019-06-05 19:59 UTC (permalink / raw)
  To: Sean Christopherson, Jarkko Sakkinen
  Cc: linux-sgx, Cedric Xing, Andy Lutomirski, Jethro Beekman,
	Dr . Greg Wettstein

On 6/5/19 12:48 PM, Sean Christopherson wrote:
>  struct sgx_enclave_add_region {
>  	__u64	addr;
> @@ -51,7 +52,8 @@ struct sgx_enclave_add_region {
>  	__u64	secinfo;
>  	__u32	flags;
>  	__u16	mrmask;
> -} __attribute__((__packed__));
> +	__u16	reserved;
> +};

Without __packed__ do we have strong enough guarantees from the compiler
for a stable ABI?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  2019-06-05 19:59   ` Dave Hansen
@ 2019-06-05 20:00     ` Andy Lutomirski
  0 siblings, 0 replies; 33+ messages in thread
From: Andy Lutomirski @ 2019-06-05 20:00 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Sean Christopherson, Jarkko Sakkinen, linux-sgx, Cedric Xing,
	Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 5, 2019 at 12:59 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/5/19 12:48 PM, Sean Christopherson wrote:
> >  struct sgx_enclave_add_region {
> >       __u64   addr;
> > @@ -51,7 +52,8 @@ struct sgx_enclave_add_region {
> >       __u64   secinfo;
> >       __u32   flags;
> >       __u16   mrmask;
> > -} __attribute__((__packed__));
> > +     __u16   reserved;
> > +};
>
> Without __packed__ do we have strong enough guarantees from the compiler
> for a stable ABI?

We rely on this kind of thing for uapi on a regular basis, so I sure hope so.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  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
  1 sibling, 0 replies; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-06 15:47 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 05, 2019 at 12:48:42PM -0700, 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>

Probably not going to look at this before other things are settled.

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  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
  0 siblings, 1 reply; 33+ messages in thread
From: Andy Lutomirski @ 2019-06-06 17:20 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Jarkko Sakkinen, linux-sgx, Dave Hansen, Cedric Xing,
	Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> For some enclaves, e.g. an enclave with a small code footprint and a
> large working set, the vast majority of pages added to the enclave are
> zero pages.  Introduce a flag to denote such zero pages.  The major
> benefit of the flag will be realized in a future patch to use Linux's
> actual zero page as the source, as opposed to explicitly zeroing the
> enclave's backing memory.
>

I feel like I probably asked this at some point, but why is there a
workqueue here at all?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-06 17:20   ` Andy Lutomirski
@ 2019-06-06 17:32     ` Sean Christopherson
  2019-06-07 19:32       ` Andy Lutomirski
  0 siblings, 1 reply; 33+ messages in thread
From: Sean Christopherson @ 2019-06-06 17:32 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jarkko Sakkinen, linux-sgx, Dave Hansen, Cedric Xing,
	Jethro Beekman, Dr . Greg Wettstein

On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > For some enclaves, e.g. an enclave with a small code footprint and a
> > large working set, the vast majority of pages added to the enclave are
> > zero pages.  Introduce a flag to denote such zero pages.  The major
> > benefit of the flag will be realized in a future patch to use Linux's
> > actual zero page as the source, as opposed to explicitly zeroing the
> > enclave's backing memory.
> >
> 
> I feel like I probably asked this at some point, but why is there a
> workqueue here at all?

Performance.  A while back I wrote a patch set to remove the worker queue
and discovered that it tanks enclave build time when the enclave is being
hosted by a Golang application.  Here's a snippet from a mail discussing
the code.

    The bad news is that I don't think we can remove the add page worker
    as applications with userspace schedulers, e.g. Go's M:N scheduler,
    can see a 10x or more throughput improvement when using the worker
    queue.  I did a bit of digging for the Golang case to make sure I
    wasn't doing something horribly stupid/naive and found that it's a
    generic issue in Golang with blocking (or just long-running) system
    calls.  Because Golang multiplexes Goroutines on top of OS threads,
    blocking syscalls introduce latency and context switching overhead,
    e.g. Go's scheduler will spin up a new OS thread to service other
    Goroutines after it realizes the syscall has blocked, and will later
    destroy one of the OS threads so that it doesn't build up too many
    unused.

IIRC, the scenario is spinning up several goroutines, each building an
enclave.  I played around with adding a flag to do a synchronous EADD
but didn't see a meaningful change in performance for the simple case.
Supporting both the worker queue and direct paths was complex enough
that I decided it wasn't worth the trouble for initial upstreaming.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-06 17:32     ` Sean Christopherson
@ 2019-06-07 19:32       ` Andy Lutomirski
  2019-06-10 17:06         ` Jarkko Sakkinen
                           ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Andy Lutomirski @ 2019-06-07 19:32 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Andy Lutomirski, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Jethro Beekman, Dr . Greg Wettstein



> On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
>> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
>> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
>> <sean.j.christopherson@intel.com> wrote:
>>> 
>>> For some enclaves, e.g. an enclave with a small code footprint and a
>>> large working set, the vast majority of pages added to the enclave are
>>> zero pages.  Introduce a flag to denote such zero pages.  The major
>>> benefit of the flag will be realized in a future patch to use Linux's
>>> actual zero page as the source, as opposed to explicitly zeroing the
>>> enclave's backing memory.
>>> 
>> 
>> I feel like I probably asked this at some point, but why is there a
>> workqueue here at all?
> 
> Performance.  A while back I wrote a patch set to remove the worker queue
> and discovered that it tanks enclave build time when the enclave is being
> hosted by a Golang application.  Here's a snippet from a mail discussing
> the code.
> 
>    The bad news is that I don't think we can remove the add page worker
>    as applications with userspace schedulers, e.g. Go's M:N scheduler,
>    can see a 10x or more throughput improvement when using the worker
>    queue.  I did a bit of digging for the Golang case to make sure I
>    wasn't doing something horribly stupid/naive and found that it's a
>    generic issue in Golang with blocking (or just long-running) system
>    calls.  Because Golang multiplexes Goroutines on top of OS threads,
>    blocking syscalls introduce latency and context switching overhead,
>    e.g. Go's scheduler will spin up a new OS thread to service other
>    Goroutines after it realizes the syscall has blocked, and will later
>    destroy one of the OS threads so that it doesn't build up too many
>    unused.
> 
> IIRC, the scenario is spinning up several goroutines, each building an
> enclave.  I played around with adding a flag to do a synchronous EADD
> but didn't see a meaningful change in performance for the simple case.
> Supporting both the worker queue and direct paths was complex enough
> that I decided it wasn't worth the trouble for initial upstreaming.

Sigh.

It seems silly to add a workaround for a language that has trouble calling somewhat-but-not-too-slow syscalls or ioctls.

How about fixing this in Go directly?  Either convince the golang people to add a way to allocate a real thread for a particular region of code or have the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and have Go call *that*.  Then the mess stays in Go where it belongs.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  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:53         ` Sean Christopherson
  2 siblings, 0 replies; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-10 17:06 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Sean Christopherson, Andy Lutomirski, linux-sgx, Dave Hansen,
	Cedric Xing, Jethro Beekman, Dr . Greg Wettstein

On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
> Sigh.
> 
> It seems silly to add a workaround for a language that has trouble
> calling somewhat-but-not-too-slow syscalls or ioctls.
> 
> How about fixing this in Go directly?  Either convince the golang
> people to add a way to allocate a real thread for a particular region
> of code or have the Go SGX folks write a bit of C code to do  a whole
> bunch of ioctls and have Go call *that*.  Then the mess stays in Go
> where it belongs.

A worker thread would be only appropriate if the existing SGX code was
already in the mainline because then it would break the user space.

Doing it either way right now does not break anything so there is no
case for having it.

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  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
  2 siblings, 1 reply; 33+ messages in thread
From: Xing, Cedric @ 2019-06-10 18:09 UTC (permalink / raw)
  To: Andy Lutomirski, Christopherson, Sean J
  Cc: Andy Lutomirski, Jarkko Sakkinen, linux-sgx, Hansen, Dave,
	Jethro Beekman, Dr . Greg Wettstein

> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Friday, June 07, 2019 12:32 PM
> 
> > On Jun 6, 2019, at 10:32 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> >> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> >> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> >> <sean.j.christopherson@intel.com> wrote:
> >>>
> >>> For some enclaves, e.g. an enclave with a small code footprint and a
> >>> large working set, the vast majority of pages added to the enclave
> >>> are zero pages.  Introduce a flag to denote such zero pages.  The
> >>> major benefit of the flag will be realized in a future patch to use
> >>> Linux's actual zero page as the source, as opposed to explicitly
> >>> zeroing the enclave's backing memory.
> >>>
> >>
> >> I feel like I probably asked this at some point, but why is there a
> >> workqueue here at all?
> >
> > Performance.  A while back I wrote a patch set to remove the worker
> > queue and discovered that it tanks enclave build time when the enclave
> > is being hosted by a Golang application.  Here's a snippet from a mail
> > discussing the code.
> >
> >    The bad news is that I don't think we can remove the add page
> worker
> >    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> >    can see a 10x or more throughput improvement when using the worker
> >    queue.  I did a bit of digging for the Golang case to make sure I
> >    wasn't doing something horribly stupid/naive and found that it's a
> >    generic issue in Golang with blocking (or just long-running) system
> >    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> >    blocking syscalls introduce latency and context switching overhead,
> >    e.g. Go's scheduler will spin up a new OS thread to service other
> >    Goroutines after it realizes the syscall has blocked, and will
> later
> >    destroy one of the OS threads so that it doesn't build up too many
> >    unused.
> >
> > IIRC, the scenario is spinning up several goroutines, each building an
> > enclave.  I played around with adding a flag to do a synchronous EADD
> > but didn't see a meaningful change in performance for the simple case.
> > Supporting both the worker queue and direct paths was complex enough
> > that I decided it wasn't worth the trouble for initial upstreaming.
> 
> Sigh.
> 
> It seems silly to add a workaround for a language that has trouble
> calling somewhat-but-not-too-slow syscalls or ioctls.
> 
> How about fixing this in Go directly?  Either convince the golang people
> to add a way to allocate a real thread for a particular region of code
> or have the Go SGX folks write a bit of C code to do  a whole bunch of
> ioctls and have Go call *that*.  Then the mess stays in Go where it
> belongs.

The add page worker is less about performance but more about concurrency restrictions in SGX ISA. EADD/EEXTEND are slow instructions and both take lock on the SECS. So if there's another EADD/EEXTEND in progress then it will #GP.

In practice, no sane applications will EADD in multiple threads because that would make MRENCLAVE unpredictable. But adversary may use that just to trigger #GP in kernel. Therefore, the SGX module would have to lock around EADD/EEXTEND anyway. And then we figured it would be better to have the add page worker so that no lock would be necessary, plus (small) improvement in performance. 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-10 18:09         ` Xing, Cedric
@ 2019-06-10 18:41           ` Sean Christopherson
  0 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-10 18:41 UTC (permalink / raw)
  To: Xing, Cedric
  Cc: Andy Lutomirski, Andy Lutomirski, Jarkko Sakkinen, linux-sgx,
	Hansen, Dave, Jethro Beekman, Dr . Greg Wettstein

On Mon, Jun 10, 2019 at 11:09:50AM -0700, Xing, Cedric wrote:
> > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > Sent: Friday, June 07, 2019 12:32 PM
> > 
> > > On Jun 6, 2019, at 10:32 AM, Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > >> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> > >> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> > >> <sean.j.christopherson@intel.com> wrote:
> > >>>
> > >>> For some enclaves, e.g. an enclave with a small code footprint and a
> > >>> large working set, the vast majority of pages added to the enclave
> > >>> are zero pages.  Introduce a flag to denote such zero pages.  The
> > >>> major benefit of the flag will be realized in a future patch to use
> > >>> Linux's actual zero page as the source, as opposed to explicitly
> > >>> zeroing the enclave's backing memory.
> > >>>
> > >>
> > >> I feel like I probably asked this at some point, but why is there a
> > >> workqueue here at all?
> > >
> > > Performance.  A while back I wrote a patch set to remove the worker
> > > queue and discovered that it tanks enclave build time when the enclave
> > > is being hosted by a Golang application.  Here's a snippet from a mail
> > > discussing the code.
> > >
> > >    The bad news is that I don't think we can remove the add page
> > worker
> > >    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> > >    can see a 10x or more throughput improvement when using the worker
> > >    queue.  I did a bit of digging for the Golang case to make sure I
> > >    wasn't doing something horribly stupid/naive and found that it's a
> > >    generic issue in Golang with blocking (or just long-running) system
> > >    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> > >    blocking syscalls introduce latency and context switching overhead,
> > >    e.g. Go's scheduler will spin up a new OS thread to service other
> > >    Goroutines after it realizes the syscall has blocked, and will
> > later
> > >    destroy one of the OS threads so that it doesn't build up too many
> > >    unused.
> > >
> > > IIRC, the scenario is spinning up several goroutines, each building an
> > > enclave.  I played around with adding a flag to do a synchronous EADD
> > > but didn't see a meaningful change in performance for the simple case.
> > > Supporting both the worker queue and direct paths was complex enough
> > > that I decided it wasn't worth the trouble for initial upstreaming.
> > 
> > Sigh.
> > 
> > It seems silly to add a workaround for a language that has trouble
> > calling somewhat-but-not-too-slow syscalls or ioctls.
> > 
> > How about fixing this in Go directly?  Either convince the golang people
> > to add a way to allocate a real thread for a particular region of code
> > or have the Go SGX folks write a bit of C code to do  a whole bunch of
> > ioctls and have Go call *that*.  Then the mess stays in Go where it
> > belongs.
> 
> The add page worker is less about performance but more about concurrency
> restrictions in SGX ISA. EADD/EEXTEND are slow instructions and both take
> lock on the SECS. So if there's another EADD/EEXTEND in progress then it will
> #GP.
> 
> In practice, no sane applications will EADD in multiple threads because that
> would make MRENCLAVE unpredictable. But adversary may use that just to
> trigger #GP in kernel. Therefore, the SGX module would have to lock around
> EADD/EEXTEND anyway. And then we figured it would be better to have the add
> page worker so that no lock would be necessary, plus (small) improvement in
> performance. 

That may have been true years ago, but it doesn't reflect the current
reality.  The ADD_PAGE ioctl *and* the worker function both take the
enclave's lock.  The worker function actually takes it twice, once to pull
the request off the queue, and once to do EADD/EEXTEND.  The lock is
temporarily released by the worker function to avoid deadlock in case EPC
page allocation triggers reclaim.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  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:53         ` Sean Christopherson
  2019-06-13  0:38           ` Jethro Beekman
  2 siblings, 1 reply; 33+ messages in thread
From: Sean Christopherson @ 2019-06-10 18:53 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Andy Lutomirski, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Jethro Beekman, Dr . Greg Wettstein

On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
> 
> > On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> >> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> >> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> >> <sean.j.christopherson@intel.com> wrote:
> >>> 
> >>> For some enclaves, e.g. an enclave with a small code footprint and a
> >>> large working set, the vast majority of pages added to the enclave are
> >>> zero pages.  Introduce a flag to denote such zero pages.  The major
> >>> benefit of the flag will be realized in a future patch to use Linux's
> >>> actual zero page as the source, as opposed to explicitly zeroing the
> >>> enclave's backing memory.
> >>> 
> >> 
> >> I feel like I probably asked this at some point, but why is there a
> >> workqueue here at all?
> > 
> > Performance.  A while back I wrote a patch set to remove the worker queue
> > and discovered that it tanks enclave build time when the enclave is being
> > hosted by a Golang application.  Here's a snippet from a mail discussing
> > the code.
> > 
> >    The bad news is that I don't think we can remove the add page worker
> >    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> >    can see a 10x or more throughput improvement when using the worker
> >    queue.  I did a bit of digging for the Golang case to make sure I
> >    wasn't doing something horribly stupid/naive and found that it's a
> >    generic issue in Golang with blocking (or just long-running) system
> >    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> >    blocking syscalls introduce latency and context switching overhead,
> >    e.g. Go's scheduler will spin up a new OS thread to service other
> >    Goroutines after it realizes the syscall has blocked, and will later
> >    destroy one of the OS threads so that it doesn't build up too many
> >    unused.
> > 
> > IIRC, the scenario is spinning up several goroutines, each building an
> > enclave.  I played around with adding a flag to do a synchronous EADD
> > but didn't see a meaningful change in performance for the simple case.
> > Supporting both the worker queue and direct paths was complex enough
> > that I decided it wasn't worth the trouble for initial upstreaming.
> 
> Sigh.
> 
> It seems silly to add a workaround for a language that has trouble calling
> somewhat-but-not-too-slow syscalls or ioctls.
> 
> How about fixing this in Go directly?  Either convince the golang people to
> add a way to allocate a real thread for a particular region of code or have
> the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and
> have Go call *that*.  Then the mess stays in Go where it belongs.

Actually, I'm pretty sure changing the ioctl() from ADD_PAGE to ADD_REGION
would eliminate the worst of the golang slowdown without requiring
userspace to get super fancy.  I'm in favor of eliminating the work queue,
especially if the UAPI is changed to allow adding multiple pages in a
single syscall.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  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-12 15:14   ` Jarkko Sakkinen
  2019-06-12 15:23     ` Sean Christopherson
  1 sibling, 1 reply; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-12 15:14 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote:
> A reserved field could prove useful in the future, and packed structs
> can result in inefficiencies due to its impact on alignment.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Rather should extend mrmask just to 32 bits.

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl
  2019-06-05 19:48 [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl Sean Christopherson
                   ` (6 preceding siblings ...)
  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-12 15:16 ` Jarkko Sakkinen
  2019-06-12 18:14   ` Jarkko Sakkinen
  7 siblings, 1 reply; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-12 15:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 05, 2019 at 12:48:38PM -0700, Sean Christopherson wrote:
> This series is intended to be included in v21 of Jarkko's SGX series and
> applies on Jarkko's current master:
> 
>   dfc89a83b5bc ("docs: x86/sgx: Document the enclave API")
> 
> The primary goal of the series is to tweak the ioctl for adding pages to
> an enclave so that it is somewhat extensible, e.g. add a flags field that
> can be reused for access control integration and SGX2/EAUG, and add a size
> field so that multiple pages can be added in a single call (batching EADD
> has been mentioned at various times in the past).
> 
> The secondary goal is to improve the performance of building enclaves.
> Handling multiple pages in a single call helps somewhat, but the real win
> (for some enclaves) is using the kernel's zero page as the source for EADD
> when possible.

For v21 I will add 1, 2 and 6 from these. Thanks.

I'll change the size or mrmask field to 32 bits.

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  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
  0 siblings, 2 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-12 15:23 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote:
> > A reserved field could prove useful in the future, and packed structs
> > can result in inefficiencies due to its impact on alignment.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Rather should extend mrmask just to 32 bits.

I considered that option, but extending mrmask would probably confusing
to userspace, e.g. we'd have to document that bits 31:15 are ignored.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/7] x86/sgx: Clean up and enhance add pages ioctl
  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
  0 siblings, 0 replies; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-12 18:14 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 12, 2019 at 06:16:05PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 05, 2019 at 12:48:38PM -0700, Sean Christopherson wrote:
> > This series is intended to be included in v21 of Jarkko's SGX series and
> > applies on Jarkko's current master:
> > 
> >   dfc89a83b5bc ("docs: x86/sgx: Document the enclave API")
> > 
> > The primary goal of the series is to tweak the ioctl for adding pages to
> > an enclave so that it is somewhat extensible, e.g. add a flags field that
> > can be reused for access control integration and SGX2/EAUG, and add a size
> > field so that multiple pages can be added in a single call (batching EADD
> > has been mentioned at various times in the past).
> > 
> > The secondary goal is to improve the performance of building enclaves.
> > Handling multiple pages in a single call helps somewhat, but the real win
> > (for some enclaves) is using the kernel's zero page as the source for EADD
> > when possible.
> 
> For v21 I will add 1, 2 and 6 from these. Thanks.

I meant to say 1, 2 and 3 (typo). 

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-10 18:53         ` Sean Christopherson
@ 2019-06-13  0:38           ` Jethro Beekman
  2019-06-13 13:46             ` Sean Christopherson
  0 siblings, 1 reply; 33+ messages in thread
From: Jethro Beekman @ 2019-06-13  0:38 UTC (permalink / raw)
  To: Sean Christopherson, Andy Lutomirski
  Cc: Andy Lutomirski, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Dr . Greg Wettstein

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

On 2019-06-10 11:53, Sean Christopherson wrote:
> On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
>>
>>> On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>>
>>>> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
>>>> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
>>>> <sean.j.christopherson@intel.com> wrote:
>>>>>
>>>>> For some enclaves, e.g. an enclave with a small code footprint and a
>>>>> large working set, the vast majority of pages added to the enclave are
>>>>> zero pages.  Introduce a flag to denote such zero pages.  The major
>>>>> benefit of the flag will be realized in a future patch to use Linux's
>>>>> actual zero page as the source, as opposed to explicitly zeroing the
>>>>> enclave's backing memory.
>>>>>
>>>>
>>>> I feel like I probably asked this at some point, but why is there a
>>>> workqueue here at all?
>>>
>>> Performance.  A while back I wrote a patch set to remove the worker queue
>>> and discovered that it tanks enclave build time when the enclave is being
>>> hosted by a Golang application.  Here's a snippet from a mail discussing
>>> the code.
>>>
>>>     The bad news is that I don't think we can remove the add page worker
>>>     as applications with userspace schedulers, e.g. Go's M:N scheduler,
>>>     can see a 10x or more throughput improvement when using the worker
>>>     queue.  I did a bit of digging for the Golang case to make sure I
>>>     wasn't doing something horribly stupid/naive and found that it's a
>>>     generic issue in Golang with blocking (or just long-running) system
>>>     calls.  Because Golang multiplexes Goroutines on top of OS threads,
>>>     blocking syscalls introduce latency and context switching overhead,
>>>     e.g. Go's scheduler will spin up a new OS thread to service other
>>>     Goroutines after it realizes the syscall has blocked, and will later
>>>     destroy one of the OS threads so that it doesn't build up too many
>>>     unused.
>>>
>>> IIRC, the scenario is spinning up several goroutines, each building an
>>> enclave.  I played around with adding a flag to do a synchronous EADD
>>> but didn't see a meaningful change in performance for the simple case.
>>> Supporting both the worker queue and direct paths was complex enough
>>> that I decided it wasn't worth the trouble for initial upstreaming.
>>
>> Sigh.
>>
>> It seems silly to add a workaround for a language that has trouble calling
>> somewhat-but-not-too-slow syscalls or ioctls.
>>
>> How about fixing this in Go directly?  Either convince the golang people to
>> add a way to allocate a real thread for a particular region of code or have
>> the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and
>> have Go call *that*.  Then the mess stays in Go where it belongs.
> 
> Actually, I'm pretty sure changing the ioctl() from ADD_PAGE to ADD_REGION
> would eliminate the worst of the golang slowdown without requiring
> userspace to get super fancy.  I'm in favor of eliminating the work queue,
> especially if the UAPI is changed to allow adding multiple pages in a
> single syscall.
> 

I don't know if this is going to matter a whole lot, but have you 
considered the performance impact of needing to the EPC paging while 
doing the EADD ioctl and how this interacts with having a workqueue?

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  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
  1 sibling, 1 reply; 33+ messages in thread
From: Jethro Beekman @ 2019-06-13  0:43 UTC (permalink / raw)
  To: Sean Christopherson, Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Dr . Greg Wettstein

[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]

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.

Somewhat unrelated: have you considered optionally "gifting" enclave 
source pages to the kernel (as in vmsplice)? That would avoid the 
copy_from_user.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  2019-06-12 15:23     ` Sean Christopherson
@ 2019-06-13  0:44       ` Jethro Beekman
  2019-06-13 15:38       ` Jarkko Sakkinen
  1 sibling, 0 replies; 33+ messages in thread
From: Jethro Beekman @ 2019-06-13  0:44 UTC (permalink / raw)
  To: Sean Christopherson, Jarkko Sakkinen
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Dr . Greg Wettstein

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On 2019-06-12 08:23, Sean Christopherson wrote:
> On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote:
>>> A reserved field could prove useful in the future, and packed structs
>>> can result in inefficiencies due to its impact on alignment.
>>>
>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>>
>> Rather should extend mrmask just to 32 bits.
> 
> I considered that option, but extending mrmask would probably confusing
> to userspace, e.g. we'd have to document that bits 31:15 are ignored.
> 

Yeah this doesn't make a whole lot of sense to me.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-13  0:38           ` Jethro Beekman
@ 2019-06-13 13:46             ` Sean Christopherson
  2019-06-13 16:16               ` Andy Lutomirski
  0 siblings, 1 reply; 33+ messages in thread
From: Sean Christopherson @ 2019-06-13 13:46 UTC (permalink / raw)
  To: Jethro Beekman
  Cc: Andy Lutomirski, Andy Lutomirski, Jarkko Sakkinen, linux-sgx,
	Dave Hansen, Cedric Xing, Dr . Greg Wettstein

On Thu, Jun 13, 2019 at 12:38:02AM +0000, Jethro Beekman wrote:
> On 2019-06-10 11:53, Sean Christopherson wrote:
> >On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
> >>
> >>>On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >>>
> >>>>On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> >>>>On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> >>>><sean.j.christopherson@intel.com> wrote:
> >>>>>
> >>>>>For some enclaves, e.g. an enclave with a small code footprint and a
> >>>>>large working set, the vast majority of pages added to the enclave are
> >>>>>zero pages.  Introduce a flag to denote such zero pages.  The major
> >>>>>benefit of the flag will be realized in a future patch to use Linux's
> >>>>>actual zero page as the source, as opposed to explicitly zeroing the
> >>>>>enclave's backing memory.
> >>>>>
> >>>>
> >>>>I feel like I probably asked this at some point, but why is there a
> >>>>workqueue here at all?
> >>>
> >>>Performance.  A while back I wrote a patch set to remove the worker queue
> >>>and discovered that it tanks enclave build time when the enclave is being
> >>>hosted by a Golang application.  Here's a snippet from a mail discussing
> >>>the code.
> >>>
> >>>    The bad news is that I don't think we can remove the add page worker
> >>>    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> >>>    can see a 10x or more throughput improvement when using the worker
> >>>    queue.  I did a bit of digging for the Golang case to make sure I
> >>>    wasn't doing something horribly stupid/naive and found that it's a
> >>>    generic issue in Golang with blocking (or just long-running) system
> >>>    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> >>>    blocking syscalls introduce latency and context switching overhead,
> >>>    e.g. Go's scheduler will spin up a new OS thread to service other
> >>>    Goroutines after it realizes the syscall has blocked, and will later
> >>>    destroy one of the OS threads so that it doesn't build up too many
> >>>    unused.
> >>>
> >>>IIRC, the scenario is spinning up several goroutines, each building an
> >>>enclave.  I played around with adding a flag to do a synchronous EADD
> >>>but didn't see a meaningful change in performance for the simple case.
> >>>Supporting both the worker queue and direct paths was complex enough
> >>>that I decided it wasn't worth the trouble for initial upstreaming.
> >>
> >>Sigh.
> >>
> >>It seems silly to add a workaround for a language that has trouble calling
> >>somewhat-but-not-too-slow syscalls or ioctls.
> >>
> >>How about fixing this in Go directly?  Either convince the golang people to
> >>add a way to allocate a real thread for a particular region of code or have
> >>the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and
> >>have Go call *that*.  Then the mess stays in Go where it belongs.
> >
> >Actually, I'm pretty sure changing the ioctl() from ADD_PAGE to ADD_REGION
> >would eliminate the worst of the golang slowdown without requiring
> >userspace to get super fancy.  I'm in favor of eliminating the work queue,
> >especially if the UAPI is changed to allow adding multiple pages in a
> >single syscall.
> >
> 
> I don't know if this is going to matter a whole lot, but have you considered
> the performance impact of needing to the EPC paging while doing the EADD
> ioctl and how this interacts with having a workqueue?

Yep, other than the goroutine case, eliminating the workqueue doesn't
substantially affect performance in either direction, regardless of the
pressure on the EPC.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/7] x86/sgx: Add a reserved field to sgx_enclave_add_region to drop 'packed'
  2019-06-12 15:23     ` Sean Christopherson
  2019-06-13  0:44       ` Jethro Beekman
@ 2019-06-13 15:38       ` Jarkko Sakkinen
  1 sibling, 0 replies; 33+ messages in thread
From: Jarkko Sakkinen @ 2019-06-13 15:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-sgx, Dave Hansen, Cedric Xing, Andy Lutomirski,
	Jethro Beekman, Dr . Greg Wettstein

On Wed, Jun 12, 2019 at 08:23:42AM -0700, Sean Christopherson wrote:
> On Wed, Jun 12, 2019 at 06:14:39PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 05, 2019 at 12:48:45PM -0700, Sean Christopherson wrote:
> > > A reserved field could prove useful in the future, and packed structs
> > > can result in inefficiencies due to its impact on alignment.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Rather should extend mrmask just to 32 bits.
> 
> I considered that option, but extending mrmask would probably confusing
> to userspace, e.g. we'd have to document that bits 31:15 are ignored.

I meant u64.

Not a big deal. With the reserved field we would have to document the
same in different terms.

/Jarkko

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-13 13:46             ` Sean Christopherson
@ 2019-06-13 16:16               ` Andy Lutomirski
  2019-06-13 16:54                 ` Sean Christopherson
  0 siblings, 1 reply; 33+ messages in thread
From: Andy Lutomirski @ 2019-06-13 16:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Jethro Beekman, Andy Lutomirski, Jarkko Sakkinen, linux-sgx,
	Dave Hansen, Cedric Xing, Dr . Greg Wettstein

On Thu, Jun 13, 2019 at 6:46 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 12:38:02AM +0000, Jethro Beekman wrote:
> > On 2019-06-10 11:53, Sean Christopherson wrote:
> > >On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
> > >>
> > >>>On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > >>>
> > >>>>On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> > >>>>On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> > >>>><sean.j.christopherson@intel.com> wrote:
> > >>>>>
> > >>>>>For some enclaves, e.g. an enclave with a small code footprint and a
> > >>>>>large working set, the vast majority of pages added to the enclave are
> > >>>>>zero pages.  Introduce a flag to denote such zero pages.  The major
> > >>>>>benefit of the flag will be realized in a future patch to use Linux's
> > >>>>>actual zero page as the source, as opposed to explicitly zeroing the
> > >>>>>enclave's backing memory.
> > >>>>>
> > >>>>
> > >>>>I feel like I probably asked this at some point, but why is there a
> > >>>>workqueue here at all?
> > >>>
> > >>>Performance.  A while back I wrote a patch set to remove the worker queue
> > >>>and discovered that it tanks enclave build time when the enclave is being
> > >>>hosted by a Golang application.  Here's a snippet from a mail discussing
> > >>>the code.
> > >>>
> > >>>    The bad news is that I don't think we can remove the add page worker
> > >>>    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> > >>>    can see a 10x or more throughput improvement when using the worker
> > >>>    queue.  I did a bit of digging for the Golang case to make sure I
> > >>>    wasn't doing something horribly stupid/naive and found that it's a
> > >>>    generic issue in Golang with blocking (or just long-running) system
> > >>>    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> > >>>    blocking syscalls introduce latency and context switching overhead,
> > >>>    e.g. Go's scheduler will spin up a new OS thread to service other
> > >>>    Goroutines after it realizes the syscall has blocked, and will later
> > >>>    destroy one of the OS threads so that it doesn't build up too many
> > >>>    unused.
> > >>>
> > >>>IIRC, the scenario is spinning up several goroutines, each building an
> > >>>enclave.  I played around with adding a flag to do a synchronous EADD
> > >>>but didn't see a meaningful change in performance for the simple case.
> > >>>Supporting both the worker queue and direct paths was complex enough
> > >>>that I decided it wasn't worth the trouble for initial upstreaming.
> > >>
> > >>Sigh.
> > >>
> > >>It seems silly to add a workaround for a language that has trouble calling
> > >>somewhat-but-not-too-slow syscalls or ioctls.
> > >>
> > >>How about fixing this in Go directly?  Either convince the golang people to
> > >>add a way to allocate a real thread for a particular region of code or have
> > >>the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and
> > >>have Go call *that*.  Then the mess stays in Go where it belongs.
> > >
> > >Actually, I'm pretty sure changing the ioctl() from ADD_PAGE to ADD_REGION
> > >would eliminate the worst of the golang slowdown without requiring
> > >userspace to get super fancy.  I'm in favor of eliminating the work queue,
> > >especially if the UAPI is changed to allow adding multiple pages in a
> > >single syscall.
> > >
> >
> > I don't know if this is going to matter a whole lot, but have you considered
> > the performance impact of needing to the EPC paging while doing the EADD
> > ioctl and how this interacts with having a workqueue?
>
> Yep, other than the goroutine case, eliminating the workqueue doesn't
> substantially affect performance in either direction, regardless of the
> pressure on the EPC.


It should get rid of some extra copies and allocations, no?

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  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:45       ` Xing, Cedric
  0 siblings, 2 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-13 16:51 UTC (permalink / raw)
  To: Jethro Beekman
  Cc: Jarkko Sakkinen, linux-sgx, Dave Hansen, Cedric Xing,
	Andy Lutomirski, Dr . Greg Wettstein

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.

> Somewhat unrelated: have you considered optionally "gifting" enclave source
> pages to the kernel (as in vmsplice)? That would avoid the copy_from_user.

If we ditch the workqueue then we probably don't even need to gift the
page, e.g. I think we allocate the EPC page prior to taking mmap_sem, and
then simply do gup+kmap around EADD.  We'd just need to be careful about
not allocating EPC pages for ioctls that are guaranteed to fail.



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/7] x86/sgx: Add flag to zero added region instead of copying from source
  2019-06-13 16:16               ` Andy Lutomirski
@ 2019-06-13 16:54                 ` Sean Christopherson
  0 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-13 16:54 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jethro Beekman, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Dr . Greg Wettstein

On Thu, Jun 13, 2019 at 09:16:46AM -0700, Andy Lutomirski wrote:
> On Thu, Jun 13, 2019 at 6:46 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Thu, Jun 13, 2019 at 12:38:02AM +0000, Jethro Beekman wrote:
> > > On 2019-06-10 11:53, Sean Christopherson wrote:
> > > >On Fri, Jun 07, 2019 at 12:32:23PM -0700, Andy Lutomirski wrote:
> > > >>
> > > >>>On Jun 6, 2019, at 10:32 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > > >>>
> > > >>>>On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote:
> > > >>>>On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson
> > > >>>><sean.j.christopherson@intel.com> wrote:
> > > >>>>>
> > > >>>>>For some enclaves, e.g. an enclave with a small code footprint and a
> > > >>>>>large working set, the vast majority of pages added to the enclave are
> > > >>>>>zero pages.  Introduce a flag to denote such zero pages.  The major
> > > >>>>>benefit of the flag will be realized in a future patch to use Linux's
> > > >>>>>actual zero page as the source, as opposed to explicitly zeroing the
> > > >>>>>enclave's backing memory.
> > > >>>>>
> > > >>>>
> > > >>>>I feel like I probably asked this at some point, but why is there a
> > > >>>>workqueue here at all?
> > > >>>
> > > >>>Performance.  A while back I wrote a patch set to remove the worker queue
> > > >>>and discovered that it tanks enclave build time when the enclave is being
> > > >>>hosted by a Golang application.  Here's a snippet from a mail discussing
> > > >>>the code.
> > > >>>
> > > >>>    The bad news is that I don't think we can remove the add page worker
> > > >>>    as applications with userspace schedulers, e.g. Go's M:N scheduler,
> > > >>>    can see a 10x or more throughput improvement when using the worker
> > > >>>    queue.  I did a bit of digging for the Golang case to make sure I
> > > >>>    wasn't doing something horribly stupid/naive and found that it's a
> > > >>>    generic issue in Golang with blocking (or just long-running) system
> > > >>>    calls.  Because Golang multiplexes Goroutines on top of OS threads,
> > > >>>    blocking syscalls introduce latency and context switching overhead,
> > > >>>    e.g. Go's scheduler will spin up a new OS thread to service other
> > > >>>    Goroutines after it realizes the syscall has blocked, and will later
> > > >>>    destroy one of the OS threads so that it doesn't build up too many
> > > >>>    unused.
> > > >>>
> > > >>>IIRC, the scenario is spinning up several goroutines, each building an
> > > >>>enclave.  I played around with adding a flag to do a synchronous EADD
> > > >>>but didn't see a meaningful change in performance for the simple case.
> > > >>>Supporting both the worker queue and direct paths was complex enough
> > > >>>that I decided it wasn't worth the trouble for initial upstreaming.
> > > >>
> > > >>Sigh.
> > > >>
> > > >>It seems silly to add a workaround for a language that has trouble calling
> > > >>somewhat-but-not-too-slow syscalls or ioctls.
> > > >>
> > > >>How about fixing this in Go directly?  Either convince the golang people to
> > > >>add a way to allocate a real thread for a particular region of code or have
> > > >>the Go SGX folks write a bit of C code to do  a whole bunch of ioctls and
> > > >>have Go call *that*.  Then the mess stays in Go where it belongs.
> > > >
> > > >Actually, I'm pretty sure changing the ioctl() from ADD_PAGE to ADD_REGION
> > > >would eliminate the worst of the golang slowdown without requiring
> > > >userspace to get super fancy.  I'm in favor of eliminating the work queue,
> > > >especially if the UAPI is changed to allow adding multiple pages in a
> > > >single syscall.
> > > >
> > >
> > > I don't know if this is going to matter a whole lot, but have you considered
> > > the performance impact of needing to the EPC paging while doing the EADD
> > > ioctl and how this interacts with having a workqueue?
> >
> > Yep, other than the goroutine case, eliminating the workqueue doesn't
> > substantially affect performance in either direction, regardless of the
> > pressure on the EPC.
> 
> It should get rid of some extra copies and allocations, no?

My experiments were fairly dumb, e.g. removed the workqueue without doing
extra enhancements like avoiding copy_from_user.  IIRC, the type of
allocation changed, but the number of copies/allocations stayed the same.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  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
  1 sibling, 1 reply; 33+ messages in thread
From: Andy Lutomirski @ 2019-06-13 19:05 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Jethro Beekman, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Andy Lutomirski, Dr . Greg Wettstein

On Thu, Jun 13, 2019 at 9:51 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> 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.
>
> > Somewhat unrelated: have you considered optionally "gifting" enclave source
> > pages to the kernel (as in vmsplice)? That would avoid the copy_from_user.
>
> If we ditch the workqueue then we probably don't even need to gift the
> page, e.g. I think we allocate the EPC page prior to taking mmap_sem, and
> then simply do gup+kmap around EADD.  We'd just need to be careful about
> not allocating EPC pages for ioctls that are guaranteed to fail.
>
>

Why gup + kmap?  Can't you just do STAC; EADD; CLAC?  (Using the
appropriate C helpers, of course.)

--Andy

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  2019-06-13 19:05       ` Andy Lutomirski
@ 2019-06-13 19:15         ` Sean Christopherson
  0 siblings, 0 replies; 33+ messages in thread
From: Sean Christopherson @ 2019-06-13 19:15 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jethro Beekman, Jarkko Sakkinen, linux-sgx, Dave Hansen,
	Cedric Xing, Dr . Greg Wettstein

On Thu, Jun 13, 2019 at 12:05:37PM -0700, Andy Lutomirski wrote:
> On Thu, Jun 13, 2019 at 9:51 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > If we ditch the workqueue then we probably don't even need to gift the
> > page, e.g. I think we allocate the EPC page prior to taking mmap_sem, and
> > then simply do gup+kmap around EADD.  We'd just need to be careful about
> > not allocating EPC pages for ioctls that are guaranteed to fail.
> >
> >
> 
> Why gup + kmap?  Can't you just do STAC; EADD; CLAC?  (Using the
> appropriate C helpers, of course.)

That should work as well, we already have exception fixup on ENCLS.  I
obviously haven't given much thought to what all we can do once the
workqueue goes bye bye :-)

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: [PATCH 4/7] x86/sgx: Allow userspace to add multiple pages in single ioctl()
  2019-06-13 16:51     ` Sean Christopherson
  2019-06-13 19:05       ` Andy Lutomirski
@ 2019-06-13 19:45       ` Xing, Cedric
  1 sibling, 0 replies; 33+ messages in thread
From: Xing, Cedric @ 2019-06-13 19:45 UTC (permalink / raw)
  To: Christopherson, Sean J, Jethro Beekman
  Cc: Jarkko Sakkinen, linux-sgx, Hansen, Dave, Andy Lutomirski,
	Dr . Greg Wettstein

> 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.

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2019-06-13 19:45 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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).