Message-ID: <DM8PR11MB5750D373790399E324B98A18E7852@DM8PR11MB5750.namprd11.prod.outlook.com>
Date: Thu, 24 Apr 2025 14:16:21 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, "jarkko@...nel.org"
	<jarkko@...nel.org>
CC: "Huang, Kai" <kai.huang@...el.com>, "Hansen, Dave"
	<dave.hansen@...el.com>, "linux-sgx@...r.kernel.org"
	<linux-sgx@...r.kernel.org>, "Scarlata, Vincent R"
	<vincent.r.scarlata@...el.com>, "x86@...nel.org" <x86@...nel.org>,
	"Annapurve, Vishal" <vannapurve@...gle.com>, "Cai, Chong"
	<chongc@...gle.com>, "Mallick, Asit K" <asit.k.mallick@...el.com>, "Aktas,
 Erdem" <erdemaktas@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "bondarn@...gle.com" <bondarn@...gle.com>,
	"dionnaglaze@...gle.com" <dionnaglaze@...gle.com>, "Raynor, Scott"
	<scott.raynor@...el.com>
Subject: RE: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and
 opportunistically call it during first EPC page alloc

> 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.

Powered by blists - more mailing lists