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

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

[Elena Reshetova]

Changes since v5 following reviews by Ingo, Kai,
Jarkko and Dave:

 - rebase on x86 tip
 - patch 1 is fixed to do correct unwinding in
 case of errors
 - patch 2: add feature flag to cpuid-deps.c
 - patch 3: remove unused SGX_EPC_NOT_READY error code
 - patch 4: fix x86 feature check, return -EAGAIN
 on SGX_INSUFFICIENT_ENTROPY and -EIO on other non-
 expected errors. Comments/style are also fixed.
 - patch 5: rewrite commit message, add comments inline

Changes since v4 following reviews by Dave and Jarkko:

 - breakdown the single patch into 4 patches as
 suggested by Dave
 - fix error unwinding in sgx_(vepc_)open as 
 suggested by Jarkko
 - numerous code improvements suggested by Dave
 - numerous additional code comments and commit
 message improvements as suggested by Dave
 - switch to usage of atomic64_t for sgx_usage_count
 to ensure overflows cannot happen as suggested by Dave
 - do not return a error case when failing with
 SGX_INSUFFICIENT_ENTROPY, fail silently as suggested
 by Dave

Changes since v3 following reviews by Kai and Sean:

 - Change the overall approach to the one suggested
  by Sean and do the EUPDATESVN execution during
  sgx_open() and sgx_vepc_open().
  Note, I do not try to do EUPDATESVN during the release()
  flows since it doesnt save any noticable amount
  of time during next open flow per underlying EUPDATESVN
  microcode logic.
 - In sgx_update_svn() remove the check that we are
  running under VMM and expect the VMM to instead
  expose correct CPUID
 - Move the actual CPUID leaf check out of
  sgx_update_svn() into sgx_init()
 - Use existing RDRAND_RETRY_LOOPS define instead of 10
 - Change the sgx_update_svn() to return 0 only in
 success cases (or if unsupported)
 - Various smaller cosmetic fixes

The still to be discussed question is what sgx_update_svn()
should return in case of various failures. The current version
follows suggestion by Kai and would return an error (and block
sgx_(vepc_)open() in all cases, including running out of entropy.
I think this might be the correct approach for SGX_INSUFFICIENT_ENTROPY
since in such cases userspace can retry the open() and also
will get an info about what is actually blocking the EUPDATEVSN
(and can act on this). However, this is a change in existing API
and therefore debatable and I would like to hear people's feedback.

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.15.0_rc7 & sgx selftests.
Also tested on a Kaby Lake machine without EUPDATESVN support.
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 (5):
  x86/sgx: Introduce a counter to count the sgx_(vepc_)open()
  x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag
  x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]
  x86/sgx: Implement ENCLS[EUPDATESVN]
  x86/sgx: Enable automatic SVN updates for SGX enclaves

 arch/x86/include/asm/cpufeatures.h       |   1 +
 arch/x86/include/asm/sgx.h               |  37 +++++---
 arch/x86/kernel/cpu/cpuid-deps.c         |   1 +
 arch/x86/kernel/cpu/scattered.c          |   1 +
 arch/x86/kernel/cpu/sgx/driver.c         |  22 +++--
 arch/x86/kernel/cpu/sgx/encl.c           |   1 +
 arch/x86/kernel/cpu/sgx/encls.h          |   5 +
 arch/x86/kernel/cpu/sgx/main.c           | 112 +++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h            |   3 +
 arch/x86/kernel/cpu/sgx/virt.c           |  16 +++-
 tools/arch/x86/include/asm/cpufeatures.h |   1 +
 11 files changed, 177 insertions(+), 23 deletions(-)

-- 
2.45.2

[PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Elena Reshetova]

Currently SGX does not have a global counter to count the
active users from userspace or hypervisor. Implement such a counter,
sgx_usage_count. It will be used by the driver when attempting
to call EUPDATESVN SGX instruction.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver.c | 22 ++++++++++++++++------
 arch/x86/kernel/cpu/sgx/encl.c   |  1 +
 arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
 arch/x86/kernel/cpu/sgx/virt.c   | 16 ++++++++++++++--
 5 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..a2994a74bdff 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -19,9 +19,15 @@ 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;
+	if (!encl) {
+		ret = -ENOMEM;
+		goto err_usage_count;
+	}
 
 	kref_init(&encl->refcount);
 	xa_init(&encl->page_array);
@@ -31,14 +37,18 @@ static int sgx_open(struct inode *inode, struct file *file)
 	spin_lock_init(&encl->mm_lock);
 
 	ret = init_srcu_struct(&encl->srcu);
-	if (ret) {
-		kfree(encl);
-		return ret;
-	}
+	if (ret)
+		goto err_encl;
 
 	file->private_data = encl;
 
 	return 0;
+
+err_encl:
+	kfree(encl);
+err_usage_count:
+	sgx_dec_usage_count();
+	return ret;
 }
 
 static int sgx_release(struct inode *inode, struct file *file)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 279148e72459..3b54889ae4a4 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -765,6 +765,7 @@ 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 2de01b379aa3..a018b01b8736 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+/* Counter to count the active SGX users */
+static atomic64_t sgx_usage_count;
+
+int sgx_inc_usage_count(void)
+{
+	atomic64_inc(&sgx_usage_count);
+	return 0;
+}
+
+void sgx_dec_usage_count(void)
+{
+	atomic64_dec(&sgx_usage_count);
+}
+
 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..6ce908ed51c9 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,22 +255,34 @@ 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)
-		return -ENOMEM;
+	if (!vepc) {
+		ret = -ENOMEM;
+		goto err_usage_count;
+	}
 	mutex_init(&vepc->lock);
 	xa_init(&vepc->page_array);
 
 	file->private_data = vepc;
 
 	return 0;
+
+err_usage_count:
+	sgx_dec_usage_count();
+	return ret;
 }
 
 static long sgx_vepc_ioctl(struct file *file,
-- 
2.45.2

[PATCH v6 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Elena Reshetova]

Add a flag indicating whenever ENCLS[EUPDATESVN] SGX instruction
is supported. This will be used by SGX driver to perform CPU
SVN updates.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/cpufeatures.h       | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c         | 1 +
 arch/x86/kernel/cpu/scattered.c          | 1 +
 tools/arch/x86/include/asm/cpufeatures.h | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5b50e0e35129..ee8f0e30ab6c 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -483,6 +483,7 @@
 #define X86_FEATURE_PREFER_YMM		(21*32+ 8) /* Avoid ZMM registers due to downclocking */
 #define X86_FEATURE_APX			(21*32+ 9) /* Advanced Performance Extensions */
 #define X86_FEATURE_INDIRECT_THUNK_ITS	(21*32+10) /* Use thunk for indirect branches in lower half of cacheline */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32+11) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 46efcbd6afa4..3d9f49ad0efd 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -79,6 +79,7 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_SGX_LC,			X86_FEATURE_SGX	      },
 	{ X86_FEATURE_SGX1,			X86_FEATURE_SGX       },
 	{ X86_FEATURE_SGX2,			X86_FEATURE_SGX1      },
+	{ X86_FEATURE_SGX_EUPDATESVN,		X86_FEATURE_SGX1      },
 	{ X86_FEATURE_SGX_EDECCSSA,		X86_FEATURE_SGX1      },
 	{ X86_FEATURE_XFD,			X86_FEATURE_XSAVES    },
 	{ X86_FEATURE_XFD,			X86_FEATURE_XGETBV1   },
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index dbf6d71bdf18..2a29fc33a891 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -42,6 +42,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_PER_THREAD_MBA,		CPUID_ECX,  0, 0x00000010, 3 },
 	{ X86_FEATURE_SGX1,			CPUID_EAX,  0, 0x00000012, 0 },
 	{ X86_FEATURE_SGX2,			CPUID_EAX,  1, 0x00000012, 0 },
+	{ X86_FEATURE_SGX_EUPDATESVN,		CPUID_EAX, 10, 0x00000012, 0 },
 	{ X86_FEATURE_SGX_EDECCSSA,		CPUID_EAX, 11, 0x00000012, 0 },
 	{ X86_FEATURE_HW_PSTATE,		CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,			CPUID_EDX,  9, 0x80000007, 0 },
diff --git a/tools/arch/x86/include/asm/cpufeatures.h b/tools/arch/x86/include/asm/cpufeatures.h
index bc81b9d1aeca..769ee7e411c3 100644
--- a/tools/arch/x86/include/asm/cpufeatures.h
+++ b/tools/arch/x86/include/asm/cpufeatures.h
@@ -481,6 +481,7 @@
 #define X86_FEATURE_AMD_HTR_CORES	(21*32+ 6) /* Heterogeneous Core Topology */
 #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32+ 7) /* Workload Classification */
 #define X86_FEATURE_PREFER_YMM		(21*32+ 8) /* Avoid ZMM registers due to downclocking */
+#define X86_FEATURE_SGX_EUPDATESVN	(21*32+11) /* Support for ENCLS[EUPDATESVN] instruction */
 
 /*
  * BUG word(s)
-- 
2.45.2

[PATCH v6 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Elena Reshetova]

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

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/include/asm/sgx.h | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 6a0069761508..1abf1461fab6 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -28,21 +28,22 @@
 #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
 
 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 +74,10 @@ 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_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
+ *				updated because current SVN was not newer than
+ *				CPUSVN.
  * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
  */
 enum sgx_return_code {
@@ -81,6 +86,8 @@ enum sgx_return_code {
 	SGX_CHILD_PRESENT		= 13,
 	SGX_INVALID_EINITTOKEN		= 16,
 	SGX_PAGE_NOT_MODIFIABLE		= 20,
+	SGX_INSUFFICIENT_ENTROPY	= 29,
+	SGX_NO_UPDATE				= 31,
 	SGX_UNMASKED_EVENT		= 128,
 };
 
-- 
2.45.2

[PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Elena Reshetova]

All running enclaves and cryptographic assets (such as internal SGX
encryption keys) are assumed to be compromised whenever an SGX-related
microcode update occurs. To mitigate this assumed compromise the new
supervisor SGX instruction ENCLS[EUPDATESVN] can generate fresh
cryptographic assets.

Before executing EUPDATESVN, all SGX memory must be marked as unused.
This requirement ensures that no potentially compromised enclave
survives the update and allows the system to safely regenerate
cryptographic assets.

Add the method to perform ENCLS[EUPDATESVN].

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 arch/x86/kernel/cpu/sgx/encls.h |  5 +++
 arch/x86/kernel/cpu/sgx/main.c  | 67 +++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 99004b02e2ed..d9160c89a93d 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
 	return __encls_2(EAUG, pginfo, addr);
 }
 
+/* Attempt to 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 a018b01b8736..109d40c89fe8 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 #include <asm/msr.h>
 #include <asm/sgx.h>
+#include <asm/archrandom.h>
 #include "driver.h"
 #include "encl.h"
 #include "encls.h"
@@ -920,6 +921,72 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
 /* Counter to count the active SGX users */
 static atomic64_t sgx_usage_count;
 
+/**
+ * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
+ * This instruction attempts to update CPUSVN to the
+ * currently loaded microcode update SVN and generate new
+ * cryptographic assets. Must be called when EPC is empty.
+ * Most of the time, there will be no update and that's OK.
+ * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
+ * operation can be safely retried. In other failure cases,
+ * the retry should not be attempted.
+ *
+ * Return:
+ * 0: Success or not supported
+ * -EAGAIN: Can be safely retried, failure is due to lack of
+ *  entropy in RNG.
+ * -EIO: Unexpected error, retries are not advisable.
+ */
+static int sgx_update_svn(void)
+{
+	int ret;
+
+	/*
+	 * If EUPDATESVN is not available, it is ok to
+	 * silently skip it to comply with legacy behavior.
+	 */
+	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
+		return 0;
+
+	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
+		ret = __eupdatesvn();
+
+		/* Stop on success or unexpected errors: */
+		if (ret != SGX_INSUFFICIENT_ENTROPY)
+			break;
+	}
+
+	/*
+	 * SVN was already up-to-date. This is the most
+	 * common case.
+	 */
+	if (ret == SGX_NO_UPDATE)
+		return 0;
+
+	/*
+	 * SVN update failed due to lack of entropy in DRNG.
+	 * Indicate to userspace that it should retry.
+	 */
+	if (ret == SGX_INSUFFICIENT_ENTROPY)
+		return -EAGAIN;
+
+	if (!ret) {
+		/*
+		 * SVN successfully updated.
+		 * Let users know when the update was successful.
+		 */
+		pr_info("SVN updated successfully\n");
+		return 0;
+	}
+
+	/*
+	 * EUPDATESVN was called when EPC is empty, all other error
+	 * codes are unexpected.
+	 */
+	ENCLS_WARN(ret, "EUPDATESVN");
+	return -EIO;
+}
+
 int sgx_inc_usage_count(void)
 {
 	atomic64_inc(&sgx_usage_count);
-- 
2.45.2

[PATCH v6 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Elena Reshetova]

== Background ==

ENCLS[EUPDATESVN] is a new SGX instruction [1] which allows enclave
attestation to include information about updated microcode SVN without a
reboot. Before an EUPDATESVN operation can be successful, all SGX memory
(aka. EPC) must be marked as “unused” in the SGX hardware metadata
(aka.EPCM). This requirement ensures that no compromised enclave can
survive the EUPDATESVN procedure and provides an opportunity to generate
new cryptographic assets.

== Patch Contents ==

Attempt to execute ENCLS[EUPDATESVN] every time the first file descriptor
is obtained via sgx_(vepc_)open(). In the most common case the microcode
SVN is already up-to-date, and the operation succeeds without updating SVN.
If it fails with any other error code than SGX_INSUFFICIENT_ENTROPY, this
is considered unexpected and the *open() returns an error. This should not
happen in practice. On contrary, SGX_INSUFFICIENT_ENTROPY might happen due
to a pressure on the system's DRNG (RDSEED) and therefore the *open() can
be safely retried to allow normal enclave operation.

[1] Runtime Microcode Updates with Intel Software Guard Extensions,
https://cdrdv2.intel.com/v1/dl/getContent/648682

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

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 109d40c89fe8..73ec5ccff3ae 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -920,6 +920,8 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
 /* Counter to count the active SGX users */
 static atomic64_t sgx_usage_count;
+/* Mutex to ensure no concurrent EPC accesses during EUPDATESVN */
+static DEFINE_MUTEX(sgx_svn_lock);
 
 /**
  * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
@@ -989,8 +991,37 @@ static int sgx_update_svn(void)
 
 int sgx_inc_usage_count(void)
 {
-	atomic64_inc(&sgx_usage_count);
-	return 0;
+	int ret;
+
+	/*
+	 * Increments from non-zero indicate potential other
+	 * active EPC users and EUPDATESVN is not attempted.
+	 */
+	if (atomic64_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	/*
+	 * Ensure no other concurrent threads can start
+	 * touching EPC while EUPDATESVN is running.
+	 */
+	guard(mutex)(&sgx_svn_lock);
+
+	if (atomic64_inc_not_zero(&sgx_usage_count))
+		return 0;
+
+	/*
+	 * Attempt to call EUPDATESVN since EPC must be
+	 * empty at this point.
+	 */
+	ret = sgx_update_svn();
+
+	/*
+	 * If EUPDATESVN failed, return failure to sgx_(vepc_)open and
+	 * do not increment the sgx_usage_count.
+	 */
+	if (!ret)
+		atomic64_inc(&sgx_usage_count);
+	return ret;
 }
 
 void sgx_dec_usage_count(void)
-- 
2.45.2

Re: [PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Huang, Kai]

>  
> +/**
> + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].

sgx_updatesvn() -> sgx_update_svn():

arch/x86/kernel/cpu/sgx/main.c:941: warning: expecting prototype for
sgx_updatesvn(). Prototype was for sgx_update_svn() instead


> + * This instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets. Must be called when EPC is empty.
> + * Most of the time, there will be no update and that's OK.
> + * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
> + * operation can be safely retried. In other failure cases,
> + * the retry should not be attempted.
> + *
> + * Return:
> + * 0: Success or not supported
> + * -EAGAIN: Can be safely retried, failure is due to lack of
> + *  entropy in RNG.
> + * -EIO: Unexpected error, retries are not advisable.
> + */
> +static int sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> +		return 0;
> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	/*
> +	 * SVN was already up-to-date. This is the most
> +	 * common case.
> +	 */
> +	if (ret == SGX_NO_UPDATE)
> +		return 0;
> +
> +	/*
> +	 * SVN update failed due to lack of entropy in DRNG.
> +	 * Indicate to userspace that it should retry.
> +	 */
> +	if (ret == SGX_INSUFFICIENT_ENTROPY)
> +		return -EAGAIN;
> +
> +	if (!ret) {
> +		/*
> +		 * SVN successfully updated.
> +		 * Let users know when the update was successful.
> +		 */
> +		pr_info("SVN updated successfully\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * EUPDATESVN was called when EPC is empty, all other error
> +	 * codes are unexpected.
> +	 */
> +	ENCLS_WARN(ret, "EUPDATESVN");
> +	return -EIO;
> +}
> +

This patch alone generates below build warning (both w/ and w/o 'W=1'):

khuang2@khuang2-desk:~/work/enabling/src/tip$ make arch/x86/kernel/cpu/sgx/ W=1
  DESCEND objtool
  CALL    scripts/checksyscalls.sh
  INSTALL libsubcmd_headers
  CC      arch/x86/kernel/cpu/sgx/main.o
arch/x86/kernel/cpu/sgx/main.c:940:12: warning: ‘sgx_update_svn’ defined but not
used [-Wunused-function]
  940 | static int sgx_update_svn(void)
      |            ^~~~~~~~~~~~~~

Regardless of whether this warning is reasonable or not, it is a warning during
build process which may impact bisect.

You can silence it by annotating __maybe_unused attribute to sgx_update_svn() in
this patch, and then remove it in the next one.

But I am not sure whether it is necessary, though.  We can merge the last two
patches together.  The ending patch won't be too big to review IMHO.

We can even merge patch 3 together too.  The reason is current changelog of that
patch doesn't explain why we only define that two error codes (or return values)
but not others, which makes that patch *ALONE* un-reviewable without looking at
further patches.  That being said, it's fine to me we keep patch 3 alone, but
it's better to do some clarification in changelog.

But just my 2 cents.  Since Dave/Ingo/Jarkko are all on this thread, I'll leave
this to them.

Re: [PATCH v6 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Huang, Kai]

On Thu, 2025-05-22 at 12:21 +0300, Elena Reshetova wrote:
> --- a/tools/arch/x86/include/asm/cpufeatures.h
> +++ b/tools/arch/x86/include/asm/cpufeatures.h
> @@ -481,6 +481,7 @@
>  #define X86_FEATURE_AMD_HTR_CORES	(21*32+ 6) /* Heterogeneous Core Topology */
>  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32+ 7) /* Workload Classification */
>  #define X86_FEATURE_PREFER_YMM		(21*32+ 8) /* Avoid ZMM registers due to downclocking */
> +#define X86_FEATURE_SGX_EUPDATESVN	(21*32+11) /* Support for ENCLS[EUPDATESVN] instruction */

[Sorry for not mentioning in the previous version.]

Nit:

I am not sure we need to change tool headers.

Per commit

  f6d9883f8e68 ("tools/include: Sync x86 headers with the kernel sources")

.. and tools/include/uapi/README:

  ...

  What we are doing now is a third option:

   - A software-enforced copy-on-write mechanism of kernel headers to
     tooling, driven by non-fatal warnings on the tooling side build when
     kernel headers get modified:

      Warning: Kernel ABI header differences:
        diff -u tools/include/uapi/drm/i915_drm.h include/uapi/drm/i915_drm.h
        diff -u tools/include/uapi/linux/fs.h include/uapi/linux/fs.h
        diff -u tools/include/uapi/linux/kvm.h include/uapi/linux/kvm.h
        ...

     The tooling policy is to always pick up the kernel side headers as-is,
     and integate them into the tooling build. The warnings above serve as a
     notification to tooling maintainers that there's changes on the kernel
     side.

  We've been using this for many years now, and it might seem hacky, but
  works surprisingly well.

.. I interpret the updating to tools headers is not mandatory (unless building
tools fails w/o the new feature bit definition which I believe isn't the case of
SGX_UPDATESVN).  The tools maintainers will eventually do the sync.

But on the other hand, modifying tools headers in this patch also reduces tools
maintainer's effort in the future.

That being said, I am unclear with the rule here.  Perhaps Dave/Ingo can help to
clarify.

RE: [PATCH v6 2/5] x86/cpufeatures: Add X86_FEATURE_SGX_EUPDATESVN feature flag

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Friday, May 23, 2025 3:18 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: Raynor, Scott <scott.raynor@intel.com>; seanjc@google.com;
> mingo@kernel.org; Scarlata, Vincent R <vincent.r.scarlata@intel.com>;
> x86@kernel.org; jarkko@kernel.org; Annapurve, Vishal
> <vannapurve@google.com>; linux-kernel@vger.kernel.org; Mallick, Asit K
> <asit.k.mallick@intel.com>; Aktas, Erdem <erdemaktas@google.com>; Cai,
> Chong <chongc@google.com>; bondarn@google.com; linux-
> sgx@vger.kernel.org; dionnaglaze@google.com
> Subject: Re: [PATCH v6 2/5] x86/cpufeatures: Add
> X86_FEATURE_SGX_EUPDATESVN feature flag
> 
> On Thu, 2025-05-22 at 12:21 +0300, Elena Reshetova wrote:
> > --- a/tools/arch/x86/include/asm/cpufeatures.h
> > +++ b/tools/arch/x86/include/asm/cpufeatures.h
> > @@ -481,6 +481,7 @@
> >  #define X86_FEATURE_AMD_HTR_CORES	(21*32+ 6) /* Heterogeneous
> Core Topology */
> >  #define X86_FEATURE_AMD_WORKLOAD_CLASS	(21*32+ 7) /*
> Workload Classification */
> >  #define X86_FEATURE_PREFER_YMM		(21*32+ 8) /* Avoid ZMM
> registers due to downclocking */
> > +#define X86_FEATURE_SGX_EUPDATESVN	(21*32+11) /* Support for
> ENCLS[EUPDATESVN] instruction */
> 
> [Sorry for not mentioning in the previous version.]
> 
> Nit:
> 
> I am not sure we need to change tool headers.
> 
> Per commit
> 
>   f6d9883f8e68 ("tools/include: Sync x86 headers with the kernel sources")
> 
> .. and tools/include/uapi/README:
> 
>   ...
> 
>   What we are doing now is a third option:
> 
>    - A software-enforced copy-on-write mechanism of kernel headers to
>      tooling, driven by non-fatal warnings on the tooling side build when
>      kernel headers get modified:
> 
>       Warning: Kernel ABI header differences:
>         diff -u tools/include/uapi/drm/i915_drm.h include/uapi/drm/i915_drm.h
>         diff -u tools/include/uapi/linux/fs.h include/uapi/linux/fs.h
>         diff -u tools/include/uapi/linux/kvm.h include/uapi/linux/kvm.h
>         ...
> 
>      The tooling policy is to always pick up the kernel side headers as-is,
>      and integate them into the tooling build. The warnings above serve as a
>      notification to tooling maintainers that there's changes on the kernel
>      side.
> 
>   We've been using this for many years now, and it might seem hacky, but
>   works surprisingly well.
> 
> .. I interpret the updating to tools headers is not mandatory (unless building
> tools fails w/o the new feature bit definition which I believe isn't the case of
> SGX_UPDATESVN).  The tools maintainers will eventually do the sync.
> 
> But on the other hand, modifying tools headers in this patch also reduces
> tools
> maintainer's effort in the future.
> 
> That being said, I am unclear with the rule here.  Perhaps Dave/Ingo can help
> to
> clarify.


Thank you Kai! I am also not sure what is the rule since I have checked before
and different patches to x86/cpufeatures.c do it differently (some do the
updates to tools and some don't).
I also would like to hear suggestions from Dave and Ingo on this.

Best Regards,
Elena.

Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen]

On Thu, May 22, 2025 at 12:21:34PM +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. Implement such a counter,
> sgx_usage_count. It will be used by the driver when attempting
> to call EUPDATESVN SGX instruction.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver.c | 22 ++++++++++++++++------
>  arch/x86/kernel/cpu/sgx/encl.c   |  1 +
>  arch/x86/kernel/cpu/sgx/main.c   | 14 ++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h    |  3 +++
>  arch/x86/kernel/cpu/sgx/virt.c   | 16 ++++++++++++++--
>  5 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index 7f8d1e11dbee..a2994a74bdff 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -19,9 +19,15 @@ 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;
> +	if (!encl) {
> +		ret = -ENOMEM;
> +		goto err_usage_count;
> +	}
>  
>  	kref_init(&encl->refcount);
>  	xa_init(&encl->page_array);
> @@ -31,14 +37,18 @@ static int sgx_open(struct inode *inode, struct file *file)
>  	spin_lock_init(&encl->mm_lock);
>  
>  	ret = init_srcu_struct(&encl->srcu);
> -	if (ret) {
> -		kfree(encl);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_encl;
>  
>  	file->private_data = encl;
>  
>  	return 0;
> +
> +err_encl:
> +	kfree(encl);
> +err_usage_count:
> +	sgx_dec_usage_count();
> +	return ret;
>  }
>  
>  static int sgx_release(struct inode *inode, struct file *file)
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 279148e72459..3b54889ae4a4 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -765,6 +765,7 @@ 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 2de01b379aa3..a018b01b8736 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
>  }
>  EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  
> +/* Counter to count the active SGX users */
> +static atomic64_t sgx_usage_count;
> +
> +int sgx_inc_usage_count(void)
> +{
> +	atomic64_inc(&sgx_usage_count);
> +	return 0;
> +}

Maybe this was discussed but why this is not just a void-function?

> +
> +void sgx_dec_usage_count(void)
> +{
> +	atomic64_dec(&sgx_usage_count);
> +}

I think these both should be static inlines in arch/x86/kernel/cpu/sgx.h.
Global symbols is over the top. Even if I think disassembly (when doing
debugging, bug hunting or similar tasks), it'd nicer that way.

> +
>  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..6ce908ed51c9 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,22 +255,34 @@ 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)
> -		return -ENOMEM;
> +	if (!vepc) {
> +		ret = -ENOMEM;
> +		goto err_usage_count;
> +	}
>  	mutex_init(&vepc->lock);
>  	xa_init(&vepc->page_array);
>  
>  	file->private_data = vepc;
>  
>  	return 0;
> +
> +err_usage_count:
> +	sgx_dec_usage_count();
> +	return ret;

Right, mirrors here my earlier suggestion for rollback, great (two
iterations ago)!

>  }
>  
>  static long sgx_vepc_ioctl(struct file *file,
> -- 
> 2.45.2
> 
> 

BR, Jrakko

Re: [PATCH v6 3/5] x86/sgx: Define error codes for use by ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Thu, May 22, 2025 at 12:21:36PM +0300, Elena Reshetova wrote:
> Add error codes for ENCLS[EUPDATESVN], then SGX CPUSVN update
> process can know the execution state of EUPDATESVN and notify
> userspace.
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/include/asm/sgx.h | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 6a0069761508..1abf1461fab6 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -28,21 +28,22 @@
>  #define SGX_CPUID_EPC_MASK	GENMASK(3, 0)
>  
>  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 +74,10 @@ 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_NO_UPDATE:		EUPDATESVN was successful, but CPUSVN was not
> + *				updated because current SVN was not newer than
> + *				CPUSVN.
>   * %SGX_UNMASKED_EVENT:		An unmasked event, e.g. INTR, was received
>   */
>  enum sgx_return_code {
> @@ -81,6 +86,8 @@ enum sgx_return_code {
>  	SGX_CHILD_PRESENT		= 13,
>  	SGX_INVALID_EINITTOKEN		= 16,
>  	SGX_PAGE_NOT_MODIFIABLE		= 20,
> +	SGX_INSUFFICIENT_ENTROPY	= 29,
> +	SGX_NO_UPDATE				= 31,
>  	SGX_UNMASKED_EVENT		= 128,
>  };
>  
> -- 
> 2.45.2
> 

Saving reviewed-by up until head of the series looks good to me but
in principle this is as good as it can get ;-)

BR, Jarkko

Re: [PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Thu, May 22, 2025 at 12:21:37PM +0300, Elena Reshetova wrote:
> All running enclaves and cryptographic assets (such as internal SGX
> encryption keys) are assumed to be compromised whenever an SGX-related
> microcode update occurs. To mitigate this assumed compromise the new
> supervisor SGX instruction ENCLS[EUPDATESVN] can generate fresh
> cryptographic assets.
> 
> Before executing EUPDATESVN, all SGX memory must be marked as unused.
> This requirement ensures that no potentially compromised enclave
> survives the update and allows the system to safely regenerate
> cryptographic assets.
> 
> Add the method to perform ENCLS[EUPDATESVN].
> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
>  arch/x86/kernel/cpu/sgx/main.c  | 67 +++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> index 99004b02e2ed..d9160c89a93d 100644
> --- a/arch/x86/kernel/cpu/sgx/encls.h
> +++ b/arch/x86/kernel/cpu/sgx/encls.h
> @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
>  	return __encls_2(EAUG, pginfo, addr);
>  }
>  
> +/* Attempt to 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 a018b01b8736..109d40c89fe8 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -16,6 +16,7 @@
>  #include <linux/vmalloc.h>
>  #include <asm/msr.h>
>  #include <asm/sgx.h>
> +#include <asm/archrandom.h>
>  #include "driver.h"
>  #include "encl.h"
>  #include "encls.h"
> @@ -920,6 +921,72 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  /* Counter to count the active SGX users */
>  static atomic64_t sgx_usage_count;
>  
> +/**
> + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
> + * This instruction attempts to update CPUSVN to the
> + * currently loaded microcode update SVN and generate new
> + * cryptographic assets. Must be called when EPC is empty.
> + * Most of the time, there will be no update and that's OK.
> + * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
> + * operation can be safely retried. In other failure cases,
> + * the retry should not be attempted.
> + *
> + * Return:
> + * 0: Success or not supported
> + * -EAGAIN: Can be safely retried, failure is due to lack of
> + *  entropy in RNG.
> + * -EIO: Unexpected error, retries are not advisable.
> + */
> +static int sgx_update_svn(void)
> +{
> +	int ret;
> +
> +	/*
> +	 * If EUPDATESVN is not available, it is ok to
> +	 * silently skip it to comply with legacy behavior.
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> +		return 0;
> +
> +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> +		ret = __eupdatesvn();
> +
> +		/* Stop on success or unexpected errors: */
> +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> +			break;
> +	}
> +
> +	/*
> +	 * SVN was already up-to-date. This is the most
> +	 * common case.
> +	 */
> +	if (ret == SGX_NO_UPDATE)
> +		return 0;
> +
> +	/*
> +	 * SVN update failed due to lack of entropy in DRNG.
> +	 * Indicate to userspace that it should retry.
> +	 */
> +	if (ret == SGX_INSUFFICIENT_ENTROPY)
> +		return -EAGAIN;
> +
> +	if (!ret) {
> +		/*
> +		 * SVN successfully updated.
> +		 * Let users know when the update was successful.
> +		 */
> +		pr_info("SVN updated successfully\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * EUPDATESVN was called when EPC is empty, all other error
> +	 * codes are unexpected.
> +	 */
> +	ENCLS_WARN(ret, "EUPDATESVN");
> +	return -EIO;
> +}

Even if unlikely() was not used I still don't agree with the order i.e.,
dealing with the success case in the middle. So I stand with my earlier
suggestion, except unlikely() (since that was a problem for David, not
going to fight over it).

BR, Jarkko

Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Dave Hansen]

On 5/23/25 08:54, Jarkko Sakkinen wrote:
>> +void sgx_dec_usage_count(void)
>> +{
>> +	atomic64_dec(&sgx_usage_count);
>> +}
> I think these both should be static inlines in arch/x86/kernel/cpu/sgx.h.
> Global symbols is over the top. Even if I think disassembly (when doing
> debugging, bug hunting or similar tasks), it'd nicer that way.

If they're just used in a single file, make them 'static' and let the
compiler decide whether to inline them or not.

Re: [PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Jarkko Sakkinen]

On Fri, May 23, 2025 at 06:57:50PM +0300, Jarkko Sakkinen wrote:
> On Thu, May 22, 2025 at 12:21:37PM +0300, Elena Reshetova wrote:
> > All running enclaves and cryptographic assets (such as internal SGX
> > encryption keys) are assumed to be compromised whenever an SGX-related
> > microcode update occurs. To mitigate this assumed compromise the new
> > supervisor SGX instruction ENCLS[EUPDATESVN] can generate fresh
> > cryptographic assets.
> > 
> > Before executing EUPDATESVN, all SGX memory must be marked as unused.
> > This requirement ensures that no potentially compromised enclave
> > survives the update and allows the system to safely regenerate
> > cryptographic assets.
> > 
> > Add the method to perform ENCLS[EUPDATESVN].
> > 
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/encls.h |  5 +++
> >  arch/x86/kernel/cpu/sgx/main.c  | 67 +++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
> > index 99004b02e2ed..d9160c89a93d 100644
> > --- a/arch/x86/kernel/cpu/sgx/encls.h
> > +++ b/arch/x86/kernel/cpu/sgx/encls.h
> > @@ -233,4 +233,9 @@ static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
> >  	return __encls_2(EAUG, pginfo, addr);
> >  }
> >  
> > +/* Attempt to 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 a018b01b8736..109d40c89fe8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <asm/msr.h>
> >  #include <asm/sgx.h>
> > +#include <asm/archrandom.h>
> >  #include "driver.h"
> >  #include "encl.h"
> >  #include "encls.h"
> > @@ -920,6 +921,72 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >  /* Counter to count the active SGX users */
> >  static atomic64_t sgx_usage_count;
> >  
> > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
> > + * This instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets. Must be called when EPC is empty.
> > + * Most of the time, there will be no update and that's OK.
> > + * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
> > + * operation can be safely retried. In other failure cases,
> > + * the retry should not be attempted.
> > + *
> > + * Return:
> > + * 0: Success or not supported
> > + * -EAGAIN: Can be safely retried, failure is due to lack of
> > + *  entropy in RNG.
> > + * -EIO: Unexpected error, retries are not advisable.
> > + */
> > +static int sgx_update_svn(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If EUPDATESVN is not available, it is ok to
> > +	 * silently skip it to comply with legacy behavior.
> > +	 */
> > +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> > +		return 0;
> > +
> > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > +		ret = __eupdatesvn();
> > +
> > +		/* Stop on success or unexpected errors: */
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +	}
> > +
> > +	/*
> > +	 * SVN was already up-to-date. This is the most
> > +	 * common case.
> > +	 */
> > +	if (ret == SGX_NO_UPDATE)
> > +		return 0;
> > +
> > +	/*
> > +	 * SVN update failed due to lack of entropy in DRNG.
> > +	 * Indicate to userspace that it should retry.
> > +	 */
> > +	if (ret == SGX_INSUFFICIENT_ENTROPY)
> > +		return -EAGAIN;
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> > +		pr_info("SVN updated successfully\n");
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * EUPDATESVN was called when EPC is empty, all other error
> > +	 * codes are unexpected.
> > +	 */
> > +	ENCLS_WARN(ret, "EUPDATESVN");
> > +	return -EIO;
> > +}
> 
> Even if unlikely() was not used I still don't agree with the order i.e.,
> dealing with the success case in the middle. So I stand with my earlier
> suggestion, except unlikely() (since that was a problem for David, not
> going to fight over it).

Oops s/David/Dave/, sorry.

BR, Jarkko

Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Jarkko Sakkinen]

On Fri, May 23, 2025 at 08:58:54AM -0700, Dave Hansen wrote:
> On 5/23/25 08:54, Jarkko Sakkinen wrote:
> >> +void sgx_dec_usage_count(void)
> >> +{
> >> +	atomic64_dec(&sgx_usage_count);
> >> +}
> > I think these both should be static inlines in arch/x86/kernel/cpu/sgx.h.
> > Global symbols is over the top. Even if I think disassembly (when doing
> > debugging, bug hunting or similar tasks), it'd nicer that way.
> 
> If they're just used in a single file, make them 'static' and let the
> compiler decide whether to inline them or not.

This would be totally fine.

BR, Jarkko

RE: [PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Reshetova, Elena]

> > +	/*
> > +	 * SVN was already up-to-date. This is the most
> > +	 * common case.
> > +	 */
> > +	if (ret == SGX_NO_UPDATE)
> > +		return 0;
> > +
> > +	/*
> > +	 * SVN update failed due to lack of entropy in DRNG.
> > +	 * Indicate to userspace that it should retry.
> > +	 */
> > +	if (ret == SGX_INSUFFICIENT_ENTROPY)
> > +		return -EAGAIN;
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> > +		pr_info("SVN updated successfully\n");
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * EUPDATESVN was called when EPC is empty, all other error
> > +	 * codes are unexpected.
> > +	 */
> > +	ENCLS_WARN(ret, "EUPDATESVN");
> > +	return -EIO;
> > +}
> 
> Even if unlikely() was not used I still don't agree with the order i.e.,
> dealing with the success case in the middle. So I stand with my earlier
> suggestion, except unlikely() (since that was a problem for David, not
> going to fight over it).

I can change the order in the next patch if this is what everyone agrees on.
So, your preference would be to have smth like this:

+	/*
+	 * SVN successfully updated.
+	* Let users know when the update was successful.
+	*/
+	if (!ret) 
+		pr_info("SVN updated successfully\n");
+	
+	if (!ret) || (ret == SGX_NO_UPDATE)
+		return 0;
+
+	/*
+	 * SVN update failed due to lack of entropy in DRNG.
+	 * Indicate to userspace that it should retry.
+	 */
+	if (ret == SGX_INSUFFICIENT_ENTROPY)
+		return -EAGAIN;
+
+	/*
+	 * EUPDATESVN was called when EPC is empty, all other error
+	 * codes are unexpected.
+	 */
+	ENCLS_WARN(ret, "EUPDATESVN");
+	return -EIO;

RE: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Friday, May 23, 2025 6:54 PM
> To: Reshetova, Elena <elena.reshetova@intel.com>
> Cc: Hansen, Dave <dave.hansen@intel.com>; seanjc@google.com; Huang, Kai
> <kai.huang@intel.com>; mingo@kernel.org; linux-sgx@vger.kernel.org; linux-
> kernel@vger.kernel.org; x86@kernel.org; Mallick, Asit K
> <asit.k.mallick@intel.com>; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; Cai, Chong <chongc@google.com>; Aktas,
> Erdem <erdemaktas@google.com>; Annapurve, Vishal
> <vannapurve@google.com>; dionnaglaze@google.com;
> bondarn@google.com; Raynor, Scott <scott.raynor@intel.com>
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()


> >  /*
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> b/arch/x86/kernel/cpu/sgx/main.c
> > index 2de01b379aa3..a018b01b8736 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long
> *allowed_attributes,
> >  }
> >  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >
> > +/* Counter to count the active SGX users */
> > +static atomic64_t sgx_usage_count;
> > +
> > +int sgx_inc_usage_count(void)
> > +{
> > +	atomic64_inc(&sgx_usage_count);
> > +	return 0;
> > +}
> 
> Maybe this was discussed but why this is not just a void-function?

The last patch is cleaner if the prototype is already
returning int here. Also error unwinding takes this into account
right in this patch. Do you have objections to leave it as it is? 

Best Regards,
Elena.

RE: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Saturday, May 24, 2025 2:46 AM
> To: Hansen, Dave <dave.hansen@intel.com>
> Cc: Reshetova, Elena <elena.reshetova@intel.com>; seanjc@google.com;
> Huang, Kai <kai.huang@intel.com>; mingo@kernel.org; linux-
> sgx@vger.kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org; Mallick,
> Asit K <asit.k.mallick@intel.com>; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; Cai, Chong <chongc@google.com>; Aktas,
> Erdem <erdemaktas@google.com>; Annapurve, Vishal
> <vannapurve@google.com>; dionnaglaze@google.com;
> bondarn@google.com; Raynor, Scott <scott.raynor@intel.com>
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()
> 
> On Fri, May 23, 2025 at 08:58:54AM -0700, Dave Hansen wrote:
> > On 5/23/25 08:54, Jarkko Sakkinen wrote:
> > >> +void sgx_dec_usage_count(void)
> > >> +{
> > >> +	atomic64_dec(&sgx_usage_count);
> > >> +}
> > > I think these both should be static inlines in arch/x86/kernel/cpu/sgx.h.
> > > Global symbols is over the top. Even if I think disassembly (when doing
> > > debugging, bug hunting or similar tasks), it'd nicer that way.
> >
> > If they're just used in a single file, make them 'static' and let the
> > compiler decide whether to inline them or not.

But they are not used in single file. They are used in driver.c, encl.c and virt.c
So just to covert to static enough or? 

Best  Regards,
Elena.

Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Huang, Kai]

> 
> 
> > >  /*
> > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > b/arch/x86/kernel/cpu/sgx/main.c
> > > index 2de01b379aa3..a018b01b8736 100644
> > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long
> > *allowed_attributes,
> > >  }
> > >  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> > > 
> > > +/* Counter to count the active SGX users */
> > > +static atomic64_t sgx_usage_count;
> > > +
> > > +int sgx_inc_usage_count(void)
> > > +{
> > > +	atomic64_inc(&sgx_usage_count);
> > > +	return 0;
> > > +}
> > 
> > Maybe this was discussed but why this is not just a void-function?
> 
> The last patch is cleaner if the prototype is already
> returning int here. Also error unwinding takes this into account
> right in this patch. Do you have objections to leave it as it is? 
> 
> 

You can clarify this in the changelog of this patch (which I also suggested in
v5).

RE: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Tuesday, May 27, 2025 2:20 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; jarkko@kernel.org
> Cc: Raynor, Scott <scott.raynor@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>; mingo@kernel.org; Scarlata, Vincent R
> <vincent.r.scarlata@intel.com>; x86@kernel.org; linux-sgx@vger.kernel.org;
> Annapurve, Vishal <vannapurve@google.com>; linux-kernel@vger.kernel.org;
> Mallick, Asit K <asit.k.mallick@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Cai, Chong <chongc@google.com>;
> bondarn@google.com; seanjc@google.com; dionnaglaze@google.com
> Subject: Re: [PATCH v6 1/5] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()
> 
> >
> >
> > > >  /*
> > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c
> > > b/arch/x86/kernel/cpu/sgx/main.c
> > > > index 2de01b379aa3..a018b01b8736 100644
> > > > --- a/arch/x86/kernel/cpu/sgx/main.c
> > > > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > > > @@ -917,6 +917,20 @@ int sgx_set_attribute(unsigned long
> > > *allowed_attributes,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(sgx_set_attribute);
> > > >
> > > > +/* Counter to count the active SGX users */
> > > > +static atomic64_t sgx_usage_count;
> > > > +
> > > > +int sgx_inc_usage_count(void)
> > > > +{
> > > > +	atomic64_inc(&sgx_usage_count);
> > > > +	return 0;
> > > > +}
> > >
> > > Maybe this was discussed but why this is not just a void-function?
> >
> > The last patch is cleaner if the prototype is already
> > returning int here. Also error unwinding takes this into account
> > right in this patch. Do you have objections to leave it as it is?
> >
> >
> 
> You can clarify this in the changelog of this patch (which I also suggested in
> v5).

Sure, will do. The reason I didn’t do it in v6 now is because I actually added
the error handling in case error is returned in this patch, so thought it was
self-explanatory, but as this comment from Jarkko shows I was wrong. 

Best Regards,
Elena.

RE: [PATCH v6 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Reshetova, Elena]

> >
> > +/**
> > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN].
> 
> sgx_updatesvn() -> sgx_update_svn():
> 
> arch/x86/kernel/cpu/sgx/main.c:941: warning: expecting prototype for
> sgx_updatesvn(). Prototype was for sgx_update_svn() instead
> 
> 
> > + * This instruction attempts to update CPUSVN to the
> > + * currently loaded microcode update SVN and generate new
> > + * cryptographic assets. Must be called when EPC is empty.
> > + * Most of the time, there will be no update and that's OK.
> > + * If the failure is due to SGX_INSUFFICIENT_ENTROPY, the
> > + * operation can be safely retried. In other failure cases,
> > + * the retry should not be attempted.
> > + *
> > + * Return:
> > + * 0: Success or not supported
> > + * -EAGAIN: Can be safely retried, failure is due to lack of
> > + *  entropy in RNG.
> > + * -EIO: Unexpected error, retries are not advisable.
> > + */
> > +static int sgx_update_svn(void)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * If EUPDATESVN is not available, it is ok to
> > +	 * silently skip it to comply with legacy behavior.
> > +	 */
> > +	if (!cpu_feature_enabled(X86_FEATURE_SGX_EUPDATESVN))
> > +		return 0;
> > +
> > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > +		ret = __eupdatesvn();
> > +
> > +		/* Stop on success or unexpected errors: */
> > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > +			break;
> > +	}
> > +
> > +	/*
> > +	 * SVN was already up-to-date. This is the most
> > +	 * common case.
> > +	 */
> > +	if (ret == SGX_NO_UPDATE)
> > +		return 0;
> > +
> > +	/*
> > +	 * SVN update failed due to lack of entropy in DRNG.
> > +	 * Indicate to userspace that it should retry.
> > +	 */
> > +	if (ret == SGX_INSUFFICIENT_ENTROPY)
> > +		return -EAGAIN;
> > +
> > +	if (!ret) {
> > +		/*
> > +		 * SVN successfully updated.
> > +		 * Let users know when the update was successful.
> > +		 */
> > +		pr_info("SVN updated successfully\n");
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * EUPDATESVN was called when EPC is empty, all other error
> > +	 * codes are unexpected.
> > +	 */
> > +	ENCLS_WARN(ret, "EUPDATESVN");
> > +	return -EIO;
> > +}
> > +
> 
> This patch alone generates below build warning (both w/ and w/o 'W=1'):
> 
> khuang2@khuang2-desk:~/work/enabling/src/tip$ make
> arch/x86/kernel/cpu/sgx/ W=1
>   DESCEND objtool
>   CALL    scripts/checksyscalls.sh
>   INSTALL libsubcmd_headers
>   CC      arch/x86/kernel/cpu/sgx/main.o
> arch/x86/kernel/cpu/sgx/main.c:940:12: warning: ‘sgx_update_svn’ defined
> but not
> used [-Wunused-function]
>   940 | static int sgx_update_svn(void)
>       |            ^~~~~~~~~~~~~~
> 
> Regardless of whether this warning is reasonable or not, it is a warning during
> build process which may impact bisect.
> 
> You can silence it by annotating __maybe_unused attribute to
> sgx_update_svn() in
> this patch, and then remove it in the next one.
> 
> But I am not sure whether it is necessary, though.  We can merge the last two
> patches together.  The ending patch won't be too big to review IMHO.
> 
> We can even merge patch 3 together too.  The reason is current changelog of
> that
> patch doesn't explain why we only define that two error codes (or return
> values)
> but not others, which makes that patch *ALONE* un-reviewable without
> looking at
> further patches.  That being said, it's fine to me we keep patch 3 alone, but
> it's better to do some clarification in changelog.
> 
> But just my 2 cents.  Since Dave/Ingo/Jarkko are all on this thread, I'll leave
> this to them.

Dave, do you have a strong opinion on this?