Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the sgx_(vepc_)open()

["Huang, Kai" <kai.huang@intel.com>]

(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?