> -----Original Message----- > From: Huang, Kai <kai.hu...@intel.com> > Sent: Thursday, August 7, 2025 2:39 AM > To: Reshetova, Elena <elena.reshet...@intel.com>; Hansen, Dave > <dave.han...@intel.com> > Cc: sea...@google.com; mi...@kernel.org; Scarlata, Vincent R > <vincent.r.scarl...@intel.com>; x...@kernel.org; jar...@kernel.org; > Annapurve, Vishal <vannapu...@google.com>; linux-kernel@vger.kernel.org; > Mallick, Asit K <asit.k.mall...@intel.com>; Aktas, Erdem > <erdemak...@google.com>; Cai, Chong <cho...@google.com>; Bondarevska, > Nataliia <bond...@google.com>; linux-...@vger.kernel.org; Raynor, Scott > <scott.ray...@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.
