Re: [PATCH v39 11/24] x86/sgx: Add SGX enclave driver

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Mon, 05 Oct 2020 07:42:21 -0500, Jarkko Sakkinen  
<jarkko.sakkinen@linux.intel.com> wrote:

> On Mon, Oct 05, 2020 at 11:42:46AM +0200, Greg KH wrote:
>> > > You use gpl-only header files in this file, so how in the world can  
>> it
>> > > be bsd-3 licensed?
>> > >
>> > > Please get your legal department to agree with this, after you  
>> explain
>> > > to them how you are mixing gpl2-only code in with this file.
>> >
>> > I'll do what I already stated that I will do. Should I do something
>> > more?
>>
>> This was written before your previous response.
>
> OK, that is weird, I got this one some time later.
>
>> > > > +	mutex_lock(&encl->lock);
>> > > > +	atomic_or(SGX_ENCL_DEAD, &encl->flags);
>> > >
>> > > So you set a flag that this is dead, and then instantly delete it?   
>> Why
>> > > does that matter?  I see you check for this flag elsewhere, but as  
>> you
>> > > are just about to delete this structure, how can this be an issue?
>> >
>> > It matters because ksgxswapd (sgx_reclaimer_*) might be processing it.
>>
>> I don't see that happening in this patch, did I miss it?
>
> It's implemented in 16/24:
>
> https://lore.kernel.org/linux-sgx/20201004223921.GA48517@linux.intel.com/T/#u
>
>> > It will use the flag to skip the operations that it would do to a  
>> victim
>> > page, when the enclave is still alive.
>>
>> Again, why are you adding flags when the patch does not use them?
>> Please put new functionality in the specific patch that uses it.
>>
>> And can you really rely on this?  How did sgx_reclaimer_* (whatever that
>> is), get the reference on this object in the first place?  Again, I
>> don't see that happening at all in here, and at a quick glance in the
>> other patches I don't see it there either.  What am I missing?
>
> I went through the patch, and yes, they can be migrated to 16/24.
> I agree with this, no excuses.
>
> In 16/24 pages are added to sgx_active_page_list from which they are
> swapped by the reclaimer to the main memory when Enclave Page Cache
> (EPC), the memory where enclave pages reside, gets full.
>
> When a reclaimer thread takes a victim page from that list, it will also
> get a kref to the enclave so that struct sgx_encl instance does not
> get wiped while it's doing its job.
>
>> > Because ksgxswapd needs the alive enclave instance while it is in the
>> > process of swapping a victim page. The reason for this is the
>> > hierarchical nature of the enclave pages.
>> >
>> > As an example, a write operation to main memory, EWB (SDM vol 3D  
>> 40-79)
>>
>> What is that referencing?
>
> https://software.intel.com/content/dam/develop/public/us/en/documents/332831-sdm-vol-3d.pdf
>
>> > needs to access SGX Enclave Control Structure (SECS) page, which is
>> > contains global data for an enclave, like the unswapped child count.
>>
>> Ok, but how did it get access to this structure in the first place, like
>> I ask above?
>
> I guess I answered that, and I also fully agree with your suggestions.
>
> It used to be many iterations ago that enclaves were not file based but
> just memory mappings (long story short: was not great way to make them
> multiprocess, that's why file centered now), and then refcount played a
> bigger role. Having those "extras" in this patch is by no means
> intentional but more like cruft of many iterations of refactoring.
>
> Sometimes when you work long with this kind of pile of code, which has
> converged through many iterations, you really need someone else to point
> some of the simple and obvious things out.
>
>> > There is a patch that adds "sgx/provision".
>>
>> What number in this series?
>
> It's 15/24.
>

Don't know if this is critical. I'd prefer to keep them as is. Directory  
seems natural to me and makes sense to add more under the same dir in case  
there are more to come.

Thanks
Haitao