Currently SGX does not have a global counter to count the
active users from userspace or hypervisor. Define placeholder
functions sgx_inc/dec_usage_count() that are used to increment
and decrement such a counter. Also, wire the call sites for
these functions. For the latter, in order to introduce the
counting of active sgx users on top of clean functions that
allocate vepc structures, covert existing sgx_(vepc_)open() to
__sgx_(vepc_)open().

The definition of the counter itself and the actual implementation
of these two functions comes next. The counter will be used by
the driver that would be attempting to call EUPDATESVN SGX instruction
only when incrementing from zero.

Note: the sgx_inc_usage_count() prototype is defined to return
int for the cleanliness of the follow-up patches despite always
returning zero in this patch. When the EUPDATESVN SGX instruction
will be enabled in the follow-up patch, the sgx_inc_usage_count()
will start to return the actual return code.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 7f8d1e11dbee..79d6020dfe9c 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -14,7 +14,7 @@ u64 sgx_attributes_reserved_mask;
 u64 sgx_xfrm_reserved_mask = ~0x3;
 u32 sgx_misc_reserved_mask;
 
-static int sgx_open(struct inode *inode, struct file *file)
+static int __sgx_open(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl;
 	int ret;
@@ -41,6 +41,23 @@ static int sgx_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int sgx_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
+
+	ret = __sgx_open(inode, file);
+	if (ret) {
+		sgx_dec_usage_count();
+		return ret;
+	}
+
+	return 0;
+}
+
 static int sgx_release(struct inode *inode, struct file *file)
 {
 	struct sgx_encl *encl = file->private_data;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 308dbbae6c6e..cf149b9f4916 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..3a5cbd1c170e 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -917,6 +917,16 @@ int sgx_set_attribute(unsigned long *allowed_attributes,
 }
 EXPORT_SYMBOL_GPL(sgx_set_attribute);
 
+int sgx_inc_usage_count(void)
+{
+	return 0;
+}
+
+void sgx_dec_usage_count(void)
+{
+	return;
+}
+
 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..b649c0610019 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -255,10 +255,11 @@ 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)
+static int __sgx_vepc_open(struct inode *inode, struct file *file)
 {
 	struct sgx_vepc *vepc;
 
@@ -273,6 +274,23 @@ static int sgx_vepc_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int sgx_vepc_open(struct inode *inode, struct file *file)
+{
+	int ret;
+
+	ret = sgx_inc_usage_count();
+	if (ret)
+		return ret;
+
+	ret =  __sgx_vepc_open(inode, file);
+	if (ret) {
+		sgx_dec_usage_count();
+		return ret;
+	}
+
+	return 0;
+}
+
 static long sgx_vepc_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-- 
2.45.2

(sorry for back-and-forth and not saying those before, but since I found
some issues in this version so I feel I should still point out.)

On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> Currently SGX does not have a global counter to count the
> active users from userspace or hypervisor. 

First, the text wrap of this is not consistent with other lines.  It
breaks too aggressively AFAICT.  I personally set textwidth=72, but I've
seen other people suggesting to wrap at 75 characters.  But this looks too
aggressive and not consistent with  other lines.

I also observed this problem in other patches too.  Could you check all
changelogs and re-wrap the text if needed? 

Back to technical:

"virtual EPC" is also opened from the userspace, so using "hypervisor"
doesn't look quite right to me.

Also, it would be better to mention the "why" first, so people don't need
to find out the reason after reading these "how"s.

How about:

Currently, when SGX is compromised and the microcode update fix is
applied, the machine needs to be rebooted to invalidate old SGX crypto-
assets and make SGX be in an updated safe state.  It's not friendly for
the cloud.

To avoid having to reboot, a new ENCLS[EUPDATESVN] is introduced to update
SGX environment at runtime.  This process needs to be done when there's no
SGX user to make sure no compromised enclaves can survive from the update
and allow the system to regenerate crypto-assets etc.

For now there's no counter to track the active SGX users of host enclave
and virtual EPC.  Introduce such counter mechanism so that the EUPDATESVN
can be done only when there's no SGX users.

> Define placeholder
> functions sgx_inc/dec_usage_count() that are used to increment
> and decrement such a counter. Also, wire the call sites for
> these functions. 
> 

[...]

> For the latter, in order to introduce the
> counting of active sgx users on top of clean functions that
> allocate vepc structures
> 

It's not just "vepc structures" only, right?

Encapsulate the current sgx_(vepc_)open() to __sgx_(vepc_)open() to make
the new sgx_(vepc_)open() easy to read. 

> , covert existing sgx_(vepc_)open() to __sgx_(vepc_)open().
    ^
    convert ?

> 
> The definition of the counter itself and the actual implementation
> of these two functions comes next. 
			 ^
			 come next ?

> The counter will be used by
> the driver that would be attempting to call EUPDATESVN SGX instruction
> only when incrementing from zero.

This can be removed if my paragraphs are used (which already mentioned
this).

> 
> Note: the sgx_inc_usage_count() prototype is defined to return
> int for the cleanliness of the follow-up patches despite always
> returning zero in this patch. When the EUPDATESVN SGX instruction
> will be enabled in the follow-up patch, the sgx_inc_usage_count()
			 ^
			 follow-up patches?

> will start to return the actual return code.

Could this paragraph be shorter, like below?

The EUPDATESVN, which may fail, will be done in sgx_inc_usage_count(). 
Make it return 'int' to make subsequent patches which implement EUPDATESVN
easier to review.  For now it always returns success.


[...]

> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 308dbbae6c6e..cf149b9f4916 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();
>  }
>  
> 

[...]

> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode, struct file *file)
>  	xa_destroy(&vepc->page_array);
>  	kfree(vepc);
>  
> +	sgx_dec_usage_count();
>  	return 0;
>  }

Given we have __sgx_(vepc_)open(), I think it makes more sense to have
__sgx_(encl_|vepc_)release() counterpart?

> -----Original Message-----
> From: Huang, Kai <kai.huang@intel.com>
> Sent: Thursday, August 7, 2025 2:39 AM
> To: Reshetova, Elena <elena.reshetova@intel.com>; Hansen, Dave
> <dave.hansen@intel.com>
> Cc: 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>; Bondarevska,
> Nataliia <bondarn@google.com>; linux-sgx@vger.kernel.org; Raynor, Scott
> <scott.raynor@intel.com>
> Subject: Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the
> sgx_(vepc_)open()
> 
> 
> (sorry for back-and-forth and not saying those before, but since I found
> some issues in this version so I feel I should still point out.)

Thank you Kai for your review! Later is better than never ))

> 
> On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> > Currently SGX does not have a global counter to count the
> > active users from userspace or hypervisor.
> 
> First, the text wrap of this is not consistent with other lines.  It
> breaks too aggressively AFAICT.  I personally set textwidth=72, but I've
> seen other people suggesting to wrap at 75 characters.  But this looks too
> aggressive and not consistent with  other lines.
> 
> I also observed this problem in other patches too.  Could you check all
> changelogs and re-wrap the text if needed?

Ok, I can do the 75 wrap, I think this is considered standard. 

> 
> Back to technical:
> 
> "virtual EPC" is also opened from the userspace, so using "hypervisor"
> doesn't look quite right to me.
> 
> Also, it would be better to mention the "why" first, so people don't need
> to find out the reason after reading these "how"s.
> 
> How about:
> 
> Currently, when SGX is compromised and the microcode update fix is
> applied, the machine needs to be rebooted to invalidate old SGX crypto-
> assets and make SGX be in an updated safe state.  It's not friendly for
> the cloud.
> 
> To avoid having to reboot, a new ENCLS[EUPDATESVN] is introduced to update
> SGX environment at runtime.  This process needs to be done when there's no
> SGX user to make sure no compromised enclaves can survive from the update
> and allow the system to regenerate crypto-assets etc.
> 
> For now there's no counter to track the active SGX users of host enclave
> and virtual EPC.  Introduce such counter mechanism so that the EUPDATESVN
> can be done only when there's no SGX users.


Thank you! I am fine with this description, will use it. 

> 
> > Define placeholder
> > functions sgx_inc/dec_usage_count() that are used to increment
> > and decrement such a counter. Also, wire the call sites for
> > these functions.
> >
> 
> [...]
> 
> > For the latter, in order to introduce the
> > counting of active sgx users on top of clean functions that
> > allocate vepc structures
> >
> 
> It's not just "vepc structures" only, right?
> 
> Encapsulate the current sgx_(vepc_)open() to __sgx_(vepc_)open() to make
> the new sgx_(vepc_)open() easy to read.

Sure, will change to this wording. 

> 
> > , covert existing sgx_(vepc_)open() to __sgx_(vepc_)open().
>     ^
>     convert ?
> 
> >
> > The definition of the counter itself and the actual implementation
> > of these two functions comes next.
> 			 ^
> 			 come next ?

Yes, will fix.

> 
> > The counter will be used by
> > the driver that would be attempting to call EUPDATESVN SGX instruction
> > only when incrementing from zero.
> 
> This can be removed if my paragraphs are used (which already mentioned
> this).

Agree. 

> 
> >
> > Note: the sgx_inc_usage_count() prototype is defined to return
> > int for the cleanliness of the follow-up patches despite always
> > returning zero in this patch. When the EUPDATESVN SGX instruction
> > will be enabled in the follow-up patch, the sgx_inc_usage_count()
> 			 ^
> 			 follow-up patches?
> 
> > will start to return the actual return code.
> 
> Could this paragraph be shorter, like below?
> 
> The EUPDATESVN, which may fail, will be done in sgx_inc_usage_count().
> Make it return 'int' to make subsequent patches which implement
> EUPDATESVN
> easier to review.  For now it always returns success.

Sure, will change. 

> 
> 
> [...]
> 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 308dbbae6c6e..cf149b9f4916 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();
> >  }
> >
> >
> 
> [...]
> 
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode,
> struct file *file)
> >  	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >
> > +	sgx_dec_usage_count();
> >  	return 0;
> >  }
> 
> Given we have __sgx_(vepc_)open(), I think it makes more sense to have
> __sgx_(encl_|vepc_)release() counterpart?

Is it worth it? In case of *_open() variants there are quite error handling
under different cases, but for release as you can see it is just a one-line
addition. Not sure it is worth adding the wrappers just for that. 
But I can change it if people think it would look better this way. 

Best Regards,
Elena.

On Fri, 2025-08-08 at 10:47 +0000, Reshetova, Elena wrote:
> > 
> > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > > index 308dbbae6c6e..cf149b9f4916 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();
> > >   }
> > > 
> > > 
> > 
> > [...]
> > 
> > > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > > @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode,
> > struct file *file)
> > >   	xa_destroy(&vepc->page_array);
> > >   	kfree(vepc);
> > > 
> > > +	sgx_dec_usage_count();
> > >   	return 0;
> > >   }
> > 
> > Given we have __sgx_(vepc_)open(), I think it makes more sense to have
> > __sgx_(encl_|vepc_)release() counterpart?
> 
> Is it worth it? In case of *_open() variants there are quite error handling
> under different cases, but for release as you can see it is just a one-line
> addition. Not sure it is worth adding the wrappers just for that. 
> But I can change it if people think it would look better this way.

Either way is fine to me.  Feel free to ignore.