diff mbox series

[RFC,01/23] x86/sgx: Split out adding EPC page to free list to separate helper

Message ID 3d50c2614ff8a46b44062a398fd8644bcda92132.1609890536.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM SGX virtualization support | expand

Commit Message

Huang, Kai Jan. 6, 2021, 1:55 a.m. UTC 
From: Sean Christopherson <sean.j.christopherson@intel.com>

SGX virtualization requires to allocate "raw" EPC and use it as virtual
EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
track how EPC pages are used in VM, e.g. (de)construction of enclaves,
so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
knowledge of which pages are SECS with non-zero child counts.

Split sgx_free_page() into two parts so that the "add to free list"
part can be used by virtual EPC without having to modify the EREMOVE
logic in sgx_free_page().

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 24 ++++++++++++++++++------
 arch/x86/kernel/cpu/sgx/sgx.h  |  1 +
 2 files changed, 19 insertions(+), 6 deletions(-)

Comments

Jarkko Sakkinen Jan. 11, 2021, 10:38 p.m. UTC | #1 
On Wed, 2021-01-06 at 14:55 +1300, Kai Huang wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> SGX virtualization requires to allocate "raw" EPC and use it as virtual
> EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
> track how EPC pages are used in VM, e.g. (de)construction of enclaves,
> so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
> knowledge of which pages are SECS with non-zero child counts.
> 
> Split sgx_free_page() into two parts so that the "add to free list"
> part can be used by virtual EPC without having to modify the EREMOVE
> logic in sgx_free_page().
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

I have a better idea with the same outcome for KVM.

https://lore.kernel.org/linux-sgx/20210111223610.62261-1-jarkko@kernel.org/T/#t

/Jarkko
Huang, Kai Jan. 12, 2021, 12:19 a.m. UTC | #2 
On Tue, 12 Jan 2021 00:38:40 +0200 Jarkko Sakkinen wrote:
> On Wed, 2021-01-06 at 14:55 +1300, Kai Huang wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > SGX virtualization requires to allocate "raw" EPC and use it as virtual
> > EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
> > track how EPC pages are used in VM, e.g. (de)construction of enclaves,
> > so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
> > knowledge of which pages are SECS with non-zero child counts.
> > 
> > Split sgx_free_page() into two parts so that the "add to free list"
> > part can be used by virtual EPC without having to modify the EREMOVE
> > logic in sgx_free_page().
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> I have a better idea with the same outcome for KVM.
> 
> https://lore.kernel.org/linux-sgx/20210111223610.62261-1-jarkko@kernel.org/T/#t

I agree with your patch this one can be replaced. I'll include your patch in
next version, and once it is upstreamed, it can be removed in my series.

Sean, please let me know if you have objection.
Sean Christopherson Jan. 12, 2021, 9:45 p.m. UTC | #3 
On Tue, Jan 12, 2021, Kai Huang wrote:
> On Tue, 12 Jan 2021 00:38:40 +0200 Jarkko Sakkinen wrote:
> > On Wed, 2021-01-06 at 14:55 +1300, Kai Huang wrote:
> > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > SGX virtualization requires to allocate "raw" EPC and use it as virtual
> > > EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
> > > track how EPC pages are used in VM, e.g. (de)construction of enclaves,
> > > so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
> > > knowledge of which pages are SECS with non-zero child counts.
> > > 
> > > Split sgx_free_page() into two parts so that the "add to free list"
> > > part can be used by virtual EPC without having to modify the EREMOVE
> > > logic in sgx_free_page().
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > 
> > I have a better idea with the same outcome for KVM.
> > 
> > https://lore.kernel.org/linux-sgx/20210111223610.62261-1-jarkko@kernel.org/T/#t
> 
> I agree with your patch this one can be replaced. I'll include your patch in
> next version, and once it is upstreamed, it can be removed in my series.
> 
> Sean, please let me know if you have objection.

6 of one, half dozen of the other.  I liked not having to modify the existing
call sites, but it's your code.

Though on that topic, this snippet is wrong:

@@ -431,7 +443,8 @@ void sgx_encl_release(struct kref *ref)
 		va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
 					   list);
 		list_del(&va_page->list);
-		sgx_free_epc_page(va_page->epc_page);
+		sgx_reset_epc_page(entry->epc_page);
+		sgx_free_epc_page(entry->epc_page);

s/entry/va_page in the new code.

P.S. I apparently hadn't been subscribed linux-sgx and so didn't see those
     patches.  I'm now subscribed and can chime-in as needed.
Huang, Kai Jan. 13, 2021, 1:15 a.m. UTC | #4 
On Tue, 12 Jan 2021 13:45:24 -0800 Sean Christopherson wrote:
> On Tue, Jan 12, 2021, Kai Huang wrote:
> > On Tue, 12 Jan 2021 00:38:40 +0200 Jarkko Sakkinen wrote:
> > > On Wed, 2021-01-06 at 14:55 +1300, Kai Huang wrote:
> > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > 
> > > > SGX virtualization requires to allocate "raw" EPC and use it as virtual
> > > > EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
> > > > track how EPC pages are used in VM, e.g. (de)construction of enclaves,
> > > > so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
> > > > knowledge of which pages are SECS with non-zero child counts.
> > > > 
> > > > Split sgx_free_page() into two parts so that the "add to free list"
> > > > part can be used by virtual EPC without having to modify the EREMOVE
> > > > logic in sgx_free_page().
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > 
> > > I have a better idea with the same outcome for KVM.
> > > 
> > > https://lore.kernel.org/linux-sgx/20210111223610.62261-1-jarkko@kernel.org/T/#t
> > 
> > I agree with your patch this one can be replaced. I'll include your patch in
> > next version, and once it is upstreamed, it can be removed in my series.
> > 
> > Sean, please let me know if you have objection.
> 
> 6 of one, half dozen of the other.  I liked not having to modify the existing
> call sites, but it's your code.
> 
> Though on that topic, this snippet is wrong:
> 
> @@ -431,7 +443,8 @@ void sgx_encl_release(struct kref *ref)
>  		va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
>  					   list);
>  		list_del(&va_page->list);
> -		sgx_free_epc_page(va_page->epc_page);
> +		sgx_reset_epc_page(entry->epc_page);
> +		sgx_free_epc_page(entry->epc_page);
> 
> s/entry/va_page in the new code.
> 
> P.S. I apparently hadn't been subscribed linux-sgx and so didn't see those
>      patches.  I'm now subscribed and can chime-in as needed.

Thanks. I also have replied to Jarkko's v2 patch, and I think you can see it
now.

I think if Jarkko's patch is eventually merged to upstream, we can drop
this patch. So please help to comment if Jarkko's patch is reasonable, since I
don't have history with SGX driver and cannot immediately tell if it is
reasonable.
Jarkko Sakkinen Jan. 13, 2021, 5:05 p.m. UTC | #5 
On Tue, Jan 12, 2021 at 01:45:24PM -0800, Sean Christopherson wrote:
> On Tue, Jan 12, 2021, Kai Huang wrote:
> > On Tue, 12 Jan 2021 00:38:40 +0200 Jarkko Sakkinen wrote:
> > > On Wed, 2021-01-06 at 14:55 +1300, Kai Huang wrote:
> > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > 
> > > > SGX virtualization requires to allocate "raw" EPC and use it as virtual
> > > > EPC for SGX guest.  Unlike EPC used by SGX driver, virtual EPC doesn't
> > > > track how EPC pages are used in VM, e.g. (de)construction of enclaves,
> > > > so it cannot guarantee EREMOVE success, e.g. it doesn't have a priori
> > > > knowledge of which pages are SECS with non-zero child counts.
> > > > 
> > > > Split sgx_free_page() into two parts so that the "add to free list"
> > > > part can be used by virtual EPC without having to modify the EREMOVE
> > > > logic in sgx_free_page().
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > 
> > > I have a better idea with the same outcome for KVM.
> > > 
> > > https://lore.kernel.org/linux-sgx/20210111223610.62261-1-jarkko@kernel.org/T/#t
> > 
> > I agree with your patch this one can be replaced. I'll include your patch in
> > next version, and once it is upstreamed, it can be removed in my series.
> > 
> > Sean, please let me know if you have objection.
> 
> 6 of one, half dozen of the other.  I liked not having to modify the existing
> call sites, but it's your code.

Only the call sites contained sgx_encl_release() require a call to
sgx_reset_epc_page(). That's three in total.

> Though on that topic, this snippet is wrong:
> 
> @@ -431,7 +443,8 @@ void sgx_encl_release(struct kref *ref)
>  		va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
>  					   list);
>  		list_del(&va_page->list);
> -		sgx_free_epc_page(va_page->epc_page);
> +		sgx_reset_epc_page(entry->epc_page);
> +		sgx_free_epc_page(entry->epc_page);

Thanks for the remark.

I checked why I did not see this when running test_sgx. I had not specified
local tree for BuildRoot (LINUX_OVERRIDE_SRCDIR) when building test image.

> s/entry/va_page in the new code.
> 
> P.S. I apparently hadn't been subscribed linux-sgx and so didn't see those
>      patches.  I'm now subscribed and can chime-in as needed.

OK, great, I can also CC directly to the next version.

/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index c519fc5f6948..95aad183bb65 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -594,15 +594,30 @@  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
 	return page;
 }
 
+/**
+ * __sgx_free_epc_page() - Free an EPC page
+ * @page:	pointer to a previously allocated EPC page
+ *
+ * Insert an EPC page back to the list of free pages.
+ */
+void __sgx_free_epc_page(struct sgx_epc_page *page)
+{
+	struct sgx_epc_section *section = &sgx_epc_sections[page->section];
+
+	spin_lock(&section->lock);
+	list_add_tail(&page->list, &section->page_list);
+	section->free_cnt++;
+	spin_unlock(&section->lock);
+}
+
 /**
  * sgx_free_epc_page() - Free an EPC page
- * @page:	an EPC page
+ * @page:	pointer to a previously allocated EPC page
  *
  * Call EREMOVE for an EPC page and insert it back to the list of free pages.
  */
 void sgx_free_epc_page(struct sgx_epc_page *page)
 {
-	struct sgx_epc_section *section = &sgx_epc_sections[page->section];
 	int ret;
 
 	WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
@@ -611,10 +626,7 @@  void sgx_free_epc_page(struct sgx_epc_page *page)
 	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
 		return;
 
-	spin_lock(&section->lock);
-	list_add_tail(&page->list, &section->page_list);
-	section->free_cnt++;
-	spin_unlock(&section->lock);
+	__sgx_free_epc_page(page);
 }
 
 static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 5fa42d143feb..4dddd81cbbc3 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -77,6 +77,7 @@  static inline void *sgx_get_epc_virt_addr(struct sgx_epc_page *page)
 }
 
 struct sgx_epc_page *__sgx_alloc_epc_page(void);
+void __sgx_free_epc_page(struct sgx_epc_page *page);
 void sgx_free_epc_page(struct sgx_epc_page *page);
 
 void sgx_mark_page_reclaimable(struct sgx_epc_page *page);