[PATCH 0/4] Enable automatic SVN updates for SGX enclaves

[PATCH 0/4] Enable automatic SVN updates for SGX enclaves

[Elena Reshetova]

Background
----------

In case an SGX vulnerability is discovered and TCB recovery
for SGX is triggered, Intel specifies a process that must be
followed for a given vulnerability. Steps to mitigate can vary
based on vulnerability type, affected components, etc.
In some cases, a vulnerability can be mitigated via a runtime
recovery flow by shutting down all running SGX enclaves,
clearing enclave page cache (EPC), applying a microcode patch
that does not require a reboot (via late microcode loading) and
restarting all SGX enclaves.


Problem statement
-------------------------
Even when the above-described runtime recovery flow to mitigate the
SGX vulnerability is followed, the SGX attestation evidence will
still reflect the security SVN version being equal to the previous
state of security SVN (containing vulnerability) that created
and managed the enclave until the runtime recovery event. This
limitation currently can be only overcome via a platform reboot,
which negates all the benefits from the rebootless late microcode
loading and not required in this case for functional or security
purposes.


Proposed solution
-----------------

SGX architecture introduced  a new instruction called EUPDATESVN [1]
to Ice Lake. It allows updating security SVN version, given that EPC
is completely empty. The latter is required for security reasons
in order to reason that enclave security posture is as secure as the
security SVN version of the TCB that created it.

This series enables opportunistic execution of EUPDATESVN upon first
EPC page allocation for a first enclave to be run on the platform.

This series is partly based on the previous work done by Cathy Zhang
[2], which attempted to enable forceful destruction of all SGX
enclaves and execution of EUPDATESVN upon successful application of
any microcode patch. This approach is determined as being too
intrusive for the running SGX enclaves, especially taking into account
that it would be performed upon *every* microcode patch application
regardless if it changes the security SVN version or not (change to the
security SVN version is a rare event).

Testing
-------

Tested on EMR machine using kernel-6.14.0_rc7 & sgx selftests.
If Google folks in CC can test on their side, it would be greatly appreciated.


References
----------

[1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
[2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#r2399940e5b10465162529c05e9579b30883849f1

Elena Reshetova (4):
  x86/sgx: Add total number of EPC pages
  x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages
  x86/sgx: Define error codes for ENCLS[EUPDATESVN]
  x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it
    during first EPC page alloc

 arch/x86/include/asm/sgx.h      | 41 ++++++++++-------
 arch/x86/kernel/cpu/sgx/encls.h |  6 +++
 arch/x86/kernel/cpu/sgx/main.c  | 78 ++++++++++++++++++++++++++++++---
 arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
 4 files changed, 107 insertions(+), 20 deletions(-)

-- 
2.45.2

[PATCH 1/4] x86/sgx: Add total number of EPC pages

[Elena Reshetova]

In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty.
SGX already has a variable sgx_nr_free_pages that tracks free
EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
track of total number of EPC pages. It will be used in subsequent
patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
allow an easy check for an empty EPC.

Note: The serialization for sgx_nr_total_pages is not needed because
the variable is only updated during the initialization and there's no
concurrent access.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8ce352fc72ac..d5df67dab247 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -33,6 +33,7 @@ static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
 static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
+static unsigned long sgx_nr_total_pages;
 
 /* Nodes with one or more EPC sections. */
 static nodemask_t sgx_numa_mask;
@@ -648,6 +649,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
 		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
 	}
 
+	sgx_nr_total_pages += nr_pages;
+
 	return true;
 }
 
-- 
2.45.2

[PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Elena Reshetova]

sgx_nr_free_pages is an atomic that is used to keep track of
free EPC pages and detect whenever page reclaiming should start.
Since successful execution of ENCLS[EUPDATESVN] requires empty
EPC and a fast way of checking for this, change this variable
around to indicate number of used pages instead. The subsequent
patch that introduces ENCLS[EUPDATESVN] will take use of this change.

No functional changes intended.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d5df67dab247..b61d3bad0446 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -32,7 +32,7 @@ static DEFINE_XARRAY(sgx_epc_address_space);
 static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
-static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
+static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
 static unsigned long sgx_nr_total_pages;
 
 /* Nodes with one or more EPC sections. */
@@ -379,8 +379,8 @@ static void sgx_reclaim_pages(void)
 
 static bool sgx_should_reclaim(unsigned long watermark)
 {
-	return atomic_long_read(&sgx_nr_free_pages) < watermark &&
-	       !list_empty(&sgx_active_page_list);
+	return (sgx_nr_total_pages - atomic_long_read(&sgx_nr_used_pages))
+	       < watermark && !list_empty(&sgx_active_page_list);
 }
 
 /*
@@ -457,7 +457,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
 	page->flags = 0;
 
 	spin_unlock(&node->lock);
-	atomic_long_dec(&sgx_nr_free_pages);
+	atomic_long_inc(&sgx_nr_used_pages);
 
 	return page;
 }
@@ -617,7 +617,7 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
 	page->flags = SGX_EPC_PAGE_IS_FREE;
 
 	spin_unlock(&node->lock);
-	atomic_long_inc(&sgx_nr_free_pages);
+	atomic_long_dec(&sgx_nr_used_pages);
 }
 
 static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
@@ -851,6 +851,8 @@ static bool __init sgx_page_cache_init(void)
 		return false;
 	}
 
+	atomic_long_set(&sgx_nr_used_pages, sgx_nr_total_pages);
+
 	for_each_online_node(nid) {
 		if (!node_isset(nid, sgx_numa_mask) &&
 		    node_state(nid, N_MEMORY) && node_state(nid, N_CPU))
-- 
2.45.2

[PATCH 3/4] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Elena Reshetova]

Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
process can know the execution state of EUPDATESVN.

Code is from previous submission in https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#m1becf67078dc8c59d454e2e6c6d67ca64db341a4

Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..8ba39bbf4e91 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -73,6 +73,11 @@ enum sgx_encls_function {
  *				public key does not match IA32_SGXLEPUBKEYHASH.
  * %SGX_PAGE_NOT_MODIFIABLE:	The EPC page cannot be modified because it
  *				is in the PENDING or MODIFIED state.
+ * %SGX_INSUFFICIENT_ENTROPY:	Insufficient entropy in RNG.
+ * %SGX_EPC_NOT_READY:		EPC is not ready for SVN update.
+ * %SGX_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
+ *				updated because current SVN was not newer than
+ *				CPUSVN.
  * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
  */
 enum sgx_return_code {
@@ -81,6 +86,9 @@ enum sgx_return_code {
 	SGX_CHILD_PRESENT		= 13,
 	SGX_INVALID_EINITTOKEN		= 16,
 	SGX_PAGE_NOT_MODIFIABLE		= 20,
+	SGX_INSUFFICIENT_ENTROPY	= 29,
+	SGX_EPC_NOT_READY		= 30,
+	SGX_NO_UPDATE			= 31,
 	SGX_UNMASKED_EVENT		= 128,
 };
 
-- 
2.45.2

[PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Elena Reshetova]

SGX architecture introduced  a new instruction called EUPDATESVN [1]
to Ice Lake. It allows updating security SVN version, given that EPC
is completely empty. The latter is required for security reasons
in order to reason that enclave security posture is as secure as the
security SVN version of the TCB that created it.

Additionally it is important to ensure that while ENCLS[EUPDATESVN]
runs, no concurrent page creation happens in EPC, because it might
result in #GP delivered to the creator. Legacy SW might not be prepared
to handle such unexpected #GPs and therefore this patch introduces
a locking mechanism to ensure no concurrent EPC allocations can happen.

It is also ensured that ENCLS[EUPDATESVN] is not called when running
in a VM since it does not have a meaning in this context (microcode
updates application is limited to the host OS) and will create
unnecessary load.

The implementation of ENCLS[EUPDATESVN] is based on previous submision in [2]

[1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
[2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90

Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h      | 33 +++++++++--------
 arch/x86/kernel/cpu/sgx/encls.h |  6 +++
 arch/x86/kernel/cpu/sgx/main.c  | 65 ++++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
 4 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 8ba39bbf4e91..5caf5c31ebc6 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -26,23 +26,26 @@
 #define SGX_CPUID_EPC_SECTION	0x1
 /* The bitmask for the EPC section type. */
 #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
+/* EUPDATESVN presence indication */
+#define SGX_CPUID_EUPDATESVN	BIT(10)
 
 enum sgx_encls_function {
-	ECREATE	= 0x00,
-	EADD	= 0x01,
-	EINIT	= 0x02,
-	EREMOVE	= 0x03,
-	EDGBRD	= 0x04,
-	EDGBWR	= 0x05,
-	EEXTEND	= 0x06,
-	ELDU	= 0x08,
-	EBLOCK	= 0x09,
-	EPA	= 0x0A,
-	EWB	= 0x0B,
-	ETRACK	= 0x0C,
-	EAUG	= 0x0D,
-	EMODPR	= 0x0E,
-	EMODT	= 0x0F,
+	ECREATE		= 0x00,
+	EADD		= 0x01,
+	EINIT		= 0x02,
+	EREMOVE		= 0x03,
+	EDGBRD		= 0x04,
+	EDGBWR		= 0x05,
+	EEXTEND		= 0x06,
+	ELDU		= 0x08,
+	EBLOCK		= 0x09,
+	EPA		= 0x0A,
+	EWB		= 0x0B,
+	ETRACK		= 0x0C,
+	EAUG		= 0x0D,
+	EMODPR		= 0x0E,
+	EMODT		= 0x0F,
+	EUPDATESVN	= 0x18,
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 99004b02e2ed..3d83c76dc91f 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
 	return __encls_2(EAUG, pginfo, addr);
 }
 
+/* Update CPUSVN at runtime. */
+static inline int __eupdatesvn(void)
+{
+	return __encls_ret_1(EUPDATESVN, "");
+}
+
 #endif /* _X86_ENCLS_H */
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index b61d3bad0446..698921229094 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space);
 static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
 
+/* This lock is held to prevent new EPC pages from being created
+ * during the execution of ENCLS[EUPDATESVN].
+ */
+static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
+
 static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
 static unsigned long sgx_nr_total_pages;
 
@@ -457,7 +462,17 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
 	page->flags = 0;
 
 	spin_unlock(&node->lock);
-	atomic_long_inc(&sgx_nr_used_pages);
+
+	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
+		spin_lock(&sgx_epc_eupdatesvn_lock);
+		/* Only call sgx_updatesvn() once the first enclave's
+		 * page is allocated from EPC
+		 */
+		if (atomic_long_read(&sgx_nr_used_pages) == 0)
+			sgx_updatesvn();
+		atomic_long_inc(&sgx_nr_used_pages);
+		spin_unlock(&sgx_epc_eupdatesvn_lock);
+	}
 
 	return page;
 }
@@ -970,3 +985,51 @@ static int __init sgx_init(void)
 }
 
 device_initcall(sgx_init);
+
+/**
+ * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
+ * If EPC is ready, this instruction will update CPUSVN to the currently
+ * loaded microcode update SVN and generate new cryptographic assets.
+ */
+void sgx_updatesvn(void)
+{
+	int ret;
+	int retry = 10;
+
+	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
+
+	/* Do not execute EUPDATESVN if instruction is unavalible or running in a VM */
+	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) ||
+		    boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return;
+
+	do {
+		ret = __eupdatesvn();
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+
+	} while (--retry);
+
+	switch (ret) {
+	case 0:
+		pr_debug("EUPDATESVN was successful!\n");
+		break;
+	case SGX_NO_UPDATE:
+		pr_debug("EUPDATESVN was successful, but CPUSVN was not updated, "
+			"because current SVN was not newer than CPUSVN.\n");
+		break;
+	case SGX_EPC_NOT_READY:
+		pr_debug("EPC is not ready for SVN update.");
+		break;
+	case SGX_INSUFFICIENT_ENTROPY:
+		pr_debug("CPUSVN update is failed due to Insufficient entropy in RNG, "
+			"please try it later.\n");
+		break;
+	case SGX_EPC_PAGE_CONFLICT:
+		pr_debug("CPUSVN update is failed due to concurrency violation, please "
+			"stop running any other ENCLS leaf and try it later.\n");
+		break;
+	default:
+		break;
+	}
+}
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index d2dad21259a8..92c9e226f1b5 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void)
 
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
+void sgx_updatesvn(void);
+
 #endif /* _X86_SGX_H */
-- 
2.45.2

Re: [PATCH 3/4] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Fri, Mar 21, 2025 at 02:34:42PM +0200, Elena Reshetova wrote:
> Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> process can know the execution state of EUPDATESVN.
> 

Enumerate the error codes. Do we need all of the three added?

> Code is from previous submission in https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#m1becf67078dc8c59d454e2e6c6d67ca64db341a4
"Link: https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#m1becf67078dc8c59d454e2e6c6d67ca64db341a4"

> 
> Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

> Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
Should be removed:
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..8ba39bbf4e91 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -73,6 +73,11 @@ enum sgx_encls_function {
>   *				public key does not match IA32_SGXLEPUBKEYHASH.
>   * %SGX_PAGE_NOT_MODIFIABLE:	The EPC page cannot be modified because it
>   *				is in the PENDING or MODIFIED state.
> + * %SGX_INSUFFICIENT_ENTROPY:	Insufficient entropy in RNG.
> + * %SGX_EPC_NOT_READY:		EPC is not ready for SVN update.
> + * %SGX_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
> + *				updated because current SVN was not newer than
> + *				CPUSVN.
>   * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
>   */
>  enum sgx_return_code {
> @@ -81,6 +86,9 @@ enum sgx_return_code {
>  	SGX_CHILD_PRESENT		= 13,
>  	SGX_INVALID_EINITTOKEN		= 16,
>  	SGX_PAGE_NOT_MODIFIABLE		= 20,
> +	SGX_INSUFFICIENT_ENTROPY	= 29,
> +	SGX_EPC_NOT_READY		= 30,
>  +	SGX_NO_UPDATE			= 31,



>  	SGX_UNMASKED_EVENT		= 128,
>  };
>  
> -- 
> 2.45.2
> 
> 

BR, Jarkko

Re: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Jarkko Sakkinen]

On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty.
> SGX already has a variable sgx_nr_free_pages that tracks free
> EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> track of total number of EPC pages. It will be used in subsequent
> patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> allow an easy check for an empty EPC.

First off, remove "in subsequent patch".

What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean?

> 
> Note: The serialization for sgx_nr_total_pages is not needed because
> the variable is only updated during the initialization and there's no
> concurrent access.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8ce352fc72ac..d5df67dab247 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -33,6 +33,7 @@ static LIST_HEAD(sgx_active_page_list);
>  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
>  
>  static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
> +static unsigned long sgx_nr_total_pages;
>  
>  /* Nodes with one or more EPC sections. */
>  static nodemask_t sgx_numa_mask;
> @@ -648,6 +649,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>  	}
>  
> +	sgx_nr_total_pages += nr_pages;
> +
>  	return true;
>  }
>  
> -- 
> 2.45.2
> 

BR, Jarkko

Re: [PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Jarkko Sakkinen]

On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote:
> sgx_nr_free_pages is an atomic that is used to keep track of
> free EPC pages and detect whenever page reclaiming should start.
> Since successful execution of ENCLS[EUPDATESVN] requires empty
> EPC and a fast way of checking for this, change this variable
> around to indicate number of used pages instead. The subsequent
> patch that introduces ENCLS[EUPDATESVN] will take use of this change.

s/subsequent patch//

You should rather express how EUPDATESVN trigger will depend on the
state of sgx_nr_used_pages and sgx_nr_free_pages.

> 
> No functional changes intended.

Not really understanding how I should interpret this sentence.

The commit message does not mention sgx_nr_used_pages, and neiher it
makes a case why implementing the feature based on sgx_nr_free_pages is
not possible.

BR, Jarkko

Re: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Jarkko Sakkinen]

On Fri, Mar 21, 2025 at 02:34:43PM +0200, Elena Reshetova wrote:
> SGX architecture introduced  a new instruction called EUPDATESVN [1]
> to Ice Lake. It allows updating security SVN version, given that EPC
> is completely empty. The latter is required for security reasons
> in order to reason that enclave security posture is as secure as the
> security SVN version of the TCB that created it.
> 
> Additionally it is important to ensure that while ENCLS[EUPDATESVN]
> runs, no concurrent page creation happens in EPC, because it might
> result in #GP delivered to the creator. Legacy SW might not be prepared
> to handle such unexpected #GPs and therefore this patch introduces
> a locking mechanism to ensure no concurrent EPC allocations can happen.
> 
> It is also ensured that ENCLS[EUPDATESVN] is not called when running
> in a VM since it does not have a meaning in this context (microcode
> updates application is limited to the host OS) and will create
> unnecessary load.
> 
> The implementation of ENCLS[EUPDATESVN] is based on previous submision in [2]
> 
> [1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true

I don't think for Intel opcodes a link is needed. We know where to look
them up.

> [2] https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90

Link:

> 
> Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
> Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h      | 33 +++++++++--------
>  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 65 ++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
>  4 files changed, 90 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 8ba39bbf4e91..5caf5c31ebc6 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -26,23 +26,26 @@
>  #define SGX_CPUID_EPC_SECTION	0x1
>  /* The bitmask for the EPC section type. */
>  #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
> +/* EUPDATESVN presence indication */
> +#define SGX_CPUID_EUPDATESVN	BIT(10)
>  
>  enum sgx_encls_function {
> -	ECREATE	= 0x00,
> -	EADD	= 0x01,
> -	EINIT	= 0x02,
> -	EREMOVE	= 0x03,
> -	EDGBRD	= 0x04,
> -	EDGBWR	= 0x05,
> -	EEXTEND	= 0x06,
> -	ELDU	= 0x08,
> -	EBLOCK	= 0x09,
> -	EPA	= 0x0A,
> -	EWB	= 0x0B,
> -	ETRACK	= 0x0C,
> -	EAUG	= 0x0D,
> -	EMODPR	= 0x0E,
> -	EMODT	= 0x0F,
> +	ECREATE		= 0x00,
> +	EADD		= 0x01,
> +	EINIT		= 0x02,
> +	EREMOVE		= 0x03,
> +	EDGBRD		= 0x04,
> +	EDGBWR		= 0x05,
> +	EEXTEND		= 0x06,
> +	ELDU		= 0x08,
> +	EBLOCK		= 0x09,
> +	EPA		= 0x0A,
> +	EWB		= 0x0B,
> +	ETRACK		= 0x0C,
> +	EAUG		= 0x0D,
> +	EMODPR		= 0x0E,
> +	EMODT		= 0x0F,
> +	EUPDATESVN	= 0x18,
>  };
>  
>  /**
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..3d83c76dc91f 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
>  	return __encls_2(EAUG, pginfo, addr);
>  }
>  
> +/* Update CPUSVN at runtime. */
> +static inline int __eupdatesvn(void)
> +{
> +	return __encls_ret_1(EUPDATESVN, "");
> +}
> +
>  #endif /* _X86_ENCLS_H */
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index b61d3bad0446..698921229094 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space);
>  static LIST_HEAD(sgx_active_page_list);
>  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
>  
> +/* This lock is held to prevent new EPC pages from being created
> + * during the execution of ENCLS[EUPDATESVN].
> + */
> +static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
> +
>  static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
>  static unsigned long sgx_nr_total_pages;
>  
> @@ -457,7 +462,17 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  	page->flags = 0;
>  
>  	spin_unlock(&node->lock);
> -	atomic_long_inc(&sgx_nr_used_pages);
> +
> +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
> +		spin_lock(&sgx_epc_eupdatesvn_lock);
> +		/* Only call sgx_updatesvn() once the first enclave's
> +		 * page is allocated from EPC
> +		 */
> +		if (atomic_long_read(&sgx_nr_used_pages) == 0)
> +			sgx_updatesvn();
> +		atomic_long_inc(&sgx_nr_used_pages);
> +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> +	}
>  
>  	return page;
>  }
> @@ -970,3 +985,51 @@ static int __init sgx_init(void)
>  }
>  
>  device_initcall(sgx_init);
> +
> +/**
> + * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
> + * If EPC is ready, this instruction will update CPUSVN to the currently
> + * loaded microcode update SVN and generate new cryptographic assets.
> + */
> +void sgx_updatesvn(void)
> +{
> +	int ret;
> +	int retry = 10;

Reverse declaration order.

> +
> +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> +
> +	/* Do not execute EUPDATESVN if instruction is unavalible or running in a VM */
> +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) ||
> +		    boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		return;


	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
		return;

	/* ... */
	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
		return;

The first check really does not need a comment. The second would benefit
from explaining why bail out inside a VM.
		
		



> +
> +	do {
> +		ret = __eupdatesvn();
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +
> +	} while (--retry);
> +
> +	switch (ret) {
> +	case 0:
> +		pr_debug("EUPDATESVN was successful!\n");
> +		break;
> +	case SGX_NO_UPDATE:
> +		pr_debug("EUPDATESVN was successful, but CPUSVN was not updated, "
> +			"because current SVN was not newer than CPUSVN.\n");
> +		break;
> +	case SGX_EPC_NOT_READY:
> +		pr_debug("EPC is not ready for SVN update.");
> +		break;
> +	case SGX_INSUFFICIENT_ENTROPY:
> +		pr_debug("CPUSVN update is failed due to Insufficient entropy in RNG, "
> +			"please try it later.\n");
> +		break;
> +	case SGX_EPC_PAGE_CONFLICT:
> +		pr_debug("CPUSVN update is failed due to concurrency violation, please "
> +			"stop running any other ENCLS leaf and try it later.\n");
> +		break;
> +	default:
> +		break;

Remove pr_debug() statements.

> +	}
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..92c9e226f1b5 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void)
>  
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
>  

I don't think we need a newline in-between.

> +void sgx_updatesvn(void);
> +
>  #endif /* _X86_SGX_H */
> -- 
> 2.45.2
> 

BR, Jarkko

RE: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Reshetova, Elena]

> On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> > In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty.
> > SGX already has a variable sgx_nr_free_pages that tracks free
> > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> > track of total number of EPC pages. It will be used in subsequent
> > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> > allow an easy check for an empty EPC.
> 
> First off, remove "in subsequent patch".

Ok

> 
> What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean?

As you can see from patch 2/4, I had to turn around the meaning of the
existing sgx_nr_free_pages atomic counter not to count the # of free pages
in EPC, but to count the # of used EPC pages (hence the change of name
to sgx_nr_used_pages). The reason for doing this is only apparent in patch
4/4 because by having a counter sgx_nr_used_pages  incremented in the
atomic_long_inc_not_zero, there is a fast path that avoids taking any locks
in cases when the EPC page is not the first one to be created (most of cases).
I originally created a version with just using sgx_nr_free_pages, but could
not avoided taking a lock in each case and it did look much less pretty than
this version. The credit for the idea btw goes to Kirill who kindly reviewed
my  patches before. 

Best Regards,
Elena.

RE: [PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Reshetova, Elena]

 
> On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote:
> > sgx_nr_free_pages is an atomic that is used to keep track of
> > free EPC pages and detect whenever page reclaiming should start.
> > Since successful execution of ENCLS[EUPDATESVN] requires empty
> > EPC and a fast way of checking for this, change this variable
> > around to indicate number of used pages instead. The subsequent
> > patch that introduces ENCLS[EUPDATESVN] will take use of this change.
> 
> s/subsequent patch//

Ok

> 
> You should rather express how EUPDATESVN trigger will depend on the
> state of sgx_nr_used_pages and sgx_nr_free_pages.

How about this explanation:

"By counting the # of used pages instead of #of free pages, it allows the
EPC page allocation path execute without a need to take the lock in all
but a single case when the first page is being allocated in EPC. This is
achieved via a fast check in atomic_long_inc_not_zero."

Also, if you think that it is hard to interpret the patch 2/4 without 4/4
I can also squeeze them together and then it becomes right away clear
why the change was done. 


> 
> >
> > No functional changes intended.
> 
> Not really understanding how I should interpret this sentence.

Just as usual: this patch itself doesn’t bring any functional changes
to the way as current SGX code works. I only needed this change to
implement patch 4/4 in more lockless way. 

> 
> The commit message does not mention sgx_nr_used_pages, and neiher it
> makes a case why implementing the feature based on sgx_nr_free_pages is
> not possible.

It is possible to implement it, in fact I did exactly this in the beginning instead,
but as mentioned previously this would have resulted in taking a lock for each
case the page is being allocated. 

Best Regards,
Elena.

RE: [PATCH 3/4] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Reshetova, Elena]

> On Fri, Mar 21, 2025 at 02:34:42PM +0200, Elena Reshetova wrote:
> > Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> > process can know the execution state of EUPDATESVN.
> >
> 
> Enumerate the error codes. 

You mean in the commit message or? 


> Do we need all of the three added?

Yes, we do. They are all valid error codes that can be received as a result of
execution of EUPDATESVN. 

Best Regards,
Elena.

RE: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Reshetova, Elena]

> On Fri, Mar 21, 2025 at 02:34:43PM +0200, Elena Reshetova wrote:
> > SGX architecture introduced  a new instruction called EUPDATESVN [1]
> > to Ice Lake. It allows updating security SVN version, given that EPC
> > is completely empty. The latter is required for security reasons
> > in order to reason that enclave security posture is as secure as the
> > security SVN version of the TCB that created it.
> >
> > Additionally it is important to ensure that while ENCLS[EUPDATESVN]
> > runs, no concurrent page creation happens in EPC, because it might
> > result in #GP delivered to the creator. Legacy SW might not be prepared
> > to handle such unexpected #GPs and therefore this patch introduces
> > a locking mechanism to ensure no concurrent EPC allocations can happen.
> >
> > It is also ensured that ENCLS[EUPDATESVN] is not called when running
> > in a VM since it does not have a meaning in this context (microcode
> > updates application is limited to the host OS) and will create
> > unnecessary load.
> >
> > The implementation of ENCLS[EUPDATESVN] is based on previous
> submision in [2]
> >
> > [1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
> 
> I don't think for Intel opcodes a link is needed. We know where to look
> them up.

Ok, I can drop this reference. It is just a whitepaper explaining to readers
the background and operation of EUPDATESVN. It is not part of standard
SDM, so I thought I would put a link. 


> 
> > [2] https://lore.kernel.org/all/20220520103904.1216-1-
> cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90
> 
> Link:

?  Not sure what you mean by it. 

> 
> >
> > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
> > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/include/asm/sgx.h      | 33 +++++++++--------
> >  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
> >  arch/x86/kernel/cpu/sgx/main.c  | 65
> ++++++++++++++++++++++++++++++++-
> >  arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
> >  4 files changed, 90 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > index 8ba39bbf4e91..5caf5c31ebc6 100644
> > --- a/arch/x86/include/asm/sgx.h
> > +++ b/arch/x86/include/asm/sgx.h
> > @@ -26,23 +26,26 @@
> >  #define SGX_CPUID_EPC_SECTION	0x1
> >  /* The bitmask for the EPC section type. */
> >  #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
> > +/* EUPDATESVN presence indication */
> > +#define SGX_CPUID_EUPDATESVN	BIT(10)
> >
> >  enum sgx_encls_function {
> > -	ECREATE	= 0x00,
> > -	EADD	= 0x01,
> > -	EINIT	= 0x02,
> > -	EREMOVE	= 0x03,
> > -	EDGBRD	= 0x04,
> > -	EDGBWR	= 0x05,
> > -	EEXTEND	= 0x06,
> > -	ELDU	= 0x08,
> > -	EBLOCK	= 0x09,
> > -	EPA	= 0x0A,
> > -	EWB	= 0x0B,
> > -	ETRACK	= 0x0C,
> > -	EAUG	= 0x0D,
> > -	EMODPR	= 0x0E,
> > -	EMODT	= 0x0F,
> > +	ECREATE		= 0x00,
> > +	EADD		= 0x01,
> > +	EINIT		= 0x02,
> > +	EREMOVE		= 0x03,
> > +	EDGBRD		= 0x04,
> > +	EDGBWR		= 0x05,
> > +	EEXTEND		= 0x06,
> > +	ELDU		= 0x08,
> > +	EBLOCK		= 0x09,
> > +	EPA		= 0x0A,
> > +	EWB		= 0x0B,
> > +	ETRACK		= 0x0C,
> > +	EAUG		= 0x0D,
> > +	EMODPR		= 0x0E,
> > +	EMODT		= 0x0F,
> > +	EUPDATESVN	= 0x18,
> >  };
> >
> >  /**
> > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> b/arch/x86/kernel/cpu/sgx/encls.h
> > index 99004b02e2ed..3d83c76dc91f 100644
> > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > @@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo,
> void *addr)
> >  	return __encls_2(EAUG, pginfo, addr);
> >  }
> >
> > +/* Update CPUSVN at runtime. */
> > +static inline int __eupdatesvn(void)
> > +{
> > +	return __encls_ret_1(EUPDATESVN, "");
> > +}
> > +
> >  #endif /* _X86_ENCLS_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> b/arch/x86/kernel/cpu/sgx/main.c
> > index b61d3bad0446..698921229094 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space);
> >  static LIST_HEAD(sgx_active_page_list);
> >  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> >
> > +/* This lock is held to prevent new EPC pages from being created
> > + * during the execution of ENCLS[EUPDATESVN].
> > + */
> > +static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
> > +
> >  static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
> >  static unsigned long sgx_nr_total_pages;
> >
> > @@ -457,7 +462,17 @@ static struct sgx_epc_page
> *__sgx_alloc_epc_page_from_node(int nid)
> >  	page->flags = 0;
> >
> >  	spin_unlock(&node->lock);
> > -	atomic_long_inc(&sgx_nr_used_pages);
> > +
> > +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
> > +		spin_lock(&sgx_epc_eupdatesvn_lock);
> > +		/* Only call sgx_updatesvn() once the first enclave's
> > +		 * page is allocated from EPC
> > +		 */
> > +		if (atomic_long_read(&sgx_nr_used_pages) == 0)
> > +			sgx_updatesvn();
> > +		atomic_long_inc(&sgx_nr_used_pages);
> > +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> > +	}
> >
> >  	return page;
> >  }
> > @@ -970,3 +985,51 @@ static int __init sgx_init(void)
> >  }
> >
> >  device_initcall(sgx_init);
> > +
> > +/**
> > + * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
> > + * If EPC is ready, this instruction will update CPUSVN to the currently
> > + * loaded microcode update SVN and generate new cryptographic assets.
> > + */
> > +void sgx_updatesvn(void)
> > +{
> > +	int ret;
> > +	int retry = 10;
> 
> Reverse declaration order.

Sure, will fix. 

> 
> > +
> > +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> > +
> > +	/* Do not execute EUPDATESVN if instruction is unavalible or running
> in a VM */
> > +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) ||
> > +		    boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > +		return;
> 
> 
> 	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> 		return;
> 
> 	/* ... */
> 	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> 		return;
> 
> The first check really does not need a comment. The second would benefit
> from explaining why bail out inside a VM.

Sure, I can change. The reason why we dont want the execution in a VM is explained
in the commit message, I can duplicate it in the code comment also. 


> 
> 
> 
> 
> 
> > +
> > +	do {
> > +		ret = __eupdatesvn();
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +
> > +	} while (--retry);
> > +
> > +	switch (ret) {
> > +	case 0:
> > +		pr_debug("EUPDATESVN was successful!\n");
> > +		break;
> > +	case SGX_NO_UPDATE:
> > +		pr_debug("EUPDATESVN was successful, but CPUSVN was not
> updated, "
> > +			"because current SVN was not newer than
> CPUSVN.\n");
> > +		break;
> > +	case SGX_EPC_NOT_READY:
> > +		pr_debug("EPC is not ready for SVN update.");
> > +		break;
> > +	case SGX_INSUFFICIENT_ENTROPY:
> > +		pr_debug("CPUSVN update is failed due to Insufficient
> entropy in RNG, "
> > +			"please try it later.\n");
> > +		break;
> > +	case SGX_EPC_PAGE_CONFLICT:
> > +		pr_debug("CPUSVN update is failed due to concurrency
> violation, please "
> > +			"stop running any other ENCLS leaf and try it
> later.\n");
> > +		break;
> > +	default:
> > +		break;
> 
> Remove pr_debug() statements.

This I am not sure it is good idea. I think it would be useful for system
admins to have a way to see that update either happened or not.
It is true that you can find this out by requesting a new SGX attestation
quote (and see if newer SVN is used), but it is not the faster way. 



> 
> > +	}
> > +}
> > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> > index d2dad21259a8..92c9e226f1b5 100644
> > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > @@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void)
> >
> >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> >
> 
> I don't think we need a newline in-between.

Sure, will fix.

Thank you very much for your prompt review Jarkko!

Best Regards,
Elena.

Re: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Jarkko Sakkinen]

On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote:
> > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty.
> > > SGX already has a variable sgx_nr_free_pages that tracks free
> > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> > > track of total number of EPC pages. It will be used in subsequent
> > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> > > allow an easy check for an empty EPC.
> > 
> > First off, remove "in subsequent patch".
> 
> Ok
> 
> > 
> > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean?
> 
> As you can see from patch 2/4, I had to turn around the meaning of the
> existing sgx_nr_free_pages atomic counter not to count the # of free pages
> in EPC, but to count the # of used EPC pages (hence the change of name
> to sgx_nr_used_pages). The reason for doing this is only apparent in patch

Why you *absolutely* need to invert the meaning and cannot make
this work by any means otherwise?

I doubt highly doubt this could not be done other way around.

BR, Jarkko 

Re: [PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Jarkko Sakkinen]

On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote:
>  
> > On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote:
> > > sgx_nr_free_pages is an atomic that is used to keep track of
> > > free EPC pages and detect whenever page reclaiming should start.
> > > Since successful execution of ENCLS[EUPDATESVN] requires empty
> > > EPC and a fast way of checking for this, change this variable
> > > around to indicate number of used pages instead. The subsequent
> > > patch that introduces ENCLS[EUPDATESVN] will take use of this change.
> > 
> > s/subsequent patch//
> 
> Ok
> 
> > 
> > You should rather express how EUPDATESVN trigger will depend on the
> > state of sgx_nr_used_pages and sgx_nr_free_pages.
> 
> How about this explanation:
> 
> "By counting the # of used pages instead of #of free pages, it allows the
> EPC page allocation path execute without a need to take the lock in all
> but a single case when the first page is being allocated in EPC. This is
> achieved via a fast check in atomic_long_inc_not_zero."

Yep, whole a lot more sense.

> 
> Also, if you think that it is hard to interpret the patch 2/4 without 4/4
> I can also squeeze them together and then it becomes right away clear
> why the change was done. 
> 
> 
> > 
> > >
> > > No functional changes intended.
> > 
> > Not really understanding how I should interpret this sentence.
> 
> Just as usual: this patch itself doesn’t bring any functional changes
> to the way as current SGX code works. I only needed this change to
> implement patch 4/4 in more lockless way. 
> 
> > 
> > The commit message does not mention sgx_nr_used_pages, and neiher it
> > makes a case why implementing the feature based on sgx_nr_free_pages is
> > not possible.
> 
> It is possible to implement it, in fact I did exactly this in the beginning instead,
> but as mentioned previously this would have resulted in taking a lock for each
> case the page is being allocated. 

Have you benchmarked this (memory barrier vs putting the whole thing
inside spinlock)?

I have doubts that this would even show in margins given how much e.g.,
ELDU takes.

> 
> Best Regards,
> Elena.
> 
> 

BR, Jarkko

Re: [PATCH 3/4] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Mon, Mar 24, 2025 at 12:21:14PM +0000, Reshetova, Elena wrote:
> 
> 
> > On Fri, Mar 21, 2025 at 02:34:42PM +0200, Elena Reshetova wrote:
> > > Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> > > process can know the execution state of EUPDATESVN.
> > >
> > 
> > Enumerate the error codes. 
> 
> You mean in the commit message or? 

I'm not really sure how kernel uses them.

> 
> 
> > Do we need all of the three added?
> 
> Yes, we do. They are all valid error codes that can be received as a result of
> execution of EUPDATESVN. 
> 
> Best Regards,
> Elena.

BR, Jarkko

Re: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Jarkko Sakkinen]

On Mon, Mar 24, 2025 at 12:26:38PM +0000, Reshetova, Elena wrote:
> 
> > On Fri, Mar 21, 2025 at 02:34:43PM +0200, Elena Reshetova wrote:
> > > SGX architecture introduced  a new instruction called EUPDATESVN [1]
> > > to Ice Lake. It allows updating security SVN version, given that EPC
> > > is completely empty. The latter is required for security reasons
> > > in order to reason that enclave security posture is as secure as the
> > > security SVN version of the TCB that created it.
> > >
> > > Additionally it is important to ensure that while ENCLS[EUPDATESVN]
> > > runs, no concurrent page creation happens in EPC, because it might
> > > result in #GP delivered to the creator. Legacy SW might not be prepared
> > > to handle such unexpected #GPs and therefore this patch introduces
> > > a locking mechanism to ensure no concurrent EPC allocations can happen.
> > >
> > > It is also ensured that ENCLS[EUPDATESVN] is not called when running
> > > in a VM since it does not have a meaning in this context (microcode
> > > updates application is limited to the host OS) and will create
> > > unnecessary load.
> > >
> > > The implementation of ENCLS[EUPDATESVN] is based on previous
> > submision in [2]
> > >
> > > [1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
> > 
> > I don't think for Intel opcodes a link is needed. We know where to look
> > them up.
> 
> Ok, I can drop this reference. It is just a whitepaper explaining to readers
> the background and operation of EUPDATESVN. It is not part of standard
> SDM, so I thought I would put a link. 
> 
> 
> > 
> > > [2] https://lore.kernel.org/all/20220520103904.1216-1-
> > cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90
> > 
> > Link:
> 
> ?  Not sure what you mean by it. 
> 
> > 
> > >
> > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
> > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > ---
> > >  arch/x86/include/asm/sgx.h      | 33 +++++++++--------
> > >  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
> > >  arch/x86/kernel/cpu/sgx/main.c  | 65
> > ++++++++++++++++++++++++++++++++-
> > >  arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
> > >  4 files changed, 90 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > > index 8ba39bbf4e91..5caf5c31ebc6 100644
> > > --- a/arch/x86/include/asm/sgx.h
> > > +++ b/arch/x86/include/asm/sgx.h
> > > @@ -26,23 +26,26 @@
> > >  #define SGX_CPUID_EPC_SECTION	0x1
> > >  /* The bitmask for the EPC section type. */
> > >  #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
> > > +/* EUPDATESVN presence indication */
> > > +#define SGX_CPUID_EUPDATESVN	BIT(10)
> > >
> > >  enum sgx_encls_function {
> > > -	ECREATE	= 0x00,
> > > -	EADD	= 0x01,
> > > -	EINIT	= 0x02,
> > > -	EREMOVE	= 0x03,
> > > -	EDGBRD	= 0x04,
> > > -	EDGBWR	= 0x05,
> > > -	EEXTEND	= 0x06,
> > > -	ELDU	= 0x08,
> > > -	EBLOCK	= 0x09,
> > > -	EPA	= 0x0A,
> > > -	EWB	= 0x0B,
> > > -	ETRACK	= 0x0C,
> > > -	EAUG	= 0x0D,
> > > -	EMODPR	= 0x0E,
> > > -	EMODT	= 0x0F,
> > > +	ECREATE		= 0x00,
> > > +	EADD		= 0x01,
> > > +	EINIT		= 0x02,
> > > +	EREMOVE		= 0x03,
> > > +	EDGBRD		= 0x04,
> > > +	EDGBWR		= 0x05,
> > > +	EEXTEND		= 0x06,
> > > +	ELDU		= 0x08,
> > > +	EBLOCK		= 0x09,
> > > +	EPA		= 0x0A,
> > > +	EWB		= 0x0B,
> > > +	ETRACK		= 0x0C,
> > > +	EAUG		= 0x0D,
> > > +	EMODPR		= 0x0E,
> > > +	EMODT		= 0x0F,
> > > +	EUPDATESVN	= 0x18,
> > >  };
> > >
> > >  /**
> > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > b/arch/x86/kernel/cpu/sgx/encls.h
> > > index 99004b02e2ed..3d83c76dc91f 100644
> > > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > > @@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo *pginfo,
> > void *addr)
> > >  	return __encls_2(EAUG, pginfo, addr);
> > >  }
> > >
> > > +/* Update CPUSVN at runtime. */
> > > +static inline int __eupdatesvn(void)
> > > +{
> > > +	return __encls_ret_1(EUPDATESVN, "");
> > > +}
> > > +
> > >  #endif /* _X86_ENCLS_H */
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c
> > > index b61d3bad0446..698921229094 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space);
> > >  static LIST_HEAD(sgx_active_page_list);
> > >  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> > >
> > > +/* This lock is held to prevent new EPC pages from being created
> > > + * during the execution of ENCLS[EUPDATESVN].
> > > + */
> > > +static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
> > > +
> > >  static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
> > >  static unsigned long sgx_nr_total_pages;
> > >
> > > @@ -457,7 +462,17 @@ static struct sgx_epc_page
> > *__sgx_alloc_epc_page_from_node(int nid)
> > >  	page->flags = 0;
> > >
> > >  	spin_unlock(&node->lock);
> > > -	atomic_long_inc(&sgx_nr_used_pages);
> > > +
> > > +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
> > > +		spin_lock(&sgx_epc_eupdatesvn_lock);
> > > +		/* Only call sgx_updatesvn() once the first enclave's
> > > +		 * page is allocated from EPC
> > > +		 */
> > > +		if (atomic_long_read(&sgx_nr_used_pages) == 0)
> > > +			sgx_updatesvn();
> > > +		atomic_long_inc(&sgx_nr_used_pages);
> > > +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> > > +	}
> > >
> > >  	return page;
> > >  }
> > > @@ -970,3 +985,51 @@ static int __init sgx_init(void)
> > >  }
> > >
> > >  device_initcall(sgx_init);
> > > +
> > > +/**
> > > + * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
> > > + * If EPC is ready, this instruction will update CPUSVN to the currently
> > > + * loaded microcode update SVN and generate new cryptographic assets.
> > > + */
> > > +void sgx_updatesvn(void)
> > > +{
> > > +	int ret;
> > > +	int retry = 10;
> > 
> > Reverse declaration order.
> 
> Sure, will fix. 
> 
> > 
> > > +
> > > +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> > > +
> > > +	/* Do not execute EUPDATESVN if instruction is unavalible or running
> > in a VM */
> > > +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) ||
> > > +		    boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > > +		return;
> > 
> > 
> > 	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> > 		return;
> > 
> > 	/* ... */
> > 	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > 		return;
> > 
> > The first check really does not need a comment. The second would benefit
> > from explaining why bail out inside a VM.
> 
> Sure, I can change. The reason why we dont want the execution in a VM is explained
> in the commit message, I can duplicate it in the code comment also. 
> 
> 
> > 
> > 
> > 
> > 
> > 
> > > +
> > > +	do {
> > > +		ret = __eupdatesvn();
> > > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > > +			break;
> > > +
> > > +	} while (--retry);
> > > +
> > > +	switch (ret) {
> > > +	case 0:
> > > +		pr_debug("EUPDATESVN was successful!\n");
> > > +		break;

This is at least definite removal. We don't log for zero codes.

> > > +	case SGX_NO_UPDATE:
> > > +		pr_debug("EUPDATESVN was successful, but CPUSVN was not
> > updated, "
> > > +			"because current SVN was not newer than
> > CPUSVN.\n");
> > > +		break;
> > > +	case SGX_EPC_NOT_READY:
> > > +		pr_debug("EPC is not ready for SVN update.");
> > > +		break;
> > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > +		pr_debug("CPUSVN update is failed due to Insufficient
> > entropy in RNG, "
> > > +			"please try it later.\n");
> > > +		break;
> > > +	case SGX_EPC_PAGE_CONFLICT:
> > > +		pr_debug("CPUSVN update is failed due to concurrency
> > violation, please "
> > > +			"stop running any other ENCLS leaf and try it
> > later.\n");
> > > +		break;
> > > +	default:
> > > +		break;
> > 
> > Remove pr_debug() statements.
> 
> This I am not sure it is good idea. I think it would be useful for system
> admins to have a way to see that update either happened or not.
> It is true that you can find this out by requesting a new SGX attestation
> quote (and see if newer SVN is used), but it is not the faster way. 

Maybe pr_debug() is them wrong level if they are meant for sysadmins?

I mean these should not happen in normal behavior like ever? As
pr_debug() I don't really grab this.

> 
> 
> 
> > 
> > > +	}
> > > +}
> > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> > > index d2dad21259a8..92c9e226f1b5 100644
> > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > @@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void)
> > >
> > >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> > >
> > 
> > I don't think we need a newline in-between.
> 
> Sure, will fix.
> 
> Thank you very much for your prompt review Jarkko!

NP, despite all the complains this are really just minor details
that we need to nail :-) I'm sure we get them right within round
or two...

> 
> Best Regards,
> Elena.

BR, Jarkko

RE: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Reshetova, Elena]

> On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote:
> > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be
> empty.
> > > > SGX already has a variable sgx_nr_free_pages that tracks free
> > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> > > > track of total number of EPC pages. It will be used in subsequent
> > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> > > > allow an easy check for an empty EPC.
> > >
> > > First off, remove "in subsequent patch".
> >
> > Ok
> >
> > >
> > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean?
> >
> > As you can see from patch 2/4, I had to turn around the meaning of the
> > existing sgx_nr_free_pages atomic counter not to count the # of free pages
> > in EPC, but to count the # of used EPC pages (hence the change of name
> > to sgx_nr_used_pages). The reason for doing this is only apparent in patch
> 
> Why you *absolutely* need to invert the meaning and cannot make
> this work by any means otherwise?
> 
> I doubt highly doubt this could not be done other way around.

I can make it work. The point that this way is much better and no damage to
existing logic is done. The sgx_nr_free_pages counter that is used only for page reclaiming
and checked in a single piece of code.
To give you an idea the previous iteration of the code looked like below.
First, I had to define a new unconditional spinlock to protect the EPC page allocation:

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index c8a2542140a1..4f445c28929b 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space);
  */
 static LIST_HEAD(sgx_active_page_list);
 static DEFINE_SPINLOCK(sgx_reclaimer_lock);
+static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock);
 
 static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
 static unsigned long sgx_nr_total_pages;
@@ -457,7 +458,10 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
  page->flags = 0;
 
  spin_unlock(&node->lock);
+
+ spin_lock(&sgx_allocate_epc_page_lock);
  atomic_long_dec(&sgx_nr_free_pages);
+ spin_unlock(&sgx_allocate_epc_page_lock);
 
  return page;
 }

And then also take spinlock every time eupdatesvn attempts to run:

int sgx_updatesvn(void)
+{
+ int ret;
+ int retry = 10;
+
+ spin_lock(&sgx_allocate_epc_page_lock);
+
+ if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) {
+ spin_unlock(&sgx_allocate_epc_page_lock);
+ return SGX_EPC_NOT_READY;
+ }
+
+ do {
+ ret = __eupdatesvn();
+ if (ret != SGX_INSUFFICIENT_ENTROPY)
+ break;
+
+ } while (--retry);
+
+ spin_unlock(&sgx_allocate_epc_page_lock);

Which was called from each enclave create ioctl:

@@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
  if (copy_from_user(&create_arg, arg, sizeof(create_arg)))
  return -EFAULT;
 
+ /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */
+ if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) &&
+    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
+ sgx_updatesvn();
+
  secs = kmalloc(PAGE_SIZE, GFP_KERNEL);
  if (!secs)
  return -ENOMEM;

Would you agree that this way it is much worse even code/logic-wise even without benchmarks? 

Best Regards,
Elena. 

RE: [PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Reshetova, Elena]

> On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote:
> >
> > > On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote:
> > > > sgx_nr_free_pages is an atomic that is used to keep track of
> > > > free EPC pages and detect whenever page reclaiming should start.
> > > > Since successful execution of ENCLS[EUPDATESVN] requires empty
> > > > EPC and a fast way of checking for this, change this variable
> > > > around to indicate number of used pages instead. The subsequent
> > > > patch that introduces ENCLS[EUPDATESVN] will take use of this change.
> > >
> > > s/subsequent patch//
> >
> > Ok
> >
> > >
> > > You should rather express how EUPDATESVN trigger will depend on the
> > > state of sgx_nr_used_pages and sgx_nr_free_pages.
> >
> > How about this explanation:
> >
> > "By counting the # of used pages instead of #of free pages, it allows the
> > EPC page allocation path execute without a need to take the lock in all
> > but a single case when the first page is being allocated in EPC. This is
> > achieved via a fast check in atomic_long_inc_not_zero."
> 
> Yep, whole a lot more sense.
> 
> >
> > Also, if you think that it is hard to interpret the patch 2/4 without 4/4
> > I can also squeeze them together and then it becomes right away clear
> > why the change was done.
> >
> >
> > >
> > > >
> > > > No functional changes intended.
> > >
> > > Not really understanding how I should interpret this sentence.
> >
> > Just as usual: this patch itself doesn’t bring any functional changes
> > to the way as current SGX code works. I only needed this change to
> > implement patch 4/4 in more lockless way.
> >
> > >
> > > The commit message does not mention sgx_nr_used_pages, and neiher it
> > > makes a case why implementing the feature based on sgx_nr_free_pages
> is
> > > not possible.
> >
> > It is possible to implement it, in fact I did exactly this in the beginning
> instead,
> > but as mentioned previously this would have resulted in taking a lock for
> each
> > case the page is being allocated.
> 
> Have you benchmarked this (memory barrier vs putting the whole thing
> inside spinlock)?
> 
> I have doubts that this would even show in margins given how much e.g.,
> ELDU takes.

No, I haven’t benchmarked this. The reason to choose this approach (vs. the
other one I showed in email now) was purely logical - less locks and better code.

Best Regards,
Elena.

RE: [PATCH 3/4] x86/sgx: Define error codes for ENCLS[EUPDATESVN]

[Reshetova, Elena]

 
> On Mon, Mar 24, 2025 at 12:21:14PM +0000, Reshetova, Elena wrote:
> >
> >
> > > On Fri, Mar 21, 2025 at 02:34:42PM +0200, Elena Reshetova wrote:
> > > > Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> > > > process can know the execution state of EUPDATESVN.
> > > >
> > >
> > > Enumerate the error codes.
> >
> > You mean in the commit message or?
> 
> I'm not really sure how kernel uses them.

The SGX_INSUFFICIENT_ENTROPY is used to make reties by kernel. 
The other two are only used to print the informational message. 

Best Regards,
Elena.

RE: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Reshetova, Elena]

 
> On Mon, Mar 24, 2025 at 12:26:38PM +0000, Reshetova, Elena wrote:
> >
> > > On Fri, Mar 21, 2025 at 02:34:43PM +0200, Elena Reshetova wrote:
> > > > SGX architecture introduced  a new instruction called EUPDATESVN [1]
> > > > to Ice Lake. It allows updating security SVN version, given that EPC
> > > > is completely empty. The latter is required for security reasons
> > > > in order to reason that enclave security posture is as secure as the
> > > > security SVN version of the TCB that created it.
> > > >
> > > > Additionally it is important to ensure that while ENCLS[EUPDATESVN]
> > > > runs, no concurrent page creation happens in EPC, because it might
> > > > result in #GP delivered to the creator. Legacy SW might not be prepared
> > > > to handle such unexpected #GPs and therefore this patch introduces
> > > > a locking mechanism to ensure no concurrent EPC allocations can
> happen.
> > > >
> > > > It is also ensured that ENCLS[EUPDATESVN] is not called when running
> > > > in a VM since it does not have a meaning in this context (microcode
> > > > updates application is limited to the host OS) and will create
> > > > unnecessary load.
> > > >
> > > > The implementation of ENCLS[EUPDATESVN] is based on previous
> > > submision in [2]
> > > >
> > > > [1]
> https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true
> > >
> > > I don't think for Intel opcodes a link is needed. We know where to look
> > > them up.
> >
> > Ok, I can drop this reference. It is just a whitepaper explaining to readers
> > the background and operation of EUPDATESVN. It is not part of standard
> > SDM, so I thought I would put a link.
> >
> >
> > >
> > > > [2] https://lore.kernel.org/all/20220520103904.1216-1-
> > >
> cathy.zhang@intel.com/T/#medb89e6a916337b4f9e68c736a295ba0ae99ac90
> > >
> > > Link:
> >
> > ?  Not sure what you mean by it.
> >
> > >
> > > >
> > > > Co-developed-by: Cathy Zhang <cathy.zhang@intel.com>
> > > > Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
> > > > Co-developed-by: Elena Reshetova <elena.reshetova@intel.com>
> > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > > ---
> > > >  arch/x86/include/asm/sgx.h      | 33 +++++++++--------
> > > >  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
> > > >  arch/x86/kernel/cpu/sgx/main.c  | 65
> > > ++++++++++++++++++++++++++++++++-
> > > >  arch/x86/kernel/cpu/sgx/sgx.h   |  2 +
> > > >  4 files changed, 90 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > > > index 8ba39bbf4e91..5caf5c31ebc6 100644
> > > > --- a/arch/x86/include/asm/sgx.h
> > > > +++ b/arch/x86/include/asm/sgx.h
> > > > @@ -26,23 +26,26 @@
> > > >  #define SGX_CPUID_EPC_SECTION	0x1
> > > >  /* The bitmask for the EPC section type. */
> > > >  #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
> > > > +/* EUPDATESVN presence indication */
> > > > +#define SGX_CPUID_EUPDATESVN	BIT(10)
> > > >
> > > >  enum sgx_encls_function {
> > > > -	ECREATE	= 0x00,
> > > > -	EADD	= 0x01,
> > > > -	EINIT	= 0x02,
> > > > -	EREMOVE	= 0x03,
> > > > -	EDGBRD	= 0x04,
> > > > -	EDGBWR	= 0x05,
> > > > -	EEXTEND	= 0x06,
> > > > -	ELDU	= 0x08,
> > > > -	EBLOCK	= 0x09,
> > > > -	EPA	= 0x0A,
> > > > -	EWB	= 0x0B,
> > > > -	ETRACK	= 0x0C,
> > > > -	EAUG	= 0x0D,
> > > > -	EMODPR	= 0x0E,
> > > > -	EMODT	= 0x0F,
> > > > +	ECREATE		= 0x00,
> > > > +	EADD		= 0x01,
> > > > +	EINIT		= 0x02,
> > > > +	EREMOVE		= 0x03,
> > > > +	EDGBRD		= 0x04,
> > > > +	EDGBWR		= 0x05,
> > > > +	EEXTEND		= 0x06,
> > > > +	ELDU		= 0x08,
> > > > +	EBLOCK		= 0x09,
> > > > +	EPA		= 0x0A,
> > > > +	EWB		= 0x0B,
> > > > +	ETRACK		= 0x0C,
> > > > +	EAUG		= 0x0D,
> > > > +	EMODPR		= 0x0E,
> > > > +	EMODT		= 0x0F,
> > > > +	EUPDATESVN	= 0x18,
> > > >  };
> > > >
> > > >  /**
> > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h
> > > b/arch/x86/kernel/cpu/sgx/encls.h
> > > > index 99004b02e2ed..3d83c76dc91f 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > > > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > > > @@ -233,4 +233,10 @@ static inline int __eaug(struct sgx_pageinfo
> *pginfo,
> > > void *addr)
> > > >  	return __encls_2(EAUG, pginfo, addr);
> > > >  }
> > > >
> > > > +/* Update CPUSVN at runtime. */
> > > > +static inline int __eupdatesvn(void)
> > > > +{
> > > > +	return __encls_ret_1(EUPDATESVN, "");
> > > > +}
> > > > +
> > > >  #endif /* _X86_ENCLS_H */
> > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > > b/arch/x86/kernel/cpu/sgx/main.c
> > > > index b61d3bad0446..698921229094 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -32,6 +32,11 @@ static DEFINE_XARRAY(sgx_epc_address_space);
> > > >  static LIST_HEAD(sgx_active_page_list);
> > > >  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> > > >
> > > > +/* This lock is held to prevent new EPC pages from being created
> > > > + * during the execution of ENCLS[EUPDATESVN].
> > > > + */
> > > > +static DEFINE_SPINLOCK(sgx_epc_eupdatesvn_lock);
> > > > +
> > > >  static atomic_long_t sgx_nr_used_pages = ATOMIC_LONG_INIT(0);
> > > >  static unsigned long sgx_nr_total_pages;
> > > >
> > > > @@ -457,7 +462,17 @@ static struct sgx_epc_page
> > > *__sgx_alloc_epc_page_from_node(int nid)
> > > >  	page->flags = 0;
> > > >
> > > >  	spin_unlock(&node->lock);
> > > > -	atomic_long_inc(&sgx_nr_used_pages);
> > > > +
> > > > +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
> > > > +		spin_lock(&sgx_epc_eupdatesvn_lock);
> > > > +		/* Only call sgx_updatesvn() once the first enclave's
> > > > +		 * page is allocated from EPC
> > > > +		 */
> > > > +		if (atomic_long_read(&sgx_nr_used_pages) == 0)
> > > > +			sgx_updatesvn();
> > > > +		atomic_long_inc(&sgx_nr_used_pages);
> > > > +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> > > > +	}
> > > >
> > > >  	return page;
> > > >  }
> > > > @@ -970,3 +985,51 @@ static int __init sgx_init(void)
> > > >  }
> > > >
> > > >  device_initcall(sgx_init);
> > > > +
> > > > +/**
> > > > + * sgx_updatesvn() - Issue ENCLS[EUPDATESVN]
> > > > + * If EPC is ready, this instruction will update CPUSVN to the currently
> > > > + * loaded microcode update SVN and generate new cryptographic
> assets.
> > > > + */
> > > > +void sgx_updatesvn(void)
> > > > +{
> > > > +	int ret;
> > > > +	int retry = 10;
> > >
> > > Reverse declaration order.
> >
> > Sure, will fix.
> >
> > >
> > > > +
> > > > +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> > > > +
> > > > +	/* Do not execute EUPDATESVN if instruction is unavalible or running
> > > in a VM */
> > > > +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) ||
> > > > +		    boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > > > +		return;
> > >
> > >
> > > 	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> > > 		return;
> > >
> > > 	/* ... */
> > > 	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > > 		return;
> > >
> > > The first check really does not need a comment. The second would benefit
> > > from explaining why bail out inside a VM.
> >
> > Sure, I can change. The reason why we dont want the execution in a VM is
> explained
> > in the commit message, I can duplicate it in the code comment also.
> >
> >
> > >
> > >
> > >
> > >
> > >
> > > > +
> > > > +	do {
> > > > +		ret = __eupdatesvn();
> > > > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > > > +			break;
> > > > +
> > > > +	} while (--retry);
> > > > +
> > > > +	switch (ret) {
> > > > +	case 0:
> > > > +		pr_debug("EUPDATESVN was successful!\n");
> > > > +		break;
> 
> This is at least definite removal. We don't log for zero codes.

Ok. 

> 
> > > > +	case SGX_NO_UPDATE:
> > > > +		pr_debug("EUPDATESVN was successful, but CPUSVN was not
> > > updated, "
> > > > +			"because current SVN was not newer than
> > > CPUSVN.\n");
> > > > +		break;
> > > > +	case SGX_EPC_NOT_READY:
> > > > +		pr_debug("EPC is not ready for SVN update.");
> > > > +		break;
> > > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > > +		pr_debug("CPUSVN update is failed due to Insufficient
> > > entropy in RNG, "
> > > > +			"please try it later.\n");
> > > > +		break;
> > > > +	case SGX_EPC_PAGE_CONFLICT:
> > > > +		pr_debug("CPUSVN update is failed due to concurrency
> > > violation, please "
> > > > +			"stop running any other ENCLS leaf and try it
> > > later.\n");
> > > > +		break;
> > > > +	default:
> > > > +		break;
> > >
> > > Remove pr_debug() statements.
> >
> > This I am not sure it is good idea. I think it would be useful for system
> > admins to have a way to see that update either happened or not.
> > It is true that you can find this out by requesting a new SGX attestation
> > quote (and see if newer SVN is used), but it is not the faster way.
> 
> Maybe pr_debug() is them wrong level if they are meant for sysadmins?
> 
> I mean these should not happen in normal behavior like ever? As
> pr_debug() I don't really grab this.

SGX_NO_UPDATE will absolutely happen normally all the time. 
Since EUPDATESVN is executed every time EPC is empty, this is the
most common code you will get back (because microcode updates are rare). 
Others yes, that would indicate some error condition. 
So, what is the pr_level that you would suggest? 

> 
> >
> >
> >
> > >
> > > > +	}
> > > > +}
> > > > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h
> b/arch/x86/kernel/cpu/sgx/sgx.h
> > > > index d2dad21259a8..92c9e226f1b5 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > > > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > > > @@ -104,4 +104,6 @@ static inline int __init sgx_vepc_init(void)
> > > >
> > > >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> > > >
> > >
> > > I don't think we need a newline in-between.
> >
> > Sure, will fix.
> >
> > Thank you very much for your prompt review Jarkko!
> 
> NP, despite all the complains this are really just minor details
> that we need to nail :-) I'm sure we get them right within round
> or two...

Sure, happy to do the nailing ))

Best Regards,
Elena

Re: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 03:42:30PM +0000, Reshetova, Elena wrote:
> > > > > +	case SGX_NO_UPDATE:
> > > > > +		pr_debug("EUPDATESVN was successful, but CPUSVN was not
> > > > updated, "
> > > > > +			"because current SVN was not newer than
> > > > CPUSVN.\n");
> > > > > +		break;
> > > > > +	case SGX_EPC_NOT_READY:
> > > > > +		pr_debug("EPC is not ready for SVN update.");
> > > > > +		break;
> > > > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > > > +		pr_debug("CPUSVN update is failed due to Insufficient
> > > > entropy in RNG, "
> > > > > +			"please try it later.\n");
> > > > > +		break;
> > > > > +	case SGX_EPC_PAGE_CONFLICT:
> > > > > +		pr_debug("CPUSVN update is failed due to concurrency
> > > > violation, please "
> > > > > +			"stop running any other ENCLS leaf and try it
> > > > later.\n");
> > > > > +		break;
> > > > > +	default:
> > > > > +		break;
> > > >
> > > > Remove pr_debug() statements.
> > >
> > > This I am not sure it is good idea. I think it would be useful for system
> > > admins to have a way to see that update either happened or not.
> > > It is true that you can find this out by requesting a new SGX attestation
> > > quote (and see if newer SVN is used), but it is not the faster way.
> > 
> > Maybe pr_debug() is them wrong level if they are meant for sysadmins?
> > 
> > I mean these should not happen in normal behavior like ever? As
> > pr_debug() I don't really grab this.
> 
> SGX_NO_UPDATE will absolutely happen normally all the time. 
> Since EUPDATESVN is executed every time EPC is empty, this is the
> most common code you will get back (because microcode updates are rare). 
> Others yes, that would indicate some error condition. 
> So, what is the pr_level that you would suggest? 

Right, got it. That changes my conclusions:

So I'd reformulate it like:

	switch (ret) {
	case 0:
		pr_info("EUPDATESVN: success\n);	
		break;
	case SGX_EPC_NOT_READY:
	case SGX_INSUFFICIENT_ENTROPY:
	case SGX_EPC_PAGE_CONFLICT:
		pr_err("EUPDATESVN: error %d\n", ret);
		/* TODO: block/teardown driver? */
		break;
	case SGX_NO_UPDATE:
		break;
	default:
		pr_err("EUPDATESVN: unknown error %d\n", ret);
		/* TODO: block/teardown driver? */
		break;
	}

Since when this is executed EPC usage is zero error cases should block
or teardown SGX driver, presuming that they are because of either
incorrect driver state or spurious error code.

If this happens, we definitely do not want service, right?

I'm not sure of all error codes how serious they are, or are all of them
consequence of incorrectly working driver.

BR, Jarkko

Re: [PATCH 2/4] x86/sgx: Change counter sgx_nr_free_pages -> sgx_nr_used_pages

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 03:31:31PM +0000, Reshetova, Elena wrote:
> > On Mon, Mar 24, 2025 at 12:19:37PM +0000, Reshetova, Elena wrote:
> > >
> > > > On Fri, Mar 21, 2025 at 02:34:41PM +0200, Elena Reshetova wrote:
> > > > > sgx_nr_free_pages is an atomic that is used to keep track of
> > > > > free EPC pages and detect whenever page reclaiming should start.
> > > > > Since successful execution of ENCLS[EUPDATESVN] requires empty
> > > > > EPC and a fast way of checking for this, change this variable
> > > > > around to indicate number of used pages instead. The subsequent
> > > > > patch that introduces ENCLS[EUPDATESVN] will take use of this change.
> > > >
> > > > s/subsequent patch//
> > >
> > > Ok
> > >
> > > >
> > > > You should rather express how EUPDATESVN trigger will depend on the
> > > > state of sgx_nr_used_pages and sgx_nr_free_pages.
> > >
> > > How about this explanation:
> > >
> > > "By counting the # of used pages instead of #of free pages, it allows the
> > > EPC page allocation path execute without a need to take the lock in all
> > > but a single case when the first page is being allocated in EPC. This is
> > > achieved via a fast check in atomic_long_inc_not_zero."
> > 
> > Yep, whole a lot more sense.
> > 
> > >
> > > Also, if you think that it is hard to interpret the patch 2/4 without 4/4
> > > I can also squeeze them together and then it becomes right away clear
> > > why the change was done.
> > >
> > >
> > > >
> > > > >
> > > > > No functional changes intended.
> > > >
> > > > Not really understanding how I should interpret this sentence.
> > >
> > > Just as usual: this patch itself doesn’t bring any functional changes
> > > to the way as current SGX code works. I only needed this change to
> > > implement patch 4/4 in more lockless way.
> > >
> > > >
> > > > The commit message does not mention sgx_nr_used_pages, and neiher it
> > > > makes a case why implementing the feature based on sgx_nr_free_pages
> > is
> > > > not possible.
> > >
> > > It is possible to implement it, in fact I did exactly this in the beginning
> > instead,
> > > but as mentioned previously this would have resulted in taking a lock for
> > each
> > > case the page is being allocated.
> > 
> > Have you benchmarked this (memory barrier vs putting the whole thing
> > inside spinlock)?
> > 
> > I have doubts that this would even show in margins given how much e.g.,
> > ELDU takes.
> 
> No, I haven’t benchmarked this. The reason to choose this approach
> (vs. the other one I showed in email now) was purely logical - less
> locks and better code.

You need to have hard data to to turn things around like this.
Otherwise, let's not do this.

EPC allocation is always combined with heave-weight opcode so
this really does not serve any purpose.

> 
> Best Regards,
> Elena.

BR, Jarkko

Re: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Jarkko Sakkinen]

oN Thu, Mar 27, 2025 at 03:29:53PM +0000, Reshetova, Elena wrote:
> 
> > On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote:
> > > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> > > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be
> > empty.
> > > > > SGX already has a variable sgx_nr_free_pages that tracks free
> > > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> > > > > track of total number of EPC pages. It will be used in subsequent
> > > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> > > > > allow an easy check for an empty EPC.
> > > >
> > > > First off, remove "in subsequent patch".
> > >
> > > Ok
> > >
> > > >
> > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean?
> > >
> > > As you can see from patch 2/4, I had to turn around the meaning of the
> > > existing sgx_nr_free_pages atomic counter not to count the # of free pages
> > > in EPC, but to count the # of used EPC pages (hence the change of name
> > > to sgx_nr_used_pages). The reason for doing this is only apparent in patch
> > 
> > Why you *absolutely* need to invert the meaning and cannot make
> > this work by any means otherwise?
> > 
> > I doubt highly doubt this could not be done other way around.
> 
> I can make it work. The point that this way is much better and no damage to
> existing logic is done. The sgx_nr_free_pages counter that is used only for page reclaiming
> and checked in a single piece of code.
> To give you an idea the previous iteration of the code looked like below.
> First, I had to define a new unconditional spinlock to protect the EPC page allocation:
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index c8a2542140a1..4f445c28929b 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space);
>   */
>  static LIST_HEAD(sgx_active_page_list);
>  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> +static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock);



>  
>  static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
>  static unsigned long sgx_nr_total_pages;
> @@ -457,7 +458,10 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>   page->flags = 0;
>  
>   spin_unlock(&node->lock);
> +
> + spin_lock(&sgx_allocate_epc_page_lock);
>   atomic_long_dec(&sgx_nr_free_pages);
> + spin_unlock(&sgx_allocate_epc_page_lock);
>  
>   return page;
>  }
> 
> And then also take spinlock every time eupdatesvn attempts to run:
> 
> int sgx_updatesvn(void)
> +{
> + int ret;
> + int retry = 10;

Reverse xmas tree order.

> +
> + spin_lock(&sgx_allocate_epc_page_lock);

You could use guard for this.

https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/cleanup.h

> +
> + if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) {
> + spin_unlock(&sgx_allocate_epc_page_lock);
> + return SGX_EPC_NOT_READY;

Don't use uarch error codes. 

> + }
> +
> + do {
> + ret = __eupdatesvn();
> + if (ret != SGX_INSUFFICIENT_ENTROPY)
> + break;
> +
> + } while (--retry);
> +
> + spin_unlock(&sgx_allocate_epc_page_lock);
> 
> Which was called from each enclave create ioctl:
> 
> @@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
>   if (copy_from_user(&create_arg, arg, sizeof(create_arg)))
>   return -EFAULT;
>  
> + /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */
> + if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) &&
> +    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
> + sgx_updatesvn();
> +
>   secs = kmalloc(PAGE_SIZE, GFP_KERNEL);
>   if (!secs)
>   return -ENOMEM;
> 
> Would you agree that this way it is much worse even code/logic-wise even without benchmarks? 

Yes but obviously I cannot promise that I'll accept this as it is
until I see the final version

Also you probably should use mutex given the loop where we cannot
temporarily exit the lock (like e.g. in keyrings gc we can).

> 
> Best Regards,
> Elena. 

BR, Jarkko

RE: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Reshetova, Elena]

> oN Thu, Mar 27, 2025 at 03:29:53PM +0000, Reshetova, Elena wrote:
> >
> > > On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote:
> > > > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote:
> > > > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be
> > > empty.
> > > > > > SGX already has a variable sgx_nr_free_pages that tracks free
> > > > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep
> > > > > > track of total number of EPC pages. It will be used in subsequent
> > > > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and
> > > > > > allow an easy check for an empty EPC.
> > > > >
> > > > > First off, remove "in subsequent patch".
> > > >
> > > > Ok
> > > >
> > > > >
> > > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages"
> mean?
> > > >
> > > > As you can see from patch 2/4, I had to turn around the meaning of the
> > > > existing sgx_nr_free_pages atomic counter not to count the # of free
> pages
> > > > in EPC, but to count the # of used EPC pages (hence the change of name
> > > > to sgx_nr_used_pages). The reason for doing this is only apparent in
> patch
> > >
> > > Why you *absolutely* need to invert the meaning and cannot make
> > > this work by any means otherwise?
> > >
> > > I doubt highly doubt this could not be done other way around.
> >
> > I can make it work. The point that this way is much better and no damage to
> > existing logic is done. The sgx_nr_free_pages counter that is used only for
> page reclaiming
> > and checked in a single piece of code.
> > To give you an idea the previous iteration of the code looked like below.
> > First, I had to define a new unconditional spinlock to protect the EPC page
> allocation:
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> b/arch/x86/kernel/cpu/sgx/main.c
> > index c8a2542140a1..4f445c28929b 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space);
> >   */
> >  static LIST_HEAD(sgx_active_page_list);
> >  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
> > +static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock);
> 
> 
> 
> >
> >  static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
> >  static unsigned long sgx_nr_total_pages;
> > @@ -457,7 +458,10 @@ static struct sgx_epc_page
> *__sgx_alloc_epc_page_from_node(int nid)
> >   page->flags = 0;
> >
> >   spin_unlock(&node->lock);
> > +
> > + spin_lock(&sgx_allocate_epc_page_lock);
> >   atomic_long_dec(&sgx_nr_free_pages);
> > + spin_unlock(&sgx_allocate_epc_page_lock);
> >
> >   return page;
> >  }
> >
> > And then also take spinlock every time eupdatesvn attempts to run:
> >
> > int sgx_updatesvn(void)
> > +{
> > + int ret;
> > + int retry = 10;
> 
> Reverse xmas tree order.
> 
> > +
> > + spin_lock(&sgx_allocate_epc_page_lock);
> 
> You could use guard for this.
> 
> https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/cleanup.h
> 
> > +
> > + if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) {
> > + spin_unlock(&sgx_allocate_epc_page_lock);
> > + return SGX_EPC_NOT_READY;
> 
> Don't use uarch error codes.

Sure, thanks, I can fix all of the above, this was just to give an idea how
the other version of the code would look like. 

> 
> > + }
> > +
> > + do {
> > + ret = __eupdatesvn();
> > + if (ret != SGX_INSUFFICIENT_ENTROPY)
> > + break;
> > +
> > + } while (--retry);
> > +
> > + spin_unlock(&sgx_allocate_epc_page_lock);
> >
> > Which was called from each enclave create ioctl:
> >
> > @@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl
> *encl, void __user *arg)
> >   if (copy_from_user(&create_arg, arg, sizeof(create_arg)))
> >   return -EFAULT;
> >
> > + /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */
> > + if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) &&
> > +    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > + sgx_updatesvn();
> > +
> >   secs = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >   if (!secs)
> >   return -ENOMEM;
> >
> > Would you agree that this way it is much worse even code/logic-wise even
> without benchmarks?
> 
> Yes but obviously I cannot promise that I'll accept this as it is
> until I see the final version

Are you saying you prefer *this version with spinlock* vs. 
simpler version that utilizes the fact that sgx_nr_free_pages is changed
into tracking of number of used pages? 

> 
> Also you probably should use mutex given the loop where we cannot
> temporarily exit the lock (like e.g. in keyrings gc we can).

Not sure I understand this, could you please elaborate why do I need an
additional mutex here? Or are you suggesting switching spinlock to mutex? 

Best Regards,
Elena.

RE: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Reshetova, Elena]

> On Thu, Mar 27, 2025 at 03:42:30PM +0000, Reshetova, Elena wrote:
> > > > > > +	case SGX_NO_UPDATE:
> > > > > > +		pr_debug("EUPDATESVN was successful, but CPUSVN
> was not
> > > > > updated, "
> > > > > > +			"because current SVN was not newer than
> > > > > CPUSVN.\n");
> > > > > > +		break;
> > > > > > +	case SGX_EPC_NOT_READY:
> > > > > > +		pr_debug("EPC is not ready for SVN update.");
> > > > > > +		break;
> > > > > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > > > > +		pr_debug("CPUSVN update is failed due to Insufficient
> > > > > entropy in RNG, "
> > > > > > +			"please try it later.\n");
> > > > > > +		break;
> > > > > > +	case SGX_EPC_PAGE_CONFLICT:
> > > > > > +		pr_debug("CPUSVN update is failed due to
> concurrency
> > > > > violation, please "
> > > > > > +			"stop running any other ENCLS leaf and try it
> > > > > later.\n");
> > > > > > +		break;
> > > > > > +	default:
> > > > > > +		break;
> > > > >
> > > > > Remove pr_debug() statements.
> > > >
> > > > This I am not sure it is good idea. I think it would be useful for system
> > > > admins to have a way to see that update either happened or not.
> > > > It is true that you can find this out by requesting a new SGX attestation
> > > > quote (and see if newer SVN is used), but it is not the faster way.
> > >
> > > Maybe pr_debug() is them wrong level if they are meant for sysadmins?
> > >
> > > I mean these should not happen in normal behavior like ever? As
> > > pr_debug() I don't really grab this.
> >
> > SGX_NO_UPDATE will absolutely happen normally all the time.
> > Since EUPDATESVN is executed every time EPC is empty, this is the
> > most common code you will get back (because microcode updates are rare).
> > Others yes, that would indicate some error condition.
> > So, what is the pr_level that you would suggest?
> 
> Right, got it. That changes my conclusions:
> 
> So I'd reformulate it like:
> 
> 	switch (ret) {
> 	case 0:
> 		pr_info("EUPDATESVN: success\n);
> 		break;
> 	case SGX_EPC_NOT_READY:
> 	case SGX_INSUFFICIENT_ENTROPY:
> 	case SGX_EPC_PAGE_CONFLICT:
> 		pr_err("EUPDATESVN: error %d\n", ret);
> 		/* TODO: block/teardown driver? */
> 		break;
> 	case SGX_NO_UPDATE:
> 		break;
> 	default:
> 		pr_err("EUPDATESVN: unknown error %d\n", ret);
> 		/* TODO: block/teardown driver? */
> 		break;
> 	}
> 
> Since when this is executed EPC usage is zero error cases should block
> or teardown SGX driver, presuming that they are because of either
> incorrect driver state or spurious error code.

I agree with the above, but not sure at all about the blocking/teardown the
driver. They are all potentially temporal things and  SGX_INSUFFICIENT_ENTROPY
is even outside of SGX driver control and *does not* indicate any error
condition on the driver side itself. SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT
would mean we have a bug somewhere because we thought we could go
do EUDPATESVN on empty EPC and prevented anyone from creating
pages in meanwhile but looks like we missed smth. That said, I dont know if we
want to fail the whole system in case we have such a code bug, this is very 
aggressive (in case it is some rare edge condition that no one knew about or
guessed). So, I would propose to print the pr_err() as you have above but
avoid destroying the driver. 
Would this work? 

Best Regards,
Elena.


> 
> If this happens, we definitely do not want service, right?
> 
> I'm not sure of all error codes how serious they are, or are all of them
> consequence of incorrectly working driver.
> 
> BR, Jarkko

Re: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Jarkko Sakkinen]

On Fri, Mar 28, 2025 at 08:07:24AM +0000, Reshetova, Elena wrote:
> > Yes but obviously I cannot promise that I'll accept this as it is
> > until I see the final version
> 
> Are you saying you prefer *this version with spinlock* vs. 
> simpler version that utilizes the fact that sgx_nr_free_pages is changed
> into tracking of number of used pages? 

I don't know really what I do prefer.

Maybe +1 version would make sense where you keep with the approach
you've chosen (used pages) and better rationalize why it is mandatory,
and why free pages would be worse?

> 
> > 
> > Also you probably should use mutex given the loop where we cannot
> > temporarily exit the lock (like e.g. in keyrings gc we can).
> 
> Not sure I understand this, could you please elaborate why do I need an
> additional mutex here? Or are you suggesting switching spinlock to mutex? 

In your code example you had a loop inside spinlock, which was based on
a return code of an opcode, i.e. potentially infinite loop.

I'd like to remind you that the hardware I have is NUC7 from 2018 so
you really have to nail how things will work semantically as I can
only think these things only in theoretical level ;-) [1]


> 
> Best Regards,
> Elena.
> 

[1] https://social.kernel.org/notice/AsUbsYH0T4bTcUSdUW

BR, Jarkko

Re: [PATCH 4/4] x86/sgx: Implement ENCLS[EUPDATESVN] and opportunistically call it during first EPC page alloc

[Jarkko Sakkinen]

On Fri, Mar 28, 2025 at 08:27:51AM +0000, Reshetova, Elena wrote:
> 
> > On Thu, Mar 27, 2025 at 03:42:30PM +0000, Reshetova, Elena wrote:
> > > > > > > +	case SGX_NO_UPDATE:
> > > > > > > +		pr_debug("EUPDATESVN was successful, but CPUSVN
> > was not
> > > > > > updated, "
> > > > > > > +			"because current SVN was not newer than
> > > > > > CPUSVN.\n");
> > > > > > > +		break;
> > > > > > > +	case SGX_EPC_NOT_READY:
> > > > > > > +		pr_debug("EPC is not ready for SVN update.");
> > > > > > > +		break;
> > > > > > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > > > > > +		pr_debug("CPUSVN update is failed due to Insufficient
> > > > > > entropy in RNG, "
> > > > > > > +			"please try it later.\n");
> > > > > > > +		break;
> > > > > > > +	case SGX_EPC_PAGE_CONFLICT:
> > > > > > > +		pr_debug("CPUSVN update is failed due to
> > concurrency
> > > > > > violation, please "
> > > > > > > +			"stop running any other ENCLS leaf and try it
> > > > > > later.\n");
> > > > > > > +		break;
> > > > > > > +	default:
> > > > > > > +		break;
> > > > > >
> > > > > > Remove pr_debug() statements.
> > > > >
> > > > > This I am not sure it is good idea. I think it would be useful for system
> > > > > admins to have a way to see that update either happened or not.
> > > > > It is true that you can find this out by requesting a new SGX attestation
> > > > > quote (and see if newer SVN is used), but it is not the faster way.
> > > >
> > > > Maybe pr_debug() is them wrong level if they are meant for sysadmins?
> > > >
> > > > I mean these should not happen in normal behavior like ever? As
> > > > pr_debug() I don't really grab this.
> > >
> > > SGX_NO_UPDATE will absolutely happen normally all the time.
> > > Since EUPDATESVN is executed every time EPC is empty, this is the
> > > most common code you will get back (because microcode updates are rare).
> > > Others yes, that would indicate some error condition.
> > > So, what is the pr_level that you would suggest?
> > 
> > Right, got it. That changes my conclusions:
> > 
> > So I'd reformulate it like:
> > 
> > 	switch (ret) {
> > 	case 0:
> > 		pr_info("EUPDATESVN: success\n);
> > 		break;
> > 	case SGX_EPC_NOT_READY:
> > 	case SGX_INSUFFICIENT_ENTROPY:
> > 	case SGX_EPC_PAGE_CONFLICT:
> > 		pr_err("EUPDATESVN: error %d\n", ret);
> > 		/* TODO: block/teardown driver? */
> > 		break;
> > 	case SGX_NO_UPDATE:
> > 		break;
> > 	default:
> > 		pr_err("EUPDATESVN: unknown error %d\n", ret);
> > 		/* TODO: block/teardown driver? */
> > 		break;
> > 	}
> > 
> > Since when this is executed EPC usage is zero error cases should block
> > or teardown SGX driver, presuming that they are because of either
> > incorrect driver state or spurious error code.
> 
> I agree with the above, but not sure at all about the blocking/teardown the
> driver. They are all potentially temporal things and  SGX_INSUFFICIENT_ENTROPY
> is even outside of SGX driver control and *does not* indicate any error
> condition on the driver side itself. SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT
> would mean we have a bug somewhere because we thought we could go
> do EUDPATESVN on empty EPC and prevented anyone from creating
> pages in meanwhile but looks like we missed smth. That said, I dont know if we
> want to fail the whole system in case we have such a code bug, this is very 
> aggressive (in case it is some rare edge condition that no one knew about or
> guessed). So, I would propose to print the pr_err() as you have above but
> avoid destroying the driver. 
> Would this work? 

I think now is the time that you should really roll out a new version in
the way you see fit and we will revisit that.

I already grabbed from your example that I got some of the error codes
horribly wrong :-) Still I think the draft of error planning I put is
at least towards right direction.

> 
> Best Regards,
> Elena.
> 
> 
> > 
> > If this happens, we definitely do not want service, right?
> > 
> > I'm not sure of all error codes how serious they are, or are all of them
> > consequence of incorrectly working driver.
> > 
> > BR, Jarkko

BR, Jarkko

Re: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Jarkko Sakkinen]

On Fri, Mar 28, 2025 at 10:42:18AM +0200, Jarkko Sakkinen wrote:
> In your code example you had a loop inside spinlock, which was based on
> a return code of an opcode, i.e. potentially infinite loop.
> 
> I'd like to remind you that the hardware I have is NUC7 from 2018 so
> you really have to nail how things will work semantically as I can
> only think these things only in theoretical level ;-) [1]

That said, I do execute these in NUC7 but is getting a bit old..

Cheapest hardware I've heard is Xeon E-2334 but even that with
case etc. is like nearing 2k in price.

BR, Jarkko

RE: [PATCH 1/4] x86/sgx: Add total number of EPC pages

[Reshetova, Elena]

 
> On Fri, Mar 28, 2025 at 08:07:24AM +0000, Reshetova, Elena wrote:
> > > Yes but obviously I cannot promise that I'll accept this as it is
> > > until I see the final version
> >
> > Are you saying you prefer *this version with spinlock* vs.
> > simpler version that utilizes the fact that sgx_nr_free_pages is changed
> > into tracking of number of used pages?
> 
> I don't know really what I do prefer.
> 
> Maybe +1 version would make sense where you keep with the approach
> you've chosen (used pages) and better rationalize why it is mandatory,
> and why free pages would be worse?

Sure, let me send out v2 with the old approach, all suggestions and fixes
taken into account and better reasoning. 

> 
> >
> > >
> > > Also you probably should use mutex given the loop where we cannot
> > > temporarily exit the lock (like e.g. in keyrings gc we can).
> >
> > Not sure I understand this, could you please elaborate why do I need an
> > additional mutex here? Or are you suggesting switching spinlock to mutex?
> 
> In your code example you had a loop inside spinlock, which was based on
> a return code of an opcode, i.e. potentially infinite loop.

Oh, this is a misunderstanding due to limited snippet posting. 
The loop was bounded also by "retry" condition in while with the max number of
retires being 10. It only exists earlier if there is success. 


> 
> I'd like to remind you that the hardware I have is NUC7 from 2018 so
> you really have to nail how things will work semantically as I can
> only think these things only in theoretical level ;-) [1]

Sure, I understand. 

Best Regards,
Elena.


> 
> 
> >
> > Best Regards,
> > Elena.
> >
> 
> [1] https://social.kernel.org/notice/AsUbsYH0T4bTcUSdUW
> 
> BR, Jarkko