This is an optimization of a set of sgx-related codes, each of which is independent of the patch. Because the second and third patches have conflicting dependencies, these patches are put together.
--- v5 changes: * Remove the two patches with no actual value * Typo fix in commit message
v4 changes: * Improvements suggested by review
v3 changes: * split free_cnt count and spin lock optimization into two patches
v2 changes: * review suggested changes
Tianjia Zhang (3): selftests/x86: Use getauxval() to simplify the code in sgx x86/sgx: Allows ioctl PROVISION to execute before CREATE x86/sgx: Remove redundant if conditions in sgx_encl_create
arch/x86/kernel/cpu/sgx/driver.c | 1 + arch/x86/kernel/cpu/sgx/ioctl.c | 8 ++++---- tools/testing/selftests/sgx/main.c | 24 ++++-------------------- 3 files changed, 9 insertions(+), 24 deletions(-)
Simplify the sgx code implemntation by using library function getauxval() instead of a custom function to get the base address of vDSO.
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com Reviewed-by: Jarkko Sakkinen jarkko@kernel.org Acked-by: Shuah Khan skhan@linuxfoundation.org --- tools/testing/selftests/sgx/main.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 724cec700926..5167505fbb46 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -15,6 +15,7 @@ #include <sys/stat.h> #include <sys/time.h> #include <sys/types.h> +#include <sys/auxv.h> #include "defines.h" #include "main.h" #include "../kselftest.h" @@ -28,24 +29,6 @@ struct vdso_symtab { Elf64_Word *elf_hashtab; };
-static void *vdso_get_base_addr(char *envp[]) -{ - Elf64_auxv_t *auxv; - int i; - - for (i = 0; envp[i]; i++) - ; - - auxv = (Elf64_auxv_t *)&envp[i + 1]; - - for (i = 0; auxv[i].a_type != AT_NULL; i++) { - if (auxv[i].a_type == AT_SYSINFO_EHDR) - return (void *)auxv[i].a_un.a_val; - } - - return NULL; -} - static Elf64_Dyn *vdso_get_dyntab(void *addr) { Elf64_Ehdr *ehdr = addr; @@ -162,7 +145,7 @@ static int user_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r return 0; }
-int main(int argc, char *argv[], char *envp[]) +int main(int argc, char *argv[]) { struct sgx_enclave_run run; struct vdso_symtab symtab; @@ -203,7 +186,8 @@ int main(int argc, char *argv[], char *envp[]) memset(&run, 0, sizeof(run)); run.tcs = encl.encl_base;
- addr = vdso_get_base_addr(envp); + /* Get vDSO base address */ + addr = (void *)getauxval(AT_SYSINFO_EHDR); if (!addr) goto err;
In the function sgx_create_enclave(), the direct assignment operation of attributes_mask determines that the ioctl PROVISION operation must be executed after the ioctl CREATE operation, which will limit the flexibility of SGX developers.
This patch takes the assignment of attributes_mask from the function sgx_create_enclave() has been moved to the function sgx_open(), this will allow users to perform ioctl PROVISION operations before ioctl CREATE, increase the flexibility of the API and reduce restrictions.
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com --- arch/x86/kernel/cpu/sgx/driver.c | 1 + arch/x86/kernel/cpu/sgx/ioctl.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c index 8ce6d8371cfb..892e2a2a3221 100644 --- a/arch/x86/kernel/cpu/sgx/driver.c +++ b/arch/x86/kernel/cpu/sgx/driver.c @@ -36,6 +36,7 @@ static int sgx_open(struct inode *inode, struct file *file) return ret; }
+ encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS; file->private_data = encl;
return 0; diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 90a5caf76939..1c6ecf9fbeff 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -109,7 +109,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) encl->base = secs->base; encl->size = secs->size; encl->attributes = secs->attributes; - encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS;
/* Set only after completion, as encl->lock has not been taken. */ set_bit(SGX_ENCL_CREATED, &encl->flags);
In this scenario, there is no case where va_page is NULL, and the error has been checked. The if-condition statement here is redundant, so remove the condition detection.
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com Acked-by: Jarkko Sakkinen jarkko@kernel.org --- arch/x86/kernel/cpu/sgx/ioctl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 1c6ecf9fbeff..719c21cca569 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -66,9 +66,10 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) va_page = sgx_encl_grow(encl); if (IS_ERR(va_page)) return PTR_ERR(va_page); - else if (va_page) - list_add(&va_page->list, &encl->va_pages); - /* else the tail page of the VA page list had free slots. */ + if (!va_page) + return -EIO; + + list_add(&va_page->list, &encl->va_pages);
/* The extra page goes to SECS. */ encl_size = secs->size + PAGE_SIZE;
On Tue, Feb 16, 2021 at 11:31:33AM +0800, Tianjia Zhang wrote:
In this scenario, there is no case where va_page is NULL, and the error has been checked. The if-condition statement here is redundant, so remove the condition detection.
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com Acked-by: Jarkko Sakkinen jarkko@kernel.org
arch/x86/kernel/cpu/sgx/ioctl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 1c6ecf9fbeff..719c21cca569 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -66,9 +66,10 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) va_page = sgx_encl_grow(encl); if (IS_ERR(va_page)) return PTR_ERR(va_page);
- else if (va_page)
list_add(&va_page->list, &encl->va_pages);- /* else the tail page of the VA page list had free slots. */
- if (!va_page)
return -EIO;
Why this check?
- list_add(&va_page->list, &encl->va_pages);
/* The extra page goes to SECS. */ encl_size = secs->size + PAGE_SIZE; -- 2.19.1.3.ge56e4f7
/Jarkko
On 2/16/21 4:29 PM, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 11:31:33AM +0800, Tianjia Zhang wrote:
In this scenario, there is no case where va_page is NULL, and the error has been checked. The if-condition statement here is redundant, so remove the condition detection.
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com Acked-by: Jarkko Sakkinen jarkko@kernel.org
arch/x86/kernel/cpu/sgx/ioctl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 1c6ecf9fbeff..719c21cca569 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -66,9 +66,10 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) va_page = sgx_encl_grow(encl); if (IS_ERR(va_page)) return PTR_ERR(va_page);
- else if (va_page)
list_add(&va_page->list, &encl->va_pages);- /* else the tail page of the VA page list had free slots. */
- if (!va_page)
return -EIO;Why this check?
The value of va_page may be a negative error value, it may be NULL, or it may be a valid pointer, so check this return value.
Tianjia
- list_add(&va_page->list, &encl->va_pages);
/* The extra page goes to SECS. */ encl_size = secs->size + PAGE_SIZE; -- 2.19.1.3.ge56e4f7
/Jarkko
linux-kselftest-mirror@lists.linaro.org
-
Jarkko Sakkinen -
Tianjia Zhang