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

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

[Elena Reshetova]

Changes since v2 following review by Jarkko:

 - formatting of comments is fixed
 - change from pr_error to ENCLS_WARN to communicate errors from
 EUPDATESVN
 - In case an unknown error is detected (must not happen per spec),
 make page allocation from EPC fail in order to prevent EPC usage

Changes since v1 following review by Jarkko:

 - first and second patch are squashed together and a better
   explanation of the change is added into the commit message
 - third and fourth patch are also combined for better understanding
   of error code purposes used in 4th patch
 - implementation of sgx_updatesvn adjusted following Jarkko's 
   suggestions
 - minor fixes in both commit messages and code from the review
 - dropping co-developed-by tag since the code now differs enough
   from the original submission. However, the reference where the
   original code came from and credits to original author is kept

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/

Elena Reshetova (2):
  x86/sgx: Use sgx_nr_used_pages for EPC page count instead of
    sgx_nr_free_pages
  x86/sgx: Implement 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  | 95 +++++++++++++++++++++++++++++++--
 arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
 4 files changed, 123 insertions(+), 20 deletions(-)

-- 
2.45.2

[PATCH v3 1/2] x86/sgx: Use sgx_nr_used_pages for EPC page count instead of sgx_nr_free_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 preferably a fast lockless way of checking for this
condition in all code paths where EPC is already used, change the
reclaiming code to track the number of used pages via
sgx_nr_used_pages instead of sgx_nr_free_pages.
For this change to work in the page reclamation code, add a new
variable, sgx_nr_total_pages, that will keep track of total
number of EPC pages.

It would have been possible to implement ENCLS[EUPDATESVN] using
existing sgx_nr_free_pages counter and a new sgx_nr_total_pages
counter, but it won't be possible to avoid taking a lock *every time*
a new EPC page is being allocated. The conversion of sgx_nr_free_pages
into sgx_nr_used_pages allows avoiding the lock in all cases except
when it is the first EPC page being allocated via a quick
atomic_long_inc_not_zero check.

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8ce352fc72ac..b61d3bad0446 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -32,7 +32,8 @@ 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. */
 static nodemask_t sgx_numa_mask;
@@ -378,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);
 }
 
 /*
@@ -456,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;
 }
@@ -616,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,
@@ -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;
 }
 
@@ -848,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 v3 2/2] x86/sgx: Implement EUPDATESVN and opportunistically call it during first EPC page alloc

[Elena Reshetova]

SGX architecture introduced a new instruction called EUPDATESVN
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.

This patch is based on previous submision by Cathy Zhang
https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h      | 41 +++++++++++------
 arch/x86/kernel/cpu/sgx/encls.h |  6 +++
 arch/x86/kernel/cpu/sgx/main.c  | 82 ++++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
 4 files changed, 114 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..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,
 };
 
 /**
@@ -73,6 +76,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 +89,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,
 };
 
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..c21f4f6226b0 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;
 
@@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
 {
 	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
 	struct sgx_epc_page *page = NULL;
+	bool ret;
 
 	spin_lock(&node->lock);
 
@@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
 		return NULL;
 	}
 
+	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) {
+			ret = sgx_updatesvn();
+			if (!ret) {
+				/*
+				 * sgx_updatesvn() returned unknown error, smth
+				 * must be broken, do not allocate a page from EPC
+				 */
+				spin_unlock(&sgx_epc_eupdatesvn_lock);
+				spin_unlock(&node->lock);
+				return NULL;
+			}
+		}
+		atomic_long_inc(&sgx_nr_used_pages);
+		spin_unlock(&sgx_epc_eupdatesvn_lock);
+	}
+
 	page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list);
 	list_del_init(&page->list);
 	page->flags = 0;
 
 	spin_unlock(&node->lock);
-	atomic_long_inc(&sgx_nr_used_pages);
 
 	return page;
 }
@@ -970,3 +997,56 @@ 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.
+ *
+ * Return:
+ * True: success or EUPDATESVN can be safely retried next time
+ * False: Unexpected error occurred
+ */
+bool sgx_updatesvn(void)
+{
+	int retry = 10;
+	int ret;
+
+	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
+
+	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
+		return true;
+
+	/*
+	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
+	 * microcode updates are only meaningful to be applied on the host.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return true;
+
+	do {
+		ret = __eupdatesvn();
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+
+	} while (--retry);
+
+	switch (ret) {
+	case 0:
+		pr_info("EUPDATESVN: success\n");
+		break;
+	case SGX_EPC_NOT_READY:
+	case SGX_INSUFFICIENT_ENTROPY:
+	case SGX_EPC_PAGE_CONFLICT:
+		ENCLS_WARN(ret, "EUPDATESVN");
+		break;
+	case SGX_NO_UPDATE:
+		break;
+	default:
+		ENCLS_WARN(ret, "EUPDATESVN");
+		return false;
+	}
+
+	return true;
+
+}
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index d2dad21259a8..d7d631c973d0 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -103,5 +103,6 @@ static inline int __init sgx_vepc_init(void)
 #endif
 
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
+bool sgx_updatesvn(void);
 
 #endif /* _X86_SGX_H */
-- 
2.45.2

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

[Huang, Kai]

On Tue, 2025-04-15 at 14:51 +0300, Elena Reshetova wrote:
> SGX architecture introduced a new instruction called EUPDATESVN

"a new ENCLS leaf function EUPDATESVN"?

> 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.
> 
> This patch is based on previous submision by Cathy Zhang
> https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/

When Jarkko suggested to use "Link" here:

https://lore.kernel.org/lkml/Z983ZaTaWNqFUpYS@kernel.org/

I think he meant something like below:

This patch is based on ... [1]

Link: https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/
[1]

> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h      | 41 +++++++++++------
>  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 82 ++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
>  4 files changed, 114 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..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)

I am not sure whether this should be a new X86_FEATURE bit, just like other SGX
bits:

  X86_FEATURE_SGX1
  X86_FEATURE_SGX2
  X86_FEATURE_SGX_EDECCSSA

.. reported in CPUID.0x12.0x0:EAX.

But this is never going to be exposed to VMs, i.e., KVM should never need to use
it, so perhaps fine to just put it here.

[...]


>  
> +/* This lock is held to prevent new EPC pages from being created
> + * during the execution of ENCLS[EUPDATESVN].
> + */

/*
 * This lock ...
 * ...
 */

> +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;
>  
> @@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  {
>  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
>  	struct sgx_epc_page *page = NULL;
> +	bool ret;
>  
>  	spin_lock(&node->lock);
>  
> @@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  		return NULL;
>  	}
>  
> +	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
> +		 */

The VMs can pre-populate EPC w/o having any enclave being created inside.  How
about just:

		/*
		 * Make sure SVN is up-to-date before any EPC page is
		 * allocated for any enclave.
		 */

> +		if (atomic_long_read(&sgx_nr_used_pages) == 0) {
> +			ret = sgx_updatesvn();
> +			if (!ret) {
> +				/*
> +				 * sgx_updatesvn() returned unknown error, smth
> +				 * must be broken, do not allocate a page from EPC
> +				 */

Strictly speaking, sgx_updatesvn() returns true or false, but not "unknown
error".

Reading below, it could also fail due to running out of entropy, which is still
legally possible to happen IMHO.

Maybe just:
				/*
				 * Updating SVN failed.  SGX might be broken,
				 * or running out of entropy happened.  Do not
				 * allocate EPC page since it is not safe to
use
				 * SGX anymore if it was the former.  If it was
				 * due to running out of entropy, the further
				 * call of EPC allocation will try
				 * sgx_updatesvn() again.
				 */

> +				spin_unlock(&sgx_epc_eupdatesvn_lock);
> +				spin_unlock(&node->lock);
> +				return NULL;
> +			}
> +		}
> +		atomic_long_inc(&sgx_nr_used_pages);
> +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> +	}
> +
>  	page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list);
>  	list_del_init(&page->list);
>  	page->flags = 0;
>  
>  	spin_unlock(&node->lock);
> -	atomic_long_inc(&sgx_nr_used_pages);
>  
>  	return page;
>  }
> @@ -970,3 +997,56 @@ 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

Not sure how to interpret "EPC is ready".  Assume it means "EPC is empty", in
which case should we just say so?

It is consistent with what said in the changelog anyway.

> + * loaded microcode update SVN and generate new cryptographic assets.
> + *
> + * Return:
> + * True: success or EUPDATESVN can be safely retried next time
> + * False: Unexpected error occurred

Hmm.. IIUC it could fail with running out of entropy but this is still legally
possible to happen.  And it is safe to retry.

> + */
> +bool sgx_updatesvn(void)
> +{
> +	int retry = 10;

"10" is a bit out of blue.

We can use RDRAND_RETRY_LOOPS in <asm/archrandom.h> instead:

  #define RDRAND_RETRY_LOOPS      10

> +	int ret;
> +
> +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> +
> +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		return true;
> +
> +	/*
> +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> +	 * microcode updates are only meaningful to be applied on the host.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		return true;
> +
> +	do {
> +		ret = __eupdatesvn();
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +

The blank line here is not needed.

> +	} while (--retry);
> +
> +	switch (ret) {
> +	case 0:
> +		pr_info("EUPDATESVN: success\n");
> +		break;
> +	case SGX_EPC_NOT_READY:
> +	case SGX_INSUFFICIENT_ENTROPY:
> +	case SGX_EPC_PAGE_CONFLICT:
> +		ENCLS_WARN(ret, "EUPDATESVN");
> +		break;

I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY, since
IIUC it is still legally possible to happen after the above retry.

Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are needed
since IIUC the only possible error is out of entropy.

> +	case SGX_NO_UPDATE:
> +		break;
> +	default:
> +		ENCLS_WARN(ret, "EUPDATESVN");
> +		return false;
> +	}
> +
> +	return true;
> +

Please remove this blank line.

> +}
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..d7d631c973d0 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -103,5 +103,6 @@ static inline int __init sgx_vepc_init(void)
>  #endif
>  
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> +bool sgx_updatesvn(void);

Seems sgx_updatesvn() is only called by __sgx_alloc_epc_page_from_node().  Can
it be made static?

Re: [PATCH v3 1/2] x86/sgx: Use sgx_nr_used_pages for EPC page count instead of sgx_nr_free_pages

[Huang, Kai]

On Tue, 2025-04-15 at 14:51 +0300, 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

The mentioning of ENCLS[EUPDATESVN] is kinda out of blue here.  It's better to
introduce it first like the next patch does.

> EPC and preferably a fast lockless way of checking for this
> condition in all code paths where EPC is already used, change the
> reclaiming code to track the number of used pages via
> sgx_nr_used_pages instead of sgx_nr_free_pages.
> For this change to work in the page reclamation code, add a new
> variable, sgx_nr_total_pages, that will keep track of total
> number of EPC pages.
> 
> It would have been possible to implement ENCLS[EUPDATESVN] using
> existing sgx_nr_free_pages counter and a new sgx_nr_total_pages
> counter, but it won't be possible to avoid taking a lock *every time*
> a new EPC page is being allocated. The conversion of sgx_nr_free_pages
> into sgx_nr_used_pages allows avoiding the lock in all cases except
> when it is the first EPC page being allocated via a quick
> atomic_long_inc_not_zero check.
> 
> 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>

Reviewed-by: Kai Huang <kai.huang@intel.com>

RE: [PATCH v3 1/2] x86/sgx: Use sgx_nr_used_pages for EPC page count instead of sgx_nr_free_pages

[Reshetova, Elena]

 
> On Tue, 2025-04-15 at 14:51 +0300, 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
> 
> The mentioning of ENCLS[EUPDATESVN] is kinda out of blue here.  It's better
> to
> introduce it first like the next patch does.

Thank you, will expand more. 

> 
> > EPC and preferably a fast lockless way of checking for this
> > condition in all code paths where EPC is already used, change the
> > reclaiming code to track the number of used pages via
> > sgx_nr_used_pages instead of sgx_nr_free_pages.
> > For this change to work in the page reclamation code, add a new
> > variable, sgx_nr_total_pages, that will keep track of total
> > number of EPC pages.
> >
> > It would have been possible to implement ENCLS[EUPDATESVN] using
> > existing sgx_nr_free_pages counter and a new sgx_nr_total_pages
> > counter, but it won't be possible to avoid taking a lock *every time*
> > a new EPC page is being allocated. The conversion of sgx_nr_free_pages
> > into sgx_nr_used_pages allows avoiding the lock in all cases except
> > when it is the first EPC page being allocated via a quick
> > atomic_long_inc_not_zero check.
> >
> > 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>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>

Thank you very much for your review!

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

[Reshetova, Elena]

> On Tue, 2025-04-15 at 14:51 +0300, Elena Reshetova wrote:
> > SGX architecture introduced a new instruction called EUPDATESVN
> 
> "a new ENCLS leaf function EUPDATESVN"?

Yes, you are right, better wording, will fix. 

> 
> > 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.
> >
> > This patch is based on previous submision by Cathy Zhang
> > https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/
> 
> When Jarkko suggested to use "Link" here:
> 
> https://lore.kernel.org/lkml/Z983ZaTaWNqFUpYS@kernel.org/
> 
> I think he meant something like below:
> 
> This patch is based on ... [1]
> 
> Link: https://lore.kernel.org/all/20220520103904.1216-1-
> cathy.zhang@intel.com/
> [1]

Oh, I see, I didn’t understand this. Can fix in the next version. 

> 
> >
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/include/asm/sgx.h      | 41 +++++++++++------
> >  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
> >  arch/x86/kernel/cpu/sgx/main.c  | 82
> ++++++++++++++++++++++++++++++++-
> >  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
> >  4 files changed, 114 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > index 6a0069761508..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)
> 
> I am not sure whether this should be a new X86_FEATURE bit, just like other
> SGX
> bits:
> 
>   X86_FEATURE_SGX1
>   X86_FEATURE_SGX2
>   X86_FEATURE_SGX_EDECCSSA
> 
> .. reported in CPUID.0x12.0x0:EAX.
> 
> But this is never going to be exposed to VMs, i.e., KVM should never need to
> use
> it, so perhaps fine to just put it here.

I am fine either way. I can change this if people consider it is a better fit. 

> 
> [...]
> 
> 
> >
> > +/* This lock is held to prevent new EPC pages from being created
> > + * during the execution of ENCLS[EUPDATESVN].
> > + */
> 
> /*
>  * This lock ...
>  * ...
>  */

Thank you for catching this! I thought that I fixed all comments to this
Format but obvious I missed this one. Sad that checkpatch doesn’t
complain on this (

> 
> > +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;
> >
> > @@ -444,6 +449,7 @@ static struct sgx_epc_page
> *__sgx_alloc_epc_page_from_node(int nid)
> >  {
> >  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
> >  	struct sgx_epc_page *page = NULL;
> > +	bool ret;
> >
> >  	spin_lock(&node->lock);
> >
> > @@ -452,12 +458,33 @@ static struct sgx_epc_page
> *__sgx_alloc_epc_page_from_node(int nid)
> >  		return NULL;
> >  	}
> >
> > +	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
> > +		 */
> 
> The VMs can pre-populate EPC w/o having any enclave being created inside.
> How
> about just:
> 
> 		/*
> 		 * Make sure SVN is up-to-date before any EPC page is
> 		 * allocated for any enclave.
> 		 */
> 

Right, agree with this wording change, good point!

> > +		if (atomic_long_read(&sgx_nr_used_pages) == 0) {
> > +			ret = sgx_updatesvn();
> > +			if (!ret) {
> > +				/*
> > +				 * sgx_updatesvn() returned unknown error,
> smth
> > +				 * must be broken, do not allocate a page
> from EPC
> > +				 */
> 
> Strictly speaking, sgx_updatesvn() returns true or false, but not "unknown
> error".

Yes, will fix the wording. 

> 
> Reading below, it could also fail due to running out of entropy, which is still
> legally possible to happen IMHO.

Actually, no in this case, we only return false from sgx_updatesvn in case unknown
error happens as agreed previously. In case we run out of entropy it should be safe
to retry later and we dont prevent per current code EPC page allocation. 

> 
> Maybe just:
> 				/*
> 				 * Updating SVN failed.  SGX might be broken,
> 				 * or running out of entropy happened.  Do
> not
> 				 * allocate EPC page since it is not safe to
> use
> 				 * SGX anymore if it was the former.  If it was
> 				 * due to running out of entropy, the further
> 				 * call of EPC allocation will try
> 				 * sgx_updatesvn() again.
> 				 */

I agree with this except that the current code doesn’t prevent EPC allocation on any
other error return than unknown error. The question is whenever we want to
change the behaviour to require it? 

> 
> > +				spin_unlock(&sgx_epc_eupdatesvn_lock);
> > +				spin_unlock(&node->lock);
> > +				return NULL;
> > +			}
> > +		}
> > +		atomic_long_inc(&sgx_nr_used_pages);
> > +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> > +	}
> > +
> >  	page = list_first_entry(&node->free_page_list, struct sgx_epc_page,
> list);
> >  	list_del_init(&page->list);
> >  	page->flags = 0;
> >
> >  	spin_unlock(&node->lock);
> > -	atomic_long_inc(&sgx_nr_used_pages);
> >
> >  	return page;
> >  }
> > @@ -970,3 +997,56 @@ 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
> 
> Not sure how to interpret "EPC is ready".  Assume it means "EPC is empty", in
> which case should we just say so?

Yes, correct, it means EPC is empty, will fix. 

> 
> It is consistent with what said in the changelog anyway.
> 
> > + * loaded microcode update SVN and generate new cryptographic assets.
> > + *
> > + * Return:
> > + * True: success or EUPDATESVN can be safely retried next time
> > + * False: Unexpected error occurred
> 
> Hmm.. IIUC it could fail with running out of entropy but this is still legally
> possible to happen.  And it is safe to retry.

Yes, in this case we get back SGX_INSUFFICIENT_ENTROPY currently we
return "true" here and do not prevent EPC allocations of the page in this
case, which means we will start populate EPC and can next time retry only
when EPC is empty again. 

> 
> > + */
> > +bool sgx_updatesvn(void)
> > +{
> > +	int retry = 10;
> 
> "10" is a bit out of blue.
> 
> We can use RDRAND_RETRY_LOOPS in <asm/archrandom.h> instead:
> 
>   #define RDRAND_RETRY_LOOPS      10

I can change it to this value. 

> 
> > +	int ret;
> > +
> > +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> > +
> > +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> > +		return true;
> > +
> > +	/*
> > +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> > +	 * microcode updates are only meaningful to be applied on the host.
> > +	 */
> > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > +		return true;
> > +
> > +	do {
> > +		ret = __eupdatesvn();
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +
> 
> The blank line here is not needed.

Will fix

> 
> > +	} while (--retry);
> > +
> > +	switch (ret) {
> > +	case 0:
> > +		pr_info("EUPDATESVN: success\n");
> > +		break;
> > +	case SGX_EPC_NOT_READY:
> > +	case SGX_INSUFFICIENT_ENTROPY:
> > +	case SGX_EPC_PAGE_CONFLICT:
> > +		ENCLS_WARN(ret, "EUPDATESVN");
> > +		break;
> 
> I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY,
> since
> IIUC it is still legally possible to happen after the above retry.
> 
> Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT
> are needed
> since IIUC the only possible error is out of entropy.

Well, in case we have a kernel bug, and we think EPC is empty and it is safe
to execute EUPDATESVN, while it is not the case, we can still get back the 
SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT from HW, right? 
Which means we probably should warn about such buggy cases. 
And maybe we should also prevent page allocation from EPC in this case also
similarly as for unknown error? 

> 
> > +	case SGX_NO_UPDATE:
> > +		break;
> > +	default:
> > +		ENCLS_WARN(ret, "EUPDATESVN");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +
> 
> Please remove this blank line.

Sure, thanks for catching!

> 
> > +}
> > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> > index d2dad21259a8..d7d631c973d0 100644
> > --- a/arch/x86/kernel/cpu/sgx/sgx.h
> > +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> > @@ -103,5 +103,6 @@ static inline int __init sgx_vepc_init(void)
> >  #endif
> >
> >  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> > +bool sgx_updatesvn(void);
> 
> Seems sgx_updatesvn() is only called by __sgx_alloc_epc_page_from_node().
> Can
> it be made static?

Yes, will do. 

Re: [PATCH v3 1/2] x86/sgx: Use sgx_nr_used_pages for EPC page count instead of sgx_nr_free_pages

[Jarkko Sakkinen]

On Tue, Apr 15, 2025 at 02:51:21PM +0300, Elena Reshetova wrote:
> 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.

No. It's

- not a side-note but core part of the rationale.
- the reasoning here is nonsense, or more like it does not exist at all.

sgx_nr_free_pages can be substituted with sgx_nr_used_pages at the sites
where sgx_free_pages was previously used *exactly* because
sgx_reclaimer_init() is called only after sgx_page_cache_init(). This
gives the invariant of it to be constant whenever sgx_alloc_epc_page()
is called.

These type of changes give a proof of the legitimity of the invariant,
which I addressed here.

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue, Apr 15, 2025 at 02:51:22PM +0300, Elena Reshetova wrote:
> SGX architecture introduced a new instruction called EUPDATESVN
> 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.

"Ice Lake introduced ENCLS[EUPDATESVN], which allows to to update the
microcode version for an online system" ?

Now you are saying it allows updating SVN which is null information.

> 
> 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.
> 
> This patch is based on previous submision by Cathy Zhang
> https://lore.kernel.org/all/20220520103904.1216-1-cathy.zhang@intel.com/
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h      | 41 +++++++++++------
>  arch/x86/kernel/cpu/sgx/encls.h |  6 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 82 ++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
>  4 files changed, 114 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..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,
>  };
>  
>  /**
> @@ -73,6 +76,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 +89,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,
>  };
>  
> 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..c21f4f6226b0 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;
>  
> @@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  {
>  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
>  	struct sgx_epc_page *page = NULL;
> +	bool ret;
>  
>  	spin_lock(&node->lock);
>  
> @@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  		return NULL;
>  	}
>  
> +	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) {
> +			ret = sgx_updatesvn();
> +			if (!ret) {
> +				/*
> +				 * sgx_updatesvn() returned unknown error, smth
> +				 * must be broken, do not allocate a page from EPC
> +				 */
> +				spin_unlock(&sgx_epc_eupdatesvn_lock);
> +				spin_unlock(&node->lock);
> +				return NULL;
> +			}
> +		}
> +		atomic_long_inc(&sgx_nr_used_pages);
> +		spin_unlock(&sgx_epc_eupdatesvn_lock);
> +	}
> +
>  	page = list_first_entry(&node->free_page_list, struct sgx_epc_page, list);
>  	list_del_init(&page->list);
>  	page->flags = 0;
>  
>  	spin_unlock(&node->lock);
> -	atomic_long_inc(&sgx_nr_used_pages);
>  
>  	return page;
>  }
> @@ -970,3 +997,56 @@ 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.
> + *
> + * Return:
> + * True: success or EUPDATESVN can be safely retried next time
> + * False: Unexpected error occurred
> + */
> +bool sgx_updatesvn(void)
> +{
> +	int retry = 10;
> +	int ret;
> +
> +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> +
> +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		return true;
> +
> +	/*
> +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> +	 * microcode updates are only meaningful to be applied on the host.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		return true;
> +
> +	do {
> +		ret = __eupdatesvn();
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +
> +	} while (--retry);
> +
> +	switch (ret) {
> +	case 0:
> +		pr_info("EUPDATESVN: success\n");
> +		break;
> +	case SGX_EPC_NOT_READY:
> +	case SGX_INSUFFICIENT_ENTROPY:
> +	case SGX_EPC_PAGE_CONFLICT:
> +		ENCLS_WARN(ret, "EUPDATESVN");
> +		break;
> +	case SGX_NO_UPDATE:
> +		break;
> +	default:
> +		ENCLS_WARN(ret, "EUPDATESVN");
> +		return false;

I'm lost why only ENCLS_WARN() is practiced here. For spurious error
code at minimum EPC allocation should be turned off really. Something
is likely working incorrectly in the driver.

> +	}
> +
> +	return true;
> +
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index d2dad21259a8..d7d631c973d0 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -103,5 +103,6 @@ static inline int __init sgx_vepc_init(void)
>  #endif
>  
>  void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
> +bool sgx_updatesvn(void);
>  
>  #endif /* _X86_SGX_H */
> -- 
> 2.45.2
> 

BR, Jarkko

Re: [PATCH v3 1/2] x86/sgx: Use sgx_nr_used_pages for EPC page count instead of sgx_nr_free_pages

[Jarkko Sakkinen]

On Wed, Apr 16, 2025 at 09:50:44PM +0300, Jarkko Sakkinen wrote:
> On Tue, Apr 15, 2025 at 02:51:21PM +0300, Elena Reshetova wrote:
> > 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.
> 
> No. It's
> 
> - not a side-note but core part of the rationale.
> - the reasoning here is nonsense, or more like it does not exist at all.
> 
> sgx_nr_free_pages can be substituted with sgx_nr_used_pages at the sites
> where sgx_free_pages was previously used *exactly* because
> sgx_reclaimer_init() is called only after sgx_page_cache_init(). This
> gives the invariant of it to be constant whenever sgx_alloc_epc_page()
> is called.
> 
> These type of changes give a proof of the legitimity of the invariant,
> which I addressed here.

Let's assume that this patch set had a bug just as a mind game.

If we have a reasoning of the change documented, we will end up having
better tools to reason what we did not consider while acking this patch
set per se.

With convoluted reasoning we have absolutely nothing.

This why what I'm saying is important.

BR, Jarkko

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

[Huang, Kai]

> 
> > 
> > Reading below, it could also fail due to running out of entropy, which is still
> > legally possible to happen IMHO.
> 
> Actually, no in this case, we only return false from sgx_updatesvn in case unknown
> error happens as agreed previously. In case we run out of entropy it should be safe
> to retry later and we dont prevent per current code EPC page allocation. 
> 
> > 
> > Maybe just:
> > 				/*
> > 				 * Updating SVN failed.  SGX might be broken,
> > 				 * or running out of entropy happened.  Do
> > not
> > 				 * allocate EPC page since it is not safe to
> > use
> > 				 * SGX anymore if it was the former.  If it was
> > 				 * due to running out of entropy, the further
> > 				 * call of EPC allocation will try
> > 				 * sgx_updatesvn() again.
> > 				 */
> 
> I agree with this except that the current code doesn’t prevent EPC allocation on any
> other error return than unknown error. The question is whenever we want to
> change the behaviour to require it? 

[...]

> > > + * Return:
> > > + * True: success or EUPDATESVN can be safely retried next time
> > > + * False: Unexpected error occurred
> > 
> > Hmm.. IIUC it could fail with running out of entropy but this is still legally
> > possible to happen.  And it is safe to retry.
> 
> Yes, in this case we get back SGX_INSUFFICIENT_ENTROPY currently we
> return "true" here and do not prevent EPC allocations of the page in this
> case, which means we will start populate EPC and can next time retry only
> when EPC is empty again. 

[...]

> > > +	switch (ret) {
> > > +	case 0:
> > > +		pr_info("EUPDATESVN: success\n");
> > > +		break;
> > > +	case SGX_EPC_NOT_READY:
> > > +	case SGX_INSUFFICIENT_ENTROPY:
> > > +	case SGX_EPC_PAGE_CONFLICT:
> > > +		ENCLS_WARN(ret, "EUPDATESVN");
> > > +		break;
> > 
> > I don't think we should use ENCLS_WARN() for SGX_INSUFFICIENT_ENTROPY,
> > since
> > IIUC it is still legally possible to happen after the above retry.
> > 
> > Also, it doesn't seem SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT
> > are needed
> > since IIUC the only possible error is out of entropy.
> 
> Well, in case we have a kernel bug, and we think EPC is empty and it is safe
> to execute EUPDATESVN, while it is not the case, we can still get back the 
> SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT from HW, right? 

Right, but as you said, in case we have a kernel bug.

Which means it is not expected and we should just use ENCLS_WARN() for them.

IMHO we can even add

	WARN_ON_ONCE(atomic_long_read(&sgx_nr_used_pages) != 0);

to sgx_updatesvn(), e.g., right after
 
	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);

to assert sgx_updatesvn() is called when EPC is empty, thus SGX_EPC_NOT_READY
and SGX_EPC_PAGE_CONFLICT are not possible to happen.

> Which means we probably should warn about such buggy cases. 

Yes.

> And maybe we should also prevent page allocation from EPC in this case also
> similarly as for unknown error?

Yes.

I don't see any reason we should continue to allow SGX to work in case of
SGX_EPC_NOT_READY or SGX_EPC_PAGE_CONFLICT.

IIUC, we also agreed in the last round discussion that we should:

 "I guess the best action would be make sgx_alloc_epc_page() return
 consistently -ENOMEM, if the unexpected happens."

SGX_EPC_NOT_READY and SGX_EPC_PAGE_CONFLICT are indeed unexpected to me.

So my suggestion would be:

I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or
SGX_NO_UPDATE, and return false for all other error codes.  And it should
ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because
it can still legally happen.

Something like:

	do {
		ret = __eupdatesvn();
		if (ret != SGX_INSUFFICIENT_ENTROPY)
			break;
	} while (--retry);

	if (!ret || ret == SGX_NO_UPDATE) {
		/*
		 * SVN successfully updated, or it was already up-to-date.
		 * Let users know when the update was successful.
		 */
		if (!ret)
			pr_info("SVN updated successfully\n");
		return true;
	}

	/*
	 * EUPDATESVN was called when EPC is empty, all other error
	 * codes are unexcepted except running out of entropy.
	 */
	if (ret != SGX_INSUFFICIENT_ENTROPY)
		ENCLS_WARN(ret, "EUPDATESVN");

	return false;
		

In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
return -ENOMEM when sgx_updatesvn() returns false.  We should only allow EPC to
be allocated when we know the SVN is already up-to-date.

Any further call of EPC allocation will trigger sgx_updatesvn() again.  If it
was failed due to unexpected error, then it should continue to fail,
guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
unexpected happens".  If it was failed due to running out of entropy, it then
may fail again, or it will just succeed and then SGX can continue to work.

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

[Sean Christopherson]

On Thu, Apr 17, 2025, Kai Huang wrote:
> I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or
> SGX_NO_UPDATE, and return false for all other error codes.  And it should
> ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because
> it can still legally happen.
> 
> Something like:
> 
> 	do {
> 		ret = __eupdatesvn();
> 		if (ret != SGX_INSUFFICIENT_ENTROPY)
> 			break;
> 	} while (--retry);

This can be:

	do {
		ret = __eupdatesvn();
	} while (ret == SGX_INSUFFICIENT_ENTROPY && --retry)

To make it super obvious that retry is only relevant to lack of entropy.

> 	if (!ret || ret == SGX_NO_UPDATE) {
> 		/*
> 		 * SVN successfully updated, or it was already up-to-date.
> 		 * Let users know when the update was successful.
> 		 */
> 		if (!ret)
> 			pr_info("SVN updated successfully\n");
> 		return true;

Returning true/false is confusing since the vast majority of the SGX code uses
'0' for success.  A lot of cleverness went into splicing SGX's error codes into
the kernel's -ernno; it would be a shame to ignore that :-)

E.g. this looks wrong at first (and second glance)

			ret = sgx_updatesvn();
			if (!ret) {
				/*
				 * sgx_updatesvn() returned unknown error, smth
				 * must be broken, do not allocate a page from EPC
				 */
				spin_unlock(&sgx_epc_eupdatesvn_lock);
				spin_unlock(&node->lock);
				return NULL;
			}

> 	}
> 
> 	/*
> 	 * EUPDATESVN was called when EPC is empty, all other error
> 	 * codes are unexcepted except running out of entropy.
> 	 */
> 	if (ret != SGX_INSUFFICIENT_ENTROPY)
> 		ENCLS_WARN(ret, "EUPDATESVN");
> 
> 	return false;
> 		
> 
> In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
> return -ENOMEM when sgx_updatesvn() returns false.  We should only allow EPC to

No, it should return a meaningful error code, not -ENOMEM.  And if that's the
behavior you want, then __sgx_alloc_epc_page() should be updated to bail immediately.
The current code assuming -ENOMEM is the only failure scenario:

	do {
		page = __sgx_alloc_epc_page_from_node(nid);
		if (page)
			return page;

		nid = next_node_in(nid, sgx_numa_mask);
	} while (nid != nid_start);

That should be something like:

	do {
		page = __sgx_alloc_epc_page_from_node(nid);
		if (!IS_ERR(page) || PTR_ERR(page) != -ENOMEM)
			return page;

		nid = next_node_in(nid, sgx_numa_mask);
	} while (nid != nid_start);

> be allocated when we know the SVN is already up-to-date.
> 
> Any further call of EPC allocation will trigger sgx_updatesvn() again.  If it
> was failed due to unexpected error, then it should continue to fail,
> guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
> unexpected happens".  If it was failed due to running out of entropy, it then
> may fail again, or it will just succeed and then SGX can continue to work.


Side topic, the function comment for __sgx_alloc_epc_page() is stale/wrong.  It
returns ERR_PTR(-ENOMEM), not NULL, on failure.

/**
 * __sgx_alloc_epc_page() - Allocate an EPC page
 *
 * Iterate through NUMA nodes and reserve ia free EPC page to the caller. Start
 * from the NUMA node, where the caller is executing.
 *
 * Return:
 * - an EPC page:	A borrowed EPC pages were available.
 * - NULL:		Out of EPC pages.
 */

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

[Sean Christopherson]

On Tue, Apr 15, 2025, Elena Reshetova wrote:
> +/* 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;
>  
> @@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  {
>  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
>  	struct sgx_epc_page *page = NULL;
> +	bool ret;
>  
>  	spin_lock(&node->lock);
>  
> @@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
>  		return NULL;
>  	}
>  
> +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {

FWIW, the entire automatic scheme has terrible behavior when KVM is running SGX
capable guests.  KVM will allocate EPC when the virtual EPC is first touched by
the guest, and won't free the EPC pages until the VM is terminated.  And IIRC,
userspace (QEMU) typically preallocates the virtual EPC to ensure the VM doesn't
need to be killed later on due to lack of EPC.

I.e. running an SGX capable VM, even if there are no active enclaves in said VM,
will prevent SVN updates.

Unfortunately, I can't think of a sane way around that, because while it would
be easy enough to force interception of ECREATE, there's no definitive ENCLS leaf
that KVM can intercept to detect when an enclave is destroyed (interception
EREMOVE would have terrible performance implications).

That said, handling this deep in the bowels of EPC page allocation seems
unnecessary.  The only way for there to be no active EPC pages is if there are
no enclaves.  As above, virtual EPC usage is already all but guaranteed to hit
false positives, so I don't think there's anything gained by trying to update
the SVN based on EPC allocations.

So rather than react to EPC allocations, why not hook sgx_open() and sgx_vepc_open()?
Then you're hooking a relative slow path (I assume EUPDATESVN is not fast), error
codes can be returned to userspace (if you really want the kernel to freak out if
EUPDATESVN unexpected fails), and you don't _have_ to use a spinlock, e.g. if
EUPDATESVN takes a long time, using a mutex might be desirable.

> +bool sgx_updatesvn(void)
> +{
> +	int retry = 10;
> +	int ret;
> +
> +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> +
> +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		return true;

Checking CPUID features inside the spinlock is asinine.  They can't change, barring
a misbehaving hypervisor.  This should be a "cache once during boot" sorta thing.

> +
> +	/*
> +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> +	 * microcode updates are only meaningful to be applied on the host.

I don't think the kernel should check HYPERVISOR.  The SDM doesn't actually
say anything about EUPDATESVN, but unless it's explicitly special cased, I doubt
XuCode cares whether ENCLS was executed in Root vs. Non-Root.

It's the hypervisor's responsibility to enumerate SGX_CPUID_EUPDATESVN or not.
E.g. if the kernel is running in a "passthrough" type setup, it would be perfectly
fine to do EUPDATESVN from a guest kernel.  EUPDATESVN could even be proxied,
e.g. by intercepting ECREATE to track EPC usage

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

[Huang, Kai]

On Fri, 2025-04-18 at 07:18 -0700, Sean Christopherson wrote:
> On Thu, Apr 17, 2025, Kai Huang wrote:
> > I think the sgx_updatesvn() should just return true when EUPDATESVN returns 0 or
> > SGX_NO_UPDATE, and return false for all other error codes.  And it should
> > ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY because
> > it can still legally happen.
> > 
> > Something like:
> > 
> > 	do {
> > 		ret = __eupdatesvn();
> > 		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > 			break;
> > 	} while (--retry);
> 
> This can be:
> 
> 	do {
> 		ret = __eupdatesvn();
> 	} while (ret == SGX_INSUFFICIENT_ENTROPY && --retry)
> 
> To make it super obvious that retry is only relevant to lack of entropy.

Yep looks better.

> 
> > 	if (!ret || ret == SGX_NO_UPDATE) {
> > 		/*
> > 		 * SVN successfully updated, or it was already up-to-date.
> > 		 * Let users know when the update was successful.
> > 		 */
> > 		if (!ret)
> > 			pr_info("SVN updated successfully\n");
> > 		return true;
> 
> Returning true/false is confusing since the vast majority of the SGX code uses
> '0' for success.  A lot of cleverness went into splicing SGX's error codes into
> the kernel's -ernno; it would be a shame to ignore that :-)

Agreed :-)

> 
> E.g. this looks wrong at first (and second glance)
> 
> 			ret = sgx_updatesvn();
> 			if (!ret) {
> 				/*
> 				 * sgx_updatesvn() returned unknown error, smth
> 				 * must be broken, do not allocate a page from EPC
> 				 */
> 				spin_unlock(&sgx_epc_eupdatesvn_lock);
> 				spin_unlock(&node->lock);
> 				return NULL;
> 			}

Yep.

> 
> > 	}
> > 
> > 	/*
> > 	 * EUPDATESVN was called when EPC is empty, all other error
> > 	 * codes are unexcepted except running out of entropy.
> > 	 */
> > 	if (ret != SGX_INSUFFICIENT_ENTROPY)
> > 		ENCLS_WARN(ret, "EUPDATESVN");
> > 
> > 	return false;
> > 		
> > 
> > In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
> > return -ENOMEM when sgx_updatesvn() returns false.  We should only allow EPC to
> 
> No, it should return a meaningful error code, not -ENOMEM.  
> 

Oh I was actually thinking to keep __sgx_alloc_epc_page_from_node() returning
NULL if sgx_updatesvn() fails (given it returns true/false) and make
sgx_alloc_epc_page() return -ENOMEM.

Sorry for didn't say fully. :-(

> And if that's the
> behavior you want, then __sgx_alloc_epc_page() should be updated to bail immediately.
> The current code assuming -ENOMEM is the only failure scenario:
> 
> 	do {
> 		page = __sgx_alloc_epc_page_from_node(nid);
> 		if (page)
> 			return page;
> 
> 		nid = next_node_in(nid, sgx_numa_mask);
> 	} while (nid != nid_start);
> 
> That should be something like:
> 
> 	do {
> 		page = __sgx_alloc_epc_page_from_node(nid);
> 		if (!IS_ERR(page) || PTR_ERR(page) != -ENOMEM)
> 			return page;
> 
> 		nid = next_node_in(nid, sgx_numa_mask);
> 	} while (nid != nid_start);

If we want to bail out immediately, then perhaps -EIO is a better option instead
of -ENOMEM.  And in this case sgx_alloc_epc_page() can also bail out early:

--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -569,6 +569,9 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool
reclaim)
                        break;
                }
 
+               if (PTR_ERR(page) == -EIO)
+                       return page;
+
                if (list_empty(&sgx_active_page_list))
                        return ERR_PTR(-ENOMEM);

> 
> > be allocated when we know the SVN is already up-to-date.
> > 
> > Any further call of EPC allocation will trigger sgx_updatesvn() again.  If it
> > was failed due to unexpected error, then it should continue to fail,
> > guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
> > unexpected happens".  If it was failed due to running out of entropy, it then
> > may fail again, or it will just succeed and then SGX can continue to work.
> 
> 
> Side topic, the function comment for __sgx_alloc_epc_page() is stale/wrong.  It
> returns ERR_PTR(-ENOMEM), not NULL, on failure.

Right :-)

Thanks for spending time on review!

> 
> /**
>  * __sgx_alloc_epc_page() - Allocate an EPC page
>  *
>  * Iterate through NUMA nodes and reserve ia free EPC page to the caller. Start
>  * from the NUMA node, where the caller is executing.
>  *
>  * Return:
>  * - an EPC page:	A borrowed EPC pages were available.
>  * - NULL:		Out of EPC pages.
>  */

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

[Huang, Kai]

On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > +/* 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;
> >  
> > @@ -444,6 +449,7 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
> >  {
> >  	struct sgx_numa_node *node = &sgx_numa_nodes[nid];
> >  	struct sgx_epc_page *page = NULL;
> > +	bool ret;
> >  
> >  	spin_lock(&node->lock);
> >  
> > @@ -452,12 +458,33 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid)
> >  		return NULL;
> >  	}
> >  
> > +	if (!atomic_long_inc_not_zero(&sgx_nr_used_pages)) {
> 
> FWIW, the entire automatic scheme has terrible behavior when KVM is running SGX
> capable guests.  KVM will allocate EPC when the virtual EPC is first touched by
> the guest, and won't free the EPC pages until the VM is terminated.  And IIRC,
> userspace (QEMU) typically preallocates the virtual EPC to ensure the VM doesn't
> need to be killed later on due to lack of EPC.
> 
> I.e. running an SGX capable VM, even if there are no active enclaves in said VM,
> will prevent SVN updates.
> 
> Unfortunately, I can't think of a sane way around that, because while it would
> be easy enough to force interception of ECREATE, there's no definitive ENCLS leaf
> that KVM can intercept to detect when an enclave is destroyed (interception
> EREMOVE would have terrible performance implications).
> 
> That said, handling this deep in the bowels of EPC page allocation seems
> unnecessary.  The only way for there to be no active EPC pages is if there are
> no enclaves.  As above, virtual EPC usage is already all but guaranteed to hit
> false positives, so I don't think there's anything gained by trying to update
> the SVN based on EPC allocations.
> 
> So rather than react to EPC allocations, why not hook sgx_open() and sgx_vepc_open()?

The only thing I don't like about this is we need to hook both of them.

> Then you're hooking a relative slow path (I assume EUPDATESVN is not fast), 
> 

I was expecting the SGX_NO_UPDATE case should be fast, i.e., some sort of simple
compare and return, but looking at the pseudo code it still re-generates the
CR_BASE_KEY etc.

> error
> codes can be returned to userspace (if you really want the kernel to freak out if
> EUPDATESVN unexpected fails), and you don't _have_ to use a spinlock, e.g. if
> EUPDATESVN takes a long time, using a mutex might be desirable.

Seems reasonable to me.

> 
> > +bool sgx_updatesvn(void)
> > +{
> > +	int retry = 10;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&sgx_epc_eupdatesvn_lock);
> > +
> > +	if (!(cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> > +		return true;
> 
> Checking CPUID features inside the spinlock is asinine.  They can't change, barring
> a misbehaving hypervisor.  This should be a "cache once during boot" sorta thing.

Agreed.

> 
> > +
> > +	/*
> > +	 * Do not execute ENCLS[EUPDATESVN] if running in a VM since
> > +	 * microcode updates are only meaningful to be applied on the host.
> 
> I don't think the kernel should check HYPERVISOR.  The SDM doesn't actually
> say anything about EUPDATESVN, but unless it's explicitly special cased, I doubt
> XuCode cares whether ENCLS was executed in Root vs. Non-Root.

The spec is here:

[1] https://cdrdv2.intel.com/v1/dl/getContent/648682?explicitVersion=true

And from the pseudo code it doesn't distinguish root vs non-root.

> 
> It's the hypervisor's responsibility to enumerate SGX_CPUID_EUPDATESVN or not.
> E.g. if the kernel is running in a "passthrough" type setup, it would be perfectly
> fine to do EUPDATESVN from a guest kernel.  EUPDATESVN could even be proxied,
> e.g. by intercepting ECREATE to track EPC usage

I am open to this HYPERVISOR check, because I don't think we currently support
any kind of "passthrough" setup? And depending on what kinda of "passthrough"
types, the things that the hypervisor traps can vary.

Anyway, I agree it's not necessary to have this check, because currently it is
not possible for a guest to see the EUPDATESVN in CPUID.

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

[Sean Christopherson]

On Tue, Apr 22, 2025, Kai Huang wrote:
> On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> > On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > That said, handling this deep in the bowels of EPC page allocation seems
> > unnecessary.  The only way for there to be no active EPC pages is if there are
> > no enclaves.  As above, virtual EPC usage is already all but guaranteed to hit
> > false positives, so I don't think there's anything gained by trying to update
> > the SVN based on EPC allocations.
> > 
> > So rather than react to EPC allocations, why not hook sgx_open() and sgx_vepc_open()?
> 
> The only thing I don't like about this is we need to hook both of them.

And having to maintain a separate counter.

> > It's the hypervisor's responsibility to enumerate SGX_CPUID_EUPDATESVN or not.
> > E.g. if the kernel is running in a "passthrough" type setup, it would be perfectly
> > fine to do EUPDATESVN from a guest kernel.  EUPDATESVN could even be proxied,
> > e.g. by intercepting ECREATE to track EPC usage
> 
> I am open to this HYPERVISOR check, because I don't think we currently support
> any kind of "passthrough" setup?

Who is "we"?  KVM?  From the SGX driver's perspective, it doesn't know on which
hypervisor it's running, or the intent of that hypervisor.  I.e. whether or not
KVM or any other hypervisor supports something is irrelevant.

> And depending on what kinda of "passthrough" types, the things that the
> hypervisor traps can vary.
> 
> Anyway, I agree it's not necessary to have this check, because currently it is
> not possible for a guest to see the EUPDATESVN in CPUID.

Why is that?

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

[Huang, Kai]

On Tue, 2025-04-22 at 06:54 -0700, Sean Christopherson wrote:
> On Tue, Apr 22, 2025, Kai Huang wrote:
> > On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> > > On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > > That said, handling this deep in the bowels of EPC page allocation seems
> > > unnecessary.  The only way for there to be no active EPC pages is if there are
> > > no enclaves.  As above, virtual EPC usage is already all but guaranteed to hit
> > > false positives, so I don't think there's anything gained by trying to update
> > > the SVN based on EPC allocations.
> > > 
> > > So rather than react to EPC allocations, why not hook sgx_open() and sgx_vepc_open()?
> > 
> > The only thing I don't like about this is we need to hook both of them.
> 
> And having to maintain a separate counter.
> 
> > > It's the hypervisor's responsibility to enumerate SGX_CPUID_EUPDATESVN or not.
> > > E.g. if the kernel is running in a "passthrough" type setup, it would be perfectly
> > > fine to do EUPDATESVN from a guest kernel.  EUPDATESVN could even be proxied,
> > > e.g. by intercepting ECREATE to track EPC usage
> > 
> > I am open to this HYPERVISOR check, because I don't think we currently support
> > any kind of "passthrough" setup?
> 
> Who is "we"?  KVM?  From the SGX driver's perspective, it doesn't know on which
> hypervisor it's running, or the intent of that hypervisor.  I.e. whether or not
> KVM or any other hypervisor supports something is irrelevant.

I was thinking about KVM, but right we cannot assume what will the underneath 
hypervisor do.

> 
> > And depending on what kinda of "passthrough" types, the things that the
> > hypervisor traps can vary.
> > 
> > Anyway, I agree it's not necessary to have this check, because currently it is
> > not possible for a guest to see the EUPDATESVN in CPUID.
> 
> Why is that?
> 

I was thinking about KVM :-( You are right it's up to the hypervisor to decide
whether to advertise EUPDATESVN in CPUID.

Given we don't want to rule out there might be some hypervisor out there which
may just passthrough everything, I agree we should remove this HYPERVISOR check.

Thanks :-)

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

[Reshetova, Elena]

> On Tue, Apr 22, 2025, Kai Huang wrote:
> > On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> > > On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > > That said, handling this deep in the bowels of EPC page allocation seems
> > > unnecessary.  The only way for there to be no active EPC pages is if there
> are
> > > no enclaves.  As above, virtual EPC usage is already all but guaranteed to
> hit
> > > false positives, so I don't think there's anything gained by trying to update
> > > the SVN based on EPC allocations.
> > >
> > > So rather than react to EPC allocations, why not hook sgx_open() and
> sgx_vepc_open()?
> >
> > The only thing I don't like about this is we need to hook both of them.
> 
> And having to maintain a separate counter.

Sorry for the delayed response, I was away for easter holidays. 
Thank you very much for your review, Sean!

I see your point about the issues with SGX-enabled VMs. My overall
understanding was that this would have to be up to the userspace to
prepare the system for EUPDATESVN, which *must* include shutting down
all host enclaves, and also migrating the SGX enabled VMs out of this
host or destroying them (likely the former). Is this an unacceptable assumption?
Because I would rather prefer to keep the kernel code clean and simple
and ask userspace to handle this (note: EUPDATESVN is not a common
action, we expect it to be needed only a couple of times per year max). 

If we follow the approach of trying to execute EUPDATESVN via 
sgx_open() and sgx_vepc_open() paths, it adds more complexity to kernel
code and imo it still doesn’t remove the complexity from userspace
 orchestration sw. I.e. userspace still has to get rid of host enclaves and 
SGX enabled VMs (because syncing with VMs owners to make sure their
encalves are destroyed and these VMs are ready for EUDPATESVN seems
like a big organizational complexity and error prone).
So, the only thing this approach would address would be an EPC
pre-allocation done by qemu? Wouldn't it be more reasonable
(here I am purely speculating, I dont know qemu code) to implement
in qemu the code to drop EPC pre-allocation if no VMs with SGX are
running? That would be a good overall policy imo not to waste EPC
space when not needed in practice. 

Best Regards,
Elena. 

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

[Sean Christopherson]

On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > On Tue, Apr 22, 2025, Kai Huang wrote:
> > > On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> > > > On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > > > That said, handling this deep in the bowels of EPC page allocation seems
> > > > unnecessary.  The only way for there to be no active EPC pages is if
> > > > there are no enclaves.  As above, virtual EPC usage is already all but
> > > > guaranteed to hit false positives, so I don't think there's anything
> > > > gained by trying to update the SVN based on EPC allocations.
> > > >
> > > > So rather than react to EPC allocations, why not hook sgx_open() and
> > sgx_vepc_open()?
> > >
> > > The only thing I don't like about this is we need to hook both of them.
> > 
> > And having to maintain a separate counter.

...

> If we follow the approach of trying to execute EUPDATESVN via 
> sgx_open() and sgx_vepc_open() paths, it adds more complexity to kernel
> code 

This is where I disagree.  I don't see how it's more complex even on the surface,
and when you start considering the implications of "randomly" inserting a non-
trivial operation into EPC allocation, IMO it's far less complex overall.

> and imo it still doesn’t remove the complexity from userspace
> orchestration sw. I.e. userspace still has to get rid of host enclaves and 
> SGX enabled VMs (because syncing with VMs owners to make sure their
> encalves are destroyed and these VMs are ready for EUDPATESVN seems
> like a big organizational complexity and error prone).

Yeah, I don't see a way around that.

> So, the only thing this approach would address would be an EPC
> pre-allocation done by qemu? Wouldn't it be more reasonable
> (here I am purely speculating, I dont know qemu code) to implement
> in qemu the code to drop EPC pre-allocation if no VMs with SGX are
> running? That would be a good overall policy imo not to waste EPC
> space when not needed in practice. 

QEMU only preallocates when the VM is being created, i.e. QEMU doesn't maintain
a persistent pool.  All I was saying is that userspace needs to shut down SGX
capable VMs, even if the VM isn't actively running enclaves, which is a shame.

Untested sketch of hooking create/delete to do SVN updates.

---
 arch/x86/kernel/cpu/sgx/driver.c |  4 ++++
 arch/x86/kernel/cpu/sgx/encl.c   |  2 ++
 arch/x86/kernel/cpu/sgx/main.c   | 34 ++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
 arch/x86/kernel/cpu/sgx/virt.c   |  6 ++++++
 5 files changed, 49 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..669e44d61f9f 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -19,6 +19,10 @@ static int sgx_open(struct inode *inode, struct file *file)
 	struct sgx_encl *encl;
 	int ret;
 
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
+
 	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
 	if (!encl)
 		return -ENOMEM;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 279148e72459..84ca78627e55 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -765,6 +765,8 @@ void sgx_encl_release(struct kref *ref)
 	WARN_ON_ONCE(encl->secs.epc_page);
 
 	kfree(encl);
+
+	sgx_dec_usage_count();
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8ce352fc72ac..ca74c91d4291 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -914,6 +914,40 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+static atomic_t sgx_usage_count;
+static DEFINE_MUTEX(sgx_svn_lock);
+
+static int sgx_update_svn(void)
+{
+	// blah blah blah
+}
+
+int sgx_inc_usage_count(void)
+{
+	if (atomic_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	guard(mutex)(&sgx_svn_lock);
+
+	if (atomic_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	return sgx_update_svn();
+}
+
+void sgx_dec_usage_count(void)
+{
+	if (atomic_dec_return(&sgx_usage_count))
+		return;
+
+	guard(mutex)(&sgx_svn_lock);
+
+	if (atomic_read(&sgx_usage_count))
+		return;
+
+	sgx_update_svn();
+}
+
 static int __init sgx_init(void)
 {
 	int ret;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index d2dad21259a8..f5940393d9bd 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
 }
 #endif
 
+int sgx_inc_usage_count(void);
+void sgx_dec_usage_count(void);
+
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
 #endif /* _X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 7aaa3652e31d..e548de340c2e 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,12 +255,18 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
+	sgx_dec_usage_count();
 	return 0;
 }
 
 static int sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
 
 	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
 	if (!vepc)

base-commit: 37374f38f6c5291ae66ce05f9435c3b519b6f234
-- 

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

[Reshetova, Elena]

> On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > > On Tue, Apr 22, 2025, Kai Huang wrote:
> > > > On Fri, 2025-04-18 at 07:55 -0700, Sean Christopherson wrote:
> > > > > On Tue, Apr 15, 2025, Elena Reshetova wrote:
> > > > > That said, handling this deep in the bowels of EPC page allocation
> seems
> > > > > unnecessary.  The only way for there to be no active EPC pages is if
> > > > > there are no enclaves.  As above, virtual EPC usage is already all but
> > > > > guaranteed to hit false positives, so I don't think there's anything
> > > > > gained by trying to update the SVN based on EPC allocations.
> > > > >
> > > > > So rather than react to EPC allocations, why not hook sgx_open() and
> > > sgx_vepc_open()?
> > > >
> > > > The only thing I don't like about this is we need to hook both of them.
> > >
> > > And having to maintain a separate counter.
> 
> ...
> 
> > If we follow the approach of trying to execute EUPDATESVN via
> > sgx_open() and sgx_vepc_open() paths, it adds more complexity to kernel
> > code
> 
> This is where I disagree.  I don't see how it's more complex even on the
> surface,
> and when you start considering the implications of "randomly" inserting a
> non-
> trivial operation into EPC allocation, IMO it's far less complex overall.

Your code below looks clean enough, so I agree now. I was afraid it would
turn into more complexity.
 
> 
> > and imo it still doesn’t remove the complexity from userspace
> > orchestration sw. I.e. userspace still has to get rid of host enclaves and
> > SGX enabled VMs (because syncing with VMs owners to make sure their
> > encalves are destroyed and these VMs are ready for EUDPATESVN seems
> > like a big organizational complexity and error prone).
> 
> Yeah, I don't see a way around that.
> 
> > So, the only thing this approach would address would be an EPC
> > pre-allocation done by qemu? Wouldn't it be more reasonable
> > (here I am purely speculating, I dont know qemu code) to implement
> > in qemu the code to drop EPC pre-allocation if no VMs with SGX are
> > running? That would be a good overall policy imo not to waste EPC
> > space when not needed in practice.
> 
> QEMU only preallocates when the VM is being created, i.e. QEMU doesn't
> maintain
> a persistent pool.  All I was saying is that userspace needs to shut down SGX
> capable VMs, even if the VM isn't actively running enclaves, which is a shame.

OK, now we are on the same page then. Sorry I misunderstood your comment
about qemu preallocation. 

> 
> Untested sketch of hooking create/delete to do SVN updates.

Thank you very much, I can give this a try. 
Jarkko does this new approach looks good to you on the high level? 

One question though on details, see below inline. 

> 
> ---
>  arch/x86/kernel/cpu/sgx/driver.c |  4 ++++
>  arch/x86/kernel/cpu/sgx/encl.c   |  2 ++
>  arch/x86/kernel/cpu/sgx/main.c   | 34 ++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
>  arch/x86/kernel/cpu/sgx/virt.c   |  6 ++++++
>  5 files changed, 49 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..669e44d61f9f 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,6 +19,10 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	struct sgx_encl *encl;
>  	int ret;
> 
> +	ret = sgx_inc_usage_count();
> +	if (ret)
> +		return ret;
> +
>  	encl = kzalloc(sizeof(*encl), GFP_KERNEL);
>  	if (!encl)
>  		return -ENOMEM;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..84ca78627e55 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,8 @@ void sgx_encl_release(struct kref *ref)
>  	WARN_ON_ONCE(encl->secs.epc_page);
> 
>  	kfree(encl);
> +
> +	sgx_dec_usage_count();
>  }
> 
>  /*
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8ce352fc72ac..ca74c91d4291 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -914,6 +914,40 @@ int sgx_set_attribute(unsigned long
> *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> 
> +static atomic_t sgx_usage_count;
> +static DEFINE_MUTEX(sgx_svn_lock);
> +
> +static int sgx_update_svn(void)
> +{
> +	// blah blah blah
> +}
> +
> +int sgx_inc_usage_count(void)
> +{
> +	if (atomic_inc_not_zero(&sgx_usage_count))
> +		return 0;
> +
> +	guard(mutex)(&sgx_svn_lock);
> +
> +	if (atomic_inc_not_zero(&sgx_usage_count))
> +		return 0;
> +
> +	return sgx_update_svn();
> +}
> +
> +void sgx_dec_usage_count(void)
> +{
> +	if (atomic_dec_return(&sgx_usage_count))
> +		return;
> +
> +	guard(mutex)(&sgx_svn_lock);
> +
> +	if (atomic_read(&sgx_usage_count))
> +		return;
> +
> +	sgx_update_svn();

Why do we want to try to execute this on release also? 
I would think that doing this in sgx_inc_usage_count()
is enough.


Best Regards,
Elena.

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

[Sean Christopherson]

On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > +void sgx_dec_usage_count(void)
> > +{
> > +	if (atomic_dec_return(&sgx_usage_count))
> > +		return;
> > +
> > +	guard(mutex)(&sgx_svn_lock);
> > +
> > +	if (atomic_read(&sgx_usage_count))
> > +		return;
> > +
> > +	sgx_update_svn();
> 
> Why do we want to try to execute this on release also?  I would think that
> doing this in sgx_inc_usage_count() is enough.

I assume an actual SVN update takes some amount of time?

If that's correct, then doing the work upon destroying the last enclave is desirable,
as it's less likely to introduce delay that negatively affects userspace.  Userspace
generally won't care about a 10us delay when destroying a process, but a 10us delay
to launch an enclave could be quite problematic, e.g. in the TDX use case where
enclaves may be launched on-demand in response to a guest attestation request.

If the update time is tiny, then I agree that hooking release would probably do
more harm than good.

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

[Reshetova, Elena]

> On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > > On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > > +void sgx_dec_usage_count(void)
> > > +{
> > > +	if (atomic_dec_return(&sgx_usage_count))
> > > +		return;
> > > +
> > > +	guard(mutex)(&sgx_svn_lock);
> > > +
> > > +	if (atomic_read(&sgx_usage_count))
> > > +		return;
> > > +
> > > +	sgx_update_svn();
> >
> > Why do we want to try to execute this on release also?  I would think that
> > doing this in sgx_inc_usage_count() is enough.
> 
> I assume an actual SVN update takes some amount of time?

Yes, correct, it is not a fast action and can be even slower if we are
running out of entropy and have to retry.

> 
> If that's correct, then doing the work upon destroying the last enclave is
> desirable,
> as it's less likely to introduce delay that negatively affects userspace.
> Userspace
> generally won't care about a 10us delay when destroying a process, but a
> 10us delay
> to launch an enclave could be quite problematic, e.g. in the TDX use case
> where
> enclaves may be launched on-demand in response to a guest attestation
> request.

Ok, but in this case, you are hooking both: create and release. 
I guess your line of thinking is that in most of the cases it will be handled by
a release flow when destroying enclaves, but in cases we happen to need
to update a machine which doesn’t have a single enclave yet, the create flow
will be used. The problem is that if you look at the instruction definition, 
we won't save too much when executing this in release flow (Kai I think already pointed
this out):

IF (Other instruction is accessing EPC) THEN
 RFLAGS.ZF := 1
 RAX := SGX_LOCKFAIL;
 GOTO ERROR_EXIT;
FI
(* Verify EPC is ready *)
IF (the EPC contains any valid pages) THEN
 RFLAGS.ZF := 1;
 RAX := SGX_EPC_NOT_READY;
 GOTO ERROR_EXIT;
FI
(* Refresh paging key *)  
IF (NOT RDSEED(&TMP_KEY, 16)) THEN
 RFLAGS.ZF := 1;
 RAX := SGX_INSUFFICIENT_ENTROPY;
GOTO ERROR_EXIT;
FI
(* Commit *)
CR_BASE_KEY := TMP_KEY;
TMP_CPUSVN := CR_CPUSVN;
(* Update CPUSVN to current minimum patch even if locked *)
(* Determine if info status is needed *)

----------------------------
All above would be done anyhow on create even if we successfully
executed it on release previously (( And then finally we go into:

IF (TMP_CPUSVN = CR_CPUSVN) THEN
 RFLAGS.CF := 1;
 RAX := SGX_NO_UPDATE;
FI
ERROR_EXIT:

> 
> If the update time is tiny, then I agree that hooking release would probably do
> more harm than good.

So, it is not that the time is tiny, it is like we will do it twice, unnecessary
and potentially quite long in both cases (taking RDSEED step into account).

The reason why the instruction is defined this way is that it was not intended originally
to be inserted into some existing SGX flows, but was envisioned to be standalone cmd
for the host orchestrator to execute once it thinks system is ready. 

Best Regards,
Elena.

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

[Sean Christopherson]

On Fri, Apr 25, 2025, Elena Reshetova wrote:
> > On Thu, Apr 24, 2025, Elena Reshetova wrote:
> > Userspace generally won't care about a 10us delay when destroying a
> > process, but a 10us delay to launch an enclave could be quite problematic,
> > e.g. in the TDX use case where enclaves may be launched on-demand in
> > response to a guest attestation request.
> 
> Ok, but in this case, you are hooking both: create and release. 
> I guess your line of thinking is that in most of the cases it will be handled by
> a release flow when destroying enclaves, but in cases we happen to need
> to update a machine which doesn’t have a single enclave yet, the create flow
> will be used. The problem is that if you look at the instruction definition, 
> we won't save too much when executing this in release flow (Kai I think already pointed
> this out):
> 
> IF (Other instruction is accessing EPC) THEN
>  RFLAGS.ZF := 1
>  RAX := SGX_LOCKFAIL;
>  GOTO ERROR_EXIT;
> FI
> (* Verify EPC is ready *)
> IF (the EPC contains any valid pages) THEN
>  RFLAGS.ZF := 1;
>  RAX := SGX_EPC_NOT_READY;
>  GOTO ERROR_EXIT;
> FI
> (* Refresh paging key *)  
> IF (NOT RDSEED(&TMP_KEY, 16)) THEN
>  RFLAGS.ZF := 1;
>  RAX := SGX_INSUFFICIENT_ENTROPY;
> GOTO ERROR_EXIT;
> FI
> (* Commit *)
> CR_BASE_KEY := TMP_KEY;
> TMP_CPUSVN := CR_CPUSVN;
> (* Update CPUSVN to current minimum patch even if locked *)
> (* Determine if info status is needed *)
> 
> ----------------------------
> All above would be done anyhow on create even if we successfully
> executed it on release previously (( And then finally we go into:

Ah, so the slow part happens no matter what.

> IF (TMP_CPUSVN = CR_CPUSVN) THEN
>  RFLAGS.CF := 1;
>  RAX := SGX_NO_UPDATE;
> FI
> ERROR_EXIT:
> 
> > 
> > If the update time is tiny, then I agree that hooking release would probably do
> > more harm than good.
> 
> So, it is not that the time is tiny, it is like we will do it twice,
> unnecessary and potentially quite long in both cases (taking RDSEED step into
> account).
> 
> The reason why the instruction is defined this way is that it was not
> intended originally to be inserted into some existing SGX flows, but was
> envisioned to be standalone cmd for the host orchestrator to execute once it
> thinks system is ready. 

So then why on earth is the kernel implementing automatic updates?  I read back
through most of the cover letters, and IIUC, we went straight from "destroy all
enclaves and force an update" to "blindly try to do EUPDATESVN every time the
number of enclaves goes from 0=>1".  Those are essentially the two most extreme
options.

Even worse, rejecting enclave creation on EUPDATESVN failure represents an ABI
change, i.e. could break existing setups.

Why not simply add an ioctl() or sysfs knob to let userspace trigger EUPDATESVN?
If there are pages in the EPC, return -EBUSY.  That would give userspace full
control of the update policy, and would allow for a super simple implementation
in the kernel.  Userspace should darn well know when it's an appropriate time to
do an SVN update, e.g. after userspace has initiated a ucode update, periodically
if it wants to, after the last TDX VM is destroyed, etc.

Assuming TDX module updates are coming down the pipe, with my KVM maintainer hat
on, that is exactly how I will "request" the module be updated.  Userspace initiates
the update and is therefore responsible for knowing when to do (or not do) an update.
The kernel's job is purely to do the actual update and ensure correctness/safety,
e.g. reject the module update if there are active TDX VMs.

That is also how SEV firmware updates are being implemented.  Userspace is
responsible for stopping any VM types that aren't compatible with conccurent
updates.

I see no reason to do something different for SGX.

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

[Dave Hansen]

On 4/25/25 10:40, Sean Christopherson wrote:
> So then why on earth is the kernel implementing automatic updates?

Because it's literally the least amount of code and doesn't create any
new ABI.

> I read back through most of the cover letters, and IIUC, we went
> straight from "destroy all enclaves and force an update" to "blindly
> try to do EUPDATESVN every time the number of enclaves goes from
> 0=>1".  Those are essentially the two most extreme options.
I'm sure we can think of a bunch more extreme things. How about after
every ENCLS? ;)

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

[Sean Christopherson]

On Fri, Apr 25, 2025, Dave Hansen wrote:
> On 4/25/25 10:40, Sean Christopherson wrote:
> > So then why on earth is the kernel implementing automatic updates?
> 
> Because it's literally the least amount of code

It's literally not.

This series:

 4 files changed, 104 insertions(+), 20 deletions(-)

The below:

 7 files changed, 94 insertions(+), 15 deletions(-)

And that's not counting the extra code need to return something other than -ENOMEM
when EUPDATESVN fails.

> and doesn't create any new ABI.

It doesn't create new uAPI, but it certainly creates new ABI.  The SVN is visible
to both userspace, and when VMs are in play, the guest and the end customer.
Before this, userspace must explicitly reboot for the SVN to change.  After this,
the SVN will change at a completely opaque time.  Userspace can try to coerce
the kernel into performing an update, but it's not guaranteed to happen.

Even worse, rejecting EPC allocation if EUPDATESVN fails risks breaking existing
setups.

I verified writing the param works as expected, but otherwise untested.

--
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 25 Apr 2025 11:56:08 -0700
Subject: [PATCH] x86/sgx: Implement EUPDATESVN via "module" param

Add support for a new SGX ENCLS instruction, EUPDATESVN, which updates
the Security Version Number (SVN) that's capture in attestation reports.
To succeed, EUPDATESVN requires that there be no active pages in any EPC
section, i.e. there cannot be active enclaves.

Give userspace full control over when updates are attempted, as updates
are typically required only after a corresponding microcode update, e.g.
on the order of every few months or so, and EUPDATESVN is quite slow even
when the SVN isn't changing, i.e. triggering updates automatically isn't
desirable as doing so would negatively impact workloads that spawn enclaves
on-demand.

To ensure no EPC pages are active without introducing overhead in EPC
allocation, which is a hot path, count the number of active enclaves or
virtual EPC instances, and only attempt an update if there are no such
users.  Alternatively, the kernel could track the number of EPC pages that
are allocated,

Note, if the kernel or hardware is buggy and leaks or fails to reap an
EPC page, EUPDATESVN will unexpectedly fail.  But there's nothing that
the update flow can do to resolve such problems.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/sgx.h       | 41 +++++++++++++++---------
 arch/x86/kernel/cpu/sgx/driver.c |  1 +
 arch/x86/kernel/cpu/sgx/encl.c   |  2 ++
 arch/x86/kernel/cpu/sgx/encls.h  |  6 ++++
 arch/x86/kernel/cpu/sgx/main.c   | 54 ++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h    |  3 ++
 arch/x86/kernel/cpu/sgx/virt.c   |  2 ++
 7 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..4329187e085f 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 support in CPUID.0x12.0.EAX */
+#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,
 };
 
 /**
@@ -73,6 +76,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 +89,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,
 };
 
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..b2a19dd7388e 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -37,6 +37,7 @@ static int sgx_open(struct inode *inode, struct file *file)
 	}
 
 	file->private_data = encl;
+	sgx_inc_usage_count();
 
 	return 0;
 }
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 279148e72459..84ca78627e55 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -765,6 +765,8 @@ void sgx_encl_release(struct kref *ref)
 	WARN_ON_ONCE(encl->secs.epc_page);
 
 	kfree(encl);
+
+	sgx_dec_usage_count();
 }
 
 /*
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 8ce352fc72ac..e8fcf032e842 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -914,6 +914,59 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+static bool sgx_has_eupdatesvn;
+static atomic_t sgx_usage_count;
+static DECLARE_RWSEM(sgx_svn_lock);
+
+void sgx_inc_usage_count(void)
+{
+	if (atomic_inc_not_zero(&sgx_usage_count))
+		return;
+
+	guard(rwsem_read)(&sgx_svn_lock);
+
+	atomic_inc(&sgx_usage_count);
+}
+
+void sgx_dec_usage_count(void)
+{
+	atomic_dec(&sgx_usage_count);
+}
+
+static int sgx_update_svn(const char *buffer, const struct kernel_param *kp)
+{
+	int r;
+
+	if (!sgx_has_eupdatesvn)
+		return -EOPNOTSUPP;
+
+	guard(rwsem_write)(&sgx_svn_lock);
+
+	if (atomic_read(&sgx_usage_count))
+		return -EBUSY;
+
+	r = __eupdatesvn();
+	switch (r) {
+	case 0:
+	case SGX_NO_UPDATE:
+		return 0;
+	case SGX_INSUFFICIENT_ENTROPY:
+		return -EAGAIN;
+	default:
+		ENCLS_WARN(r, "EUPDATESVN");
+		return -EIO;
+	}
+}
+
+static const struct kernel_param_ops sgx_update_svn_ops = {
+	.set = sgx_update_svn,
+};
+
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "sgx."
+device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0200);
+__MODULE_PARM_TYPE(update_svn, "bool");
+
 static int __init sgx_init(void)
 {
 	int ret;
@@ -947,6 +1000,7 @@ static int __init sgx_init(void)
 	if (sgx_vepc_init() && ret)
 		goto err_provision;
 
+	sgx_has_eupdatesvn = (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN);
 	return 0;
 
 err_provision:
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index d2dad21259a8..4f4ed042f1d6 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -102,6 +102,9 @@ static inline int __init sgx_vepc_init(void)
 }
 #endif
 
+void sgx_inc_usage_count(void);
+void sgx_dec_usage_count(void);
+
 void sgx_update_lepubkeyhash(u64 *lepubkeyhash);
 
 #endif /* _X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 7aaa3652e31d..f57f1270033c 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
 	xa_destroy(&vepc->page_array);
 	kfree(vepc);
 
+	sgx_dec_usage_count();
 	return 0;
 }
 
@@ -269,6 +270,7 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 	xa_init(&vepc->page_array);
 
 	file->private_data = vepc;
+	sgx_inc_usage_count();
 
 	return 0;
 }

base-commit: a33b5a08cbbdd7aadff95f40cbb45ab86841679e
-- 

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

[Dave Hansen]

On 4/25/25 12:29, Sean Christopherson wrote:
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }

->release() is not close(). Userspace doesn't have control over when
release() gets called, so it's a poor thing to say: "wait until all SGX
struct files have been released, then do EUPDATESVN". At least that's
what folks have always told me when I went poking around the VFS.

That alone would make this a non-starter.

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

[Sean Christopherson]

On Fri, Apr 25, 2025, Dave Hansen wrote:
> On 4/25/25 12:29, Sean Christopherson wrote:
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -255,6 +255,7 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
> >  	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >  
> > +	sgx_dec_usage_count();
> >  	return 0;
> >  }
> 
> ->release() is not close(). Userspace doesn't have control over when
> release() gets called, so it's a poor thing to say: "wait until all SGX
> struct files have been released, then do EUPDATESVN". At least that's
> what folks have always told me when I went poking around the VFS.
> 
> That alone would make this a non-starter.

And what frees all the EPC pages?  Hint: it's starts with an 'r'.

Userspace is going to be waiting on ->release() no matter what.  There's no getting
around that, all enclaves and virtual EPC instances must be fully destroyed to
ensure the EPC is empty.

At least with this approach, userspace can know that the EPC is "busy", whereas
with automatic updates, userspace has zero visibility.  The only breadcrumb it
would get is the SVN, which presumably can only be retrieved from within an encave.
But even if userspace can easily read the SVN, unless userspace has a priori
knowledge of the SVN it expects, userspace has no way of knowing if the SVN isn't
changing because no update was required, or if the SVN isn't changing because
the kernel can't find a window where there's no active EPC page.

Exposing -EBUSY to userspace gives it a variety of options.  E.g. retry N times,
with M delay, and then force a reboot if all else fails.

If we wanted to be more explicit, it would be trivial to add a getter, but IMO
that's completely unnecessary, as it's fully redundant with the errno from the
setter.

E.g.

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index e8fcf032e842..4dc95d59c11f 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -958,14 +958,26 @@ static int sgx_update_svn(const char *buffer, const struct kernel_param *kp)
        }
 }
 
+static int sgx_can_update_svn(char *buffer, const struct kernel_param *kp)
+{
+       if (!sgx_has_eupdatesvn)
+               return sysfs_emit(buffer, "unsupported\n");
+
+       if (atomic_read(&sgx_usage_count))
+               return sysfs_emit(buffer, "busy\n");
+
+       return sysfs_emit(buffer, "ready\n");
+}
+
 static const struct kernel_param_ops sgx_update_svn_ops = {
        .set = sgx_update_svn,
+       .get = sgx_can_update_svn,
 };
 
 #undef MODULE_PARAM_PREFIX
 #define MODULE_PARAM_PREFIX "sgx."
-device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0200);
-__MODULE_PARM_TYPE(update_svn, "bool");
+device_param_cb(update_svn, &sgx_update_svn_ops, NULL, 0644);
+__MODULE_PARM_TYPE(update_svn, "int");
 
 static int __init sgx_init(void)
 {

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

[Dave Hansen]

On 4/25/25 14:04, Sean Christopherson wrote:
> Userspace is going to be waiting on ->release() no matter what.

Unless it isn't even involved and it happens automatically.

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

[Sean Christopherson]

On Fri, Apr 25, 2025, Dave Hansen wrote:
> On 4/25/25 14:04, Sean Christopherson wrote:
> > Userspace is going to be waiting on ->release() no matter what.
> 
> Unless it isn't even involved and it happens automatically.

With my Google hat on: no thanks.

Customer:   Hey Google, why haven't you applied security update XYZ?
Support:    We have.
Customer:   The SVN in my attestation report says otherwise.
Support:    Let me check with engineering.
TDX team:   We applied the ucode update provided by platforms.  Platforms, what's up?
Platforms:  That's the right ucode patch.
TDX team:   Hmm, the kernel is supposed to update the SVN.  Let's bug the kernel team.
Me:         Have you guaranteed there are no active enclaves after the update?
TDX team:   Yep.
Me:         <tries to debug the problem, but it's in prod and only happens on
             some platforms>
Me:         Our theory is that enclaves haven't been fully destroyed when the
            hold is lifted.  Try adding a delay?  Maybe 1s?
TDX team:   That helped, but we still have intermittent failures.
Me:         How about 5 seconds?
TDX team:   Great, that worked!
Support:    Sorry for the delay, we're rolling out a fix, you should see the correct
            SVN shortly.
<time passes>
Customer:   Hey Google, my TDX VMs are stalled for 5 seconds during boot.
Support:    Let me check with engineering...


Is that likely to happen?  No.  Is a delay of multiple seconds likely?  Also no.
But it's not that far fetched.  And if something does go sideways, e.g. an EPC
page gets leaked, or enclave FD gets orphaned and left opened, etc., then I would
much, much prefer that the issue be visible to userspace.  Things going sideways
is inevitable; being able to take action when badness happens makes a world of
difference.

Coupled with adding latency to launching the 0=>1 enclave, just to handle something
that happens a few times per year, and I don't see any value in automatic updates.
Maybe it sounds nice on paper, but from my perspective, I see nothing but pain.

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

[Dave Hansen]

On 4/25/25 14:58, Sean Christopherson wrote:
> On Fri, Apr 25, 2025, Dave Hansen wrote:
>> On 4/25/25 14:04, Sean Christopherson wrote:
>>> Userspace is going to be waiting on ->release() no matter what.
>> Unless it isn't even involved and it happens automatically.
> With my Google hat on: no thanks.

I'm completely open to the idea of adding transparency so that folks can
tell when the SVN increments. I'm mostly open to having a new ABI to do
it explicitly in addition to doing it implicitly.]

> Coupled with adding latency to launching the 0=>1 enclave, just to
> handle something that happens a few times per year, and I don't see
> any value in automatic updates. Maybe it sounds nice on paper, but
> from my perspective, I see nothing but pain.
I'm not worried about the latency until I see numbers. If the number
show it's horrible, then we either change course or go yell at the folks
who wrote the xucode.

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

[Reshetova, Elena]

> So then why on earth is the kernel implementing automatic updates?  I read
> back
> through most of the cover letters, and IIUC, we went straight from "destroy all
> enclaves and force an update" to "blindly try to do EUPDATESVN every time
> the
> number of enclaves goes from 0=>1".  Those are essentially the two most
> extreme
> options.
> 
> Even worse, rejecting enclave creation on EUPDATESVN failure represents an
> ABI
> change, i.e. could break existing setups.
> 
> Why not simply add an ioctl() or sysfs knob to let userspace trigger
> EUPDATESVN?

Just for the record, this was my initial proposal on how to do this :)
So, I personally agree with this line of thinking fully. 

Best Regards,
Elena.

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

[Reshetova, Elena]

 
> On 4/25/25 14:58, Sean Christopherson wrote:
> > On Fri, Apr 25, 2025, Dave Hansen wrote:
> >> On 4/25/25 14:04, Sean Christopherson wrote:
> >>> Userspace is going to be waiting on ->release() no matter what.
> >> Unless it isn't even involved and it happens automatically.
> > With my Google hat on: no thanks.
> 
> I'm completely open to the idea of adding transparency so that folks can
> tell when the SVN increments. I'm mostly open to having a new ABI to do
> it explicitly in addition to doing it implicitly.]

Could you please clarify here Dave what ABI do you have in mind? 

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

[Dave Hansen]

On 4/29/25 04:44, Reshetova, Elena wrote:
>> On 4/25/25 14:58, Sean Christopherson wrote:
>>> On Fri, Apr 25, 2025, Dave Hansen wrote:
>>>> On 4/25/25 14:04, Sean Christopherson wrote:
>>>>> Userspace is going to be waiting on ->release() no matter what.
>>>> Unless it isn't even involved and it happens automatically.
>>> With my Google hat on: no thanks.
>> I'm completely open to the idea of adding transparency so that folks can
>> tell when the SVN increments. I'm mostly open to having a new ABI to do
>> it explicitly in addition to doing it implicitly.]
> Could you please clarify here Dave what ABI do you have in mind? 

I should have said I'm open to *eventually* adding features and new ABI
to prod at the SVN state.

Not now.

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

[Reshetova, Elena]

> On 4/29/25 04:44, Reshetova, Elena wrote:
> >> On 4/25/25 14:58, Sean Christopherson wrote:
> >>> On Fri, Apr 25, 2025, Dave Hansen wrote:
> >>>> On 4/25/25 14:04, Sean Christopherson wrote:
> >>>>> Userspace is going to be waiting on ->release() no matter what.
> >>>> Unless it isn't even involved and it happens automatically.
> >>> With my Google hat on: no thanks.
> >> I'm completely open to the idea of adding transparency so that folks can
> >> tell when the SVN increments. I'm mostly open to having a new ABI to do
> >> it explicitly in addition to doing it implicitly.]
> > Could you please clarify here Dave what ABI do you have in mind?
> 
> I should have said I'm open to *eventually* adding features and new ABI
> to prod at the SVN state.
> 
> Not now.
> 

OK, so in what direction should I prepare and send a proper v4?
Here are the options as I understand them:

1. Keep the current approach (with all suggested fixes) to execute EUPDATESVN
during first EPC page creation. 

Proc: Single flow inside EPC page allocation. No new uABI. 
Const: Rejecting EPC allocation if EUPDATESVN fails breaks existing behaviour
(note this can be change to original version of the patch where eupdatesvn
failures are ignored and EPC allocation can proceed normally)
More unpredictability to svn change compared to option 2. 

2. Switch to Sean's approach to execute EUPDATESVN during the sgx_open().
Btw, Sean do you agree that we don't gain much doing it second time during
release() given the microcode flow?
I would rather leave only one invocation of eupdatesvn during sgx_inc_usage_count().

Proc: No new uABI. More predictable on svn change compared to option 1.
Cons: Two explicit paths to hook: sgx_open() and sgx_vepc_open(). 

3. explicit uAPI to execute this

Proc: Allows clear explicit, targeted action to execute eupdatesvn, aligned with
instruction definition. 
Cons: deemed not acceptable to create new uABI for this usecase.  

I am personally learning towards option 2 (but without release flow).
Opinions? Sean? Dave? Jarkko? 

Best Regards,
Elena.

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

[Jarkko Sakkinen]

On Wed, Apr 30, 2025 at 06:53:32AM +0000, Reshetova, Elena wrote:
> 2. Switch to Sean's approach to execute EUPDATESVN during the sgx_open().
> Btw, Sean do you agree that we don't gain much doing it second time during
> release() given the microcode flow?
> I would rather leave only one invocation of eupdatesvn during sgx_inc_usage_count().
> 
> Proc: No new uABI. More predictable on svn change compared to option 1.

> Cons: Two explicit paths to hook: sgx_open() and sgx_vepc_open(). 

Why this is a con?

BR, Jarkko

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

[Reshetova, Elena]

> 
> On Wed, Apr 30, 2025 at 06:53:32AM +0000, Reshetova, Elena wrote:
> > 2. Switch to Sean's approach to execute EUPDATESVN during the
> sgx_open().
> > Btw, Sean do you agree that we don't gain much doing it second time during
> > release() given the microcode flow?
> > I would rather leave only one invocation of eupdatesvn during
> sgx_inc_usage_count().
> >
> > Proc: No new uABI. More predictable on svn change compared to option 1.
> 
> > Cons: Two explicit paths to hook: sgx_open() and sgx_vepc_open().
> 
> Why this is a con?

Well, just from the pov of not having a single path to enable. 
Are you ok with option 2? 

Best Regards,
Elena.

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

[Jarkko Sakkinen]

On Fri, 2025-05-02 at 07:22 +0000, Reshetova, Elena wrote:
> 
> > 
> > On Wed, Apr 30, 2025 at 06:53:32AM +0000, Reshetova, Elena wrote:
> > > 2. Switch to Sean's approach to execute EUPDATESVN during the
> > sgx_open().
> > > Btw, Sean do you agree that we don't gain much doing it second
> > > time during
> > > release() given the microcode flow?
> > > I would rather leave only one invocation of eupdatesvn during
> > sgx_inc_usage_count().
> > > 
> > > Proc: No new uABI. More predictable on svn change compared to
> > > option 1.
> > 
> > > Cons: Two explicit paths to hook: sgx_open() and sgx_vepc_open().
> > 
> > Why this is a con?
> 
> Well, just from the pov of not having a single path to enable. 
> Are you ok with option 2? 
> 

Yep, as SGX is anyway very much run-time managed feature and these
hooks fit better on how it is used.

> Best Regards,
> Elena.

BR, Jarkko

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

[Nataliia Bondarevska]

Tested-by: Nataliia Bondarevska <bondarn@google.com>




On Fri, May 2, 2025 at 1:56 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri, 2025-05-02 at 07:22 +0000, Reshetova, Elena wrote:
> >
> > >
> > > On Wed, Apr 30, 2025 at 06:53:32AM +0000, Reshetova, Elena wrote:
> > > > 2. Switch to Sean's approach to execute EUPDATESVN during the
> > > sgx_open().
> > > > Btw, Sean do you agree that we don't gain much doing it second
> > > > time during
> > > > release() given the microcode flow?
> > > > I would rather leave only one invocation of eupdatesvn during
> > > sgx_inc_usage_count().
> > > >
> > > > Proc: No new uABI. More predictable on svn change compared to
> > > > option 1.
> > >
> > > > Cons: Two explicit paths to hook: sgx_open() and sgx_vepc_open().
> > >
> > > Why this is a con?
> >
> > Well, just from the pov of not having a single path to enable.
> > Are you ok with option 2?
> >
>
> Yep, as SGX is anyway very much run-time managed feature and these
> hooks fit better on how it is used.
>
> > Best Regards,
> > Elena.
>
> BR, Jarkko
>