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