> > > +static struct mutex virt_epc_lock;
> > > +static struct list_head virt_epc_zombie_pages;
> >
> > What does the lock protect?
>
> Effectively, the list of zombie SECS pages. Not sure why I used a generic name.
>
> > What are zombie pages?
>
> My own terminology for SECS pages whose virtual EPC has been destroyed but can't
> be reclaimed due to them having child EPC pages in other virtual EPCs.
>
> > BTW, if zombies are SECS-only, shouldn't that be in the name rather than
> > "epc"?
>
> I used the virt_epc prefix/namespace to tag it as a global list. I've no
> argument against something like zombie_secs_pages.
I'll change to zombie_secs_pages, and lock name to zombie_secs_pages_lock,
respectively.
[...]
> > > +static int sgx_virt_epc_free_page(struct sgx_epc_page *epc_page)
> > > +{
> > > + int ret;
> > > +
> > > + if (!epc_page)
> > > + return 0;
> >
> > I always worry about these. Why is passing NULL around OK?
>
> I suspect I did it to mimic kfree() behavior. I don't _think_ the radix (now
> xarray) usage will ever encounter a NULL entry.
I'll remove the NULL page check.
>
> >
> > > + ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> > > + if (ret) {
> > > + /*
> > > + * Only SGX_CHILD_PRESENT is expected, which is because of
> > > + * EREMOVE-ing an SECS still with child, in which case it can
> > > + * be handled by EREMOVE-ing the SECS again after all pages in
> > > + * virtual EPC have been EREMOVE-ed. See comments in below in
> > > + * sgx_virt_epc_release().
> > > + */
> > > + WARN_ON_ONCE(ret != SGX_CHILD_PRESENT);
> > > + return ret;
> > > + }
> >
> > I find myself wondering what errors could cause the WARN_ON_ONCE() to be
> > hit. The SDM indicates that it's only:
> >
> > SGX_ENCLAVE_ACT If there are still logical processors executing
> > inside the enclave.
> >
> > Should that be mentioned in the comment?
>
> And faults, which are also spliced into the return value by the ENCLS macros.
> I do remember hitting this WARN when I broke things, though I can't remember
> whether it was a fault or the SGX_ENCLAVE_ACT scenario. Probably the latter?
I'll add a comment saying that there should be no active logical processor
still running inside guest's enclave. We cannot handle SGX_ENCLAVE_ACT here
anyway.
