[for_v22,v2,4/8] x86/sgx: Set SGX_ENCL_PAGE_TCS when allocating encl_page
diff mbox series

Message ID 20190813011252.4121-5-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Remove EADD worker and page copy
Related show

Commit Message

Sean Christopherson Aug. 13, 2019, 1:12 a.m. UTC
Set SGX_ENCL_PAGE_TCS when encl_page->desc is initialized in
sgx_encl_page_alloc() to improve readability, and so that the code
isn't affected when the bulk of __sgx_encl_add_page() is rewritten
to remove the EADD worker in a future patch.

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

Comments

Jarkko Sakkinen Aug. 22, 2019, 12:56 p.m. UTC | #1
On Mon, Aug 12, 2019 at 06:12:48PM -0700, Sean Christopherson wrote:
> Set SGX_ENCL_PAGE_TCS when encl_page->desc is initialized in
> sgx_encl_page_alloc() to improve readability, and so that the code
> isn't affected when the bulk of __sgx_encl_add_page() is rewritten
> to remove the EADD worker in a future patch.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I don't mean to be impolite but this change only decreases readability,
and in no possible way improves it. Clear semantics and such things
improve readability

The semantics are terrible if you have a parameter that can have
multiple values but only a single value would trigger something. We
don't want that kind of weirdness to the codebase. We want clean
up such glitches.

If you have a boolean behaviour, please use a boolean value then.

I changed it to always assign the page type and added spacing to make
the code more readable. SECINFO gets validated early in the ioctl so
there should not be problem.

/Jarkko
Sean Christopherson Aug. 22, 2019, 2:24 p.m. UTC | #2
On Thu, Aug 22, 2019 at 03:56:43PM +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 12, 2019 at 06:12:48PM -0700, Sean Christopherson wrote:
> > Set SGX_ENCL_PAGE_TCS when encl_page->desc is initialized in
> > sgx_encl_page_alloc() to improve readability, and so that the code
> > isn't affected when the bulk of __sgx_encl_add_page() is rewritten
> > to remove the EADD worker in a future patch.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I don't mean to be impolite but this change only decreases readability,
> and in no possible way improves it. Clear semantics and such things
> improve readability

No worries, it's not the first time my interpretation of what's readable
has deviated from the norm :-)

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 5831f51d64cd..2b3b86412131 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -247,7 +247,8 @@  static int sgx_validate_secs(const struct sgx_secs *secs,
 
 static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 						 unsigned long addr,
-						 unsigned long prot)
+						 unsigned long prot,
+						 u64 page_type)
 {
 	struct sgx_encl_page *encl_page;
 	int ret;
@@ -258,6 +259,8 @@  static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 	if (!encl_page)
 		return ERR_PTR(-ENOMEM);
 	encl_page->desc = addr;
+	if (page_type == SGX_SECINFO_TCS)
+		encl_page->desc |= SGX_ENCL_PAGE_TCS;
 	encl_page->encl = encl;
 	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
@@ -476,7 +479,6 @@  static int __sgx_encl_add_page(struct sgx_encl *encl,
 			       unsigned int mrmask)
 {
 	unsigned long page_index = sgx_encl_get_index(encl_page);
-	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_add_page_req *req = NULL;
 	struct page *backing;
 	void *backing_ptr;
@@ -495,8 +497,7 @@  static int __sgx_encl_add_page(struct sgx_encl *encl,
 	backing_ptr = kmap(backing);
 	memcpy(backing_ptr, data, PAGE_SIZE);
 	kunmap(backing);
-	if (page_type == SGX_SECINFO_TCS)
-		encl_page->desc |= SGX_ENCL_PAGE_TCS;
+
 	memcpy(&req->secinfo, secinfo, sizeof(*secinfo));
 	req->encl = encl;
 	req->encl_page = encl_page;
@@ -533,7 +534,7 @@  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto err_out_unlock;
 	}
 
-	encl_page = sgx_encl_page_alloc(encl, addr, prot);
+	encl_page = sgx_encl_page_alloc(encl, addr, prot, page_type);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
 		goto err_out_shrink;