Re: [PATCH v3 17/17] x86/sgx: Fix pages in the BLOCKED state ending up to the free pool

[Sean Christopherson <sean.j.christopherson@xxxxxxxxx>]

On Wed, Sep 25, 2019 at 03:27:49AM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 18, 2019 at 07:21:20AM +0300, Jarkko Sakkinen wrote:
> > > > +			goto skip;
> > > >  
> > > > +		ret = sgx_encl_get_backing(encl_page->encl,
> > > > +					   SGX_ENCL_PAGE_INDEX(encl_page),
> > > > +					   &backing[i]);
> > > > +		if (ret)
> > > > +			goto skip;
> > > > +
> > > > +		mutex_lock(&encl_page->encl->lock);
> > > > +		encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> > > > +		mutex_unlock(&encl_page->encl->lock);
> > > > +		continue;
> > > > +
> > > > +skip:
> > > 
> > > Eww.  The call to sgx_encl_get_backing() makes it rather ugly no matter
> > > what, but this seems slightly less ugly:
> > > 
> > > 	for (i = 0; i < cnt; i++) {
> > > 		epc_page = chunk[i];
> > > 		encl_page = epc_page->owner;
> > > 
> > > 		if (!sgx_can_reclaim(chunk[i]) ||
> > > 		    sgx_encl_get_backing(encl_page->encl,
> > > 					 SGX_ENCL_PAGE_INDEX(encl_page),
> > > 					 &backing[i]) {
> > > 			kref_put(&encl_page->encl->refcount, sgx_encl_release);
> > > 
> > > 			spin_lock(&sgx_active_page_list_lock);
> > > 			list_add_tail(&epc_page->list, &sgx_active_page_list);
> > > 			spin_unlock(&sgx_active_page_list_lock);
> > > 
> > > 			chunk[i] = NULL;
> > > 			continue;
> > > 		}
> > > 
> > > 		mutex_lock(&encl_page->encl->lock);
> > > 		encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
> > > 		mutex_unlock(&encl_page->encl->lock);
> > > 	}
> > > 
> 
> Well that is one big nested mess where as the version I did is legit use
> of gotos: two conditions that can cause to skip the action. And also
> fairly normal use of gotos with same ideas as with out/err etc. labels
> except now it is used inside a loop..

Yeah, it's the "inside a loop" part that's ugly.  I agree the nested code
is also heinous.

What if we add a helper to split out the verbose check?  Maybe as below,
or move the entire guts to a separate helper?

static int sgx_prepare_to_reclaim(struct sgx_encl_page *encl_page,
				  struct sgx_backing *backing)
{
	if (!sgx_can_reclaim(encl_page))
		return -EBUSY;

	return sgx_encl_get_backing(encl_page->encl,
				    SGX_ENCL_PAGE_INDEX(encl_page), backing);
}

void sgx_reclaim_pages(void)
{
	for (i = 0; i < cnt; i++) {
	epc_page = chunk[i];
	encl_page = epc_page->owner;

	if (!sgx_prepare_to_reclaim(encl_page, &backing[i])) {
		mutex_lock(&encl_page->encl->lock);
		encl_page->desc |= SGX_ENCL_PAGE_RECLAIMED;
		mutex_unlock(&encl_page->encl->lock);
	else {
		kref_put(&encl_page->encl->refcount, sgx_encl_release);

		spin_lock(&sgx_active_page_list_lock);
		list_add_tail(&epc_page->list, &sgx_active_page_list);
		spin_unlock(&sgx_active_page_list_lock);

		chunk[i] = NULL;
	}
}